From 484d68d72b4d3ca49a446ba570386e1ffaa56624 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 18 Oct 2024 06:48:33 +0200 Subject: [PATCH] feat: make Kubernetes informers cancellable (#9411) * feat: make informer cancellable Signed-off-by: Philippe Martin * test: unit tests Signed-off-by: Philippe Martin * fix: cancel timer directly Signed-off-by: Philippe Martin --- .../contexts-informers-registry.spec.ts | 35 ++++++++-- .../kubernetes/contexts-informers-registry.ts | 12 ++-- .../kubernetes/contexts-manager.spec.ts | 68 +++++++++++++++++++ .../src/plugin/kubernetes/contexts-manager.ts | 35 ++++++---- .../kubernetes/contexts-states-registry.ts | 6 +- 5 files changed, 129 insertions(+), 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 index 53235637743c5..b98b018e610c7 100644 --- a/packages/main/src/plugin/kubernetes/contexts-informers-registry.spec.ts +++ b/packages/main/src/plugin/kubernetes/contexts-informers-registry.spec.ts @@ -20,6 +20,7 @@ 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', () => { @@ -27,7 +28,15 @@ 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 => {}, + }, + ], + ]), ); 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); + 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)).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 index db3d60c4264ed..d8ff8411255d2 100644 --- 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): 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 index de3e1744a58ff..1b5a1fcebc1e9 100644 --- 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, + ) => { + 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 index 72678b680e83e..5396c335102dc 100644 --- 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 { 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>(); + const result = new Map(); 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; + 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 { + private createPodInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer { const k8sApi = kc.makeApiClient(CoreV1Api); const listFn = (): Promise => 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 { + private createDeploymentInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer { const k8sApi = kc.makeApiClient(AppsV1Api); const listFn = (): Promise => 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 { + public createConfigMapInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer { const k8sApi = kc.makeApiClient(CoreV1Api); const listFn = (): Promise => 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 { + public createSecretInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer { const k8sApi = kc.makeApiClient(CoreV1Api); const listFn = (): Promise => k8sApi.listNamespacedSecret({ namespace }); const path = `/api/v1/namespaces/${namespace}/secrets`; @@ -588,7 +588,7 @@ export class ContextsManager { kc: KubeConfig, namespace: string, context: KubeContext, - ): Informer { + ): CancellableInformer { const k8sApi = kc.makeApiClient(CoreV1Api); const listFn = (): Promise => k8sApi.listNamespacedPersistentVolumeClaim({ namespace }); @@ -633,7 +633,7 @@ export class ContextsManager { }); } - public createNodeInformer(kc: KubeConfig, _ns: string, context: KubeContext): Informer { + public createNodeInformer(kc: KubeConfig, _ns: string, context: KubeContext): CancellableInformer { const k8sApi = kc.makeApiClient(CoreV1Api); const listFn = (): Promise => k8sApi.listNode(); const path = '/api/v1/nodes'; @@ -673,7 +673,7 @@ export class ContextsManager { }); } - public createServiceInformer(kc: KubeConfig, namespace: string, context: KubeContext): Informer { + public createServiceInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer { const k8sApi = kc.makeApiClient(CoreV1Api); const listFn = (): Promise => 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 { + public createIngressInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer { const k8sNetworkingApi = this.kubeConfig.makeApiClient(NetworkingV1Api); const listFn = (): Promise => 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 { + public createRouteInformer(kc: KubeConfig, namespace: string, context: KubeContext): CancellableInformer { const customObjectsApi = this.kubeConfig.makeApiClient(CustomObjectsApi); const listFn = (): Promise> => customObjectsApi.listNamespacedCustomObject({ @@ -809,7 +809,7 @@ export class ContextsManager { path: string, listPromiseFn: ListPromise, options: CreateInformerOptions, - ): Informer { + ): CancellableInformer { const informer = makeInformer(kc, path, listPromiseFn); informer.on('add', (obj: T) => { @@ -860,7 +860,12 @@ export class ContextsManager { }); } this.restartInformer(informer, context, options); - return informer; + return { + informer, + cancel: (): void => { + clearTimeout(options.timer); + }, + }; } private setReachableDelay(options: CreateInformerOptions, 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 index 881799b76f89e..c70a5096aff8f 100644 --- 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; + cancel: () => void; +} // ContextInternalState stores informers for a kube context -export type ContextInternalState = Map>; +export type ContextInternalState = Map; // ContextState stores information for the user about a kube context: is the cluster reachable, the number // of instances of different resources