Add storage class support for notebook PVCs (opendatahub-io#3255)

* Add storage class support for notebook PVCs * add tests * fix perms * enable feature flag
red-hat-data-services · Sep 27, 2024 · 50a1054 · 50a1054
1 parent a3d4b1b
commit 50a1054
Show file tree

Hide file tree

Showing 13 changed files with 58 additions and 42 deletions.
diff --git a/backend/src/types.ts b/backend/src/types.ts
@@ -838,6 +838,7 @@ export type NotebookData = {
   envVars: EnvVarReducedTypeKeyValues;
   state: NotebookState;
   username?: string;
+  storageClassName?: string;
 };
 
 export type AcceleratorProfileState = {

diff --git a/backend/src/utils/constants.ts b/backend/src/utils/constants.ts
@@ -65,7 +65,7 @@ export const blankDashboardCR: DashboardConfig = {
       disableDistributedWorkloads: false,
       disableModelRegistry: false,
       disableConnectionTypes: true,
-      disableStorageClasses: true,
+      disableStorageClasses: false,
       disableNIMModelServing: true,
     },
     notebookController: {

diff --git a/backend/src/utils/notebookUtils.ts b/backend/src/utils/notebookUtils.ts
@@ -611,7 +611,7 @@ const generateNotebookResources = async (
     await fastify.kube.coreV1Api.readNamespacedPersistentVolumeClaim(pvcName, namespace);
   } catch (e) {
     if (e.statusCode === 404) {
-      await createPvc(fastify, namespace, pvcName);
+      await createPvc(fastify, namespace, pvcName, notebookData.storageClassName);
     } else {
       throw e;
     }
@@ -636,10 +636,12 @@ const createPvc = async (
   fastify: KubeFastifyInstance,
   namespace: string,
   pvcName: string,
+  storageClassName?: string,
 ): Promise<V1PersistentVolumeClaim> => {
   const pvcSize = getDashboardConfig().spec?.notebookController?.pvcSize ?? DEFAULT_PVC_SIZE;
-  const storageClassName = getDashboardConfig().spec.notebookController?.storageClassName;
-  const pvc = assemblePvc(pvcName, namespace, pvcSize, storageClassName);
+  const preferredStorageClassName =
+    getDashboardConfig().spec.notebookController?.storageClassName ?? storageClassName;
+  const pvc = assemblePvc(pvcName, namespace, pvcSize, preferredStorageClassName);
 
   try {
     const pvcResponse = await fastify.kube.coreV1Api.createNamespacedPersistentVolumeClaim(

diff --git a/docs/dashboard-config.md b/docs/dashboard-config.md
@@ -35,7 +35,7 @@ The following are a list of features that are supported, along with there defaul
 | disableDistributedWorkloads  | false   | Disables Distributed Workload Metrics from the dashboard.                                            |
 | disableModelRegistry         | false   | Disables Model Registry from the dashboard.                                                          |
 | disableConnectionTypes       | true    | Disables creating custom data connection types from the dashboard.                                   |
-| disableStorageClasses        | true    | Disables storage classes settings nav item from the dashboard.                                       |
+| disableStorageClasses        | false    | Disables storage classes settings nav item from the dashboard.                                       |
 | disableNIMModelServing       | true    | Disables components of NIM Model UI from the dashboard.   
 
 ## Defaults
@@ -67,7 +67,7 @@ spec:
     disablePipelineExperiments: true
     disableDistributedWorkloads: false
     disableConnectionTypes: false
-    disableStorageClasses: true
+    disableStorageClasses: false
     disableNIMModelServing: true
 ```
 

diff --git a/frontend/src/__mocks__/mockStorageClasses.ts b/frontend/src/__mocks__/mockStorageClasses.ts
@@ -1,26 +1,19 @@
 import { K8sResourceListResult, StorageClassConfig, StorageClassKind } from '~/k8sTypes';
 
-export type MockStorageClass = Omit<StorageClassKind, 'apiVersion' | 'kind'>;
-
-export type MockStorageClassList = Omit<K8sResourceListResult<MockStorageClass>, 'metadata'> & {
-  metadata: {
-    resourceVersion: string;
-  };
-};
-
 export const mockStorageClassList = (
-  storageClasses: MockStorageClass[] = mockStorageClasses,
-): MockStorageClassList => ({
+  storageClasses: StorageClassKind[] = mockStorageClasses,
+): K8sResourceListResult<StorageClassKind> => ({
   kind: 'StorageClassList',
   apiVersion: 'storage.k8s.io/v1',
   metadata: {
     resourceVersion: '55571379',
+    continue: '',
   },
   items: storageClasses,
 });
 
 export const buildMockStorageClassConfig = (
-  mockStorageClass: MockStorageClass,
+  mockStorageClass: StorageClassKind,
   config: Partial<StorageClassConfig>,
 ): string =>
   JSON.stringify({
@@ -31,9 +24,9 @@ export const buildMockStorageClassConfig = (
   });
 
 export const buildMockStorageClass = (
-  mockStorageClass: MockStorageClass,
+  mockStorageClass: StorageClassKind,
   config: Partial<StorageClassConfig> | string,
-): MockStorageClass => ({
+): StorageClassKind => ({
   ...mockStorageClass,
   metadata: {
     ...mockStorageClass.metadata,
@@ -45,8 +38,10 @@ export const buildMockStorageClass = (
   },
 });
 
-export const mockStorageClasses: MockStorageClass[] = [
+export const mockStorageClasses: StorageClassKind[] = [
   {
+    apiVersion: 'storage.k8s.io/v1',
+    kind: 'StorageClass',
     metadata: {
       name: 'openshift-default-sc',
       uid: '5de188ae-aa8e-43d1-a714-4d60ecc5c6da',
@@ -99,6 +94,8 @@ export const mockStorageClasses: MockStorageClass[] = [
     volumeBindingMode: 'WaitForFirstConsumer',
   },
   {
+    apiVersion: 'storage.k8s.io/v1',
+    kind: 'StorageClass',
     metadata: {
       name: 'test-storage-class-1',
       uid: 'c3c05a4a-c1b7-4358-a246-da6b6dfd12cd',

diff --git a/frontend/src/__tests__/cypress/cypress/pages/storageClasses.ts b/frontend/src/__tests__/cypress/cypress/pages/storageClasses.ts
@@ -1,7 +1,8 @@
-import type { MockStorageClass } from '~/__mocks__';
-import { mockStorageClassList } from '~/__mocks__';
 import { appChrome } from '~/__tests__/cypress/cypress/pages/appChrome';
 import { TableRow } from '~/__tests__/cypress/cypress/pages/components/table';
+import { mockStorageClassList } from '~/__mocks__';
+import type { StorageClassKind } from '~/k8sTypes';
+import { StorageClassModel } from '~/__tests__/cypress/cypress/utils/models';
 import { Modal } from './components/Modal';
 import { TableToolbar } from './components/TableToolbar';
 
@@ -24,10 +25,12 @@ class StorageClassesPage {
     return cy.findByTestId('storage-classes-empty-state');
   }
 
-  mockGetStorageClasses(storageClasses?: MockStorageClass[], times?: number) {
-    return cy.interceptOdh(
-      'GET /api/k8s/apis/storage.k8s.io/v1/storageclasses',
-      { times },
+  mockGetStorageClasses(storageClasses?: StorageClassKind[], times?: number) {
+    return cy.interceptK8sList(
+      {
+        model: StorageClassModel,
+        times,
+      },
       mockStorageClassList(storageClasses),
     );
   }

diff --git a/frontend/src/__tests__/cypress/cypress/support/commands/odh.ts b/frontend/src/__tests__/cypress/cypress/support/commands/odh.ts
@@ -51,7 +51,7 @@ import type {
   PipelineVersionKFv2,
 } from '~/concepts/pipelines/kfTypes';
 import type { GrpcResponse } from '~/__mocks__/mlmd/utils';
-import type { BuildMockPipelinveVersionsType, MockStorageClassList } from '~/__mocks__';
+import type { BuildMockPipelinveVersionsType } from '~/__mocks__';
 import type { ArtifactStorage } from '~/concepts/pipelines/types';
 import type { ConnectionTypeConfigMap } from '~/concepts/connectionTypes/types';
 
@@ -194,11 +194,6 @@ declare global {
           options: { query: { dryRun: 'All' } } | null,
           response: OdhResponse<ServingRuntimeKind>,
         ) => Cypress.Chainable<null>) &
-        ((
-          type: 'GET /api/k8s/apis/storage.k8s.io/v1/storageclasses',
-          options: { times?: number },
-          response: OdhResponse<MockStorageClassList>,
-        ) => Cypress.Chainable<null>) &
         ((
           type: 'PUT /api/storage-class/:name/config',
           options: { path: { name: string }; times?: number },

diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/applications/notebookServer.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/applications/notebookServer.cy.ts
@@ -1,16 +1,22 @@
 import { mockRoleBindingK8sResource } from '~/__mocks__/mockRoleBindingK8sResource';
-import { mockK8sResourceList, mockNotebookK8sResource } from '~/__mocks__';
+import {
+  mockK8sResourceList,
+  mockNotebookK8sResource,
+  mockDashboardConfig,
+  mockStorageClassList,
+} from '~/__mocks__';
 import type { RoleBindingSubject } from '~/k8sTypes';
 import { mockAllowedUsers } from '~/__mocks__/mockAllowedUsers';
 import { mockNotebookImageInfo } from '~/__mocks__/mockNotebookImageInfo';
 import { mockStartNotebookData } from '~/__mocks__/mockStartNotebookData';
 import { notebookServer } from '~/__tests__/cypress/cypress/pages/notebookServer';
-import { asProductAdminUser, asProjectEditUser } from '~/__tests__/cypress/cypress/utils/mockUsers';
+import { asClusterAdminUser, asProjectEditUser } from '~/__tests__/cypress/cypress/utils/mockUsers';
 import {
   notebookController,
   stopNotebookModal,
 } from '~/__tests__/cypress/cypress/pages/administration';
 import { homePage } from '~/__tests__/cypress/cypress/pages/home/home';
+import { StorageClassModel } from '~/__tests__/cypress/cypress/utils/models';
 
 const groupSubjects: RoleBindingSubject[] = [
   {
@@ -33,6 +39,14 @@ const initIntercepts = () => {
   );
   cy.interceptOdh('GET /api/images/:type', { path: { type: 'jupyter' } }, mockNotebookImageInfo());
   cy.interceptOdh('GET /api/status/openshift-ai-notebooks/allowedUsers', mockAllowedUsers({}));
+  cy.interceptOdh(
+    'GET /api/config',
+    mockDashboardConfig({
+      disableStorageClasses: false,
+    }),
+  );
+
+  cy.interceptK8sList(StorageClassModel, mockStorageClassList());
 };
 
 it('Administration tab should not be accessible for non-project admins', () => {
@@ -46,8 +60,8 @@ it('Administration tab should not be accessible for non-project admins', () => {
 
 describe('NotebookServer', () => {
   beforeEach(() => {
+    asClusterAdminUser();
     initIntercepts();
-    asProductAdminUser();
   });
 
   it('should start notebook server', () => {
@@ -64,6 +78,7 @@ describe('NotebookServer', () => {
         acceleratorProfile: { acceleratorProfiles: [], count: 0, unknownProfileDetected: false },
         envVars: { configMap: {}, secrets: {} },
         state: 'started',
+        storageClassName: 'openshift-default-sc',
       });
     });
   });

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
@@ -23,6 +23,7 @@ import {
   PVCModel,
   PodModel,
   ProjectModel,
+  StorageClassModel,
 } from '~/__tests__/cypress/cypress/utils/models';
 import { mock200Status } from '~/__mocks__/mockK8sStatus';
 import { mockPrometheusQueryResponse } from '~/__mocks__/mockPrometheusQueryResponse';
@@ -80,11 +81,7 @@ describe('ClusterStorage', () => {
         }),
       );
 
-      cy.interceptOdh(
-        'GET /api/k8s/apis/storage.k8s.io/v1/storageclasses',
-        {},
-        mockStorageClassList(),
-      );
+      cy.interceptK8sList(StorageClassModel, mockStorageClassList());
     });
 
     it('Check whether the Storage class column is present', () => {

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
@@ -32,6 +32,7 @@ import {
   ProjectModel,
   RouteModel,
   SecretModel,
+  StorageClassModel,
 } from '~/__tests__/cypress/cypress/utils/models';
 import { mock200Status } from '~/__mocks__/mockK8sStatus';
 import type { NotebookSize } from '~/types';
@@ -61,7 +62,7 @@ const initIntercepts = ({
     },
   ],
 }: HandlersProps) => {
-  cy.interceptOdh('GET /api/k8s/apis/storage.k8s.io/v1/storageclasses', {}, mockStorageClassList());
+  cy.interceptK8sList(StorageClassModel, mockStorageClassList());
   cy.interceptOdh(
     'GET /api/dsc/status',
     mockDscStatus({

diff --git a/frontend/src/pages/notebookController/screens/server/SpawnerPage.tsx b/frontend/src/pages/notebookController/screens/server/SpawnerPage.tsx
@@ -47,6 +47,7 @@ import { SupportedArea, useIsAreaAvailable } from '~/concepts/areas';
 import { fireFormTrackingEvent } from '~/concepts/analyticsTracking/segmentIOUtils';
 import { TrackingOutcome } from '~/concepts/analyticsTracking/trackingProperties';
 import useGenericObjectState from '~/utilities/useGenericObjectState';
+import useDefaultStorageClass from '~/pages/projects/screens/spawner/storage/useDefaultStorageClass';
 import SizeSelectField from './SizeSelectField';
 import useSpawnerNotebookModalState from './useSpawnerNotebookModalState';
 import BrowserTabPreferenceCheckbox from './BrowserTabPreferenceCheckbox';
@@ -83,6 +84,8 @@ const SpawnerPage: React.FC = () => {
   const [variableRows, setVariableRows] = React.useState<VariableRow[]>([]);
   const [submitError, setSubmitError] = React.useState<Error | null>(null);
 
+  const defaultStorageClass = useDefaultStorageClass();
+
   const [selectedAcceleratorProfile, setSelectedAcceleratorProfile] =
     useGenericObjectState<AcceleratorProfileSelectFieldState>({
       profile: undefined,
@@ -278,6 +281,7 @@ const SpawnerPage: React.FC = () => {
       envVars,
       state: NotebookState.Started,
       username: impersonatedUsername || undefined,
+      storageClassName: defaultStorageClass?.metadata.name,
     })
       .then(() => {
         fireStartServerEvent();

diff --git a/frontend/src/types.ts b/frontend/src/types.ts
@@ -601,6 +601,7 @@ export type NotebookData = {
   state: NotebookState;
   // only used for admin calls, regular users cannot use this field
   username?: string;
+  storageClassName?: string;
 };
 
 export type UsernameMap<V> = { [username: string]: V };

diff --git a/manifests/rhoai/shared/odhdashboardconfig/odhdashboardconfig.yaml b/manifests/rhoai/shared/odhdashboardconfig/odhdashboardconfig.yaml
@@ -29,7 +29,7 @@ spec:
     disableDistributedWorkloads: false
     disableModelRegistry: false
     disableConnectionTypes: true
-    disableStorageClasses: true
+    disableStorageClasses: false
     disableNIMModelServing: true
   groupsConfig:
     adminGroups: "$(admin_groups)"