[Tracker] Custom Notebook (BYON) Improvements - Part 1

[andrewballantyne]

Description

General improvements to the Custom Notebooks Admin UI (part 1 is the "low hanging fruit").

Part 2 continues this effort

• [Tracker] Custom Notebook (BYON) Improvements - Part 2 #1266

Target Branch

f/byon

Requirements

Update all text items and field labels as defined in the UX
After editing an image, the 'enabled' state is automatically set to disabled. Any edits should not change the state - if enabled prior to edits, it should remain enabled after edits and if disabled prior to edits, it should remain disabled after edits.
When adding custom images, the image stream names are not intuitive - eg. 'byon-xxxx'. This makes it hard to differentiate across custom images. Would like the name to be more intuitive and not include 'byon' prefix.

• Idea is to have an object (Kubernetes) name and display name for each custom NB image. The image stream would be "Custom: (Kubernetes name)".
• Users should be able to access the Kubernetes name from the custom NB images admin UI, so they can find it in the list of image streams.
• Kubernetes name must be unique (for custom NB images); display name does not need to be unique. Display name is what users will see in the UI for selecting images.
• When adding a URL for image location field, remove preceding and trailing white spaces.

Itemized UX Issues

• Custom Notebooks Updates sig-ml-developer-experience#28

Itemized Dev Issues

• Update all text items and field labels on Notebook images table #1432
• Update all text items and field labels in modals #1433
• Keep the enabled/disabled status after editing the custom notebook image #1434
• Generate meaningful name for custom notebook image #1435
• [Bug]: Image Error has Misleading Enabled State #1388
• [Tech Debt]: Cleanup Image Regex #1326
• [Test]: Custom Notebook (BYON) Improvements #1970

Related artifacts

UX Mocks: https://www.sketch.com/s/c8183f89-76f5-4a23-b885-a9e404a61414/p/C1443B6E-E3F9-4136-BBC2-994F4D7AF7D7

[vconzola]

LGTM.

[shalberd]

Does byon always use spec.tags[tagname].referencePolicy.type: Source in assembling the imagestream yaml, thereby always pulling from the original url location, or is there spec.tags[tagname].referencePolicy.type: Local for saving the externally referenced image in the openshift internal registry, if present?

Also, if referencePolicy is set to type: Local (for openshift internal registry reference after import), are there plans to add fields in OdhDashboardConfig for authenticated external docker registry access (think i.e. a corporate Artifactory or Harbor with pull/push authentication enabled)? Such credentials, in the form of either a reference to an already existing image pull secret or constructing that image pull secret from username, password, and repo base path, in the admin section, would be very useful for authenticated registry access. Furthermore, the name of the image pull secret could be used later in the assembled Notebook CR.

Just some food for thought for later on.

[andrewballantyne]

@shalberd this is already done & just working through final verification before we move it to stable. No major changes to supporting external registries (IIUC) will happen at this point. FWIW, we use lookupPolicy.local on the imagestream, but I don't see extra referencePolicy info on the tags -- could be default? (code).

Your comment seems related to your PR800 effort, and if you'd like to suggest future enhancements to our infra outside of what you're doing in PR800, Phase 2 is not that far off. We will be supporting things like OOTB images turning off.

If I am mistaking things again with respect to the image stuff or what you're saying, please correct me. Happy to work with you to make this more robust.

[shalberd]

FWIW, we use lookupPolicy.local on the imagestream, but I don't see extra referencePolicy info on the tags

Thank you for the code where the imagestream is assembled. Yes, indeed, the default tag referencePolicy is Source:

https://docs.openshift.com/container-platform/4.12/rest_api/image_apis/imagestreamtag-image-openshift-io-v1.html#tag-referencepolicy

• The default value is Source, indicating the original location of the image should be used (if imported).

• The user may also specify Local, indicating that the pull spec should point to the integrated container image registry and leverage the registry’s ability to proxy the pull to an upstream registry.

As you see, the current way that the dashboard assembleNotebook logic fills in the image-field value for the container is contrary to the behavior decribed in the bullet points and the link.

Thus, work in dashboard PR 800 and kubeflow notebook controller PR 133 will lay a good groundwork for further improvements to image stream and BYON image handling. Agreed, phase 2.

[jedemo]

Looks good to me

[manosnoam]

FYI, the Doc Issue is: https://issues.redhat.com/browse/RHODS-12590