Add creation of RBD volumes and document it.

[olifre]

Fixes #54.

I'm unsure what to make configurable - cache option should likely be made configurable (and could go to ceph.conf), but discard (which works only with bus=scsi) seems like the best default to me.

Also, it seems deletion of volumes created this way fails with fog: https://projects.theforeman.org/issues/12063

[alexjfisher]

I'm interested in this feature, but this PR was created quite a while back now. Could it be revived?

[olifre]

Could it be revived?

What do you mean with "revived"? I'm still here and could rebase, but I did not do anything yet since I received no reply from anybody upstream :cry:

[alexjfisher]

@olifre Hi! Sorry, I'm just a (foreman) user hoping this feature would be accepted.

I guess I should have pinged @plribeiro3000? Thanks.

[alexjfisher]

or maybe @strzibny ?

[plribeiro3000]

LGTM except some details that i don't have proper knowledge.

Is there a scenario where /etc/foreman/ceph.conf exists but vol.pool_name.include? args["libvirt_ceph_pool"] evaluates to false? And what about the vice versa? Is that something we should be worried here?

[olifre]

Is there a scenario where /etc/foreman/ceph.conf exists but vol.pool_name.include? args["libvirt_ceph_pool"] evaluates to false? And what about the vice versa? Is that something we should be worried here?

I think both these cases can only happen on user misconfiguration (e.g. filling /etc/foreman/ceph.conf with a broken pool name). So I do not think we need to worry about it here.

[plribeiro3000]

@olifre In case the user missconfigure, which error he is gonna face?

My worry is that the error might not point to the right spot. If a simple condition might give a better output in case of error i believe it worth.

Can we add something to check this? Or even use the same condition in both lines?

[olifre]

@plribeiro3000 Rethinking about the two possibilities:

1. The first possibility would tbe that the user creates a volume in a pool which does not have configuration in /etc/foreman/ceph.conf. We have no way to distinguish whether that's meant to be a Ceph pool, or maybe in an LVM-based pool at this point, for example (we use the user configuration, i.e. the file to decide). So it's good that vol.pool_name.include? args["libvirt_ceph_pool"] evaluates to false even if /etc/foreman/ceph.conf is present since this may be what the user wants - the user might not be using Ceph exclusively. In other words: It would not be bad to generate an error, since it would mean that if the file exists, the user can only use Ceph RBD exclusively and any use of other pools would be turned off.
2. I'm not really sure when the vice-versa case should happen. The expression can never really evaluate to true if args is not filled from the file - right?

[plribeiro3000]

@olifre I guess my concern would be to evaluate the same condition in both scenarios as it seems the 2 code blocks are co dependents.

Maybe we can have an extra conditional one of the blocks that evaluates if one of those 2 scenarios are happening and Log a message specifying it. WDYT?

[olifre]

@plribeiro3000 What exactly do you mean with "co-dependents"?

Maybe we can have an extra conditional one of the blocks that evaluates if one of those 2 scenarios are happening and Log a message specifying it. WDYT?

I don't think it's a good idea to log something in case of wanted behaviour, since this may irritate the user. Which kind of log message would you propose? And how exactly would the second scenario be triggered?

[strzibny]

Shouldn't there be a nicer opt-in than just reading /etc/foreman/ceph.conf file? I mean other features have attribute in Compute::Server to determine the feature. Perhaps if there is an attribute with the ceph configuration then we know the user is interested in this.

[olifre]

It seemed the most straightforward solution to me as a Ceph user, since the format of the ceph configuration file is similar, and needs to be maintained by any ceph user anyways (at least for the list of mons) - and also the libvirt-secret has to be maintained outside of libfog.

[plribeiro3000]

@olifre My understanding is that the 2 blocks need to work together for this feature to succeed. But on each of those there is a different check which may lead to confusion if he user is not an expert with CEPH.

The best approach here would be to either have a configuration option like @strzibny suggested or a way to inform the user of the missconfiguration.

IMHO this code is not good because most of the users do not have proper knowledge of the tools and this kind of implementation does not help besides add more complexity to it.

[plribeiro3000]

@plribeiro3000 What exactly do you mean with "co-dependents"?

I mean that the 2 blocks need to be executed for this feature to work, it can't be just one or another but rather both.

[olifre]

My understanding is that the 2 blocks need to work together for this feature to succeed.

Yes and no. The first block parses the config file. The second block is executed iff the user creates a volume inside a Ceph RBD pool - and must not be executed if the volume is created in a different pool. So both need different conditionals, and it's impossible to see if there is a misconfiguration algorithmically since the user may have a Ceph RBD pool and a non-Ceph pool, and may use both. So even if the configuration is there, there is a valid usecase to not execute the second part for some volumes, in case the VM which is created has it's volumes (or only some volumes) in a different pool.

But on each of those there is a different check which may lead to confusion if he user is not an expert with CEPH.

There is no Ceph knowledge needed here apart from the documentation part which I also added in this PR. To rephrase the conditional:

• The config file is always read and - in a way - global configuration.
• The second part (volume creation) depends on which pool the volume is created in. If it's a Ceph RBD pool, the newly added code should be executed, if it's in a different pool, it should not be executed, and that's not an error.

The best approach here would be to either have a configuration option like @strzibny suggested or a way to inform the user of the missconfiguration.

I don't understand how to detect the actual misconfiguration case - can you point it out to me?

I also do not understand how a configuration option would help - having the configuration file is something globally needed, while the actual template generation depends on which pool the volume is created in, which may differ for each single volume if the user creates the volumes in different pools. Where would this global configuration option be, how should a tool like Foreman interface with it (there's no global libfog configuration in there as far as I can see), and how should the user maintain it? Adding a different kind of configuration in addition to the Ceph configuration any Ceph user has to maintain is an extra complication in my opinion. @alexjfisher , how would you expect configuration to be?

[plribeiro3000]

@olifre As i mentioned before i do now know anything about CEPH to discuss the actual process of it so i'm trying to make sense out of it.

I do understand what you are saying about being an user miss configuration but as i see there is still some improvement we can do here to help out newcomers.

The way i see the conditional should be equal in both statements or at least make it in a way that the system will tell the user if something unexpected happens (Like configuring a ceph file for a pool where no volume is CEPH).

But i wont block this since i'm not an active contributor. If @strzibny thinks its good im ok with it.

[olifre]

The way i see the conditional should be equal in both statements or at least make it in a way that the system will tell the user if something unexpected happens (Like configuring a ceph file for a pool where no volume is CEPH).

I think you did not get the point I tried to make (or I did not get your point): If the condition would be the same, you would force the user to only ever create Ceph RBD volumes, breaking the existing use case of creating non-RBD libvirt volumes in a different pool. As the code is now:

• The config file is always read (if it exists).
• Depending on the libvirt pool the volume is created in, it will either be configured as a Ceph RBD volume (using the new code branch) or as a "classic" volume (existing code).

The user actively has to create a config file containing libvirt_ceph_pool=name_of_the_libvirt_pool_holding_ceph_rbd_volumes as I documented, but even if the user does that, volumes are still allowed to be created in other pools (which is an existing use case). So I don't see a chance for misconfiguration.

[plribeiro3000]

Agree that we are not on the same page. Communication is hard. 😅

I understand that there are other possibilities here but i would like to make it a little bit better for a scenario where the user wants to use CEPH and does create a file but no volume is CEPH or he select volumes that are CEPH but does not create the configuration file.

If if vol.pool_name.include? args["libvirt_ceph_pool"] evaluates to false you are executing the old code so it is not true that if the conditional is the same the user wont be able to create other non-RBD libvirt volumes in a different pool.

I would change this it to check both states at least.

<% if File.file?('/etc/foreman/ceph.conf') && vol.pool_name.include?(args["libvirt_ceph_pool"]) %>
  <disk type='network' device='disk'>
    <driver name='qemu' type='<%= vol.format_type %>' cache='writeback' discard='unmap'/>
    <source protocol='rbd' name='<%= vol.path %>'>
      <% args["monitor"].split(",").each do |mon| %>
        <host name='<%= mon %>' port='<%= args["port"] %>'/>
      <% end %>
    </source>
    <auth username='<%= args["auth_username"] %>'>
      <secret type='ceph' uuid='<%= args["auth_uuid"] %>'/>
    </auth>
    <target dev='sd<%= ('a'..'z').to_a[volumes.index(vol)] %>' bus='scsi'/>
  </disk>
<% else %>
  <disk type='file' device='disk'>
    <driver name='qemu' type='<%= vol.format_type %>'/>
    <source file='<%= vol.path %>'/>
    <%# we need to ensure a unique target dev -%>
    <target dev='vd<%= ('a'..'z').to_a[volumes.index(vol)] %>' bus='virtio'/>
  </disk>
<% end %>

This condition makes a clearer statement of the condition necessary to execute the block of code:

If i have a configuration file AND this volume is in a CEPH pool do this otherwise do the usual

[olifre]

Agree that we are not on the same page. Communication is hard. sweat_smile

Indeed - only now I get what you meant :wink:. Yes, that is indeed more readable - behaviour will be the same, since args will in any case not be filled if the file does not exist, but the code shows the intended behaviour better (the performance loss of an addtional stat syscall is reasonable I think). Will change, thanks!

[plribeiro3000]

I'm glad we figured it out.

Thank you for bear with me as we get to understand each other. 🎉

[plribeiro3000]

GTG for me!

[olifre]

@plribeiro3000 Thanks, also for bearing with me until we pulled this communication hurdle down :smile:.

[Bluewind]

We'd love to use this as well. What's blocking this from getting merged?

[olifre]

@strzibny I believe the commit I added just now addresses all of the suggestions by @Bluewind , and can confirm it creates the XML files correctly (while I can't test the usage attribute myself in my environment, it matches documentation and the tests by @Bluewind ).

[strzibny]

I am unsure about coupling RBD feature with Foreman by hardcoding /etc/foreman/ceph.conf path.

The official documentation talks about the following locations (https://docs.ceph.com/docs/jewel/rados/configuration/ceph-conf/):

The default Ceph configuration file locations in sequential order include:

    $CEPH_CONF (i.e., the path following the $CEPH_CONF environment variable)
    -c path/path (i.e., the -c command line argument)
    /etc/ceph/ceph.conf
    ~/.ceph/config
    ./ceph.conf (i.e., in the current working directory)

I am not a Ceph guy, mind you. I am just suspicious about this path being the only option.

[olifre]

@strzibny The ceph configuration file has different syntax (and actually, in more recent ceph versions, it is going away). The config file used here only contains the minimum information needed to access ceph as a client, and additional information about the necessary libvirt secret to use. So this is a different configuration file than the ceph.conf from the Ceph documentation and is, in fact, specific to this usecase.

Also, in common setups, Foreman will run on a node without direct access to Ceph (in our case, there's nothing from Ceph installed on the node) - but the configuration is still needed to pass it on to remote libvirt nodes upon creation of new VMs.

[strzibny]

In that case I think it's okay. If there would be someone requiring RBD in different setups, we can revisit it.

[strzibny]

@olifre can I ask you about https://projects.theforeman.org/issues/12063 which you raised as a concern in the beginning?

[olifre]

@strzibny This issue still persists. I am unsure about the correct fix - in principle, one could change: https://github.com/fog/fog-libvirt/blob/ca9298336a26e96ba3870f2e01ba4d294f9e9f11/lib/fog/libvirt/requests/compute/list_domains.rb#L41-L43 to fall back to the name attribute in case the file attribute does not exist, but I did not yet have the time to test this out. However, this affects any existing VMs not based on files (not only RBD volumes).

[olifre]

@strzibny I have finally investigated this more in-depth and using the following code instead:

        def domain_volumes xml
          vols_by_file = xml_elements(xml, "domain/devices/disk/source", "file")
          vols_by_name = xml_elements(xml, "domain/devices/disk/source", "name")
          vols = []
          vols_by_file.zip(vols_by_name).each do |by_file,by_name|
              vols.push(by_file.nil? ? by_name : by_file)
          end
          vols
        end

fixes it for any volume which is not file-based, but has a unique name (which, I think, is everything) and should not break existing cases. Would you like to see this as part of this PR, or separately?

[olifre]

@strzibny Let me know which approach you prefer, and I'll take it :wink:.

[strzibny]

@olifre cool. Can you add here as a new commit perhaps?

[olifre]

@strzibny Done :smile:. Btw if you have a suggestion on how to make this ruby snippet more efficient, let me know, I'm not a ruby expert, but it works well in all my tests.

[olifre]

@strzibny Just a friendly ping: Do you see anything still missing here?

[strzibny]

I am going to merge this and do a release soon, thanks for your contributions everyone.