Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added ability to deploy more than one NIM model. #3453

Merged
merged 9 commits into from
Nov 13, 2024
40 changes: 38 additions & 2 deletions frontend/src/api/k8s/__tests__/pvcs.spec.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,21 @@
import {
K8sStatus,
k8sCreateResource,
k8sDeleteResource,
k8sGetResource,
k8sListResourceItems,
K8sStatus,
k8sUpdateResource,
} from '@openshift/dynamic-plugin-sdk-utils';
import { mock200Status, mock404Error } from '~/__mocks__/mockK8sStatus';
import { mockPVCK8sResource } from '~/__mocks__/mockPVCK8sResource';
import { assemblePvc, createPvc, deletePvc, getDashboardPvcs, updatePvc } from '~/api/k8s/pvcs';
import {
assemblePvc,
createPvc,
deletePvc,
getDashboardPvcs,
getPvc,
updatePvc,
} from '~/api/k8s/pvcs';
import { PVCModel } from '~/api/models/k8s';
import { PersistentVolumeClaimKind } from '~/k8sTypes';
import { CreatingStorageObject } from '~/pages/projects/types';
Expand All @@ -25,6 +33,7 @@ const k8sListResourceItemsMock = jest.mocked(k8sListResourceItems<PersistentVolu
const k8sCreateResourceMock = jest.mocked(k8sCreateResource<PersistentVolumeClaimKind>);
const k8sUpdateResourceMock = jest.mocked(k8sUpdateResource<PersistentVolumeClaimKind>);
const k8sDeleteResourceMock = jest.mocked(k8sDeleteResource<PersistentVolumeClaimKind, K8sStatus>);
const k8sGetResourceMock = jest.mocked(k8sGetResource<PersistentVolumeClaimKind>);

const data: CreatingStorageObject = {
nameDesc: {
Expand Down Expand Up @@ -193,3 +202,30 @@ describe('deletePvc', () => {
});
});
});

describe('getPvc', () => {
it('should fetch and return PVC', async () => {
k8sGetResourceMock.mockResolvedValue(pvcMock);
const result = await getPvc('projectName', 'pvcName');

expect(k8sGetResourceMock).toHaveBeenCalledWith({
fetchOptions: { requestInit: {} },
model: PVCModel,
queryOptions: { name: 'pvcName', ns: 'projectName', queryParams: {} },
});
expect(k8sGetResourceMock).toHaveBeenCalledTimes(1);
expect(result).toStrictEqual(pvcMock);
});

it('should handle errors and rethrow', async () => {
k8sGetResourceMock.mockRejectedValue(new Error('error1'));

await expect(getPvc('projectName', 'pvcName')).rejects.toThrow('error1');
expect(k8sGetResourceMock).toHaveBeenCalledTimes(1);
expect(k8sGetResourceMock).toHaveBeenCalledWith({
fetchOptions: { requestInit: {} },
model: PVCModel,
queryOptions: { name: 'pvcName', ns: 'projectName', queryParams: {} },
});
});
});
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import * as React from 'react';
import DeleteModal from '~/pages/projects/components/DeleteModal';
import { InferenceServiceKind, ServingRuntimeKind } from '~/k8sTypes';
import { deleteInferenceService, deletePvc, deleteSecret, deleteServingRuntime } from '~/api';
import { deleteInferenceService, deleteServingRuntime } from '~/api';
import { getDisplayNameFromK8sResource } from '~/concepts/k8s/utils';
import { byName, ProjectsContext } from '~/concepts/projects/ProjectsContext';
import { isProjectNIMSupported } from '~/pages/modelServing/screens/projects/nimUtils';
import {
getNIMResourcesToDelete,
isProjectNIMSupported,
} from '~/pages/modelServing/screens/projects/nimUtils';

type DeleteInferenceServiceModalProps = {
inferenceService?: InferenceServiceKind;
Expand Down Expand Up @@ -34,62 +37,43 @@
? getDisplayNameFromK8sResource(inferenceService)
: 'this deployed model';

const onDelete = async () => {
andrewballantyne marked this conversation as resolved.
Show resolved Hide resolved
if (!inferenceService) {
return;

Check warning on line 42 in frontend/src/pages/modelServing/screens/global/DeleteInferenceServiceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/global/DeleteInferenceServiceModal.tsx#L42

Added line #L42 was not covered by tests
}

setIsDeleting(true);
try {
const nimResourcesToDelete =
isKServeNIMEnabled && project && servingRuntime
? await getNIMResourcesToDelete(project.metadata.name, servingRuntime)
: [];

await Promise.all([
deleteInferenceService(inferenceService.metadata.name, inferenceService.metadata.namespace),
...(servingRuntime
? [deleteServingRuntime(servingRuntime.metadata.name, servingRuntime.metadata.namespace)]
: []),

Check warning on line 56 in frontend/src/pages/modelServing/screens/global/DeleteInferenceServiceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/global/DeleteInferenceServiceModal.tsx#L56

Added line #L56 was not covered by tests
...nimResourcesToDelete,
]);

onBeforeClose(true);
} catch (e: unknown) {
if (e instanceof Error) {
setError(e);
} else {
setError(new Error('An unknown error occurred'));

Check warning on line 65 in frontend/src/pages/modelServing/screens/global/DeleteInferenceServiceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/global/DeleteInferenceServiceModal.tsx#L64-L65

Added lines #L64 - L65 were not covered by tests
}
setIsDeleting(false);
}
};

return (
<DeleteModal
title="Delete deployed model?"
onClose={() => onBeforeClose(false)}
submitButtonLabel="Delete deployed model"
onDelete={() => {
if (inferenceService) {
setIsDeleting(true);
const pvcName = servingRuntime?.spec.volumes?.find(
(vol) => vol.persistentVolumeClaim?.claimName,
)?.persistentVolumeClaim?.claimName;
const containerWithEnv = servingRuntime?.spec.containers.find(
(container) =>
container.env && container.env.some((env) => env.valueFrom?.secretKeyRef?.name),
);
const nimSecretName = containerWithEnv?.env?.find(
(env) => env.valueFrom?.secretKeyRef?.name,
)?.valueFrom?.secretKeyRef?.name;
const imagePullSecretName = servingRuntime?.spec.imagePullSecrets?.[0]?.name ?? '';
Promise.all([
deleteInferenceService(
inferenceService.metadata.name,
inferenceService.metadata.namespace,
),
...(servingRuntime
? [
deleteServingRuntime(
servingRuntime.metadata.name,
servingRuntime.metadata.namespace,
),
]
: []),
...(isKServeNIMEnabled && pvcName
? [deletePvc(pvcName, inferenceService.metadata.namespace)]
: []),
...(isKServeNIMEnabled &&
project &&
nimSecretName &&
nimSecretName.length > 0 &&
imagePullSecretName.length > 0
? [
deleteSecret(project.metadata.name, nimSecretName),
deleteSecret(project.metadata.name, imagePullSecretName),
]
: []),
])

.then(() => {
onBeforeClose(true);
})
.catch((e) => {
setError(e);
setIsDeleting(false);
});
}
}}
onDelete={onDelete}
deleting={isDeleting}
error={error}
deleteName={displayName}
Expand All @@ -98,5 +82,4 @@
</DeleteModal>
);
};

export default DeleteInferenceServiceModal;
Original file line number Diff line number Diff line change
Expand Up @@ -222,23 +222,19 @@
shouldShowPlatformSelection || platformError || emptyModelServer
? undefined
: [
...(!isKServeNIMEnabled
? [
<ModelServingPlatformButtonAction
isProjectModelMesh={isProjectModelMesh}
testId={`${isProjectModelMesh ? 'add-server' : 'deploy'}-button`}
emptyTemplates={emptyTemplates}
onClick={() => {
setPlatformSelected(
isProjectModelMesh
? ServingRuntimePlatform.MULTI
: ServingRuntimePlatform.SINGLE,
);
}}
key="serving-runtime-actions"
/>,
]
: []),
<ModelServingPlatformButtonAction
isProjectModelMesh={isProjectModelMesh}
testId={`${isProjectModelMesh ? 'add-server' : 'deploy'}-button`}
emptyTemplates={emptyTemplates}
onClick={() => {
setPlatformSelected(
isProjectModelMesh
? ServingRuntimePlatform.MULTI
: ServingRuntimePlatform.SINGLE,

Check warning on line 233 in frontend/src/pages/modelServing/screens/projects/ModelServingPlatform.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/ModelServingPlatform.tsx#L233

Added line #L233 was not covered by tests
);
}}
key="serving-runtime-actions"
/>,
]
}
description={
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
translateDisplayNameForK8s,
translateDisplayNameForK8sAndReport,
} from '~/concepts/k8s/utils';
import { updatePvc, useAccessReview } from '~/api';
import { getSecret, updatePvc, useAccessReview } from '~/api';
import { SupportedArea, useIsAreaAvailable } from '~/concepts/areas';
import KServeAutoscalerReplicaSection from '~/pages/modelServing/screens/projects/kServeModal/KServeAutoscalerReplicaSection';
import NIMPVCSizeSection from '~/pages/modelServing/screens/projects/NIMServiceModal/NIMPVCSizeSection';
Expand Down Expand Up @@ -169,6 +169,15 @@
}
}, [dashboardNamespace, editInfo]);

const isSecretNeeded = async (ns: string, secretName: string): Promise<boolean> => {
try {
await getSecret(ns, secretName);
return false; // Secret exists, no need to create

Check warning on line 175 in frontend/src/pages/modelServing/screens/projects/NIMServiceModal/ManageNIMServingModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/NIMServiceModal/ManageNIMServingModal.tsx#L175

Added line #L175 was not covered by tests
} catch {
return true; // Secret does not exist, needs to be created
}
};

const onBeforeClose = (submitted: boolean) => {
onClose(submitted);
setError(undefined);
Expand Down Expand Up @@ -239,14 +248,21 @@
submitServingRuntimeResources({ dryRun: false }).then(() => undefined),
submitInferenceServiceResource({ dryRun: false }).then(() => undefined),
];

if (!editInfo) {
promises.push(
createNIMSecret(namespace, NIM_SECRET_NAME, false, false).then(() => undefined),
createNIMSecret(namespace, NIM_NGC_SECRET_NAME, true, false).then(() => undefined),
createNIMPVC(namespace, nimPVCName, pvcSize, false).then(() => undefined),
);
if (await isSecretNeeded(namespace, NIM_SECRET_NAME)) {
promises.push(
createNIMSecret(namespace, NIM_SECRET_NAME, false, false).then(() => undefined),
);
}
if (await isSecretNeeded(namespace, NIM_NGC_SECRET_NAME)) {
promises.push(
createNIMSecret(namespace, NIM_NGC_SECRET_NAME, true, false).then(() => undefined),
);
}
promises.push(createNIMPVC(namespace, nimPVCName, pvcSize, false).then(() => undefined));
} else if (pvc && pvc.spec.resources.requests.storage !== pvcSize) {
const createData: CreatingStorageObject = {
const updatePvcData: CreatingStorageObject = {

Check warning on line 265 in frontend/src/pages/modelServing/screens/projects/NIMServiceModal/ManageNIMServingModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/NIMServiceModal/ManageNIMServingModal.tsx#L265

Added line #L265 was not covered by tests
size: pvcSize, // New size
nameDesc: {
name: pvc.metadata.name,
Expand All @@ -255,7 +271,7 @@
storageClassName: pvc.spec.storageClassName,
};
promises.push(
updatePvc(createData, pvc, namespace, { dryRun: false }).then(() => undefined),
updatePvc(updatePvcData, pvc, namespace, { dryRun: false }).then(() => undefined),

Check warning on line 274 in frontend/src/pages/modelServing/screens/projects/NIMServiceModal/ManageNIMServingModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/NIMServiceModal/ManageNIMServingModal.tsx#L274

Added line #L274 was not covered by tests
);
}
return Promise.all(promises);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ jest.mock('~/api', () => ({
getSecret: jest.fn(),
createSecret: jest.fn(),
createPvc: jest.fn(),
getInferenceServiceContext: jest.fn(),
}));

jest.mock('~/pages/modelServing/screens/projects/nimUtils', () => ({
Expand Down
64 changes: 62 additions & 2 deletions frontend/src/pages/modelServing/screens/projects/nimUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

import { K8sResourceCommon } from '@openshift/dynamic-plugin-sdk-utils';
import { ProjectKind, SecretKind, ServingRuntimeKind, TemplateKind } from '~/k8sTypes';
import { getTemplate } from '~/api';
import { deletePvc, deleteSecret, getTemplate } from '~/api';
import { fetchInferenceServiceCount } from '~/pages/modelServing/screens/projects/utils';

const NIM_SECRET_NAME = 'nvidia-nim-access';
const NIM_NGC_SECRET_NAME = 'nvidia-nim-image-pull';
Expand Down Expand Up @@ -107,7 +108,7 @@

if (updatedServingRuntime.spec.volumes) {
const updatedVolumes = updatedServingRuntime.spec.volumes.map((volume) => {
if (volume.name === 'nim-pvc') {
if (volume.name.startsWith('nim-pvc')) {
return {
...volume,
name: pvcName,
Expand All @@ -123,3 +124,62 @@
}
return updatedServingRuntime;
};

export const getNIMResourcesToDelete = async (
Copy link
Member

@andrewballantyne andrewballantyne Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway we can get a unit test for this? It seems the logic is not working as expected (at least on the cluster)

projectName: string,
servingRuntime: ServingRuntimeKind,
): Promise<Promise<void>[]> => {
const resourcesToDelete: Promise<void>[] = [];

let inferenceCount = 0;

try {
inferenceCount = await fetchInferenceServiceCount(projectName);
} catch (error) {
if (error instanceof Error) {

Check warning on line 139 in frontend/src/pages/modelServing/screens/projects/nimUtils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/nimUtils.ts#L139

Added line #L139 was not covered by tests
// eslint-disable-next-line no-console
console.error(

Check warning on line 141 in frontend/src/pages/modelServing/screens/projects/nimUtils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/nimUtils.ts#L141

Added line #L141 was not covered by tests
`Failed to fetch inference service count for project "${projectName}": ${error.message}`,
);
} else {

Check warning on line 144 in frontend/src/pages/modelServing/screens/projects/nimUtils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/nimUtils.ts#L144

Added line #L144 was not covered by tests
// eslint-disable-next-line no-console
console.error(

Check warning on line 146 in frontend/src/pages/modelServing/screens/projects/nimUtils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/nimUtils.ts#L146

Added line #L146 was not covered by tests
`Failed to fetch inference service count for project "${projectName}": ${error}`,
);
}
}

const pvcName = servingRuntime.spec.volumes?.find((vol) =>
vol.persistentVolumeClaim?.claimName.startsWith('nim-pvc'),
)?.persistentVolumeClaim?.claimName;

if (pvcName) {
resourcesToDelete.push(deletePvc(pvcName, projectName).then(() => undefined));

Check warning on line 157 in frontend/src/pages/modelServing/screens/projects/nimUtils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/nimUtils.ts#L157

Added line #L157 was not covered by tests
}

let nimSecretName: string | undefined;
let imagePullSecretName: string | undefined;

const pullNGCSecret = servingRuntime.spec.imagePullSecrets?.[0]?.name ?? '';
if (pullNGCSecret === 'ngc-secret') {
imagePullSecretName = pullNGCSecret;

Check warning on line 165 in frontend/src/pages/modelServing/screens/projects/nimUtils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/nimUtils.ts#L165

Added line #L165 was not covered by tests
}

servingRuntime.spec.containers.forEach((container) => {
container.env?.forEach((env) => {
const secretName = env.valueFrom?.secretKeyRef?.name;
if (secretName === 'nvidia-nim-secrets') {
nimSecretName = secretName;

Check warning on line 172 in frontend/src/pages/modelServing/screens/projects/nimUtils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/nimUtils.ts#L170-L172

Added lines #L170 - L172 were not covered by tests
}
});
});

if (nimSecretName && imagePullSecretName && inferenceCount === 1) {
resourcesToDelete.push(
deleteSecret(projectName, nimSecretName).then(() => undefined),
deleteSecret(projectName, imagePullSecretName).then(() => undefined),

Check warning on line 180 in frontend/src/pages/modelServing/screens/projects/nimUtils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/nimUtils.ts#L178-L180

Added lines #L178 - L180 were not covered by tests
);
}

return resourcesToDelete;
};
14 changes: 14 additions & 0 deletions frontend/src/pages/modelServing/screens/projects/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
createPvc,
createSecret,
createServingRuntime,
getInferenceServiceContext,
updateInferenceService,
updateServingRuntime,
} from '~/api';
Expand Down Expand Up @@ -709,3 +710,16 @@

export const isConnectionPathValid = (path: string): boolean =>
!(containsOnlySlashes(path) || !isS3PathValid(path) || path === '');

export const fetchInferenceServiceCount = async (namespace: string): Promise<number> => {
try {
const inferenceServices = await getInferenceServiceContext(namespace);
return inferenceServices.length;
} catch (error) {
throw new Error(

Check warning on line 719 in frontend/src/pages/modelServing/screens/projects/utils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/utils.ts#L719

Added line #L719 was not covered by tests
`Failed to fetch inference services for namespace "${namespace}": ${
error instanceof Error ? error.message : error

Check warning on line 721 in frontend/src/pages/modelServing/screens/projects/utils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/utils.ts#L721

Added line #L721 was not covered by tests
}`,
);
}
};
Loading