Skip to content

Commit

Permalink
Nim work for 2.16 (#3484)
Browse files Browse the repository at this point in the history
* feat: added ability to deploy more than one NIM model. (#3453)

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

Signed-off-by: Olga Lavtar <[email protected]>

* feat: refactored NIM related logic to nimUtils

Signed-off-by: Olga Lavtar <[email protected]>

* feat: changes to the error handling

Signed-off-by: Olga Lavtar <[email protected]>

* feat: changes to the error handling

Signed-off-by: Olga Lavtar <[email protected]>

* feat: changes .some for .forEach

Signed-off-by: Olga Lavtar <[email protected]>

* feat: deleting secrets fix

Signed-off-by: Olga Lavtar <[email protected]>

* feat: deploying the same model pvc fix

Signed-off-by: Olga Lavtar <[email protected]>

* feat: added a unit test for getNIMResourcesToDelete

Signed-off-by: Olga Lavtar <[email protected]>

* feat: will add a unit test for getNIMResourcesToDelete later

Signed-off-by: Olga Lavtar <[email protected]>

---------

Signed-off-by: Olga Lavtar <[email protected]>

* feat: Modify NIM enablement process (#3455)

* Modify NIM enablement process

* Clean up code and remove unnecessary manifests

* Add logic to check the conditions of the odh-nim-account CR

* Added more check for enabled integration apps

* If failed to fetch integration app status, should show error on the related tile in the enable application page, should not remove the tile

* check integration app status in explore application page

* Fix lint issue

* Fix lint issue

* add annotations to the secret and the account CR

* Update backend/src/routes/api/integrations/nim/index.ts

Co-authored-by: Andrew Ballantyne <[email protected]>

* Update backend/src/routes/api/components/list.ts

Co-authored-by: Andrew Ballantyne <[email protected]>

* clean up

* feat: listing all NIM accounts in the namespace, returning the first one.

Signed-off-by: Olga Lavtar <[email protected]>

* Avoid mutating object in useWatchIntegrationComponents

* Clean up

* feat: added logic for displaying the tile correctly

Signed-off-by: Olga Lavtar <[email protected]>

* feat: changes for enabling

Signed-off-by: Olga Lavtar <[email protected]>

* feat: updated mock component with the new properties.

Signed-off-by: Olga Lavtar <[email protected]>

* feat: addressed PR comments with updates and improvements

Signed-off-by: Olga Lavtar <[email protected]>

* feat: backend bug fix

Signed-off-by: Olga Lavtar <[email protected]>

* feat: Missed change from previous commit

Signed-off-by: Olga Lavtar <[email protected]>

* feat: fix for the enabled page and enabled.cy.ts

Signed-off-by: Olga Lavtar <[email protected]>

---------

Signed-off-by: Olga Lavtar <[email protected]>
Co-authored-by: Andrew Ballantyne <[email protected]>
Co-authored-by: Olga Lavtar <[email protected]>

* Fix NIM selection issue (#3482)

* Fix NIM selection issue

* Switch back to using a numerical value

---------

Signed-off-by: Olga Lavtar <[email protected]>
Co-authored-by: olavtar <[email protected]>
Co-authored-by: yu zhao <[email protected]>
Co-authored-by: Olga Lavtar <[email protected]>
  • Loading branch information
4 people authored Nov 15, 2024
1 parent b09ef1b commit 411cc1e
Show file tree
Hide file tree
Showing 32 changed files with 837 additions and 678 deletions.
11 changes: 9 additions & 2 deletions backend/src/routes/api/components/list.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import { FastifyRequest } from 'fastify';
import { KubeFastifyInstance, OdhApplication } from '../../../types';
import { getApplications, updateApplications } from '../../../utils/resourceUtils';
import {
getApplications,
updateApplications,
isIntegrationApp,
} from '../../../utils/resourceUtils';
import { checkJupyterEnabled, getRouteForApplication } from '../../../utils/componentUtils';

export const listComponents = async (
Expand All @@ -17,7 +21,10 @@ export const listComponents = async (
return Promise.resolve(applications);
}
for (const app of applications) {
if (app.spec.shownOnEnabledPage) {
if (isIntegrationApp(app)) {
// Include all integration apps -- Client can check if it's enabled
installedComponents.push(app);
} else if (app.spec.shownOnEnabledPage) {
const newApp = {
...app,
spec: {
Expand Down
97 changes: 97 additions & 0 deletions backend/src/routes/api/integrations/nim/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
import { FastifyReply, FastifyRequest } from 'fastify';
import { secureAdminRoute } from '../../../../utils/route-security';
import { KubeFastifyInstance } from '../../../../types';
import { isString } from 'lodash';
import { createNIMAccount, createNIMSecret, getNIMAccount, isAppEnabled } from './nimUtils';

module.exports = async (fastify: KubeFastifyInstance) => {
const { namespace } = fastify.kube;
const PAGE_NOT_FOUND_MESSAGE = '404 page not found';

fastify.get(
'/',
secureAdminRoute(fastify)(async (request: FastifyRequest, reply: FastifyReply) => {
await getNIMAccount(fastify, namespace)
.then((response) => {
if (response) {
// Installed
const isEnabled = isAppEnabled(response);
reply.send({ isInstalled: true, isEnabled: isEnabled, canInstall: false, error: '' });
} else {
// Not installed
fastify.log.info(`NIM account does not exist`);
reply.send({ isInstalled: false, isEnabled: false, canInstall: true, error: '' });
}
})
.catch((e) => {
if (e.response?.statusCode === 404) {
// 404 error means the Account CRD does not exist, so cannot create CR based on it.
if (
isString(e.response.body) &&
e.response.body.trim() === PAGE_NOT_FOUND_MESSAGE.trim()
) {
fastify.log.info(`NIM not installed, ${e.response?.body}`);
reply.send({
isInstalled: false,
isEnabled: false,
canInstall: false,
error: 'NIM not installed',
});
}
} else {
fastify.log.error(`An unexpected error occurred: ${e.message || e}`);
reply.send({
isInstalled: false,
isAppEnabled: false,
canInstall: false,
error: 'An unexpected error occurred. Please try again later.',
});
}
});
}),
);

fastify.post(
'/',
secureAdminRoute(fastify)(
async (
request: FastifyRequest<{
Body: { [key: string]: string };
}>,
reply: FastifyReply,
) => {
const enableValues = request.body;

await createNIMSecret(fastify, namespace, enableValues)
.then(async () => {
await createNIMAccount(fastify, namespace)
.then((response) => {
const isEnabled = isAppEnabled(response);
reply.send({
isInstalled: true,
isEnabled: isEnabled,
canInstall: false,
error: '',
});
})
.catch((e) => {
const message = `Failed to create NIM account, ${e.response?.body?.message}`;
fastify.log.error(message);
reply.status(e.response.statusCode).send(new Error(message));
});
})
.catch((e) => {
if (e.response?.statusCode === 409) {
fastify.log.error(`NIM secret already exists, skipping creation.`);
reply.status(409).send(new Error(`NIM secret already exists, skipping creation.`));
} else {
fastify.log.error(`Failed to create NIM secret. ${e.response?.body?.message}`);
reply
.status(e.response.statusCode)
.send(new Error(`Failed to create NIM secret, ${e.response?.body?.message}`));
}
});
},
),
);
};
91 changes: 91 additions & 0 deletions backend/src/routes/api/integrations/nim/nimUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import { KubeFastifyInstance, NIMAccountKind, SecretKind } from '../../../../types';

const NIM_SECRET_NAME = 'nvidia-nim-access';
const NIM_ACCOUNT_NAME = 'odh-nim-account';

export const isAppEnabled = (app: NIMAccountKind): boolean => {
const conditions = app?.status?.conditions || [];
return (
conditions.find(
(condition) => condition.type === 'AccountStatus' && condition.status === 'True',
) !== undefined
);
};

export const getNIMAccount = async (
fastify: KubeFastifyInstance,
namespace: string,
): Promise<NIMAccountKind | undefined> => {
const { customObjectsApi } = fastify.kube;
try {
const response = await customObjectsApi.listNamespacedCustomObject(
'nim.opendatahub.io',
'v1',
namespace,
'accounts',
);
// Get the list of accounts from the response
const accounts = response.body as {
items: NIMAccountKind[];
};

return accounts.items[0] || undefined;
} catch (e) {
return Promise.reject(e);
}
};

export const createNIMAccount = async (
fastify: KubeFastifyInstance,
namespace: string,
): Promise<NIMAccountKind> => {
const { customObjectsApi } = fastify.kube;
const account = {
apiVersion: 'nim.opendatahub.io/v1',
kind: 'Account',
metadata: {
name: NIM_ACCOUNT_NAME,
namespace,
labels: {
'opendatahub.io/managed': 'true',
},
},
spec: {
apiKeySecret: {
name: NIM_SECRET_NAME,
},
},
};
const response = await customObjectsApi.createNamespacedCustomObject(
'nim.opendatahub.io',
'v1',
namespace,
'accounts',
account,
);
return Promise.resolve(response.body as NIMAccountKind);
};

export const createNIMSecret = async (
fastify: KubeFastifyInstance,
namespace: string,
enableValues: { [key: string]: string },
): Promise<SecretKind> => {
const { coreV1Api } = fastify.kube;
const nimSecret = {
apiVersion: 'v1',
kind: 'Secret',
metadata: {
name: NIM_SECRET_NAME,
namespace,
labels: {
'opendatahub.io/managed': 'true',
},
},
type: 'Opaque',
stringData: enableValues,
};

const response = await coreV1Api.createNamespacedSecret(namespace, nimSecret);
return Promise.resolve(response.body as SecretKind);
};
25 changes: 25 additions & 0 deletions backend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ export type OdhApplication = {
displayName: string;
docsLink: string;
hidden?: boolean | null;
internalRoute?: string;
enable?: {
actionLabel: string;
description?: string;
Expand Down Expand Up @@ -1228,3 +1229,27 @@ export enum ServiceAddressAnnotation {
EXTERNAL_REST = 'routing.opendatahub.io/external-address-rest',
EXTERNAL_GRPC = 'routing.opendatahub.io/external-address-grpc',
}

export type NIMAccountKind = K8sResourceCommon & {
metadata: {
name: string;
namespace: string;
};
spec: {
apiKeySecret: {
name: string;
};
};
status?: {
nimConfig?: {
name: string;
};
runtimeTemplate?: {
name: string;
};
nimPullSecret?: {
name: string;
};
conditions?: K8sCondition[];
};
};
55 changes: 31 additions & 24 deletions backend/src/utils/resourceUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -332,33 +332,38 @@ export const fetchApplications = async (
.then((result) => result.body)
.catch(() => null);
for (const appDef of applicationDefs) {
appDef.spec.shownOnEnabledPage = enabledAppsCM?.data?.[appDef.metadata.name] === 'true';
appDef.spec.isEnabled = await getIsAppEnabled(fastify, appDef).catch((e) => {
fastify.log.warn(
`"${
appDef.metadata.name
}" OdhApplication is being disabled due to an error determining if it's enabled. ${
e.response?.body?.message || e.message
}`,
);
if (isIntegrationApp(appDef)) {
// Ignore logic for apps that use internal routes for status information
applications.push(appDef);
} else {
appDef.spec.shownOnEnabledPage = enabledAppsCM?.data?.[appDef.metadata.name] === 'true';
appDef.spec.isEnabled = await getIsAppEnabled(fastify, appDef).catch((e) => {
fastify.log.warn(
`"${
appDef.metadata.name
}" OdhApplication is being disabled due to an error determining if it's enabled. ${
e.response?.body?.message || e.message
}`,
);

return false;
});
if (appDef.spec.isEnabled) {
if (!appDef.spec.shownOnEnabledPage) {
changed = true;
enabledAppsCMData[appDef.metadata.name] = 'true';
appDef.spec.shownOnEnabledPage = true;
return false;
});
if (appDef.spec.isEnabled) {
if (!appDef.spec.shownOnEnabledPage) {
changed = true;
enabledAppsCMData[appDef.metadata.name] = 'true';
appDef.spec.shownOnEnabledPage = true;
}
}
applications.push({
...appDef,
spec: {
...appDef.spec,
getStartedLink: getRouteForClusterId(fastify, appDef.spec.getStartedLink),
link: appDef.spec.isEnabled ? await getRouteForApplication(fastify, appDef) : undefined,
},
});
}
applications.push({
...appDef,
spec: {
...appDef.spec,
getStartedLink: getRouteForClusterId(fastify, appDef.spec.getStartedLink),
link: appDef.spec.isEnabled ? await getRouteForApplication(fastify, appDef) : undefined,
},
});
}
if (changed) {
// write enabled apps configmap
Expand Down Expand Up @@ -1037,3 +1042,5 @@ export const translateDisplayNameForK8s = (name: string): string =>
.toLowerCase()
.replace(/\s/g, '-')
.replace(/[^A-Za-z0-9-]/g, '');
export const isIntegrationApp = (app: OdhApplication): boolean =>
app.spec.internalRoute?.startsWith('/api/');
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: {} },
});
});
});
1 change: 1 addition & 0 deletions frontend/src/concepts/areas/const.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,5 +122,6 @@ export const SupportedAreasStateMap: SupportedAreasState = {
},
[SupportedArea.NIM_MODEL]: {
featureFlags: ['disableNIMModelServing'],
reliantAreas: [SupportedArea.K_SERVE],
},
};
Loading

0 comments on commit 411cc1e

Please sign in to comment.