Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
26 changes: 18 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 @@ def to_xml

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 @@ def read_ceph_args(path = "/etc/foreman/ceph.conf")

args = {}

valid_keys = ["monitor", "port", "libvirt_ceph_pools", "libvirt_ceph_pool", "auth_username", "auth_uuid", "bus_type"]
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
Copy Markdown
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
Copy Markdown
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")
Copy link
Copy Markdown
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
Copy Markdown
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"]]
args.delete("libvirt_ceph_pool")
end

args
Expand Down
59 changes: 59 additions & 0 deletions tests/libvirt/models/compute/server_tests.rb
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'
return [
"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)
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" })
]
}
)

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