Incorrect SharedInformerFactory#getExistingSharedIndexInformer

[hibnico]

The caching key of informers has changed in the client 5.2. And as far as I can see, the lookup function is not coherent with the create functions. The function getExistingSharedIndexInformer is sensitive to the name I chose for the Java class of my custom resource.

For instance, if I have a custom resource under the API com.acme/myapp:v1, and that I have created the Java class MyAppCustomResource annotated to be a proper fabric8 CustomResource, in the following code existingInformer will be null, but I would expect that existingInformer is the same instance as createdInformer.

SharedIndexInformer createdInformer = sharedInformerFactory.sharedIndexInformerForCustomResource(MyAppCustomResource.class, MyAppCustomResourceList.class)
SharedIndexInformer existingInformer = sharedInformerFactory.getExistingSharedIndexInformer(MyAppCustomResource.class)

[manusa]

Relates to: #2746 #2882

[rohanKanojia]

@hibnico : hello, I'm not able to reproduce this issue. I created MyAppCustomResource class like this:

package io.fabric8.crd;

import io.fabric8.kubernetes.client.CustomResource;
import io.fabric8.kubernetes.model.annotation.Group;
import io.fabric8.kubernetes.model.annotation.Version;

@Group("com.acme")
@Version("v1")
public class MyAppCustomResource extends CustomResource<Void, Void> { }

When I'm running this code I'm able to see both createdInformer and existingInformer being not null and same:

try (KubernetesClient client = new DefaultKubernetesClient()) {
    SharedInformerFactory informerFactory = client.informers();

    SharedIndexInformer<MyAppCustomResource> createdInformer = informerFactory.sharedIndexInformerForCustomResource(MyAppCustomResource.class, MyAppCustomResourceList.class, 5 * 1000L);
    SharedIndexInformer<MyAppCustomResource> existingInformer = informerFactory.getExistingSharedIndexInformer(MyAppCustomResource.class);

    System.out.println(createdInformer);
    System.out.println(existingInformer);
}

Could you please check what I'm doing wrong?

[hibnico]

The @kind annotation is missing, if the kind is not something close to the class name (considering the code, it seems about containing the plural of the class name), the lookup fails.

[rohanKanojia]

Thanks for quick reply,

I think I can see what's the issue now. For calculating plural I'm resolving it from class name instead of checking kind:

kubernetes-client/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/informers/SharedInformerFactory.java

Lines 269 to 271 in 0d9e634

	for (Map.Entry<String, SharedIndexInformer> entry : this.informers.entrySet()) {
	if (entry.getKey().contains(Pluralize.toPlural(apiTypeClass.getSimpleName().toLowerCase()))) {
	foundSharedIndexInformer = (SharedIndexInformer<T>) entry.getValue();

[hibnico]

I am sorry I have missed the updates on this issue. Looking at the fix now, it will indeed fix the issue I faced, but I fear that the lookup strategy in the map of informers is still not safe.

In the SharedInformerFactory, the function getInformerKey which is used at insert time in the internal map, is not the symmetric of isKeyOfType which is used at lookup time. The former function is very precise, the latter is matching the first that match a subset of the key. It means that if two informers with kinds with the same plural are created, both will be indeed created and stored in the internal map, but only one can be looked up.

[rohanKanojia]

I think I should have removed getExistingSharedIndexInformer(Class<T> apiTypeClass) while changing the key of informers to String rather than type. It doesn't make much sense to get Informer with just the type when we're forming a key based on OperationContext.

[rohanKanojia]

What I'm saying is that it's no longer possible to fetch informer from map precisely when you have more than one informer of same type present in informer map.

Do you think it's a good idea to add getExistingSharedIndexInformer(OperationContext context)? I think it will be even more confusing.

[hibnico]

Ok, I see the deeper issue at stake here. Making getExistingSharedIndexInformer working properly in every case seems hard.

In my use case, I wanted to have access to my specific informer into two parts of my software. And since in these two parts I can easily have access to the SharedInformerFactory, my code just leverages the fact that in an SharedInformerFactory is stored what I am looking for. Considering the effort at stake here, the safer way to handle my use case is just managing myself the sharing of my specific informer, and not rely on getExistingSharedIndexInformer.

[rohanKanojia]

Umm, do you think it's a good idea for users to provide their own key. In case someone provides their own key like this:

informerFactory.inNamespace("foo").withInformerKey("MyAppResource_Informer1").sharedIndexInformerForCustomResource(MyAppCustomResource.class, MyAppCustomResourceList.class, 5 * 1000L);

Or maybe add another method List<SharedIndexInformer<T>> getExistingSharedIndexInformer ? It'll be up to the user to iterate informers matching type

[hibnico]

Letting choose the key in the API is probably error prone, since the key must be unique. And then if a key has been overriden, the API should notify about it somehow. This doesn't simplify the API much.

I like though the idea of being able to list all existing informers. Maybe the API could list all the informers with their creating contexts. Something like: List<Entry<OperationContext, SharedIndexInformer<T>>> getExistingSharedIndexInformers()

[manusa]

Relates to: #2010