[RHOAIENG-1101] Cannot remove additional storage from the workbench c…

…onfiguration
opendatahub-io · Nov 7, 2024 · 5f829bd · 5f829bd
1 parent 0e4420f
commit 5f829bd
Show file tree

Hide file tree

Showing 31 changed files with 703 additions and 482 deletions.
diff --git a/frontend/src/__mocks__/mockStartNotebookData.ts b/frontend/src/__mocks__/mockStartNotebookData.ts
@@ -94,18 +94,15 @@ export const mockStartNotebookData = ({
   },
 });
 
-export const mockStorageData: StorageData = {
-  storageType: StorageType.NEW_PVC,
-  creating: {
-    nameDesc: {
-      name: 'test-pvc',
-      description: '',
-    },
+export const mockStorageData: StorageData[] = [
+  {
+    storageType: StorageType.NEW_PVC,
+    name: 'test-pvc',
+    description: '',
     size: '20Gi',
     storageClassName: 'gp2-csi',
   },
-  existing: { storage: '' },
-};
+];
 
 export const mockDataConnectionData: DataConnectionData = {
   type: 'creating',

diff --git a/frontend/src/__tests__/cypress/cypress/pages/workbench.ts b/frontend/src/__tests__/cypress/cypress/pages/workbench.ts
@@ -202,6 +202,39 @@ class AttachConnectionModal extends Modal {
   }
 }
 
+class StorageTableRow extends TableRow {
+  private findByDataLabel(label: string) {
+    return this.find().find(`[data-label="${label}"]`);
+  }
+
+  findNameValue() {
+    return this.findByDataLabel('Name');
+  }
+
+  findStorageSizeValue() {
+    return this.findByDataLabel('Storage size');
+  }
+
+  findMountPathValue() {
+    return this.findByDataLabel('Mount path');
+  }
+}
+
+class StorageTable {
+  find() {
+    return cy.findByTestId('cluster-storage-table');
+  }
+
+  getRowById(id: number) {
+    return new StorageTableRow(
+      () =>
+        this.find().findByTestId(['cluster-storage-table-row', id]) as unknown as Cypress.Chainable<
+          JQuery<HTMLTableRowElement>
+        >,
+    );
+  }
+}
+
 class CreateSpawnerPage {
   k8sNameDescription = new K8sNameDescriptionField('workbench');
 
@@ -210,16 +243,8 @@ class CreateSpawnerPage {
     return this;
   }
 
-  findNewStorageRadio() {
-    return cy.findByTestId('persistent-new-storage-type-radio');
-  }
-
-  findExistingStorageRadio() {
-    return cy.findByTestId('persistent-existing-storage-type-radio');
-  }
-
-  findClusterStorageInput() {
-    return cy.findByTestId('create-new-storage-name');
+  getStorageTable() {
+    return new StorageTable();
   }
 
   private findPVSizeField() {
@@ -363,11 +388,6 @@ class EditSpawnerPage extends CreateSpawnerPage {
     cy.testA11y();
   }
 
-  shouldHavePersistentStorage(name: string) {
-    cy.findByTestId('persistent-storage-group').find('input').should('have.value', name);
-    return this;
-  }
-
   shouldHaveNotebookImageSelectInput(name: string) {
     cy.findByTestId('workbench-image-stream-selection').contains(name).should('exist');
     return this;

diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts
@@ -300,22 +300,11 @@ describe('Workbench page', () => {
     environmentVariableField.uploadConfigYaml(configYamlPath);
     environmentVariableField.findRemoveEnvironmentVariableButton().should('be.enabled');
 
-    //cluster storage
-    createSpawnerPage.shouldHaveClusterStorageAlert();
-
-    //add new cluster storage
-    createSpawnerPage.findNewStorageRadio().click();
-    createSpawnerPage.findClusterStorageInput().should('have.value', 'test-project');
-    createSpawnerPage.findClusterStorageDescriptionInput().fill('test-description');
-    createSpawnerPage.findPVSizeMinusButton().click();
-    createSpawnerPage.findPVSizeInput().should('have.value', '19');
-    createSpawnerPage.findPVSizePlusButton().click();
-    createSpawnerPage.findPVSizeInput().should('have.value', '20');
-    createSpawnerPage.selectPVSize('MiB');
-
-    //add existing cluster storage
-    createSpawnerPage.findExistingStorageRadio().click();
-    createSpawnerPage.selectExistingPersistentStorage('Test Storage');
+    // cluster storage
+    const storageTableRow = createSpawnerPage.getStorageTable().getRowById(0);
+    storageTableRow.findNameValue().should('have.text', 'test-project-storage');
+    storageTableRow.findStorageSizeValue().should('have.text', 'Max 20Gi');
+    storageTableRow.findMountPathValue().should('have.text', '/opt/app-root/src/');
 
     //add data connection
     createSpawnerPage.findDataConnectionCheckbox().check();
@@ -352,15 +341,6 @@ describe('Workbench page', () => {
           name: 'test-project',
           namespace: 'test-project',
         },
-        spec: {
-          template: {
-            spec: {
-              volumes: [
-                { name: 'test-storage-1', persistentVolumeClaim: { claimName: 'test-storage-1' } },
-              ],
-            },
-          },
-        },
       });
     });
     verifyRelativeURL('/projects/test-project?section=workbenches');
@@ -693,7 +673,11 @@ describe('Workbench page', () => {
     editSpawnerPage.k8sNameDescription.findDisplayNameInput().should('have.value', 'Test Notebook');
     editSpawnerPage.shouldHaveNotebookImageSelectInput('Test Image');
     editSpawnerPage.shouldHaveContainerSizeInput('Small');
-    editSpawnerPage.shouldHavePersistentStorage('Test Storage');
+    editSpawnerPage
+      .getStorageTable()
+      .getRowById(0)
+      .findNameValue()
+      .should('have.text', 'Test Storage');
     editSpawnerPage.findSubmitButton().should('be.enabled');
     editSpawnerPage.k8sNameDescription.findDisplayNameInput().fill('Updated Notebook');
 
@@ -702,9 +686,35 @@ describe('Workbench page', () => {
     editSpawnerPage.findAcceleratorProfileSelect().findSelectOption('None').click();
     editSpawnerPage.findAcceleratorProfileSelect().should('contain', 'None');
 
+    cy.interceptK8s('PUT', PVCModel, mockPVCK8sResource({ name: 'test-notebook' })).as(
+      'editClusterStorage',
+    );
     cy.interceptK8s('PUT', NotebookModel, mockNotebookK8sResource({})).as('editWorkbenchDryRun');
     cy.interceptK8s('PATCH', NotebookModel, mockNotebookK8sResource({})).as('editWorkbench');
+
     editSpawnerPage.findSubmitButton().click();
+
+    cy.wait('@editClusterStorage').then((interception) => {
+      expect(interception.request.url).to.include('?dryRun=All');
+      expect(interception.request.body).to.containSubset({
+        metadata: {
+          annotations: {
+            'openshift.io/description': '',
+            'openshift.io/display-name': 'Test Storage',
+          },
+          name: 'test-notebook',
+          namespace: 'test-project',
+          labels: {
+            'opendatahub.io/dashboard': 'true',
+          },
+        },
+        spec: {
+          resources: { requests: { storage: '5Gi' } },
+        },
+        status: { phase: 'Pending', accessModes: ['ReadWriteOnce'], capacity: { storage: '5Gi' } },
+      });
+    });
+
     cy.wait('@editWorkbenchDryRun').then((interception) => {
       expect(interception.request.url).to.include('?dryRun=All');
       expect(interception.request.body).to.containSubset({

diff --git a/frontend/src/api/k8s/__tests__/pvcs.spec.ts b/frontend/src/api/k8s/__tests__/pvcs.spec.ts
@@ -10,7 +10,7 @@ import { mockPVCK8sResource } from '~/__mocks__/mockPVCK8sResource';
 import { assemblePvc, createPvc, deletePvc, getDashboardPvcs, updatePvc } from '~/api/k8s/pvcs';
 import { PVCModel } from '~/api/models/k8s';
 import { PersistentVolumeClaimKind } from '~/k8sTypes';
-import { CreatingStorageObject } from '~/pages/projects/types';
+import { StorageData } from '~/pages/projects/types';
 
 jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({
   k8sGetResource: jest.fn(),
@@ -26,11 +26,9 @@ const k8sCreateResourceMock = jest.mocked(k8sCreateResource<PersistentVolumeClai
 const k8sUpdateResourceMock = jest.mocked(k8sUpdateResource<PersistentVolumeClaimKind>);
 const k8sDeleteResourceMock = jest.mocked(k8sDeleteResource<PersistentVolumeClaimKind, K8sStatus>);
 
-const data: CreatingStorageObject = {
-  nameDesc: {
-    name: 'pvc',
-    description: 'Test Storage',
-  },
+const data: StorageData = {
+  name: 'pvc',
+  description: 'Test Storage',
   size: '5Gi',
 };
 

diff --git a/frontend/src/api/k8s/pvcs.ts b/frontend/src/api/k8s/pvcs.ts
@@ -12,20 +12,15 @@ import { PVCModel } from '~/api/models';
 import { translateDisplayNameForK8s } from '~/concepts/k8s/utils';
 import { LABEL_SELECTOR_DASHBOARD_RESOURCE } from '~/const';
 import { applyK8sAPIOptions } from '~/api/apiMergeUtils';
-import { CreatingStorageObject } from '~/pages/projects/types';
+import { StorageData } from '~/pages/projects/types';
 
 export const assemblePvc = (
-  data: CreatingStorageObject,
+  data: StorageData,
   namespace: string,
   editName?: string,
   hideFromUI?: boolean,
 ): PersistentVolumeClaimKind => {
-  const {
-    nameDesc: { name: pvcName, description },
-    size,
-    storageClassName,
-  } = data;
-
+  const { name: pvcName, description, size, storageClassName } = data;
   const name = editName || translateDisplayNameForK8s(pvcName);
 
   return {
@@ -41,14 +36,14 @@ export const assemblePvc = (
       }),
       annotations: {
         'openshift.io/display-name': pvcName.trim(),
-        'openshift.io/description': description,
+        ...(description && { 'openshift.io/description': description }),
       },
     },
     spec: {
       accessModes: ['ReadWriteOnce'],
       resources: {
         requests: {
-          storage: size,
+          storage: String(size),
         },
       },
       storageClassName,
@@ -70,7 +65,7 @@ export const getDashboardPvcs = (projectName: string): Promise<PersistentVolumeC
   });
 
 export const createPvc = (
-  data: CreatingStorageObject,
+  data: StorageData,
   namespace: string,
   opts?: K8sAPIOptions,
   hideFromUI?: boolean,
@@ -83,7 +78,7 @@ export const createPvc = (
 };
 
 export const updatePvc = (
-  data: CreatingStorageObject,
+  data: StorageData,
   existingData: PersistentVolumeClaimKind,
   namespace: string,
   opts?: K8sAPIOptions,

diff --git a/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/ManageNIMServingModal.tsx b/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/ManageNIMServingModal.tsx
@@ -33,11 +33,7 @@ import ServingRuntimeSizeSection from '~/pages/modelServing/screens/projects/Ser
 import NIMModelListSection from '~/pages/modelServing/screens/projects/NIMServiceModal/NIMModelListSection';
 import NIMModelDeploymentNameSection from '~/pages/modelServing/screens/projects/NIMServiceModal/NIMModelDeploymentNameSection';
 import ProjectSection from '~/pages/modelServing/screens/projects/InferenceServiceModal/ProjectSection';
-import {
-  CreatingStorageObject,
-  DataConnection,
-  NamespaceApplicationCase,
-} from '~/pages/projects/types';
+import { DataConnection, NamespaceApplicationCase } from '~/pages/projects/types';
 import {
   getDisplayNameFromK8sResource,
   translateDisplayNameForK8s,
@@ -246,12 +242,10 @@ const ManageNIMServingModal: React.FC<ManageNIMServingModalProps> = ({
             createNIMPVC(namespace, nimPVCName, pvcSize, false).then(() => undefined),
           );
         } else if (pvc && pvc.spec.resources.requests.storage !== pvcSize) {
-          const createData: CreatingStorageObject = {
+          const createData = {
             size: pvcSize, // New size
-            nameDesc: {
-              name: pvc.metadata.name,
-              description: pvc.metadata.annotations?.description || '',
-            },
+            name: pvc.metadata.name,
+            description: pvc.metadata.annotations?.description || '',
             storageClassName: pvc.spec.storageClassName,
           };
           promises.push(

diff --git a/frontend/src/pages/modelServing/screens/projects/__tests__/utils.spec.ts b/frontend/src/pages/modelServing/screens/projects/__tests__/utils.spec.ts
@@ -432,10 +432,8 @@ describe('createNIMPVC', () => {
 
     expect(createPvc).toHaveBeenCalledWith(
       {
-        nameDesc: {
-          name: pvcName,
-          description: '',
-        },
+        name: pvcName,
+        description: '',
         size: pvcSize,
       },
       projectName,
@@ -451,10 +449,8 @@ describe('createNIMPVC', () => {
 
     expect(createPvc).toHaveBeenCalledWith(
       {
-        nameDesc: {
-          name: pvcName,
-          description: '',
-        },
+        name: pvcName,
+        description: '',
         size: pvcSize,
       },
       projectName,

diff --git a/frontend/src/pages/modelServing/screens/projects/utils.ts b/frontend/src/pages/modelServing/screens/projects/utils.ts
@@ -674,10 +674,8 @@ export const createNIMPVC = (
 ): Promise<PersistentVolumeClaimKind> =>
   createPvc(
     {
-      nameDesc: {
-        name: pvcName,
-        description: '',
-      },
+      name: pvcName,
+      description: '',
       size: pvcSize,
     },
     projectName,

diff --git a/frontend/src/pages/projects/pvc/useNotebookPVCItems.ts b/frontend/src/pages/projects/pvc/useNotebookPVCItems.ts
@@ -5,16 +5,16 @@ import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext';
 import { getNotebookPVCNames } from './utils';
 
 const useNotebookPVCItems = (
-  notebook: NotebookKind,
+  notebook: NotebookKind | undefined,
 ): [pvcs: PersistentVolumeClaimKind[], loaded: boolean, loadError?: Error] => {
   const {
     pvcs: { data: allPvcs, loaded, error },
   } = React.useContext(ProjectDetailsContext);
 
-  const pvcNames = useDeepCompareMemoize(getNotebookPVCNames(notebook));
+  const pvcNames = useDeepCompareMemoize(notebook && getNotebookPVCNames(notebook));
 
   const pvcs = React.useMemo(
-    () => allPvcs.filter((pvc) => pvcNames.includes(pvc.metadata.name)),
+    () => allPvcs.filter((pvc) => pvcNames?.includes(pvc.metadata.name)),
     [allPvcs, pvcNames],
   );