Skip to content
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

[Bug]: jupyter tile notebook server control panel image details not sha256 digest compatible #1340

Closed
1 task done
shalberd opened this issue Jun 6, 2023 · 10 comments
Closed
1 task done
Assignees
Labels
community feature/notebook-controller KubeFlow NoteBook Controller (KFNBC) Feature field-priority Flag to track improvements that are for stability -- effort to put in front of new functionality kind/bug Something isn't working migrated priority/normal An issue with the product; fix when possible

Comments

@shalberd
Copy link
Contributor

shalberd commented Jun 6, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

More of a cosmetic bug, but still:

Was a direct bug resulting from my PR changes, has nothing to do with dashboard itself, more with the reliance on openshift internal registry and imagestream tag and name. The image change trigger annotation admission plug-in in my PR that handles image stream values and looking up container image field values from them is always working with sha256 resolve of an imagestream tag and image behind it. It also resolves to references in the internal openshift registry in sha256 format, which makes sense, but leads to some side effects with your current manual parsing of image fields. We still need info on the actual imagestream tag, so the way to do that is to keep imagestreamame:imagestreamtag in the ENV var JUPYER_IMAGE, something that I have confirmed CI tests downstream need (actual imagestream name) anyways.

My PR 800 makes it so that container image fields are, via image change trigger annotation, always in sha256 format.
registry-openshift-registry:5000/opendatahub/jupyter-standard-datascience@sha256:5df71f5542d2e0161f0f4342aa9a390679d72dc6fae192fd8da1e5671b27e8d4

so imagestream name and tag parsing did not work anymore ... Fixed in my PR 800, see last comment.

Discussion on/about putting imagestreamname:tag string into env var JUPYTER_IMAGE, being compatible with downstream CI tests here

Bildschirmfoto 2023-06-06 um 15 21 11

Jupyter tile

notebook server successfully spawned

non-weekly image tag version, that is, not the one with opendatahub.io/workbench-image-recommended:

Bildschirmfoto 2023-06-06 um 16 20 14

image-field-value of container is in sha256 digest format, e.g. myrepo.com/notebook@sha256:492991292929cm19291929

notebook server details notebook image shows:

Danger alert:Error loading related images...
Unable to show notebook image details at this time.

https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/pages/notebookController/screens/server/NotebookServerDetails.tsx#L64

This is because image and tag parsing at

https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/pages/notebookController/screens/server/NotebookServerDetails.tsx#L64

is currently only tag-based, not digest-based. This presents a problem when using imagestreams such as

https://github.com/opendatahub-io/odh-manifests/blob/master/notebook-images/base/jupyter-datascience-notebook-imagestream.yaml#L35

where the tag is in sha256 digest format.

https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/utilities/imageUtils.ts#L159

You'd have to make a check for image and tag or image and digest.

Expected Behavior

parse image, tag

e.g. from quai.io/myrepo:v2.5

or

image, digest

e.g. from quai.io/myrepo@sha256:xxxxxxxx

My notebook running container section looks like this:

     image: >-
        quay.io/opendatahub/notebooks@sha256:5df71f5542d2e0161f0f4342aa9a390679d72dc6fae192fd8da1e5671b27e8d4

but could also look like this when using the internal registry of Openshift

     image: >-
        registry-openshift-registry:5000/opendatahub/notebooks@sha256:5df71f5542d2e0161f0f4342aa9a390679d72dc6fae192fd8da1e5671b27e8d4

Among others, I think you want to display imagestream name and tag (not image name and tag) on the gui there, e.g. jupyter-datascience-notebook:1.2

but you do a kind of reverse lookup from the container image field value back to that info.

Since imagestreams always have tags and names, while container image-sections do not, the entire logic in imageUtils does not work with that approach.

Steps To Reproduce

latest master version of dashboard

master branch odh-manifests

image: Jupyter Data Science
Version: 1.2

Workaround (if any)

No response

What browsers are you seeing the problem on?

No response

Open Data Hub Version

1.6.0

Anything else

You probably have to check your frontend image utils

https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/utilities/imageUtils.ts

in general for parsing out digests, too in addition to image names and tags (when tag is not present)

It is not always clear and maybe even a misnomer in the code types to me if image tag is the image tag in the container or the imagestream tag.

Same for imagestream name vs image name from tags.from.name

Probably a matter of having a more consolidated look at e.g. imageUtils.ts

There's gotta be an easier way of getting to imagestream name and tag, not based on the container image-value, as done here https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/utilities/imageUtils.ts#L167

You'd have to make lookup of

https://github.com/opendatahub-io/odh-manifests/blob/master/notebook-images/base/jupyter-datascience-notebook-imagestream.yaml#L25

and

https://github.com/opendatahub-io/odh-manifests/blob/master/notebook-images/base/jupyter-datascience-notebook-imagestream.yaml#L36

tricky ...

@andrewballantyne
I once suggested putting notebook annotation value

notebooks.opendatahub.io/last-image-selection: jupyter-datascience-notebook:1.2

which is composed of imagestream name and tag but not present in the type: Pod at the end

into env variable JUPYTER_IMAGE, ie. jupyter-datascience-notebook:1.2

listed in pod spec.containers[0].env array ...

which would be way easier to getting to imagestream name and tag the running container is based on.

But never got an answer from RHOSDS folks and @VaishnaviHire as to what exactly env JUPYTER_IMAGE in current format
registry-openshift-registry:5000/opendatahub/jupyter-datascience-notebook@sha256:5df71f5542d2e0161f0f4342aa9a390679d72dc6fae192fd8da1e5671b27e8d4

or

registry-openshift-registry:5000/opendatahub/jupyter-datascience-notebook:1.2
is necessary for.

Seems to be for some CI spawned image tests at downstream that would not work for digest-formats anyways anymore, or whether the CI test could be written differently. @lugi0 As far as I see, the python version tag check is run live in the running notebook, not dependent in JUPYTER_IMAGE env value being in one format or the other. Also, ${image} in the CI tests is in reality really the imagestream name, so the goal is to get to the imagestream name of the currently selected notebook, i.e. jupyter-datascience-notebook, to compare with what the original imagestream name was.

current situation with JUPYTER_IMAGE content get the imagestream name as well:

Bildschirmfoto 2023-06-06 um 18 26 46

So, if we were to put imagestreamname:tag directly in the notebook env var JUPYTER_IMAGE in dashboard PR 800, @lugi0 would not even need to change the line downstream at

https://github.com/red-hat-data-services/ods-ci/blob/2c62e207183f1ec560ca44cefdcfa0bfb83c0c4d/ods_ci/tests/Resources/Page/ODH/JupyterHub/JupyterHubSpawner.robot#L305

since imagestream name still comes out

Bildschirmfoto 2023-06-06 um 18 17 14

Of course, since we don't have the internal docker image registry full path in there anymore, optionally, you could change the line in CI test to

${out} = Run Cell And Get Output import os; print(os.environ["JUPYTER_IMAGE"].split(":")[0])
Bildschirmfoto 2023-06-06 um 18 19 05

but not necessary, the parse-string is variable enough for both cases.

@VaishnaviHire putting imagestreamname:imagestreamtag in env VAR JUPYTER_IMAGE would not affect CI tests, but would make getting to imagestream name and tag of the currently running notebook pod wayy easier.

See https://github.com/opendatahub-io/odh-dashboard/pull/800/files#diff-7231b0281c274f99ded5edc8d0172a35bded20a62358f877916fa2181e573ffbL295

these thoughts also have some loose connection to PR 800

@shalberd shalberd added kind/bug Something isn't working untriaged Indicates the newly create issue has not been triaged yet labels Jun 6, 2023
@lucferbux
Copy link
Contributor

@shalberd When you talk about jupyter tile you mean non DS Projects notebooks right?

@shalberd
Copy link
Contributor Author

shalberd commented Jun 6, 2023

@lucferbux correct, I mean non DS projects notebook details.

I don't know if you also display image name and tag in the DS projects view for a workbench ... in any case, this specific bug here is for jupyter tile non DS

Could also have an effect on edit workbench and dropdown list there for the current running image ... I have not looked at that.

@lucferbux
Copy link
Contributor

@lucferbux correct, I mean non DS projects notebook details.

I don't know if you also display image name and tag in the DS projects view for a workbench ... in any case, this specific bug here is for jupyter tile non DS

Could also have an effect on edit workbench and dropdown list there for the current running image ... I have not looked at that.

Yes.... we are bringing efforts to unify code between standard notebooks and DS Projects notebooks, and thanks for all the info cause we have a very scoped solution to fix. We have a grooming session tomorrow, so we can prioritize this.

Thanks for all the help!

@shalberd
Copy link
Contributor Author

shalberd commented Jun 6, 2023

Yes, having e.g. the imageutils all in frontend is a good thing

https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/utilities/imageUtils.ts

My input / suggestion is mainly making container image parsing work for tag and digest format image field values

     image: >-
        quay.io/opendatahub/notebooks@sha256:5df71f5542d2e0161f0f4342aa9a390679d72dc6fae192fd8da1e5671b27e8d4

but could also look like this when using the internal registry of Openshift

     image: >-
        quay.io/opendatahub/notebook
     image: >-
        registry-openshift-registry:5000/opendatahub/notebooks@sha256:5df71f5542d2e0161f0f4342aa9a390679d72dc6fae192fd8da1e5671b27e8d4

or look like this tag-based

     image: >-
        registry-openshift-registry:5000/opendatahub/notebooks:jupyter-stable-3.8-datascience

display name and all the stuff coming from ImageInfo should be unaffected by this bug, as ImageInfo really comes from the imagestream with all its details.

@shalberd
Copy link
Contributor Author

shalberd commented Jun 6, 2023

@lucferbux I'd suggest the following approach: First, you consolidate frontend and backend types like ImageTag, ImageInfo, ImageTagInfo and all that other stuff. I understand now how you stored all the info from an Openshift Imagestream CR in all those types.

I will find an alternative and put in code for now in PR-800 assuming the env variable JUPYTER_IMAGE can be used to lookup what we need without parsing imagestream name and tag from the container image field. I will always follow along master as you all move along.

That way, we don't interfere with each other. My error occurs because I have container image value resolved to a string that contains digest value, not tag type notation. That (always resolving to digest-notation) is a result of the image trigger annotation I use in PR-800 to be independent of the openshift internal registry.

I will within PR 800 make my way towards all instances where that container image digest notation i.e. registry-openshift-registry:5000/opendatahub/jupyter-datascience@sha256:5df71f5542d can be a problem, I find my way around the code easily now that I understand your types used.

For example, assuming we can in future get imagestream name and tag from env var JUPYTER_IMAGE, as in my PR, I modified the bits where container image field value splits were done for getting imagestream name and tag and instead getting it now from env JUPYTER_IMAGE. 54dbba2

All working fine, both Jupyter tile and notebook / workbench edit page.

Bildschirmfoto 2023-06-06 um 23 51 06

@lugi0
Copy link
Contributor

lugi0 commented Jun 7, 2023

@lugi0 As far as I see, the python version tag check is run live in the running notebook, not dependent in JUPYTER_IMAGE env value being in one format or the other. Also, ${image} in the CI tests is in reality really the imagestream name, so the goal is to get to the imagestream name of the currently selected notebook, i.e. jupyter-datascience-notebook, to compare with what the original imagestream name was.
[...]
So, if we were to put imagestreamname:tag directly in the notebook env var JUPYTER_IMAGE in dashboard PR 800, @lugi0 would not even need to change the line downstream at
https://github.com/red-hat-data-services/ods-ci/blob/2c62e207183f1ec560ca44cefdcfa0bfb83c0c4d/ods_ci/tests/Resources/Page/ODH/JupyterHub/JupyterHubSpawner.robot#L305
since imagestream name still comes out

I can confirm that this looks good to me.
FWIW, even if we had to update our tests downstream to get to the imagestream name because it was moved to another env var or because the format changed, it would not be a big problem to deal with; I do appreciate the heads up though!

@Gkrumbach07 Gkrumbach07 added feature/notebook-controller KubeFlow NoteBook Controller (KFNBC) Feature priority/normal An issue with the product; fix when possible and removed untriaged Indicates the newly create issue has not been triaged yet labels Jun 7, 2023
@andrewballantyne andrewballantyne added the field-priority Flag to track improvements that are for stability -- effort to put in front of new functionality label Jun 22, 2023
@dgutride
Copy link
Contributor

dgutride commented Dec 5, 2023

Migrated to jira: https://issues.redhat.com/browse/RHOAIENG-320

@shalberd
Copy link
Contributor Author

shalberd commented Dec 5, 2023

@dgutride I am working on this, maybe you can add it to the redhat ticket.
It is working fine as part of dashboard PR-800

However, to close this issue, we will have to merge in kubeflow notebook controller PR-133, which is a requirement for dashboard PR-800 along with the image change trigger annotation to work.

I have been working very well with Harshad Reddy Nalla @harshad16 and Alexandra Theodorapolou @atheo89, but seems like due to other pressing customer matters, both PRs cannot get finalized.

Just so much from me: I've got PR-800 dashboard and PR-133 kubeflow-notebook-controller working fine in my environment, and Alexandra also confirmed it is working well in environments with the internal openshift docker registry enabled.

Impacts on CI tests downstream have also been cleared up and are a non-issue, as well as existing running notebook CRs that do not yet have the image content change trigger annotation.

@dgutride
Copy link
Contributor

dgutride commented Dec 5, 2023

Keeping open -we will double track for now until we figure out a model for upstream/downstream contribution tracking

@shalberd
Copy link
Contributor Author

shalberd commented Jun 4, 2024

@harshad16 @jiridanek @jstourac @atheo89 @lucferbux

closed in favor odh odh notebook controller doing the notebook image field lookup in all cases (no openshift internal registry, openshift internal registry): opendatahub-io/kubeflow#336 and opendatahub-io/kubeflow#329

as well as dashboard changes #2867

Thank you all very much, neat work

minor remaining: changing notebook container imagePullPolicy: IfNotPresent from old imagePullPolicy: Always

as a nice-to-have feature for external registry and large images with a unique hash. Needs to be validated if that works with internal openshift registry, too. Saves times on workbench startup.

@shalberd shalberd closed this as completed Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community feature/notebook-controller KubeFlow NoteBook Controller (KFNBC) Feature field-priority Flag to track improvements that are for stability -- effort to put in front of new functionality kind/bug Something isn't working migrated priority/normal An issue with the product; fix when possible
Projects
Status: Done
Status: Dashboard
Archived in project
6 participants