proxmox inventory: proposal for #9710 (caching)

[iqt4]

SUMMARY

Updated cache logic according to Anible's dynamic inventory description

Fixes #9710

bugfixes:

• proxmox inventory plugin did not update inventory correctly after meta refresh_inventory.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

proxmox inventory

ADDITIONAL INFORMATION

Details see #9710

[ansibullbot]

cc @Ajpantuso @Thulium-Drake @ilijamt @joshainglis @karmab @krauthosting
click here for bot help

[felixfontein]

Thanks for your contribution! I've added some comments below, I'd change the flow slightly so that the cache is still used if both update_cache and use_cache are true.

Also, can you please add a changelog fragment? Thanks.

[felixfontein]

Looks good from my side. @Thulium-Drake if it's ok for you as well I'll merge.

[russoz]

I'm not sure about this - the _get_json() method is used multiple times during the execution of "higher" level _populate(). This looks similar to a "global" variable that is used to go behind the logic of the program. data is already being returned by the method (and it is marked unsafe as it should). It is completely out of the scope of what this method is supposed to do.

IMO this is a very cheap solution to the short term problem and an expensive problem in the long term.

[felixfontein]

How does this differ from what the code was doing before? Except that it was using self._cache[self.cache_key][url] instead of self._results[url].

Also yes, the cache is a global variable and that's how the cache should work, shouldn't it? How else should it cache?

[russoz]

I would argue that it was already a poor design (IMO) before :-) Literally ALL urls passed to _get_json() are being stored and that includes more than just the URLs containing inventory data.

That being said, I believe this method should not be concerned with storing its results in the "global" cache. Some other method on a higher level should be doing that.

One other point worth of mention is the make_unsafe(). I am not familiar with the semantics of this and I can only make inferences based on the name, but if this data is unsafe in anyway, why is a raw copy of it being stored unchecked in the cache for global consumption? Shouldn't it be marked unsafe as well?

I won't be able to produce a better design at short notice, but I would definitely like to take a look at that later on.

[felixfontein]

Yes, that's a different story :) The thing is that that's harder to change, since it is a breaking change (in the sense that existing caches will stop working and we need to make sure that the plugin won't produce garbage out when finding a cache from an older version, or alternatively an older version finding a cache from a newer version). This is likely also something we should only merge for a new major release, just in case :)

[Thulium-Drake]

As a general note, this could mean that the module this module is originally based on also needs some love to fix cache handling. As I (5 years ago already 😮) copied that module as a basis to build this one.

I've opened an issue on the foreman repo, maybe @evgeni has a good idea about @russoz 's points :-)

[Thulium-Drake]

I did some digging in the changes made to the source module since the fork, these commits might be of interest/inspiration (by the sole virtue of having something to do with caching):

• Addresses an issue with caching in general: theforeman/foreman-ansible-modules@2b71de1
• Addresses an issue with modifying the source data retrieved: theforeman/foreman-ansible-modules@4881b80

[iqt4]

Thank you for your remarks.

I agree, the solution may be called very cheap ;) but it is an effective bugfix without rewriting the plugin.

The reference design forsees the steps:

1. attempt to read from cache
2. if not possible, read from source
3. update cache if required
4. populate inventory object

This plugin bundled steps 1-3 "below" the _populate() function. The proposed bugfix moves the cache update (3) after _populate().

A further unbundling of reading (cached) data from source and populating the inventory object requires some redesign.
The challenge is that the urls are dynamic depending on the status of the proxmox host, e.g.

community.general/plugins/inventory/proxmox.py

Lines 374 to 377 in 94e1511

	if status_key not in properties or not properties[status_key] == 'running':
	return
	ret = self._get_json(f"{self.proxmox_url}/api2/json/nodes/{node}/lxc/{vmid}/interfaces", ignore_errors=[501])

I.e. a logic is required to a) dynamically read the (cached) data from source and b) dynamically populate the inventory object

ps: the foreman plugin still suffers the same issue

[patchback]

Backport to stable-9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-9/d696bb7b8992bfc6535c5e51dd37a00b9b4e22df/pr-9760

Backported as #9769

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[patchback]

Backport to stable-10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-10/d696bb7b8992bfc6535c5e51dd37a00b9b4e22df/pr-9760

Backported as #9770

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@iqt4 thanks for fixing this!
@Thulium-Drake @russoz thanks for reviewing!

[russoz]

Thanks @Thulium-Drake @iqt4 @felixfontein for the comments. As I wrote before I am interested in looking into that but when time allows it, which won't be soon.