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.

Sign up for GitHub

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 3 commits into
base: master
Choose a base branch
from
Open

Implement Ceph multiple pools and tests #167

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb6c1cb
Implement Ceph multiple pools and tests
GaetanF Dec 24, 2024
a2a8799
Handle libvirt_ceph_pool as a complement of libvirt_ceph_pools
GaetanF Jan 10, 2025
60db506
Handle libvirt_ceph_pool only if libvirt_ceph_pools not defined
GaetanF Jan 10, 2025
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
View file
Open in desktop
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
26 changes: 18 additions & 8 deletions lib/fog/libvirt/models/compute/server.rb
View file
Open in desktop
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,11 +458,21 @@

args = {}

valid_keys = ["monitor", "port", "libvirt_ceph_pools", "libvirt_ceph_pool", "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?

Copy link
Author

Choose a reason for hiding this comment

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

The configuration file can contains some comments or extra lines. So I think it's better to only import configuration line respecting a minimal schema.

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

if args.has_key?("libvirt_ceph_pool")

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

View workflow job for this annotation

GitHub Actions / runner / rubocop 

[rubocop] reported by reviewdog 🐶
Use `Hash#key?` instead of `Hash#has_key?`.

Raw Output:
lib/fog/libvirt/models/compute/server.rb:473:19: C: Style/PreferredHashMethods: Use `Hash#key?` instead of `Hash#has_key?`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be tempted to only read libvirt_ceph_pool if libvirt_ceph_pools isn't set. The presence of libvirt_ceph_pools IMHO indicates a user has migrated to the newer format. Then only read libvirt_ceph_pool as a fallback. Would you agree?

Copy link
Author

Choose a reason for hiding this comment

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

Okay. I think it's more of a transitive way of managing both at the same time. But in the end, you're right.

args.has_key?("libvirt_ceph_pools") ? args["libvirt_ceph_pools"] << args["libvirt_ceph_pool"] : args["libvirt_ceph_pools"] = [args["libvirt_ceph_pool"]]

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

View workflow job for this annotation

GitHub Actions / runner / rubocop 

[rubocop] reported by reviewdog 🐶
Use `Hash#key?` instead of `Hash#has_key?`.

Raw Output:
lib/fog/libvirt/models/compute/server.rb:474:18: C: Style/PreferredHashMethods: Use `Hash#key?` instead of `Hash#has_key?`.
args.delete("libvirt_ceph_pool")
end

args
Expand Down
59 changes: 59 additions & 0 deletions tests/libvirt/models/compute/server_tests.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,47 @@
RealFile = File
class FakeFile < RealFile
def self.file?(path)
path == '/etc/foreman/ceph.conf' || RealFile.file(path)
end

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

Check warning on line 8 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:8: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 9 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:9: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",
"libvirt_ceph_pool=third_rbd_pool_name",
"auth_username=libvirt",
"auth_uuid=uuid_of_libvirt_secret",
"bus_type=virtio"
]
else
return RealFile.readlines(path)

Check warning on line 19 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:19: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 +126,28 @@
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" }),
Fog::Libvirt::Compute::Volume.new({ :path => "third_rbd_pool_name/block-2", :pool_name => "third_rbd_pool_name" })
]
}
)

Check warning on line 139 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:139: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_block1_rbd = xml.match?(%r{<source protocol="rbd" name="rbd_pool_name/block-1">})
source_block2_rbd = xml.match?(%r{<source protocol="rbd" name="third_rbd_pool_name/block-2">})
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_block1_rbd && source_block2_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