From 674d964f0727efbe95de8e2de155f34ed93a4adc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20Dombya?= <135591453+hervedombya@users.noreply.github.com> Date: Thu, 20 Mar 2025 18:09:55 +0100 Subject: [PATCH 1/4] replace token in Metalk8sLocalVolumeProvider by getToken --- .../k8s/Metalk8sLocalVolumeProvider.test.ts | 4 +-- .../k8s/Metalk8sLocalVolumeProvider.ts | 26 ++++++++++++------- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/ui/src/services/k8s/Metalk8sLocalVolumeProvider.test.ts b/ui/src/services/k8s/Metalk8sLocalVolumeProvider.test.ts index 88c7d7fed3..5b32e118d6 100644 --- a/ui/src/services/k8s/Metalk8sLocalVolumeProvider.test.ts +++ b/ui/src/services/k8s/Metalk8sLocalVolumeProvider.test.ts @@ -12,7 +12,7 @@ jest.mock('../k8s/api', () => ({ describe('Metalk8sLocalVolumeProvider', () => { let provider: Metalk8sLocalVolumeProvider; const mockUrl = 'mock-url'; - const mockToken = 'mock-token'; + const mockToken = jest.fn(() => Promise.resolve('mock-token')); const mockCustomObjectsApi = { listClusterCustomObject: jest.fn(), @@ -39,8 +39,6 @@ describe('Metalk8sLocalVolumeProvider', () => { }); provider = new Metalk8sLocalVolumeProvider(mockUrl, mockToken); - provider.k8sClient = mockCoreV1Api; - provider.volumeClient = mockVolumeClient; }); describe('listLocalPersistentVolumes', () => { diff --git a/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts b/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts index 851ed67b22..4249e2d792 100644 --- a/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts +++ b/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts @@ -1,4 +1,4 @@ -import { CoreV1Api, V1PersistentVolume } from '@kubernetes/client-node'; +import { V1PersistentVolume } from '@kubernetes/client-node'; import * as ApiK8s from './api'; import { Metalk8sV1alpha1VolumeClient, @@ -22,18 +22,24 @@ export type LocalPersistentVolume = V1PersistentVolume & { }; export default class Metalk8sLocalVolumeProvider { - volumeClient: Metalk8sV1alpha1VolumeClient; - k8sClient: CoreV1Api; - constructor(url: string, token: string) { - const { coreV1, customObjects } = ApiK8s.updateApiServerConfig(url, token); - this.volumeClient = new Metalk8sV1alpha1VolumeClient(customObjects); - this.k8sClient = coreV1; + apiUrl: string; + constructor(apiUrl: string, private getToken: () => Promise) { + this.apiUrl = apiUrl; } + public listLocalPersistentVolumes = async ( serverName: string, ): Promise => { try { - const nodes = await this.k8sClient.listNode(); + const token = await this.getToken(); + const { coreV1, customObjects } = ApiK8s.updateApiServerConfig( + this.apiUrl, + token, + ); + const volumeClient = new Metalk8sV1alpha1VolumeClient(customObjects); + const k8sClient = coreV1; + + const nodes = await k8sClient.listNode(); const nodeIP = nodes.body.items .find((node) => node.metadata.name === serverName) ?.status.addresses.find((address) => address.type === 'InternalIP'); @@ -42,13 +48,13 @@ export default class Metalk8sLocalVolumeProvider { throw new Error(`Failed to find IP for node ${serverName}`); } - const volumes = await this.volumeClient.getMetalk8sV1alpha1VolumeList(); + const volumes = await volumeClient.getMetalk8sV1alpha1VolumeList(); if (!isError(volumes)) { const nodeVolumes = volumes.body.items.filter( (volume) => volume.spec.nodeName === serverName, ); - const pv = await this.k8sClient.listPersistentVolume(); + const pv = await k8sClient.listPersistentVolume(); const localPv = nodeVolumes.reduce((acc, item) => { const isLocalPv = pv.body.items.find( From 3f80335efc7713700737e33ba1ded88cd589816e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20Dombya?= <135591453+hervedombya@users.noreply.github.com> Date: Thu, 27 Mar 2025 15:08:14 +0100 Subject: [PATCH 2/4] Use volumeClient from updated ApiK8s configuration in delete volume method --- ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts b/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts index 4249e2d792..5bf3f443b8 100644 --- a/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts +++ b/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts @@ -93,10 +93,13 @@ export default class Metalk8sLocalVolumeProvider { ): Promise => { // The volume name is the same as the PV name const volumeNames = localPVs.map((localPV) => localPV.metadata.name); + const token = await this.getToken(); + const { customObjects } = ApiK8s.updateApiServerConfig(this.apiUrl, token); + const volumeClient = new Metalk8sV1alpha1VolumeClient(customObjects); for (const volumeName of volumeNames) { try { - await this.volumeClient.deleteMetalk8sV1alpha1Volume(volumeName); + await volumeClient.deleteMetalk8sV1alpha1Volume(volumeName); } catch (error) { throw new Error( `Failed to delete MetalK8s volume ${volumeName}: ${ From 837dae4bffbcc34561af306eb3df4d3ef60f12b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20Dombya?= <135591453+hervedombya@users.noreply.github.com> Date: Thu, 27 Mar 2025 16:33:42 +0100 Subject: [PATCH 3/4] Refactor volume deletion logic to use CustomObjectsApi and improve error handling --- .../k8s/Metalk8sLocalVolumeProvider.test.ts | 44 +++++++++++-------- .../k8s/Metalk8sLocalVolumeProvider.ts | 12 ++++- 2 files changed, 36 insertions(+), 20 deletions(-) diff --git a/ui/src/services/k8s/Metalk8sLocalVolumeProvider.test.ts b/ui/src/services/k8s/Metalk8sLocalVolumeProvider.test.ts index 5b32e118d6..1d869f0191 100644 --- a/ui/src/services/k8s/Metalk8sLocalVolumeProvider.test.ts +++ b/ui/src/services/k8s/Metalk8sLocalVolumeProvider.test.ts @@ -1,14 +1,17 @@ import { CoreV1Api, CustomObjectsApi } from '@kubernetes/client-node'; +import { updateApiServerConfig } from './api'; import Metalk8sLocalVolumeProvider, { VolumeType, } from './Metalk8sLocalVolumeProvider'; -import { updateApiServerConfig } from './api'; -import { Metalk8sV1alpha1VolumeClient } from './Metalk8sVolumeClient.generated'; jest.mock('../k8s/api', () => ({ updateApiServerConfig: jest.fn(), })); +const MOCK_GROUP = 'storage.metalk8s.scality.com'; +const MOCK_VERSION = 'v1alpha1'; +const MOCK_PLURAL = 'volumes'; + describe('Metalk8sLocalVolumeProvider', () => { let provider: Metalk8sLocalVolumeProvider; const mockUrl = 'mock-url'; @@ -16,16 +19,9 @@ describe('Metalk8sLocalVolumeProvider', () => { const mockCustomObjectsApi = { listClusterCustomObject: jest.fn(), + deleteClusterCustomObject: jest.fn(), } as unknown as CustomObjectsApi; - const mockVolumeClient = { - deleteMetalk8sV1alpha1Volume: jest.fn().mockResolvedValue({ body: {} }), - getMetalk8sV1alpha1VolumeList: jest.fn(), - getMetalk8sV1alpha1Volume: jest.fn(), - createMetalk8sV1alpha1Volume: jest.fn(), - patchMetalk8sV1alpha1Volume: jest.fn(), - } as unknown as Metalk8sV1alpha1VolumeClient; - const mockCoreV1Api = { listNode: jest.fn(), listPersistentVolume: jest.fn(), @@ -71,7 +67,7 @@ describe('Metalk8sLocalVolumeProvider', () => { }); ( - mockVolumeClient.getMetalk8sV1alpha1VolumeList as jest.Mock + mockCustomObjectsApi.listClusterCustomObject as jest.Mock ).mockResolvedValue({ body: { items: [ @@ -153,7 +149,7 @@ describe('Metalk8sLocalVolumeProvider', () => { }); ( - mockVolumeClient.getMetalk8sV1alpha1VolumeList as jest.Mock + mockCustomObjectsApi.listClusterCustomObject as jest.Mock ).mockRejectedValue(new Error('Failed to fetch volumes')); await expect( @@ -168,7 +164,7 @@ describe('Metalk8sLocalVolumeProvider', () => { it('should detach hardware volumes and virtual volumes', async () => { //S ( - mockVolumeClient.deleteMetalk8sV1alpha1Volume as jest.Mock + mockCustomObjectsApi.deleteClusterCustomObject as jest.Mock ).mockResolvedValue({ body: {}, }); @@ -191,17 +187,29 @@ describe('Metalk8sLocalVolumeProvider', () => { ]); //V expect( - mockVolumeClient.deleteMetalk8sV1alpha1Volume, - ).toHaveBeenCalledWith('test-volume'); + mockCustomObjectsApi.deleteClusterCustomObject, + ).toHaveBeenCalledWith( + MOCK_GROUP, + MOCK_VERSION, + MOCK_PLURAL, + 'test-volume', + {}, + ); expect( - mockVolumeClient.deleteMetalk8sV1alpha1Volume, - ).toHaveBeenCalledWith('test-lvm'); + mockCustomObjectsApi.deleteClusterCustomObject, + ).toHaveBeenCalledWith( + MOCK_GROUP, + MOCK_VERSION, + MOCK_PLURAL, + 'test-lvm', + {}, + ); }); it('should raise an error if metalk8s volume deletion fails', async () => { //S ( - mockVolumeClient.deleteMetalk8sV1alpha1Volume as jest.Mock + mockCustomObjectsApi.deleteClusterCustomObject as jest.Mock ).mockRejectedValue(new Error('Failed to delete metalk8s volume')); //E+V await expect( diff --git a/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts b/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts index 5bf3f443b8..25de30767d 100644 --- a/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts +++ b/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts @@ -78,7 +78,7 @@ export default class Metalk8sLocalVolumeProvider { return localPv; } else { - throw new Error(`Failed to fetch metalk8s volumes: ${volumes.error}`); + throw new Error(`${volumes.error.message}`); } } catch (error) { throw new Error( @@ -99,7 +99,15 @@ export default class Metalk8sLocalVolumeProvider { for (const volumeName of volumeNames) { try { - await volumeClient.deleteMetalk8sV1alpha1Volume(volumeName); + const deleteVolume = await volumeClient.deleteMetalk8sV1alpha1Volume( + volumeName, + ); + + if (isError(deleteVolume)) { + throw new Error( + `Failed to delete MetalK8s volume ${volumeName}: ${deleteVolume.error.message}`, + ); + } } catch (error) { throw new Error( `Failed to delete MetalK8s volume ${volumeName}: ${ From b4427313228413d088dde59157d9152ede438878 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20Dombya?= <135591453+hervedombya@users.noreply.github.com> Date: Thu, 27 Mar 2025 18:09:38 +0100 Subject: [PATCH 4/4] Refactor volume deletion to retrieve token and client within the loop --- ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts b/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts index 25de30767d..cb8e914d54 100644 --- a/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts +++ b/ui/src/services/k8s/Metalk8sLocalVolumeProvider.ts @@ -93,11 +93,15 @@ export default class Metalk8sLocalVolumeProvider { ): Promise => { // The volume name is the same as the PV name const volumeNames = localPVs.map((localPV) => localPV.metadata.name); - const token = await this.getToken(); - const { customObjects } = ApiK8s.updateApiServerConfig(this.apiUrl, token); - const volumeClient = new Metalk8sV1alpha1VolumeClient(customObjects); for (const volumeName of volumeNames) { + const token = await this.getToken(); + const { customObjects } = ApiK8s.updateApiServerConfig( + this.apiUrl, + token, + ); + const volumeClient = new Metalk8sV1alpha1VolumeClient(customObjects); + try { const deleteVolume = await volumeClient.deleteMetalk8sV1alpha1Volume( volumeName,