fix: pvc fixes (opendatahub-io#3296)

* fix: pvc fixes Signed-off-by: Olga Lavtar <[email protected]> * fix: pvc fixes based on feedback Signed-off-by: Olga Lavtar <[email protected]> --------- Signed-off-by: Olga Lavtar <[email protected]>
jenny-s51 · Oct 7, 2024 · 82d4f12 · 82d4f12
1 parent a792657
commit 82d4f12
Show file tree

Hide file tree

Showing 6 changed files with 149 additions and 60 deletions.
diff --git a/frontend/src/api/k8s/servingRuntimes.ts b/frontend/src/api/k8s/servingRuntimes.ts
@@ -35,7 +35,6 @@ export const assembleServingRuntime = (
   initialAcceleratorProfile?: AcceleratorProfileState,
   selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState,
   isModelMesh?: boolean,
-  nimPVCName?: string,
 ): ServingRuntimeKind => {
   const {
     name: displayName,
@@ -134,15 +133,6 @@ export const assembleServingRuntime = (
       if (!volumeMounts.find((volumeMount) => volumeMount.mountPath === '/dev/shm')) {
         volumeMounts.push(getshmVolumeMount());
       }
-      const updatedVolumeMounts = volumeMounts.map((volumeMount) => {
-        if (volumeMount.name === 'nim-pvc' && nimPVCName) {
-          return {
-            ...volumeMount,
-            name: nimPVCName,
-          };
-        }
-        return volumeMount;
-      });
 
       const updatedContainer = {
         ...container,
@@ -155,7 +145,7 @@ export const assembleServingRuntime = (
         ...containerWithoutResources,
         ...(isModelMesh ? { resources } : {}),
         affinity,
-        volumeMounts: updatedVolumeMounts,
+        volumeMounts,
       };
     },
   );
@@ -181,33 +171,8 @@ export const assembleServingRuntime = (
     volumes.push(getshmVolume('2Gi'));
   }
 
-  if (nimPVCName) {
-    const updatedVolumes = volumes.map((volume) => {
-      if (volume.name === 'nim-pvc') {
-        return {
-          ...volume,
-          name: nimPVCName,
-          persistentVolumeClaim: {
-            claimName: nimPVCName,
-          },
-        };
-      }
-      return volume;
-    });
-
-    if (!updatedVolumes.find((volume) => volume.name === nimPVCName)) {
-      updatedVolumes.push({
-        name: nimPVCName,
-        persistentVolumeClaim: {
-          claimName: nimPVCName,
-        },
-      });
-    }
+  updatedServingRuntime.spec.volumes = volumes;
 
-    updatedServingRuntime.spec.volumes = updatedVolumes;
-  } else {
-    updatedServingRuntime.spec.volumes = volumes;
-  }
   return updatedServingRuntime;
 };
 
@@ -277,7 +242,6 @@ export const updateServingRuntime = (options: {
   initialAcceleratorProfile?: AcceleratorProfileState;
   selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState;
   isModelMesh?: boolean;
-  nimPVCName?: string;
 }): Promise<ServingRuntimeKind> => {
   const {
     data,
@@ -287,7 +251,6 @@ export const updateServingRuntime = (options: {
     initialAcceleratorProfile,
     selectedAcceleratorProfile,
     isModelMesh,
-    nimPVCName,
   } = options;
 
   const updatedServingRuntime = assembleServingRuntime(
@@ -299,7 +262,6 @@ export const updateServingRuntime = (options: {
     initialAcceleratorProfile,
     selectedAcceleratorProfile,
     isModelMesh,
-    nimPVCName,
   );
 
   return k8sUpdateResource<ServingRuntimeKind>(
@@ -322,7 +284,6 @@ export const createServingRuntime = (options: {
   initialAcceleratorProfile?: AcceleratorProfileState;
   selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState;
   isModelMesh?: boolean;
-  nimPVCName?: string;
 }): Promise<ServingRuntimeKind> => {
   const {
     data,
@@ -333,7 +294,6 @@ export const createServingRuntime = (options: {
     initialAcceleratorProfile,
     selectedAcceleratorProfile,
     isModelMesh,
-    nimPVCName,
   } = options;
   const assembledServingRuntime = assembleServingRuntime(
     data,
@@ -344,7 +304,6 @@ export const createServingRuntime = (options: {
     initialAcceleratorProfile,
     selectedAcceleratorProfile,
     isModelMesh,
-    nimPVCName,
   );
 
   return k8sCreateResource<ServingRuntimeKind>(

diff --git a/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/DeployNIMServiceModal.tsx b/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/DeployNIMServiceModal.tsx
@@ -45,7 +45,10 @@ import KServeAutoscalerReplicaSection from '~/pages/modelServing/screens/project
 import useGenericObjectState from '~/utilities/useGenericObjectState';
 import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField';
 import NIMPVCSizeSection from '~/pages/modelServing/screens/projects/NIMServiceModal/NIMPVCSizeSection';
-import { getNIMServingRuntimeTemplate } from '~/pages/modelServing/screens/projects/nimUtils';
+import {
+  getNIMServingRuntimeTemplate,
+  updateServingRuntimeTemplate,
+} from '~/pages/modelServing/screens/projects/nimUtils';
 import { useDashboardNamespace } from '~/redux/selectors';
 import { getServingRuntimeFromTemplate } from '~/pages/modelServing/customServingRuntimes/utils';
 
@@ -95,7 +98,6 @@ const DeployNIMServiceModal: React.FC<DeployNIMServiceModalProps> = ({
   const isAuthorinoEnabled = useIsAreaAvailable(SupportedArea.K_SERVE_AUTH).status;
   const currentProjectName = projectContext?.currentProject.metadata.name;
   const namespace = currentProjectName || createDataInferenceService.project;
-  const nimPVCName = getUniqueId('nim-pvc');
 
   const [translatedName] = translateDisplayNameForK8sAndReport(createDataInferenceService.name, {
     maxLength: 253,
@@ -190,8 +192,14 @@ const DeployNIMServiceModal: React.FC<DeployNIMServiceModalProps> = ({
       editInfo?.inferenceServiceEditInfo?.spec.predictor.model?.runtime ||
       translateDisplayNameForK8s(createDataInferenceService.name);
 
+    const nimPVCName = getUniqueId('nim-pvc');
+
+    const updatedServingRuntime = servingRuntimeSelected
+      ? updateServingRuntimeTemplate(servingRuntimeSelected, nimPVCName)
+      : undefined;
+
     const submitServingRuntimeResources = getSubmitServingRuntimeResourcesFn(
-      servingRuntimeSelected,
+      updatedServingRuntime,
       createDataServingRuntime,
       customServingRuntimesEnabled,
       namespace,
@@ -203,7 +211,6 @@ const DeployNIMServiceModal: React.FC<DeployNIMServiceModalProps> = ({
       projectContext?.currentProject,
       servingRuntimeName,
       true,
-      nimPVCName,
     );
 
     const submitInferenceServiceResource = getSubmitInferenceServiceResourceFn(

diff --git a/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/NIMModelListSection.tsx b/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/NIMModelListSection.tsx
@@ -8,7 +8,6 @@ import {
 } from '~/pages/modelServing/screens/types';
 import SimpleSelect from '~/components/SimpleSelect';
 import { fetchNIMModelNames, ModelInfo } from '~/pages/modelServing/screens/projects/utils';
-import { useDashboardNamespace } from '~/redux/selectors';
 
 type NIMModelListSectionProps = {
   inferenceServiceData: CreatingInferenceServiceObject;
@@ -25,7 +24,6 @@ const NIMModelListSection: React.FC<NIMModelListSectionProps> = ({
 }) => {
   const [options, setOptions] = useState<{ key: string; label: string }[]>([]);
   const [modelList, setModelList] = useState<ModelInfo[]>([]);
-  const { dashboardNamespace } = useDashboardNamespace();
   const [error, setError] = useState<string>('');
   const [selectedModel, setSelectedModel] = useState('');
 
@@ -53,7 +51,7 @@ const NIMModelListSection: React.FC<NIMModelListSectionProps> = ({
       }
     };
     getModelNames();
-  }, [dashboardNamespace]);
+  }, []);
 
   const getSupportedModelFormatsInfo = (key: string) => {
     const lastHyphenIndex = key.lastIndexOf('-');

diff --git a/frontend/src/pages/modelServing/screens/projects/__tests__/utils.spec.ts b/frontend/src/pages/modelServing/screens/projects/__tests__/utils.spec.ts
@@ -13,11 +13,12 @@ import { LabeledDataConnection, ServingPlatformStatuses } from '~/pages/modelSer
 import { ServingRuntimePlatform } from '~/types';
 import { mockInferenceServiceK8sResource } from '~/__mocks__/mockInferenceServiceK8sResource';
 import { createPvc, createSecret } from '~/api';
-import { PersistentVolumeClaimKind } from '~/k8sTypes';
+import { PersistentVolumeClaimKind, ServingRuntimeKind } from '~/k8sTypes';
 import {
   getNGCSecretType,
   getNIMData,
   getNIMResource,
+  updateServingRuntimeTemplate,
 } from '~/pages/modelServing/screens/projects/nimUtils';
 
 jest.mock('~/api', () => ({
@@ -27,6 +28,7 @@ jest.mock('~/api', () => ({
 }));
 
 jest.mock('~/pages/modelServing/screens/projects/nimUtils', () => ({
+  ...jest.requireActual('~/pages/modelServing/screens/projects/nimUtils'),
   getNIMData: jest.fn(),
   getNGCSecretType: jest.fn(),
   getNIMResource: jest.fn(),
@@ -455,3 +457,82 @@ describe('createNIMPVC', () => {
     );
   });
 });
+
+describe('updateServingRuntimeTemplate', () => {
+  const servingRuntimeMock: ServingRuntimeKind = {
+    apiVersion: 'serving.kserve.io/v1alpha1',
+    kind: 'ServingRuntime',
+    metadata: {
+      name: 'test-serving-runtime',
+      namespace: 'test-namespace',
+    },
+    spec: {
+      containers: [
+        {
+          name: 'test-container',
+          volumeMounts: [
+            { name: 'nim-pvc', mountPath: '/mnt/models/cache' },
+            { name: 'other-volume', mountPath: '/mnt/other-path' },
+          ],
+        },
+      ],
+      volumes: [
+        { name: 'nim-pvc', persistentVolumeClaim: { claimName: 'old-nim-pvc' } },
+        { name: 'other-volume', emptyDir: {} },
+      ],
+    },
+  };
+
+  it('should update PVC name in volumeMounts and volumes', () => {
+    const pvcName = 'new-nim-pvc';
+    const updatedServingRuntime = updateServingRuntimeTemplate(servingRuntimeMock, pvcName);
+
+    expect(updatedServingRuntime.spec.containers[0].volumeMounts).toEqual([
+      { name: pvcName, mountPath: '/mnt/models/cache' },
+      { name: 'other-volume', mountPath: '/mnt/other-path' },
+    ]);
+
+    expect(updatedServingRuntime.spec.volumes).toEqual([
+      { name: pvcName, persistentVolumeClaim: { claimName: pvcName } },
+      { name: 'other-volume', emptyDir: {} },
+    ]);
+  });
+
+  it('should not modify unrelated volumeMounts and volumes', () => {
+    const pvcName = 'new-nim-pvc';
+    const updatedServingRuntime = updateServingRuntimeTemplate(servingRuntimeMock, pvcName);
+
+    expect(updatedServingRuntime.spec.containers[0].volumeMounts?.[1]).toEqual({
+      name: 'other-volume',
+      mountPath: '/mnt/other-path',
+    });
+
+    expect(updatedServingRuntime.spec.volumes?.[1]).toEqual({
+      name: 'other-volume',
+      emptyDir: {},
+    });
+  });
+
+  it('should handle serving runtime with containers but no volumeMounts', () => {
+    const servingRuntimeWithoutVolumeMounts: ServingRuntimeKind = {
+      apiVersion: 'serving.kserve.io/v1alpha1',
+      kind: 'ServingRuntime',
+      metadata: {
+        name: 'test-serving-runtime-no-volumeMounts',
+        namespace: 'test-namespace',
+      },
+      spec: {
+        containers: [
+          {
+            name: 'test-container',
+          },
+        ],
+      },
+    };
+
+    const pvcName = 'new-nim-pvc';
+    const result = updateServingRuntimeTemplate(servingRuntimeWithoutVolumeMounts, pvcName);
+
+    expect(result.spec.containers[0].volumeMounts).toBeUndefined();
+  });
+});
diff --git a/frontend/src/pages/modelServing/screens/projects/nimUtils.ts b/frontend/src/pages/modelServing/screens/projects/nimUtils.ts
@@ -1,15 +1,19 @@
 // NGC stands for NVIDIA GPU Cloud.
 
-import { ProjectKind, SecretKind, TemplateKind } from '~/k8sTypes';
+import { K8sResourceCommon } from '@openshift/dynamic-plugin-sdk-utils';
+import { ProjectKind, SecretKind, ServingRuntimeKind, TemplateKind } from '~/k8sTypes';
 import { getTemplate } from '~/api';
 
 const NIM_SECRET_NAME = 'nvidia-nim-access';
 const NIM_NGC_SECRET_NAME = 'nvidia-nim-image-pull';
+const TEMPLATE_NAME = 'nvidia-nim-serving-template';
 
 export const getNGCSecretType = (isNGC: boolean): string =>
   isNGC ? 'kubernetes.io/dockerconfigjson' : 'Opaque';
 
-export const getNIMResource = async (resourceName: string): Promise<SecretKind> => {
+export const getNIMResource = async <T extends K8sResourceCommon = SecretKind>(
+  resourceName: string,
+): Promise<T> => {
   try {
     const response = await fetch(`/api/nim-serving/${resourceName}`, {
       method: 'GET',
@@ -56,8 +60,6 @@ export const isProjectNIMSupported = (currentProject: ProjectKind): boolean => {
 export const isNIMServingRuntimeTemplateAvailable = async (
   dashboardNamespace: string,
 ): Promise<boolean> => {
-  const TEMPLATE_NAME = 'nvidia-nim-serving-template';
-
   try {
     await getTemplate(TEMPLATE_NAME, dashboardNamespace);
     return true;
@@ -69,11 +71,55 @@ export const isNIMServingRuntimeTemplateAvailable = async (
 export const getNIMServingRuntimeTemplate = async (
   dashboardNamespace: string,
 ): Promise<TemplateKind | undefined> => {
-  const TEMPLATE_NAME = 'nvidia-nim-serving-template';
   try {
     const template = await getTemplate(TEMPLATE_NAME, dashboardNamespace);
     return template;
   } catch (error) {
     return undefined;
   }
 };
+
+export const updateServingRuntimeTemplate = (
+  servingRuntime: ServingRuntimeKind,
+  pvcName: string,
+): ServingRuntimeKind => {
+  const updatedServingRuntime = { ...servingRuntime };
+
+  updatedServingRuntime.spec.containers = updatedServingRuntime.spec.containers.map((container) => {
+    if (container.volumeMounts) {
+      const updatedVolumeMounts = container.volumeMounts.map((volumeMount) => {
+        if (volumeMount.mountPath === '/mnt/models/cache') {
+          return {
+            ...volumeMount,
+            name: pvcName,
+          };
+        }
+        return volumeMount;
+      });
+
+      return {
+        ...container,
+        volumeMounts: updatedVolumeMounts,
+      };
+    }
+    return container;
+  });
+
+  if (updatedServingRuntime.spec.volumes) {
+    const updatedVolumes = updatedServingRuntime.spec.volumes.map((volume) => {
+      if (volume.name === 'nim-pvc') {
+        return {
+          ...volume,
+          name: pvcName,
+          persistentVolumeClaim: {
+            claimName: pvcName,
+          },
+        };
+      }
+      return volume;
+    });
+
+    updatedServingRuntime.spec.volumes = updatedVolumes;
+  }
+  return updatedServingRuntime;
+};