Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: make Kubernetes informers cancellable #9411

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+129 −27
Open

feat: make Kubernetes informers cancellable #9411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc76e47
feat: make informer cancellable
feloy Oct 16, 2024
54b21bb
test: unit tests
feloy Oct 16, 2024
7c521fa
fix: cancel timer directly
feloy Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions packages/main/src/plugin/kubernetes/contexts-informers-registry.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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']);
});
Expand All @@ -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',
);
});
Expand Down
12 changes: 6 additions & 6 deletions packages/main/src/plugin/kubernetes/contexts-informers-registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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`);
Expand All @@ -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);
}
}
Expand All @@ -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);
Expand Down
68 changes: 68 additions & 0 deletions packages/main/src/plugin/kubernetes/contexts-manager.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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
Expand Down Expand Up @@ -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<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',
Expand Down
35 changes: 20 additions & 15 deletions packages/main/src/plugin/kubernetes/contexts-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we losing the generic type, is it important ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check again, does it does not seem to be useful

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so:

The V1Pod is necessary when calling this.createInformer<V1Pod>, to have he onAdd and other options have the matching type. But the result of createPodInformer is stored in an Informer<KubernetesObject>; the specific type is never used on this side.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okey thanks

const k8sApi = kc.makeApiClient(CoreV1Api);
const listFn = (): Promise<V1PodList> => k8sApi.listNamespacedPod({ namespace });
const path = `/api/v1/namespaces/${namespace}/pods`;
Expand Down Expand Up @@ -449,7 +449,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`;
Expand Down Expand Up @@ -496,7 +496,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`;
Expand Down Expand Up @@ -538,7 +538,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`;
Expand Down Expand Up @@ -582,7 +582,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 });
Expand Down Expand Up @@ -627,7 +627,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';
Expand Down Expand Up @@ -667,7 +667,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`;
Expand Down Expand Up @@ -707,7 +707,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`;
Expand Down Expand Up @@ -747,7 +747,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({
Expand Down Expand Up @@ -803,7 +803,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) => {
Expand Down Expand Up @@ -854,7 +854,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 {
Expand Down
6 changes: 5 additions & 1 deletion packages/main/src/plugin/kubernetes/contexts-states-registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down