Use cached get() results to limit calls to get()

amandaharth commented 5 months ago

Summary

There may be reasons this is the way it is (I am still wrapping my head around the Puppet Resource API) - but I am wondering if we can get rid of the names.nil? checks in the rsapi_provider_get function.

Please feel free to educate me if there is something I've misunderstood or not considered.

In my testing, with simple_get_filter not implemented and operating in a Puppet Agent and Master Server environment, removing the names.nil? checks stopped get(context) from being called again for every resource which had pending changes. All resources of the given type were collected from the system with one get() call during the agent run and cached, with subsequent calls to rsapi_provider_get using the cached information. To me this seems like it would be the desired behaviour.

Additional Context

The behaviour I observed before removing the names.nil? checks:

The cached data is returned if the cache has been marked as complete and names is nil. E.g. after the initial get(context) has been done, when retrieving current state for resources which are defined in the catalogue but have no pending changes.
Therefore, if names is not nil or if the cache hasn't been marked complete, information about all resources of the given type is fetched by calling the provider's get(context) function.
The fetched information for all resources of the given type is added to the cache.
The cache is marked complete if names is nil and simple_get_feature is not implemented.
Subsequent calls to rsapi_provider_get (e.g. to retrieve the current state of a resource which has pending changes) pass in a value for 'names', and therefore names.nil? is false, and the cache in point 1 above isn't returned or used. get() is therefore called again per resource with pending changes, retrieving all resource information of that type again each time.

Removing the names.nil? checks allows the cache to be populated with information about all of the resources of the given type with one get() call, mark the cache as complete, and allow the cache to be used in subsequent calls to rsapi_provider_get for each resource, whether they have pending changes or not.

Simple_get_filter behaviour shouldn't be affected by this change, as when simple_get_filter is implemented the cache would never be marked as complete or returned and my_provider.get(context, names) would still be called every time.

Testing and observations

Before the change:

Defined 60+ resources of the same type in the catalogue
Manually made changes to 5 of those resources, to trigger Puppet to revert changes on those 5 resources
Observed 5 calls to get(context); once per resource with pending changes
Observed current state for the other defined (but unchanged) resources was retrieved from cache

After the change:

Defined 60+ resources of the same type in the catalogue
Manually made changes to 5 of those resources, to trigger Puppet to revert changes on those 5 resources
Observed 1 call to get(context)
Current state for all defined resources (whether unchanged or with changes pending) was retrieved from cache