Conversation
| 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_pool"]&.split(",")&.include?(volume.pool_name) |
There was a problem hiding this comment.
I'd prefer to fix this up in read_ceph_args so you don't do this in a loop. Same below for the monitor. Perhaps also add in .map(&:strip) so you can also use spaces between the values while you're at it.
Another thing is that libvirt_ceph_pools suggests it's singular while so perhaps store the result of that as libvirt_ceph_pools to indicate it's an array?
There was a problem hiding this comment.
Ok ! I have updated the branch to move the split and verify keys in the read_ceph_args.
| end | ||
|
|
||
| xml.target(:dev => target_device, :bus => args["bus_type"] == "virtio" ? "virtio" : "scsi") | ||
| xml.target(:dev => target_device, :bus => ceph_args["bus_type"] == "virtio" ? "virtio" : "scsi") |
There was a problem hiding this comment.
I merged this separate so it properly closed the issue. Doing 2 different things (fixing a bug and adding a new feature) in the same commit is something I prefer to avoid.
There was a problem hiding this comment.
Ok, I haven't seen the fix when I have started to work on it. I will rebase the merge on it.
GaetanF
force-pushed
the
feature/handle_multiple_ceph_pools
branch
from
51d0a00 to
cb6c1cb
Compare
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
I think it would be good to have some fallback code for compatibility. If libvirt_ceph_pools isn't present, check for libvirt_ceph_pool and present it as libvirt_ceph_pools in ceph_args. I'm not sure if there's a deprecation mechanism, but it would be good to consider that too.
| end | ||
| end | ||
|
|
||
| puts args |
| value = pair[1] | ||
| args[key] = value | ||
| key = pair[0].strip | ||
| if valid_keys.include?(key) |
There was a problem hiding this comment.
Is there a reason to limit this? Wouldn't it be safe to just parse everything?
There was a problem hiding this comment.
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.
| if path == '/etc/foreman/ceph.conf' | ||
| return true | ||
| else | ||
| return RealFile.file(path) | ||
| end |
There was a problem hiding this comment.
To remove some RuboCop concerns, perhaps reduce it to
| if path == '/etc/foreman/ceph.conf' | |
| return true | |
| else | |
| return RealFile.file(path) | |
| end | |
| path == '/etc/foreman/ceph.conf' || RealFile.file(path) |
There was a problem hiding this comment.
Nice ! I will change that. I'm more used to developing in Python than in Ruby.
| end | ||
|
|
||
| puts args | ||
| if args.has_key?("libvirt_ceph_pool") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Okay. I think it's more of a transitive way of managing both at the same time. But in the end, you're right.
2 participants
Hello,
Following the issue #162, I have implemented the capability to handle multiple Ceph RBD Pool by using a comma-separated list on the ceph.conf file.
Regarding the Libvirt documentation, the host tag must be in the source tag and not after. I have fix them also.
I have also implemented the tests to handle the Ceph part by using a mock of the File object.
Feel free if you have questions