Storage class dropdown select (#3212)

* RHOAIENG-1109 Select Storage Classes in add cluster storage sections * refactor: Remove unnecessary storageClassName parameter in replaceRootVolumesForNotebook function fix tests fix disabled checks pr fixes to state added tests linter fix * fix alert on empty * fixed storage class default --------- Co-authored-by: Dipanshu Gupta <[email protected]>
opendatahub-io · Sep 17, 2024 · b5351a7 · b5351a7
1 parent e9c8a33
commit b5351a7
Show file tree

Hide file tree

Showing 17 changed files with 306 additions and 74 deletions.
diff --git a/frontend/src/__mocks__/mockStorageClasses.ts b/frontend/src/__mocks__/mockStorageClasses.ts
@@ -1,4 +1,4 @@
-import { K8sResourceListResult, StorageClassKind } from '~/k8sTypes';
+import { K8sResourceListResult, StorageClassConfig, StorageClassKind } from '~/k8sTypes';
 
 export type MockStorageClass = Omit<StorageClassKind, 'apiVersion' | 'kind'>;
 
@@ -19,6 +19,23 @@ export const mockStorageClassList = (
   items: storageClasses,
 });
 
+export const buildMockStorageClass = (
+  mockStorageClass: MockStorageClass,
+  config: Partial<StorageClassConfig>,
+): MockStorageClass => ({
+  ...mockStorageClass,
+  metadata: {
+    ...mockStorageClass.metadata,
+    annotations: {
+      ...mockStorageClass.metadata.annotations,
+      'opendatahub.io/sc-config': JSON.stringify({
+        ...JSON.parse(String(mockStorageClass.metadata.annotations?.['opendatahub.io/sc-config'])),
+        ...config,
+      }),
+    },
+  },
+});
+
 export const mockStorageClasses: MockStorageClass[] = [
   {
     metadata: {

diff --git a/frontend/src/__tests__/cypress/cypress/pages/clusterStorage.ts b/frontend/src/__tests__/cypress/cypress/pages/clusterStorage.ts
@@ -85,6 +85,14 @@ class ClusterStorageModal extends Modal {
   findPVSizePlusButton() {
     return this.findPVSizeField().findByRole('button', { name: 'Plus' });
   }
+
+  findStorageClassSelect() {
+    return this.find().findByTestId('storage-classes-selector');
+  }
+
+  findStorageClassDeprecatedWarning() {
+    return this.find().findByTestId('deprecated-storage-warning');
+  }
 }
 
 class ClusterStorage {

diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/clusterStorage.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/clusterStorage.cy.ts
@@ -1,4 +1,10 @@
-import { mockK8sResourceList, mockNotebookK8sResource, mockProjectK8sResource } from '~/__mocks__';
+import {
+  buildMockStorageClass,
+  mockK8sResourceList,
+  mockNotebookK8sResource,
+  mockProjectK8sResource,
+  mockStorageClasses,
+} from '~/__mocks__';
 import { mockClusterSettings } from '~/__mocks__/mockClusterSettings';
 import { mockPVCK8sResource } from '~/__mocks__/mockPVCK8sResource';
 import { mockPodK8sResource } from '~/__mocks__/mockPodK8sResource';
@@ -17,6 +23,7 @@ import {
 } from '~/__tests__/cypress/cypress/utils/models';
 import { mock200Status } from '~/__mocks__/mockK8sStatus';
 import { mockPrometheusQueryResponse } from '~/__mocks__/mockPrometheusQueryResponse';
+import { storageClassesTable } from '~/__tests__/cypress/cypress/pages/storageClasses';
 
 type HandlersProps = {
   isEmpty?: boolean;
@@ -45,6 +52,8 @@ const initInterceptors = ({ isEmpty = false }: HandlersProps) => {
   cy.interceptK8sList(NotebookModel, mockK8sResourceList([mockNotebookK8sResource({})]));
 };
 
+const [openshiftDefaultStorageClass, otherStorageClass] = mockStorageClasses;
+
 describe('ClusterStorage', () => {
   it('Empty state', () => {
     initInterceptors({ isEmpty: true });
@@ -56,9 +65,20 @@ describe('ClusterStorage', () => {
 
   it('Add cluster storage', () => {
     initInterceptors({ isEmpty: true });
+    storageClassesTable.mockGetStorageClasses([
+      openshiftDefaultStorageClass,
+      buildMockStorageClass(otherStorageClass, { isEnabled: true }),
+    ]);
+
     clusterStorage.visit('test-project');
     clusterStorage.findCreateButton().click();
     addClusterStorageModal.findNameInput().fill('test-storage');
+
+    // default selected
+    addClusterStorageModal.find().findByText('openshift-default-sc').should('exist');
+
+    // select storage class
+    addClusterStorageModal.findStorageClassSelect().findSelectOption('Test SC 1').click();
     addClusterStorageModal.findSubmitButton().should('be.enabled');
     addClusterStorageModal.findDescriptionInput().fill('description');
     addClusterStorageModal.findPVSizeMinusButton().click();

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
@@ -6,6 +6,7 @@ import {
   mockProjectK8sResource,
   mockRouteK8sResource,
   mockSecretK8sResource,
+  mockStorageClassList,
 } from '~/__mocks__';
 import { mockConfigMap } from '~/__mocks__/mockConfigMap';
 import { mockImageStreamK8sResource } from '~/__mocks__/mockImageStreamK8sResource';
@@ -60,6 +61,7 @@ const initIntercepts = ({
     },
   ],
 }: HandlersProps) => {
+  cy.interceptOdh('GET /api/k8s/apis/storage.k8s.io/v1/storageclasses', {}, mockStorageClassList());
   cy.interceptOdh(
     'GET /api/dsc/status',
     mockDscStatus({

diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/storageClasses/storageClasses.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/storageClasses/storageClasses.cy.ts
@@ -1,13 +1,11 @@
-import type { MockStorageClass } from '~/__mocks__';
-import { mockStorageClassList, mockStorageClasses } from '~/__mocks__';
+import { buildMockStorageClass, mockStorageClassList, mockStorageClasses } from '~/__mocks__';
 import { asProductAdminUser } from '~/__tests__/cypress/cypress/utils/mockUsers';
 import { pageNotfound } from '~/__tests__/cypress/cypress/pages/pageNotFound';
 import {
   storageClassEditModal,
   storageClassesPage,
   storageClassesTable,
 } from '~/__tests__/cypress/cypress/pages/storageClasses';
-import type { StorageClassConfig } from '~/k8sTypes';
 
 describe('Storage classes', () => {
   it('shows "page not found" and does not show nav item as a non-admin user', () => {
@@ -153,20 +151,3 @@ describe('Storage classes', () => {
     });
   });
 });
-
-const buildMockStorageClass = (
-  mockStorageClass: MockStorageClass,
-  config: Partial<StorageClassConfig>,
-) => ({
-  ...mockStorageClass,
-  metadata: {
-    ...mockStorageClass.metadata,
-    annotations: {
-      ...mockStorageClass.metadata.annotations,
-      'opendatahub.io/sc-config': JSON.stringify({
-        ...JSON.parse(String(mockStorageClass.metadata.annotations?.['opendatahub.io/sc-config'])),
-        ...config,
-      }),
-    },
-  },
-});
diff --git a/frontend/src/api/k8s/pvcs.ts b/frontend/src/api/k8s/pvcs.ts
@@ -17,11 +17,11 @@ export const assemblePvc = (
   data: CreatingStorageObject,
   namespace: string,
   editName?: string,
-  storageClassName?: string,
 ): PersistentVolumeClaimKind => {
   const {
     nameDesc: { name: pvcName, description },
     size,
+    storageClassName,
   } = data;
 
   const name = editName || translateDisplayNameForK8s(pvcName);
@@ -68,10 +68,9 @@ export const getDashboardPvcs = (projectName: string): Promise<PersistentVolumeC
 export const createPvc = (
   data: CreatingStorageObject,
   namespace: string,
-  storageClassName?: string,
   opts?: K8sAPIOptions,
 ): Promise<PersistentVolumeClaimKind> => {
-  const pvc = assemblePvc(data, namespace, undefined, storageClassName);
+  const pvc = assemblePvc(data, namespace);
 
   return k8sCreateResource<PersistentVolumeClaimKind>(
     applyK8sAPIOptions({ model: PVCModel, resource: pvc }, opts),

diff --git a/frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx b/frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx
@@ -12,8 +12,10 @@ import useRelatedNotebooks, {
 import NotebookRestartAlert from '~/pages/projects/components/NotebookRestartAlert';
 import useWillNotebooksRestart from '~/pages/projects/notebook/useWillNotebooksRestart';
 import DashboardModalFooter from '~/concepts/dashboard/DashboardModalFooter';
-import usePreferredStorageClass from '~/pages/projects/screens/spawner/storage/usePreferredStorageClass';
 import { getDescriptionFromK8sResource, getDisplayNameFromK8sResource } from '~/concepts/k8s/utils';
+import { SupportedArea, useIsAreaAvailable } from '~/concepts/areas';
+import usePreferredStorageClass from '~/pages/projects/screens/spawner/storage/usePreferredStorageClass';
+import useDefaultStorageClass from '~/pages/projects/screens/spawner/storage/useDefaultStorageClass';
 import ExistingConnectedNotebooks from './ExistingConnectedNotebooks';
 
 type AddStorageModalProps = {
@@ -23,6 +25,10 @@ type AddStorageModalProps = {
 };
 
 const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOpen, onClose }) => {
+  const isStorageClassesAvailable = useIsAreaAvailable(SupportedArea.STORAGE_CLASSES).status;
+  const preferredStorageClass = usePreferredStorageClass();
+  const defaultStorageClass = useDefaultStorageClass();
+
   const [createData, setCreateData, resetData] = useCreateStorageObjectForNotebook(existingData);
   const [actionInProgress, setActionInProgress] = React.useState(false);
   const [error, setError] = React.useState<Error | undefined>();
@@ -40,7 +46,22 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
     createData.forNotebook.name,
   ]);
 
-  const storageClass = usePreferredStorageClass();
+  React.useEffect(() => {
+    if (!existingData && isOpen) {
+      if (isStorageClassesAvailable) {
+        setCreateData('storageClassName', defaultStorageClass?.metadata.name);
+      } else {
+        setCreateData('storageClassName', preferredStorageClass?.metadata.name);
+      }
+    }
+  }, [
+    isStorageClassesAvailable,
+    defaultStorageClass,
+    preferredStorageClass,
+    existingData,
+    isOpen,
+    setCreateData,
+  ]);
 
   const onBeforeClose = (submitted: boolean) => {
     onClose(submitted);
@@ -53,8 +74,13 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
   const hasValidNotebookRelationship = createData.forNotebook.name
     ? !!createData.forNotebook.mountPath.value && !createData.forNotebook.mountPath.error
     : true;
+
+  const storageClassSelected = isStorageClassesAvailable ? createData.storageClassName : true;
   const canCreate =
-    !actionInProgress && createData.nameDesc.name.trim() && hasValidNotebookRelationship;
+    !actionInProgress &&
+    createData.nameDesc.name.trim() &&
+    hasValidNotebookRelationship &&
+    storageClassSelected;
 
   const runPromiseActions = async (dryRun: boolean) => {
     const {
@@ -66,7 +92,8 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
       if (
         getDisplayNameFromK8sResource(existingData) !== createData.nameDesc.name ||
         getDescriptionFromK8sResource(existingData) !== createData.nameDesc.description ||
-        existingData.spec.resources.requests.storage !== createData.size
+        existingData.spec.resources.requests.storage !== createData.size ||
+        existingData.spec.storageClassName !== createData.storageClassName
       ) {
         pvcPromises.push(updatePvc(createData, existingData, namespace, { dryRun }));
       }
@@ -87,9 +114,7 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
       }
       return;
     }
-    const createdPvc = await createPvc(createData, namespace, storageClass?.metadata.name, {
-      dryRun,
-    });
+    const createdPvc = await createPvc(createData, namespace, { dryRun });
     if (notebookName) {
       await attachNotebookPVC(notebookName, namespace, createdPvc.metadata.name, mountPath.value, {
         dryRun,
@@ -145,6 +170,7 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
               setData={(key, value) => setCreateData(key, value)}
               currentSize={existingData?.status?.capacity?.storage}
               autoFocusName
+              disableStorageClassSelect={!!existingData}
             />
           </StackItem>
           {createData.hasExistingNotebookConnections && (

diff --git a/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx b/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx
@@ -18,7 +18,6 @@ import {
 import { useUser } from '~/redux/selectors';
 import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext';
 import { AppContext } from '~/app/AppContext';
-import usePreferredStorageClass from '~/pages/projects/screens/spawner/storage/usePreferredStorageClass';
 import { ProjectSectionID } from '~/pages/projects/screens/detail/types';
 import { fireFormTrackingEvent } from '~/concepts/analyticsTracking/segmentIOUtils';
 import {
@@ -57,7 +56,6 @@ const SpawnerFooter: React.FC<SpawnerFooterProps> = ({
     },
   } = React.useContext(AppContext);
   const tolerationSettings = notebookController?.notebookTolerationSettings;
-  const storageClass = usePreferredStorageClass();
   const {
     notebooks: { data },
     dataConnections: { data: existingDataConnections },
@@ -158,7 +156,6 @@ const SpawnerFooter: React.FC<SpawnerFooterProps> = ({
           projectName,
           editNotebook,
           storageData,
-          storageClass?.metadata.name,
           dryRun,
         ).catch(handleError);
 
@@ -242,11 +239,7 @@ const SpawnerFooter: React.FC<SpawnerFooterProps> = ({
         ? [dataConnection.existing]
         : [];
 
-    const pvcDetails = await createPvcDataForNotebook(
-      projectName,
-      storageData,
-      storageClass?.metadata.name,
-    ).catch(handleError);
+    const pvcDetails = await createPvcDataForNotebook(projectName, storageData).catch(handleError);
     const envFrom = await createConfigMapsAndSecretsForNotebook(projectName, [
       ...envVariables,
       ...newDataConnection,

diff --git a/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx b/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx
@@ -26,6 +26,7 @@ import useNotebookAcceleratorProfile from '~/pages/projects/screens/detail/noteb
 import { NotebookImageAvailability } from '~/pages/projects/screens/detail/notebooks/const';
 import { getDisplayNameFromK8sResource } from '~/concepts/k8s/utils';
 import useGenericObjectState from '~/utilities/useGenericObjectState';
+import { SupportedArea, useIsAreaAvailable } from '~/concepts/areas';
 import K8sNameDescriptionField, {
   useK8sNameDescriptionFieldData,
 } from '~/concepts/k8s/K8sNameDescriptionField/K8sNameDescriptionField';
@@ -47,6 +48,8 @@ import { useNotebookEnvVariables } from './environmentVariables/useNotebookEnvVa
 import DataConnectionField from './dataConnection/DataConnectionField';
 import { useNotebookDataConnection } from './dataConnection/useNotebookDataConnection';
 import { useNotebookSizeState } from './useNotebookSizeState';
+import useDefaultStorageClass from './storage/useDefaultStorageClass';
+import usePreferredStorageClass from './storage/usePreferredStorageClass';
 
 type SpawnerPageProps = {
   existingNotebook?: NotebookKind;
@@ -70,10 +73,19 @@ const SpawnerPage: React.FC<SpawnerPageProps> = ({ existingNotebook }) => {
     string[] | undefined
   >();
   const [storageDataWithoutDefault, setStorageData] = useStorageDataObject(existingNotebook);
+
+  const defaultStorageClass = useDefaultStorageClass();
+  const preferredStorageClass = usePreferredStorageClass();
+  const isStorageClassesAvailable = useIsAreaAvailable(SupportedArea.STORAGE_CLASSES).status;
+  const defaultStorageClassName = isStorageClassesAvailable
+    ? defaultStorageClass?.metadata.name
+    : preferredStorageClass?.metadata.name;
   const storageData = useMergeDefaultPVCName(
     storageDataWithoutDefault,
     k8sNameDescriptionData.data.name,
+    defaultStorageClassName,
   );
+
   const [envVariables, setEnvVariables] = useNotebookEnvVariables(existingNotebook);
   const [dataConnectionData, setDataConnectionData] = useNotebookDataConnection(
     dataConnections.data,

diff --git a/frontend/src/pages/projects/screens/spawner/service.ts b/frontend/src/pages/projects/screens/spawner/service.ts
@@ -30,14 +30,13 @@ import { fetchNotebookEnvVariables } from './environmentVariables/useNotebookEnv
 export const createPvcDataForNotebook = async (
   projectName: string,
   storageData: StorageData,
-  storageClassName?: string,
 ): Promise<{ volumes: Volume[]; volumeMounts: VolumeMount[] }> => {
   const { storageType } = storageData;
 
   const { volumes, volumeMounts } = getVolumesByStorageData(storageData);
 
   if (storageType === StorageType.NEW_PVC) {
-    const pvc = await createPvc(storageData.creating, projectName, storageClassName);
+    const pvc = await createPvc(storageData.creating, projectName);
     const newPvcName = pvc.metadata.name;
     volumes.push({ name: newPvcName, persistentVolumeClaim: { claimName: newPvcName } });
     volumeMounts.push({ mountPath: ROOT_MOUNT_PATH, name: newPvcName });
@@ -49,7 +48,6 @@ export const replaceRootVolumesForNotebook = async (
   projectName: string,
   notebook: NotebookKind,
   storageData: StorageData,
-  storageClassName?: string,
   dryRun?: boolean,
 ): Promise<{ volumes: Volume[]; volumeMounts: VolumeMount[] }> => {
   const {
@@ -70,7 +68,7 @@ export const replaceRootVolumesForNotebook = async (
     };
     replacedVolumeMount = { name: existingName, mountPath: ROOT_MOUNT_PATH };
   } else {
-    const pvc = await createPvc(storageData.creating, projectName, storageClassName, { dryRun });
+    const pvc = await createPvc(storageData.creating, projectName, { dryRun });
     const newPvcName = pvc.metadata.name;
     replacedVolume = { name: newPvcName, persistentVolumeClaim: { claimName: newPvcName } };
     replacedVolumeMount = { mountPath: ROOT_MOUNT_PATH, name: newPvcName };

diff --git a/frontend/src/pages/projects/screens/spawner/spawnerUtils.ts b/frontend/src/pages/projects/screens/spawner/spawnerUtils.ts
@@ -33,6 +33,7 @@ import { FAILED_PHASES, PENDING_PHASES, IMAGE_ANNOTATIONS } from './const';
 export const useMergeDefaultPVCName = (
   storageData: StorageData,
   defaultPVCName: string,
+  defaultStorageClassName?: string,
 ): StorageData => {
   const modifiedRef = React.useRef(false);
 
@@ -49,6 +50,7 @@ export const useMergeDefaultPVCName = (
         ...storageData.creating.nameDesc,
         name: storageData.creating.nameDesc.name || defaultPVCName,
       },
+      storageClassName: storageData.creating.storageClassName || defaultStorageClassName,
     },
   };
 };
@@ -406,7 +408,8 @@ export const checkRequiredFieldsForNotebookStart = (
     image.imageVersion
   );
 
-  const newStorageFieldInvalid = storageType === StorageType.NEW_PVC && !creating.nameDesc.name;
+  const newStorageFieldInvalid =
+    storageType === StorageType.NEW_PVC && (!creating.nameDesc.name || !creating.storageClassName);
   const existingStorageFieldInvalid = storageType === StorageType.EXISTING_PVC && !existing.storage;
   const isStorageDataValid = !newStorageFieldInvalid && !existingStorageFieldInvalid;