Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Ceph multiple pools and tests #167

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/fog/libvirt/models/compute/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ After adding the authentication key to a libvirt secret, it can be configured as
```
monitor=mon001.example.com,mon002.example.com,mon003.example.com
port=6789
libvirt_ceph_pool=rbd_pool_name
libvirt_ceph_pools=rbd_pool_name,second_rbd_pool_name
auth_username=libvirt
auth_uuid=uuid_of_libvirt_secret
bus_type=virtio
Expand Down
23 changes: 15 additions & 8 deletions lib/fog/libvirt/models/compute/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -346,13 +346,13 @@

volumes.each_with_index do |volume, index|
target_device = "vd#{('a'..'z').to_a[index]}"
if ceph_args && volume.pool_name.include?(ceph_args["libvirt_ceph_pool"])
if ceph_args && ceph_args["libvirt_ceph_pools"]&.include?(volume.pool_name)
xml.disk(:type => "network", :device => "disk") do
xml.driver(:name => "qemu", :type => volume.format_type, :cache => "writeback", :discard => "unmap")
xml.source(:protocol => "rbd", :name => volume.path)

ceph_args["monitor"]&.split(",")&.each do |monitor|
xml.host(:name => monitor, :port => ceph_args["port"])
xml.source(:protocol => "rbd", :name => volume.path) do
ceph_args["monitor"]&.each do |monitor|
xml.host(:name => monitor, :port => ceph_args["port"])
end
end

xml.auth(:username => ceph_args["auth_username"]) do
Expand Down Expand Up @@ -458,13 +458,20 @@

args = {}

valid_keys = ["monitor", "port", "libvirt_ceph_pools", "auth_username", "auth_uuid", "bus_type"]

Check warning on line 461 in lib/fog/libvirt/models/compute/server.rb

View workflow job for this annotation

GitHub Actions / runner / rubocop

[rubocop] reported by reviewdog 🐶 Use `%w` or `%W` for an array of words. Raw Output: lib/fog/libvirt/models/compute/server.rb:461:24: C: Style/WordArray: Use `%w` or `%W` for an array of words.
array_values = ["monitor", "libvirt_ceph_pools"]

File.readlines(path).each do |line|
pair = line.strip.split("=")
key = pair[0]
value = pair[1]
args[key] = value
key = pair[0].strip
if valid_keys.include?(key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to limit this? Wouldn't it be safe to just parse everything?

value = array_values.include?(key) ? pair[1].split(',').map(&:strip) : pair[1].strip
args[key] = value
end
end

puts args
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like debug code


args
end

Expand Down
60 changes: 60 additions & 0 deletions tests/libvirt/models/compute/server_tests.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,50 @@
RealFile = File
class FakeFile < RealFile
def self.file?(path)
if path == '/etc/foreman/ceph.conf'

Check warning on line 4 in tests/libvirt/models/compute/server_tests.rb

View workflow job for this annotation

GitHub Actions / runner / rubocop

[rubocop] reported by reviewdog 🐶 Use a guard clause (`return true if path == '/etc/foreman/ceph.conf'`) instead of wrapping the code inside a conditional expression. Raw Output: tests/libvirt/models/compute/server_tests.rb:4:5: C: Style/GuardClause: Use a guard clause (`return true if path == '/etc/foreman/ceph.conf'`) instead of wrapping the code inside a conditional expression.
return true

Check warning on line 5 in tests/libvirt/models/compute/server_tests.rb

View workflow job for this annotation

GitHub Actions / runner / rubocop

[rubocop] reported by reviewdog 🐶 Redundant `return` detected. Raw Output: tests/libvirt/models/compute/server_tests.rb:5:7: C: Style/RedundantReturn: Redundant `return` detected.
else
return RealFile.file(path)

Check warning on line 7 in tests/libvirt/models/compute/server_tests.rb

View workflow job for this annotation

GitHub Actions / runner / rubocop

[rubocop] reported by reviewdog 🐶 Redundant `return` detected. Raw Output: tests/libvirt/models/compute/server_tests.rb:7:7: C: Style/RedundantReturn: Redundant `return` detected.
end
Comment on lines +4 to +8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To remove some RuboCop concerns, perhaps reduce it to

Suggested change
if path == '/etc/foreman/ceph.conf'
return true
else
return RealFile.file(path)
end
path == '/etc/foreman/ceph.conf' || RealFile.file(path)

end

def self.readlines(path)
if path == '/etc/foreman/ceph.conf'

Check warning on line 12 in tests/libvirt/models/compute/server_tests.rb

View workflow job for this annotation

GitHub Actions / runner / rubocop

[rubocop] reported by reviewdog 🐶 Use a guard clause (`return RealFile.readlines(path) unless path == '/etc/foreman/ceph.conf'`) instead of wrapping the code inside a conditional expression. Raw Output: tests/libvirt/models/compute/server_tests.rb:12:5: C: Style/GuardClause: Use a guard clause (`return RealFile.readlines(path) unless path == '/etc/foreman/ceph.conf'`) instead of wrapping the code inside a conditional expression.
return [

Check warning on line 13 in tests/libvirt/models/compute/server_tests.rb

View workflow job for this annotation

GitHub Actions / runner / rubocop

[rubocop] reported by reviewdog 🐶 Redundant `return` detected. Raw Output: tests/libvirt/models/compute/server_tests.rb:13:7: C: Style/RedundantReturn: Redundant `return` detected.
"monitor=mon001.example.com,mon002.example.com,mon003.example.com",
"port=6789",
"libvirt_ceph_pools=rbd_pool_name,second_rbd_pool_name",
"auth_username=libvirt",
"auth_uuid=uuid_of_libvirt_secret",
"bus_type=virtio"
]
else
return RealFile.readlines(path)

Check warning on line 22 in tests/libvirt/models/compute/server_tests.rb

View workflow job for this annotation

GitHub Actions / runner / rubocop

[rubocop] reported by reviewdog 🐶 Redundant `return` detected. Raw Output: tests/libvirt/models/compute/server_tests.rb:22:7: C: Style/RedundantReturn: Redundant `return` detected.
end
end
end

Shindo.tests('Fog::Compute[:libvirt] | server model', ['libvirt']) do

servers = Fog::Compute[:libvirt].servers
# Match the mac in dhcp_leases mock
nics = Fog.mock? ? [{ :type => 'network', :network => 'default', :mac => 'aa:bb:cc:dd:ee:ff' }] : nil
server = servers.create(:name => Fog::Mock.random_letters(8), :nics => nics)

before do
Object.class_eval do
remove_const(:File)
const_set(:File, FakeFile)
end
end

after do
Object.class_eval do
remove_const(:File)
const_set(:File, RealFile)
end
end

tests('The server model should') do
tests('have the action') do
test('autostart') { server.respond_to? 'autostart' }
Expand Down Expand Up @@ -89,6 +129,26 @@
xml = server.to_xml
xml.match?(/<disk type="block" device="disk">/) && xml.match?(%r{<source dev="/dev/sda"/>})
end
test("with disk of type ceph") do
server = Fog::Libvirt::Compute::Server.new(
{
:nics => [],
:volumes => [
Fog::Libvirt::Compute::Volume.new({ :path => "rbd_pool_name/block-1", :pool_name => "rbd_pool_name" })
]
}
)

Check warning on line 141 in tests/libvirt/models/compute/server_tests.rb

View workflow job for this annotation

GitHub Actions / runner / rubocop

[rubocop] reported by reviewdog 🐶 Trailing whitespace detected. Raw Output: tests/libvirt/models/compute/server_tests.rb:141:1: C: Layout/TrailingWhitespace: Trailing whitespace detected.
xml = server.to_xml

network_disk = xml.match?(/<disk type="network" device="disk">/)
mon_host = xml.match?(%r{<host name="mon001.example.com" port="6789"/>})
source_rbd = xml.match?(%r{<source protocol="rbd" name="rbd_pool_name/block-1">})
auth_username = xml.match?(/<auth username="libvirt">/)
auth_secret = xml.match?(%r{<secret type="ceph" uuid="uuid_of_libvirt_secret"/>})

network_disk && mon_host && source_rbd && auth_username && auth_secret
end
test("with q35 machine type on x86_64") { server.to_xml.match?(%r{<type arch="x86_64" machine="q35">hvm</type>}) }
end
test("with efi firmware") do
Expand Down
Loading