allow for pvc to be added to multiple workbenches (#3231)

* allow for pvc to be added to multiple workbenches fix: Fix issue with adding cluster storage in tests fix roles added test * fix lint * fix removabale notebooks logic * fixed perms and iscompact
opendatahub-io · Sep 23, 2024 · 84628b2 · 84628b2
1 parent 854cb3c
commit 84628b2
Show file tree

Hide file tree

Showing 12 changed files with 203 additions and 61 deletions.
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
@@ -162,6 +162,162 @@ describe('ClusterStorage', () => {
     });
   });
 
+  it('Add cluster storage with multiple workbench connections', () => {
+    // one notebook already connected to a PVC
+    const testNotebook = mockNotebookK8sResource({
+      displayName: 'Test Notebook',
+      name: 'test-notebook',
+      opts: {
+        spec: {
+          template: {
+            spec: {
+              volumes: [
+                { name: 'existing-pvc', persistentVolumeClaim: { claimName: 'existing-pvc' } },
+              ],
+              containers: [
+                {
+                  volumeMounts: [{ name: 'existing-pvc', mountPath: '/' }],
+                },
+              ],
+            },
+          },
+        },
+      },
+    });
+
+    // another notebook not connected to PVC
+    const anotherNotebook = mockNotebookK8sResource({
+      displayName: 'Another Notebook',
+      name: 'another-notebook',
+    });
+
+    initInterceptors({});
+    cy.interceptK8sList(NotebookModel, mockK8sResourceList([testNotebook, anotherNotebook]));
+
+    cy.interceptK8sList(
+      { model: PVCModel, ns: 'test-project' },
+      mockK8sResourceList([
+        mockPVCK8sResource({ name: 'existing-pvc', displayName: 'Existing PVC' }),
+      ]),
+    );
+
+    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');
+
+    cy.interceptK8s('PATCH', NotebookModel, anotherNotebook).as('updateClusterStorage');
+
+    updateClusterStorageModal.findSubmitButton().click();
+    cy.wait('@updateClusterStorage').then((interception) => {
+      expect(interception.request.url).to.include('?dryRun=All');
+      expect(interception.request.body).to.eql([
+        {
+          op: 'add',
+          path: '/spec/template/spec/volumes/-',
+          value: { name: 'existing-pvc', persistentVolumeClaim: { claimName: 'existing-pvc' } },
+        },
+        {
+          op: 'add',
+          path: '/spec/template/spec/containers/0/volumeMounts/-',
+          value: { name: 'existing-pvc', mountPath: '/opt/app-root/src/new-data' },
+        },
+      ]);
+    });
+
+    cy.wait('@updateClusterStorage').then((interception) => {
+      expect(interception.request.url).not.to.include('?dryRun=All');
+    });
+
+    cy.get('@updateClusterStorage.all').then((interceptions) => {
+      expect(interceptions).to.have.length(2);
+    });
+  });
+
+  it('Add cluster storage with multiple workbench connections', () => {
+    // one notebook already connected to a PVC
+    const testNotebook = mockNotebookK8sResource({
+      displayName: 'Test Notebook',
+      name: 'test-notebook',
+      opts: {
+        spec: {
+          template: {
+            spec: {
+              volumes: [
+                { name: 'existing-pvc', persistentVolumeClaim: { claimName: 'existing-pvc' } },
+              ],
+              containers: [
+                {
+                  volumeMounts: [{ name: 'existing-pvc', mountPath: '/' }],
+                },
+              ],
+            },
+          },
+        },
+      },
+    });
+
+    // another notebook not connected to PVC
+    const anotherNotebook = mockNotebookK8sResource({
+      displayName: 'Another Notebook',
+      name: 'another-notebook',
+    });
+
+    initInterceptors({});
+    cy.interceptK8sList(NotebookModel, mockK8sResourceList([testNotebook, anotherNotebook]));
+
+    cy.interceptK8sList(
+      { model: PVCModel, ns: 'test-project' },
+      mockK8sResourceList([
+        mockPVCK8sResource({ name: 'existing-pvc', displayName: 'Existing PVC' }),
+      ]),
+    );
+
+    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');
+
+    cy.interceptK8s('PATCH', NotebookModel, anotherNotebook).as('updateClusterStorage');
+
+    updateClusterStorageModal.findSubmitButton().click();
+    cy.wait('@updateClusterStorage').then((interception) => {
+      expect(interception.request.url).to.include('?dryRun=All');
+      expect(interception.request.body).to.eql([
+        {
+          op: 'add',
+          path: '/spec/template/spec/volumes/-',
+          value: { name: 'existing-pvc', persistentVolumeClaim: { claimName: 'existing-pvc' } },
+        },
+        {
+          op: 'add',
+          path: '/spec/template/spec/containers/0/volumeMounts/-',
+          value: { name: 'existing-pvc', mountPath: '/opt/app-root/src/new-data' },
+        },
+      ]);
+    });
+
+    cy.wait('@updateClusterStorage').then((interception) => {
+      expect(interception.request.url).not.to.include('?dryRun=All');
+    });
+
+    cy.get('@updateClusterStorage.all').then((interceptions) => {
+      expect(interceptions).to.have.length(2);
+    });
+  });
+
   it('list cluster storage and Table sorting', () => {
     cy.interceptOdh(
       'GET /api/config',

diff --git a/frontend/src/pages/projects/notebook/ConnectedNotebookNames.tsx b/frontend/src/pages/projects/notebook/ConnectedNotebookNames.tsx
@@ -1,5 +1,5 @@
 import * as React from 'react';
-import { Badge, List, ListItem, Spinner } from '@patternfly/react-core';
+import { Label, LabelGroup, Spinner } from '@patternfly/react-core';
 import { getDisplayNameFromK8sResource } from '~/concepts/k8s/utils';
 import useRelatedNotebooks, { ConnectedNotebookContext } from './useRelatedNotebooks';
 
@@ -32,13 +32,13 @@ const ConnectedNotebookNames: React.FC<ConnectedNotebookNamesProps> = ({
   }
 
   return (
-    <List isPlain>
+    <LabelGroup>
       {connectedNotebooks.map((notebook) => (
-        <ListItem key={notebook.metadata.uid}>
-          <Badge isRead>{getDisplayNameFromK8sResource(notebook)}</Badge>
-        </ListItem>
+        <Label isCompact key={notebook.metadata.uid}>
+          {getDisplayNameFromK8sResource(notebook)}
+        </Label>
       ))}
-    </List>
+    </LabelGroup>
   );
 };
 

diff --git a/frontend/src/pages/projects/notebook/StorageNotebookConnections.tsx b/frontend/src/pages/projects/notebook/StorageNotebookConnections.tsx
@@ -3,19 +3,20 @@ 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;
-  isDisabled: boolean;
+  connectedNotebooks: NotebookKind[];
 };
 
 const StorageNotebookConnections: React.FC<StorageNotebookConnectionsProps> = ({
   forNotebookData,
   setForNotebookData,
-  isDisabled,
+  connectedNotebooks,
 }) => {
   const {
     notebooks: { data, loaded, error },
@@ -34,12 +35,17 @@ const StorageNotebookConnections: React.FC<StorageNotebookConnectionsProps> = ({
     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 (
     <>
       <ConnectedNotebookField
-        isDisabled={isDisabled}
+        isDisabled={availableNotebooks.length === 0}
         loaded={loaded}
-        notebooks={notebooks}
+        notebooks={availableNotebooks}
         selections={[forNotebookData.name]}
         onSelect={(selectionItems) => {
           const selection = selectionItems[0];

diff --git a/frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx b/frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx
@@ -34,13 +34,18 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
   const [error, setError] = React.useState<Error | undefined>();
   const { currentProject } = React.useContext(ProjectDetailsContext);
   const namespace = currentProject.metadata.name;
-  const {
-    notebooks: connectedNotebooks,
-    loaded: notebookLoaded,
-    error: notebookError,
-  } = useRelatedNotebooks(ConnectedNotebookContext.EXISTING_PVC, existingData?.metadata.name);
+  const { notebooks: connectedNotebooks } = useRelatedNotebooks(
+    ConnectedNotebookContext.EXISTING_PVC,
+    existingData?.metadata.name,
+  );
   const [removedNotebooks, setRemovedNotebooks] = React.useState<string[]>([]);
 
+  const {
+    notebooks: removableNotebooks,
+    loaded: removableNotebookLoaded,
+    error: removableNotebookError,
+  } = useRelatedNotebooks(ConnectedNotebookContext.REMOVABLE_PVC, existingData?.metadata.name);
+
   const restartNotebooks = useWillNotebooksRestart([
     ...removedNotebooks,
     createData.forNotebook.name,
@@ -75,12 +80,8 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
     ? !!createData.forNotebook.mountPath.value && !createData.forNotebook.mountPath.error
     : true;
 
-  const storageClassSelected = isStorageClassesAvailable ? createData.storageClassName : true;
   const canCreate =
-    !actionInProgress &&
-    createData.nameDesc.name.trim() &&
-    hasValidNotebookRelationship &&
-    storageClassSelected;
+    !actionInProgress && createData.nameDesc.name.trim() && hasValidNotebookRelationship;
 
   const runPromiseActions = async (dryRun: boolean) => {
     const {
@@ -176,12 +177,12 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
           {createData.hasExistingNotebookConnections && (
             <StackItem>
               <ExistingConnectedNotebooks
-                connectedNotebooks={connectedNotebooks}
+                connectedNotebooks={removableNotebooks}
                 onNotebookRemove={(notebook: NotebookKind) =>
                   setRemovedNotebooks([...removedNotebooks, notebook.metadata.name])
                 }
-                loaded={notebookLoaded}
-                error={notebookError}
+                loaded={removableNotebookLoaded}
+                error={removableNotebookError}
               />
             </StackItem>
           )}
@@ -191,7 +192,7 @@ const ManageStorageModal: React.FC<AddStorageModalProps> = ({ existingData, isOp
                 setCreateData('forNotebook', forNotebookData);
               }}
               forNotebookData={createData.forNotebook}
-              isDisabled={connectedNotebooks.length !== 0 && removedNotebooks.length === 0}
+              connectedNotebooks={connectedNotebooks}
             />
           </StackItem>
           {restartNotebooks.length !== 0 && (

diff --git a/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx b/frontend/src/pages/projects/screens/spawner/SpawnerPage.tsx
@@ -39,11 +39,7 @@ import ContainerSizeSelector from './deploymentSize/ContainerSizeSelector';
 import StorageField from './storage/StorageField';
 import EnvironmentVariables from './environmentVariables/EnvironmentVariables';
 import { useStorageDataObject } from './storage/utils';
-import {
-  getCompatibleAcceleratorIdentifiers,
-  getRootVolumeName,
-  useMergeDefaultPVCName,
-} from './spawnerUtils';
+import { getCompatibleAcceleratorIdentifiers, useMergeDefaultPVCName } from './spawnerUtils';
 import { useNotebookEnvVariables } from './environmentVariables/useNotebookEnvVariables';
 import DataConnectionField from './dataConnection/DataConnectionField';
 import { useNotebookDataConnection } from './dataConnection/useNotebookDataConnection';
@@ -220,11 +216,7 @@ const SpawnerPage: React.FC<SpawnerPageProps> = ({ existingNotebook }) => {
                 isInline
                 title="Cluster storage will mount to /"
               />
-              <StorageField
-                storageData={storageData}
-                setStorageData={setStorageData}
-                editStorage={getRootVolumeName(existingNotebook)}
-              />
+              <StorageField storageData={storageData} setStorageData={setStorageData} />
             </FormSection>
             <FormSection
               title={SpawnerPageSectionTitles[SpawnerPageSectionID.DATA_CONNECTIONS]}

diff --git a/frontend/src/pages/projects/screens/spawner/spawnerUtils.ts b/frontend/src/pages/projects/screens/spawner/spawnerUtils.ts
@@ -408,8 +408,7 @@ export const checkRequiredFieldsForNotebookStart = (
     image.imageVersion
   );
 
-  const newStorageFieldInvalid =
-    storageType === StorageType.NEW_PVC && (!creating.nameDesc.name || !creating.storageClassName);
+  const newStorageFieldInvalid = storageType === StorageType.NEW_PVC && !creating.nameDesc.name;
   const existingStorageFieldInvalid = storageType === StorageType.EXISTING_PVC && !existing.storage;
   const isStorageDataValid = !newStorageFieldInvalid && !existingStorageFieldInvalid;
 

diff --git a/frontend/src/pages/projects/screens/spawner/storage/AddExistingStorageField.tsx b/frontend/src/pages/projects/screens/spawner/storage/AddExistingStorageField.tsx
@@ -4,32 +4,23 @@ import { ExistingStorageObject } from '~/pages/projects/types';
 import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext';
 import { getDisplayNameFromK8sResource } from '~/concepts/k8s/utils';
 import TypeaheadSelect from '~/components/TypeaheadSelect';
-import useAvailablePvcs from './useAvailablePvcs';
+import useProjectPvcs from '~/pages/projects/screens/detail/storage/useProjectPvcs';
 
 type AddExistingStorageFieldProps = {
   data: ExistingStorageObject;
   setData: (data: ExistingStorageObject) => void;
-  editStorage?: string;
   selectDirection?: 'up' | 'down';
   menuAppendTo?: HTMLElement;
 };
 
 const AddExistingStorageField: React.FC<AddExistingStorageFieldProps> = ({
   data,
   setData,
-  editStorage,
   selectDirection,
   menuAppendTo,
 }) => {
-  const {
-    currentProject,
-    notebooks: { data: allNotebooks },
-  } = React.useContext(ProjectDetailsContext);
-  const [storages, loaded, loadError] = useAvailablePvcs(
-    currentProject.metadata.name,
-    allNotebooks,
-    editStorage,
-  );
+  const { currentProject } = React.useContext(ProjectDetailsContext);
+  const [storages, loaded, loadError] = useProjectPvcs(currentProject.metadata.name);
 
   const selectOptions = React.useMemo(
     () =>

diff --git a/frontend/src/pages/projects/screens/spawner/storage/StorageClassSelect.tsx b/frontend/src/pages/projects/screens/spawner/storage/StorageClassSelect.tsx
@@ -77,7 +77,7 @@ const StorageClassSelect: React.FC<StorageClassSelectProps> = ({
   });
 
   return (
-    <FormGroup label="Storage class" fieldId="storage-class" isRequired>
+    <FormGroup label="Storage class" fieldId="storage-class">
       <SimpleSelect
         dataTestId="storage-classes-selector"
         id="storage-classes-selector"

diff --git a/frontend/src/pages/projects/screens/spawner/storage/StorageField.tsx b/frontend/src/pages/projects/screens/spawner/storage/StorageField.tsx
@@ -8,10 +8,9 @@ import AddExistingStorageField from './AddExistingStorageField';
 type StorageFieldType = {
   storageData: StorageData;
   setStorageData: UpdateObjectAtPropAndValue<StorageData>;
-  editStorage: string;
 };
 
-const StorageField: React.FC<StorageFieldType> = ({ storageData, setStorageData, editStorage }) => {
+const StorageField: React.FC<StorageFieldType> = ({ storageData, setStorageData }) => {
   const { storageType, creating, existing } = storageData;
 
   return (
@@ -53,7 +52,6 @@ const StorageField: React.FC<StorageFieldType> = ({ storageData, setStorageData,
                 <AddExistingStorageField
                   data={existing}
                   setData={(data) => setStorageData('existing', data)}
-                  editStorage={editStorage}
                   selectDirection="up"
                   menuAppendTo={getDashboardMainContainer()}
                 />