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;