fix(kubernetes): Fix all agents caching cluster-scoped resources #4128
Conversation
Pull some common logic out of ProcessOnDemandResult so that it can be reused when we need to process the results of a loadData request in an upcoming commit.
There are no tests of loadData() in the KubernetesCoreCachingAgent; add a simple test that validates a namespaced and cluster-scoped kind can be succesfully cached (ie, is returned from loadData and is persisted to the cache).
Make a few collections returned by the kubernetes caching agent immutable, and add some nonnull annotations that simplify the work of calling code.
This commit splits the work to cache kubernetes objects into two functions: the first caches namespace-scoped objects for all namespaces relevant for the caching agent, and the second caches all cluster-scoped resources. This means that we'll no longer try to read the cluster-scoped resources once per namespace and will read them once per caching agent.
Currently all caching agents are caching cluster-scoped resources; this should be delegated to a single agent so that we don't duplicate work.
| .extracting(data -> data.getAttributes().get("name")) | ||
| .containsExactly(STORAGE_CLASS_NAME); | ||
|
|
||
| assertThat(loadDataResult.getResults()).containsKey(DEPLOYMENT_KIND); |
There was a problem hiding this comment.
Is there a reason we have this assertion twice (once on line 496 and once here)?
There was a problem hiding this comment.
Good catch! I had intended one set of assertions to check that the return value of loadData had the expected entries, and the other set to check that the cache actually contained the expected entries, but was checking the return value in both.
When trying to add the assertion on the contents of the cache, I realized that loadData doesn't actually update the cache itself---the return value is used by another function to actually update the cache. So I just removed the duplicate set of assertions, as updating the cache is not part of loadData's contract.
| @@ -19,6 +19,7 @@ | |||
|
|
|||
| import com.fasterxml.jackson.databind.ObjectMapper; | |||
| import com.google.common.collect.ImmutableList; | |||
| import com.google.common.collect.ImmutableSetMultimap; | |||
There was a problem hiding this comment.
Whoah, new favorite Pokemon!
I had intended the second set of assertions to be testing the contents of the cache. But it loadData doesn't actually update the cache (it just returns results to an outer function that stores the result) so we can't test that the cache is updated after calling loadData. Just remove the extra assertions.
refactor(kubernetes): Minor refactor of core caching agent test
Pull some common logic out of ProcessOnDemandResult so that it can be reused when we need to process the results of a loadData request in an upcoming commit.
test(kubernetes): Add tests to loadData in core caching agent
There are no tests of loadData() in the KubernetesCoreCachingAgent; add a simple test that validates a namespaced and cluster-scoped kind can be succesfully cached (ie, is returned from loadData and is persisted to the cache).
refactor(kubernetes): Immutable collections and nonnull annotations
Make a few collections returned by the kubernetes caching agent immutable, and add some nonnull annotations that simplify the work of calling code.
refactor(kubernetes): Split caching of resources by scope
This commit splits the work to cache kubernetes objects into two functions: the first caches namespace-scoped objects for all namespaces relevant for the caching agent, and the second caches all cluster-scoped resources.
This means that we'll no longer try to read the cluster-scoped resources once per namespace and will read them once per caching agent.
fix(kubernetes): Fix all agents caching cluster-scoped resources
Currently all caching agents are caching cluster-scoped resources; this should be delegated to a single agent so that we don't duplicate work.