feat: make Kubernetes informers cancellable (podman-desktop#9411)

* feat: make informer cancellable Signed-off-by: Philippe Martin <[email protected]> * test: unit tests Signed-off-by: Philippe Martin <[email protected]> * fix: cancel timer directly Signed-off-by: Philippe Martin <[email protected]>
feloy · Oct 18, 2024 · 484d68d · 484d68d
1 parent 8af3761
commit 484d68d
Show file tree

Hide file tree

Showing 5 changed files with 129 additions and 27 deletions.
diff --git a/packages/main/src/plugin/kubernetes/contexts-informers-registry.spec.ts b/packages/main/src/plugin/kubernetes/contexts-informers-registry.spec.ts
@@ -20,14 +20,23 @@ import type { Informer, KubernetesObject } from '@kubernetes/client-node';
 import { describe, expect, test } from 'vitest';
 
 import { ContextsInformersRegistry } from './contexts-informers-registry.js';
+import type { CancellableInformer } from './contexts-states-registry.js';
 import { TestInformer } from './test-informer.js';
 
 describe('ContextsInformers tests', () => {
   test('hasInformer should check if informer exists for context', () => {
     const client = new ContextsInformersRegistry();
     client.setInformers(
       'context1',
-      new Map([['pods', new TestInformer('context1', '/path/to/resource', 0, undefined, [], [])]]),
+      new Map([
+        [
+          'pods',
+          {
+            informer: new TestInformer('context1', '/path/to/resource', 0, undefined, [], []),
+            cancel: (): void => {},
+          },
+        ],
+      ]),
     );
     expect(client.hasInformer('context1', 'pods')).toBeTruthy();
     expect(client.hasInformer('context1', 'deployments')).toBeFalsy();
@@ -39,11 +48,27 @@ describe('ContextsInformers tests', () => {
     const client = new ContextsInformersRegistry();
     client.setInformers(
       'context1',
-      new Map([['pods', new TestInformer('context1', '/path/to/resource', 0, undefined, [], [])]]),
+      new Map([
+        [
+          'pods',
+          {
+            informer: new TestInformer('context1', '/path/to/resource', 0, undefined, [], []),
+            cancel: (): void => {},
+          },
+        ],
+      ]),
     );
     client.setInformers(
       'context2',
-      new Map([['pods', new TestInformer('context2', '/path/to/resource', 0, undefined, [], [])]]),
+      new Map([
+        [
+          'pods',
+          {
+            informer: new TestInformer('context2', '/path/to/resource', 0, undefined, [], []),
+            cancel: (): void => {},
+          },
+        ],
+      ]),
     );
     expect(Array.from(client.getContextsNames())).toEqual(['context1', 'context2']);
   });
@@ -66,12 +91,12 @@ describe('ContextsInformers tests', () => {
     expect(states.hasInformer('ctx1', 'services')).toBeTruthy();
     expect(states.hasInformer('ctx1', 'pods')).toBeFalsy();
 
-    states.setResourceInformer('ctx1', 'pods', {} as Informer<KubernetesObject>);
+    states.setResourceInformer('ctx1', 'pods', {} as CancellableInformer);
     expect(states.hasContext('ctx1')).toBeTruthy();
     expect(states.hasInformer('ctx1', 'services')).toBeTruthy();
     expect(states.hasInformer('ctx1', 'pods')).toBeTruthy();
 
-    expect(() => states.setResourceInformer('ctx2', 'pods', {} as Informer<KubernetesObject>)).toThrow(
+    expect(() => states.setResourceInformer('ctx2', 'pods', {} as CancellableInformer)).toThrow(
       'watchers for context ctx2 not found',
     );
   });

diff --git a/packages/main/src/plugin/kubernetes/contexts-informers-registry.ts b/packages/main/src/plugin/kubernetes/contexts-informers-registry.ts
@@ -16,11 +16,9 @@
  * SPDX-License-Identifier: Apache-2.0
  ***********************************************************************/
 
-import type { Informer, KubernetesObject } from '@kubernetes/client-node';
-
 import type { ResourceName } from '/@api/kubernetes-contexts-states.js';
 
-import type { ContextInternalState } from './contexts-states-registry.js';
+import type { CancellableInformer, ContextInternalState } from './contexts-states-registry.js';
 import { isSecondaryResourceName } from './contexts-states-registry.js';
 
 export class ContextsInformersRegistry {
@@ -41,7 +39,7 @@ export class ContextsInformersRegistry {
     }
   }
 
-  setResourceInformer(contextName: string, resourceName: ResourceName, informer: Informer<KubernetesObject>): void {
+  setResourceInformer(contextName: string, resourceName: ResourceName, informer: CancellableInformer): void {
     const informers = this.informers.get(contextName);
     if (!informers) {
       throw new Error(`watchers for context ${contextName} not found`);
@@ -58,7 +56,8 @@ export class ContextsInformersRegistry {
     if (informers) {
       for (const [resourceName, informer] of informers) {
         if (isSecondaryResourceName(resourceName)) {
-          await informer?.stop();
+          await informer?.informer.stop();
+          informer?.cancel();
           informers.delete(resourceName);
         }
       }
@@ -69,7 +68,8 @@ export class ContextsInformersRegistry {
     const informers = this.informers.get(name);
     if (informers) {
       for (const informer of informers.values()) {
-        await informer.stop();
+        await informer.informer.stop();
+        informer.cancel();
       }
     }
     this.informers.delete(name);

diff --git a/packages/main/src/plugin/kubernetes/contexts-manager.spec.ts b/packages/main/src/plugin/kubernetes/contexts-manager.spec.ts
@@ -24,6 +24,7 @@ import type { KubeContext } from '/@api/kubernetes-context.js';
 import type { CheckingState, ContextGeneralState, ResourceName } from '/@api/kubernetes-contexts-states.js';
 
 import type { ApiSenderType } from '../api.js';
+import type { ContextsInformersRegistry } from './contexts-informers-registry.js';
 import { ContextsManager } from './contexts-manager.js';
 import type { ContextsStatesRegistry } from './contexts-states-registry.js';
 import { informerStopMock, TestInformer } from './test-informer.js';
@@ -39,6 +40,9 @@ class TestContextsManager extends ContextsManager {
   getStates(): ContextsStatesRegistry {
     return this.states;
   }
+  getInformers(): ContextsInformersRegistry {
+    return this.informers;
+  }
 }
 
 // fakeMakeInformer describes how many resources are in the different namespaces and if cluster is reachable
@@ -946,6 +950,70 @@ describe('update', async () => {
     });
   });
 
+  test('informers should be cancellable', async () => {
+    const kubeConfig = new kubeclient.KubeConfig();
+    const config = {
+      clusters: [
+        {
+          name: 'cluster1',
+          server: 'server1',
+        },
+      ],
+      users: [
+        {
+          name: 'user1',
+        },
+      ],
+      contexts: [
+        {
+          name: 'context1',
+          cluster: 'cluster1',
+          user: 'user1',
+          namespace: 'ns1',
+        },
+      ],
+      currentContext: 'context1',
+    };
+    kubeConfig.loadFromOptions(config);
+    client = new TestContextsManager(apiSender);
+
+    vi.mocked(makeInformer).mockImplementation(
+      (
+        kubeconfig: kubeclient.KubeConfig,
+        path: string,
+        _listPromiseFn: kubeclient.ListPromise<kubeclient.KubernetesObject>,
+      ) => {
+        const connectResult = new Error('an err');
+        return new TestInformer(kubeconfig.currentContext, path, 0, connectResult, [], []);
+      },
+    );
+
+    const setStateAndDispatchMock = vi.spyOn(client.getStates(), 'setStateAndDispatch');
+    await client.update(kubeConfig);
+
+    // Initial check
+    vi.advanceTimersByTime(10);
+    expect(setStateAndDispatchMock).toHaveBeenCalled();
+    setStateAndDispatchMock.mockClear();
+
+    // No other check before next backoff tick
+    vi.advanceTimersByTime(9000);
+    expect(setStateAndDispatchMock).not.toHaveBeenCalled();
+    setStateAndDispatchMock.mockClear();
+
+    // Other check at next backoff tick
+    vi.advanceTimersByTime(2000);
+    expect(setStateAndDispatchMock).toHaveBeenCalled();
+    setStateAndDispatchMock.mockClear();
+
+    // Cancel the informers
+    await client.getInformers().deleteContextInformers('context1');
+
+    // No other checks
+    vi.advanceTimersByTime(200_000);
+    expect(setStateAndDispatchMock).not.toHaveBeenCalled();
+  });
+
   const secondaryInformers = [
     {
       resource: 'services',

diff --git a/packages/main/src/plugin/kubernetes/contexts-manager.ts b/packages/main/src/plugin/kubernetes/contexts-manager.ts
@@ -60,7 +60,7 @@ import type { ApiSenderType } from '../api.js';
 import { Backoff } from './backoff.js';
 import { backoffInitialValue, backoffJitter, backoffLimit, connectTimeout } from './contexts-constants.js';
 import { ContextsInformersRegistry } from './contexts-informers-registry.js';
-import type { ContextInternalState } from './contexts-states-registry.js';
+import type { CancellableInformer, ContextInternalState } from './contexts-states-registry.js';
 import { ContextsStatesRegistry, dispatchAllResources, isSecondaryResourceName } from './contexts-states-registry.js';
 import { ResourceWatchersRegistry } from './resource-watchers-registry.js';
 
@@ -96,7 +96,7 @@ interface CreateInformerOptions<T> {
 export class ContextsManager {
   private kubeConfig = new KubeConfig();
   protected states: ContextsStatesRegistry;
-  private informers = new ContextsInformersRegistry();
+  protected informers = new ContextsInformersRegistry();
   private currentContext: KubeContext | undefined;
   private secondaryWatchers = new ResourceWatchersRegistry();
 
@@ -305,7 +305,7 @@ export class ContextsManager {
     });
 
     const ns = context.namespace ?? 'default';
-    const result = new Map<ResourceName, Informer<KubernetesObject>>();
+    const result = new Map<ResourceName, CancellableInformer>();
     result.set('pods', this.createPodInformer(kc, ns, context));
     result.set('deployments', this.createDeploymentInformer(kc, ns, context));
     return result;
@@ -323,7 +323,7 @@ export class ContextsManager {
     }
     const kubeContext: KubeContext = this.getKubeContext(context);
     const ns = context.namespace ?? 'default';
-    let informer: Informer<KubernetesObject>;
+    let informer: CancellableInformer;
     switch (resourceName) {
       case 'services':
         informer = this.createServiceInformer(this.kubeConfig, ns, kubeContext);
@@ -364,7 +364,7 @@ export class ContextsManager {
     };
   }
 
-  private createPodInformer(kc: KubeConfig, namespace: string, context: KubeContext): Informer<V1Pod> {
+  private createPodInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer {
     const k8sApi = kc.makeApiClient(CoreV1Api);
     const listFn = (): Promise<V1PodList> => k8sApi.listNamespacedPod({ namespace });
     const path = `/api/v1/namespaces/${namespace}/pods`;
@@ -455,7 +455,7 @@ export class ContextsManager {
     });
   }
 
-  private createDeploymentInformer(kc: KubeConfig, namespace: string, context: KubeContext): Informer<V1Deployment> {
+  private createDeploymentInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer {
     const k8sApi = kc.makeApiClient(AppsV1Api);
     const listFn = (): Promise<V1DeploymentList> => k8sApi.listNamespacedDeployment({ namespace });
     const path = `/apis/apps/v1/namespaces/${namespace}/deployments`;
@@ -502,7 +502,7 @@ export class ContextsManager {
     });
   }
 
-  public createConfigMapInformer(kc: KubeConfig, namespace: string, context: KubeContext): Informer<V1ConfigMap> {
+  public createConfigMapInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer {
     const k8sApi = kc.makeApiClient(CoreV1Api);
     const listFn = (): Promise<V1ConfigMapList> => k8sApi.listNamespacedConfigMap({ namespace });
     const path = `/api/v1/namespaces/${namespace}/configmaps`;
@@ -544,7 +544,7 @@ export class ContextsManager {
     });
   }
 
-  public createSecretInformer(kc: KubeConfig, namespace: string, context: KubeContext): Informer<V1Secret> {
+  public createSecretInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer {
     const k8sApi = kc.makeApiClient(CoreV1Api);
     const listFn = (): Promise<V1SecretList> => k8sApi.listNamespacedSecret({ namespace });
     const path = `/api/v1/namespaces/${namespace}/secrets`;
@@ -588,7 +588,7 @@ export class ContextsManager {
     kc: KubeConfig,
     namespace: string,
     context: KubeContext,
-  ): Informer<V1PersistentVolumeClaim> {
+  ): CancellableInformer {
     const k8sApi = kc.makeApiClient(CoreV1Api);
     const listFn = (): Promise<V1PersistentVolumeClaimList> =>
       k8sApi.listNamespacedPersistentVolumeClaim({ namespace });
@@ -633,7 +633,7 @@ export class ContextsManager {
     });
   }
 
-  public createNodeInformer(kc: KubeConfig, _ns: string, context: KubeContext): Informer<V1Node> {
+  public createNodeInformer(kc: KubeConfig, _ns: string, context: KubeContext): CancellableInformer {
     const k8sApi = kc.makeApiClient(CoreV1Api);
     const listFn = (): Promise<V1NodeList> => k8sApi.listNode();
     const path = '/api/v1/nodes';
@@ -673,7 +673,7 @@ export class ContextsManager {
     });
   }
 
-  public createServiceInformer(kc: KubeConfig, namespace: string, context: KubeContext): Informer<V1Service> {
+  public createServiceInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer {
     const k8sApi = kc.makeApiClient(CoreV1Api);
     const listFn = (): Promise<V1ServiceList> => k8sApi.listNamespacedService({ namespace });
     const path = `/api/v1/namespaces/${namespace}/services`;
@@ -713,7 +713,7 @@ export class ContextsManager {
     });
   }
 
-  public createIngressInformer(kc: KubeConfig, namespace: string, context: KubeContext): Informer<V1Ingress> {
+  public createIngressInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer {
     const k8sNetworkingApi = this.kubeConfig.makeApiClient(NetworkingV1Api);
     const listFn = (): Promise<V1IngressList> => k8sNetworkingApi.listNamespacedIngress({ namespace });
     const path = `/apis/networking.k8s.io/v1/namespaces/${namespace}/ingresses`;
@@ -753,7 +753,7 @@ export class ContextsManager {
     });
   }
 
-  public createRouteInformer(kc: KubeConfig, namespace: string, context: KubeContext): Informer<V1Route> {
+  public createRouteInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer {
     const customObjectsApi = this.kubeConfig.makeApiClient(CustomObjectsApi);
     const listFn = (): Promise<KubernetesListObject<V1Route>> =>
       customObjectsApi.listNamespacedCustomObject({
@@ -809,7 +809,7 @@ export class ContextsManager {
     path: string,
     listPromiseFn: ListPromise<T>,
     options: CreateInformerOptions<T>,
-  ): Informer<T> {
+  ): CancellableInformer {
     const informer = makeInformer(kc, path, listPromiseFn);
 
     informer.on('add', (obj: T) => {
@@ -860,7 +860,12 @@ export class ContextsManager {
       });
     }
     this.restartInformer<T>(informer, context, options);
-    return informer;
+    return {
+      informer,
+      cancel: (): void => {
+        clearTimeout(options.timer);
+      },
+    };
   }
 
   private setReachableDelay<T extends KubernetesObject>(options: CreateInformerOptions<T>, reachable: boolean): void {

diff --git a/packages/main/src/plugin/kubernetes/contexts-states-registry.ts b/packages/main/src/plugin/kubernetes/contexts-states-registry.ts
@@ -29,8 +29,12 @@ import { NO_CURRENT_CONTEXT_ERROR, secondaryResources } from '/@api/kubernetes-c
 import type { ApiSenderType } from '../api.js';
 import { dispatchTimeout } from './contexts-constants.js';
 
+export interface CancellableInformer {
+  informer: Informer<KubernetesObject>;
+  cancel: () => void;
+}
 // ContextInternalState stores informers for a kube context
-export type ContextInternalState = Map<ResourceName, Informer<KubernetesObject>>;
+export type ContextInternalState = Map<ResourceName, CancellableInformer>;
 
 // ContextState stores information for the user about a kube context: is the cluster reachable, the number
 // of instances of different resources