Not using Informer cache for Pipelines and ISBServices

[juliev0]

Describe the bug

We made a decision that in Numaplane we don't need to define the Pipeline/ISBService specs - therefore we are using the Kubernetes Dynamic client to create/update/get resources.

It looks like currently, we are not using an Informer cache and instead are making multiple repeated calls to GET from the Kubernetes API directly.

Assuming that we stick with the decision to use dynamic client (which may first be worth a deep dive analysis), we can incorporate a Dynamic client Informer described here.

See slack thread.

Expected behavior
When calling Get() for a given resource multiple times in a row when the resource hasn't changed, I shouldn't hit the K8S API every time. Instead, my resource should be updated asynchronously into my local cache.

---

Message from the maintainers:

Impacted by this bug? Give it a 👍. We often sort issues this way to know what to prioritize.

[juliev0]

So, I've been trying to figure out 2 different possible ways to handle this:

1. see if there's a way to make this work using the controller-runtime framework
2. Imitate what Argo Workflows does with its wfInformer (Workflow informer), in which the Event Handlers (AddFunc, UpdateFunc, and DeleteFunc described in the article) basically add a Workflow to a queue

I thought it would be nice if we could do 1 if it's easy. After seeing the comment on watching unstructured objects here, I think it may work to change the existing Watch for Pipelines (as an example) to:

        u := &unstructured.Unstructured{}
	u.SetGroupVersionKind(schema.GroupVersionKind{
		Kind:    "Pipeline",
		Group:   "numaflow.numaproj.io",
		Version: "v1alpha1",
	})
	if err := controller.Watch(internal.Kind{Cache: mgr.GetCache(), Type: u},
		handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &apiv1.PipelineRollout{}, handler.OnlyControllerOwner()),
		predicate.ResourceVersionChangedPredicate{}); err != nil {
		return err
	}

but if that does work, we still need to use this Informer cache in the actual calls to Get our child resource. Would it work to call r.client.Get() and pass in an unstructured.Unstructured{}instead?

[juliev0]

This article Hao found is a good read: https://medium.com/@timebertt/kubernetes-controllers-at-scale-clients-caches-conflicts-patches-explained-aa0f7a8b4332

One thing I see in there is:

By default, the client.Client created by a controller-runtime Manager is a DelegatingClient. It delegates Get and List calls to a Cache and all other calls to a client, that talks directly to the API server. Exceptions are requests with *unstructured.Unstructured objects and object kinds that were configured to be excluded from the cache in the DelegatingClient.

It kind of sounds like it's saying that a Get() call won't go to the cache if it's for an Unstructured type. I wonder if that's the case even if we explicitly watch the Unstructured type and associate it with our cache, like my code above ?

[juliev0]

I guess before we begin on this work, we also should probably make sure that nothing will go wrong as a result of having slightly older Pipelines and ISBServices. Right now we always have the latest and greatest ones. What will be the result of having a slightly older one? (I will try to think about this as well.) cc @xdevxy

[juliev0]

added this issue

[juliev0]

I added this comment. I think it might be wise, if we are in the PPND strategy to actually get the Pipeline from the K8S API instead of the Informer. In any case, I think the strategy we choose should be able to get the unstructured resource from either the K8S API or from the Informer.