From 3af201e07743e759a55138aaf42edc5b8504705e Mon Sep 17 00:00:00 2001 From: Juntao Wang Date: Fri, 6 Sep 2024 16:21:08 -0400 Subject: [PATCH] Improve update call in editing workbench --- backend/src/routes/api/k8s/index.ts | 1 + backend/src/routes/api/k8s/pass-through.ts | 2 + backend/src/server.ts | 15 ++ .../src/__mocks__/mockStartNotebookData.ts | 8 +- .../tests/mocked/projects/workbench.cy.ts | 10 +- .../src/api/k8s/__tests__/notebooks.spec.ts | 70 ++++---- frontend/src/api/k8s/notebooks.ts | 18 +- frontend/src/api/k8sUtils.ts | 31 ++++ .../screens/spawner/SpawnerFooter.tsx | 159 +++++++++++------- 9 files changed, 201 insertions(+), 113 deletions(-) diff --git a/backend/src/routes/api/k8s/index.ts b/backend/src/routes/api/k8s/index.ts index 21d63cf24e..6780853049 100644 --- a/backend/src/routes/api/k8s/index.ts +++ b/backend/src/routes/api/k8s/index.ts @@ -40,6 +40,7 @@ module.exports = async (fastify: KubeFastifyInstance) => { url, method: req.method, requestData: data, + overrideContentType: req.headers['content-type'], }; let promise: Promise; diff --git a/backend/src/routes/api/k8s/pass-through.ts b/backend/src/routes/api/k8s/pass-through.ts index e646ff8cdf..d4c24272f3 100644 --- a/backend/src/routes/api/k8s/pass-through.ts +++ b/backend/src/routes/api/k8s/pass-through.ts @@ -12,6 +12,8 @@ export type PassThroughData = { method: string; requestData?: string; url: string; + /** Option to substitute your own content type for the API call -- defaults to JSON */ + overrideContentType?: string; }; export const isK8sStatus = (data: unknown): data is K8sStatus => diff --git a/backend/src/server.ts b/backend/src/server.ts index 480d1e36a9..6467d80aa3 100644 --- a/backend/src/server.ts +++ b/backend/src/server.ts @@ -33,6 +33,21 @@ const app = fastify({ pluginTimeout: 10000, }); +// Allow the fastify server to parse the merge-patch/json content type +app.addContentTypeParser( + 'application/merge-patch+json', + { parseAs: 'string' }, + function (req, body, done) { + try { + const json = JSON.parse(String(body)); + done(null, json); + } catch (err) { + err.statusCode = 400; + done(err, undefined); + } + }, +); + app.register(initializeApp); app.listen({ port: PORT, host: IP }, (err) => { diff --git a/frontend/src/__mocks__/mockStartNotebookData.ts b/frontend/src/__mocks__/mockStartNotebookData.ts index 94379dff22..fe26856b90 100644 --- a/frontend/src/__mocks__/mockStartNotebookData.ts +++ b/frontend/src/__mocks__/mockStartNotebookData.ts @@ -4,12 +4,18 @@ import { mockK8sNameDescriptionFieldData } from '~/__mocks__/mockK8sNameDescript type MockResourceConfigType = { volumeName?: string; + notebookId?: string; }; export const mockStartNotebookData = ({ + notebookId, volumeName = 'test-volume', }: MockResourceConfigType): StartNotebookData => ({ projectName: 'test-project', - notebookData: mockK8sNameDescriptionFieldData({ name: 'test-notebook', description: '' }), + notebookData: mockK8sNameDescriptionFieldData({ + name: 'test-notebook', + description: '', + k8sName: { value: notebookId }, + }), image: { imageStream: { metadata: { 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 499e484000..b32e324cee 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 @@ -556,9 +556,10 @@ describe('Workbench page', () => { editSpawnerPage.findSubmitButton().should('be.enabled'); editSpawnerPage.k8sNameDescription.findDisplayNameInput().fill('Updated Notebook'); - cy.interceptK8s('PUT', NotebookModel, mockNotebookK8sResource({})).as('editWorkbench'); + cy.interceptK8s('PUT', NotebookModel, mockNotebookK8sResource({})).as('editWorkbenchDryRun'); + cy.interceptK8s('PATCH', NotebookModel, mockNotebookK8sResource({})).as('editWorkbench'); editSpawnerPage.findSubmitButton().click(); - cy.wait('@editWorkbench').then((interception) => { + cy.wait('@editWorkbenchDryRun').then((interception) => { expect(interception.request.url).to.include('?dryRun=All'); expect(interception.request.body).to.containSubset({ metadata: { @@ -580,15 +581,10 @@ describe('Workbench page', () => { }, }); }); - // Actual request cy.wait('@editWorkbench').then((interception) => { expect(interception.request.url).not.to.include('?dryRun=All'); }); - - cy.get('@editWorkbench.all').then((interceptions) => { - expect(interceptions).to.have.length(2); // 1 dry run request and 1 actual request - }); }); it('Handle deleted notebook sizes in workbenches table', () => { diff --git a/frontend/src/api/k8s/__tests__/notebooks.spec.ts b/frontend/src/api/k8s/__tests__/notebooks.spec.ts index 83844643aa..0e7d2cc6db 100644 --- a/frontend/src/api/k8s/__tests__/notebooks.spec.ts +++ b/frontend/src/api/k8s/__tests__/notebooks.spec.ts @@ -3,11 +3,9 @@ import { k8sGetResource, k8sPatchResource, k8sListResource, - k8sUpdateResource, k8sDeleteResource, K8sStatus, } from '@openshift/dynamic-plugin-sdk-utils'; -import * as _ from 'lodash-es'; import { NotebookKind } from '~/k8sTypes'; import { Toleration } from '~/types'; import { mockNotebookK8sResource } from '~/__mocks__/mockNotebookK8sResource'; @@ -23,7 +21,6 @@ import { getNotebooks, stopNotebook, startNotebook, - updateNotebook, deleteNotebook, attachNotebookSecret, replaceNotebookSecret, @@ -32,6 +29,7 @@ import { removeNotebookSecret, getStopPatch, startPatch, + mergePatchUpdateNotebook, } from '~/api/k8s/notebooks'; import { @@ -43,6 +41,7 @@ import { import { TolerationChanges, getTolerationPatch } from '~/utilities/tolerations'; import { NotebookModel } from '~/api/models'; +import { k8sMergePatchResource } from '~/api/k8sUtils'; jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({ k8sCreateResource: jest.fn(), @@ -50,7 +49,10 @@ jest.mock('@openshift/dynamic-plugin-sdk-utils', () => ({ k8sGetResource: jest.fn(), k8sListResource: jest.fn(), k8sPatchResource: jest.fn(), - k8sUpdateResource: jest.fn(), +})); + +jest.mock('~/api/k8sUtils', () => ({ + k8sMergePatchResource: jest.fn(), })); jest.mock('~/concepts/pipelines/elyra/utils', () => { @@ -65,7 +67,7 @@ const k8sCreateResourceMock = jest.mocked(k8sCreateResource); const k8sGetResourceMock = jest.mocked(k8sGetResource); const k8sPatchResourceMock = jest.mocked(k8sPatchResource); const k8sListResourceMock = jest.mocked(k8sListResource); -const k8sUpdateResourceMock = jest.mocked(k8sUpdateResource); +const k8sMergePatchResourceMock = jest.mocked(k8sMergePatchResource); const k8sDeleteResourceMock = jest.mocked(k8sDeleteResource); const createElyraServiceAccountRoleBindingMock = jest.mocked(createElyraServiceAccountRoleBinding); @@ -468,66 +470,52 @@ describe('stopNotebook', () => { }); describe('updateNotebook', () => { it('should update a notebook', async () => { - const notebook = assembleNotebook(mockStartNotebookData({}), username); - const existingNotebook = mockNotebookK8sResource({ uid }); - const oldNotebook = structuredClone(existingNotebook); - const container = oldNotebook.spec.template.spec.containers[0]; - - // clean the envFrom array in case of merging the old value again - container.envFrom = []; - - // clean the resources, affinity and tolerations for accelerator - oldNotebook.spec.template.spec.tolerations = []; - oldNotebook.spec.template.spec.affinity = {}; - container.resources = {}; + const notebook = assembleNotebook( + mockStartNotebookData({ notebookId: existingNotebook.metadata.name }), + username, + ); - k8sUpdateResourceMock.mockResolvedValue(existingNotebook); + k8sMergePatchResourceMock.mockResolvedValue(existingNotebook); - const renderResult = await updateNotebook( - existingNotebook, - mockStartNotebookData({}), + const renderResult = await mergePatchUpdateNotebook( + mockStartNotebookData({ notebookId: existingNotebook.metadata.name }), username, ); - expect(k8sUpdateResourceMock).toHaveBeenCalledWith({ + expect(k8sMergePatchResourceMock).toHaveBeenCalledWith({ fetchOptions: { requestInit: {}, }, model: NotebookModel, queryOptions: { queryParams: {} }, - resource: _.merge({}, oldNotebook, notebook), + resource: notebook, }); - expect(k8sUpdateResourceMock).toHaveBeenCalledTimes(1); + expect(k8sMergePatchResourceMock).toHaveBeenCalledTimes(1); expect(renderResult).toStrictEqual(existingNotebook); }); it('should handle errors and rethrow', async () => { - const notebook = assembleNotebook(mockStartNotebookData({}), username); const existingNotebook = mockNotebookK8sResource({ uid }); + const notebook = assembleNotebook( + mockStartNotebookData({ notebookId: existingNotebook.metadata.name }), + username, + ); - const oldNotebook = structuredClone(existingNotebook); - const container = oldNotebook.spec.template.spec.containers[0]; - - // clean the envFrom array in case of merging the old value again - container.envFrom = []; - - // clean the resources, affinity and tolerations for accelerator - oldNotebook.spec.template.spec.tolerations = []; - oldNotebook.spec.template.spec.affinity = {}; - container.resources = {}; - - k8sUpdateResourceMock.mockRejectedValue(new Error('error1')); + k8sMergePatchResourceMock.mockRejectedValue(new Error('error1')); await expect( - updateNotebook(existingNotebook, mockStartNotebookData({}), username), + mergePatchUpdateNotebook( + mockStartNotebookData({ notebookId: existingNotebook.metadata.name }), + username, + ), ).rejects.toThrow('error1'); - expect(k8sUpdateResourceMock).toHaveBeenCalledTimes(1); - expect(k8sUpdateResourceMock).toHaveBeenCalledWith({ + expect(k8sMergePatchResourceMock).toHaveBeenCalledTimes(1); + expect(k8sMergePatchResourceMock).toHaveBeenCalledWith({ fetchOptions: { requestInit: {}, }, model: NotebookModel, queryOptions: { queryParams: {} }, - resource: _.merge({}, oldNotebook, notebook), + resource: notebook, }); }); }); diff --git a/frontend/src/api/k8s/notebooks.ts b/frontend/src/api/k8s/notebooks.ts index c5e1e48bf7..6a8e29232f 100644 --- a/frontend/src/api/k8s/notebooks.ts +++ b/frontend/src/api/k8s/notebooks.ts @@ -4,9 +4,9 @@ import { k8sGetResource, k8sListResource, k8sPatchResource, - k8sUpdateResource, Patch, K8sStatus, + k8sUpdateResource, } from '@openshift/dynamic-plugin-sdk-utils'; import * as _ from 'lodash-es'; import { NotebookModel } from '~/api/models'; @@ -26,6 +26,7 @@ import { } from '~/concepts/pipelines/elyra/utils'; import { Volume, VolumeMount } from '~/types'; import { getImageStreamDisplayName } from '~/pages/projects/screens/spawner/spawnerUtils'; +import { k8sMergePatchResource } from '~/api/k8sUtils'; import { assemblePodSpecOptions, getshmVolume, getshmVolumeMount } from './utils'; export const assembleNotebook = ( @@ -301,6 +302,21 @@ export const updateNotebook = ( ); }; +export const mergePatchUpdateNotebook = ( + assignableData: StartNotebookData, + username: string, + opts?: K8sAPIOptions, +): Promise => + k8sMergePatchResource( + applyK8sAPIOptions( + { + model: NotebookModel, + resource: assembleNotebook(assignableData, username), + }, + opts, + ), + ); + export const deleteNotebook = (notebookName: string, namespace: string): Promise => k8sDeleteResource({ model: NotebookModel, diff --git a/frontend/src/api/k8sUtils.ts b/frontend/src/api/k8sUtils.ts index d8cab4adf4..961d79421b 100644 --- a/frontend/src/api/k8sUtils.ts +++ b/frontend/src/api/k8sUtils.ts @@ -1,7 +1,11 @@ +import { applyOverrides } from '@openshift/dynamic-plugin-sdk'; import { + commonFetchJSON, + getK8sResourceURL, K8sGroupVersionKind, K8sModelCommon, K8sResourceCommon, + K8sResourceUpdateOptions, } from '@openshift/dynamic-plugin-sdk-utils'; export const addOwnerReference = ( @@ -40,3 +44,30 @@ export const groupVersionKind = (model: K8sModelCommon): K8sGroupVersionKind => version: model.apiVersion, kind: model.kind, }); + +export const k8sMergePatchResource = < + TResource extends K8sResourceCommon, + TUpdatedResource extends TResource = TResource, +>({ + model, + resource, + queryOptions = {}, + fetchOptions = {}, +}: K8sResourceUpdateOptions): Promise => { + if (!resource.metadata?.name) { + return Promise.reject(new Error('Resource payload name not specified')); + } + + return commonFetchJSON( + getK8sResourceURL(model, resource, queryOptions), + applyOverrides(fetchOptions.requestInit, { + method: 'PATCH', + headers: { + 'Content-Type': 'application/merge-patch+json', + }, + body: JSON.stringify(resource), + }), + fetchOptions.timeout, + true, + ); +}; diff --git a/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx b/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx index 68137ca2c9..d72dee49db 100644 --- a/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx +++ b/frontend/src/pages/projects/screens/spawner/SpawnerFooter.tsx @@ -4,11 +4,19 @@ import { ActionList, ActionListItem, Alert, + AlertActionLink, Button, Stack, StackItem, } from '@patternfly/react-core'; -import { assembleSecret, createNotebook, createSecret, updateNotebook } from '~/api'; +import { + assembleSecret, + createNotebook, + createSecret, + K8sStatusError, + mergePatchUpdateNotebook, + updateNotebook, +} from '~/api'; import { DataConnectionData, EnvVariable, @@ -48,7 +56,7 @@ const SpawnerFooter: React.FC = ({ dataConnection, canEnablePipelines, }) => { - const [errorMessage, setErrorMessage] = React.useState(''); + const [error, setError] = React.useState(); const { dashboardConfig: { @@ -114,20 +122,64 @@ const SpawnerFooter: React.FC = ({ refreshAllProjectData(); navigate(`/projects/${projectName}?section=${ProjectSectionID.WORKBENCHES}`); }; - const handleError = (e: Error) => { + const handleError = (e: K8sStatusError) => { fireFormTrackingEvent('Workbench Created', { outcome: TrackingOutcome.submit, success: false, error: e.message, }); - setErrorMessage(e.message || 'Error creating workbench'); + setError(e); setCreateInProgress(false); }; const handleStart = () => { - setErrorMessage(''); + setError(undefined); setCreateInProgress(true); }; + const updateNotebookPromise = async (dryRun: boolean) => { + if (!editNotebook) { + return; + } + + const pvcDetails = await replaceRootVolumesForNotebook( + projectName, + editNotebook, + storageData, + dryRun, + ).catch(handleError); + + const envFrom = await updateConfigMapsAndSecretsForNotebook( + projectName, + editNotebook, + envVariables, + dataConnection, + existingNotebookDataConnection, + dryRun, + ).catch(handleError); + + if (!pvcDetails || !envFrom) { + return; + } + + const annotations = { ...editNotebook.metadata.annotations }; + if (envFrom.length > 0) { + annotations['notebooks.opendatahub.io/notebook-restart'] = 'true'; + } + + const { volumes, volumeMounts } = pvcDetails; + const newStartNotebookData: StartNotebookData = { + ...startNotebookData, + volumes, + volumeMounts, + envFrom, + tolerationSettings, + }; + if (dryRun) { + return updateNotebook(editNotebook, newStartNotebookData, username, { dryRun }); + } + return mergePatchUpdateNotebook(newStartNotebookData, username); + }; + const onUpdateNotebook = async () => { if (dataConnection.type === 'creating') { const dataAsRecord = dataConnection.creating?.values?.data.reduce>( @@ -150,61 +202,17 @@ const SpawnerFooter: React.FC = ({ } handleStart(); - if (editNotebook) { - const updateNotebookPromise = async (dryRun: boolean) => { - const pvcDetails = await replaceRootVolumesForNotebook( - projectName, - editNotebook, - storageData, - dryRun, - ).catch(handleError); - - const envFrom = await updateConfigMapsAndSecretsForNotebook( - projectName, - editNotebook, - envVariables, - dataConnection, - existingNotebookDataConnection, - dryRun, - ).catch(handleError); - - if (!pvcDetails || !envFrom) { - return; - } - - const annotations = { ...editNotebook.metadata.annotations }; - if (envFrom.length > 0) { - annotations['notebooks.opendatahub.io/notebook-restart'] = 'true'; - } - - const { volumes, volumeMounts } = pvcDetails; - const newStartNotebookData: StartNotebookData = { - ...startNotebookData, - volumes, - volumeMounts, - envFrom, - tolerationSettings, - }; - return updateNotebook( - { ...editNotebook, metadata: { ...editNotebook.metadata, annotations } }, - newStartNotebookData, - username, - { dryRun }, - ); - }; - - updateNotebookPromise(true) - .then(() => - updateNotebookPromise(false) - .then((notebook) => { - if (notebook) { - afterStart(notebook.metadata.name, 'updated'); - } - }) - .catch(handleError), - ) - .catch(handleError); - } + updateNotebookPromise(true) + .then(() => + updateNotebookPromise(false) + .then((notebook) => { + if (notebook) { + afterStart(notebook.metadata.name, 'updated'); + } + }) + .catch(handleError), + ) + .catch(handleError); }; const onCreateNotebook = async () => { @@ -266,10 +274,35 @@ const SpawnerFooter: React.FC = ({ return ( - {errorMessage && ( + {error && ( - - {errorMessage} + + + updateNotebookPromise(false) + .then((notebook) => { + if (notebook) { + afterStart(notebook.metadata.name, 'updated'); + } + }) + .catch(handleError) + } + > + Force update + + location.reload()}>Refresh + + ) : undefined + } + > + {error.message} )}