From 7ca7481b98583308b78a04f205e8627ab21ccdc2 Mon Sep 17 00:00:00 2001 From: Jeff Puzzo Date: Mon, 14 Oct 2024 10:11:50 -0400 Subject: [PATCH] [RHOAIENG-1101] Cannot remove additional storage from the workbench configuration --- .../src/__mocks__/mockStartNotebookData.ts | 22 ++- .../cypress/cypress/pages/workbench.ts | 50 +++-- .../tests/mocked/projects/workbench.cy.ts | 36 +--- .../screens/spawner/SpawnerFooter.tsx | 43 ++++- .../projects/screens/spawner/SpawnerPage.tsx | 68 +++++-- .../projects/screens/spawner/spawnerUtils.ts | 33 +--- .../storage/ClusterStorageDetachModal.tsx | 34 ++++ .../storage/ClusterStorageEditModal.tsx | 100 ++++++++++ .../spawner/storage/ClusterStorageTable.tsx | 175 ++++++++++++++++++ .../screens/spawner/storage/StorageField.tsx | 67 ------- .../screens/spawner/storage/constants.ts | 35 ++++ .../projects/screens/spawner/storage/utils.ts | 50 ++--- frontend/src/pages/projects/types.ts | 2 + 13 files changed, 507 insertions(+), 208 deletions(-) create mode 100644 frontend/src/pages/projects/screens/spawner/storage/ClusterStorageDetachModal.tsx create mode 100644 frontend/src/pages/projects/screens/spawner/storage/ClusterStorageEditModal.tsx create mode 100644 frontend/src/pages/projects/screens/spawner/storage/ClusterStorageTable.tsx delete mode 100644 frontend/src/pages/projects/screens/spawner/storage/StorageField.tsx create mode 100644 frontend/src/pages/projects/screens/spawner/storage/constants.ts diff --git a/frontend/src/__mocks__/mockStartNotebookData.ts b/frontend/src/__mocks__/mockStartNotebookData.ts index b01f705ea3..5e55cbaee5 100644 --- a/frontend/src/__mocks__/mockStartNotebookData.ts +++ b/frontend/src/__mocks__/mockStartNotebookData.ts @@ -94,18 +94,20 @@ export const mockStartNotebookData = ({ }, }); -export const mockStorageData: StorageData = { - storageType: StorageType.NEW_PVC, - creating: { - nameDesc: { - name: 'test-pvc', - description: '', +export const mockStorageData: StorageData[] = [ + { + storageType: StorageType.NEW_PVC, + creating: { + nameDesc: { + name: 'test-pvc', + description: '', + }, + size: '20Gi', + storageClassName: 'gp2-csi', }, - size: '20Gi', - storageClassName: 'gp2-csi', + existing: { storage: '' }, }, - existing: { storage: '' }, -}; +]; export const mockDataConnectionData: DataConnectionData = { type: 'creating', diff --git a/frontend/src/__tests__/cypress/cypress/pages/workbench.ts b/frontend/src/__tests__/cypress/cypress/pages/workbench.ts index dee82e7e10..096628e781 100644 --- a/frontend/src/__tests__/cypress/cypress/pages/workbench.ts +++ b/frontend/src/__tests__/cypress/cypress/pages/workbench.ts @@ -158,6 +158,39 @@ class NotebookRow extends TableRow { } } +class StorageTableRow extends TableRow { + private findByDataLabel(label: string) { + return this.find().find(`[data-label="${label}"]`); + } + + findNameValue() { + return this.findByDataLabel('Name'); + } + + findStorageSizeValue() { + return this.findByDataLabel('Storage size'); + } + + findMountPathValue() { + return this.findByDataLabel('Mount path'); + } +} + +class StorageTable { + find() { + return cy.findByTestId('cluster-storage-table'); + } + + getRowById(id: number) { + return new StorageTableRow( + () => + this.find().findByTestId(['cluster-storage-table-row', id]) as unknown as Cypress.Chainable< + JQuery + >, + ); + } +} + class CreateSpawnerPage { k8sNameDescription = new K8sNameDescriptionField('workbench'); @@ -166,16 +199,8 @@ class CreateSpawnerPage { return this; } - findNewStorageRadio() { - return cy.findByTestId('persistent-new-storage-type-radio'); - } - - findExistingStorageRadio() { - return cy.findByTestId('persistent-existing-storage-type-radio'); - } - - findClusterStorageInput() { - return cy.findByTestId('create-new-storage-name'); + getStorageTable() { + return new StorageTable(); } private findPVSizeField() { @@ -306,11 +331,6 @@ class EditSpawnerPage extends CreateSpawnerPage { cy.testA11y(); } - shouldHavePersistentStorage(name: string) { - cy.findByTestId('persistent-storage-group').find('input').should('have.value', name); - return this; - } - shouldHaveNotebookImageSelectInput(name: string) { cy.findByTestId('workbench-image-stream-selection').contains(name).should('exist'); return this; diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts index c14950243c..91a0c9e1d6 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts @@ -271,22 +271,11 @@ describe('Workbench page', () => { environmentVariableField.uploadConfigYaml(configYamlPath); environmentVariableField.findRemoveEnvironmentVariableButton().should('be.enabled'); - //cluster storage - createSpawnerPage.shouldHaveClusterStorageAlert(); - - //add new cluster storage - createSpawnerPage.findNewStorageRadio().click(); - createSpawnerPage.findClusterStorageInput().should('have.value', 'test-project'); - createSpawnerPage.findClusterStorageDescriptionInput().fill('test-description'); - createSpawnerPage.findPVSizeMinusButton().click(); - createSpawnerPage.findPVSizeInput().should('have.value', '19'); - createSpawnerPage.findPVSizePlusButton().click(); - createSpawnerPage.findPVSizeInput().should('have.value', '20'); - createSpawnerPage.selectPVSize('Mi'); - - //add existing cluster storage - createSpawnerPage.findExistingStorageRadio().click(); - createSpawnerPage.selectExistingPersistentStorage('Test Storage'); + // cluster storage + const storageTableRow = createSpawnerPage.getStorageTable().getRowById(0); + storageTableRow.findNameValue().should('have.text', 'test-project-storage'); + storageTableRow.findStorageSizeValue().should('have.text', 'Max 20Gi'); + storageTableRow.findMountPathValue().should('have.text', '/opt/apt-root/src/'); //add data connection createSpawnerPage.findDataConnectionCheckbox().check(); @@ -323,15 +312,6 @@ describe('Workbench page', () => { name: 'test-project', namespace: 'test-project', }, - spec: { - template: { - spec: { - volumes: [ - { name: 'test-storage', persistentVolumeClaim: { claimName: 'test-storage' } }, - ], - }, - }, - }, }); }); verifyRelativeURL('/projects/test-project?section=workbenches'); @@ -570,7 +550,11 @@ describe('Workbench page', () => { editSpawnerPage.k8sNameDescription.findDisplayNameInput().should('have.value', 'Test Notebook'); editSpawnerPage.shouldHaveNotebookImageSelectInput('Test Image'); editSpawnerPage.shouldHaveContainerSizeInput('Small'); - editSpawnerPage.shouldHavePersistentStorage('Test Storage'); + editSpawnerPage + .getStorageTable() + .getRowById(0) + .findNameValue() + .should('have.text', 'test-notebook-Test Notebook'); editSpawnerPage.findSubmitButton().should('be.enabled'); editSpawnerPage.k8sNameDescription.findDisplayNameInput().fill('Updated Notebook'); diff --git a/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx b/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx index c693e944d1..7fbee866b6 100644 --- a/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx +++ b/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx @@ -33,10 +33,11 @@ import { } from './service'; import { checkRequiredFieldsForNotebookStart } from './spawnerUtils'; import { getNotebookDataConnection } from './dataConnection/useNotebookDataConnection'; +import { defaultClusterStorage } from './storage/constants'; type SpawnerFooterProps = { startNotebookData: StartNotebookData; - storageData: StorageData; + storageData: StorageData[]; envVariables: EnvVariable[]; dataConnection: DataConnectionData; canEnablePipelines: boolean; @@ -83,6 +84,11 @@ const SpawnerFooter: React.FC = ({ editNotebook, existingDataConnections, ); + const rootPathStorageData = + storageData.find( + (formData) => formData.creating.mountPath === defaultClusterStorage.mountPath, + ) || storageData[0]; + const afterStart = (name: string, type: 'created' | 'updated') => { const { selectedAcceleratorProfile, notebookSize, image } = startNotebookData; const tep: FormTrackingEventProperties = { @@ -103,8 +109,8 @@ const SpawnerFooter: React.FC = ({ imageName: image.imageStream?.metadata.name, projectName, notebookName: name, - storageType: storageData.storageType, - storageDataSize: storageData.creating.size, + storageType: rootPathStorageData.storageType, + storageDataSize: rootPathStorageData.creating.size, dataConnectionType: dataConnection.creating?.type?.toString(), dataConnectionCategory: dataConnection.creating?.values?.category?.toString(), dataConnectionEnabled: dataConnection.enabled, @@ -137,7 +143,7 @@ const SpawnerFooter: React.FC = ({ const pvcDetails = await replaceRootVolumesForNotebook( projectName, editNotebook, - storageData, + rootPathStorageData, dryRun, ); @@ -193,7 +199,34 @@ const SpawnerFooter: React.FC = ({ dataConnection.enabled && dataConnection.type === 'existing' && dataConnection.existing ? [dataConnection.existing] : []; - const pvcDetails = await createPvcDataForNotebook(projectName, storageData, dryRun); + + const createPvcRequests = storageData.map((pvcData) => + createPvcDataForNotebook(projectName, pvcData, dryRun), + ); + + const pvcResponses = await Promise.all(createPvcRequests); + const pvcDetails = pvcResponses.reduce( + (acc, response) => { + if (response.volumes.length) { + acc.volumes = acc.volumes.concat(response.volumes); + } else { + acc.volumes = response.volumes; + } + + if (response.volumeMounts.length) { + acc.volumeMounts = acc.volumeMounts.concat(response.volumeMounts); + } else { + acc.volumeMounts = response.volumeMounts; + } + + return acc; + }, + { + volumes: [], + volumeMounts: [], + }, + ); + const envFrom = await createConfigMapsAndSecretsForNotebook( projectName, [...envVariables, ...newDataConnection], diff --git a/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx b/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx index 9fc14141c0..230073423b 100644 --- a/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx +++ b/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx @@ -1,9 +1,11 @@ import * as React from 'react'; import { Link } from 'react-router-dom'; import { - Alert, Breadcrumb, BreadcrumbItem, + Button, + Flex, + FlexItem, Form, FormSection, PageSection, @@ -31,21 +33,23 @@ import K8sNameDescriptionField, { useK8sNameDescriptionFieldData, } from '~/concepts/k8s/K8sNameDescriptionField/K8sNameDescriptionField'; import { LimitNameResourceType } from '~/concepts/k8s/K8sNameDescriptionField/utils'; +import { StorageData, StorageType } from '~/pages/projects/types'; import { SpawnerPageSectionID } from './types'; import { ScrollableSelectorID, SpawnerPageSectionTitles } from './const'; import SpawnerFooter from './SpawnerFooter'; import ImageSelectorField from './imageSelector/ImageSelectorField'; import ContainerSizeSelector from './deploymentSize/ContainerSizeSelector'; -import StorageField from './storage/StorageField'; import EnvironmentVariables from './environmentVariables/EnvironmentVariables'; -import { useStorageDataObject } from './storage/utils'; -import { getCompatibleAcceleratorIdentifiers, useMergeDefaultPVCName } from './spawnerUtils'; +import { getCompatibleAcceleratorIdentifiers, getRootVolumeName } from './spawnerUtils'; import { useNotebookEnvVariables } from './environmentVariables/useNotebookEnvVariables'; import DataConnectionField from './dataConnection/DataConnectionField'; import { useNotebookDataConnection } from './dataConnection/useNotebookDataConnection'; import { useNotebookSizeState } from './useNotebookSizeState'; import useDefaultStorageClass from './storage/useDefaultStorageClass'; import usePreferredStorageClass from './storage/usePreferredStorageClass'; +import { ClusterStorageTable } from './storage/ClusterStorageTable'; +import useDefaultPvcSize from './storage/useDefaultPvcSize'; +import { defaultClusterStorage } from './storage/constants'; type SpawnerPageProps = { existingNotebook?: NotebookKind; @@ -68,19 +72,30 @@ const SpawnerPage: React.FC = ({ existingNotebook }) => { const [supportedAcceleratorProfiles, setSupportedAcceleratorProfiles] = React.useState< 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 defaultNotebookSize = useDefaultPvcSize(); + const [storageData, setStorageData] = React.useState([ + { + storageType: existingNotebook ? StorageType.EXISTING_PVC : StorageType.NEW_PVC, + creating: { + nameDesc: { + name: k8sNameDescriptionData.data.name || defaultClusterStorage.name, + description: defaultClusterStorage.description, + }, + size: defaultClusterStorage.size || defaultNotebookSize, + storageClassName: defaultStorageClassName || defaultClusterStorage.storageClassName, + mountPath: defaultClusterStorage.mountPath, + }, + existing: { + storage: getRootVolumeName(existingNotebook), + }, + }, + ]); const [envVariables, setEnvVariables] = useNotebookEnvVariables(existingNotebook); const [dataConnectionData, setDataConnectionData] = useNotebookDataConnection( @@ -204,19 +219,32 @@ const SpawnerPage: React.FC = ({ existingNotebook }) => { + + {SpawnerPageSectionTitles[SpawnerPageSectionID.CLUSTER_STORAGE]} + + + + + + + } id={SpawnerPageSectionID.CLUSTER_STORAGE} aria-label={SpawnerPageSectionTitles[SpawnerPageSectionID.CLUSTER_STORAGE]} > - ({ ...formData, id: index }))} + setStorageData={setStorageData} + workbenchName={k8sNameDescriptionData.data.k8sName.value} /> - { - const modifiedRef = React.useRef(false); - - if (modifiedRef.current || storageData.creating.nameDesc.name) { - modifiedRef.current = true; - return storageData; - } - - return { - ...storageData, - creating: { - ...storageData.creating, - nameDesc: { - ...storageData.creating.nameDesc, - name: storageData.creating.nameDesc.name || defaultPVCName, - }, - storageClassName: storageData.creating.storageClassName || defaultStorageClassName, - }, - }; -}; - export const getVersion = (version?: string | number, prefix?: string): string => { if (!version) { return ''; @@ -395,12 +369,11 @@ export const isEnvVariableDataValid = (envVariables: EnvVariable[]): boolean => export const checkRequiredFieldsForNotebookStart = ( startNotebookData: StartNotebookData, - storageData: StorageData, + storageData: StorageData[], envVariables: EnvVariable[], dataConnection: DataConnectionData, ): boolean => { const { projectName, notebookData, image } = startNotebookData; - const { storageType, creating, existing } = storageData; const isNotebookDataValid = !!( projectName && isK8sNameDescriptionDataValid(notebookData) && @@ -408,9 +381,7 @@ export const checkRequiredFieldsForNotebookStart = ( image.imageVersion ); - const newStorageFieldInvalid = storageType === StorageType.NEW_PVC && !creating.nameDesc.name; - const existingStorageFieldInvalid = storageType === StorageType.EXISTING_PVC && !existing.storage; - const isStorageDataValid = !newStorageFieldInvalid && !existingStorageFieldInvalid; + const isStorageDataValid = storageData.length > 0; const newDataConnectionInvalid = dataConnection.type === 'creating' && diff --git a/frontend/src/pages/projects/screens/spawner/storage/ClusterStorageDetachModal.tsx b/frontend/src/pages/projects/screens/spawner/storage/ClusterStorageDetachModal.tsx new file mode 100644 index 0000000000..3675b65c61 --- /dev/null +++ b/frontend/src/pages/projects/screens/spawner/storage/ClusterStorageDetachModal.tsx @@ -0,0 +1,34 @@ +import React from 'react'; +import { Modal, ModalVariant, Button } from '@patternfly/react-core'; + +interface ClusterStorageDetachModalProps { + storageName: string; + onConfirm: () => void; + onClose: () => void; + isDestructive?: boolean; +} + +export const ClusterStorageDetachModal: React.FC = ({ + storageName, + isDestructive, + onConfirm, + onClose, +}) => ( + + Detach + , + , + ]} + > + The {storageName} storage and all of its resources will be detached from the workbench. + +); diff --git a/frontend/src/pages/projects/screens/spawner/storage/ClusterStorageEditModal.tsx b/frontend/src/pages/projects/screens/spawner/storage/ClusterStorageEditModal.tsx new file mode 100644 index 0000000000..81ad1643ee --- /dev/null +++ b/frontend/src/pages/projects/screens/spawner/storage/ClusterStorageEditModal.tsx @@ -0,0 +1,100 @@ +import React from 'react'; + +import { Form, FormGroup, Modal, ModalVariant, TextArea, TextInput } from '@patternfly/react-core'; + +import DashboardModalFooter from '~/concepts/dashboard/DashboardModalFooter'; +import PVSizeField from '~/pages/projects/components/PVSizeField'; +import { StorageData } from '~/pages/projects/types'; +import StorageClassSelect from './StorageClassSelect'; + +interface ClusterStorageEditModalProps { + storageData: StorageData; + onUpdate: (storageData: StorageData) => void; + onClose: () => void; +} + +export const ClusterStorageEditModal: React.FC = ({ + storageData, + onUpdate, + onClose, +}) => { + const [name, setName] = React.useState(storageData.creating.nameDesc.name); + const [description, setDescription] = React.useState(storageData.creating.nameDesc.description); + const [size, setSize] = React.useState(storageData.creating.size); + const [storageClassName, setStorageClassName] = React.useState( + storageData.creating.storageClassName, + ); + const [mountPath, setMountPath] = React.useState(storageData.creating.mountPath); + + const onSubmit = () => { + onUpdate({ + ...storageData, + creating: { nameDesc: { name, description }, size, storageClassName, mountPath }, + }); + onClose(); + }; + + return ( + + } + data-testid="edit-cluster-storage-modal" + > +
+ + setName(value)} + id="display-name" + data-testid="display-name-input" + /> + + + +