From b80669df7e0e9a54aef3c8f335047d4fd627c3e9 Mon Sep 17 00:00:00 2001 From: Juntao Wang Date: Tue, 26 Sep 2023 16:24:57 -0400 Subject: [PATCH 1/2] Log group config errors to the pod and refine logic --- .../api/groups-config/groupsConfigUtil.ts | 79 ++++++++++--------- backend/src/routes/api/groups-config/index.ts | 9 ++- backend/src/utils/groupsUtils.ts | 2 +- frontend/src/services/groupSettingsService.ts | 4 +- .../__tests__/useWatchGroups.spec.ts | 34 ++++++++ frontend/src/utilities/useWatchGroups.tsx | 16 ++-- 6 files changed, 91 insertions(+), 53 deletions(-) create mode 100644 frontend/src/utilities/__tests__/useWatchGroups.spec.ts diff --git a/backend/src/routes/api/groups-config/groupsConfigUtil.ts b/backend/src/routes/api/groups-config/groupsConfigUtil.ts index 488993efb8..44b20e348b 100644 --- a/backend/src/routes/api/groups-config/groupsConfigUtil.ts +++ b/backend/src/routes/api/groups-config/groupsConfigUtil.ts @@ -16,19 +16,12 @@ const SYSTEM_AUTHENTICATED = 'system:authenticated'; export const getGroupsConfig = async (fastify: KubeFastifyInstance): Promise => { const customObjectsApi = fastify.kube.customObjectsApi; - try { - const groupsCluster = await getAllGroups(customObjectsApi); - const groupsData = getGroupsCR(); - const groupsProcessed = processGroupData(groupsData); - const groupsConfigProcessed = processGroupConfig(groupsProcessed, groupsCluster); - await removeDeletedGroups(fastify, groupsData, groupsConfigProcessed.groupsCRData); - - return groupsConfigProcessed.groupsConfig; - } catch (e) { - fastify.log.error(e, 'Error retrieving group configuration.'); - const error = createError(500, 'Error retrieving group configuration'); - throw error; - } + const groupsCluster = await getAllGroups(customObjectsApi); + const groupsData = getGroupsCR(); + const groupsProcessed = processGroupData(groupsData); + const groupsConfigProcessed = processGroupConfig(fastify, groupsProcessed, groupsCluster); + await removeDeletedGroups(fastify, groupsData, groupsConfigProcessed.groupsCRData); + return groupsConfigProcessed.groupsConfig; }; const transformGroupsConfig = (groupStatus: GroupStatus[]): string[] => { @@ -38,7 +31,7 @@ const transformGroupsConfig = (groupStatus: GroupStatus[]): string[] => { export const updateGroupsConfig = async ( fastify: KubeFastifyInstance, request: FastifyRequest<{ Body: GroupsConfig }>, -): Promise<{ success: GroupsConfig | null; error: string | null }> => { +): Promise => { const customObjectsApi = fastify.kube.customObjectsApi; const { namespace } = fastify.kube; @@ -58,26 +51,17 @@ export const updateGroupsConfig = async ( const error = createError(403, 'Error, groups cannot be empty'); throw error; } - try { - const dataUpdated: GroupsConfigBody = { - adminGroups: adminConfig.join(','), - allowedGroups: allowedConfig.join(','), - }; - - const groupsData = await updateGroupsCR(fastify, dataUpdated); - const groupsProcessed = processGroupData(groupsData); - const groupsCluster = await getAllGroups(customObjectsApi); - const updatedConfig = processGroupConfig(groupsProcessed, groupsCluster); - await removeDeletedGroups(fastify, groupsData, updatedConfig.groupsCRData); - return { - success: updatedConfig.groupsConfig, - error: null, - }; - } catch (e) { - fastify.log.error(e, 'Error updating group configuration.'); - const error = createError(500, 'Error updating group configuration'); - throw error; - } + const dataUpdated: GroupsConfigBody = { + adminGroups: adminConfig.join(','), + allowedGroups: allowedConfig.join(','), + }; + + const groupsData = await updateGroupsCR(fastify, dataUpdated); + const groupsProcessed = processGroupData(groupsData); + const groupsCluster = await getAllGroups(customObjectsApi); + const updatedConfig = processGroupConfig(fastify, groupsProcessed, groupsCluster); + await removeDeletedGroups(fastify, groupsData, updatedConfig.groupsCRData); + return updatedConfig.groupsConfig; }; const processGroupData = (groupsData: GroupsConfigBody): GroupsConfigBodyList => { @@ -105,6 +89,7 @@ const mapListToGroupStatus = * @returns Processed object with the groups, removing missing groups that might be selected */ const processGroupConfig = ( + fastify: KubeFastifyInstance, groupsDataList: GroupsConfigBodyList, groups: string[], ): { groupsConfig: GroupsConfig; groupsCRData: GroupsConfigBody } => { @@ -120,9 +105,14 @@ const processGroupConfig = ( const groupsConfig: GroupsConfig = { adminGroups: adminGroupsConfig, allowedGroups: allowedGroupsConfig, - errorAdmin: getError(groupsDataList.adminGroups, (group) => !groups.includes(group)), + errorAdmin: getError( + fastify, + groupsDataList.adminGroups.filter((group) => group), + (group) => !groups.includes(group), + ), errorUser: getError( - groupsDataList.allowedGroups, + fastify, + groupsDataList.allowedGroups.filter((group) => group), (group) => !groups.includes(group) && group !== SYSTEM_AUTHENTICATED, ), }; @@ -137,13 +127,26 @@ const processGroupConfig = ( return { groupsConfig, groupsCRData: updatedBody }; }; -const getError = (array: string[], predicate: (group: string) => boolean): string | undefined => { +const getError = ( + fastify: KubeFastifyInstance, + array: string[], + predicate: (group: string) => boolean, +): string | undefined => { + let error; + if (array.length === 0) { + error = 'No group is set in the group config, please set one or more group.'; + fastify.log.error(error); + return error; + } + const missingItems = array.filter(predicate); if (missingItems.length === 0) return undefined; - return `The group${missingItems.length === 1 ? '' : 's'} ${missingItems.join( + error = `The group${missingItems.length === 1 ? '' : 's'} ${missingItems.join( ', ', )} no longer exists in OpenShift and has been removed from the selected group list.`; + fastify.log.error(error); + return error; }; /** diff --git a/backend/src/routes/api/groups-config/index.ts b/backend/src/routes/api/groups-config/index.ts index e2b07c58ac..9b70b5e59e 100644 --- a/backend/src/routes/api/groups-config/index.ts +++ b/backend/src/routes/api/groups-config/index.ts @@ -6,8 +6,13 @@ import { secureAdminRoute } from '../../../utils/route-security'; export default async (fastify: FastifyInstance): Promise => { fastify.get( '/', - secureAdminRoute(fastify)(async () => { - return getGroupsConfig(fastify); + secureAdminRoute(fastify)(async (request, reply) => { + return getGroupsConfig(fastify).catch((e) => { + fastify.log.error( + `Error retrieving group configuration, ${e.response?.body?.message || e.message}`, + ); + reply.status(500).send({ message: e.response?.body?.message || e.message }); + }); }), ); diff --git a/backend/src/utils/groupsUtils.ts b/backend/src/utils/groupsUtils.ts index cca1ad8077..2cfc266b7a 100644 --- a/backend/src/utils/groupsUtils.ts +++ b/backend/src/utils/groupsUtils.ts @@ -16,7 +16,7 @@ export class MissingGroupError extends Error { } export const getGroupsCR = (): GroupsConfigBody => { - if (typeof getDashboardConfig().spec.groupsConfig !== 'undefined') { + if (getDashboardConfig().spec.groupsConfig) { return getDashboardConfig().spec.groupsConfig; } throw new Error(`Failed to retrieve Dashboard CR groups configuration`); diff --git a/frontend/src/services/groupSettingsService.ts b/frontend/src/services/groupSettingsService.ts index 1704c8518f..2e957a342d 100644 --- a/frontend/src/services/groupSettingsService.ts +++ b/frontend/src/services/groupSettingsService.ts @@ -11,9 +11,7 @@ export const fetchGroupsSettings = (): Promise => { }); }; -export const updateGroupsSettings = ( - settings: GroupsConfig, -): Promise<{ success: GroupsConfig | null; error: string | null }> => { +export const updateGroupsSettings = (settings: GroupsConfig): Promise => { const url = '/api/groups-config'; return axios .put(url, settings) diff --git a/frontend/src/utilities/__tests__/useWatchGroups.spec.ts b/frontend/src/utilities/__tests__/useWatchGroups.spec.ts new file mode 100644 index 0000000000..1474cfb4d8 --- /dev/null +++ b/frontend/src/utilities/__tests__/useWatchGroups.spec.ts @@ -0,0 +1,34 @@ +import { testHook } from '~/__tests__/unit/testUtils/hooks'; +import { fetchGroupsSettings } from '~/services/groupSettingsService'; +import { useWatchGroups } from '~/utilities/useWatchGroups'; + +jest.mock('~/services/groupSettingsService', () => ({ + fetchGroupsSettings: jest.fn(), +})); + +jest.mock('react-redux', () => ({ + useDispatch: jest.fn(), +})); + +const fetchGroupSettingsMock = fetchGroupsSettings as jest.Mock; + +describe('useWatchGroups', () => { + it('should fetch groups successfully', async () => { + const mockEmptyGroupSettings = { + adminGroups: [], + allowedGroups: [], + }; + const mockGroupSettings = { + adminGroups: ['odh-admins'], + allowedGroups: [], + }; + fetchGroupSettingsMock.mockReturnValue(Promise.resolve(mockGroupSettings)); + + const renderResult = testHook(useWatchGroups)(); + expect(fetchGroupSettingsMock).toHaveBeenCalledTimes(1); + expect(renderResult.result.current.groupSettings).toStrictEqual(mockEmptyGroupSettings); + + await renderResult.waitForNextUpdate(); + expect(renderResult.result.current.groupSettings).toStrictEqual(mockGroupSettings); + }); +}); diff --git a/frontend/src/utilities/useWatchGroups.tsx b/frontend/src/utilities/useWatchGroups.tsx index 66311036e4..46bb7aadfc 100644 --- a/frontend/src/utilities/useWatchGroups.tsx +++ b/frontend/src/utilities/useWatchGroups.tsx @@ -48,10 +48,10 @@ export const useWatchGroups = (): { React.useEffect(() => { if (errorAdmin) { - notification.error(`Group no longer exists`, errorAdmin); + notification.error(`Group error`, errorAdmin); } if (errorUser) { - notification.error(`Group no longer exists`, errorUser); + notification.error(`Group error`, errorUser); } }, [errorAdmin, errorUser, notification]); @@ -59,13 +59,11 @@ export const useWatchGroups = (): { setIsLoading(true); updateGroupsSettings(group) .then((response) => { - if (response.success) { - setGroupSettings(response.success); - notification.success( - 'Group settings changes saved', - 'It may take up to 2 minutes for configuration changes to be applied.', - ); - } + setGroupSettings(response); + notification.success( + 'Group settings changes saved', + 'It may take up to 2 minutes for configuration changes to be applied.', + ); }) .catch((error) => { setLoadError(error); From a5f345828d59062e4e3d54ed80a3b36efde4f7e7 Mon Sep 17 00:00:00 2001 From: Andrew Ballantyne Date: Wed, 18 Oct 2023 15:13:17 -0400 Subject: [PATCH 2/2] Expose the DSC status for the client --- backend/src/routes/api/dsc/index.ts | 12 ++++++++++ backend/src/types.ts | 36 ++++++++++++++++++++++++----- backend/src/utils/dsc.ts | 26 +++++++++++++++++++++ frontend/src/k8sTypes.ts | 17 ++++++++++++++ manifests/base/cluster-role.yaml | 8 +++++++ 5 files changed, 93 insertions(+), 6 deletions(-) create mode 100644 backend/src/routes/api/dsc/index.ts create mode 100644 backend/src/utils/dsc.ts diff --git a/backend/src/routes/api/dsc/index.ts b/backend/src/routes/api/dsc/index.ts new file mode 100644 index 0000000000..bae485b6db --- /dev/null +++ b/backend/src/routes/api/dsc/index.ts @@ -0,0 +1,12 @@ +import { KubeFastifyInstance } from '../../../types'; +import { secureRoute } from '../../../utils/route-security'; +import { getClusterStatus } from '../../../utils/dsc'; + +module.exports = async (fastify: KubeFastifyInstance) => { + fastify.get( + '/status', + secureRoute(fastify)(async () => { + return getClusterStatus(fastify); + }), + ); +}; diff --git a/backend/src/types.ts b/backend/src/types.ts index 1b8e0e6e1e..6c0a4de077 100644 --- a/backend/src/types.ts +++ b/backend/src/types.ts @@ -254,7 +254,6 @@ export type KubeDecorator = KubeStatus & { customObjectsApi: k8s.CustomObjectsApi; rbac: k8s.RbacAuthorizationV1Api; currentToken: string; - }; export type KubeFastifyInstance = FastifyInstance & { @@ -759,10 +758,10 @@ export type GPUInfo = { export type AcceleratorInfo = { configured: boolean; - available: {[key: string]: number}; - total: {[key: string]: number}; - allocated: {[key: string]: number}; -} + available: { [key: string]: number }; + total: { [key: string]: number }; + allocated: { [key: string]: number }; +}; export type EnvironmentVariable = EitherNotBoth< { value: string | number }, @@ -882,7 +881,6 @@ export type SupportedModelFormats = { autoSelect?: boolean; }; - export enum ContainerResourceAttributes { CPU = 'cpu', MEMORY = 'memory', @@ -947,3 +945,29 @@ export enum KnownLabels { MODEL_SERVING_PROJECT = 'modelmesh-enabled', DATA_CONNECTION_AWS = 'opendatahub.io/managed', } + +type ComponentNames = + | 'codeflare' + | 'data-science-pipelines-operator' + | 'kserve' + | 'model-mesh' + // Bug: https://github.com/opendatahub-io/opendatahub-operator/issues/641 + | 'odh-dashboard' + | 'ray' + | 'workbenches'; + +export type DataScienceClusterKindStatus = { + conditions: []; + installedComponents: { [key in ComponentNames]: boolean }; + phase?: string; +}; + +export type DataScienceClusterKind = K8sResourceCommon & { + spec: unknown; // we should never need to look into this + status: DataScienceClusterKindStatus; +}; + +export type DataScienceClusterList = { + kind: 'DataScienceClusterList'; + items: DataScienceClusterKind[]; +}; diff --git a/backend/src/utils/dsc.ts b/backend/src/utils/dsc.ts new file mode 100644 index 0000000000..eae01bc1e1 --- /dev/null +++ b/backend/src/utils/dsc.ts @@ -0,0 +1,26 @@ +import { + DataScienceClusterKind, + DataScienceClusterKindStatus, + DataScienceClusterList, + KubeFastifyInstance, +} from '../types'; +import { createCustomError } from './requestUtils'; + +export const getClusterStatus = async ( + fastify: KubeFastifyInstance, +): Promise => { + const result: DataScienceClusterKind | null = await fastify.kube.customObjectsApi + .listClusterCustomObject('datasciencecluster.opendatahub.io', 'v1', 'datascienceclusters') + .then((res) => (res.body as DataScienceClusterList).items[0]) + .catch((e) => { + fastify.log.error(`Failure to fetch dsc: ${e.response.body}`); + return null; + }); + + if (!result) { + // May not be using v2 Operator + throw createCustomError('DSC Unavailable', 'Unable to get status', 404); + } + + return result.status; +}; diff --git a/frontend/src/k8sTypes.ts b/frontend/src/k8sTypes.ts index 329d5008be..bf50b108f1 100644 --- a/frontend/src/k8sTypes.ts +++ b/frontend/src/k8sTypes.ts @@ -769,3 +769,20 @@ export type K8sResourceListResult> continue: string; }; }; + +type ComponentNames = + | 'codeflare' + | 'data-science-pipelines-operator' + | 'kserve' + | 'model-mesh' + // Bug: https://github.com/opendatahub-io/opendatahub-operator/issues/641 + | 'odh-dashboard' + | 'ray' + | 'workbenches'; + +/** We don't need or should ever get the full kind, this is the status section */ +export type DataScienceClusterKindStatus = { + conditions: K8sCondition[]; + installedComponents: { [key in ComponentNames]: boolean }; + phase?: string; +}; diff --git a/manifests/base/cluster-role.yaml b/manifests/base/cluster-role.yaml index d21d47cd04..228eaeed7c 100644 --- a/manifests/base/cluster-role.yaml +++ b/manifests/base/cluster-role.yaml @@ -164,3 +164,11 @@ rules: - delete resources: - notebooks + - apiGroups: + - datasciencecluster.opendatahub.io + verbs: + - list + - watch + - get + resources: + - datascienceclusters