Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Storage class dropdown select #3212

Merged
merged 4 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 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 @@
};

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 @@
createData.forNotebook.name,
]);

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

Check warning on line 54 in frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx#L53-L54

Added lines #L53 - L54 were not covered by tests
}
}
}, [
isStorageClassesAvailable,
defaultStorageClass,
preferredStorageClass,
existingData,
isOpen,
setCreateData,
]);

const onBeforeClose = (submitted: boolean) => {
onClose(submitted);
Expand All @@ -53,8 +74,13 @@
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 @@
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

Check warning on line 96 in frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx#L95-L96

Added lines #L95 - L96 were not covered by tests
) {
pvcPromises.push(updatePvc(createData, existingData, namespace, { dryRun }));
}
Expand All @@ -87,9 +114,7 @@
}
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 @@
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 { 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 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 @@
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;

Check warning on line 82 in frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx

View check run for this annotation

Codecov / codecov/patch

frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx#L82

Added line #L82 was not covered by tests
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
Loading