nb_inventory - fix cache key uniqueness across sources

[s-hertel]

Related Issue

Possibly fixes #1411 (comment) / ansible/ansible#86292.

I suspect a large inventory is needed to reproduce the performance issue. I was not able to reproduce by using a small inventory provided by the netbox free trial. The issue reported against ansible/ansible outputs ~30x as many lines as my attempt to reproduce.

No related issues.

New Behavior

Pass get_cache_key the inventory source, and read/set the backing cache once per source. This aligns with the developer guide and intended use of the method: https://docs.ansible.com/projects/ansible/latest/dev_guide/developing_inventory.html#inventory-cache.

This reduces the number of times the raw data is converted to/from the internal cache format, and fixes cache key uniqueness across sources.

Contrast to Current Behavior

Every HTTP request is converted to/from the internal cache format.

Multiple netbox.netbox.nb_inventory sources can read from/write to the same cache keys, which is precisely what the get_cache_key method is intended to prevent.

Discussion: Benefits and Drawbacks

The change is backwards compatible.

Changes to the Documentation

N/A

Proposed Release Note Entry

N/A

Double Check

• I have read the comments and followed the CONTRIBUTING.md.
• I have explained my PR according to the information in the comments or in a linked issue.
• My PR targets the devel branch.

[sc68cal]

Can you discuss this change in a bit more detail? I understand that you are trying to reduce the number of HTTP requests that are sent, but my concern is that it's using a global variable. Can you discuss the global variable usage as opposed to having the InventoryModule class contain the dictionary that you are using as the cache, and then getters/setters?

I ask this because the only uses I see of it are within InventoryModule._fetch_information and InventoryModule.parse which means that it would be very simple to have it as an internal field of the class.

Thanks.

[s-hertel]

It can certainly use an internal field instead of a global. To clarify, this has nothing to do with reducing the number of HTTP requests.

When parsing multiple inventory sources with this plugin (for example ansible-inventory -i netbox1.yml -i netbox2.yml --list), nothing prevents those sources from using the same cached data. A user can work around it by configuring different backends or ensuring no sources use the same api_endpoint, but normally this is handled by default, by passing the inventory source to self.get_cache_key. This way if netbox1.yml has access to some objects, and netbox2.yml has access to other objects, they retrieve their own data instead of each other's. (Note: I'm assuming netbox object-level permissions are similar to other cloud services.)

I hoped reducing the number of times the cache needs to be deserialized would improve the performance, however, it looks like that is not the case. In light of that, I've simplified this change.

[sc68cal]

Awesome. Thank you for the detailed write-up, I am now in sync with your thinking. This change looks good to me

I think I still have some questions about cache key uniqueness

[sc68cal]

Can you explain this a bit more? Because it still isn't quite clear to me. You set cache_key_base down in parse with the results of self.get_cache_key(path) and then just add it together with a second call to self.get_cache_key(url) but it's not clear to me why you are doing this.

I would like to get a little more context on why you are doing this the way you are doing it.

I did take a look at some other Ansible plugins that call self.get_cache_key and none of them appear to do the behavior that you are introducing, so I'd like to understand it a bit more.

I also read the link that you provided from the Ansible plugin development guide and they too, do not appear to call out this issue you describe.

When parsing multiple inventory sources with this plugin (for example ansible-inventory -i netbox1.yml -i netbox2.yml --list), nothing prevents those sources from using the same cached data

In your example, are these two netbox inventory sources pointing at the same NetBox server, or different servers?

Thanks!

[s-hertel]

This ensures separate inventory sources use separate cache keys by adding a unique but predictable prefix created from the inventory source.

I did take a look at some other Ansible plugins that call self.get_cache_key and none of them appear to do the behavior that you are introducing, so I'd like to understand it a bit more.

Most inventory plugins call get_cache_key per inventory source, not per URL. I've added the former, but haven't removed the latter (mainly because I'm not sure whether or not it's a significant issue, however, you might want to look at ansible/ansible#86504 doing something very similar). Here's an example of how inventory plugins should use self.get_cache_key: https://github.com/ansible-collections/amazon.aws/blob/11.1.0/plugins/inventory/aws_ec2.py#L964 -> https://github.com/ansible-collections/amazon.aws/blob/11.1.0/plugins/plugin_utils/inventory.py#L193.

In your example, are these two netbox inventory sources pointing at the same NetBox server, or different servers?

Both inventory sources need to call the same URL(s) to reproduce what I'm talking about. You can test by setting token to a valid value in the first file, and omit it from the second. If you compare with an inventory plugin like amazon.aws.aws_ec2, separate sources use their own authentication.