Fix workbench update corruption when removing cluster storage (#5264)

* Clear old volumes and volumeMounts in updateNotebook to prevent corrupted merges

* added unit test

* Enhance Volume type to include optional configMap property and update related tests for volume handling in updateNotebook

* Update Volume type to include defaultMode property and refactor volume extraction in updateNotebook tests

* remove product bug label, and bug tag, add minor test fix

Signed-off-by: dalthecow <dalcowboiz@gmail.com>

---------

Signed-off-by: dalthecow <dalcowboiz@gmail.com>
Co-authored-by: dalthecow <dalcowboiz@gmail.com>

Author: Gkrumbach07

frontend/src/api/k8s/__tests__/notebooks.spec.ts

@@ -4,6 +4,7 @@ import {
   k8sPatchResource,
   k8sListResource,
   k8sDeleteResource,
+  k8sUpdateResource,
   K8sStatus,
 } from '@openshift/dynamic-plugin-sdk-utils';
 import { NotebookKind } from '#~/k8sTypes';
@@ -30,6 +31,7 @@ import {
   getStopPatch,
   startPatch,
   mergePatchUpdateNotebook,
+  updateNotebook,
   restartNotebook,
   patchNotebookImage,
 } from '#~/api/k8s/notebooks';
@@ -50,6 +52,7 @@ jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({
   k8sGetResource: jest.fn(),
   k8sListResource: jest.fn(),
   k8sPatchResource: jest.fn(),
+  k8sUpdateResource: jest.fn(),
 }));
 
 jest.mock('#~/api/k8sUtils', () => ({
@@ -67,6 +70,7 @@ const k8sCreateResourceMock = jest.mocked(k8sCreateResource<NotebookKind>);
 const k8sGetResourceMock = jest.mocked(k8sGetResource<NotebookKind>);
 const k8sPatchResourceMock = jest.mocked(k8sPatchResource<NotebookKind>);
 const k8sListResourceMock = jest.mocked(k8sListResource<NotebookKind>);
+const k8sUpdateResourceMock = jest.mocked(k8sUpdateResource<NotebookKind>);
 const k8sMergePatchResourceMock = jest.mocked(k8sMergePatchResource<NotebookKind>);
 const k8sDeleteResourceMock = jest.mocked(k8sDeleteResource<NotebookKind, K8sStatus>);
 
@@ -1350,3 +1354,78 @@ describe('restartNotebook', () => {
     expect(renderResult).toStrictEqual(notebookMock);
   });
 });
+
+describe('updateNotebook', () => {
+  it('should clear old volumes and volumeMounts to prevent lodash merge corruption', async () => {
+    // Create existing notebook with multiple volumes including system volumes
+    const existingNotebook = mockNotebookK8sResource({ uid });
+
+    // Add volumes that would cause corruption if merged by index with lodash
+    existingNotebook.spec.template.spec.volumes = [
+      { name: 'storage-1', persistentVolumeClaim: { claimName: 'storage-1' } },
+      { name: 'storage-2', persistentVolumeClaim: { claimName: 'storage-2' } },
+      { name: 'shm', emptyDir: { medium: 'Memory' } },
+      {
+        name: 'trusted-ca',
+        configMap: {
+          name: 'workbench-trusted-ca-bundle',
+          optional: true,
+        },
+      },
+      { name: 'oauth-config', secret: { secretName: 'test-oauth-config', defaultMode: 420 } },
+    ];
+
+    existingNotebook.spec.template.spec.containers[0].volumeMounts = [
+      { name: 'storage-1', mountPath: '/opt/app-root/src' },
+      { name: 'storage-2', mountPath: '/data' },
+      { name: 'shm', mountPath: '/dev/shm' },
+      { name: 'trusted-ca', mountPath: '/etc/pki/tls/certs/ca-bundle.crt' },
+    ];
+
+    // Create new notebook data with only storage-1 (simulating removal of storage-2)
+    const newNotebookData = mockStartNotebookData({
+      notebookId: existingNotebook.metadata.name,
+    });
+
+    k8sUpdateResourceMock.mockResolvedValue(existingNotebook);
+
+    await updateNotebook(existingNotebook, newNotebookData, username);
+
+    // Get the resource that was passed to k8sUpdateResource
+    const { resource: mergedNotebook } = k8sUpdateResourceMock.mock.calls[0][0];
+
+    // Verify volumes are clean (no duplicates, no multiple source types per volume)
+    const { volumes } = mergedNotebook.spec.template.spec;
+    expect(volumes).toBeDefined();
+
+    // Check that shm volume was added and is clean (only has emptyDir, not merged with PVC)
+    const shmVolume = volumes?.find((v) => v.name === 'shm');
+    expect(shmVolume).toBeDefined();
+    expect(shmVolume).toEqual({
+      name: 'shm',
+      emptyDir: { medium: 'Memory' },
+    });
+    // Verify no corruption - should not have both emptyDir and persistentVolumeClaim
+    expect(shmVolume?.persistentVolumeClaim).toBeUndefined();
+    expect(shmVolume?.secret).toBeUndefined();
+    expect(shmVolume?.configMap).toBeUndefined();
+
+    // Verify no duplicate volume names
+    const volumeNames = volumes?.map((v) => v.name) || [];
+    const uniqueVolumeNames = new Set(volumeNames);
+    expect(volumeNames.length).toBe(uniqueVolumeNames.size);
+
+    // Verify volumeMounts are clean
+    const volumeMounts = mergedNotebook.spec.template.spec.containers[0].volumeMounts || [];
+
+    // Verify no duplicate mount paths
+    const mountPaths = volumeMounts.map((m) => m.mountPath);
+    const uniqueMountPaths = new Set(mountPaths);
+    expect(mountPaths.length).toBe(uniqueMountPaths.size);
+
+    // Verify shm volumeMount exists
+    const shmMount = volumeMounts.find((m) => m.name === 'shm');
+    expect(shmMount).toBeDefined();
+    expect(shmMount?.mountPath).toBe('/dev/shm');
+  });
+});

frontend/src/api/k8s/notebooks.ts

@@ -291,6 +291,11 @@ export const updateNotebook = (
   oldNotebook.spec.template.spec.nodeSelector = {};
   container.resources = {};
 
+  // Clear old volumes and volumeMounts to prevent lodash merge from
+  // merging them by array index, which creates corrupted volumes with multiple types
+  oldNotebook.spec.template.spec.volumes = [];
+  container.volumeMounts = [];
+
   return k8sUpdateResource<NotebookKind>(
     applyK8sAPIOptions(
       {

frontend/src/types.ts

@@ -631,6 +631,12 @@ export type Volume = {
   secret?: {
     secretName: string;
     optional?: boolean;
+    defaultMode?: number;
+  };
+  configMap?: {
+    name: string;
+    optional?: boolean;
+    defaultMode?: number;
   };
 };
 

packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchCreation.cy.ts

@@ -20,7 +20,7 @@ import {
   getImageStreamDisplayName,
 } from '../../../../utils/oc_commands/imageStreams';
 
-describe('[Product Bug: RHOAIENG-31579] Create, Delete and Edit - Workbench Tests', () => {
+describe('Create, Delete and Edit - Workbench Tests', () => {
   let editTestNamespace: string;
   let editedTestNamespace: string;
   let editedTestDescription: string;
@@ -71,15 +71,7 @@ describe('[Product Bug: RHOAIENG-31579] Create, Delete and Edit - Workbench Test
   it(
     'Create Workbench from the launcher page and verify that it is created successfully.',
     {
-      tags: [
-        '@Sanity',
-        '@SanitySet1',
-        '@ODS-1931',
-        '@ODS-2218',
-        '@Dashboard',
-        '@Workbenches',
-        '@Bug',
-      ],
+      tags: ['@Sanity', '@SanitySet1', '@ODS-1931', '@ODS-2218', '@Dashboard', '@Workbenches'],
     },
     () => {
       const workbenchName = editTestNamespace.replace('dsp-', '');
@@ -127,7 +119,8 @@ describe('[Product Bug: RHOAIENG-31579] Create, Delete and Edit - Workbench Test
             // Edit the workbench and update
             cy.step('Editing the workbench - both the Name and Description');
             notebookRow.findKebab().click();
-            notebookRow.findKebabAction('Edit workbench').click();
+            // If we click to edit the wb immediately after stopping we risk encountering a sync issue when editing the wb, delay remedies this
+            notebookRow.findKebabAction('Edit workbench').trigger('click', { delay: 2000 });
             createSpawnerPage.getNameInput().clear().type(editedTestNamespace);
             createSpawnerPage.getDescriptionInput().type(editedTestDescription);
             createSpawnerPage.findSubmitButton().click();
@@ -149,15 +142,7 @@ describe('[Product Bug: RHOAIENG-31579] Create, Delete and Edit - Workbench Test
   it(
     'Verify user can delete PV storage, data connection and workbench in a shared DS project',
     {
-      tags: [
-        '@Sanity',
-        '@SanitySet1',
-        '@ODS-1931',
-        '@ODS-2218',
-        '@Dashboard',
-        '@Workbenches',
-        '@Bug',
-      ],
+      tags: ['@Sanity', '@SanitySet1', '@ODS-1931', '@ODS-2218', '@Dashboard', '@Workbenches'],
     },
     () => {
       // Authentication and navigation