-
Notifications
You must be signed in to change notification settings - Fork 178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ossm): adds OSSM annotations to the relevant cluster resources #1088
Changes from 18 commits
bfa5075
1408c29
166e409
47e43ac
dd22cd7
c917d76
93aa3ae
a0ea817
abf520d
5b26e64
c106663
8376c28
48bfe4e
6b12f82
ca51ef3
74b4c79
b55490f
2d2244c
104c88b
df9f285
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -590,6 +590,9 @@ export const getDashboardConfig = (): DashboardConfig => { | |||||||||||||||||||
return dashboardConfigWatcher.getResources()?.[0]; | ||||||||||||||||||||
}; | ||||||||||||||||||||
|
||||||||||||||||||||
export const featureFlagEnabled = (disabledSettingState?: boolean): boolean => | ||||||||||||||||||||
disabledSettingState === false; | ||||||||||||||||||||
|
||||||||||||||||||||
Comment on lines
+593
to
+595
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest adding the same comment as we have present in the frontend code.
Suggested change
|
||||||||||||||||||||
export const updateDashboardConfig = (): Promise<void> => { | ||||||||||||||||||||
return dashboardConfigWatcher.updateResults(); | ||||||||||||||||||||
}; | ||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,11 +26,14 @@ import { | |
} from '~/concepts/pipelines/elyra/utils'; | ||
import { createRoleBinding } from '~/api'; | ||
import { Volume, VolumeMount } from '~/types'; | ||
import { DashboardConfig } from '~/types'; | ||
import { featureFlagEnabled } from '~/utilities/utils'; | ||
import { assemblePodSpecOptions, getshmVolume, getshmVolumeMount } from './utils'; | ||
|
||
const assembleNotebook = ( | ||
data: StartNotebookData, | ||
username: string, | ||
enableServiceMesh: boolean, | ||
canEnablePipelines?: boolean, | ||
): NotebookKind => { | ||
const { | ||
|
@@ -92,14 +95,16 @@ const assembleNotebook = ( | |
'opendatahub.io/odh-managed': 'true', | ||
'opendatahub.io/user': translatedUsername, | ||
[KnownLabels.DASHBOARD_RESOURCE]: 'true', | ||
'sidecar.istio.io/inject': String(enableServiceMesh), | ||
}, | ||
annotations: { | ||
'openshift.io/display-name': notebookName.trim(), | ||
'openshift.io/description': description || '', | ||
'notebooks.opendatahub.io/oauth-logout-url': `${origin}/projects/${projectName}?notebookLogout=${notebookId}`, | ||
'notebooks.opendatahub.io/last-size-selection': notebookSize.name, | ||
'notebooks.opendatahub.io/last-image-selection': imageSelection, | ||
'notebooks.opendatahub.io/inject-oauth': 'true', | ||
'notebooks.opendatahub.io/inject-oauth': String(!enableServiceMesh), | ||
'opendatahub.io/service-mesh': String(enableServiceMesh), | ||
'opendatahub.io/username': username, | ||
}, | ||
name: notebookId, | ||
|
@@ -187,6 +192,24 @@ const getStopPatch = (): Patch => ({ | |
value: getStopPatchDataString(), | ||
}); | ||
|
||
const getInjectOAuthPatch = (enableServiceMesh: boolean): Patch => ({ | ||
op: 'add', | ||
path: '/metadata/annotations/notebooks.opendatahub.io~1inject-oauth', | ||
value: String(!enableServiceMesh), | ||
}); | ||
|
||
const getProxyInjectPatch = (enableServiceMesh: boolean): Patch => ({ | ||
op: 'add', | ||
path: '/metadata/labels/sidecar.istio.io~1inject', | ||
value: String(enableServiceMesh), | ||
}); | ||
|
||
const getServiceMeshPatch = (enableServiceMesh: boolean): Patch => ({ | ||
op: 'add', | ||
path: '/metadata/annotations/opendatahub.io~1service-mesh', | ||
value: String(enableServiceMesh), | ||
}); | ||
|
||
export const getNotebooks = (namespace: string): Promise<NotebookKind[]> => | ||
k8sListResource<NotebookKind>({ | ||
model: NotebookModel, | ||
|
@@ -210,10 +233,19 @@ export const startNotebook = async ( | |
name: string, | ||
namespace: string, | ||
tolerationChanges: TolerationChanges, | ||
dashboardConfig: DashboardConfig, | ||
enablePipelines?: boolean, | ||
): Promise<NotebookKind> => { | ||
const patches: Patch[] = []; | ||
patches.push(startPatch); | ||
const enableServiceMesh = featureFlagEnabled( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tip: If you are using several times the same constant, you can add it into a utils function and reuse it several times. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure I understand. Are you suggesting creating a util function that getsAppContext, then calls featureFlagEnabled to parse the service mesh flag (therefore taking no inputs and returning this bool)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I'm talking about something like this: https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/pages/projects/projectSharing/utils.ts#L7 is not mandatory but it might be nice to have |
||
dashboardConfig.spec.dashboardConfig.disableServiceMesh, | ||
); | ||
|
||
const patches: Patch[] = [ | ||
startPatch, | ||
getInjectOAuthPatch(enableServiceMesh), | ||
getServiceMeshPatch(enableServiceMesh), | ||
getProxyInjectPatch(enableServiceMesh), | ||
Comment on lines
+245
to
+247
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These seem all very related, we may want to look at cleaning that up in the future to make it one "set of patches" rather than 3 separate ones. Just for code cleanness. |
||
]; | ||
|
||
const tolerationPatch = getTolerationPatch(tolerationChanges); | ||
if (tolerationPatch) { | ||
|
@@ -242,9 +274,10 @@ export const startNotebook = async ( | |
export const createNotebook = ( | ||
data: StartNotebookData, | ||
username: string, | ||
enableServiceMesh: boolean, | ||
canEnablePipelines?: boolean, | ||
): Promise<NotebookKind> => { | ||
const notebook = assembleNotebook(data, username, canEnablePipelines); | ||
const notebook = assembleNotebook(data, username, enableServiceMesh, canEnablePipelines); | ||
|
||
const notebookPromise = k8sCreateResource<NotebookKind>({ | ||
model: NotebookModel, | ||
|
@@ -264,9 +297,10 @@ export const updateNotebook = ( | |
existingNotebook: NotebookKind, | ||
data: StartNotebookData, | ||
username: string, | ||
enableServiceMesh: boolean, | ||
): Promise<NotebookKind> => { | ||
data.notebookId = existingNotebook.metadata.name; | ||
const notebook = assembleNotebook(data, username); | ||
const notebook = assembleNotebook(data, username, enableServiceMesh); | ||
|
||
const oldNotebook = structuredClone(existingNotebook); | ||
const container = oldNotebook.spec.template.spec.containers[0]; | ||
|
@@ -287,9 +321,10 @@ export const updateNotebook = ( | |
export const createNotebookWithoutStarting = ( | ||
data: StartNotebookData, | ||
username: string, | ||
enableServiceMesh: boolean, | ||
): Promise<NotebookKind> => | ||
new Promise((resolve, reject) => | ||
createNotebook(data, username).then((notebook) => | ||
createNotebook(data, username, enableServiceMesh).then((notebook) => | ||
setTimeout( | ||
() => | ||
stopNotebook(notebook.metadata.name, notebook.metadata.namespace) | ||
|
@@ -299,7 +334,6 @@ export const createNotebookWithoutStarting = ( | |
), | ||
), | ||
); | ||
|
||
export const deleteNotebook = (notebookName: string, namespace: string): Promise<K8sStatus> => | ||
k8sDeleteResource<NotebookKind, K8sStatus>({ | ||
model: NotebookModel, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we fall back to retrieving the host through the route?
By enabling the flag without service mesh, we'll get an invalid url.