Cannot list inactive pools

[electrofelix]

https://github.com/fog/fog-libvirt/blob/master/lib/fog/libvirt/requests/compute/list_pools.rb#L30 makes a call to get the num_of_volumes, however for inactive pools this will throw an exception in pool_to_attrbiutes that gets swallowed by either find_pool_by_name or find_pool_by_uuid.

I installed the gem, created a new pool in libvirt using virt-manager or virsh. Next I deactived the pool and ran the following code (similar to what vagrant-libvirt would do):

require 'fog/libvirt'
conn_attr = {
    :provider => "libvirt",
    :libvirt_uri => "qemu:///system",
    :libvirt_ip_command => " awk \"/$mac/ {print \\$1}\" /proc/net/arp"
}
conn = Fog::Compute.new(conn_attr)
pool = conn.client.lookup_storage_pool_by_name("test")
pool.num_of_volumes

The result was be similar to:

Libvirt::RetrieveError: Call to virStoragePoolNumOfVolumes failed: Requested operation is not valid: storage pool 'test' is not active
    from (irb):63:in `num_of_volumes'
    from (irb):63
    from /home/baileybd/.rvm/rubies/ruby-2.0.0-p643/bin/irb:12:in `<main>'

Looking at the states supported by list_pools, it suggests it can handle inactive pools.

This caused vagrant-libvirt/vagrant-libvirt#536

[lzap]

Unable to reproduce with vagrant-2.2.4-2.fc30 both active and inactive are listed correctly now.

Edit: I take it back, the bug is still there :-)

[github-actions]

This issue has been marked inactive and will be closed if no further activity occurs.

[electrofelix]

I'd forgotten this issue.

The fix appears to be a relatively simple one in changing to check if the pool is active before retrieving the number of volumes:

--- lib/fog/libvirt/requests/compute/list_pools.rb
+++ lib/fog/libvirt/requests/compute/list_pools.rb
@@ -27,7 +27,7 @@ module Fog
             :name           => pool.name,
             :allocation     => pool.info.allocation,
             :capacity       => pool.info.capacity,
-            :num_of_volumes => pool.num_of_volumes,
+            :num_of_volumes => pool.active? ? pool.num_of_volumes : nil,
             :state          => states[pool.info.state]
           }
         end

Question is, what's the impact?

Right now it's not possible to list inactive pools without going directly to the client and checking if the pool that is of interest needs to be made active. It's possible that some code will be surprised by the code reporting additional pools that previously were hidden and such code might be surprised by getting a nil value for the num_of_volumes instead of the actual number.

I could add a guard where this only fixes this if some option is set so you only see active pools by default and those that want to list all must include a flag for the inactive ones to appear.

Thoughts?

[electrofelix]

The solution in the case of only listing inactive if requested instead of patching to include by default is as follows:

--- lib/fog/libvirt/requests/compute/list_pools.rb
+++ lib/fog/libvirt/requests/compute/list_pools.rb
@@ -5,19 +5,21 @@ module Fog
         def list_pools(filter = { })
           data=[]
           if filter.key?(:name)
-            data << find_pool_by_name(filter[:name])
+            data << find_pool_by_name(filter[:name], filter[:include_inactive])
           elsif filter.key?(:uuid)
-            data << find_pool_by_uuid(filter[:uuid])
+            data << find_pool_by_uuid(filter[:uuid], filter[:include_inactive])
           else
             (client.list_storage_pools + client.list_defined_storage_pools).each do |name|
-              data << find_pool_by_name(name)
+              data << find_pool_by_name(name, filter[:include_inactive])
             end
           end
           data.compact
         end
 
         private
-        def pool_to_attributes(pool)
+        def pool_to_attributes(pool, include_inactive)
+          return nil unless pool.active? || include_inactive
+
           states=[:inactive, :building, :running, :degrated, :inaccessible]
           {
             :uuid           => pool.uuid,
@@ -27,19 +29,19 @@ module Fog
             :name           => pool.name,
             :allocation     => pool.info.allocation,
             :capacity       => pool.info.capacity,
-            :num_of_volumes => pool.num_of_volumes,
+            :num_of_volumes => pool.active? ? pool.num_of_volumes : nil,
             :state          => states[pool.info.state]
           }
         end
 
-        def find_pool_by_name name
-          pool_to_attributes(client.lookup_storage_pool_by_name(name))
+        def find_pool_by_name name, include_inactive
+          pool_to_attributes(client.lookup_storage_pool_by_name(name), include_inactive)
         rescue ::Libvirt::RetrieveError
           nil
         end
 
-        def find_pool_by_uuid uuid
-          pool_to_attributes(client.lookup_storage_pool_by_uuid(uuid))
+        def find_pool_by_uuid uuid, include_inactive
+          pool_to_attributes(client.lookup_storage_pool_by_uuid(uuid), include_inactive)
         rescue ::Libvirt::RetrieveError
           nil
         end

While I can test this by hand it might take me a little while to work out how to add shindo tests to ensure the correct behaviour.

[electrofelix]

So based on the above change what I really need to test is that the functionality of

fog-libvirt/lib/fog/libvirt/requests/compute/list_pools.rb

Lines 19 to 46 in 783e7fa

	private
	def pool_to_attributes(pool)
	states=[:inactive, :building, :running, :degrated, :inaccessible]
	{
	:uuid => pool.uuid,
	:persistent => pool.persistent?,
	:autostart => pool.autostart?,
	:active => pool.active?,
	:name => pool.name,
	:allocation => pool.info.allocation,
	:capacity => pool.info.capacity,
	:num_of_volumes => pool.num_of_volumes,
	:state => states[pool.info.state]
	}
	end
	def find_pool_by_name name
	pool_to_attributes(client.lookup_storage_pool_by_name(name))
	rescue ::Libvirt::RetrieveError
	nil
	end
	def find_pool_by_uuid uuid
	pool_to_attributes(client.lookup_storage_pool_by_uuid(uuid))
	rescue ::Libvirt::RetrieveError
	nil
	end
	end

handles various pool objects as returned by calling client.lookup_storage_pool_by_name() and client.lookup_storage_pool_by_uuid() in functions find_pool_by_name and find_pool_by_uuid are processed as expected by pool_to_attributes.

However I'm not seeing how to mock the client part with shindo tests? I can certainly restructure the code so the function pool_to_attributes is common to both the Mock and Real classes and construct a fake pool object to be passed by the Mock through pool_to_attributes to ensure the behaviour is exercised on each call to the Mock, but this feels wrong to me, as it's the calls to client that are external inputs to this where need to validate that any helper behaviour processes it correctly in the Real class.

Any suggestions on how best to write a test for the above to ensure pool_to_attributes behaves as expected?

[lzap]

The proposed change via a filter argument looks good to me.

Looking into tests is on my TODO, I haven't worked with Shindo yet. Maybe @geemus would have an advice how to approach testing in this case?

[electrofelix]

@geemus I've put up #95 to act as a discussion point for how to better write tests for this.

I don't think it's quite doing the right approach with regards to how to test, but it does show that the general concept of the filter approach should work.

[lzap]

Thanks for that.

Sidenote: I would love to have real tests as well, fixtures are nice but a real test against a libvirt session is a real test. We would not be able to run them on our CI however ability to run them locally would increase our confidence. That is just a wish, this is outside of your scope indeed.

[geemus]

Agreed. I like having the mocks (partly to aide in CI/testing, partly because they are useful for end users when they are doing testing), but I always prefer to back them up with real tests that can be run to double-check stuff.

[github-actions]

This issue has been marked inactive and will be closed if no further activity occurs.

[electrofelix]

Hoping to get back to this sometime next month, is that enough to stall the inactivity bot?

[ekohl]

Not sure but I removed the label so I think so?

[geemus]

Yeah, I think comments will reset the timer. You can also use a pinned tag to have it ignore the timer, I think, but I find in most cases having it ping every once in a while (as long as a comment restarts it) can help keep things fresh and in mind.

[github-actions]

This issue has been marked inactive and will be closed if no further activity occurs.