Support older libvirt address retrieval

[electrofelix]

Libvirt releases before 1.2.8 do not fully support the virNetworkGetDHCPLeases used by dhcp_leases, so provide a compatibility check to determine the version of the library in order to determine which method to use to retrieve the mac address and then replace the current addresses method reference with this.

Currently uses calling 'virsh verison' and regex comparison of the response to determine whether to use the previous approach which has been restored or the more recent method that calls dhcp_leases.

Fixes: #27

[electrofelix]

So while this works, I've been struggling to adjust the tests to allow for checking the version. I'm also wondering if there is a better way to check the libvirt library version of the connection being used as opposed to assuming a local version check?

[plribeiro3000]

Hmm. This is quite complex to review code only.

Since i don't have a libvirt environment to test this i believe we will have to rely on @strzibny to check this out. Sorry about that.

[electrofelix]

@plribeiro3000 @strzibny looks like I've got all the test issues ironed out, and also looks like I've a better way to detect firstly if ruby-libvirt is compiled with support for dhcp_leases, and secondly whether the connection is talking to a libvirt API that is new enough to respond to such a call.

Only question now remains, are you happy to restore the old approach to maintain compatibility with libvirt older than 1.2.8?

[electrofelix]

Anyone able to comment on whether this will be fundamentally acceptable before I start working on changing the old restored code to use the request model?

[plribeiro3000]

@electrofelix Can you point me to the code that introduced the problem you are having? I dont use libvirt anymore so i need to triple check everything before i merge.

[electrofelix]

@plribeiro3000 It's from a number of users running vagrant-libvirt with an older libvirt that doesn't support the newer call dhcp_leases as described in #27. Unfortunately some of these distributions are in maintenance mode such as RHEL and Ubuntu 14.04. I'll try to put together a Vagrantfile that can use virtualbox to demonstrate the problem when running ubuntu 14.04 fully patched if that would help?

That should at least allow others to see the issue

[plribeiro3000]

No. thats ok. We don't need the extra effort. I just wanted to make sure the reason behind this is documented somewhere. Since there is an issue for it already we don't need anything else.

Could you just answer my question here? I'm really concerned about that hardcoded number.

[infernix]

@electrofelix @plribeiro3000 thanks for the effort here! But can this be merged at some point? If not I'm afraid we will have to deprecate vagrant-libvirt support for older distros if not :(

[electrofelix]

@infernix thanks for the reminder, I've hacked around what I think is the last issue, mainly it's been the difficulty in understanding how to mock the libversion for one of the shindo tests that was causing me the most problems.

If the updated fix makes sense I'll provide some additional changes to help tidy up the restored code based on some of the comments until such time as we no longer need support for the earlier versions of libvirt.

@plribeiro3000 is what I've put in place to patch in a return value for the libversion during testing make sense? or is there a better way to add that in? I had to allow the client attribute to be public, but seeing as it appears to be public in the real object, I'm hoping that's not a problem.

[plribeiro3000]

@electrofelix @infernix Can you guys confirm this work as intended?

I'm a bit worried about this code. The methods are too big and not in the place they belong (requests). Since i'm not working with libvirt for quite a while i'm not able to tell if this change makes sense at all (So much going on here).

If you guys say that this does work, i will have only one question. Does this code make the client method public just for testing purposes?

[infernix]

@electrofelix did you intend to write the exact same comment twice? :)

[electrofelix]

Nope, I'll delete it, was still in the GH comment field, so I assumed I didn't send it the first time.

[electrofelix]

@plribeiro3000 Inside the existing Fog::Compute::Libvirt::Real class the client attribute is publically available https://github.com/fog/fog-libvirt/pull/29/files#diff-d0d400fc75bff5967bdd315151c45251L75

So wasn't too sure why the Fog::Compute::Libvirt::Mock code made it private?

I'm happy to add another commit on top of this to refactor the code into using the request approach before it's landed, just wanted to check that I've done to fix up the testing side makes sense. Already had a look at that, don't think it will be too difficult.

[infernix]

@plribeiro3000 could you provide some feedback? Thanks!

[plribeiro3000]

@infernix Sorry i took too long to review this.

Gotta say i'm not really comfortable with the changes added here. I do know that the method addresses was a mess before but i don't feel we should put it back as a mass again just because.

Maybe we can try to break addresses_ip_command into smaller methods? I've saw at least 2 parts of the code that could become smaller methods that will make this easier to maintain.

Is that something we can work on before merging this in?

[electrofelix]

@plribeiro3000 finally got around to sorting out some of the refactoring, split most of address_ip_command into two helpers, one local and one remote. Not sure if this is what you meant?

Also moved the libversion call to a requests so I could remove the changes to the shindo tests and switched the Mock client attribute back to being private.

Looks like there might be an issue with library compatibility around the fog-core/fog-json dependencies that is breaking for some of the test matrices.

[plribeiro3000]

@electrofelix I believe the split in helpers was a good move. The private move on Mock client attribute was also a good move. 👍 for those refactorings. Maybe we can improve it a little bit more?

It seems that the final piece of code in #addresses_ip_comand does quite a few things:

• It initializes as nil
• If ssh is enabled it does get it by ssh
• raises an exception if is a tls encrypted connection
• get the ip locally
• raise exception if ip is invalid

I believe we can break this flow a little bit like:

• Drop the nil initialization
• Check for TLS connection from within the method that gets the ip locally
• Raise exception if ip is invalid from within #addresses_ip_comand

This would make #addresses_ip_comand a simple if else to get the ip and check if the result is valid

And you can simplify addresses to be something like:

def addresses(service_arg=service, options={})
  service.networks.first.dhcp_leases(self.mac)
  return addresses_ip_command if service.libversion() < 1002008
  addresses_dhcp(service_arg, options)
rescue NoMethodError
  addresses_ip_command(service_arg, options)
end

You could also improve #ssh_ip_command by rescueing the exceptions at the end like the example above.

This modifications would make the code simplier and better to maintain.

[electrofelix]

ok, so a bit more tidy up to be done.

The existing code I added for addresses takes care of replacing the initial method with the correct target method so that all future calls will go direct instead of needing to check on each invocation of this call on the same connection. I was thinking that for any remote connections, that trying to call dhcp_leases each time to determine it doesn't exist, before verifying the library version might be somewhat expensive in latency.

I'm happy enough not to worry about this myself and simplify the code as I don't use it for remote connections, can leave it for someone to report an issue, or maybe just create a PR subsequently and we can discuss the performance versus maintenance pros/cons separately?

On the 'TLS' connection test, I was assuming this would require another method of the form tls_ip_command, which would mean calling it from the address_ip_command and removing the code from the local_ip_command to error out? So it did seem a little odd to move it there in the first place, but maybe I should move it to a separate elseif part of the if/else block so it's not part of the 'else' statement? I'll put that up and we can discuss further.

I also spotted you had a few comments about moving the SSH/IO calls out to requests as well from an earlier review, and wondering how to do that, as most of the Requests appear to be around use of the libvirt client to perform remote requests. Any examples elsewhere that would have a good guideline on what to do?

[plribeiro3000]

Ok. This is open for quite a while now. @electrofelix do you feel strong about merging this in? We can discuss the other points in other PR's.

Now that you mentioned on the TLS and remote connections i think the code is ok for now. It would be better to bring it in and discuss its improvement in another PR.

Regarding using requests i don't have any examples. fog is so big that each provider has its own way to do it nowadays but i believe the best approach would be to leave the models as thin layers of logic and the requests would be anything that touches a service outside fog. This would make all requests simple enough so they would not need that much test. And all the logic like parsing data, generating data to send to the provider, change a state and so on can be easily tested without depending on any http requests or any sort of problematic dependency.

[infernix]

Merging this in fixes libvirt for older distros such as debian 8, centos 6, ubuntu 12.xx, 14.xx (iirc). It does become less relevant over time but some of those are LTS so would be good to have it just work[tm] on them.

[plribeiro3000]

Coll. Bring it in!

[infernix]

So this is working great on older distros but with a minor patch:

diff --git a/lib/fog/libvirt/models/compute/server.rb b/lib/fog/libvirt/models/compute/server.rb
index cecaf5b..7fd663d 100644
--- a/lib/fog/libvirt/models/compute/server.rb
+++ b/lib/fog/libvirt/models/compute/server.rb
@@ -321,7 +321,7 @@ module Fog
           end
         end

-        def local_ip_command(command)
+        def local_ip_command(ip_command)
           # Execute the ip_command locally
           # Initialize empty ip_address string
           ip_address=""

If not patched, it's throwing:

lib/fog/libvirt/models/compute/server.rb:329:in `local_ip_command': undefined local variable or method `ip_command' for #<Fog::Compute::Libvirt::Server:0x0000000002475048> (NameError)
    from /root/.vagrant.d/gems/2.4.3/gems/fog-libvirt-0.4.2/lib/fog/libvirt/models/compute/server.rb:367:in `addresses_ip_command'
    from /root/.vagrant.d/gems/2.4.3/gems/fog-libvirt-0.4.2/lib/fog/libvirt/models/compute/server.rb:290:in `addresses'
    from /root/.vagrant.d/gems/2.4.3/gems/vagrant-libvirt-0.0.43/lib/vagrant-libvirt/action/wait_till_up.rb:43:in `block (3 levels) in call'
...

@electrofelix can you confirm this is a typo?

[electrofelix]

@infernix it is, and it seems the tests I added are not testing the ssh_ip_command and local_ip_command methods sufficiently, otherwise they should have both thrown an error on this, working on a fix