From 6fdafbae655677d3d10a71f50979eea5a1d2ac12 Mon Sep 17 00:00:00 2001 From: Purva Naik Date: Tue, 10 Dec 2024 21:31:37 +0530 Subject: [PATCH 01/10] Hardware profile empty state (#3543) --- backend/src/types.ts | 1 + backend/src/utils/constants.ts | 1 + docs/dashboard-config.md | 3 + frontend/src/__mocks__/mockDashboardConfig.ts | 3 + frontend/src/__mocks__/mockHardwareProfile.ts | 71 +++++ frontend/src/api/index.ts | 1 + .../k8s/__tests__/hardwareProfiles.spec.ts | 255 ++++++++++++++++++ frontend/src/api/k8s/hardwareProfiles.ts | 82 ++++++ frontend/src/api/models/odh.ts | 7 + frontend/src/app/AppRoutes.tsx | 5 + frontend/src/concepts/areas/const.ts | 4 + frontend/src/concepts/areas/types.ts | 1 + frontend/src/k8sTypes.ts | 17 ++ .../hardwareProfiles/HardwareProfiles.tsx | 75 ++++++ .../HardwareProfilesRoutes.tsx | 12 + .../__tests__/useHardwareProfile.spec.ts | 75 ++++++ .../__tests__/useHardwareProfiles.spec.ts | 68 +++++ .../hardwareProfiles/useHardwareProfile.ts | 24 ++ .../hardwareProfiles/useHardwareProfiles.ts | 11 + frontend/src/types.ts | 13 + frontend/src/utilities/NavData.tsx | 10 + .../hardwareprofiles.opendatahub.io.crd.yaml | 98 +++++++ manifests/common/crd/kustomization.yaml | 1 + ...dhdashboardconfigs.opendatahub.io.crd.yaml | 2 + .../core-bases/base/fetch-hardwares.rbac.yaml | 26 ++ manifests/core-bases/base/kustomization.yaml | 1 + 26 files changed, 867 insertions(+) create mode 100644 frontend/src/__mocks__/mockHardwareProfile.ts create mode 100644 frontend/src/api/k8s/__tests__/hardwareProfiles.spec.ts create mode 100644 frontend/src/api/k8s/hardwareProfiles.ts create mode 100644 frontend/src/pages/hardwareProfiles/HardwareProfiles.tsx create mode 100644 frontend/src/pages/hardwareProfiles/HardwareProfilesRoutes.tsx create mode 100644 frontend/src/pages/hardwareProfiles/__tests__/useHardwareProfile.spec.ts create mode 100644 frontend/src/pages/hardwareProfiles/__tests__/useHardwareProfiles.spec.ts create mode 100644 frontend/src/pages/hardwareProfiles/useHardwareProfile.ts create mode 100644 frontend/src/pages/hardwareProfiles/useHardwareProfiles.ts create mode 100644 manifests/common/crd/hardwareprofiles.opendatahub.io.crd.yaml create mode 100644 manifests/core-bases/base/fetch-hardwares.rbac.yaml diff --git a/backend/src/types.ts b/backend/src/types.ts index 3c0fec5d22..c29a17d571 100644 --- a/backend/src/types.ts +++ b/backend/src/types.ts @@ -37,6 +37,7 @@ export type DashboardConfig = K8sResourceCommon & { disableKServeRaw: boolean; disableModelMesh: boolean; disableAcceleratorProfiles: boolean; + disableHardwareProfiles: boolean; disableDistributedWorkloads: boolean; disableModelRegistry: boolean; disableServingRuntimeParams: boolean; diff --git a/backend/src/utils/constants.ts b/backend/src/utils/constants.ts index e053331c33..9e1a94a3c7 100644 --- a/backend/src/utils/constants.ts +++ b/backend/src/utils/constants.ts @@ -62,6 +62,7 @@ export const blankDashboardCR: DashboardConfig = { disableKServeRaw: true, disableModelMesh: false, disableAcceleratorProfiles: false, + disableHardwareProfiles: true, disableDistributedWorkloads: false, disableModelRegistry: false, disableServingRuntimeParams: false, diff --git a/docs/dashboard-config.md b/docs/dashboard-config.md index ba712ffbcc..c4eeb8d7a4 100644 --- a/docs/dashboard-config.md +++ b/docs/dashboard-config.md @@ -31,6 +31,7 @@ The following are a list of features that are supported, along with there defaul | disableKServeRaw | true | Disables the option to deploy in raw instead of serverless. | | disableModelMesh | false | Disables the ability to select ModelMesh as a Serving Platform. | | disableAcceleratorProfiles | false | Disables Accelerator profiles from the Admin Panel. | +| disableHardwareProfiles | true | Disables Hardware profiles from the Admin Panel. | | disableTrustyBiasMetrics | false | Disables Model Bias tab from Model Serving metrics. | | disablePerformanceMetrics | false | Disables Endpoint Performance tab from Model Serving metrics. | | disableDistributedWorkloads | false | Disables Distributed Workload Metrics from the dashboard. | @@ -62,6 +63,7 @@ spec: disableProjectSharing: false disableCustomServingRuntimes: false disableAcceleratorProfiles: false + disableHardwareProfiles: true disableKServeMetrics: false disableTrustyBiasMetrics: false disablePerformanceMetrics: false @@ -158,6 +160,7 @@ spec: disableProjectSharing: true disableCustomServingRuntimes: false disableAcceleratorProfiles: true + disableHardwareProfiles: true disableKServeMetrics: true disableTrustyBiasMetrics: false disablePerformanceMetrics: false diff --git a/frontend/src/__mocks__/mockDashboardConfig.ts b/frontend/src/__mocks__/mockDashboardConfig.ts index d0c40465fd..d3724fa78b 100644 --- a/frontend/src/__mocks__/mockDashboardConfig.ts +++ b/frontend/src/__mocks__/mockDashboardConfig.ts @@ -21,6 +21,7 @@ export type MockDashboardConfigType = { disableKServeRaw?: boolean; disableModelMesh?: boolean; disableAcceleratorProfiles?: boolean; + disableHardwareProfiles?: boolean; disablePerformanceMetrics?: boolean; disableTrustyBiasMetrics?: boolean; disableDistributedWorkloads?: boolean; @@ -53,6 +54,7 @@ export const mockDashboardConfig = ({ disableKServeRaw = true, disableModelMesh = false, disableAcceleratorProfiles = false, + disableHardwareProfiles = false, disablePerformanceMetrics = false, disableTrustyBiasMetrics = false, disableDistributedWorkloads = false, @@ -164,6 +166,7 @@ export const mockDashboardConfig = ({ disableKServeRaw, disableModelMesh, disableAcceleratorProfiles, + disableHardwareProfiles, disableDistributedWorkloads, disableModelRegistry, disableServingRuntimeParams, diff --git a/frontend/src/__mocks__/mockHardwareProfile.ts b/frontend/src/__mocks__/mockHardwareProfile.ts new file mode 100644 index 0000000000..a1a9eca282 --- /dev/null +++ b/frontend/src/__mocks__/mockHardwareProfile.ts @@ -0,0 +1,71 @@ +import { HardwareProfileKind } from '~/k8sTypes'; +import { + Identifier, + NodeSelector, + Toleration, + TolerationEffect, + TolerationOperator, +} from '~/types'; +import { genUID } from './mockUtils'; + +type MockResourceConfigType = { + name?: string; + namespace?: string; + uid?: string; + displayName?: string; + identifiers?: Identifier[]; + description?: string; + enabled?: boolean; + nodeSelectors?: NodeSelector[]; + tolerations?: Toleration[]; +}; + +export const mockHardwareProfile = ({ + name = 'migrated-gpu', + namespace = 'test-project', + uid = genUID('service'), + displayName = 'Nvidia GPU', + identifiers = [ + { + displayName: 'Memory', + identifier: 'memory', + minCount: 5, + maxCount: 2, + defaultCount: 2, + }, + ], + description = '', + enabled = true, + tolerations = [ + { + key: 'nvidia.com/gpu', + operator: TolerationOperator.EXISTS, + effect: TolerationEffect.NO_SCHEDULE, + }, + ], + nodeSelectors = [ + { + key: 'test', + value: 'va;ue', + }, + ], +}: MockResourceConfigType): HardwareProfileKind => ({ + apiVersion: 'dashboard.opendatahub.io/v1alpha1', + kind: 'HardwareProfile', + metadata: { + creationTimestamp: '2023-03-17T16:12:41Z', + generation: 1, + name, + namespace, + resourceVersion: '1309350', + uid, + }, + spec: { + identifiers, + displayName, + enabled, + tolerations, + nodeSelectors, + description, + }, +}); diff --git a/frontend/src/api/index.ts b/frontend/src/api/index.ts index c9f464c809..ceee524c0f 100644 --- a/frontend/src/api/index.ts +++ b/frontend/src/api/index.ts @@ -19,6 +19,7 @@ export * from './k8s/groups'; export * from './k8s/templates'; export * from './k8s/dashboardConfig'; export * from './k8s/acceleratorProfiles'; +export * from './k8s/hardwareProfiles'; export * from './k8s/clusterQueues'; export * from './k8s/localQueues'; export * from './k8s/workloads'; diff --git a/frontend/src/api/k8s/__tests__/hardwareProfiles.spec.ts b/frontend/src/api/k8s/__tests__/hardwareProfiles.spec.ts new file mode 100644 index 0000000000..ba22cfbebb --- /dev/null +++ b/frontend/src/api/k8s/__tests__/hardwareProfiles.spec.ts @@ -0,0 +1,255 @@ +import { + k8sCreateResource, + k8sDeleteResource, + k8sGetResource, + k8sListResource, + K8sStatus, + k8sUpdateResource, +} from '@openshift/dynamic-plugin-sdk-utils'; +import { mockK8sResourceList } from '~/__mocks__/mockK8sResourceList'; +import { HardwareProfileKind } from '~/k8sTypes'; +import { HardwareProfileModel } from '~/api/models'; +import { + createHardwareProfile, + deleteHardwareProfile, + getHardwareProfile, + listHardwareProfiles, + updateHardwareProfile, +} from '~/api/k8s/hardwareProfiles'; +import { mockHardwareProfile } from '~/__mocks__/mockHardwareProfile'; +import { TolerationEffect, TolerationOperator } from '~/types'; +import { mock200Status, mock404Error } from '~/__mocks__'; + +jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({ + k8sListResource: jest.fn(), + k8sCreateResource: jest.fn(), + k8sUpdateResource: jest.fn(), + k8sDeleteResource: jest.fn(), + k8sGetResource: jest.fn(), +})); + +const mockListResource = jest.mocked(k8sListResource); +const mockGetResource = jest.mocked(k8sGetResource); +const k8sCreateResourceMock = jest.mocked(k8sCreateResource); +const k8sUpdateResourceMock = jest.mocked(k8sUpdateResource); +const k8sDeleteResourceMock = jest.mocked(k8sDeleteResource); + +const data: HardwareProfileKind['spec'] = { + displayName: 'test', + identifiers: [ + { + displayName: 'Memory', + identifier: 'memory', + minCount: 5, + maxCount: 2, + defaultCount: 2, + }, + ], + description: 'test description', + enabled: true, + tolerations: [ + { + key: 'nvidia.com/gpu', + operator: TolerationOperator.EXISTS, + effect: TolerationEffect.NO_SCHEDULE, + }, + ], +}; + +const assembleHardwareProfileResult: HardwareProfileKind = { + apiVersion: 'dashboard.opendatahub.io/v1alpha1', + kind: 'HardwareProfile', + metadata: { + name: 'test-1', + namespace: 'namespace', + }, + spec: data, +}; + +describe('listHardwareProfile', () => { + it('should fetch and return list of hardware profile', async () => { + const namespace = 'test-project'; + mockListResource.mockResolvedValue( + mockK8sResourceList([mockHardwareProfile({ uid: 'test-project-12' })]), + ); + + const result = await listHardwareProfiles(namespace); + expect(mockListResource).toHaveBeenCalledWith({ + model: HardwareProfileModel, + queryOptions: { + ns: namespace, + }, + }); + expect(mockListResource).toHaveBeenCalledTimes(1); + expect(result).toStrictEqual([mockHardwareProfile({ uid: 'test-project-12' })]); + }); + + it('should handle errors when fetching list of hardware profile', async () => { + const namespace = 'test-project'; + mockListResource.mockRejectedValue(new Error('error1')); + + await expect(listHardwareProfiles(namespace)).rejects.toThrow('error1'); + expect(mockListResource).toHaveBeenCalledTimes(1); + expect(mockListResource).toHaveBeenCalledWith({ + model: HardwareProfileModel, + queryOptions: { + ns: namespace, + }, + }); + }); +}); + +describe('getHardwareProfile', () => { + it('should fetch and return specific hardware profile', async () => { + const namespace = 'test-project'; + const projectName = 'test'; + mockGetResource.mockResolvedValue(mockHardwareProfile({ uid: 'test-project-12' })); + + const result = await getHardwareProfile(projectName, namespace); + expect(mockGetResource).toHaveBeenCalledWith({ + model: HardwareProfileModel, + queryOptions: { + name: projectName, + ns: namespace, + }, + }); + expect(mockGetResource).toHaveBeenCalledTimes(1); + expect(result).toStrictEqual(mockHardwareProfile({ uid: 'test-project-12' })); + }); + + it('should handle errors when fetching a specific hardware profile', async () => { + const namespace = 'test-project'; + const projectName = 'test'; + mockGetResource.mockRejectedValue(new Error('error1')); + + await expect(getHardwareProfile(projectName, namespace)).rejects.toThrow('error1'); + expect(mockGetResource).toHaveBeenCalledTimes(1); + expect(mockGetResource).toHaveBeenCalledWith({ + model: HardwareProfileModel, + queryOptions: { + name: projectName, + ns: namespace, + }, + }); + }); +}); + +describe('createHardwareProfiles', () => { + it('should create hadware profile', async () => { + k8sCreateResourceMock.mockResolvedValue(mockHardwareProfile({ uid: 'test' })); + const result = await createHardwareProfile('test-1', data, 'namespace'); + expect(k8sCreateResourceMock).toHaveBeenCalledWith({ + fetchOptions: { requestInit: {} }, + model: HardwareProfileModel, + queryOptions: { queryParams: {} }, + resource: assembleHardwareProfileResult, + }); + expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1); + expect(result).toStrictEqual(mockHardwareProfile({ uid: 'test' })); + }); + + it('should handle errors and rethrow', async () => { + k8sCreateResourceMock.mockRejectedValue(new Error('error1')); + await expect(createHardwareProfile('test-1', data, 'namespace')).rejects.toThrow('error1'); + expect(k8sCreateResourceMock).toHaveBeenCalledWith({ + fetchOptions: { requestInit: {} }, + model: HardwareProfileModel, + queryOptions: { queryParams: {} }, + resource: assembleHardwareProfileResult, + }); + expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1); + }); +}); + +describe('updateHardwareProfile', () => { + it('should update hardware profile', async () => { + k8sUpdateResourceMock.mockResolvedValue( + mockHardwareProfile({ + uid: 'test', + displayName: 'test', + description: 'test description', + }), + ); + const result = await updateHardwareProfile( + data, + mockHardwareProfile({ uid: 'test-1' }), + 'namespace', + ); + expect(k8sUpdateResourceMock).toHaveBeenCalledWith({ + fetchOptions: { requestInit: {} }, + model: HardwareProfileModel, + queryOptions: { queryParams: {} }, + resource: mockHardwareProfile({ + uid: 'test-1', + namespace: 'namespace', + description: 'test description', + displayName: 'test', + }), + }); + expect(k8sUpdateResourceMock).toHaveBeenCalledTimes(1); + expect(result).toStrictEqual( + mockHardwareProfile({ + uid: 'test', + displayName: 'test', + description: 'test description', + }), + ); + }); + + it('should handle errors and rethrow', async () => { + k8sUpdateResourceMock.mockRejectedValue(new Error('error1')); + await expect( + updateHardwareProfile(data, mockHardwareProfile({ uid: 'test-1' }), 'namespace'), + ).rejects.toThrow('error1'); + expect(k8sUpdateResourceMock).toHaveBeenCalledTimes(1); + expect(k8sUpdateResourceMock).toHaveBeenCalledWith({ + fetchOptions: { requestInit: {} }, + model: HardwareProfileModel, + queryOptions: { queryParams: {} }, + resource: mockHardwareProfile({ + uid: 'test-1', + namespace: 'namespace', + description: 'test description', + displayName: 'test', + }), + }); + }); +}); + +describe('deleteHardwareProfile', () => { + it('should return status as Success', async () => { + const mockK8sStatus = mock200Status({}); + k8sDeleteResourceMock.mockResolvedValue(mockK8sStatus); + const result = await deleteHardwareProfile('hardwareProfileName', 'namespace'); + expect(k8sDeleteResourceMock).toHaveBeenCalledWith({ + model: HardwareProfileModel, + queryOptions: { name: 'hardwareProfileName', ns: 'namespace' }, + }); + expect(k8sDeleteResourceMock).toHaveBeenCalledTimes(1); + expect(result).toStrictEqual(mockK8sStatus); + }); + + it('should return status as Failure', async () => { + const mockK8sStatus = mock404Error({}); + k8sDeleteResourceMock.mockResolvedValue(mockK8sStatus); + const result = await deleteHardwareProfile('hardwareProfileName', 'namespace'); + expect(k8sDeleteResourceMock).toHaveBeenCalledWith({ + model: HardwareProfileModel, + queryOptions: { name: 'hardwareProfileName', ns: 'namespace' }, + }); + expect(k8sDeleteResourceMock).toHaveBeenCalledTimes(1); + expect(result).toStrictEqual(mockK8sStatus); + }); + + it('should handle errors and rethrow', async () => { + k8sDeleteResourceMock.mockRejectedValue(new Error('error1')); + await expect(deleteHardwareProfile('hardwareProfileName', 'namespace')).rejects.toThrow( + 'error1', + ); + expect(k8sDeleteResourceMock).toHaveBeenCalledTimes(1); + expect(k8sDeleteResourceMock).toHaveBeenCalledWith({ + model: HardwareProfileModel, + queryOptions: { name: 'hardwareProfileName', ns: 'namespace' }, + }); + }); +}); diff --git a/frontend/src/api/k8s/hardwareProfiles.ts b/frontend/src/api/k8s/hardwareProfiles.ts new file mode 100644 index 0000000000..4b87bace99 --- /dev/null +++ b/frontend/src/api/k8s/hardwareProfiles.ts @@ -0,0 +1,82 @@ +import * as _ from 'lodash-es'; +import { + k8sCreateResource, + k8sDeleteResource, + k8sGetResource, + k8sListResource, + K8sStatus, + k8sUpdateResource, +} from '@openshift/dynamic-plugin-sdk-utils'; +import { HardwareProfileKind, K8sAPIOptions } from '~/k8sTypes'; +import { HardwareProfileModel } from '~/api/models'; +import { applyK8sAPIOptions } from '~/api/apiMergeUtils'; +import { translateDisplayNameForK8s } from '~/concepts/k8s/utils'; + +export const listHardwareProfiles = async (namespace: string): Promise => + k8sListResource({ + model: HardwareProfileModel, + queryOptions: { + ns: namespace, + }, + }).then((listResource) => listResource.items); + +export const getHardwareProfile = (name: string, namespace: string): Promise => + k8sGetResource({ + model: HardwareProfileModel, + queryOptions: { name, ns: namespace }, + }); + +export const assembleHardwareProfile = ( + hardwareProfileName: string, + data: HardwareProfileKind['spec'], + namespace: string, +): HardwareProfileKind => ({ + apiVersion: `${HardwareProfileModel.apiGroup}/${HardwareProfileModel.apiVersion}`, + kind: HardwareProfileModel.kind, + metadata: { + name: hardwareProfileName || translateDisplayNameForK8s(data.displayName), + namespace, + }, + spec: data, +}); + +export const createHardwareProfile = ( + hardwareProfileName: string, + data: HardwareProfileKind['spec'], + namespace: string, + opts?: K8sAPIOptions, +): Promise => { + const resource = assembleHardwareProfile(hardwareProfileName, data, namespace); + return k8sCreateResource( + applyK8sAPIOptions( + { + model: HardwareProfileModel, + resource, + }, + opts, + ), + ); +}; + +export const updateHardwareProfile = ( + data: HardwareProfileKind['spec'], + existingHardwareProfile: HardwareProfileKind, + namespace: string, + opts?: K8sAPIOptions, +): Promise => { + const resource = assembleHardwareProfile(existingHardwareProfile.metadata.name, data, namespace); + const hardwareProfileResource = _.merge({}, existingHardwareProfile, resource); + + return k8sUpdateResource( + applyK8sAPIOptions({ model: HardwareProfileModel, resource: hardwareProfileResource }, opts), + ); +}; + +export const deleteHardwareProfile = ( + hardwareProfileName: string, + namespace: string, +): Promise => + k8sDeleteResource({ + model: HardwareProfileModel, + queryOptions: { name: hardwareProfileName, ns: namespace }, + }); diff --git a/frontend/src/api/models/odh.ts b/frontend/src/api/models/odh.ts index c16e5b013b..daf18aef99 100644 --- a/frontend/src/api/models/odh.ts +++ b/frontend/src/api/models/odh.ts @@ -14,6 +14,13 @@ export const AcceleratorProfileModel: K8sModelCommon = { plural: 'acceleratorprofiles', }; +export const HardwareProfileModel = { + apiVersion: 'v1alpha1', + apiGroup: 'dashboard.opendatahub.io', + kind: 'HardwareProfile', + plural: 'hardwareprofiles', +} satisfies K8sModelCommon; + export const NotebookModel: K8sModelCommon = { apiVersion: 'v1', apiGroup: 'kubeflow.org', diff --git a/frontend/src/app/AppRoutes.tsx b/frontend/src/app/AppRoutes.tsx index 1eab78e3d5..1749385da5 100644 --- a/frontend/src/app/AppRoutes.tsx +++ b/frontend/src/app/AppRoutes.tsx @@ -68,6 +68,10 @@ const AcceleratorProfileRoutes = React.lazy( () => import('../pages/acceleratorProfiles/AcceleratorProfilesRoutes'), ); +const HardwareProfileRoutes = React.lazy( + () => import('../pages/hardwareProfiles/HardwareProfilesRoutes'), +); + const StorageClassesPage = React.lazy(() => import('../pages/storageClasses/StorageClassesPage')); const ModelRegistryRoutes = React.lazy(() => import('../pages/modelRegistry/ModelRegistryRoutes')); @@ -131,6 +135,7 @@ const AppRoutes: React.FC = () => { } /> } /> } /> + } /> } /> {isConnectionTypesAvailable ? ( } /> diff --git a/frontend/src/concepts/areas/const.ts b/frontend/src/concepts/areas/const.ts index c444dfb6f8..42e4df9c24 100644 --- a/frontend/src/concepts/areas/const.ts +++ b/frontend/src/concepts/areas/const.ts @@ -25,6 +25,7 @@ export const allFeatureFlags: string[] = Object.keys({ disableKServeRaw: true, disableModelMesh: false, disableAcceleratorProfiles: false, + disableHardwareProfiles: false, disableDistributedWorkloads: false, disableModelRegistry: false, disableServingRuntimeParams: false, @@ -40,6 +41,9 @@ export const SupportedAreasStateMap: SupportedAreasState = { [SupportedArea.ACCELERATOR_PROFILES]: { featureFlags: ['disableAcceleratorProfiles'], }, + [SupportedArea.HARDWARE_PROFILES]: { + featureFlags: ['disableHardwareProfiles'], + }, [SupportedArea.CLUSTER_SETTINGS]: { featureFlags: ['disableClusterManager'], }, diff --git a/frontend/src/concepts/areas/types.ts b/frontend/src/concepts/areas/types.ts index b6d0032fb3..73710595a4 100644 --- a/frontend/src/concepts/areas/types.ts +++ b/frontend/src/concepts/areas/types.ts @@ -37,6 +37,7 @@ export enum SupportedArea { CLUSTER_SETTINGS = 'cluster-settings', USER_MANAGEMENT = 'user-management', ACCELERATOR_PROFILES = 'accelerator-profiles', + HARDWARE_PROFILES = 'hardware-profiles', CONNECTION_TYPES = 'connections-types', STORAGE_CLASSES = 'storage-classes', diff --git a/frontend/src/k8sTypes.ts b/frontend/src/k8sTypes.ts index d36b3863b8..4b89950116 100644 --- a/frontend/src/k8sTypes.ts +++ b/frontend/src/k8sTypes.ts @@ -5,8 +5,10 @@ import { StackComponent } from '~/concepts/areas/types'; import { ContainerResourceAttributes, ContainerResources, + Identifier, ImageStreamStatusTagCondition, ImageStreamStatusTagItem, + NodeSelector, NotebookSize, PodAffinity, PodContainer, @@ -1187,6 +1189,7 @@ export type DashboardCommonConfig = { disableKServeRaw: boolean; disableModelMesh: boolean; disableAcceleratorProfiles: boolean; + disableHardwareProfiles: boolean; disableDistributedWorkloads: boolean; disableModelRegistry: boolean; disableServingRuntimeParams: boolean; @@ -1232,6 +1235,20 @@ export type AcceleratorProfileKind = K8sResourceCommon & { }; }; +export type HardwareProfileKind = K8sResourceCommon & { + metadata: { + name: string; + }; + spec: { + displayName: string; + enabled: boolean; + description?: string; + tolerations?: Toleration[]; + identifiers?: Identifier[]; + nodeSelectors?: NodeSelector[]; + }; +}; + // In the SDK TResource extends from K8sResourceCommon, but both kind and apiVersion are mandatory export type K8sResourceListResult> = { apiVersion: string; diff --git a/frontend/src/pages/hardwareProfiles/HardwareProfiles.tsx b/frontend/src/pages/hardwareProfiles/HardwareProfiles.tsx new file mode 100644 index 0000000000..99c1a105e1 --- /dev/null +++ b/frontend/src/pages/hardwareProfiles/HardwareProfiles.tsx @@ -0,0 +1,75 @@ +import * as React from 'react'; +import { + Button, + ButtonVariant, + EmptyState, + EmptyStateVariant, + EmptyStateBody, + PageSection, + Title, + EmptyStateActions, + EmptyStateFooter, +} from '@patternfly/react-core'; +import { PlusCircleIcon } from '@patternfly/react-icons'; +import ApplicationsPage from '~/pages/ApplicationsPage'; +import { useDashboardNamespace } from '~/redux/selectors'; +import { ODH_PRODUCT_NAME } from '~/utilities/const'; +import useHardwareProfiles from './useHardwareProfiles'; + +const description = `Manage hardware profile settings for users in your organization.`; + +const HardwareProfiles: React.FC = () => { + const { dashboardNamespace } = useDashboardNamespace(); + const [hardwareProfiles, loaded, loadError] = useHardwareProfiles(dashboardNamespace); + + const isEmpty = hardwareProfiles.length === 0; + + const noHardwareProfilePageSection = ( + + + + No available hardware profiles yet + + + You don't have any hardware profiles yet. To get started, please ask your cluster + administrator about the hardware availability in your cluster and create corresponding + profiles in {ODH_PRODUCT_NAME}. + + + + + + + + + ); + + return ( + + {/* Todo: Create hardware table */} + + ); +}; + +export default HardwareProfiles; diff --git a/frontend/src/pages/hardwareProfiles/HardwareProfilesRoutes.tsx b/frontend/src/pages/hardwareProfiles/HardwareProfilesRoutes.tsx new file mode 100644 index 0000000000..8e4ea99a3d --- /dev/null +++ b/frontend/src/pages/hardwareProfiles/HardwareProfilesRoutes.tsx @@ -0,0 +1,12 @@ +import * as React from 'react'; +import { Navigate, Route, Routes } from 'react-router-dom'; +import HardwareProfiles from './HardwareProfiles'; + +const HardwareProfilesRoutes: React.FC = () => ( + + } /> + } /> + +); + +export default HardwareProfilesRoutes; diff --git a/frontend/src/pages/hardwareProfiles/__tests__/useHardwareProfile.spec.ts b/frontend/src/pages/hardwareProfiles/__tests__/useHardwareProfile.spec.ts new file mode 100644 index 0000000000..7ca48a4332 --- /dev/null +++ b/frontend/src/pages/hardwareProfiles/__tests__/useHardwareProfile.spec.ts @@ -0,0 +1,75 @@ +import { k8sGetResource } from '@openshift/dynamic-plugin-sdk-utils'; +import { act } from 'react'; +import { mockHardwareProfile } from '~/__mocks__/mockHardwareProfile'; +import { standardUseFetchState, testHook } from '~/__tests__/unit/testUtils/hooks'; +import { HardwareProfileModel } from '~/api'; +import { HardwareProfileKind } from '~/k8sTypes'; +import useHardwareProfile from '~/pages/hardwareProfiles/useHardwareProfile'; + +jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({ + k8sGetResource: jest.fn(), +})); + +const k8sGetResourceMock = jest.mocked(k8sGetResource); + +describe('useHardwareProfile', () => { + it('should return hardware profile', async () => { + k8sGetResourceMock.mockResolvedValue(mockHardwareProfile({ uid: 'test-1' })); + const options = { + model: HardwareProfileModel, + queryOptions: { name: 'migrated-gpu', ns: 'test' }, + }; + const renderResult = testHook(useHardwareProfile)('test', 'migrated-gpu'); + expect(k8sGetResourceMock).toHaveBeenCalledTimes(1); + expect(k8sGetResourceMock).toHaveBeenCalledWith(options); + expect(renderResult).hookToStrictEqual(standardUseFetchState(null)); + expect(renderResult).hookToHaveUpdateCount(1); + + // wait for update + await renderResult.waitForNextUpdate(); + expect(k8sGetResourceMock).toHaveBeenCalledTimes(1); + expect(renderResult).hookToStrictEqual( + standardUseFetchState(mockHardwareProfile({ uid: 'test-1' }), true), + ); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable([false, false, true, true]); + + // refresh + k8sGetResourceMock.mockResolvedValue(mockHardwareProfile({})); + await act(() => renderResult.result.current[3]()); + expect(k8sGetResourceMock).toHaveBeenCalledTimes(2); + expect(renderResult).hookToHaveUpdateCount(3); + expect(renderResult).hookToBeStable([false, true, true, true]); + }); + + it('should handle no name error', async () => { + const renderResult = testHook(useHardwareProfile)('test'); + expect(k8sGetResourceMock).not.toHaveBeenCalled(); + expect(renderResult).hookToStrictEqual(standardUseFetchState(null)); + expect(renderResult).hookToHaveUpdateCount(1); + }); + + it('should handle errors and rethrow', async () => { + k8sGetResourceMock.mockRejectedValue(new Error('error1')); + + const renderResult = testHook(useHardwareProfile)('namespace', 'test'); + expect(k8sGetResourceMock).toHaveBeenCalledTimes(1); + expect(renderResult).hookToStrictEqual(standardUseFetchState(null)); + expect(renderResult).hookToHaveUpdateCount(1); + + // wait for update + await renderResult.waitForNextUpdate(); + expect(k8sGetResourceMock).toHaveBeenCalledTimes(1); + expect(renderResult).hookToStrictEqual(standardUseFetchState(null, false, new Error('error1'))); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable([true, true, false, true]); + + // refresh + k8sGetResourceMock.mockRejectedValue(new Error('error2')); + await act(() => renderResult.result.current[3]()); + expect(k8sGetResourceMock).toHaveBeenCalledTimes(2); + expect(renderResult).hookToStrictEqual(standardUseFetchState(null, false, new Error('error2'))); + expect(renderResult).hookToHaveUpdateCount(3); + expect(renderResult).hookToBeStable([true, true, false, true]); + }); +}); diff --git a/frontend/src/pages/hardwareProfiles/__tests__/useHardwareProfiles.spec.ts b/frontend/src/pages/hardwareProfiles/__tests__/useHardwareProfiles.spec.ts new file mode 100644 index 0000000000..087d139202 --- /dev/null +++ b/frontend/src/pages/hardwareProfiles/__tests__/useHardwareProfiles.spec.ts @@ -0,0 +1,68 @@ +import { k8sListResource } from '@openshift/dynamic-plugin-sdk-utils'; +import { act } from 'react'; +import { mockHardwareProfile } from '~/__mocks__/mockHardwareProfile'; +import { mockK8sResourceList } from '~/__mocks__/mockK8sResourceList'; +import { standardUseFetchState, testHook } from '~/__tests__/unit/testUtils/hooks'; +import { HardwareProfileModel } from '~/api'; +import { HardwareProfileKind } from '~/k8sTypes'; +import useHardwareProfiles from '~/pages/hardwareProfiles/useHardwareProfiles'; + +jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({ + k8sListResource: jest.fn(), +})); + +const k8sListResourceMock = jest.mocked(k8sListResource); + +describe('useHardwareProfile', () => { + const hardwareProfilesMock = mockK8sResourceList([mockHardwareProfile({ uid: 'test-1' })]); + it('should return hardware profile', async () => { + k8sListResourceMock.mockResolvedValue(hardwareProfilesMock); + const options = { + model: HardwareProfileModel, + queryOptions: { ns: 'test' }, + }; + const renderResult = testHook(useHardwareProfiles)('test'); + expect(k8sListResourceMock).toHaveBeenCalledTimes(1); + expect(k8sListResourceMock).toHaveBeenCalledWith(options); + expect(renderResult).hookToStrictEqual(standardUseFetchState([])); + expect(renderResult).hookToHaveUpdateCount(1); + + // wait for update + await renderResult.waitForNextUpdate(); + expect(k8sListResourceMock).toHaveBeenCalledTimes(1); + expect(renderResult).hookToStrictEqual(standardUseFetchState(hardwareProfilesMock.items, true)); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable([false, false, true, true]); + + // refresh + k8sListResourceMock.mockResolvedValue(hardwareProfilesMock); + await act(() => renderResult.result.current[3]()); + expect(k8sListResourceMock).toHaveBeenCalledTimes(2); + expect(renderResult).hookToHaveUpdateCount(3); + expect(renderResult).hookToBeStable([false, true, true, true]); + }); + + it('should handle errors and rethrow', async () => { + k8sListResourceMock.mockRejectedValue(new Error('error1')); + + const renderResult = testHook(useHardwareProfiles)('namespace'); + expect(k8sListResourceMock).toHaveBeenCalledTimes(1); + expect(renderResult).hookToStrictEqual(standardUseFetchState([])); + expect(renderResult).hookToHaveUpdateCount(1); + + // wait for update + await renderResult.waitForNextUpdate(); + expect(k8sListResourceMock).toHaveBeenCalledTimes(1); + expect(renderResult).hookToStrictEqual(standardUseFetchState([], false, new Error('error1'))); + expect(renderResult).hookToHaveUpdateCount(2); + expect(renderResult).hookToBeStable([true, true, false, true]); + + // refresh + k8sListResourceMock.mockRejectedValue(new Error('error2')); + await act(() => renderResult.result.current[3]()); + expect(k8sListResourceMock).toHaveBeenCalledTimes(2); + expect(renderResult).hookToStrictEqual(standardUseFetchState([], false, new Error('error2'))); + expect(renderResult).hookToHaveUpdateCount(3); + expect(renderResult).hookToBeStable([true, true, false, true]); + }); +}); diff --git a/frontend/src/pages/hardwareProfiles/useHardwareProfile.ts b/frontend/src/pages/hardwareProfiles/useHardwareProfile.ts new file mode 100644 index 0000000000..182640fef3 --- /dev/null +++ b/frontend/src/pages/hardwareProfiles/useHardwareProfile.ts @@ -0,0 +1,24 @@ +import React from 'react'; +import { getHardwareProfile } from '~/api'; +import { HardwareProfileKind } from '~/k8sTypes'; +import useFetchState, { + FetchState, + FetchStateCallbackPromise, + NotReadyError, +} from '~/utilities/useFetchState'; + +const useHardwareProfile = ( + namespace: string, + name?: string, +): FetchState => { + const callback = React.useCallback>(() => { + if (!name) { + return Promise.reject(new NotReadyError('Hardware profile name is missing')); + } + return getHardwareProfile(name, namespace); + }, [name, namespace]); + + return useFetchState(callback, null); +}; + +export default useHardwareProfile; diff --git a/frontend/src/pages/hardwareProfiles/useHardwareProfiles.ts b/frontend/src/pages/hardwareProfiles/useHardwareProfiles.ts new file mode 100644 index 0000000000..ae554bf1aa --- /dev/null +++ b/frontend/src/pages/hardwareProfiles/useHardwareProfiles.ts @@ -0,0 +1,11 @@ +import React from 'react'; +import useFetchState, { FetchState } from '~/utilities/useFetchState'; +import { HardwareProfileKind } from '~/k8sTypes'; +import { listHardwareProfiles } from '~/api'; + +const useHardwareProfiles = (namespace: string): FetchState => { + const getHardwareProfiles = React.useCallback(() => listHardwareProfiles(namespace), [namespace]); + return useFetchState(getHardwareProfiles, []); +}; + +export default useHardwareProfiles; diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 0b8c01ca01..4289c9988d 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -304,6 +304,19 @@ export type Toleration = { tolerationSeconds?: number; }; +export type Identifier = { + displayName: string; + identifier: string; + minCount: number | string; + maxCount: number | string; + defaultCount: number | string; +}; + +export type NodeSelector = { + key: string; + value: string; +}; + export type PodContainer = { name: string; image: string; diff --git a/frontend/src/utilities/NavData.tsx b/frontend/src/utilities/NavData.tsx index 737068d6de..db3b324874 100644 --- a/frontend/src/utilities/NavData.tsx +++ b/frontend/src/utilities/NavData.tsx @@ -197,11 +197,21 @@ const useAcceleratorProfilesNav = (): NavDataHref[] => }, ]); +const useHardwareProfilesNav = (): NavDataHref[] => + useAreaCheck(SupportedArea.HARDWARE_PROFILES, [ + { + id: 'settings-hardware-profiles', + label: 'Hardware profiles', + href: '/hardwareProfiles', + }, + ]); + const useSettingsNav = (): NavDataGroup[] => { const settingsNavs: NavDataHref[] = [ ...useCustomNotebooksNav(), ...useClusterSettingsNav(), ...useAcceleratorProfilesNav(), + ...useHardwareProfilesNav(), ...useCustomRuntimesNav(), ...useConnectionTypesNav(), ...useStorageClassesNav(), diff --git a/manifests/common/crd/hardwareprofiles.opendatahub.io.crd.yaml b/manifests/common/crd/hardwareprofiles.opendatahub.io.crd.yaml new file mode 100644 index 0000000000..1386e7216e --- /dev/null +++ b/manifests/common/crd/hardwareprofiles.opendatahub.io.crd.yaml @@ -0,0 +1,98 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: hardwareprofiles.dashboard.opendatahub.io +spec: + group: dashboard.opendatahub.io + scope: Namespaced + names: + plural: hardwareprofiles + singular: hardwareprofile + kind: HardwareProfile + versions: + - name: v1alpha1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + required: + - spec + properties: + spec: + type: object + required: + - displayName + - enabled + properties: + displayName: + type: string + description: 'The display name of the hardware profile.' + enabled: + type: boolean + description: 'Indicates whether the hardware profile is available for new resources.' + description: + type: string + description: 'A short description of the hardware profile.' + tolerations: + type: array + description: 'Any number of Kubernetes toleration values that are added to resources when created or updated to this hardware profile.' + items: + type: object + required: + - key + properties: + key: + type: string + description: 'Taint key. Empty matches all keys.' + operator: + type: string + description: "Relationship with the value. Valid: 'Exists', 'Equal'. Defaults to 'Equal'." + value: + type: string + description: "Tolerance value. If key is empty, use 'Exists' to match all values and keys." + effect: + type: string + description: "Taint effect. Empty matches all effects. Allowed: 'NoSchedule', 'PreferNoSchedule', 'NoExecute'." + tolerationSeconds: + type: integer + description: "Duration in seconds. If effect is 'NoExecute', specifies eviction time. Default is forever." + identifiers: + type: array + description: 'The array of identifiers' + items: + type: object + required: + - displayName + - identifier + - minCount + - maxCount + - defaultCount + properties: + displayName: + type: string + description: 'The display name of identifier.' + identifier: + type: string + description: 'The resource identifier of the hardware device.' + minCount: + x-kubernetes-int-or-string: true + description: 'The minimum count can be a integer or a number.' + maxCount: + x-kubernetes-int-or-string: true + description: 'The maximum count can be a integer or a number.' + defaultCount: + x-kubernetes-int-or-string: true + description: 'The default count can be a integer or a number.' + nodeSelectors: + type: array + description: 'Number of node selector available.' + items: + type: object + properties: + key: + type: string + description: 'The key for the node selector.' + value: + type: string + description: 'The value for the node selector.' diff --git a/manifests/common/crd/kustomization.yaml b/manifests/common/crd/kustomization.yaml index ba8313ec34..f6d0f78913 100644 --- a/manifests/common/crd/kustomization.yaml +++ b/manifests/common/crd/kustomization.yaml @@ -6,3 +6,4 @@ resources: - odhdocuments.dashboard.opendatahub.io.crd.yaml - odhapplications.dashboard.opendatahub.io.crd.yaml - acceleratorprofiles.opendatahub.io.crd.yaml +- hardwareprofiles.opendatahub.io.crd.yaml diff --git a/manifests/common/crd/odhdashboardconfigs.opendatahub.io.crd.yaml b/manifests/common/crd/odhdashboardconfigs.opendatahub.io.crd.yaml index 1ea799da53..ed4dd72dda 100644 --- a/manifests/common/crd/odhdashboardconfigs.opendatahub.io.crd.yaml +++ b/manifests/common/crd/odhdashboardconfigs.opendatahub.io.crd.yaml @@ -67,6 +67,8 @@ spec: type: boolean disableAcceleratorProfiles: type: boolean + disableHardwareProfiles: + type: boolean disableDistributedWorkloads: type: boolean disableModelRegistry: diff --git a/manifests/core-bases/base/fetch-hardwares.rbac.yaml b/manifests/core-bases/base/fetch-hardwares.rbac.yaml new file mode 100644 index 0000000000..808ab2e952 --- /dev/null +++ b/manifests/core-bases/base/fetch-hardwares.rbac.yaml @@ -0,0 +1,26 @@ +kind: Role +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: fetch-hardware-profiles-role +rules: + - apiGroups: + - dashboard.opendatahub.io + verbs: + - get + - list + - watch + resources: + - hardwareprofiles +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: hardware-profile-role-binding +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: fetch-hardware-profiles-role +subjects: + - apiGroup: rbac.authorization.k8s.io + kind: Group + name: system:authenticated diff --git a/manifests/core-bases/base/kustomization.yaml b/manifests/core-bases/base/kustomization.yaml index abfe1421c8..259790261b 100644 --- a/manifests/core-bases/base/kustomization.yaml +++ b/manifests/core-bases/base/kustomization.yaml @@ -17,6 +17,7 @@ resources: - model-serving-role.yaml - model-serving-role-binding.yaml - fetch-accelerators.rbac.yaml + - fetch-hardwares.rbac.yaml images: - name: oauth-proxy newName: registry.redhat.io/openshift4/ose-oauth-proxy From 7977023ca632d3223f0785abd554d5bbe8281aae Mon Sep 17 00:00:00 2001 From: Juntao Wang <37624318+DaoDaoNoCode@users.noreply.github.com> Date: Tue, 10 Dec 2024 15:03:09 -0500 Subject: [PATCH 02/10] Fetch correct cluster admin list for the notebook controller admin panel (#3538) * Fetch correct cluster admin list for the notebook controller admin panel * Only show cluster admins with notebooks --- .../routes/api/status/adminAllowedUsers.ts | 17 +++++++-- backend/src/types.ts | 5 +++ backend/src/utils/adminUtils.ts | 36 +++++++++++++++++-- 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/backend/src/routes/api/status/adminAllowedUsers.ts b/backend/src/routes/api/status/adminAllowedUsers.ts index 89663678f7..da1553fcec 100644 --- a/backend/src/routes/api/status/adminAllowedUsers.ts +++ b/backend/src/routes/api/status/adminAllowedUsers.ts @@ -4,6 +4,7 @@ import { getUserInfo } from '../../../utils/userUtils'; import { getAdminUserList, getAllowedUserList, + getClusterAdminUserList, isUserAdmin, KUBE_SAFE_PREFIX, } from '../../../utils/adminUtils'; @@ -77,22 +78,34 @@ export const getAllowedUsers = async ( return []; } + const activityMap = await getUserActivityFromNotebook(fastify, namespace); + + const withNotebookUsers = Object.keys(activityMap); const adminUsers = await getAdminUserList(fastify); const allowedUsers = await getAllowedUserList(fastify); - const activityMap = await getUserActivityFromNotebook(fastify, namespace); + // get cluster admins that have a notebook + const clusterAdminUsers = (await getClusterAdminUserList(fastify)).filter((user) => + withNotebookUsers.includes(user), + ); const usersWithNotebooksMap: AllowedUserMap = convertUserListToMap( - Object.keys(activityMap), + withNotebookUsers, 'User', activityMap, ); const allowedUsersMap: AllowedUserMap = convertUserListToMap(allowedUsers, 'User', activityMap); const adminUsersMap: AllowedUserMap = convertUserListToMap(adminUsers, 'Admin', activityMap); + const clusterAdminUsersMap: AllowedUserMap = convertUserListToMap( + clusterAdminUsers, + 'Admin', + activityMap, + ); const returnUsers: AllowedUserMap = { ...usersWithNotebooksMap, ...allowedUsersMap, ...adminUsersMap, + ...clusterAdminUsersMap, }; return Object.values(returnUsers); }; diff --git a/backend/src/types.ts b/backend/src/types.ts index c29a17d571..4024b82a55 100644 --- a/backend/src/types.ts +++ b/backend/src/types.ts @@ -1255,3 +1255,8 @@ export type NIMAccountKind = K8sResourceCommon & { conditions?: K8sCondition[]; }; }; + +export type ResourceAccessReviewResponse = { + groups?: string[]; + users?: string[]; +}; diff --git a/backend/src/utils/adminUtils.ts b/backend/src/utils/adminUtils.ts index 517ce0ddaf..e42a0c7848 100644 --- a/backend/src/utils/adminUtils.ts +++ b/backend/src/utils/adminUtils.ts @@ -3,9 +3,10 @@ import { V1ClusterRoleBinding, V1ClusterRoleBindingList, } from '@kubernetes/client-node'; -import { KubeFastifyInstance } from '../types'; +import { KubeFastifyInstance, ResourceAccessReviewResponse } from '../types'; import { getAdminGroups, getAllGroupsByUser, getAllowedGroups, getGroup } from './groupsUtils'; import { flatten, uniq } from 'lodash'; +import { getNamespaces } from '../utils/notebookUtils'; const SYSTEM_AUTHENTICATED = 'system:authenticated'; /** Usernames with invalid characters can start with `b64:` to keep their unwanted characters */ @@ -14,10 +15,11 @@ export const KUBE_SAFE_PREFIX = 'b64:'; const getGroupUserList = async ( fastify: KubeFastifyInstance, groupListNames: string[], + additionalUsers: string[] = [], ): Promise => { const customObjectApi = fastify.kube.customObjectsApi; return Promise.all(groupListNames.map((group) => getGroup(customObjectApi, group))).then( - (usersPerGroup: string[][]) => uniq(flatten(usersPerGroup)), + (usersPerGroup: string[][]) => uniq([...flatten(usersPerGroup), ...additionalUsers]), ); }; @@ -26,10 +28,38 @@ export const getAdminUserList = async (fastify: KubeFastifyInstance): Promise groupName && !groupName.startsWith('system:')); // Handle edge-cases and ignore k8s defaults - adminGroupsList.push('cluster-admins'); + return getGroupUserList(fastify, adminGroupsList); }; +export const getClusterAdminUserList = async (fastify: KubeFastifyInstance): Promise => { + // fetch all the users and groups who have cluster-admin role and put them into the admin user list + const { notebookNamespace } = getNamespaces(fastify); + const clusterAdminUsersAndGroups = await fastify.kube.customObjectsApi + // This is not actually fetching all the groups who have admin access to the notebook resources + // But only the cluster admins + // The "*" in the verb field is more like a placeholder + .createClusterCustomObject('authorization.openshift.io', 'v1', 'resourceaccessreviews', { + resource: 'notebooks', + resourceAPIGroup: 'kubeflow.org', + resourceAPIVersion: 'v1', + verb: '*', + namespace: notebookNamespace, + }) + .then((rar) => rar.body as ResourceAccessReviewResponse) + .catch((e) => { + fastify.log.error(`Failure to fetch cluster admin users and groups: ${e.response.body}`); + return { users: [], groups: [] }; + }); + const clusterAdminUsers = clusterAdminUsersAndGroups.users || []; + const clusterAdminGroups = clusterAdminUsersAndGroups.groups || []; + const filteredClusterAdminGroups = clusterAdminGroups.filter( + (group) => !group.startsWith('system:'), + ); + const filteredClusterAdminUsers = clusterAdminUsers.filter((user) => !user.startsWith('system:')); + return getGroupUserList(fastify, filteredClusterAdminGroups, filteredClusterAdminUsers); +}; + export const getAllowedUserList = async (fastify: KubeFastifyInstance): Promise => { const allowedGroups = getAllowedGroups(); const allowedGroupList = allowedGroups From e441ba190e7e949b5f7ceb2d5ab053a26f758b88 Mon Sep 17 00:00:00 2001 From: Dallas <5322142+gitdallas@users.noreply.github.com> Date: Tue, 10 Dec 2024 14:22:39 -0600 Subject: [PATCH 03/10] feat(15128): load and error state for modelArtifact on mr version detail (#3515) Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> --- .../modelRegistry/modelVersionDetails.cy.ts | 11 + .../ModelVersionDetailsView.tsx | 220 ++++++++++-------- 2 files changed, 133 insertions(+), 98 deletions(-) diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.ts index f7abdc4db9..90288a91e9 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.ts @@ -326,6 +326,17 @@ describe('Model version details', () => { }); it('Switching model versions', () => { + cy.interceptOdh( + `GET /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_versions/:modelVersionId/artifacts`, + { + path: { + serviceName: 'modelregistry-sample', + apiVersion: MODEL_REGISTRY_API_VERSION, + modelVersionId: 2, + }, + }, + mockModelArtifactList({}), + ); modelVersionDetails.findVersionId().contains('1'); modelVersionDetails.findModelVersionDropdownButton().click(); modelVersionDetails.findModelVersionDropdownItem('Version 3').should('not.exist'); diff --git a/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx b/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx index d9cd669ae5..a30fe6b566 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx @@ -6,6 +6,9 @@ import { FlexItem, ContentVariants, Title, + Bullseye, + Spinner, + Alert, } from '@patternfly/react-core'; import { ModelVersion } from '~/concepts/modelRegistry/types'; import DashboardDescriptionListGroup from '~/components/DashboardDescriptionListGroup'; @@ -30,12 +33,21 @@ const ModelVersionDetailsView: React.FC = ({ isArchiveVersion, refresh, }) => { - // TODO handle loading state / error for artifacts here? - const [modelArtifacts, , , refreshModelArtifacts] = useModelArtifactsByVersionId(mv.id); + const [modelArtifacts, modelArtifactsLoaded, modelArtifactsLoadError, refreshModelArtifacts] = + useModelArtifactsByVersionId(mv.id); + const modelArtifact = modelArtifacts.items.length ? modelArtifacts.items[0] : null; const { apiState } = React.useContext(ModelRegistryContext); const storageFields = uriToObjectStorageFields(modelArtifact?.uri || ''); + if (!modelArtifactsLoaded) { + return ( + + + + ); + } + return ( = ({ Model location - - {storageFields && ( - <> - - - - - - - - - - - - - - )} - {!storageFields && ( - <> - - - - - )} - - - - Source model format - - - - apiState.api - .patchModelArtifact({}, { modelFormatName: value }, modelArtifact?.id || '') - .then(() => { - refreshModelArtifacts(); - }) - } - title="Model Format" - contentWhenEmpty="No model format specified" - /> - - apiState.api - .patchModelArtifact({}, { modelFormatVersion: newVersion }, modelArtifact?.id || '') - .then(() => { - refreshModelArtifacts(); - }) - } - title="Version" - contentWhenEmpty="No source model format version" - /> - + {modelArtifactsLoadError ? ( + + {modelArtifactsLoadError.message} + + ) : ( + <> + + {storageFields && ( + <> + + + + + + + + + + + + + + )} + {!storageFields && ( + <> + + + + + )} + + + + Source model format + + + + apiState.api + .patchModelArtifact({}, { modelFormatName: value }, modelArtifact?.id || '') + .then(() => { + refreshModelArtifacts(); + }) + } + title="Model Format" + contentWhenEmpty="No model format specified" + /> + + apiState.api + .patchModelArtifact( + {}, + { modelFormatVersion: newVersion }, + modelArtifact?.id || '', + ) + .then(() => { + refreshModelArtifacts(); + }) + } + title="Version" + contentWhenEmpty="No source model format version" + /> + + + )} Date: Wed, 11 Dec 2024 02:06:24 +0530 Subject: [PATCH 04/10] Improve error message for model registration (#3534) --- .../screens/RegisterModel/RegisterModel.tsx | 46 ++++-- .../RegisterModel/RegisterModelErrors.tsx | 114 +++++++++++++++ .../screens/RegisterModel/RegisterVersion.tsx | 45 ++++-- .../RegisterModel/RegistrationFormFooter.tsx | 36 ++--- .../screens/RegisterModel/const.ts | 11 ++ .../useRegistrationCommonState.ts | 41 ------ .../screens/RegisterModel/utils.ts | 135 +++++++++++------- 7 files changed, 288 insertions(+), 140 deletions(-) create mode 100644 frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModelErrors.tsx delete mode 100644 frontend/src/pages/modelRegistry/screens/RegisterModel/useRegistrationCommonState.ts diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx index 124fb3bb22..d79db22e9e 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx @@ -19,31 +19,49 @@ import { Link } from 'react-router-dom'; import FormSection from '~/components/pf-overrides/FormSection'; import ApplicationsPage from '~/pages/ApplicationsPage'; import { modelRegistryUrl, registeredModelUrl } from '~/pages/modelRegistry/screens/routeUtils'; +import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; +import { useAppSelector } from '~/redux/hooks'; import { useRegisterModelData } from './useRegisterModelData'; import { isNameValid, isRegisterModelSubmitDisabled, registerModel } from './utils'; import RegistrationCommonFormSections from './RegistrationCommonFormSections'; -import { useRegistrationCommonState } from './useRegistrationCommonState'; import PrefilledModelRegistryField from './PrefilledModelRegistryField'; import RegistrationFormFooter from './RegistrationFormFooter'; -import { MR_CHARACTER_LIMIT } from './const'; +import { MR_CHARACTER_LIMIT, SubmitLabel } from './const'; const RegisterModel: React.FC = () => { const { modelRegistry: mrName } = useParams(); const navigate = useNavigate(); - - const { isSubmitting, submitError, setSubmitError, handleSubmit, apiState, author } = - useRegistrationCommonState(); - + const { apiState } = React.useContext(ModelRegistryContext); + const author = useAppSelector((state) => state.user || ''); + const [isSubmitting, setIsSubmitting] = React.useState(false); + const [submitError, setSubmitError] = React.useState(undefined); const [formData, setData] = useRegisterModelData(); const isModelNameValid = isNameValid(formData.modelName); const isSubmitDisabled = isSubmitting || isRegisterModelSubmitDisabled(formData); const { modelName, modelDescription } = formData; + const [registeredModelName, setRegisteredModelName] = React.useState(''); + const [versionName, setVersionName] = React.useState(''); + const [errorName, setErrorName] = React.useState(undefined); + + const handleSubmit = async () => { + setIsSubmitting(true); + setSubmitError(undefined); - const onSubmit = () => - handleSubmit(async () => { - const { registeredModel } = await registerModel(apiState, formData, author); + const { + data: { registeredModel, modelVersion, modelArtifact }, + errors, + } = await registerModel(apiState, formData, author); + if (registeredModel && modelVersion && modelArtifact) { navigate(registeredModelUrl(registeredModel.id, mrName)); - }); + } else if (Object.keys(errors).length > 0) { + setIsSubmitting(false); + setRegisteredModelName(formData.modelName); + setVersionName(formData.versionName); + const resourceName = Object.keys(errors)[0]; + setErrorName(resourceName); + setSubmitError(errors[resourceName]); + } + }; const onCancel = () => navigate(modelRegistryUrl(mrName)); return ( @@ -110,13 +128,15 @@ const RegisterModel: React.FC = () => { diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModelErrors.tsx b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModelErrors.tsx new file mode 100644 index 0000000000..d14b8548ff --- /dev/null +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModelErrors.tsx @@ -0,0 +1,114 @@ +import React from 'react'; +import { Alert, AlertActionCloseButton, StackItem } from '@patternfly/react-core'; +import { ErrorName, SubmitLabel } from './const'; + +type RegisterModelErrorProp = { + submitLabel: string; + submitError: Error; + errorName?: string; + versionName?: string; + modelName?: string; +}; + +const RegisterModelErrors: React.FC = ({ + submitLabel, + submitError, + errorName, + versionName = '', + modelName = '', +}) => { + const [showAlert, setShowAlert] = React.useState(true); + + if (submitLabel === SubmitLabel.REGISTER_MODEL && errorName === ErrorName.MODEL_VERSION) { + return ( + <> + {showAlert && ( + + setShowAlert(false)} />} + /> + + )} + + + {submitError.message} + + + + ); + } + + if (submitLabel === SubmitLabel.REGISTER_VERSION && errorName === ErrorName.MODEL_VERSION) { + return ( + + + {submitError.message} + + + ); + } + + if (submitLabel === SubmitLabel.REGISTER_MODEL && errorName === ErrorName.MODEL_ARTIFACT) { + return ( + <> + {showAlert && ( + + setShowAlert(false)} />} + /> + + )} + + + {submitError.message} + + + + ); + } + + if (submitLabel === SubmitLabel.REGISTER_VERSION && errorName === ErrorName.MODEL_ARTIFACT) { + return ( + <> + {showAlert && ( + + setShowAlert(false)} />} + /> + + )} + + + {submitError.message} + + + + ); + } + + return ( + + + {submitError.message} + + + ); +}; +export default RegisterModelErrors; diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx index 4046a5ab25..07c1feac1c 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx @@ -16,25 +16,29 @@ import ApplicationsPage from '~/pages/ApplicationsPage'; import { modelRegistryUrl, registeredModelUrl } from '~/pages/modelRegistry/screens/routeUtils'; import useRegisteredModels from '~/concepts/modelRegistry/apiHooks/useRegisteredModels'; import { filterLiveModels } from '~/concepts/modelRegistry/utils'; +import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; +import { useAppSelector } from '~/redux/hooks'; import { useRegisterVersionData } from './useRegisterModelData'; import { isRegisterVersionSubmitDisabled, registerVersion } from './utils'; import RegistrationCommonFormSections from './RegistrationCommonFormSections'; -import { useRegistrationCommonState } from './useRegistrationCommonState'; import PrefilledModelRegistryField from './PrefilledModelRegistryField'; import RegistrationFormFooter from './RegistrationFormFooter'; import RegisteredModelSelector from './RegisteredModelSelector'; import { usePrefillRegisterVersionFields } from './usePrefillRegisterVersionFields'; +import { SubmitLabel } from './const'; const RegisterVersion: React.FC = () => { const { modelRegistry: mrName, registeredModelId: prefilledRegisteredModelId } = useParams(); - const navigate = useNavigate(); - - const { isSubmitting, submitError, setSubmitError, handleSubmit, apiState, author } = - useRegistrationCommonState(); - + const { apiState } = React.useContext(ModelRegistryContext); + const author = useAppSelector((state) => state.user || ''); + const [isSubmitting, setIsSubmitting] = React.useState(false); const [formData, setData] = useRegisterVersionData(prefilledRegisteredModelId); const isSubmitDisabled = isSubmitting || isRegisterVersionSubmitDisabled(formData); + const [submitError, setSubmitError] = React.useState(undefined); + const [errorName, setErrorName] = React.useState(undefined); + const [versionName, setVersionName] = React.useState(''); + const { registeredModelId } = formData; const [allRegisteredModels, loadedRegisteredModels, loadRegisteredModelsError] = @@ -48,15 +52,29 @@ const RegisterVersion: React.FC = () => { setData, }); - const onSubmit = () => { + const handleSubmit = async () => { if (!registeredModel) { return; // We shouldn't be able to hit this due to form validation } - handleSubmit(async () => { - await registerVersion(apiState, registeredModel, formData, author); + setIsSubmitting(true); + setSubmitError(undefined); + + const { + data: { modelVersion, modelArtifact }, + errors, + } = await registerVersion(apiState, registeredModel, formData, author); + + if (modelVersion && modelArtifact) { navigate(registeredModelUrl(registeredModel.id, mrName)); - }); + } else if (Object.keys(errors).length > 0) { + const resourceName = Object.keys(errors)[0]; + setVersionName(formData.versionName); + setErrorName(resourceName); + setSubmitError(errors[resourceName]); + setIsSubmitting(false); + } }; + const onCancel = () => navigate( prefilledRegisteredModelId && registeredModel @@ -125,13 +143,14 @@ const RegisterVersion: React.FC = () => { diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegistrationFormFooter.tsx b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegistrationFormFooter.tsx index 86ca64789f..2974121aa4 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegistrationFormFooter.tsx +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegistrationFormFooter.tsx @@ -1,46 +1,40 @@ import React from 'react'; -import { - PageSection, - Stack, - StackItem, - Alert, - AlertActionCloseButton, - ActionGroup, - Button, -} from '@patternfly/react-core'; +import { PageSection, Stack, StackItem, ActionGroup, Button } from '@patternfly/react-core'; +import RegisterModelErrors from './RegisterModelErrors'; type RegistrationFormFooterProps = { submitLabel: string; submitError?: Error; - setSubmitError: (e?: Error) => void; isSubmitDisabled: boolean; isSubmitting: boolean; onSubmit: () => void; onCancel: () => void; + errorName?: string; + versionName?: string; + modelName?: string; }; const RegistrationFormFooter: React.FC = ({ submitLabel, submitError, - setSubmitError, isSubmitDisabled, isSubmitting, onSubmit, onCancel, + errorName, + versionName, + modelName, }) => ( {submitError && ( - - setSubmitError(undefined)} />} - > - {submitError.message} - - + )} diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/const.ts b/frontend/src/pages/modelRegistry/screens/RegisterModel/const.ts index 360de6224e..a2ca8196b8 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/const.ts +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/const.ts @@ -1 +1,12 @@ export const MR_CHARACTER_LIMIT = 128; + +export enum SubmitLabel { + REGISTER_MODEL = 'Register model', + REGISTER_VERSION = 'Register new version', +} + +export enum ErrorName { + REGISTERED_MODEL = 'registeredModel', + MODEL_VERSION = 'modelVersion', + MODEL_ARTIFACT = 'modelArtifact', +} diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/useRegistrationCommonState.ts b/frontend/src/pages/modelRegistry/screens/RegisterModel/useRegistrationCommonState.ts deleted file mode 100644 index ae962edd3e..0000000000 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/useRegistrationCommonState.ts +++ /dev/null @@ -1,41 +0,0 @@ -import React from 'react'; -import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; -import { ModelRegistryAPIState } from '~/concepts/modelRegistry/context/useModelRegistryAPIState'; -import { useAppSelector } from '~/redux/hooks'; - -type RegistrationCommonState = { - isSubmitting: boolean; - setIsSubmitting: React.Dispatch>; - submitError: Error | undefined; - setSubmitError: React.Dispatch>; - handleSubmit: (doSubmit: () => Promise) => void; - apiState: ModelRegistryAPIState; - author: string; -}; - -export const useRegistrationCommonState = (): RegistrationCommonState => { - const [isSubmitting, setIsSubmitting] = React.useState(false); - const [submitError, setSubmitError] = React.useState(undefined); - - const { apiState } = React.useContext(ModelRegistryContext); - const author = useAppSelector((state) => state.user || ''); - - const handleSubmit = (doSubmit: () => Promise) => { - setIsSubmitting(true); - setSubmitError(undefined); - doSubmit().catch((e: Error) => { - setIsSubmitting(false); - setSubmitError(e); - }); - }; - - return { - isSubmitting, - setIsSubmitting, - submitError, - setSubmitError, - handleSubmit, - apiState, - author, - }; -}; diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts b/frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts index 8e37b74fb7..2c1686cfe7 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts @@ -13,39 +13,53 @@ import { RegisterVersionFormData, RegistrationCommonFormData, } from './useRegisterModelData'; -import { MR_CHARACTER_LIMIT } from './const'; +import { ErrorName, MR_CHARACTER_LIMIT } from './const'; export type RegisterModelCreatedResources = RegisterVersionCreatedResources & { - registeredModel: RegisteredModel; + registeredModel?: RegisteredModel; }; export type RegisterVersionCreatedResources = { - modelVersion: ModelVersion; - modelArtifact: ModelArtifact; + modelVersion?: ModelVersion; + modelArtifact?: ModelArtifact; }; export const registerModel = async ( apiState: ModelRegistryAPIState, formData: RegisterModelFormData, author: string, -): Promise => { - const registeredModel = await apiState.api.createRegisteredModel( - {}, - { - name: formData.modelName, - description: formData.modelDescription, - customProperties: {}, - owner: author, - state: ModelState.LIVE, - }, - ); - const { modelVersion, modelArtifact } = await registerVersion( - apiState, - registeredModel, - formData, - author, - ); - return { registeredModel, modelVersion, modelArtifact }; +): Promise<{ + data: RegisterModelCreatedResources; + errors: { [key: string]: Error | undefined }; +}> => { + let registeredModel; + const error: { [key: string]: Error | undefined } = {}; + try { + registeredModel = await apiState.api.createRegisteredModel( + {}, + { + name: formData.modelName, + description: formData.modelDescription, + customProperties: {}, + owner: author, + state: ModelState.LIVE, + }, + ); + } catch (e) { + if (e instanceof Error) { + error[ErrorName.REGISTERED_MODEL] = e; + } + return { data: { registeredModel }, errors: error }; + } + const { + data: { modelVersion, modelArtifact }, + errors, + } = await registerVersion(apiState, registeredModel, formData, author); + + return { + data: { registeredModel, modelVersion, modelArtifact }, + errors, + }; }; export const registerVersion = async ( @@ -53,42 +67,59 @@ export const registerVersion = async ( registeredModel: RegisteredModel, formData: Omit, author: string, -): Promise => { - const modelVersion = await apiState.api.createModelVersionForRegisteredModel( - {}, - registeredModel.id, - { +): Promise<{ + data: RegisterVersionCreatedResources; + errors: { [key: string]: Error | undefined }; +}> => { + let modelVersion; + let modelArtifact; + const errors: { [key: string]: Error | undefined } = {}; + try { + modelVersion = await apiState.api.createModelVersionForRegisteredModel({}, registeredModel.id, { name: formData.versionName, description: formData.versionDescription, customProperties: {}, state: ModelState.LIVE, author, registeredModelId: registeredModel.id, - }, - ); - const modelArtifact = await apiState.api.createModelArtifactForModelVersion({}, modelVersion.id, { - name: `${formData.versionName}`, - description: formData.versionDescription, - customProperties: {}, - state: ModelArtifactState.LIVE, - author, - modelFormatName: formData.sourceModelFormat, - modelFormatVersion: formData.sourceModelFormatVersion, - // TODO fill in the name of the data connection we used to prefill if we used one - // TODO this should be done as part of https://issues.redhat.com/browse/RHOAIENG-9914 - // storageKey: 'TODO', - uri: - formData.modelLocationType === ModelLocationType.ObjectStorage - ? objectStorageFieldsToUri({ - endpoint: formData.modelLocationEndpoint, - bucket: formData.modelLocationBucket, - region: formData.modelLocationRegion, - path: formData.modelLocationPath, - }) || '' // We'll only hit this case if required fields are empty strings, so form validation should catch it. - : formData.modelLocationURI, - artifactType: 'model-artifact', - }); - return { modelVersion, modelArtifact }; + }); + } catch (e) { + if (e instanceof Error) { + errors[ErrorName.MODEL_VERSION] = e; + } + return { data: { modelVersion, modelArtifact }, errors }; + } + + try { + modelArtifact = await apiState.api.createModelArtifactForModelVersion({}, modelVersion.id, { + name: `${formData.versionName}`, + description: formData.versionDescription, + customProperties: {}, + state: ModelArtifactState.LIVE, + author, + modelFormatName: formData.sourceModelFormat, + modelFormatVersion: formData.sourceModelFormatVersion, + // TODO fill in the name of the data connection we used to prefill if we used one + // TODO this should be done as part of https://issues.redhat.com/browse/RHOAIENG-9914 + // storageKey: 'TODO', + uri: + formData.modelLocationType === ModelLocationType.ObjectStorage + ? objectStorageFieldsToUri({ + endpoint: formData.modelLocationEndpoint, + bucket: formData.modelLocationBucket, + region: formData.modelLocationRegion, + path: formData.modelLocationPath, + }) || '' // We'll only hit this case if required fields are empty strings, so form validation should catch it. + : formData.modelLocationURI, + artifactType: 'model-artifact', + }); + } catch (e) { + if (e instanceof Error) { + errors[ErrorName.MODEL_ARTIFACT] = e; + } + } + + return { data: { modelVersion, modelArtifact }, errors }; }; const isSubmitDisabledForCommonFields = (formData: RegistrationCommonFormData): boolean => { From a1accaf1383c2ca728e419a704031a469add7228 Mon Sep 17 00:00:00 2001 From: Gage Krumbach Date: Tue, 10 Dec 2024 15:47:55 -0600 Subject: [PATCH 05/10] Increase timeout for disabling recurring run in Cypress test for pipeline runs (#3564) --- .../cypress/cypress/tests/mocked/pipelines/pipelineRuns.cy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/pipelines/pipelineRuns.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/pipelines/pipelineRuns.cy.ts index 3bd524dd26..61b06bfbe7 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/pipelines/pipelineRuns.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/pipelines/pipelineRuns.cy.ts @@ -951,7 +951,7 @@ describe('Pipeline runs', () => { .getRowByName(mockRecurringRuns[0].display_name) .findStatusSwitchByRowName() .click(); - cy.wait('@disableRecurringRun'); + cy.wait('@disableRecurringRun', { timeout: 10000 }); }); it('schedules toggle should be disabled for the schedules with archived experiment', () => { From 5bdc16057a140b0902cf815a2a2ddb798367b1d0 Mon Sep 17 00:00:00 2001 From: Purva Naik Date: Wed, 11 Dec 2024 21:07:45 +0530 Subject: [PATCH 06/10] Update cluster storage modal (#3529) --- .../src/__mocks__/mockNotebookK8sResource.ts | 11 +- .../cypress/cypress/pages/clusterStorage.ts | 34 +- .../mocked/projects/clusterStorage.cy.ts | 66 ++-- .../src/api/k8s/__tests__/notebooks.spec.ts | 4 +- frontend/src/api/k8s/notebooks.ts | 54 ++- frontend/src/components/table/TableBase.tsx | 1 + frontend/src/components/table/types.ts | 1 + .../notebook/StorageNotebookConnections.tsx | 75 ---- frontend/src/pages/projects/notebook/utils.ts | 18 - .../detail/storage/ClusterStorageModal.tsx | 372 ++++++++++++++---- .../detail/storage/ClusterStorageTableRow.tsx | 201 ++++++++++ .../storage/ExistingConnectedNotebooks.tsx | 61 --- .../detail/storage/StorageTableRow.tsx | 2 +- .../detail/storage/clusterTableColumns.ts | 33 ++ .../projects/screens/detail/storage/data.ts | 2 +- .../projects/screens/detail/storage/utils.ts | 86 +++- .../spawner/storage/ClusterStorageTable.tsx | 7 +- .../spawner/storage/__tests__/utils.spec.ts | 6 +- .../screens/spawner/storage/constants.ts | 2 +- .../projects/screens/spawner/storage/types.ts | 4 +- .../projects/screens/spawner/storage/utils.ts | 6 +- frontend/src/pages/projects/types.ts | 6 + 22 files changed, 743 insertions(+), 309 deletions(-) delete mode 100644 frontend/src/pages/projects/notebook/StorageNotebookConnections.tsx create mode 100644 frontend/src/pages/projects/screens/detail/storage/ClusterStorageTableRow.tsx delete mode 100644 frontend/src/pages/projects/screens/detail/storage/ExistingConnectedNotebooks.tsx create mode 100644 frontend/src/pages/projects/screens/detail/storage/clusterTableColumns.ts diff --git a/frontend/src/__mocks__/mockNotebookK8sResource.ts b/frontend/src/__mocks__/mockNotebookK8sResource.ts index 2ed14cb477..de2daf801d 100644 --- a/frontend/src/__mocks__/mockNotebookK8sResource.ts +++ b/frontend/src/__mocks__/mockNotebookK8sResource.ts @@ -1,7 +1,13 @@ import * as _ from 'lodash-es'; import { KnownLabels, NotebookKind } from '~/k8sTypes'; import { DEFAULT_NOTEBOOK_SIZES } from '~/pages/projects/screens/spawner/const'; -import { ContainerResources, TolerationEffect, TolerationOperator, VolumeMount } from '~/types'; +import { + ContainerResources, + TolerationEffect, + TolerationOperator, + Volume, + VolumeMount, +} from '~/types'; import { genUID } from '~/__mocks__/mockUtils'; import { RecursivePartial } from '~/typeHelpers'; import { EnvironmentFromVariable } from '~/pages/projects/types'; @@ -19,6 +25,7 @@ type MockResourceConfigType = { opts?: RecursivePartial; uid?: string; additionalVolumeMounts?: VolumeMount[]; + additionalVolumes?: Volume[]; }; export const mockNotebookK8sResource = ({ @@ -41,6 +48,7 @@ export const mockNotebookK8sResource = ({ opts = {}, uid = genUID('notebook'), additionalVolumeMounts = [], + additionalVolumes = [], }: MockResourceConfigType): NotebookKind => _.merge( { @@ -257,6 +265,7 @@ export const mockNotebookK8sResource = ({ secretName: 'workbench-tls', }, }, + ...additionalVolumes, ], }, }, diff --git a/frontend/src/__tests__/cypress/cypress/pages/clusterStorage.ts b/frontend/src/__tests__/cypress/cypress/pages/clusterStorage.ts index 8fe47da8d8..56dfad7271 100644 --- a/frontend/src/__tests__/cypress/cypress/pages/clusterStorage.ts +++ b/frontend/src/__tests__/cypress/cypress/pages/clusterStorage.ts @@ -8,7 +8,7 @@ class ClusterStorageRow extends TableRow { } findConnectedWorkbenches() { - return this.find().find('[data-label="Connected workbenches"]'); + return this.find().find('[data-label="Workbench connections"]'); } toggleExpandableContent() { @@ -55,14 +55,29 @@ class ClusterStorageModal extends Modal { super(edit ? 'Update cluster storage' : 'Add cluster storage'); } - findWorkbenchConnectionSelect() { - return this.find() - .findByTestId('connect-existing-workbench-group') - .findByRole('button', { name: 'Typeahead menu toggle', hidden: true }); + findAddWorkbenchButton() { + return cy.findByTestId('add-workbench-button'); } - findMountField() { - return this.find().findByTestId('mount-path-folder-value'); + findWorkbenchTable() { + return cy.findByTestId('workbench-connection-table'); + } + + selectWorkbenchName(row: number, name: string) { + this.findWorkbenchTable().find(`[data-label=Name]`).eq(row).find('button').click(); + cy.findByRole('option', { name, hidden: true }).click(); + } + + selectCustomPathFormat(row: number) { + this.findWorkbenchTable().find(`[data-label="Path format"]`).eq(row).find('button').click(); + cy.findByRole('option', { + name: 'Custom Custom paths that do not begin with /opt/app-root/src/ are not visible in the JupyterLab file browser.', + hidden: true, + }).click(); + } + + findMountPathField(row: number) { + return this.findWorkbenchTable().find(`[data-label="Mount path"]`).eq(row).find('input'); } findMountFieldHelperText() { @@ -127,6 +142,11 @@ class ClusterStorageModal extends Modal { return this.find().findByTestId('storage-classes-selector'); } + selectStorageClassSelectOption(name: string | RegExp) { + this.findStorageClassSelect().click(); + cy.findByRole('option', { name, hidden: true }).click(); + } + findStorageClassOption(name: string) { return cy.get('#storage-classes-selector').findByText(name); } diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/clusterStorage.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/clusterStorage.cy.ts index a905e5162e..a572757d9c 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/clusterStorage.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/clusterStorage.cy.ts @@ -69,6 +69,14 @@ const initInterceptors = ({ isEmpty = false, storageClassName }: HandlersProps) name: 'test-dupe-pvc-path', }, ], + additionalVolumes: [ + { + name: 'test-dupe-pvc-path', + persistentVolumeClaim: { + claimName: 'test-dupe-pvc-path', + }, + }, + ], }), ]), ); @@ -163,10 +171,7 @@ describe('ClusterStorage', () => { addClusterStorageModal.find().findByText('openshift-default-sc').should('exist'); // select storage class - addClusterStorageModal - .findStorageClassSelect() - .findSelectOption(/Test SC 1/) - .click(); + addClusterStorageModal.selectStorageClassSelectOption(/Test SC 1/); addClusterStorageModal.findSubmitButton().should('be.enabled'); addClusterStorageModal.findDescriptionInput().fill('description'); addClusterStorageModal.findPVSizeMinusButton().click(); @@ -176,40 +181,22 @@ describe('ClusterStorage', () => { addClusterStorageModal.selectPVSize('MiB'); //connect workbench - addClusterStorageModal - .findWorkbenchConnectionSelect() - .findSelectOption('Test Notebook') - .click(); - - // don't allow duplicate path - addClusterStorageModal.findMountField().clear(); - addClusterStorageModal.findMountField().fill('test-dupe'); - addClusterStorageModal - .findMountFieldHelperText() - .should('contain.text', 'Mount folder is already in use for this workbench.'); - - // don't allow number in the path - addClusterStorageModal.findMountField().clear(); - addClusterStorageModal.findMountField().fill('test2'); - addClusterStorageModal - .findMountFieldHelperText() - .should('contain.text', 'Must only consist of lowercase letters and dashes.'); + addClusterStorageModal.findAddWorkbenchButton().click(); + addClusterStorageModal.findWorkbenchTable().should('exist'); + addClusterStorageModal.selectWorkbenchName(0, 'test-notebook'); - // Allow trailing slash - addClusterStorageModal.findMountField().clear(); - addClusterStorageModal.findMountField().fill('test/'); + //don't allow duplicate path + addClusterStorageModal.findMountPathField(0).fill('test-dupe'); addClusterStorageModal .findMountFieldHelperText() - .should('contain.text', 'Must consist of lowercase letters and dashes.'); + .should('contain.text', 'This path is already connected to this workbench'); - addClusterStorageModal.findMountField().clear(); + addClusterStorageModal.findMountPathField(0).clear(); + addClusterStorageModal.selectCustomPathFormat(0); addClusterStorageModal .findMountFieldHelperText() - .should( - 'contain.text', - 'Enter a path to a model or folder. This path cannot point to a root folder.', - ); - addClusterStorageModal.findMountField().fill('data'); + .should('contain.text', 'Enter a path to a model or folder.'); + addClusterStorageModal.findMountPathField(0).fill('data'); addClusterStorageModal.findWorkbenchRestartAlert().should('exist'); cy.interceptK8s('PATCH', NotebookModel, mockNotebookK8sResource({})).as('addClusterStorage'); @@ -224,7 +211,7 @@ describe('ClusterStorage', () => { { op: 'add', path: '/spec/template/spec/containers/0/volumeMounts/-', - value: { mountPath: '/opt/app-root/src/data' }, + value: { mountPath: '/data' }, }, ]); }); @@ -252,7 +239,7 @@ describe('ClusterStorage', () => { ], containers: [ { - volumeMounts: [{ name: 'existing-pvc', mountPath: '/' }], + volumeMounts: [{ name: 'existing-pvc', mountPath: '/opt/app-root/src' }], }, ], }, @@ -280,14 +267,9 @@ describe('ClusterStorage', () => { clusterStorage.visit('test-project'); const clusterStorageRow = clusterStorage.getClusterStorageRow('Existing PVC'); clusterStorageRow.findKebabAction('Edit storage').click(); - - // Connect to 'Another Notebook' - updateClusterStorageModal - .findWorkbenchConnectionSelect() - .findSelectOption('Another Notebook') - .click(); - - updateClusterStorageModal.findMountField().fill('new-data'); + updateClusterStorageModal.findAddWorkbenchButton().click(); + updateClusterStorageModal.selectWorkbenchName(1, 'another-notebook'); + updateClusterStorageModal.findMountPathField(1).fill('new-data'); cy.interceptK8s('PATCH', NotebookModel, anotherNotebook).as('updateClusterStorage'); diff --git a/frontend/src/api/k8s/__tests__/notebooks.spec.ts b/frontend/src/api/k8s/__tests__/notebooks.spec.ts index cb51f1a3c6..6441cd6972 100644 --- a/frontend/src/api/k8s/__tests__/notebooks.spec.ts +++ b/frontend/src/api/k8s/__tests__/notebooks.spec.ts @@ -696,7 +696,7 @@ describe('attachNotebookPVC', () => { const namespace = 'test-project'; const notebookName = 'test-notebook'; const pvcName = 'test-pvc'; - const mountSuffix = 'data'; + const mountSuffix = '/opt/app-root/src/data'; const mockNotebook = mockNotebookK8sResource({ uid }); k8sPatchResourceMock.mockResolvedValue(mockNotebook); @@ -728,7 +728,7 @@ describe('attachNotebookPVC', () => { const namespace = 'test-project'; const notebookName = 'test-notebook'; const pvcName = 'test-pvc'; - const mountSuffix = 'data'; + const mountSuffix = '/opt/app-root/src/data'; k8sPatchResourceMock.mockRejectedValue(new Error('error1')); await expect(attachNotebookPVC(notebookName, namespace, pvcName, mountSuffix)).rejects.toThrow( diff --git a/frontend/src/api/k8s/notebooks.ts b/frontend/src/api/k8s/notebooks.ts index 0e5e5a608c..45de854d90 100644 --- a/frontend/src/api/k8s/notebooks.ts +++ b/frontend/src/api/k8s/notebooks.ts @@ -409,7 +409,7 @@ export const attachNotebookPVC = ( notebookName: string, namespace: string, pvcName: string, - mountSuffix: string, + mountPath: string, opts?: K8sAPIOptions, ): Promise => { const patches: Patch[] = [ @@ -422,7 +422,7 @@ export const attachNotebookPVC = ( op: 'add', // TODO: can we assume first container? path: '/spec/template/spec/containers/0/volumeMounts/-', - value: { mountPath: `${ROOT_MOUNT_PATH}/${mountSuffix}`, name: pvcName }, + value: { mountPath, name: pvcName }, }, ]; @@ -438,6 +438,56 @@ export const attachNotebookPVC = ( ); }; +export const updateNotebookPVC = ( + notebookName: string, + namespace: string, + mountPath: string, + pvcName: string, + opts?: K8sAPIOptions, +): Promise => + new Promise((resolve, reject) => { + getNotebook(notebookName, namespace) + .then((notebook) => { + const volumes = notebook.spec.template.spec.volumes || []; + const volumeMounts = notebook.spec.template.spec.containers[0].volumeMounts || []; + + const filteredVolumeMounts = volumeMounts.map((volumeMount) => { + if (volumeMount.name === pvcName) { + return { ...volumeMount, mountPath }; + } + return volumeMount; + }); + + const patches: Patch[] = [ + { + op: 'replace', + path: '/spec/template/spec/volumes', + value: volumes, + }, + { + op: 'replace', + // TODO: can we assume first container? + path: '/spec/template/spec/containers/0/volumeMounts', + value: filteredVolumeMounts, + }, + ]; + + k8sPatchResource( + applyK8sAPIOptions( + { + model: NotebookModel, + queryOptions: { name: notebookName, ns: namespace }, + patches, + }, + opts, + ), + ) + .then(resolve) + .catch(reject); + }) + .catch(reject); + }); + export const removeNotebookPVC = ( notebookName: string, namespace: string, diff --git a/frontend/src/components/table/TableBase.tsx b/frontend/src/components/table/TableBase.tsx index aa42a2ab4e..cb311268ed 100644 --- a/frontend/src/components/table/TableBase.tsx +++ b/frontend/src/components/table/TableBase.tsx @@ -200,6 +200,7 @@ const TableBase = ({ stickyLeftOffset={col.stickyLeftOffset} hasRightBorder={col.hasRightBorder} modifier={col.modifier} + visibility={col.visibility} className={col.className} > {col.label} diff --git a/frontend/src/components/table/types.ts b/frontend/src/components/table/types.ts index 5d12ed0e83..a223b8ee03 100644 --- a/frontend/src/components/table/types.ts +++ b/frontend/src/components/table/types.ts @@ -11,6 +11,7 @@ export type SortableData = Pick< | 'modifier' | 'width' | 'info' + | 'visibility' | 'className' > & { label: string; diff --git a/frontend/src/pages/projects/notebook/StorageNotebookConnections.tsx b/frontend/src/pages/projects/notebook/StorageNotebookConnections.tsx deleted file mode 100644 index 88b3d75cb3..0000000000 --- a/frontend/src/pages/projects/notebook/StorageNotebookConnections.tsx +++ /dev/null @@ -1,75 +0,0 @@ -import * as React from 'react'; -import { Alert } from '@patternfly/react-core'; -import { ForNotebookSelection } from '~/pages/projects/types'; -import MountPathField from '~/pages/projects/pvc/MountPathField'; -import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext'; -import { NotebookKind } from '~/k8sTypes'; -import { getNotebookMountPaths } from './utils'; -import ConnectedNotebookField from './ConnectedNotebookField'; - -type StorageNotebookConnectionsProps = { - forNotebookData: ForNotebookSelection; - setForNotebookData: (value: ForNotebookSelection) => void; - connectedNotebooks: NotebookKind[]; -}; - -const StorageNotebookConnections: React.FC = ({ - forNotebookData, - setForNotebookData, - connectedNotebooks, -}) => { - const { - notebooks: { data, loaded, error }, - } = React.useContext(ProjectDetailsContext); - const notebooks = data.map(({ notebook }) => notebook); - - if (error) { - return ( - - {error.message} - - ); - } - - const inUseMountPaths = getNotebookMountPaths( - notebooks.find((notebook) => notebook.metadata.name === forNotebookData.name), - ); - - const connectedNotebookNames = connectedNotebooks.map((notebook) => notebook.metadata.name); - const availableNotebooks = notebooks.filter( - (notebook) => !connectedNotebookNames.includes(notebook.metadata.name), - ); - - return ( - <> - { - const selection = selectionItems[0]; - if (selection) { - setForNotebookData({ - name: selection, - mountPath: { value: '', error: '' }, - }); - } else { - setForNotebookData({ name: '', mountPath: { value: '', error: '' } }); - } - }} - /> - {forNotebookData.name && ( - { - setForNotebookData({ ...forNotebookData, mountPath }); - }} - /> - )} - - ); -}; - -export default StorageNotebookConnections; diff --git a/frontend/src/pages/projects/notebook/utils.ts b/frontend/src/pages/projects/notebook/utils.ts index 022b6fb45a..49a66265dc 100644 --- a/frontend/src/pages/projects/notebook/utils.ts +++ b/frontend/src/pages/projects/notebook/utils.ts @@ -1,6 +1,5 @@ import { EventKind, NotebookKind } from '~/k8sTypes'; import { EventStatus, NotebookSize, NotebookStatus } from '~/types'; -import { ROOT_MOUNT_PATH } from '~/pages/projects/pvc/const'; import { fireFormTrackingEvent } from '~/concepts/analyticsTracking/segmentIOUtils'; import { TrackingOutcome } from '~/concepts/analyticsTracking/trackingProperties'; import { AcceleratorProfileState } from '~/utilities/useReadAcceleratorState'; @@ -24,9 +23,6 @@ export const getNotebookPVCVolumeNames = (notebook: NotebookKind): { [name: stri }; }, {}); -const relativeMountPath = (mountPath: string): string => - mountPath.slice(ROOT_MOUNT_PATH.length) || '/'; - export const getNotebookPVCMountPathMap = ( notebook?: NotebookKind, ): { [claimName: string]: string } => { @@ -52,20 +48,6 @@ export const getNotebookPVCMountPathMap = ( ); }; -export const getNotebookMountPaths = (notebook?: NotebookKind): string[] => { - if (!notebook) { - return []; - } - - return notebook.spec.template.spec.containers - .map( - (container) => - container.volumeMounts?.map((volumeMount) => relativeMountPath(volumeMount.mountPath)) || - [], - ) - .flat(); -}; - export const getEventTimestamp = (event: EventKind): string => event.lastTimestamp || event.eventTime; diff --git a/frontend/src/pages/projects/screens/detail/storage/ClusterStorageModal.tsx b/frontend/src/pages/projects/screens/detail/storage/ClusterStorageModal.tsx index 59b67f3c19..9c705aa5ed 100644 --- a/frontend/src/pages/projects/screens/detail/storage/ClusterStorageModal.tsx +++ b/frontend/src/pages/projects/screens/detail/storage/ClusterStorageModal.tsx @@ -1,20 +1,39 @@ import * as React from 'react'; -import { FormGroup } from '@patternfly/react-core'; +import { Button, Stack, FormGroup, StackItem } from '@patternfly/react-core'; +import { PlusCircleIcon } from '@patternfly/react-icons'; +import isEqual from 'lodash-es/isEqual'; import { NotebookKind, PersistentVolumeClaimKind } from '~/k8sTypes'; -import { ForNotebookSelection, StorageData } from '~/pages/projects/types'; +import { ClusterStorageNotebookSelection, StorageData } from '~/pages/projects/types'; import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext'; -import { attachNotebookPVC, createPvc, removeNotebookPVC, restartNotebook, updatePvc } from '~/api'; +import { + attachNotebookPVC, + createPvc, + removeNotebookPVC, + restartNotebook, + updateNotebookPVC, + updatePvc, +} from '~/api'; import { ConnectedNotebookContext, useRelatedNotebooks, } from '~/pages/projects/notebook/useRelatedNotebooks'; -import NotebookRestartAlert from '~/pages/projects/components/NotebookRestartAlert'; -import StorageNotebookConnections from '~/pages/projects/notebook/StorageNotebookConnections'; import useWillNotebooksRestart from '~/pages/projects/notebook/useWillNotebooksRestart'; import { useIsAreaAvailable, SupportedArea } from '~/concepts/areas'; +import { Table } from '~/components/table'; +import { getNotebookPVCMountPathMap } from '~/pages/projects/notebook/utils'; +import { MOUNT_PATH_PREFIX } from '~/pages/projects/screens/spawner/storage/const'; +import NotebookRestartAlert from '~/pages/projects/components/NotebookRestartAlert'; +import { MountPathFormat } from '~/pages/projects/screens/spawner/storage/types'; +import { useCreateStorageObject } from '~/pages/projects/screens/spawner/storage/utils'; import BaseStorageModal from './BaseStorageModal'; -import ExistingConnectedNotebooks from './ExistingConnectedNotebooks'; -import { isPvcUpdateRequired } from './utils'; +import { + handleConnectedNotebooks, + isPvcUpdateRequired, + getInUseMountPaths, + validateClusterMountPath, +} from './utils'; +import { storageColumns } from './clusterTableColumns'; +import ClusterStorageTableRow from './ClusterStorageTableRow'; type ClusterStorageModalProps = { existingPvc?: PersistentVolumeClaimKind; @@ -22,47 +41,95 @@ type ClusterStorageModalProps = { }; const ClusterStorageModal: React.FC = ({ existingPvc, onClose }) => { - const { currentProject } = React.useContext(ProjectDetailsContext); + const { + currentProject, + notebooks: { data, loaded }, + } = React.useContext(ProjectDetailsContext); const namespace = currentProject.metadata.name; - const [removedNotebooks, setRemovedNotebooks] = React.useState([]); - const [notebookData, setNotebookData] = React.useState({ - name: '', - mountPath: { value: '', error: '' }, - }); + const allNotebooks = React.useMemo(() => data.map((currentData) => currentData.notebook), [data]); const { notebooks: connectedNotebooks } = useRelatedNotebooks( - ConnectedNotebookContext.EXISTING_PVC, + ConnectedNotebookContext.REMOVABLE_PVC, existingPvc?.metadata.name, ); const workbenchEnabled = useIsAreaAvailable(SupportedArea.WORKBENCHES).status; + const [{ name }] = useCreateStorageObject(existingPvc); + const [storageName, setStorageName] = React.useState(name); + const hasExistingNotebookConnections = connectedNotebooks.length > 0; + const [notebookData, setNotebookData] = React.useState([]); + const [newRowId, setNewRowId] = React.useState(1); - const { - notebooks: removableNotebooks, - loaded: removableNotebookLoaded, - error: removableNotebookError, - } = useRelatedNotebooks(ConnectedNotebookContext.REMOVABLE_PVC, existingPvc?.metadata.name); + React.useEffect(() => { + if (hasExistingNotebookConnections) { + const addData = connectedNotebooks.map((connectedNotebook) => ({ + name: connectedNotebook.metadata.name, + mountPath: { + value: existingPvc + ? getNotebookPVCMountPathMap(connectedNotebook)[existingPvc.metadata.name] + : '', + error: '', + }, + existingPvc: true, + isUpdatedValue: false, + })); + setNotebookData(addData); + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [existingPvc, hasExistingNotebookConnections]); - const hasExistingNotebookConnections = removableNotebooks.length > 0; + const availableNotebooks = React.useMemo( + () => + allNotebooks.filter( + (currentNotebookData) => + !notebookData.some( + (currentNotebook) => currentNotebook.name === currentNotebookData.metadata.name, + ), + ), + [allNotebooks, notebookData], + ); - const restartNotebooks = useWillNotebooksRestart([...removedNotebooks, notebookData.name]); + const addEmptyRow = React.useCallback(() => { + setNotebookData(() => [ + ...notebookData, + { + name: '', + mountPath: { + value: MOUNT_PATH_PREFIX, + error: '', + }, + existingPvc: false, + isUpdatedValue: false, + newRowId, + }, + ]); + setNewRowId((prev) => prev + 1); + }, [newRowId, notebookData]); - const handleSubmit = async ( - storageData: StorageData, - attachNotebookData?: ForNotebookSelection, - dryRun?: boolean, - ) => { - const promises: Promise[] = []; + const { updatedNotebooks, removedNotebooks } = handleConnectedNotebooks( + notebookData, + connectedNotebooks, + ); + + const newNotebooks = notebookData.filter((notebook) => !notebook.existingPvc); + + const restartNotebooks = useWillNotebooksRestart([ + ...updatedNotebooks.map((updatedNotebook) => updatedNotebook.name), + ...removedNotebooks, + ...newNotebooks.map((newNotebook) => newNotebook.name), + ]); + const handleSubmit = async (submitData: StorageData, dryRun?: boolean) => { + const promises: Promise[] = []; if (existingPvc) { const pvcName = existingPvc.metadata.name; // Check if PVC needs to be updated (name, description, size, storageClass) - if (isPvcUpdateRequired(existingPvc, storageData)) { - promises.push(updatePvc(storageData, existingPvc, namespace, { dryRun })); + if (isPvcUpdateRequired(existingPvc, submitData)) { + promises.push(updatePvc(submitData, existingPvc, namespace, { dryRun })); } // Restart notebooks if the PVC size has changed - if (existingPvc.spec.resources.requests.storage !== storageData.size) { + if (existingPvc.spec.resources.requests.storage !== submitData.size) { restartNotebooks.map((connectedNotebook) => promises.push( restartNotebook(connectedNotebook.notebook.metadata.name, namespace, { dryRun }), @@ -70,92 +137,223 @@ const ClusterStorageModal: React.FC = ({ existingPvc, ); } - // Remove connections to notebooks that were removed + if (updatedNotebooks.length > 0) { + promises.push( + ...updatedNotebooks.map((nM) => + updateNotebookPVC(nM.name, namespace, nM.mountPath.value, pvcName, { dryRun }), + ), + ); + } + if (removedNotebooks.length > 0) { promises.push( ...removedNotebooks.map((nM) => removeNotebookPVC(nM, namespace, pvcName, { dryRun })), ); } - - await Promise.all(promises); - - if (attachNotebookData?.name) { - await attachNotebookPVC( - attachNotebookData.name, - namespace, - pvcName, - attachNotebookData.mountPath.value, - { - dryRun, - }, + if (newNotebooks.length > 0) { + promises.push( + ...newNotebooks.map((nM) => + attachNotebookPVC(nM.name, namespace, pvcName, nM.mountPath.value, { dryRun }), + ), ); } + await Promise.all(promises); return; } - // Create new PVC if it doesn't exist - const createdPvc = await createPvc(storageData, namespace, { dryRun }); - - // Attach the new PVC to a notebook if specified - if (attachNotebookData?.name) { - await attachNotebookPVC( - attachNotebookData.name, - namespace, - createdPvc.metadata.name, - attachNotebookData.mountPath.value, - { dryRun }, + + const createdPvc = await createPvc(submitData, namespace, { dryRun }); + + const newCreatedPVCPromises: Promise[] = []; + + if (newNotebooks.length > 0) { + newCreatedPVCPromises.push( + ...newNotebooks.map((nM) => + attachNotebookPVC(nM.name, namespace, createdPvc.metadata.name, nM.mountPath.value, { + dryRun, + }), + ), ); } + await Promise.all(newCreatedPVCPromises); }; - const submit = async (data: StorageData) => - handleSubmit(data, notebookData, true).then(() => handleSubmit(data, notebookData, false)); + const submit = async (dataSubmit: StorageData) => + handleSubmit(dataSubmit, true).then(() => handleSubmit(dataSubmit, false)); + + const hasAllValidNotebookRelationship = React.useMemo( + () => + notebookData.every((currentNotebookData) => + currentNotebookData.name + ? !!currentNotebookData.mountPath.value && !currentNotebookData.mountPath.error + : false, + ), + [notebookData], + ); + + const isValid = hasAllValidNotebookRelationship; + + React.useEffect(() => { + const updatedData = notebookData.map((notebook) => { + if (!notebook.existingPvc && !notebook.isUpdatedValue && storageName) { + const defaultPathValue = `${MOUNT_PATH_PREFIX}${storageName + .toLowerCase() + .replace(/\s+/g, '-')}-${notebook.newRowId ?? 1}`; + + const inUseMountPaths = getInUseMountPaths( + notebook.name, + allNotebooks, + existingPvc?.metadata.name, + ); + + return { + ...notebook, + mountPath: { + value: defaultPathValue, + error: validateClusterMountPath(defaultPathValue, inUseMountPaths), + }, + }; + } + return notebook; + }); + + if (!isEqual(updatedData, notebookData)) { + setNotebookData(updatedData); + } + }, [allNotebooks, existingPvc, notebookData, storageName]); + + const handleMountPathUpdate = React.useCallback( + (rowIndex: number, value: string, format: MountPathFormat) => { + const notebook = notebookData[rowIndex]; + const prefix = format === MountPathFormat.STANDARD ? MOUNT_PATH_PREFIX : '/'; + const newValue = `${prefix}${value}`; - const hasValidNotebookRelationship = notebookData.name - ? !!notebookData.mountPath.value && !notebookData.mountPath.error - : true; + const inUseMountPaths = getInUseMountPaths( + notebook.name, + allNotebooks, + existingPvc?.metadata.name, + ); + + const existingPath = notebook.name + ? getNotebookPVCMountPathMap(allNotebooks.find((n) => n.metadata.name === notebook.name))[ + existingPvc?.metadata.name ?? '' + ] + : ''; + + const error = validateClusterMountPath(newValue, inUseMountPaths); + const isSamePvcPath = notebook.existingPvc && newValue === existingPath; - const isValid = hasValidNotebookRelationship; + const updatedData = [...notebookData]; + updatedData[rowIndex] = { + ...notebook, + mountPath: { value: newValue, error }, + isUpdatedValue: !isSamePvcPath, + }; + + setNotebookData(updatedData); + }, + [notebookData, allNotebooks, existingPvc], + ); return ( submit(data)} + onSubmit={(dataSubmit) => submit(dataSubmit)} title={existingPvc ? 'Update cluster storage' : 'Add cluster storage'} description={ existingPvc ? 'Make changes to cluster storage, or connect it to additional workspaces.' : 'Add storage and optionally connect it with an existing workbench.' } + onNameChange={(currentName) => { + setStorageName(currentName); + }} submitLabel={existingPvc ? 'Update' : 'Add storage'} isValid={isValid} onClose={(submitted) => onClose(submitted)} existingPvc={existingPvc} > - {workbenchEnabled && ( - <> - {hasExistingNotebookConnections && ( - - setRemovedNotebooks([...removedNotebooks, notebook.metadata.name]) - } - loaded={removableNotebookLoaded} - error={removableNotebookError} - /> - )} - { - setNotebookData(forNotebookData); - }} - forNotebookData={notebookData} - connectedNotebooks={connectedNotebooks} - /> - {restartNotebooks.length !== 0 && ( - - - - )} - - )} + <> + {workbenchEnabled && ( + + + + ( + currentData.metadata.name === row.name, + ), + ], + loaded, + }} + onMountPathUpdate={(value, format) => + handleMountPathUpdate(rowIndex, value, format) + } + onNotebookSelect={(notebookName) => { + const updatedData = [...notebookData]; + updatedData[rowIndex] = { + ...row, + name: notebookName, + mountPath: { + ...row.mountPath, + error: validateClusterMountPath( + row.mountPath.value, + getInUseMountPaths( + notebookName, + allNotebooks, + existingPvc?.metadata.name, + ), + ), + }, + existingPvc: connectedNotebooks.some( + (n) => n.metadata.name === notebookName, + ), + }; + setNotebookData(updatedData); + }} + onDelete={() => { + const updatedData = [...notebookData]; + updatedData.splice(rowIndex, 1); + setNotebookData(updatedData); + }} + /> + )} + /> + + + + + + {restartNotebooks.length !== 0 && ( + + + + )} + + + )} + ); }; diff --git a/frontend/src/pages/projects/screens/detail/storage/ClusterStorageTableRow.tsx b/frontend/src/pages/projects/screens/detail/storage/ClusterStorageTableRow.tsx new file mode 100644 index 0000000000..b7d79536e2 --- /dev/null +++ b/frontend/src/pages/projects/screens/detail/storage/ClusterStorageTableRow.tsx @@ -0,0 +1,201 @@ +import * as React from 'react'; +import { + ActionList, + ActionListItem, + Bullseye, + Button, + HelperText, + HelperTextItem, + InputGroup, + InputGroupItem, + InputGroupText, + Label, + List, + ListItem, + Popover, + Spinner, + Stack, + StackItem, + TextInput, +} from '@patternfly/react-core'; +import { MinusCircleIcon } from '@patternfly/react-icons'; +import { Td, Tr } from '@patternfly/react-table'; +import TypeaheadSelect from '~/components/TypeaheadSelect'; +import { NotebookKind } from '~/k8sTypes'; +import { ClusterStorageNotebookSelection } from '~/pages/projects/types'; +import { MOUNT_PATH_PREFIX } from '~/pages/projects/screens/spawner/storage/const'; +import SimpleSelect from '~/components/SimpleSelect'; +import { MountPathFormat } from '~/pages/projects/screens/spawner/storage/types'; +import { isMountPathFormat, mountPathFormat, mountPathSuffix } from './utils'; + +type ClusterStorageTableRowProps = { + obj: ClusterStorageNotebookSelection; + availableNotebooks: { notebooks: NotebookKind[]; loaded: boolean }; + onMountPathUpdate: (value: string, format: MountPathFormat) => void; + onNotebookSelect: (notebookName: string) => void; + onDelete: () => void; + inUseMountPaths: string[]; +}; + +const ClusterStorageTableRow: React.FC = ({ + obj, + availableNotebooks, + onMountPathUpdate, + onNotebookSelect, + onDelete, + inUseMountPaths, +}) => { + const suffix = mountPathSuffix(obj.mountPath.value); + const format = mountPathFormat(obj.mountPath.value); + + const pathFormatOptions = [ + { + type: MountPathFormat.STANDARD, + description: `Standard paths that begins with ${MOUNT_PATH_PREFIX} are visible in JupyterLab file browser.`, + }, + { + type: MountPathFormat.CUSTOM, + description: `Custom paths that do not begin with ${MOUNT_PATH_PREFIX} are not visible in the JupyterLab file browser.`, + }, + ]; + + const selectOptions = availableNotebooks.notebooks.map((notebook) => ({ + value: notebook.metadata.name, + content: notebook.metadata.name, + })); + + let placeholderText: string; + + if (!availableNotebooks.loaded) { + placeholderText = 'Loading workbenches'; + } else if (selectOptions.length === 0) { + placeholderText = 'No existing workbench available'; + } else { + placeholderText = 'Select a workbench'; + } + + return ( + + + + + + + ); +}; + +export default ClusterStorageTableRow; diff --git a/frontend/src/pages/projects/screens/detail/storage/ExistingConnectedNotebooks.tsx b/frontend/src/pages/projects/screens/detail/storage/ExistingConnectedNotebooks.tsx deleted file mode 100644 index f59a240f20..0000000000 --- a/frontend/src/pages/projects/screens/detail/storage/ExistingConnectedNotebooks.tsx +++ /dev/null @@ -1,61 +0,0 @@ -import * as React from 'react'; -import { Label, LabelGroup, Alert, FormGroup, Spinner, Content } from '@patternfly/react-core'; - -import { NotebookKind } from '~/k8sTypes'; -import { getDisplayNameFromK8sResource } from '~/concepts/k8s/utils'; - -type ExistingConnectedNotebooksProps = { - connectedNotebooks: NotebookKind[]; - onNotebookRemove: (notebook: NotebookKind) => void; - loaded: boolean; - error?: Error; -}; - -const ExistingConnectedNotebooks: React.FC = ({ - connectedNotebooks, - onNotebookRemove, - loaded, - error, -}) => { - const [notebooksToShow, setNotebooksToShow] = React.useState(connectedNotebooks); - let content: React.ReactNode; - if (error) { - content = ( - - {error.message} - - ); - } else if (!loaded) { - content = ; - } else if (notebooksToShow.length === 0) { - content = All existing connections have been removed.; - } else { - content = ( - - {notebooksToShow.map((notebook) => { - const notebookDisplayName = getDisplayNameFromK8sResource(notebook); - return ( - - ); - })} - - ); - } - - return {content}; -}; - -export default ExistingConnectedNotebooks; diff --git a/frontend/src/pages/projects/screens/detail/storage/StorageTableRow.tsx b/frontend/src/pages/projects/screens/detail/storage/StorageTableRow.tsx index e01757a629..f07df762b1 100644 --- a/frontend/src/pages/projects/screens/detail/storage/StorageTableRow.tsx +++ b/frontend/src/pages/projects/screens/detail/storage/StorageTableRow.tsx @@ -167,7 +167,7 @@ const StorageTableRow: React.FC = ({ {workbenchEnabled && ( - + +
+ + {!obj.existingPvc ? ( + onNotebookSelect('')} + onSelect={(_ev, selectedValue) => { + if (typeof selectedValue === 'string') { + onNotebookSelect(selectedValue); + } + }} + /> + ) : ( + <> + {obj.name}{' '} + + + )} + + ({ + ...option, + label: option.type, + description: option.description, + key: option.type, + }))} + value={format} + onChange={(newSelection) => { + if (isMountPathFormat(newSelection)) { + onMountPathUpdate(suffix, newSelection); + } + }} + previewDescription={false} + /> + + + + + + {format === MountPathFormat.STANDARD ? MOUNT_PATH_PREFIX : '/'} + + + + onMountPathUpdate(value, format)} + isRequired + validated={obj.mountPath.error ? 'error' : 'success'} + /> + + + + + + {obj.mountPath.error && ( + + {obj.mountPath.error.includes(`This path is already connected`) ? ( +
+ {obj.mountPath.error}.{' '} + 0 ? ( + + {inUseMountPaths.map((mountPath, i) => ( + {mountPath} + ))} + + ) : ( + + + Loading + + ) + } + > + + +
+ ) : ( + obj.mountPath.error + )} + + )} + + + + 		+ + + + + +
+ [] = [ + { + label: 'ID', + field: 'id', + sortable: false, + visibility: ['hidden'], + }, + { + label: 'Name', + field: 'name', + sortable: false, + width: 35, + }, + { + label: 'Path format', + field: 'pathFormat', + sortable: false, + width: 20, + }, + { + label: 'Mount path', + field: 'mountPath', + sortable: false, + info: { + popover: + 'The directory within a container where a volume is mounted and accessible. Must consist of lowercase letters, numbers and hyphens (-). Use slashes (/) to indicate subdirectories.', + }, + width: 35, + }, +]; diff --git a/frontend/src/pages/projects/screens/detail/storage/data.ts b/frontend/src/pages/projects/screens/detail/storage/data.ts index 047fc40631..3731d6234e 100644 --- a/frontend/src/pages/projects/screens/detail/storage/data.ts +++ b/frontend/src/pages/projects/screens/detail/storage/data.ts @@ -38,7 +38,7 @@ export const columns: SortableData[] = [ }, { field: 'connected', - label: 'Connected workbenches', + label: 'Workbench connections', width: 20, sortable: false, }, diff --git a/frontend/src/pages/projects/screens/detail/storage/utils.ts b/frontend/src/pages/projects/screens/detail/storage/utils.ts index 8198d0c57f..5e0ed6ce75 100644 --- a/frontend/src/pages/projects/screens/detail/storage/utils.ts +++ b/frontend/src/pages/projects/screens/detail/storage/utils.ts @@ -1,6 +1,9 @@ import { getDescriptionFromK8sResource, getDisplayNameFromK8sResource } from '~/concepts/k8s/utils'; -import { PersistentVolumeClaimKind } from '~/k8sTypes'; -import { StorageData } from '~/pages/projects/types'; +import { NotebookKind, PersistentVolumeClaimKind } from '~/k8sTypes'; +import { ClusterStorageNotebookSelection, StorageData } from '~/pages/projects/types'; +import { MOUNT_PATH_PREFIX } from '~/pages/projects/screens/spawner/storage/const'; +import { MountPathFormat } from '~/pages/projects/screens/spawner/storage/types'; +import { getNotebookPVCMountPathMap } from '~/pages/projects/notebook/utils'; type Status = 'error' | 'warning' | 'info' | null; export const getFullStatusFromPercentage = (percentageFull: number): Status => { @@ -24,3 +27,82 @@ export const isPvcUpdateRequired = ( getDescriptionFromK8sResource(existingPvc) !== storageData.description || existingPvc.spec.resources.requests.storage !== storageData.size || existingPvc.spec.storageClassName !== storageData.storageClassName; + +export const handleConnectedNotebooks = ( + tempData: ClusterStorageNotebookSelection[], + notebooks: NotebookKind[], +): { + updatedNotebooks: ClusterStorageNotebookSelection[]; + removedNotebooks: string[]; +} => { + const updatedNotebooks: ClusterStorageNotebookSelection[] = []; + const removedNotebooks: string[] = []; + notebooks.forEach((notebook) => { + const tempEntry = tempData.find( + (item) => item.existingPvc && item.name === notebook.metadata.name, + ); + if (!tempEntry) { + removedNotebooks.push(notebook.metadata.name); + } else if (tempEntry.isUpdatedValue) { + updatedNotebooks.push(tempEntry); + } + }); + + return { updatedNotebooks, removedNotebooks }; +}; + +export const mountPathSuffix = (mountPath: string): string => + mountPath.startsWith(MOUNT_PATH_PREFIX) + ? mountPath.slice(MOUNT_PATH_PREFIX.length) + : mountPath.slice(1); + +export const mountPathFormat = (mountPath: string): MountPathFormat => + mountPath.startsWith(MOUNT_PATH_PREFIX) ? MountPathFormat.STANDARD : MountPathFormat.CUSTOM; + +export const isMountPathFormat = (value: string): value is MountPathFormat => + value === MountPathFormat.STANDARD || value === MountPathFormat.CUSTOM; + +export const validateClusterMountPath = (value: string, inUseMountPaths: string[]): string => { + const format = value.startsWith(MOUNT_PATH_PREFIX) + ? MountPathFormat.STANDARD + : MountPathFormat.CUSTOM; + + if (value === '/' && format === MountPathFormat.CUSTOM) { + return 'Enter a path to a model or folder. This path cannot point to a root folder.'; + } + + // Regex to allow empty string for Standard format + const regex = + format === MountPathFormat.STANDARD + ? /^(\/?[a-z0-9-]+(\/[a-z0-9-]+)*\/?|)$/ + : /^(\/?[a-z0-9-]+(\/[a-z0-9-]+)*\/?)?$/; + + if (!regex.test(value)) { + return 'Must consist of lowercase letters, numbers and hyphens (-). Use slashes (/) to indicate the subdirectories'; + } + + if ( + inUseMountPaths.includes(value) || + (format === MountPathFormat.STANDARD && + inUseMountPaths.includes(`${MOUNT_PATH_PREFIX}${value}`)) || + (format === MountPathFormat.CUSTOM && inUseMountPaths.includes(`/${value}`)) + ) { + return `This path is already connected to this workbench, Try a different folder name`; + } + + return ''; +}; + +export const getInUseMountPaths = ( + currentNotebookName: string, + availableNotebooks: NotebookKind[], + existingPvcName?: string, +): string[] => { + const allInUseMountPaths = getNotebookPVCMountPathMap( + availableNotebooks.find((notebook) => notebook.metadata.name === currentNotebookName), + ); + + return Object.keys(allInUseMountPaths) + .filter((key) => key !== existingPvcName) + .map((key) => allInUseMountPaths[key]); +}; diff --git a/frontend/src/pages/projects/screens/spawner/storage/ClusterStorageTable.tsx b/frontend/src/pages/projects/screens/spawner/storage/ClusterStorageTable.tsx index 000fda3ea5..82ed937458 100644 --- a/frontend/src/pages/projects/screens/spawner/storage/ClusterStorageTable.tsx +++ b/frontend/src/pages/projects/screens/spawner/storage/ClusterStorageTable.tsx @@ -85,7 +85,12 @@ export const ClusterStorageTable: React.FC = ({ columns={clusterStorageTableColumns} data={storageData} rowRenderer={(row, rowIndex) => ( -
{ it('should return error message for invalid characters in the path', () => { const result = validateMountPath('Invalid/Path', inUseMountPaths); - expect(result).toBe('Must only consist of lowercase letters, dashes, and slashes.'); + expect(result).toBe('Must only consist of lowercase letters, dashes, numbers and slashes.'); }); it('should return error message for already in-use mount path', () => { @@ -106,7 +106,7 @@ describe('validateMountPath', () => { it('should return error for an invalid folder name with numbers or uppercase letters', () => { const result = validateMountPath('Invalid123', inUseMountPaths); - expect(result).toBe('Must only consist of lowercase letters, dashes, and slashes.'); + expect(result).toBe('Must only consist of lowercase letters, dashes, numbers and slashes.'); }); it('should return an empty string for valid mount path in CUSTOM format', () => { @@ -116,7 +116,7 @@ describe('validateMountPath', () => { it('should return error for an invalid folder name with uppercase letters in CUSTOM format', () => { const result = validateMountPath('InvalidFolder', inUseMountPaths); - expect(result).toBe('Must only consist of lowercase letters, dashes, and slashes.'); + expect(result).toBe('Must only consist of lowercase letters, dashes, numbers and slashes.'); }); }); diff --git a/frontend/src/pages/projects/screens/spawner/storage/constants.ts b/frontend/src/pages/projects/screens/spawner/storage/constants.ts index 7a59141022..c7bf85e06f 100644 --- a/frontend/src/pages/projects/screens/spawner/storage/constants.ts +++ b/frontend/src/pages/projects/screens/spawner/storage/constants.ts @@ -7,7 +7,7 @@ export const clusterStorageTableColumns: SortableData[] = [ label: 'ID', field: 'id', sortable: false, - className: 'pf-v6-u-hidden', + visibility: ['hidden'], }, { label: 'Name', diff --git a/frontend/src/pages/projects/screens/spawner/storage/types.ts b/frontend/src/pages/projects/screens/spawner/storage/types.ts index aed06196d8..dc86350a31 100644 --- a/frontend/src/pages/projects/screens/spawner/storage/types.ts +++ b/frontend/src/pages/projects/screens/spawner/storage/types.ts @@ -1,4 +1,4 @@ export enum MountPathFormat { - STANDARD = 'standard', - CUSTOM = 'custom', + STANDARD = 'Standard', + CUSTOM = 'Custom', } diff --git a/frontend/src/pages/projects/screens/spawner/storage/utils.ts b/frontend/src/pages/projects/screens/spawner/storage/utils.ts index a0cabdcb6d..c5c88de8c8 100644 --- a/frontend/src/pages/projects/screens/spawner/storage/utils.ts +++ b/frontend/src/pages/projects/screens/spawner/storage/utils.ts @@ -148,11 +148,11 @@ export const validateMountPath = (value: string, inUseMountPaths: string[]): str // Regex to allow empty string for Standard format const regex = format === MountPathFormat.STANDARD - ? /^(\/?[a-z-]+(\/[a-z-]+)*\/?|)$/ - : /^(\/?[a-z-]+(\/[a-z-]+)*\/?)$/; + ? /^(\/?[a-z0-9-]+(\/[a-z0-9-]+)*\/?|)$/ + : /^(\/?[a-z0-9-]+(\/[a-z0-9-]+)*\/?)?$/; if (!regex.test(value)) { - return 'Must only consist of lowercase letters, dashes, and slashes.'; + return 'Must only consist of lowercase letters, dashes, numbers and slashes.'; } if ( diff --git a/frontend/src/pages/projects/types.ts b/frontend/src/pages/projects/types.ts index 45bc9a2815..57011cc431 100644 --- a/frontend/src/pages/projects/types.ts +++ b/frontend/src/pages/projects/types.ts @@ -38,6 +38,12 @@ export type ForNotebookSelection = { mountPath: MountPath; }; +export type ClusterStorageNotebookSelection = ForNotebookSelection & { + existingPvc: boolean; + isUpdatedValue: boolean; + newRowId?: number; +}; + export type CreatingStorageObjectForNotebook = NameDescType & { size: string; forNotebook: ForNotebookSelection; From 9124c70edfc87a970af94506cf918e1d954c5cce Mon Sep 17 00:00:00 2001 From: Yulia Krimerman Date: Wed, 11 Dec 2024 12:49:58 -0500 Subject: [PATCH 07/10] Fixing multiple models deploy (#3561) * fixing multiple models deploy * test update * fix tests * additional test --- .../tests/mocked/modelServing/modelServingGlobal.cy.ts | 6 +++--- .../tests/mocked/modelServing/servingRuntimeList.cy.ts | 2 +- frontend/src/api/k8s/__tests__/inferenceServices.spec.ts | 2 +- frontend/src/api/k8s/inferenceServices.ts | 2 +- .../projects/NIMServiceModal/ManageNIMServingModal.tsx | 4 +--- .../NIMServiceModal/NIMModelDeploymentNameSection.tsx | 6 +++++- 6 files changed, 12 insertions(+), 10 deletions(-) diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/modelServingGlobal.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/modelServingGlobal.cy.ts index 29ca447af0..26caee8dfe 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/modelServingGlobal.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/modelServingGlobal.cy.ts @@ -344,7 +344,7 @@ describe('Model Serving Global', () => { 'POST', InferenceServiceModel, mockInferenceServiceK8sResource({ - name: 'test-name', + name: 'test-model', path: 'test-model/', displayName: 'Test Name', isModelMesh: true, @@ -403,7 +403,7 @@ describe('Model Serving Global', () => { apiVersion: 'serving.kserve.io/v1beta1', kind: 'InferenceService', metadata: { - name: 'test-model', + name: 'test-name', namespace: 'test-project', labels: { 'opendatahub.io/dashboard': 'true' }, annotations: { @@ -468,7 +468,7 @@ describe('Model Serving Global', () => { apiVersion: 'serving.kserve.io/v1beta1', kind: 'InferenceService', metadata: { - name: 'test-model', + name: 'trigger-error', namespace: 'test-project', labels: { 'opendatahub.io/dashboard': 'true' }, annotations: { diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts index 9723d64a6e..99a08aae0d 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts @@ -478,7 +478,7 @@ describe('Serving Runtime List', () => { expect(interception.request.url).to.include('?dryRun=All'); expect(interception.request.body).to.containSubset({ metadata: { - name: 'test-model-legacy', + name: 'test-name', namespace: 'test-project', labels: { 'opendatahub.io/dashboard': 'true' }, annotations: { diff --git a/frontend/src/api/k8s/__tests__/inferenceServices.spec.ts b/frontend/src/api/k8s/__tests__/inferenceServices.spec.ts index 93d0f9aef6..ba384e29b7 100644 --- a/frontend/src/api/k8s/__tests__/inferenceServices.spec.ts +++ b/frontend/src/api/k8s/__tests__/inferenceServices.spec.ts @@ -143,7 +143,7 @@ describe('assembleInferenceService', () => { const resourceName = 'llama-model'; const inferenceService = assembleInferenceService( - mockInferenceServiceModalData({ name: displayName, servingRuntimeName: resourceName }), + mockInferenceServiceModalData({ name: displayName, k8sName: resourceName }), ); expect(inferenceService.metadata.annotations).toBeDefined(); diff --git a/frontend/src/api/k8s/inferenceServices.ts b/frontend/src/api/k8s/inferenceServices.ts index aa09bdf25c..fded27ccbc 100644 --- a/frontend/src/api/k8s/inferenceServices.ts +++ b/frontend/src/api/k8s/inferenceServices.ts @@ -40,7 +40,7 @@ export const assembleInferenceService = ( servingRuntimeArgs, servingRuntimeEnvVars, } = data; - const name = editName || servingRuntimeName; + const name = editName || data.k8sName; const { path, dataConnection, uri } = storage; const dataConnectionKey = secretKey || dataConnection; diff --git a/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/ManageNIMServingModal.tsx b/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/ManageNIMServingModal.tsx index 9ee07d0e5d..3de12e9e32 100644 --- a/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/ManageNIMServingModal.tsx +++ b/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/ManageNIMServingModal.tsx @@ -104,7 +104,6 @@ const ManageNIMServingModal: React.FC = ({ const [servingRuntimeSelected, setServingRuntimeSelected] = React.useState< ServingRuntimeKind | undefined >(undefined); - const { formData: selectedAcceleratorProfile, setFormData: setSelectedAcceleratorProfile, @@ -224,8 +223,7 @@ const ManageNIMServingModal: React.FC = ({ false, ); - const inferenceServiceName = translateDisplayNameForK8s(createDataInferenceService.name); - + const inferenceServiceName = createDataInferenceService.k8sName; const submitInferenceServiceResource = getSubmitInferenceServiceResourceFn( createDataInferenceService, editInfo?.inferenceServiceEditInfo, diff --git a/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/NIMModelDeploymentNameSection.tsx b/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/NIMModelDeploymentNameSection.tsx index 20f894ec1e..566faf80f0 100644 --- a/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/NIMModelDeploymentNameSection.tsx +++ b/frontend/src/pages/modelServing/screens/projects/NIMServiceModal/NIMModelDeploymentNameSection.tsx @@ -2,6 +2,7 @@ import * as React from 'react'; import { FormGroup, TextInput } from '@patternfly/react-core'; import { UpdateObjectAtPropAndValue } from '~/pages/projects/types'; import { CreatingInferenceServiceObject } from '~/pages/modelServing/screens/types'; +import { translateDisplayNameForK8s } from '~/concepts/k8s/utils'; type NIMModelDeploymentNameSectionProps = { data: CreatingInferenceServiceObject; @@ -18,7 +19,10 @@ const NIMModelDeploymentNameSection: React.FC setData('name', name)} + onChange={(e, name) => { + setData('name', name); + setData('k8sName', translateDisplayNameForK8s(name)); + }} /> ); From 9f2b3d1472621e2315d324ce1323e12b16544324 Mon Sep 17 00:00:00 2001 From: Dallas <5322142+gitdallas@users.noreply.github.com> Date: Wed, 11 Dec 2024 13:06:44 -0600 Subject: [PATCH 08/10] feat(15898): create MR secure db feature flag, skeleton for form elements (#3555) * feat(ca cert) 15898 - add disableMRSecureDB feature flag Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> * add checkbox/radios for adding ca cert Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> * add existing cert search fields Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> * add file upload for new ca cert Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> * securedb cert upload field, form validation Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> * cleanup Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> * renaming feature flag, fix style typo Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> * remove feature flag from odhdashboardconfig Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> * address pr nits Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> * factor out secureDB form components Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> --------- Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> --- backend/src/types.ts | 1 + backend/src/utils/constants.ts | 1 + docs/dashboard-config.md | 1 + frontend/src/__mocks__/mockDashboardConfig.ts | 3 + frontend/src/concepts/areas/const.ts | 5 + frontend/src/concepts/areas/types.ts | 5 +- frontend/src/k8sTypes.ts | 1 + .../CreateMRSecureDBSection.tsx | 260 ++++++++++++++++++ .../modelRegistrySettings/CreateModal.tsx | 51 +++- .../modelRegistrySettings/PemFileUpload.tsx | 80 ++++++ ...dhdashboardconfigs.opendatahub.io.crd.yaml | 2 + 11 files changed, 406 insertions(+), 4 deletions(-) create mode 100644 frontend/src/pages/modelRegistrySettings/CreateMRSecureDBSection.tsx create mode 100644 frontend/src/pages/modelRegistrySettings/PemFileUpload.tsx diff --git a/backend/src/types.ts b/backend/src/types.ts index 4024b82a55..59a132bd14 100644 --- a/backend/src/types.ts +++ b/backend/src/types.ts @@ -40,6 +40,7 @@ export type DashboardConfig = K8sResourceCommon & { disableHardwareProfiles: boolean; disableDistributedWorkloads: boolean; disableModelRegistry: boolean; + disableModelRegistrySecureDB: boolean; disableServingRuntimeParams: boolean; disableConnectionTypes: boolean; disableStorageClasses: boolean; diff --git a/backend/src/utils/constants.ts b/backend/src/utils/constants.ts index 9e1a94a3c7..1091725da0 100644 --- a/backend/src/utils/constants.ts +++ b/backend/src/utils/constants.ts @@ -65,6 +65,7 @@ export const blankDashboardCR: DashboardConfig = { disableHardwareProfiles: true, disableDistributedWorkloads: false, disableModelRegistry: false, + disableModelRegistrySecureDB: true, disableServingRuntimeParams: false, disableConnectionTypes: false, disableStorageClasses: false, diff --git a/docs/dashboard-config.md b/docs/dashboard-config.md index c4eeb8d7a4..ef40f7304b 100644 --- a/docs/dashboard-config.md +++ b/docs/dashboard-config.md @@ -36,6 +36,7 @@ The following are a list of features that are supported, along with there defaul | disablePerformanceMetrics | false | Disables Endpoint Performance tab from Model Serving metrics. | | disableDistributedWorkloads | false | Disables Distributed Workload Metrics from the dashboard. | | disableModelRegistry | false | Disables Model Registry from the dashboard. | +| disableModelRegistrySecureDB | true | Disables Model Registry Secure DB from the dashboard. | | disableServingRuntimeParams | false | Disables Serving Runtime params from the dashboard. | | disableStorageClasses | false | Disables storage classes settings nav item from the dashboard. | | disableNIMModelServing | true | Disables components of NIM Model UI from the dashboard. | diff --git a/frontend/src/__mocks__/mockDashboardConfig.ts b/frontend/src/__mocks__/mockDashboardConfig.ts index d3724fa78b..eaa8b4919b 100644 --- a/frontend/src/__mocks__/mockDashboardConfig.ts +++ b/frontend/src/__mocks__/mockDashboardConfig.ts @@ -26,6 +26,7 @@ export type MockDashboardConfigType = { disableTrustyBiasMetrics?: boolean; disableDistributedWorkloads?: boolean; disableModelRegistry?: boolean; + disableModelRegistrySecureDB?: boolean; disableServingRuntimeParams?: boolean; disableConnectionTypes?: boolean; disableStorageClasses?: boolean; @@ -59,6 +60,7 @@ export const mockDashboardConfig = ({ disableTrustyBiasMetrics = false, disableDistributedWorkloads = false, disableModelRegistry = false, + disableModelRegistrySecureDB = true, disableServingRuntimeParams = false, disableConnectionTypes = true, disableStorageClasses = false, @@ -169,6 +171,7 @@ export const mockDashboardConfig = ({ disableHardwareProfiles, disableDistributedWorkloads, disableModelRegistry, + disableModelRegistrySecureDB, disableServingRuntimeParams, disableConnectionTypes, disableStorageClasses, diff --git a/frontend/src/concepts/areas/const.ts b/frontend/src/concepts/areas/const.ts index 42e4df9c24..39b4809034 100644 --- a/frontend/src/concepts/areas/const.ts +++ b/frontend/src/concepts/areas/const.ts @@ -28,6 +28,7 @@ export const allFeatureFlags: string[] = Object.keys({ disableHardwareProfiles: false, disableDistributedWorkloads: false, disableModelRegistry: false, + disableModelRegistrySecureDB: true, disableServingRuntimeParams: false, disableConnectionTypes: false, disableStorageClasses: false, @@ -129,6 +130,10 @@ export const SupportedAreasStateMap: SupportedAreasState = { featureFlags: ['disableServingRuntimeParams'], reliantAreas: [SupportedArea.K_SERVE, SupportedArea.MODEL_SERVING], }, + [SupportedArea.MODEL_REGISTRY_SECURE_DB]: { + featureFlags: ['disableModelRegistrySecureDB'], + reliantAreas: [SupportedArea.MODEL_REGISTRY], + }, [SupportedArea.NIM_MODEL]: { featureFlags: ['disableNIMModelServing'], reliantAreas: [SupportedArea.K_SERVE], diff --git a/frontend/src/concepts/areas/types.ts b/frontend/src/concepts/areas/types.ts index 73710595a4..d8c2d2bf17 100644 --- a/frontend/src/concepts/areas/types.ts +++ b/frontend/src/concepts/areas/types.ts @@ -57,15 +57,14 @@ export enum SupportedArea { PERFORMANCE_METRICS = 'performance-metrics', TRUSTY_AI = 'trusty-ai', NIM_MODEL = 'nim-model', + SERVING_RUNTIME_PARAMS = 'serving-runtime-params', /* Distributed Workloads areas */ DISTRIBUTED_WORKLOADS = 'distributed-workloads', /* Model Registry areas */ MODEL_REGISTRY = 'model-registry', - - /* Alter parameters areas */ - SERVING_RUNTIME_PARAMS = 'serving-runtime-params', + MODEL_REGISTRY_SECURE_DB = 'model-registry-secure-db', } /** Components deployed by the Operator. Part of the DSC Status. */ diff --git a/frontend/src/k8sTypes.ts b/frontend/src/k8sTypes.ts index 4b89950116..e6e4cd51a8 100644 --- a/frontend/src/k8sTypes.ts +++ b/frontend/src/k8sTypes.ts @@ -1192,6 +1192,7 @@ export type DashboardCommonConfig = { disableHardwareProfiles: boolean; disableDistributedWorkloads: boolean; disableModelRegistry: boolean; + disableModelRegistrySecureDB: boolean; disableServingRuntimeParams: boolean; disableConnectionTypes: boolean; disableStorageClasses: boolean; diff --git a/frontend/src/pages/modelRegistrySettings/CreateMRSecureDBSection.tsx b/frontend/src/pages/modelRegistrySettings/CreateMRSecureDBSection.tsx new file mode 100644 index 0000000000..2ee7aafa8f --- /dev/null +++ b/frontend/src/pages/modelRegistrySettings/CreateMRSecureDBSection.tsx @@ -0,0 +1,260 @@ +import React, { useState } from 'react'; +import { FormGroup, Radio, Alert, MenuItem, MenuGroup } from '@patternfly/react-core'; +import SearchSelector from '~/components/searchSelector/SearchSelector'; +import { translateDisplayNameForK8s } from '~/concepts/k8s/utils'; +import { RecursivePartial } from '~/typeHelpers'; +import { PemFileUpload } from './PemFileUpload'; + +export enum SecureDBRType { + CLUSTER_WIDE = 'cluster-wide', + OPENSHIFT = 'openshift', + EXISTING = 'existing', + NEW = 'new', +} + +export interface SecureDBInfo { + type: SecureDBRType; + configMap: string; + key: string; + certificate: string; + nameSpace: string; + isValid: boolean; +} + +interface CreateMRSecureDBSectionProps { + secureDBInfo: SecureDBInfo; + modelRegistryNamespace: string; + nameDesc: { name: string }; + existingCertKeys: string[]; + existingCertConfigMaps: string[]; + existingCertSecrets: string[]; + setSecureDBInfo: (info: SecureDBInfo) => void; +} + +export const CreateMRSecureDBSection: React.FC = ({ + secureDBInfo, + modelRegistryNamespace, + nameDesc, + existingCertKeys, + existingCertConfigMaps, + existingCertSecrets, + setSecureDBInfo, +}) => { + const [searchValue, setSearchValue] = useState(''); + + const hasContent = (value: string): boolean => !!value.trim().length; + + const isValid = (info?: RecursivePartial) => { + const fullInfo: SecureDBInfo = { ...secureDBInfo, ...info }; + if ([SecureDBRType.CLUSTER_WIDE, SecureDBRType.OPENSHIFT].includes(fullInfo.type)) { + return true; + } + if (fullInfo.type === SecureDBRType.EXISTING) { + return hasContent(fullInfo.configMap) && hasContent(fullInfo.key); + } + if (fullInfo.type === SecureDBRType.NEW) { + return hasContent(fullInfo.certificate); + } + return false; + }; + + const handleSecureDBTypeChange = (type: SecureDBRType) => { + const newInfo = { + type, + nameSpace: '', + key: '', + configMap: '', + certificate: '', + }; + setSecureDBInfo({ + ...newInfo, + isValid: isValid(newInfo), + }); + }; + + const getFilteredExistingCAResources = () => ( + <> + + {existingCertConfigMaps + .filter((configMap) => configMap.toLowerCase().includes(searchValue.toLowerCase())) + .map((configMap, index) => ( + { + setSearchValue(''); + const newInfo = { + ...secureDBInfo, + configMap, + key: '', + }; + setSecureDBInfo({ + ...newInfo, + isValid: isValid(newInfo), + }); + }} + > + {configMap} + + ))} + + + {existingCertSecrets + .filter((secret) => secret.toLowerCase().includes(searchValue.toLowerCase())) + .map((secret, index) => ( + { + setSearchValue(''); + const newInfo = { + ...secureDBInfo, + configMap: secret, + key: '', + }; + setSecureDBInfo({ + ...newInfo, + isValid: isValid(newInfo), + }); + }} + > + {secret} + + ))} + + + ); + + return ( + <> + handleSecureDBTypeChange(SecureDBRType.CLUSTER_WIDE)} + label="Use cluster-wide CA bundle" + description={ + <> + Use the ca-bundle.crt bundle in the{' '} + odh-trusted-ca-bundle ConfigMap. + + } + id="cluster-wide-ca" + /> + handleSecureDBTypeChange(SecureDBRType.OPENSHIFT)} + label="Use OpenShift AI CA bundle" + description={ + <> + Use the odh-ca-bundle.crt bundle in the{' '} + odh-trusted-ca-bundle ConfigMap. + + } + id="openshift-ca" + /> + handleSecureDBTypeChange(SecureDBRType.EXISTING)} + label="Choose from existing certificates" + description={ + <> + You can select the key of any ConfigMap or Secret in the{' '} + {modelRegistryNamespace} namespace. + + } + id="existing-ca" + /> + {secureDBInfo.type === SecureDBRType.EXISTING && ( + <> + + setSearchValue(newValue)} + onSearchClear={() => setSearchValue('')} + searchValue={searchValue} + toggleText={secureDBInfo.configMap || 'Select a ConfigMap or a Secret'} + > + {getFilteredExistingCAResources()} + + + + setSearchValue(newValue)} + onSearchClear={() => setSearchValue('')} + searchValue={searchValue} + toggleText={secureDBInfo.key || 'Select a key'} + > + {existingCertKeys + .filter((item) => item.toLowerCase().includes(searchValue.toLowerCase())) + .map((item, index) => ( + { + setSearchValue(''); + const newInfo = { + ...secureDBInfo, + key: item, + }; + setSecureDBInfo({ ...newInfo, isValid: isValid(newInfo) }); + }} + > + {item} + + ))} + + + + )} + handleSecureDBTypeChange(SecureDBRType.NEW)} + label="Upload new certificate" + id="new-ca" + /> + {secureDBInfo.type === SecureDBRType.NEW && ( + <> + + Uploading a certificate below creates the{' '} + {translateDisplayNameForK8s(nameDesc.name)}-db-credential ConfigMap + with the ca.crt key. If you'd like to upload the certificate as a + Secret instead, see the documentation for more details. + + + { + const newInfo = { + ...secureDBInfo, + certificate: value, + }; + setSecureDBInfo({ ...newInfo, isValid: isValid(newInfo) }); + }} + /> + + + )} + + ); +}; diff --git a/frontend/src/pages/modelRegistrySettings/CreateModal.tsx b/frontend/src/pages/modelRegistrySettings/CreateModal.tsx index 2c4687e404..6017028bb5 100644 --- a/frontend/src/pages/modelRegistrySettings/CreateModal.tsx +++ b/frontend/src/pages/modelRegistrySettings/CreateModal.tsx @@ -1,6 +1,7 @@ import * as React from 'react'; import { Button, + Checkbox, Form, FormGroup, HelperText, @@ -18,6 +19,8 @@ import NameDescriptionField from '~/concepts/k8s/NameDescriptionField'; import { NameDescType } from '~/pages/projects/types'; import FormSection from '~/components/pf-overrides/FormSection'; import { AreaContext } from '~/concepts/areas/AreaContext'; +import { SupportedArea, useIsAreaAvailable } from '~/concepts/areas'; +import { CreateMRSecureDBSection, SecureDBInfo, SecureDBRType } from './CreateMRSecureDBSection'; type CreateModalProps = { onClose: () => void; @@ -37,6 +40,15 @@ const CreateModal: React.FC = ({ onClose, refresh }) => { const [username, setUsername] = React.useState(''); const [password, setPassword] = React.useState(''); const [database, setDatabase] = React.useState(''); + const [addSecureDB, setAddSecureDB] = React.useState(false); + const [secureDBInfo, setSecureDBInfo] = React.useState({ + type: SecureDBRType.CLUSTER_WIDE, + nameSpace: '', + configMap: '', + certificate: '', + key: '', + isValid: true, + }); const [isHostTouched, setIsHostTouched] = React.useState(false); const [isPortTouched, setIsPortTouched] = React.useState(false); const [isUsernameTouched, setIsUsernameTouched] = React.useState(false); @@ -44,6 +56,18 @@ const CreateModal: React.FC = ({ onClose, refresh }) => { const [isDatabaseTouched, setIsDatabaseTouched] = React.useState(false); const [showPassword, setShowPassword] = React.useState(false); const { dscStatus } = React.useContext(AreaContext); + const secureDbEnabled = useIsAreaAvailable(SupportedArea.MODEL_REGISTRY_SECURE_DB).status; + + const modelRegistryNamespace = dscStatus?.components?.modelregistry?.registriesNamespace || ''; + + // the 3 following consts are temporary hard-coded values to be replaced as part of RHOAIENG-15899 + const existingCertConfigMaps = [ + 'config-service-cabundle', + 'odh-trusted-ca-bundle', + 'foo-ca-bundle', + ]; + const existingCertSecrets = ['builder-dockercfg-b7gdr', 'builder-token-hwsps', 'foo-secret']; + const existingCertKeys = ['service-ca.crt', 'foo-ca.crt']; const onBeforeClose = () => { setIsSubmitting(false); @@ -120,7 +144,8 @@ const CreateModal: React.FC = ({ onClose, refresh }) => { hasContent(password) && hasContent(port) && hasContent(username) && - hasContent(database); + hasContent(database) && + (!addSecureDB || secureDBInfo.isValid); return ( = ({ onClose, refresh }) => { )} + {!secureDbEnabled && ( + <> + + setAddSecureDB(value)} + id="add-secure-db" + name="add-secure-db" + /> + + {addSecureDB && ( + + )} + + )} diff --git a/frontend/src/pages/modelRegistrySettings/PemFileUpload.tsx b/frontend/src/pages/modelRegistrySettings/PemFileUpload.tsx new file mode 100644 index 0000000000..801b9690df --- /dev/null +++ b/frontend/src/pages/modelRegistrySettings/PemFileUpload.tsx @@ -0,0 +1,80 @@ +import { + DropEvent, + FileUpload, + FormHelperText, + HelperText, + HelperTextItem, +} from '@patternfly/react-core'; +import React from 'react'; + +export const PemFileUpload: React.FC<{ onChange: (value: string) => void }> = ({ onChange }) => { + const [value, setValue] = React.useState(''); + const [filename, setFilename] = React.useState(''); + const [isLoading, setIsLoading] = React.useState(false); + const [isRejected, setIsRejected] = React.useState(false); + + const handleFileInputChange = (_event: DropEvent, file: File) => { + setFilename(file.name); + }; + + const handleTextChange = (_event: React.ChangeEvent, val: string) => { + setValue(val); + }; + + const handleDataChange = (_event: DropEvent, val: string) => { + onChange(val); + setValue(val); + }; + + const handleClear = () => { + setFilename(''); + setValue(''); + setIsRejected(false); + }; + + const handleFileRejected = () => { + setIsRejected(true); + }; + + const handleFileReadStarted = () => { + setIsLoading(true); + }; + + const handleFileReadFinished = () => { + setIsLoading(false); + }; + + return ( + <> + + + + + {isRejected ? 'Must be a PEM file' : 'Upload a PEM file'} + + + + + ); +}; diff --git a/manifests/common/crd/odhdashboardconfigs.opendatahub.io.crd.yaml b/manifests/common/crd/odhdashboardconfigs.opendatahub.io.crd.yaml index ed4dd72dda..75d3a354e2 100644 --- a/manifests/common/crd/odhdashboardconfigs.opendatahub.io.crd.yaml +++ b/manifests/common/crd/odhdashboardconfigs.opendatahub.io.crd.yaml @@ -73,6 +73,8 @@ spec: type: boolean disableModelRegistry: type: boolean + disableModelRegistrySecureDB: + type: boolean disableServingRuntimeParams: type: boolean disableStorageClasses: From 60ba3eedde74409d49b8e6e65644982c7fa603a1 Mon Sep 17 00:00:00 2001 From: Dallas <5322142+gitdallas@users.noreply.github.com> Date: Wed, 11 Dec 2024 15:15:59 -0600 Subject: [PATCH 09/10] fix(test): add wait in deploy model test (#3567) Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> --- .../cypress/pages/modelRegistry/modelVersionDeployModal.ts | 2 +- .../tests/mocked/modelRegistry/modelVersionDeploy.cy.ts | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/frontend/src/__tests__/cypress/cypress/pages/modelRegistry/modelVersionDeployModal.ts b/frontend/src/__tests__/cypress/cypress/pages/modelRegistry/modelVersionDeployModal.ts index 8887055e77..5551e3d10f 100644 --- a/frontend/src/__tests__/cypress/cypress/pages/modelRegistry/modelVersionDeployModal.ts +++ b/frontend/src/__tests__/cypress/cypress/pages/modelRegistry/modelVersionDeployModal.ts @@ -11,7 +11,7 @@ class ModelVersionDeployModal extends Modal { selectProjectByName(name: string) { this.findProjectSelector().click(); - this.find().findByRole('option', { name }).click(); + this.find().findByRole('option', { name, timeout: 5000 }).click(); } } diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDeploy.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDeploy.cy.ts index 0d80fa1d10..a89fe0e42b 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDeploy.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDeploy.cy.ts @@ -135,7 +135,7 @@ const initIntercepts = ({ }), mockProjectK8sResource({ k8sName: 'test-project', displayName: 'Test project' }), ]), - ); + ).as('getProjects'); cy.interceptOdh( `GET /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/model_versions/:modelVersionId/artifacts`, @@ -185,8 +185,10 @@ describe('Deploy model version', () => { cy.visit(`/modelRegistry/modelregistry-sample/registeredModels/1/versions`); const modelVersionRow = modelRegistry.getModelVersionRow('test model version'); modelVersionRow.findKebabAction('Deploy').click(); + cy.wait('@getProjects'); modelVersionDeployModal.selectProjectByName('Model mesh project'); cy.findByText('Multi-model platform is not installed').should('exist'); + cy.wait('@getProjects'); modelVersionDeployModal.selectProjectByName('KServe project'); cy.findByText('Single-model platform is not installed').should('exist'); }); From 6016195f0ed2df8bfab8ff822071859c7a736413 Mon Sep 17 00:00:00 2001 From: Dallas <5322142+gitdallas@users.noreply.github.com> Date: Wed, 11 Dec 2024 15:35:12 -0600 Subject: [PATCH 10/10] fix(secureDb): undo debugging boolean flip (#3568) Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> --- frontend/src/pages/modelRegistrySettings/CreateModal.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/pages/modelRegistrySettings/CreateModal.tsx b/frontend/src/pages/modelRegistrySettings/CreateModal.tsx index 6017028bb5..caf429ec99 100644 --- a/frontend/src/pages/modelRegistrySettings/CreateModal.tsx +++ b/frontend/src/pages/modelRegistrySettings/CreateModal.tsx @@ -282,7 +282,7 @@ const CreateModal: React.FC = ({ onClose, refresh }) => { )} - {!secureDbEnabled && ( + {secureDbEnabled && ( <>