From dc76e47e01499bede779a2ffda77ddcdcb89bbaf Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 16 Oct 2024 16:27:04 +0200 Subject: [PATCH 1/3] feat: make informer cancellable Signed-off-by: Philippe Martin --- .../contexts-informers-registry.spec.ts | 35 +++++++++++++--- .../kubernetes/contexts-informers-registry.ts | 12 +++--- .../src/plugin/kubernetes/contexts-manager.ts | 41 ++++++++++++------- .../kubernetes/contexts-states-registry.ts | 6 ++- 4 files changed, 68 insertions(+), 26 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.ts b/packages/main/src/plugin/kubernetes/contexts-manager.ts index f27eb6c65b29d..ee156644c6191 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'; @@ -89,6 +89,8 @@ interface CreateInformerOptions { backoff: Backoff; // used to delay setting the context reachable after the 'connect' event connectionDelay: NodeJS.Timeout | undefined; + // set to true to cancel the retries + cancel?: boolean; } // the ContextsState singleton (instantiated by the kubernetes-client singleton) @@ -305,7 +307,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 +325,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 +366,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`; @@ -449,7 +451,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`; @@ -496,7 +498,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`; @@ -538,7 +540,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`; @@ -582,7 +584,7 @@ export class ContextsManager { kc: KubeConfig, namespace: string, context: KubeContext, - ): Informer { + ): CancellableInformer { const k8sApi = kc.makeApiClient(CoreV1Api); const listFn = (): Promise => k8sApi.listNamespacedPersistentVolumeClaim({ namespace }); @@ -627,7 +629,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'; @@ -667,7 +669,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`; @@ -707,7 +709,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`; @@ -747,7 +749,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({ @@ -803,7 +805,7 @@ export class ContextsManager { path: string, listPromiseFn: ListPromise, options: CreateInformerOptions, - ): Informer { + ): CancellableInformer { const informer = makeInformer(kc, path, listPromiseFn); informer.on('add', (obj: T) => { @@ -836,6 +838,9 @@ export class ContextsManager { return; } options.timer = setTimeout(() => { + if (options.cancel) { + return; + } this.restartInformer(informer, context, options); }, nextTimeout); }); @@ -854,7 +859,12 @@ export class ContextsManager { }); } this.restartInformer(informer, context, options); - return informer; + return { + informer, + cancel: (): void => { + options.cancel = true; + }, + }; } private setReachableDelay(options: CreateInformerOptions, reachable: boolean): void { @@ -917,6 +927,9 @@ export class ContextsManager { return; } options.timer = setTimeout(() => { + if (options.cancel) { + return; + } // before restarting the failed informer we should check if the context is still present in the kubeconfig. // It is possible that if we start an informer on an old cluster it will keep failing without any chance to be stopped. // That happens bc, in the library, when executing the listFn 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 From 54b21bb54999254e738c553da8a44900700dd51c Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 16 Oct 2024 18:11:31 +0200 Subject: [PATCH 2/3] test: unit tests Signed-off-by: Philippe Martin --- .../kubernetes/contexts-manager.spec.ts | 68 +++++++++++++++++++ .../src/plugin/kubernetes/contexts-manager.ts | 2 +- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/packages/main/src/plugin/kubernetes/contexts-manager.spec.ts b/packages/main/src/plugin/kubernetes/contexts-manager.spec.ts index 28d456c29e77a..d1e0ca6227cc2 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 @@ -933,6 +937,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 ee156644c6191..07930bef3fb27 100644 --- a/packages/main/src/plugin/kubernetes/contexts-manager.ts +++ b/packages/main/src/plugin/kubernetes/contexts-manager.ts @@ -98,7 +98,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(); From 7c521fa517264da87c820e02acb19c382d896d2b Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 16 Oct 2024 19:57:28 +0200 Subject: [PATCH 3/3] fix: cancel timer directly Signed-off-by: Philippe Martin --- .../main/src/plugin/kubernetes/contexts-manager.ts | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/packages/main/src/plugin/kubernetes/contexts-manager.ts b/packages/main/src/plugin/kubernetes/contexts-manager.ts index 07930bef3fb27..e892250ef9d54 100644 --- a/packages/main/src/plugin/kubernetes/contexts-manager.ts +++ b/packages/main/src/plugin/kubernetes/contexts-manager.ts @@ -89,8 +89,6 @@ interface CreateInformerOptions { backoff: Backoff; // used to delay setting the context reachable after the 'connect' event connectionDelay: NodeJS.Timeout | undefined; - // set to true to cancel the retries - cancel?: boolean; } // the ContextsState singleton (instantiated by the kubernetes-client singleton) @@ -838,9 +836,6 @@ export class ContextsManager { return; } options.timer = setTimeout(() => { - if (options.cancel) { - return; - } this.restartInformer(informer, context, options); }, nextTimeout); }); @@ -862,7 +857,7 @@ export class ContextsManager { return { informer, cancel: (): void => { - options.cancel = true; + clearTimeout(options.timer); }, }; } @@ -927,9 +922,6 @@ export class ContextsManager { return; } options.timer = setTimeout(() => { - if (options.cancel) { - return; - } // before restarting the failed informer we should check if the context is still present in the kubeconfig. // It is possible that if we start an informer on an old cluster it will keep failing without any chance to be stopped. // That happens bc, in the library, when executing the listFn