Cannot list inactive pools about fog-libvirt HOT 14 CLOSED

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?

from fog-libvirt.

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

from fog-libvirt.

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 :-)

from fog-libvirt.

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

from fog-libvirt.

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?

from fog-libvirt.

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.

from fog-libvirt.

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

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?

from fog-libvirt.

@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.

from fog-libvirt.

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.

from fog-libvirt.

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.

from fog-libvirt.

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

from fog-libvirt.

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

from fog-libvirt.

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.

from fog-libvirt.

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

from fog-libvirt.