canonical / prometheus-openstack-exporter

OpenStack exporter for the prometheus monitoring system
GNU General Public License v3.0
128 stars 113 forks source link

Update marker to get the next instances #65

Closed czunker closed 5 years ago

czunker commented 5 years ago

This fixes #64

gabrielegiammatteo commented 5 years ago

this does not work for me after the merge . Looking at the commit, it seems to me that the code before the commit was correct, while now it is not because marker variable is never updated and I have an infinite loop (but this was the reason why #64 was open... so I'm a bit confused)

vmpjdc commented 5 years ago

The previous code was definitely not correct. Assigning to the marker variable within the loop won't affect search_opts, i.e.:

$ python
[...]
>>> x = 1
>>> y = {'x': x}
>>> y
{'x': 1}
>>> x = 2
>>> y
{'x': 1}
>>> x
2

However, if it's still infinite looping, that's definitely a surprise. Can you try putting something like this and see if new_instances starts repeating? I would expect it to be an empty list at some point, which would be false, and therefore break the loop.

            if new_instances:
                import pprint ; pprint.pprint(new_instances)    # debug code
                search_opts['marker'] = new_instances[-1]['id']
gabrielegiammatteo commented 5 years ago

I tired and the new_instances is repeated always with the same content. Also the call to nova is always the same: DEBUG:novaclient.v2.client:GET call to compute for http://xxx.xxx.xxx.xxx:8774/v2.1/servers/detail?all_tenants=1&limit=100 used request id req-yyy

Assigning to the marker variable within the loop won't affect search_opts, i.e.:

Actually search_opts is recreated at every iteration inside the loop, so if marker is not updated search_opts['marker'] will be always empty.

I think another solution would be to get rid of the marker variable and create search_opts outside the loop and update it inside the loop

        search_opts = {'all_tenants': '1', 'limit': '100', 'marker': ''}
        while True:
            new_instances = [x._info for x in nova.servers.list(search_opts=search_opts)]
            if new_instances:
                search_opts['marker'] = new_instances[-1]['id']
                info['instances'].extend(new_instances)
            else:
                break
vmpjdc commented 5 years ago

Writing it the way you suggest would certainly be clearer, but I agree that the original code should certainly have worked! I'll revert this PR; please let us know if it works and then we can make a change like the one you suggested as a cleanup.

gabrielegiammatteo commented 5 years ago

yes it work for me