Skip to content

Commit

Permalink
Storage class dropdown select (#3212)
Browse files Browse the repository at this point in the history
* RHOAIENG-1109 Select Storage Classes in add cluster storage sections

* refactor: Remove unnecessary storageClassName parameter in replaceRootVolumesForNotebook function

fix tests

fix disabled checks

pr fixes to state

added tests

linter fix

* fix alert on empty

* fixed storage class default

---------

Co-authored-by: Dipanshu Gupta <[email protected]>
  • Loading branch information
Gkrumbach07 and dpanshug authored Sep 17, 2024
1 parent e9c8a33 commit b5351a7
Show file tree
Hide file tree
Showing 17 changed files with 306 additions and 74 deletions.
19 changes: 18 additions & 1 deletion frontend/src/__mocks__/mockStorageClasses.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { K8sResourceListResult, StorageClassKind } from '~/k8sTypes';
import { K8sResourceListResult, StorageClassConfig, StorageClassKind } from '~/k8sTypes';

export type MockStorageClass = Omit<StorageClassKind, 'apiVersion' | 'kind'>;

Expand All @@ -19,6 +19,23 @@ export const mockStorageClassList = (
items: storageClasses,
});

export const buildMockStorageClass = (
mockStorageClass: MockStorageClass,
config: Partial<StorageClassConfig>,
): MockStorageClass => ({
...mockStorageClass,
metadata: {
...mockStorageClass.metadata,
annotations: {
...mockStorageClass.metadata.annotations,
'opendatahub.io/sc-config': JSON.stringify({
...JSON.parse(String(mockStorageClass.metadata.annotations?.['opendatahub.io/sc-config'])),
...config,
}),
},
},
});

export const mockStorageClasses: MockStorageClass[] = [
{
metadata: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,14 @@ class ClusterStorageModal extends Modal {
findPVSizePlusButton() {
return this.findPVSizeField().findByRole('button', { name: 'Plus' });
}

findStorageClassSelect() {
return this.find().findByTestId('storage-classes-selector');
}

findStorageClassDeprecatedWarning() {
return this.find().findByTestId('deprecated-storage-warning');
}
}

class ClusterStorage {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import { mockK8sResourceList, mockNotebookK8sResource, mockProjectK8sResource } from '~/__mocks__';
import {
buildMockStorageClass,
mockK8sResourceList,
mockNotebookK8sResource,
mockProjectK8sResource,
mockStorageClasses,
} from '~/__mocks__';
import { mockClusterSettings } from '~/__mocks__/mockClusterSettings';
import { mockPVCK8sResource } from '~/__mocks__/mockPVCK8sResource';
import { mockPodK8sResource } from '~/__mocks__/mockPodK8sResource';
Expand All @@ -17,6 +23,7 @@ import {
} from '~/__tests__/cypress/cypress/utils/models';
import { mock200Status } from '~/__mocks__/mockK8sStatus';
import { mockPrometheusQueryResponse } from '~/__mocks__/mockPrometheusQueryResponse';
import { storageClassesTable } from '~/__tests__/cypress/cypress/pages/storageClasses';

type HandlersProps = {
isEmpty?: boolean;
Expand Down Expand Up @@ -45,6 +52,8 @@ const initInterceptors = ({ isEmpty = false }: HandlersProps) => {
cy.interceptK8sList(NotebookModel, mockK8sResourceList([mockNotebookK8sResource({})]));
};

const [openshiftDefaultStorageClass, otherStorageClass] = mockStorageClasses;

describe('ClusterStorage', () => {
it('Empty state', () => {
initInterceptors({ isEmpty: true });
Expand All @@ -56,9 +65,20 @@ describe('ClusterStorage', () => {

it('Add cluster storage', () => {
initInterceptors({ isEmpty: true });
storageClassesTable.mockGetStorageClasses([
openshiftDefaultStorageClass,
buildMockStorageClass(otherStorageClass, { isEnabled: true }),
]);

clusterStorage.visit('test-project');
clusterStorage.findCreateButton().click();
addClusterStorageModal.findNameInput().fill('test-storage');

// default selected
addClusterStorageModal.find().findByText('openshift-default-sc').should('exist');

// select storage class
addClusterStorageModal.findStorageClassSelect().findSelectOption('Test SC 1').click();
addClusterStorageModal.findSubmitButton().should('be.enabled');
addClusterStorageModal.findDescriptionInput().fill('description');
addClusterStorageModal.findPVSizeMinusButton().click();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
mockProjectK8sResource,
mockRouteK8sResource,
mockSecretK8sResource,
mockStorageClassList,
} from '~/__mocks__';
import { mockConfigMap } from '~/__mocks__/mockConfigMap';
import { mockImageStreamK8sResource } from '~/__mocks__/mockImageStreamK8sResource';
Expand Down Expand Up @@ -60,6 +61,7 @@ const initIntercepts = ({
},
],
}: HandlersProps) => {
cy.interceptOdh('GET /api/k8s/apis/storage.k8s.io/v1/storageclasses', {}, mockStorageClassList());
cy.interceptOdh(
'GET /api/dsc/status',
mockDscStatus({
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
import type { MockStorageClass } from '~/__mocks__';
import { mockStorageClassList, mockStorageClasses } from '~/__mocks__';
import { buildMockStorageClass, mockStorageClassList, mockStorageClasses } from '~/__mocks__';
import { asProductAdminUser } from '~/__tests__/cypress/cypress/utils/mockUsers';
import { pageNotfound } from '~/__tests__/cypress/cypress/pages/pageNotFound';
import {
storageClassEditModal,
storageClassesPage,
storageClassesTable,
} from '~/__tests__/cypress/cypress/pages/storageClasses';
import type { StorageClassConfig } from '~/k8sTypes';

describe('Storage classes', () => {
it('shows "page not found" and does not show nav item as a non-admin user', () => {
Expand Down Expand Up @@ -153,20 +151,3 @@ describe('Storage classes', () => {
});
});
});

const buildMockStorageClass = (
mockStorageClass: MockStorageClass,
config: Partial<StorageClassConfig>,
) => ({
...mockStorageClass,
metadata: {
...mockStorageClass.metadata,
annotations: {
...mockStorageClass.metadata.annotations,
'opendatahub.io/sc-config': JSON.stringify({
...JSON.parse(String(mockStorageClass.metadata.annotations?.['opendatahub.io/sc-config'])),
...config,
}),
},
},
});
5 changes: 2 additions & 3 deletions frontend/src/api/k8s/pvcs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ export const assemblePvc = (
data: CreatingStorageObject,
namespace: string,
editName?: string,
storageClassName?: string,
): PersistentVolumeClaimKind => {
const {
nameDesc: { name: pvcName, description },
size,
storageClassName,
} = data;

const name = editName || translateDisplayNameForK8s(pvcName);
Expand Down Expand Up @@ -68,10 +68,9 @@ export const getDashboardPvcs = (projectName: string): Promise<PersistentVolumeC
export const createPvc = (
data: CreatingStorageObject,
namespace: string,
storageClassName?: string,
opts?: K8sAPIOptions,
): Promise<PersistentVolumeClaimKind> => {
const pvc = assemblePvc(data, namespace, undefined, storageClassName);
const pvc = assemblePvc(data, namespace);

return k8sCreateResource<PersistentVolumeClaimKind>(
applyK8sAPIOptions({ model: PVCModel, resource: pvc }, opts),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ import useRelatedNotebooks, {
import NotebookRestartAlert from '~/pages/projects/components/NotebookRestartAlert';
import useWillNotebooksRestart from '~/pages/projects/notebook/useWillNotebooksRestart';
import DashboardModalFooter from '~/concepts/dashboard/DashboardModalFooter';
import usePreferredStorageClass from '~/pages/projects/screens/spawner/storage/usePreferredStorageClass';
import { getDescriptionFromK8sResource, getDisplayNameFromK8sResource } from '~/concepts/k8s/utils';
import { SupportedArea, useIsAreaAvailable } from '~/concepts/areas';
import usePreferredStorageClass from '~/pages/projects/screens/spawner/storage/usePreferredStorageClass';
import useDefaultStorageClass from '~/pages/projects/screens/spawner/storage/useDefaultStorageClass';
import ExistingConnectedNotebooks from './ExistingConnectedNotebooks';

type AddStorageModalProps = {
Expand All @@ -23,6 +25,10 @@ type AddStorageModalProps = {
};

const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOpen, onClose }) => {
const isStorageClassesAvailable = useIsAreaAvailable(SupportedArea.STORAGE_CLASSES).status;
const preferredStorageClass = usePreferredStorageClass();
const defaultStorageClass = useDefaultStorageClass();

const [createData, setCreateData, resetData] = useCreateStorageObjectForNotebook(existingData);
const [actionInProgress, setActionInProgress] = React.useState(false);
const [error, setError] = React.useState<Error | undefined>();
Expand All @@ -40,7 +46,22 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
createData.forNotebook.name,
]);

const storageClass = usePreferredStorageClass();
React.useEffect(() => {
if (!existingData && isOpen) {
if (isStorageClassesAvailable) {
setCreateData('storageClassName', defaultStorageClass?.metadata.name);
} else {
setCreateData('storageClassName', preferredStorageClass?.metadata.name);
}
}
}, [
isStorageClassesAvailable,
defaultStorageClass,
preferredStorageClass,
existingData,
isOpen,
setCreateData,
]);

const onBeforeClose = (submitted: boolean) => {
onClose(submitted);
Expand All @@ -53,8 +74,13 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
const hasValidNotebookRelationship = createData.forNotebook.name
? !!createData.forNotebook.mountPath.value && !createData.forNotebook.mountPath.error
: true;

const storageClassSelected = isStorageClassesAvailable ? createData.storageClassName : true;
const canCreate =
!actionInProgress && createData.nameDesc.name.trim() && hasValidNotebookRelationship;
!actionInProgress &&
createData.nameDesc.name.trim() &&
hasValidNotebookRelationship &&
storageClassSelected;

const runPromiseActions = async (dryRun: boolean) => {
const {
Expand All @@ -66,7 +92,8 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
if (
getDisplayNameFromK8sResource(existingData) !== createData.nameDesc.name ||
getDescriptionFromK8sResource(existingData) !== createData.nameDesc.description ||
existingData.spec.resources.requests.storage !== createData.size
existingData.spec.resources.requests.storage !== createData.size ||
existingData.spec.storageClassName !== createData.storageClassName
) {
pvcPromises.push(updatePvc(createData, existingData, namespace, { dryRun }));
}
Expand All @@ -87,9 +114,7 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
}
return;
}
const createdPvc = await createPvc(createData, namespace, storageClass?.metadata.name, {
dryRun,
});
const createdPvc = await createPvc(createData, namespace, { dryRun });
if (notebookName) {
await attachNotebookPVC(notebookName, namespace, createdPvc.metadata.name, mountPath.value, {
dryRun,
Expand Down Expand Up @@ -145,6 +170,7 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
setData={(key, value) => setCreateData(key, value)}
currentSize={existingData?.status?.capacity?.storage}
autoFocusName
disableStorageClassSelect={!!existingData}
/>
</StackItem>
{createData.hasExistingNotebookConnections && (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {
import { useUser } from '~/redux/selectors';
import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext';
import { AppContext } from '~/app/AppContext';
import usePreferredStorageClass from '~/pages/projects/screens/spawner/storage/usePreferredStorageClass';
import { ProjectSectionID } from '~/pages/projects/screens/detail/types';
import { fireFormTrackingEvent } from '~/concepts/analyticsTracking/segmentIOUtils';
import {
Expand Down Expand Up @@ -57,7 +56,6 @@ const SpawnerFooter: React.FC<SpawnerFooterProps> = ({
},
} = React.useContext(AppContext);
const tolerationSettings = notebookController?.notebookTolerationSettings;
const storageClass = usePreferredStorageClass();
const {
notebooks: { data },
dataConnections: { data: existingDataConnections },
Expand Down Expand Up @@ -158,7 +156,6 @@ const SpawnerFooter: React.FC<SpawnerFooterProps> = ({
projectName,
editNotebook,
storageData,
storageClass?.metadata.name,
dryRun,
).catch(handleError);

Expand Down Expand Up @@ -242,11 +239,7 @@ const SpawnerFooter: React.FC<SpawnerFooterProps> = ({
? [dataConnection.existing]
: [];

const pvcDetails = await createPvcDataForNotebook(
projectName,
storageData,
storageClass?.metadata.name,
).catch(handleError);
const pvcDetails = await createPvcDataForNotebook(projectName, storageData).catch(handleError);
const envFrom = await createConfigMapsAndSecretsForNotebook(projectName, [
...envVariables,
...newDataConnection,
Expand Down
12 changes: 12 additions & 0 deletions frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import useNotebookAcceleratorProfile from '~/pages/projects/screens/detail/noteb
import { NotebookImageAvailability } from '~/pages/projects/screens/detail/notebooks/const';
import { getDisplayNameFromK8sResource } from '~/concepts/k8s/utils';
import useGenericObjectState from '~/utilities/useGenericObjectState';
import { SupportedArea, useIsAreaAvailable } from '~/concepts/areas';
import K8sNameDescriptionField, {
useK8sNameDescriptionFieldData,
} from '~/concepts/k8s/K8sNameDescriptionField/K8sNameDescriptionField';
Expand All @@ -47,6 +48,8 @@ import { useNotebookEnvVariables } from './environmentVariables/useNotebookEnvVa
import DataConnectionField from './dataConnection/DataConnectionField';
import { useNotebookDataConnection } from './dataConnection/useNotebookDataConnection';
import { useNotebookSizeState } from './useNotebookSizeState';
import useDefaultStorageClass from './storage/useDefaultStorageClass';
import usePreferredStorageClass from './storage/usePreferredStorageClass';

type SpawnerPageProps = {
existingNotebook?: NotebookKind;
Expand All @@ -70,10 +73,19 @@ const SpawnerPage: React.FC<SpawnerPageProps> = ({ existingNotebook }) => {
string[] | undefined
>();
const [storageDataWithoutDefault, setStorageData] = useStorageDataObject(existingNotebook);

const defaultStorageClass = useDefaultStorageClass();
const preferredStorageClass = usePreferredStorageClass();
const isStorageClassesAvailable = useIsAreaAvailable(SupportedArea.STORAGE_CLASSES).status;
const defaultStorageClassName = isStorageClassesAvailable
? defaultStorageClass?.metadata.name
: preferredStorageClass?.metadata.name;
const storageData = useMergeDefaultPVCName(
storageDataWithoutDefault,
k8sNameDescriptionData.data.name,
defaultStorageClassName,
);

const [envVariables, setEnvVariables] = useNotebookEnvVariables(existingNotebook);
const [dataConnectionData, setDataConnectionData] = useNotebookDataConnection(
dataConnections.data,
Expand Down
6 changes: 2 additions & 4 deletions frontend/src/pages/projects/screens/spawner/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,13 @@ import { fetchNotebookEnvVariables } from './environmentVariables/useNotebookEnv
export const createPvcDataForNotebook = async (
projectName: string,
storageData: StorageData,
storageClassName?: string,
): Promise<{ volumes: Volume[]; volumeMounts: VolumeMount[] }> => {
const { storageType } = storageData;

const { volumes, volumeMounts } = getVolumesByStorageData(storageData);

if (storageType === StorageType.NEW_PVC) {
const pvc = await createPvc(storageData.creating, projectName, storageClassName);
const pvc = await createPvc(storageData.creating, projectName);
const newPvcName = pvc.metadata.name;
volumes.push({ name: newPvcName, persistentVolumeClaim: { claimName: newPvcName } });
volumeMounts.push({ mountPath: ROOT_MOUNT_PATH, name: newPvcName });
Expand All @@ -49,7 +48,6 @@ export const replaceRootVolumesForNotebook = async (
projectName: string,
notebook: NotebookKind,
storageData: StorageData,
storageClassName?: string,
dryRun?: boolean,
): Promise<{ volumes: Volume[]; volumeMounts: VolumeMount[] }> => {
const {
Expand All @@ -70,7 +68,7 @@ export const replaceRootVolumesForNotebook = async (
};
replacedVolumeMount = { name: existingName, mountPath: ROOT_MOUNT_PATH };
} else {
const pvc = await createPvc(storageData.creating, projectName, storageClassName, { dryRun });
const pvc = await createPvc(storageData.creating, projectName, { dryRun });
const newPvcName = pvc.metadata.name;
replacedVolume = { name: newPvcName, persistentVolumeClaim: { claimName: newPvcName } };
replacedVolumeMount = { mountPath: ROOT_MOUNT_PATH, name: newPvcName };
Expand Down
5 changes: 4 additions & 1 deletion frontend/src/pages/projects/screens/spawner/spawnerUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { FAILED_PHASES, PENDING_PHASES, IMAGE_ANNOTATIONS } from './const';
export const useMergeDefaultPVCName = (
storageData: StorageData,
defaultPVCName: string,
defaultStorageClassName?: string,
): StorageData => {
const modifiedRef = React.useRef(false);

Expand All @@ -49,6 +50,7 @@ export const useMergeDefaultPVCName = (
...storageData.creating.nameDesc,
name: storageData.creating.nameDesc.name || defaultPVCName,
},
storageClassName: storageData.creating.storageClassName || defaultStorageClassName,
},
};
};
Expand Down Expand Up @@ -406,7 +408,8 @@ export const checkRequiredFieldsForNotebookStart = (
image.imageVersion
);

const newStorageFieldInvalid = storageType === StorageType.NEW_PVC && !creating.nameDesc.name;
const newStorageFieldInvalid =
storageType === StorageType.NEW_PVC && (!creating.nameDesc.name || !creating.storageClassName);
const existingStorageFieldInvalid = storageType === StorageType.EXISTING_PVC && !existing.storage;
const isStorageDataValid = !newStorageFieldInvalid && !existingStorageFieldInvalid;

Expand Down
Loading

0 comments on commit b5351a7

Please sign in to comment.