Skip to content

Commit

Permalink
Dry run all workbench create/update calls before submit (#3301)
Browse files Browse the repository at this point in the history
* Dry run all workbench create/update calls before submit

* Remove duplicated data connection creating calls
  • Loading branch information
DaoDaoNoCode authored Oct 10, 2024
1 parent 8c7fe76 commit fee50c5
Show file tree
Hide file tree
Showing 10 changed files with 333 additions and 110 deletions.
88 changes: 87 additions & 1 deletion frontend/src/__mocks__/mockStartNotebookData.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
import { ImageStreamKind } from '~/k8sTypes';
import { StartNotebookData } from '~/pages/projects/types';
import {
ConfigMapCategory,
DataConnectionData,
EnvironmentVariableType,
EnvVariable,
SecretCategory,
StartNotebookData,
StorageData,
StorageType,
} from '~/pages/projects/types';
import { mockK8sNameDescriptionFieldData } from '~/__mocks__/mockK8sNameDescriptionFieldData';

type MockResourceConfigType = {
Expand Down Expand Up @@ -84,3 +93,80 @@ export const mockStartNotebookData = ({
},
},
});

export const mockStorageData: StorageData = {
storageType: StorageType.NEW_PVC,
creating: {
nameDesc: {
name: 'test-pvc',
description: '',
},
size: '20Gi',
storageClassName: 'gp2-csi',
},
existing: { storage: '' },
};

export const mockDataConnectionData: DataConnectionData = {
type: 'creating',
enabled: true,
creating: {
type: EnvironmentVariableType.SECRET,
values: {
category: SecretCategory.AWS,
data: [
{
key: 'Name',
value: 'test-name',
},
{
key: 'AWS_ACCESS_KEY_ID',
value: 'test-access-key',
},
{
key: 'AWS_SECRET_ACCESS_KEY',
value: 'test-secret-key',
},
{
key: 'AWS_S3_BUCKET',
value: '',
},
{
key: 'AWS_S3_ENDPOINT',
value: 'test-endpoint',
},
{
key: 'AWS_DEFAULT_REGION',
value: '',
},
],
},
},
};

export const mockEnvVariables: EnvVariable[] = [
{
type: EnvironmentVariableType.CONFIG_MAP,
values: {
category: ConfigMapCategory.GENERIC,
data: [
{
key: 'test-key',
value: 'test-value',
},
],
},
},
{
type: EnvironmentVariableType.SECRET,
values: {
category: SecretCategory.GENERIC,
data: [
{
key: 'test-key',
value: 'test-value',
},
],
},
},
];
2 changes: 1 addition & 1 deletion frontend/src/api/errorUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export const isK8sStatus = (data: unknown): data is K8sStatus =>
typeof data === 'object' && data !== null && 'kind' in data && data.kind === 'Status';

export class K8sStatusError extends Error {
public statusObject: K8sStatus;
public statusObject: K8sStatus & { details?: { kind?: string } };

constructor(statusObject: K8sStatus) {
super(statusObject.message);
Expand Down
37 changes: 29 additions & 8 deletions frontend/src/api/k8s/__tests__/notebooks.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ describe('assembleNotebook', () => {
startNotebookDataMock.volumeMounts = undefined;
startNotebookDataMock.volumes = undefined;

const Notebook = assembleNotebook(startNotebookDataMock, username, true);
const notebook = assembleNotebook(startNotebookDataMock, username, true);
k8sCreateResourceMock.mockResolvedValue(mockNotebookK8sResource({ uid: 'test' }));

createElyraServiceAccountRoleBindingMock.mockResolvedValue(
Expand All @@ -171,11 +171,16 @@ describe('assembleNotebook', () => {
const renderResult = await createNotebook(startNotebookDataMock, username, true);

expect(k8sCreateResourceMock).toHaveBeenCalledWith({
fetchOptions: { requestInit: {} },
model: NotebookModel,
resource: Notebook,
resource: notebook,
queryOptions: {
queryParams: {},
},
});
expect(createElyraServiceAccountRoleBindingMock).toHaveBeenCalledWith(
mockNotebookK8sResource({ uid: 'test' }),
undefined,
);
expect(createElyraServiceAccountRoleBindingMock).toHaveBeenCalledTimes(1);
expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1);
Expand All @@ -185,7 +190,7 @@ describe('assembleNotebook', () => {
const startNotebookMock = mockStartNotebookData({});
startNotebookMock.volumes = undefined;
startNotebookMock.volumeMounts = undefined;
const Notebook = assembleNotebook(startNotebookMock, username, false);
const notebook = assembleNotebook(startNotebookMock, username, false);
k8sCreateResourceMock.mockResolvedValue(mockNotebookK8sResource({ uid: 'test' }));

createElyraServiceAccountRoleBindingMock.mockResolvedValue(
Expand All @@ -195,8 +200,12 @@ describe('assembleNotebook', () => {
const renderResult = await createNotebook(startNotebookMock, username, false);

expect(k8sCreateResourceMock).toHaveBeenCalledWith({
fetchOptions: { requestInit: {} },
model: NotebookModel,
resource: Notebook,
resource: notebook,
queryOptions: {
queryParams: {},
},
});
expect(createElyraServiceAccountRoleBindingMock).toHaveBeenCalledTimes(0);
expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1);
Expand All @@ -218,38 +227,50 @@ describe('createNotebook', () => {

expect(createElyraServiceAccountRoleBinding).not.toHaveBeenCalled();
expect(k8sCreateResourceMock).toHaveBeenCalledWith({
fetchOptions: { requestInit: {} },
model: NotebookModel,
resource: notebook,
queryOptions: {
queryParams: {},
},
});
expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1);
expect(renderResult).toStrictEqual(mockNotebookK8sResource({ uid }));
});
it('should create a notebook with pipelines', async () => {
const Notebook = assembleNotebook(mockStartNotebookData({}), username, true);
const notebook = assembleNotebook(mockStartNotebookData({}), username, true);
k8sCreateResourceMock.mockResolvedValue(mockNotebookK8sResource({ uid }));

createElyraServiceAccountRoleBindingMock.mockResolvedValue(mockRoleBindingK8sResource({ uid }));

const renderResult = await createNotebook(mockStartNotebookData({}), username, true);

expect(k8sCreateResourceMock).toHaveBeenCalledWith({
fetchOptions: { requestInit: {} },
model: NotebookModel,
resource: Notebook,
resource: notebook,
queryOptions: {
queryParams: {},
},
});
expect(createElyraServiceAccountRoleBindingMock).toHaveBeenCalledTimes(1);
expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1);
expect(renderResult).toStrictEqual(mockNotebookK8sResource({ uid }));
});
it('should handle errors and rethrow', async () => {
const Notebook = assembleNotebook(mockStartNotebookData({}), username, true);
const notebook = assembleNotebook(mockStartNotebookData({}), username, true);
k8sCreateResourceMock.mockRejectedValue(new Error('error1'));
await expect(createNotebook(mockStartNotebookData({}), username, true)).rejects.toThrow(
'error1',
);
expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1);
expect(k8sCreateResourceMock).toHaveBeenCalledWith({
fetchOptions: { requestInit: {} },
model: NotebookModel,
resource: Notebook,
resource: notebook,
queryOptions: {
queryParams: {},
},
});
});
});
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/api/k8s/__tests__/roleBindings.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -347,9 +347,10 @@ describe('patchRoleBindingOwnerRef', () => {
k8sPatchResourceMock.mockResolvedValue(roleBindingMock);
const result = await patchRoleBindingOwnerRef('rbName', namespace, []);
expect(k8sPatchResourceMock).toHaveBeenCalledWith({
fetchOptions: { requestInit: {} },
model: RoleBindingModel,
patches: [{ op: 'replace', path: '/metadata/ownerReferences', value: [] }],
queryOptions: { name: 'rbName', ns: namespace },
queryOptions: { name: 'rbName', ns: namespace, queryParams: {} },
});
expect(k8sPatchResourceMock).toHaveBeenCalledTimes(1);
expect(result).toStrictEqual(roleBindingMock);
Expand Down
37 changes: 23 additions & 14 deletions frontend/src/api/k8s/notebooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,23 +256,32 @@ export const createNotebook = (
data: StartNotebookData,
username: string,
canEnablePipelines?: boolean,
): Promise<NotebookKind> => {
const notebook = assembleNotebook(data, username, canEnablePipelines);
opts?: K8sAPIOptions,
): Promise<NotebookKind> =>
new Promise((resolve, reject) => {
const notebook = assembleNotebook(data, username, canEnablePipelines);

const notebookPromise = k8sCreateResource<NotebookKind>({
model: NotebookModel,
resource: notebook,
k8sCreateResource<NotebookKind>(
applyK8sAPIOptions(
{
model: NotebookModel,
resource: notebook,
},
opts,
),
)
.then((fetchedNotebook) => {
if (canEnablePipelines) {
createElyraServiceAccountRoleBinding(fetchedNotebook, opts)
.then(() => resolve(fetchedNotebook))
.catch(reject);
} else {
resolve(fetchedNotebook);
}
})
.catch(reject);
});

if (canEnablePipelines) {
return notebookPromise.then((fetchedNotebook) =>
createElyraServiceAccountRoleBinding(fetchedNotebook).then(() => fetchedNotebook),
);
}

return notebookPromise;
};

export const updateNotebook = (
existingNotebook: NotebookKind,
assignableData: StartNotebookData,
Expand Down
24 changes: 15 additions & 9 deletions frontend/src/api/k8s/roleBindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,15 +132,21 @@ export const patchRoleBindingOwnerRef = (
rbName: string,
namespace: string,
ownerReferences: OwnerReference[],
opts?: K8sAPIOptions,
): Promise<RoleBindingKind> =>
k8sPatchResource<RoleBindingKind>({
model: RoleBindingModel,
queryOptions: { name: rbName, ns: namespace },
patches: [
k8sPatchResource<RoleBindingKind>(
applyK8sAPIOptions(
{
op: 'replace',
path: '/metadata/ownerReferences',
value: ownerReferences,
model: RoleBindingModel,
queryOptions: { name: rbName, ns: namespace },
patches: [
{
op: 'replace',
path: '/metadata/ownerReferences',
value: ownerReferences,
},
],
},
],
});
opts,
),
);
4 changes: 4 additions & 0 deletions frontend/src/concepts/pipelines/elyra/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Patch } from '@openshift/dynamic-plugin-sdk-utils';
import {
DSPipelineExternalStorageKind,
ImageStreamSpecTagType,
K8sAPIOptions,
KnownLabels,
NotebookKind,
RoleBindingKind,
Expand Down Expand Up @@ -151,6 +152,7 @@ export const generateElyraServiceAccountRoleBinding = (

export const createElyraServiceAccountRoleBinding = async (
notebook: NotebookKind,
opts?: K8sAPIOptions,
): Promise<RoleBindingKind | void> => {
const notebookName = notebook.metadata.name;
const { namespace } = notebook.metadata;
Expand Down Expand Up @@ -181,6 +183,7 @@ export const createElyraServiceAccountRoleBinding = async (
roleBinding.metadata.name,
roleBinding.metadata.namespace,
ownerReferences,
opts,
).catch((e) => {
// This is not ideal, but it shouldn't impact the starting of the notebook. Let us log it, and mute the error
// eslint-disable-next-line no-console
Expand All @@ -191,6 +194,7 @@ export const createElyraServiceAccountRoleBinding = async (
}
return createRoleBinding(
generateElyraServiceAccountRoleBinding(notebookName, namespace, notebookUid),
opts,
).catch((e) => {
// eslint-disable-next-line no-console
console.error(
Expand Down
Loading

0 comments on commit fee50c5

Please sign in to comment.