Skip to content

Commit b880467

Browse files
katieperry4katieperry4
authored
Upgrade cluster storage modal (opendatahub-io#4457)
* upgrade cluster storage to include model context * remove isGeneralPurpose useEffect * remove redundant annotation setting
1 parent a65be2a commit b880467

File tree

12 files changed

+448
-13
lines changed

12 files changed

+448
-13
lines changed

‎frontend/src/__mocks__/mockPVCK8sResource.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ type MockResourceConfigType = {
1111
uid?: string;
1212
status?: PersistentVolumeClaimKind['status'];
1313
accessModes?: AccessMode[];
14+
annotations?: Record<string, string>;
1415
};
1516

1617
export const mockPVCK8sResource = ({
@@ -28,13 +29,15 @@ export const mockPVCK8sResource = ({
2829
},
2930
},
3031
accessModes = [AccessMode.RWO],
32+
annotations = {},
3133
}: MockResourceConfigType): PersistentVolumeClaimKind => ({
3234
kind: 'PersistentVolumeClaim',
3335
apiVersion: 'v1',
3436
metadata: {
3537
annotations: {
3638
'openshift.io/description': '',
3739
'openshift.io/display-name': displayName,
40+
...annotations,
3841
},
3942
name,
4043
namespace,

‎frontend/src/__tests__/cypress/cypress/pages/clusterStorage.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,10 @@ class ClusterStorageRow extends TableRow {
3333
return this.find().find('[data-label="Storage class"]');
3434
}
3535

36+
findStorageTypeColumn() {
37+
return this.find().find('[data-label="Type"]');
38+
}
39+
3640
findSizeColumn() {
3741
return this.find().find('[data-label="Storage size"]');
3842
}
@@ -216,6 +220,22 @@ class ClusterStorageModal extends Modal {
216220
findExistingAccessMode() {
217221
return this.find().findByTestId('existing-access-mode');
218222
}
223+
224+
findModelNameInput() {
225+
return this.find().findByTestId('model-name-input');
226+
}
227+
228+
findModelPathInput() {
229+
return this.find().findByTestId('model-path-input');
230+
}
231+
232+
findGeneralPurposeRadio() {
233+
return this.find().findByTestId('general-purpose-radio');
234+
}
235+
236+
findModelStorageRadio() {
237+
return this.find().findByTestId('model-storage-radio');
238+
}
219239
}
220240

221241
class ClusterStorage {

‎frontend/src/__tests__/cypress/cypress/tests/mocked/projects/tabs/clusterStorage.cy.ts‎

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import { mock200Status } from '#~/__mocks__/mockK8sStatus';
2929
import { mockPrometheusQueryResponse } from '#~/__mocks__/mockPrometheusQueryResponse';
3030
import { storageClassesPage } from '#~/__tests__/cypress/cypress/pages/storageClasses';
3131
import { AccessMode } from '#~/__tests__/cypress/cypress/types';
32+
import { PvcModelAnnotation } from '#~/pages/projects/screens/spawner/storage/types';
3233

3334
type HandlersProps = {
3435
isEmpty?: boolean;
@@ -79,6 +80,15 @@ const initInterceptors = ({ isEmpty = false, storageClassName }: HandlersProps)
7980
],
8081
},
8182
}),
83+
mockPVCK8sResource({
84+
displayName: 'Model Storage',
85+
storageClassName,
86+
storage: '5Gi',
87+
annotations: {
88+
[PvcModelAnnotation.MODEL_NAME]: 'name',
89+
[PvcModelAnnotation.MODEL_PATH]: 'path/example',
90+
},
91+
}),
8292
],
8393
),
8494
);
@@ -346,6 +356,89 @@ describe('ClusterStorage', () => {
346356
});
347357
});
348358

359+
it('Add cluster storage with model storage', () => {
360+
cy.interceptOdh(
361+
'GET /api/config',
362+
mockDashboardConfig({
363+
disablePVCServing: false,
364+
}),
365+
);
366+
initInterceptors({ isEmpty: true });
367+
storageClassesPage.mockGetStorageClasses([
368+
openshiftDefaultStorageClass,
369+
buildMockStorageClass(otherStorageClass, { isEnabled: true }),
370+
]);
371+
cy.interceptK8s('POST', { model: PVCModel, ns: 'test-project' }, mockPVCK8sResource({})).as(
372+
'addClusterStorage',
373+
);
374+
clusterStorage.visit('test-project');
375+
clusterStorage.findCreateButton().click();
376+
addClusterStorageModal.findNameInput().fill('test-storage');
377+
addClusterStorageModal.findModelStorageRadio().click();
378+
addClusterStorageModal.findModelNameInput().fill('name');
379+
addClusterStorageModal.findModelPathInput().fill('path/example');
380+
addClusterStorageModal.findSubmitButton().click();
381+
382+
cy.wait('@addClusterStorage').then((interception) => {
383+
expect(interception.request.url).to.include('?dryRun=All');
384+
expect(interception.request.body).to.containSubset({
385+
metadata: {
386+
annotations: {
387+
[PvcModelAnnotation.MODEL_NAME]: 'name',
388+
[PvcModelAnnotation.MODEL_PATH]: 'path/example',
389+
},
390+
},
391+
});
392+
});
393+
394+
cy.wait('@addClusterStorage').then((interception) => {
395+
expect(interception.request.url).not.to.include('?dryRun=All');
396+
});
397+
398+
cy.get('@addClusterStorage.all').then((interceptions) => {
399+
expect(interceptions).to.have.length(2);
400+
});
401+
});
402+
403+
it('Should list correct storage type', () => {
404+
initInterceptors({});
405+
storageClassesPage.mockGetStorageClasses([
406+
openshiftDefaultStorageClass,
407+
buildMockStorageClass(otherStorageClass, { isEnabled: true }),
408+
]);
409+
cy.interceptOdh(
410+
'GET /api/config',
411+
mockDashboardConfig({
412+
disablePVCServing: false,
413+
}),
414+
);
415+
clusterStorage.visit('test-project');
416+
const modelStorageRow = clusterStorage.getClusterStorageRow('Model Storage');
417+
modelStorageRow.findStorageTypeColumn().should('contain.text', 'Model storage');
418+
const genericStorageRow = clusterStorage.getClusterStorageRow('Test Storage');
419+
genericStorageRow.findStorageTypeColumn().should('contain.text', 'Generic persistent storage');
420+
});
421+
422+
it('Should prefill model storage pvc with name and path on edit', () => {
423+
initInterceptors({});
424+
storageClassesPage.mockGetStorageClasses([
425+
openshiftDefaultStorageClass,
426+
buildMockStorageClass(otherStorageClass, { isEnabled: true }),
427+
]);
428+
cy.interceptOdh(
429+
'GET /api/config',
430+
mockDashboardConfig({
431+
disablePVCServing: false,
432+
}),
433+
);
434+
clusterStorage.visit('test-project');
435+
const modelStorageRow = clusterStorage.getClusterStorageRow('Model Storage');
436+
modelStorageRow.findKebabAction('Edit storage').click();
437+
updateClusterStorageModal.findModelStorageRadio().should('be.checked');
438+
updateClusterStorageModal.findModelNameInput().should('have.value', 'name');
439+
updateClusterStorageModal.findModelPathInput().should('have.value', 'path/example');
440+
});
441+
349442
it('list cluster storage and Table sorting', () => {
350443
cy.interceptOdh(
351444
'GET /api/config',
@@ -357,7 +450,7 @@ describe('ClusterStorage', () => {
357450
clusterStorage.visit('test-project');
358451
const clusterStorageRow = clusterStorage.getClusterStorageRow('Test Storage');
359452
clusterStorageRow.findStorageClassColumn().should('not.exist');
360-
clusterStorageRow.shouldHaveStorageTypeValue('Persistent storage');
453+
clusterStorageRow.shouldHaveStorageTypeValue('Generic persistent storage');
361454
clusterStorageRow.findConnectedWorkbenches().should('have.text', 'No connections');
362455
clusterStorageRow.findSizeColumn().contains('5GiB');
363456

‎frontend/src/api/k8s/__tests__/pvcs.spec.ts‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import { PVCModel } from '#~/api/models/k8s';
2020
import { PersistentVolumeClaimKind } from '#~/k8sTypes';
2121
import { StorageData } from '#~/pages/projects/types';
2222
import { AccessMode } from '#~/pages/storageClasses/storageEnums';
23+
import { PvcModelAnnotation } from '#~/pages/projects/screens/spawner/storage/types';
2324

2425
jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({
2526
k8sGetResource: jest.fn(),
@@ -91,6 +92,27 @@ describe('assemblePvc', () => {
9192
spec: { ...assemblePvcResult.spec, accessModes: [AccessMode.RWOP] },
9293
});
9394
});
95+
it('should assemble pvc with model annotations', () => {
96+
const result = assemblePvc(
97+
{ ...data, modelName: 'model-name', modelPath: 'model-path' },
98+
'namespace',
99+
'editName',
100+
);
101+
expect(result).toStrictEqual({
102+
...assemblePvcResult,
103+
metadata: {
104+
...assemblePvcResult.metadata,
105+
name: 'editName',
106+
annotations: {
107+
[PvcModelAnnotation.MODEL_NAME]: 'model-name',
108+
[PvcModelAnnotation.MODEL_PATH]: 'model-path',
109+
'openshift.io/description': 'Test Storage',
110+
'openshift.io/display-name': 'pvc',
111+
},
112+
},
113+
spec: { ...assemblePvcResult.spec, accessModes: [AccessMode.RWO] },
114+
});
115+
});
94116
});
95117

96118
describe('getDashboardPvcs', () => {
@@ -174,6 +196,111 @@ describe('updatePvc', () => {
174196
});
175197
});
176198

199+
it('should update pvc and remove model annotations', async () => {
200+
const existingPvc = mockPVCK8sResource({
201+
name: 'pvc',
202+
namespace: 'namespace',
203+
storage: '5Gi',
204+
storageClassName: 'standard-csi',
205+
displayName: 'Old Storage',
206+
});
207+
existingPvc.metadata.annotations = {
208+
[PvcModelAnnotation.MODEL_NAME]: 'model-name',
209+
[PvcModelAnnotation.MODEL_PATH]: 'model-path',
210+
};
211+
const storageData: StorageData = {
212+
name: 'pvc',
213+
size: '5Gi',
214+
modelName: '',
215+
modelPath: '',
216+
};
217+
218+
k8sUpdateResourceMock.mockResolvedValue(existingPvc);
219+
220+
await updatePvc(storageData, existingPvc, 'namespace');
221+
222+
const expectedPvc = {
223+
...existingPvc,
224+
metadata: {
225+
...existingPvc.metadata,
226+
annotations: {
227+
'openshift.io/display-name': 'pvc',
228+
},
229+
},
230+
spec: {
231+
...existingPvc.spec,
232+
resources: {
233+
requests: {
234+
storage: '5Gi',
235+
},
236+
},
237+
},
238+
status: {
239+
...existingPvc.status,
240+
phase: 'Pending',
241+
},
242+
};
243+
244+
expect(k8sUpdateResourceMock).toHaveBeenCalledWith({
245+
model: PVCModel,
246+
fetchOptions: { requestInit: {} },
247+
queryOptions: { queryParams: {} },
248+
resource: expectedPvc,
249+
});
250+
});
251+
252+
it('should update pvc and add model annotations', async () => {
253+
const existingPvc = mockPVCK8sResource({
254+
name: 'pvc',
255+
namespace: 'namespace',
256+
storage: '5Gi',
257+
storageClassName: 'standard-csi',
258+
displayName: 'Old Storage',
259+
});
260+
existingPvc.metadata.annotations = {
261+
'openshift.io/description': 'Test Storage',
262+
};
263+
const storageData: StorageData = {
264+
name: 'pvc',
265+
size: '5Gi',
266+
modelName: 'model-name',
267+
modelPath: 'model-path',
268+
description: 'Test Storage',
269+
};
270+
k8sUpdateResourceMock.mockResolvedValue(existingPvc);
271+
await updatePvc(storageData, existingPvc, 'namespace');
272+
const expectedPvc = {
273+
...existingPvc,
274+
metadata: {
275+
...existingPvc.metadata,
276+
annotations: {
277+
'openshift.io/description': 'Test Storage',
278+
'openshift.io/display-name': 'pvc',
279+
[PvcModelAnnotation.MODEL_NAME]: 'model-name',
280+
[PvcModelAnnotation.MODEL_PATH]: 'model-path',
281+
},
282+
},
283+
spec: {
284+
...existingPvc.spec,
285+
resources: {
286+
requests: {
287+
storage: '5Gi',
288+
},
289+
},
290+
},
291+
status: {
292+
...existingPvc.status,
293+
phase: 'Pending',
294+
},
295+
};
296+
expect(k8sUpdateResourceMock).toHaveBeenCalledWith({
297+
model: PVCModel,
298+
fetchOptions: { requestInit: {} },
299+
queryOptions: { queryParams: {} },
300+
resource: expectedPvc,
301+
});
302+
});
303+
177304
it('should update pvc and remove spec when excludeSpec is true', async () => {
178305
const existingPvc = mockPVCK8sResource({
179306
name: 'Old name',

‎frontend/src/api/k8s/pvcs.ts‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { LABEL_SELECTOR_DASHBOARD_RESOURCE } from '#~/const';
1414
import { applyK8sAPIOptions } from '#~/api/apiMergeUtils';
1515
import { StorageData } from '#~/pages/projects/types';
1616
import { AccessMode } from '#~/pages/storageClasses/storageEnums';
17+
import { PvcModelAnnotation } from '#~/pages/projects/screens/spawner/storage/types';
1718

1819
export const assemblePvc = (
1920
data: StorageData,
@@ -22,12 +23,22 @@ export const assemblePvc = (
2223
hideFromUI?: boolean,
2324
additionalAnnotations?: Record<string, string>, // Generic alternative to forceRedeploy
2425
): PersistentVolumeClaimKind => {
25-
const { name: pvcName, description, size, storageClassName, accessMode } = data;
26+
const {
27+
name: pvcName,
28+
description,
29+
size,
30+
storageClassName,
31+
accessMode,
32+
modelName,
33+
modelPath,
34+
} = data;
2635
const name = editName || data.k8sName || translateDisplayNameForK8s(pvcName);
2736

2837
const annotations: Record<string, string> = {
2938
'openshift.io/display-name': pvcName.trim(),
3039
...(description && { 'openshift.io/description': description }),
40+
...(modelName && { [PvcModelAnnotation.MODEL_NAME]: modelName }),
41+
...(modelPath && { [PvcModelAnnotation.MODEL_PATH]: modelPath }),
3142
...(additionalAnnotations || {}),
3243
};
3344

@@ -98,6 +109,7 @@ export const updatePvc = (
98109
undefined,
99110
additionalAnnotations,
100111
);
112+
101113
const newData = excludeSpec
102114
? {
103115
...pvc,
@@ -116,6 +128,14 @@ export const updatePvc = (
116128
pvcResource.metadata.annotations['openshift.io/description'] = undefined;
117129
}
118130

131+
if (pvcResource.metadata.annotations) {
132+
if (!data.modelName) {
133+
delete pvcResource.metadata.annotations[PvcModelAnnotation.MODEL_NAME];
134+
}
135+
if (!data.modelPath) {
136+
delete pvcResource.metadata.annotations[PvcModelAnnotation.MODEL_PATH];
137+
}
138+
}
119139
return k8sUpdateResource<PersistentVolumeClaimKind>(
120140
applyK8sAPIOptions({ model: PVCModel, resource: pvcResource }, opts),
121141
);

0 commit comments

Comments
 (0)