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

Config map nim model name fetch #3276

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions backend/src/routes/api/nim-serving/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,38 @@ import { createCustomError } from '../../../utils/requestUtils';
import { logRequestDetails } from '../../../utils/fileUtils';

const secretNames = ['nvidia-nim-access', 'nvidia-nim-image-pull'];
const configMapName = 'nvidia-nim-images-data';

export default async (fastify: KubeFastifyInstance): Promise<void> => {
fastify.get(
'/:secretName',
'/:nimResource',
async (
request: OauthFastifyRequest<{
Params: { secretName: string };
Params: { nimResource: string };
}>,
) => {
logRequestDetails(fastify, request);
const { secretName } = request.params;
if (!secretNames.includes(secretName)) {
throw createCustomError('Not found', 'Secret not found', 404);
}
const { nimResource } = request.params;
const { coreV1Api, namespace } = fastify.kube;

return coreV1Api.readNamespacedSecret(secretName, namespace);
if (secretNames.includes(nimResource)) {
try {
return await coreV1Api.readNamespacedSecret(nimResource, namespace);
} catch (e) {
fastify.log.error(`Failed to fetch secret ${nimResource}: ${e.message}`);
throw createCustomError('Not found', 'Secret not found', 404);
}
}

if (nimResource === configMapName) {
try {
return await coreV1Api.readNamespacedConfigMap(configMapName, namespace);
} catch (e) {
fastify.log.error(`Failed to fetch configMap ${nimResource}: ${e.message}`);
throw createCustomError('Not found', 'ConfigMap not found', 404);
}
}
throw createCustomError('Not found', 'Resource not found', 404);
},
);
};
45 changes: 43 additions & 2 deletions frontend/src/api/k8s/servingRuntimes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
initialAcceleratorProfile?: AcceleratorProfileState,
selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState,
isModelMesh?: boolean,
nimPVCName?: string,
): ServingRuntimeKind => {
const {
name: displayName,
Expand Down Expand Up @@ -133,6 +134,15 @@
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,
Expand All @@ -145,7 +155,7 @@
...containerWithoutResources,
...(isModelMesh ? { resources } : {}),
affinity,
volumeMounts,
volumeMounts: updatedVolumeMounts,
};
},
);
Expand All @@ -171,8 +181,33 @@
volumes.push(getshmVolume('2Gi'));
}

updatedServingRuntime.spec.volumes = volumes;
if (nimPVCName) {
Copy link
Member

Choose a reason for hiding this comment

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

This code should never have been here -- you folks need to work on lowering your points of contact when fixing a blocker fix. You snuck this PVC thing into a high trafficked area for 2 other flows you're not touching instead of just adding this to your serving runtime template before ever calling this flow.

This change has already been tested and I'm going to let it in. But please do not sneak more "nice to have" code in when you need to fix critical blocking issues.

const updatedVolumes = volumes.map((volume) => {
if (volume.name === 'nim-pvc') {
return {

Check warning on line 187 in frontend/src/api/k8s/servingRuntimes.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/api/k8s/servingRuntimes.ts#L186-L187

Added lines #L186 - L187 were not covered by tests
...volume,
name: nimPVCName,
persistentVolumeClaim: {
claimName: nimPVCName,
},
};
}
return volume;

Check warning on line 195 in frontend/src/api/k8s/servingRuntimes.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/api/k8s/servingRuntimes.ts#L195

Added line #L195 was not covered by tests
});

if (!updatedVolumes.find((volume) => volume.name === nimPVCName)) {
updatedVolumes.push({

Check warning on line 199 in frontend/src/api/k8s/servingRuntimes.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/api/k8s/servingRuntimes.ts#L199

Added line #L199 was not covered by tests
name: nimPVCName,
persistentVolumeClaim: {
claimName: nimPVCName,
},
});
}

updatedServingRuntime.spec.volumes = updatedVolumes;

Check warning on line 207 in frontend/src/api/k8s/servingRuntimes.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/api/k8s/servingRuntimes.ts#L207

Added line #L207 was not covered by tests
} else {
updatedServingRuntime.spec.volumes = volumes;
}
return updatedServingRuntime;
};

Expand Down Expand Up @@ -242,6 +277,7 @@
initialAcceleratorProfile?: AcceleratorProfileState;
selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState;
isModelMesh?: boolean;
nimPVCName?: string;
Copy link
Member

Choose a reason for hiding this comment

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

This was unnecessary -- you don't support edit, updating the serving runtime is not done currently.

}): Promise<ServingRuntimeKind> => {
const {
data,
Expand All @@ -251,6 +287,7 @@
initialAcceleratorProfile,
selectedAcceleratorProfile,
isModelMesh,
nimPVCName,
} = options;

const updatedServingRuntime = assembleServingRuntime(
Expand All @@ -262,6 +299,7 @@
initialAcceleratorProfile,
selectedAcceleratorProfile,
isModelMesh,
nimPVCName,
);

return k8sUpdateResource<ServingRuntimeKind>(
Expand All @@ -284,6 +322,7 @@
initialAcceleratorProfile?: AcceleratorProfileState;
selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState;
isModelMesh?: boolean;
nimPVCName?: string;
}): Promise<ServingRuntimeKind> => {
const {
data,
Expand All @@ -294,6 +333,7 @@
initialAcceleratorProfile,
selectedAcceleratorProfile,
isModelMesh,
nimPVCName,
} = options;
const assembledServingRuntime = assembleServingRuntime(
data,
Expand All @@ -304,6 +344,7 @@
initialAcceleratorProfile,
selectedAcceleratorProfile,
isModelMesh,
nimPVCName,
);

return k8sCreateResource<ServingRuntimeKind>(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
Alert,
AlertActionCloseButton,
Form,
getUniqueId,
Modal,
Stack,
StackItem,
Expand Down Expand Up @@ -50,7 +51,6 @@

const NIM_SECRET_NAME = 'nvidia-nim-secrets';
const NIM_NGC_SECRET_NAME = 'ngc-secret';
const NIM_PVC_NAME = 'nim-pvc';

const accessReviewResource: AccessReviewResourceAttributes = {
group: 'rbac.authorization.k8s.io',
Expand Down Expand Up @@ -95,6 +95,7 @@
const isAuthorinoEnabled = useIsAreaAvailable(SupportedArea.K_SERVE_AUTH).status;
const currentProjectName = projectContext?.currentProject.metadata.name;
const namespace = currentProjectName || createDataInferenceService.project;
const nimPVCName = getUniqueId('nim-pvc');

Check warning on line 98 in frontend/src/pages/modelServing/screens/projects/NIMServiceModal/DeployNIMServiceModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/NIMServiceModal/DeployNIMServiceModal.tsx#L98

Added line #L98 was not covered by tests

const [translatedName] = translateDisplayNameForK8sAndReport(createDataInferenceService.name, {
maxLength: 253,
Expand Down Expand Up @@ -202,6 +203,7 @@
projectContext?.currentProject,
servingRuntimeName,
true,
nimPVCName,
);

const submitInferenceServiceResource = getSubmitInferenceServiceResourceFn(
Expand All @@ -226,7 +228,7 @@
submitInferenceServiceResource({ dryRun: false }),
createNIMSecret(namespace, NIM_SECRET_NAME, false, false),
createNIMSecret(namespace, NIM_NGC_SECRET_NAME, true, false),
createNIMPVC(namespace, NIM_PVC_NAME, pvcSize, false),
createNIMPVC(namespace, nimPVCName, pvcSize, false),
]),
)
.then(() => onSuccess())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
useEffect(() => {
const getModelNames = async () => {
try {
const modelInfos = await fetchNIMModelNames(dashboardNamespace);
const modelInfos = await fetchNIMModelNames();

Check warning on line 35 in frontend/src/pages/modelServing/screens/projects/NIMServiceModal/NIMModelListSection.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/modelServing/screens/projects/NIMServiceModal/NIMModelListSection.tsx#L35

Added line #L35 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

Your hook still has dashboardNamespace as a dependency... I think you're no longer using it in the hook... so please remove it.

if (modelInfos && modelInfos.length > 0) {
const fetchedOptions = modelInfos.flatMap((modelInfo) =>
modelInfo.tags.map((tag) => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,24 @@ import {
import { LabeledDataConnection, ServingPlatformStatuses } from '~/pages/modelServing/screens/types';
import { ServingRuntimePlatform } from '~/types';
import { mockInferenceServiceK8sResource } from '~/__mocks__/mockInferenceServiceK8sResource';
import { createPvc, createSecret, getConfigMap } from '~/api';
import { createPvc, createSecret } from '~/api';
import { PersistentVolumeClaimKind } from '~/k8sTypes';
import { getNGCSecretType, getNIMData } from '~/pages/modelServing/screens/projects/nimUtils';
import {
getNGCSecretType,
getNIMData,
getNIMResource,
} from '~/pages/modelServing/screens/projects/nimUtils';

jest.mock('~/api', () => ({
getSecret: jest.fn(),
createSecret: jest.fn(),
getConfigMap: jest.fn(),
createPvc: jest.fn(),
}));

jest.mock('~/pages/modelServing/screens/projects/nimUtils', () => ({
getNIMData: jest.fn(),
getNGCSecretType: jest.fn(),
getNIMResource: jest.fn(),
}));

describe('filterOutConnectionsWithoutBucket', () => {
Expand Down Expand Up @@ -312,7 +316,6 @@ describe('createNIMSecret', () => {
});
});
describe('fetchNIMModelNames', () => {
const dashboardNamespace = 'test-namespace';
const NIM_CONFIGMAP_NAME = 'nvidia-nim-images-data';

const configMapMock = {
Expand Down Expand Up @@ -341,11 +344,11 @@ describe('fetchNIMModelNames', () => {
});

it('should return model infos when configMap has data', async () => {
(getConfigMap as jest.Mock).mockResolvedValueOnce(configMapMock);
(getNIMResource as jest.Mock).mockResolvedValueOnce(configMapMock);

const result = await fetchNIMModelNames(dashboardNamespace);
const result = await fetchNIMModelNames();

expect(getConfigMap).toHaveBeenCalledWith(dashboardNamespace, NIM_CONFIGMAP_NAME);
expect(getNIMResource).toHaveBeenCalledWith(NIM_CONFIGMAP_NAME);
expect(result).toEqual([
{
name: 'model1',
Expand All @@ -369,20 +372,20 @@ describe('fetchNIMModelNames', () => {
});

it('should return undefined if configMap has no data', async () => {
(getConfigMap as jest.Mock).mockResolvedValueOnce({ data: {} });
(getNIMResource as jest.Mock).mockResolvedValueOnce({ data: {} });

const result = await fetchNIMModelNames(dashboardNamespace);
const result = await fetchNIMModelNames();

expect(getConfigMap).toHaveBeenCalledWith(dashboardNamespace, NIM_CONFIGMAP_NAME);
expect(getNIMResource).toHaveBeenCalledWith(NIM_CONFIGMAP_NAME);
expect(result).toBeUndefined();
});

it('should return undefined if configMap.data is not defined', async () => {
(getConfigMap as jest.Mock).mockResolvedValueOnce({ data: undefined });
(getNIMResource as jest.Mock).mockResolvedValueOnce({ data: undefined });

const result = await fetchNIMModelNames(dashboardNamespace);
const result = await fetchNIMModelNames();

expect(getConfigMap).toHaveBeenCalledWith(dashboardNamespace, NIM_CONFIGMAP_NAME);
expect(getNIMResource).toHaveBeenCalledWith(NIM_CONFIGMAP_NAME);
expect(result).toBeUndefined();
});
});
Expand Down
14 changes: 7 additions & 7 deletions frontend/src/pages/modelServing/screens/projects/nimUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
export const getNGCSecretType = (isNGC: boolean): string =>
isNGC ? 'kubernetes.io/dockerconfigjson' : 'Opaque';

const getNIMSecretData = async (secretName: string): Promise<SecretKind> => {
export const getNIMResource = async (resourceName: string): Promise<SecretKind> => {
Copy link
Member

Choose a reason for hiding this comment

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

Not just a SecretKind anymore

Suggested change
export const getNIMResource = async (resourceName: string): Promise<SecretKind> => {
export const getNIMResource = async <T extends K8sResourceCommon = SecretKind>(resourceName: string): Promise<T> => {

In your one location you call this, you'll want to call it like:

const configMap = await getNIMResource<ConfigMapKind>(NIM_CONFIGMAP_NAME);

See my next comment.

try {
const response = await fetch(`/api/nim-serving/${secretName}`, {
const response = await fetch(`/api/nim-serving/${resourceName}`, {
method: 'GET',
headers: {
'Content-Type': 'application/json',
Expand All @@ -21,16 +21,16 @@
if (!response.ok) {
throw new Error(`Error fetching secret: ${response.statusText}`);
}
const secretData = await response.json();
return secretData.body;
const resourceData = await response.json();
return resourceData.body;

Check warning on line 25 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#L25

Added line #L25 was not covered by tests
} catch (error) {
throw new Error(`Failed to fetch secret: ${secretName}.`);
throw new Error(`Failed to fetch the resource: ${resourceName}.`);

Check warning on line 27 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#L27

Added line #L27 was not covered by tests
}
};
export const getNIMData = async (isNGC: boolean): Promise<Record<string, string> | undefined> => {
const nimSecretData: SecretKind = isNGC
? await getNIMSecretData(NIM_NGC_SECRET_NAME)
: await getNIMSecretData(NIM_SECRET_NAME);
? await getNIMResource(NIM_NGC_SECRET_NAME)
: await getNIMResource(NIM_SECRET_NAME);

Check warning on line 33 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#L32-L33

Added lines #L32 - L33 were not covered by tests

if (!nimSecretData.data) {
throw new Error(`Error retrieving NIM ${isNGC ? 'NGC' : ''} secret data`);
Expand Down
16 changes: 10 additions & 6 deletions frontend/src/pages/modelServing/screens/projects/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,18 @@ import {
createPvc,
createSecret,
createServingRuntime,
getConfigMap,
updateInferenceService,
updateServingRuntime,
} from '~/api';
import { isDataConnectionAWS } from '~/pages/projects/screens/detail/data-connections/utils';
import { removeLeadingSlash } from '~/utilities/string';
import { RegisteredModelDeployInfo } from '~/pages/modelRegistry/screens/RegisteredModels/useRegisteredModelDeployInfo';
import { AcceleratorProfileSelectFieldState } from '~/pages/notebookController/screens/server/AcceleratorProfileSelectField';
import { getNGCSecretType, getNIMData } from '~/pages/modelServing/screens/projects/nimUtils';
import {
getNGCSecretType,
getNIMData,
getNIMResource,
} from '~/pages/modelServing/screens/projects/nimUtils';

const NIM_CONFIGMAP_NAME = 'nvidia-nim-images-data';

Expand Down Expand Up @@ -449,6 +452,7 @@ export const getSubmitServingRuntimeResourcesFn = (
currentProject?: ProjectKind,
name?: string,
isModelMesh?: boolean,
nimPVCName?: string,
): ((opts: { dryRun?: boolean }) => Promise<void | (string | void | ServingRuntimeKind)[]>) => {
if (!servingRuntimeSelected) {
return () =>
Expand Down Expand Up @@ -498,6 +502,7 @@ export const getSubmitServingRuntimeResourcesFn = (
selectedAcceleratorProfile: controlledState,
initialAcceleratorProfile,
isModelMesh,
nimPVCName,
}),
setUpTokenAuth(
servingRuntimeData,
Expand All @@ -524,6 +529,7 @@ export const getSubmitServingRuntimeResourcesFn = (
selectedAcceleratorProfile: controlledState,
initialAcceleratorProfile,
isModelMesh,
nimPVCName,
}).then((servingRuntime) =>
setUpTokenAuth(
servingRuntimeData,
Expand Down Expand Up @@ -579,10 +585,8 @@ export interface ModelInfo {
updatedDate: string;
}

export const fetchNIMModelNames = async (
dashboardNamespace: string,
): Promise<ModelInfo[] | undefined> => {
const configMap = await getConfigMap(dashboardNamespace, NIM_CONFIGMAP_NAME);
export const fetchNIMModelNames = async (): Promise<ModelInfo[] | undefined> => {
const configMap = await getNIMResource(NIM_CONFIGMAP_NAME);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const configMap = await getNIMResource(NIM_CONFIGMAP_NAME);
const configMap = await getNIMResource<ConfigMapKind>(NIM_CONFIGMAP_NAME);

if (configMap.data && Object.keys(configMap.data).length > 0) {
const modelInfos: ModelInfo[] = [];
for (const [key, value] of Object.entries(configMap.data)) {
Expand Down
Loading