[Bug]: Store the Notebook Image Name

[andrewballantyne]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

In the table of workbenches, we explain the notebook image in play. This is a hard reference to images we have in play at this point in time.

However, when we keep moving n & n+1 images forward... older notebooks lose that reference and fall into "Unknown" image name. This is not ideal for upgrades and on-going improvements for the Dashboard to cause "loss" of image.

Expected Behavior

Store the image name for when we lose it, it can still represent the image in the table -- but it won't be able to find it in the list of edit.

Maybe UX can let us know if we should identify it in some way or just make it seamless.

Lots of discussions on this ticket, feel free to read to get a better idea of the scenario. Solution is prescribed here.

Steps To Reproduce

1. Create a new image
2. Use it in a workbench creation
3. Delete it

See the table as Unknown -- this is effectively the effort behind upgrades.

Workaround (if any)

No response

What browsers are you seeing the problem on?

No response

Open Data Hub Version

Dashboard: v2.11.0

Anything else

No response

[andrewballantyne]

@kywalker-rh / @vconzola Thoughts on if we should denote "images that are old and untracked"? Essentially when we keep up-versioning images, the oldest ones fall off and the table represents them as Unknown -- but we should probably at least keep the name, but would we want to perhaps show "(deprecated)" or something like that beside it? Essentially the image as the name has either been deleted or the tag has moved on and you can't recreate this workbench anymore.

Or do you guys just feel we shouldn't do anything and just leave it as-is. On Edit, we won't be able to select the image because it does not exist anymore.

[vconzola]

@andrewballantyne What happens to existing workbenches that are using "old" images? Do they automatically get updated to use the newer version of the image?

[andrewballantyne]

@vconzola Nope, nothing gets automatically updated when it is created by the user; such as Workbenches. The items tracked by the Operator (our OOTB images for instance) are the only ones that get "auto updated".

The way we render the name is a live representation to what is on the cluster (eg. the Workbench stores "I use image x, on tag y" then we ask image "x:y", what is your name and render that) -- which when we do the n / n+1 upgrades, eventually falls off the cluster. Thus the name is "Unknown". Anything using it works fine because it's already in the definition of the pod, but you cannot create more with that image; you'd need to use a later one.

[shalberd]

@vconzola @andrewballantyne

I have never seen image names (really imagestream names) such as jupyter-datascience-notebook getting deleted in the master branch. What has changed though was imagestream tags getting deleted and replaced, e.g. from py3.9 to 2023.1. This is probably what you mean. A main conceptual problem is that you all intuitively refer to images when you really mean imagestream tags (which have images behind them, coming from quai.io specified in tag.from.name).

The only time an imagestream can really be deleted is in the case of BYON, importing your own notebook image, which results in an imagestream, which you can then delete. You cannot really expect imagestream or image tag details such as package versions to still be available then.

First of all, which version of odh-manifests are you working with? The master branch or a version branch, e.g. v1.6?
I think you are mixing a lot of things conceptually here, stemming from user complaints or questions.

@vconzola regarding "at least keep the name (in the details tab)": putting imagestreamname:imagestreamtag in the pod env variable JUPYTER_IMAGE will do that. But ONLY as long as the imagestream names are non-changing, i.e. no imagestreams are deleted or renamed in any way.

odh-dashboard/frontend/src/utilities/imageUtils.ts

Line 168 in b6f4c30

const image = images.find((image) => image.name === imageTag[0]);

I agree with Andrew that the name rendering is a live representation of what is on the cluster, i.e. which imagestreams and which tags they have.

You have to be careful not to confuse images with imagestream name plus tag. I see what you mean by the name getting deleted ... at least, the tag was recently deleted and changed in the manifests master branch (i.e. the recent tag name changes of imagestreams from v.3.9 to 2023.1 or so)

in which case there is no way you can get the infos on the old imagestream tag, because it was simply overwritten.
Same for OOTB Imagestreams that get renamed or deleted. There just is no way to still get all the metadata (package versions, all that stuff) after that point in time then.

The reason notebooks keep running: they run with the local copy of the image on the openshift node. When the notebook restarts for any reason, let's say notebook server is stopped (replica: 0) and started (replica:1) again, then the image is pulled again due to the container's imagePullPolicy: Always.

@vconzola @harshad16 There are ways to keep the container image up to date, a combination of using image change trigger annotation with paused: false and imagestream tag importpolicy: scheduled: true. That way, your container restarts when changes from quai.io ripple though then.

@VaishnaviHire wanted to avoid notebook restarts, that'd be either possible by not making Harshad's changes (odh manifests PRwith tag importpolicy scheduled: true) or by setting image change trigger annotation to paused: true after the notebook is initially created.

You basically have to keep in mind that weekly image changes at quai.io and long-running notebooks are kind of in conflict.

@harshad16

Regarding up-versioning images and the oldest ones falling off: That is currently most apparent when the image behind a recommended imagestream tag gets updated at quai.io, imagestream defs are applied anew to the cluster, and there are somewhere long-running notebooks. In that case, the imagestream (in master referring to the image by tag, e.g. cuda-jupyter-weekly) refers to the latest digest at time of imagestream creation in quai.io. Older created, still-running notebooks refer to old digests at the time they were created. That is fine as long as the digest behind the image is still there in the internal openshift registry. Usually, the last three digests for an image behind an imagestream tag are kept in the internal registry.

You really need to formulate your expectations regarding past spawned notebooks in terms of use cases, especially with regards to the fast-changing (in terms of content / digest) images with a weekly tag at quai.io and with regards to the master branch of odh-manifests, which is what your developers are using.

Whee the imagestream tag points to an image tag at quay.io, e.g. blabla-weekly, by definition we always get the digest of the image for the time the imagestream was created. When the digest / weekly image at quay.io gets updated, the old digest is not usable anymore.

@atheo89 try if at all possible do not change existing imagestream tags in future, as those imagestream names and tags (not image-tags) are used by odh dashboard.

My recent proposal PR-800, initially with regards to image-field values in digest format, of storing the initial imgestream name and tag value in the env var JUPYTER_IMAGE might at least help keep the imagestream name and tag persistent, but true, you will loose access to older -weekly image versions as the -weekly tag in external quay.io moves on currently.
We could, as described above, have an option for weekly / recommended / n and n-1 type images to set imagestream trigger to paused: true.

Pointing to an sha256 digest (the image trigger annotation always does that, even if from.name has a tag), which is also part of my proposal, can alleviate that problem by not pointing to the weekly tag in the internal openshift repo, which always points to the latest digest, but to the digest, which is kept in cache for a while ( last three digests values). (keepTagRevisions)

@harshad16 suggesting to use referencePolicy: type: Source for the "recommended" n n-1 imagestream tags is ok because you always softlink requests for the image to the external repo that way.

A lot of info, I realize. My main point at the end: do not expect to reliably use long-running notebooks with development images, i.e. those with weekly tag at quai.io. If you want to keep those running long-term, you have to use the possibility to set images to paused: true with the image change trigger annotation and you have to make sure to use the Openshift-internal registry cache, meaning referencePolicy: type: Local for a tag. Also, do not try to handle imagestream image versioning and image-field values with ODH Dashboard logic, let the image change trigger plug-in of Openshift handle that.

[kywalker-rh]

but would we want to perhaps show "(deprecated)" or something like that beside it?

@andrewballantyne Yes, I think that's a good idea. Deprecated even makes sense to me, although I'm open to suggestions if @kaedward thinks there is a better word for an image that is no longer supported.

[kaedward]

@andrewballantyne @kywalker-rh In RHOSAK, we used "Degraded" to refer to an instance that is experiencing a non-terminal error. It sounds like if this image is no longer supported, though, that "deprecated" might be more accurate. So thumbs up from me :)

[andrewballantyne]

Thanks for the UX Input @kywalker-rh & @kaedward let me try to figure out how to make use of it.

Okay, so I followed most of what you mentioned @shalberd -- here is my suggestion, let me know if I'm over complicating this.

1. This solution will not apply to Jupyter tile as they don't quite refer to things the same way, and we have other objectives to improve that
2. On Workbench create/update, we add the annotation opendatahub.io/image-display-name and at that time, determine based on the image selected, what we will use
3. We update our Workbench table to use that
4. On render of the Workbench table rows, we will attempt to resolve the image TAG (😉) -- if we can't find it, we will suffix the display name with (deprecated) -- eg. Pytorch (deprecated) -- might need a use case or two done to make sure it looks alright to the UX folks

That way it's better handling the "no reference" as well as the on-going "there is a reference" scenarios. On Edit of the workbench, we just need to make sure we don't "break" -- the user will need to reselect an image anyways.

[shalberd]

Hi @andrewballantyne

This solution will not apply to Jupyter tile

I am not sure what that means, it seemed to me so far you had similar logic for looking up imagestream name and tag from the container image field in backend and frontend. My main point there was that it is impossible to get the imagestream tag if ever the container image field value has sha256 digest notation, which would happen if we use the image change trigger plugin. But regarding notebook spec assembly and so on, I see no difference between the jupyter tile backend and dsp frontend logic.

On Workbench create/update, we add the annotation opendatahub.io/image-display-name and at that time, determine based on the image selected, what we will use

Where do you want to add that annotation? In the pod spec of a notebook or in the notebook metadata annotations part? I assume "workbench create/update" is the assembleNotebook / updateNotebook part, where the notebook CR yaml is constructed. If yes, then there already is a notebook CR metadata annotation called notebooks.opendatahub.io/last-image-selection that contains the last selected imagestream name and tag, e.g. jupyter-datascience:2023.1. up to now, the variable imageSelection that contains that string is also used for that annotation, in function assembleNotebook. That annotation does not ripple through to the StatefulSet and Pod, though, because notebook controller does not add them on reconciling the statefulset spec with the notebook spec.

If you want to put the human-readable name into that new notebook annotation, sounds good.
Do you mean notebook-image-name from the imagestream base spec? That is the name for all versions / tags of the imagestream. https://github.com/opendatahub-io/odh-manifests/blob/master/notebook-images/base/jupyter-datascience-notebook-imagestream.yaml#L8

My PR 800 with the new mechanism for container image field lookup from imagestream name and tag, independent of the presence of an openshift-internal docker registry, but works with the internal registry if present, ever makes it into production, has imagestreamname:imagestreamtag e.g. jupyter-datascience:2023.1 in env variable JUPYTER_IMAGE, that would also then have imageSelection, e.g. jupyter-datascience:2023.1 in it as a value. But that is of course not the same as the tag-independent, but imagestream specific name opendatahub.io/notebook-image-name: "Jupyter Data Science"

The advantage of putting imagestream name and tag e.g. jupyter-datascience:2023.1 in env var JUPYTER_IMAGE is that it ripples through all the way to the notebook pod container at the end.

Fully ACK on point 4.

[atheo89]

Hi all,

After trying various combinations to reproduce the bug, the sources of truth, in terms of which image and tag were initially used to create a specific notebook, are as follows:

• On the notebook CR by the notebooks.opendatahub.io/last-image-selection annotation (as mentioned on the comments):
  oc get notebook $notebook_name -n $notebook_namespace -o jsonpath='{.metadata.annotations.notebooks\.opendatahub\.io/last-image-selection}'
  output: s2i-minimal-notebook:1.2-new

• On the StatefullSet container.image:
  oc get statefulsets -n $notebook_namespace -o jsonpath='{range .items[*]}{.spec.template.spec.containers[*].image}{"\n"}{end}'
  output: image-registry.openshift-image-registry.svc:5000/redhat-ods-applications/s2i-minimal-notebook:1.2-new registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4bef31eb993feb6f1096b51b4876c65a6fb1f4401fee97fa4f4542b6b7c9bc46

• On the JUPYTER_IMAGE env into notebook CR & StatefullSet
  value: image-registry.openshift-image-registry.svc:5000/redhat-ods-applications/ s2i-minimal-notebook:1.2-new

Furthermore, even if you remove the associated tag from the ImageStream, the utilized workbench continue to work as expected (checked also the case to change its status from Running to Stopped and vice versa). However, the field for the notebook image still displays unknown.

Considering that there are alternatives available to identify the image:tag, is it necessary to include the opendatahub.io/image-display-name or the above sources are enough? If not, let us know where and what could be the format of the opendatahub.io/image-display-name annotation.

[shalberd]

@atheo89 @harshad16 The problem is in that case, the imagestream tag or even the imagestream name itself changed, possibly even deleted, similar to here (tag name changes) .... there is no way to get the related info when an imagestream yaml was changed. This is purely a matter of fast-changing master-branch or comparable dev changes in my view, not a conceptual problem.

Is the imagestream s2i-minimal-notebook with tag 1.2 still present in the namespace?

image-registry.openshift-image-registry.svc:5000/redhat-ods-applications/ s2i-minimal-notebook:1.2 is cached, not pruned yet in the internal registry. That explains why the pod container is still running. But I am almost 100% certain that in this scenario, the imagestream tag changed, or the imagestream itself was deleted ... it is a lookup of all these annotations for a given tag ...

[shalberd]

The current tag-based notation is not suitable for on-prem and mirrored environments anyways, because it does not use sha256 notation. In adddition, it also always references the internal openshift registry, even when no internal openshift registry is present. That's why my proposed change in PR-800 But even that PR would not have solved the problem of image metadata getting lost when either the imagestream metadata.name changes or the tags in an imagestream change.

[atheo89]

@shalberd oh, sorry I corrected my comment above, I experimented with 1.2-new tag not 1.2. I tested with that changed tag then I deleted it and checked again, and all the above mentions shows the tag that the image has been created a d it seems that it persists.

[shalberd]

Yeah, in my approach with the image content change trigger annotation on the Notebook CR / StatefulSet CR, you'd have to update that annotation with the changed tag, too.

The thing is: dashboard rightfully does not expect imagestream tags to change ... how would it be aware of those changes to tags in an imagestream? As mentioned, even my annotation approach assumes the imagestream tag does not change. Usually, tags get added to an imagestream, not changed or modified. docker.from.name tags and/or digests can change, but tags are supposed to stay in place and unchanged.

[shalberd]

And even if you continuously checked an imagestream tag's status conditions

https://docs.okd.io/4.11/rest_api/image_apis/imagestream-image-openshift-io-v1.html#status-tags-conditions

i.e. transitionTime for event ImportSuccess via the odh notebook controller (kubeflow notebook controller does not even have that annotation), even then you'd have to decide what to update the notebook with based on
last-image-selection
https://github.com/search?q=repo%3Aopendatahub-io%2Fkubeflow%20last-image-selection&type=code

1.2-new it was created with, now it is 1.2 ... but how would you know that it now needs to refer to new tag 1.2?
There is no status condition AFAIK that tells us about the fact that an imagestream tag changed. ImportSuccess refers to the import of the image behind a tag, not to the tag name change event itself.

Might just as well be another tag 1.3 or 2023-stable or whatever in the imagestream tags array and tag 1.2 is not there anymore in the imagestream yaml ... plus an imagestream tag alone says nothing about the underlying image.

[shalberd]

I use a different function to get all the image details, from env JUPYTER_IMAGE containing now the value
imagestreamname:imagestreamtag, mainly because the image-field when using the image content change trigger contains sha256 digests always. But ... that is conceptually the same as image-registry.openshift-image-registry.svc:5000/redhat-ods-applications/ imagestreamname:imagestreamtag in how dashboard parses image information itself. Meaning that if the old image-tag is present in the container field, or in my case, the JUPYTER_IMAGE env var, you still don't get the details of the tag for the imagestream ...

In the meantime .. I will check my way of handling imagestreams on how the image change trigger reacts to changes in imagestream tag names themselves, meaning mostly, whether the container keeps running.
Another topic: in the notebook container section, imagePullPolicy is currently set to Always .. why do we not use the node / VM cache of Openshift and set the imagePullPolicy to IfNotPresent? Internally, imagePullPolicy works with the sha256 digest, too, so it'd save round trips to the internal docker registry, plus, would make images being pruned from the docker registry less severe of an impact.

[andrewballantyne]

I don't think it was addressed in the comments here... but opendatahub.io/image-display-name is a Notebook annotation. It's for the Dashboard to handle the name. If a Notebook is created outside of the Dashboard, and the image reference is no longer available, it will not reflect... hence Unknown is accurate.

We could theoretically try a fallback operation here and look at the image-stream to try and figure out a display name. Most likely case here is the tag slips off the cluster, not the whole image stream. But in the BYON use-case, you could lose the entire image-stream -- of which all data is lost.

We can do this through a few stages and layers. To make this one simple, we could do my image-display-name idea and just have metadata we can pull upon. We can improve the fallover point for the metadata by using the image-stream and ignoring the current tag (if it's outdated). That'll just improve the (deprecated) use-cases.

[shalberd]

@harshad16 Yes, I just tried the same thing with my PR-800 image change trigger annotation.

First referenced an image with an imagestream tag. Notebook starts up. Then changed that imagestream tag in the imagestream spec to a new value.
Notebook keeps running because the old tag, though not in the imagestream spec.tags anymore, is still in the imagestream status section, containing the image name and same digest.

Also, and that is even better, deleting the whole imagestream (what dashboard calls image) also leads to the notebook still running. Really good for stability.

But not more than that. So, even there, no more details on the existing tag in odh dashboard. Makes sense because the spec.tags[x] with all the details is not present for the old tag.

I agree with @andrewballantyne that in such a case, setting the image name to Unknown in dashboard image details is a good idea. That does not seem to be the case yet in the classical Jupyter tile, see my picture. I could also imagine just showing the content of imagestream annotation image-display-name, e.g. Jupyter Data Science, there, but not reading in the additional details on packages used and so on in case the imagestream tag name changed. If the imagestream yaml itself is gone and deleted, that's a different story. In that case, Unknown is most definitely warranted.

You just won't have the additional info (packages, software and so on) anymore that was associated with the old imagestream tag.

image-display-name is not really the image, though ... it is the imagestream info, https://github.com/opendatahub-io/odh-manifests/blob/master/notebook-images/base/jupyter-datascience-notebook-imagestream.yaml#L8 like Jupyter Data Science.

@DaoDaoNoCode About the idea looking at imagestream status tag conditions: those conditions are not showing up at any time in my use case at least, I made so many changes to the imagestream defs and tags, there was never any sign of a conditions section in the imagestream status. At least not when not using the internal docker registry of Openshift.

To further increase stability, we should change the imagePullPolicy from Always to IfNotPresent in the notebook spec. Especially when image-fields of containers in future always have sha256 notation, that change would ensure that should a container restart for any reason, the local copy on a given openshift node could still be used on next container start, regardless of whether the image is still in the internal openshift docker registry or external registry (could be it's been pruned). Even when the image is present in the registry, this setting prevents unnecessary pulls from the image registry.

https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/api/k8s/notebooks.ts#L103
https://github.com/opendatahub-io/odh-dashboard/blob/main/backend/src/utils/notebookUtils.ts#L281

[shalberd]

if my change in PR-800, putting imagestream name and tag in env var JUPYTER_IMAGE i.e. jupyter-datascience:py3.9, were approved, then we could actually deprecate (not remove) annotation notebooks.opendatahub.io/last-image-selection and fill the new annotation opendatahub.io/image-display-name with the value from the imagestream annotation anywhere just the image name (not tag) in human readable form is required (Jupyter Datascience, R Studio). Meaning in the code, lastSelectedImage could come from container env var JUPYTER_IMAGE in future.
We will keep both, though: annotation notebooks.opendatahub.io/last-image-selection and env var JUPYTER_IMAGE for downstream, which have the same content in any case.

Same for the proposal of getting lastSelectedImage new from env var JUPYTER_IMAGE ... we'd have to either parse verbatim if there is the new value imagestreamname:imagestreamtag in it. Or a bit more complicated, only getting the last part of myopenshift.svc:5000/imagestreamname:tag for older, still-running notebook CRs.

I opted for that most flexible approach in the part of dashboard that gets imagestream name and tag details in frontend and backend. No more parsing it from container image-field value, as that is in future im sha256 digest format, which does not have imagestream tag in it.

I am thinking of integrating proposed changes in my PR-800 soon. We can then "kill multiple birds with one stone".
The main challenge will be working logic for both old existing notebook CRs and new notebook CRs. That is what is important. A tracking boolean property to determine whether the new image trigger change annotation is used and present in a notebook CR and then using that tracking property to have working logic for both new and old Notebook CRs is probably the best approach.

[shalberd]

@VaishnaviHire @harshad16 @andrewballantyne @LaVLaS on second thought, though, this here is an enhancement for tracking imagestream metadata on the GUI. Let's proceed step by step.
It'd be nice if we could finally pull in the changes of PR-800 dashboard and PR-72 KF notebook controller.

[shalberd]

@andrewballantyne

To make this one simple, we could do my image-display-name idea and just have metadata we can pull upon. We can improve the fallover point for the metadata by using the image-stream and ignoring the current tag (if it's outdated). That'll just improve the (deprecated) use-case

Full ACK, good idea. We will cover the most cases this way, as you described succinctly and as I was meandering through :-)

[harshad16]

I finally caught up with the thread.

With the imagestream tag deletion, the imagestream reference would be pruned from the internal registry with the garbage-collection runs, though as the workbench pod was already running, as the container was started, it would
only start to fail if the notebook somehow gets restarted or re-kicked.
Which I feel should be the standard, as it is on the user who deleted the tag, and still kept a workbench referenced
to the deleted tag.

To make this one simple, we could do my image-display-name idea and just have metadata we can pull upon. We can > improve the fallover point for the metadata by using the image-stream and ignoring the current tag (if it's outdated). > > That'll just improve the (deprecated) use-cas

@andrewballantyne from this comment, I take the action items as, including image-stream annotation image-display-name on the OOTB notebook image, if not present already.
For the imagestream from BYON, it is done from the dashboard code, so it could be handled from there.

Please let us know, if any further assistance is needed.

[harshad16]

on second thought, though, this here is an enhancement for tracking imagestream metadata on the GUI. Let's proceed step by step.
It'd be nice if we could finally pull in the changes of #800
dashboard andhttps://github.com/opendatahub-io/kubeflow/pull/72 KF notebook controller.

@shalberd ack, i think as you described this seems to be a great feature to have and is responding a lot of the concerns around the imagestream.
Will include it in the KF controller.

[shalberd]

@andrewballantyne @harshad16 what is important is that those changes in kubeflow notebook controller are rolled out, the build image tag/digest reference, to odh-manifests in parallel. Once PR-800 is merged and used if dashboard, it only works in conjunction with the newest v1.6 version of kubeflow notebook controller. So, whatever the new release of odh-dashboard including Pr-800 is, it needs to go together with the to-come release of kubeflow-notebook-controller, e.g. v1.6.1-3

[shalberd]

With the imagestream tag deletion, the imagestream reference would be pruned from the internal registry

Eventually, yes. We could think about setting notebook container imagePullPolicy to IfNotPresent, which is a sensible change from Always, but at some point, even the cache on then Openshift nodes will not be there anymore.

[harshad16]

Ack, we will switch to imagePullPolicy IfNotPresent.

[shalberd]

from this comment, I take the action items as, including image-stream annotation image-display-name on the OOTB notebook image, if not present already.

@andrewballantyne there is an imagestream annotation for the imagestream / image called opendatahub.io/notebook-image-name I am not familiar with the BYON code for assembling imagestream, but that could be useful, right? You could add a notebook CR annotation opendatahub.io/image-display-name on that.

[shalberd]

@harshad16 @andrewballantyne @lucferbux changed imagePullPolicy to IfNotPresent in Notebook CR assembly. Also in some mock notebooks, except Deployment nvidia-dcgm-mock-exporter file, which uses an image with :latest tag, for which "Always" is needed. Regarding BYON: what if someone were to bring their own notebook image with a latest tag image behind it, and then has an imagestream again, same name and imagestream tag, with a new latest tag: Don't fret, as with PR-800, image-field value of container is in sha256 digest format anyways, that is, latest tag in from.name would change to a new digest and current ":latest" image would be pulled even with "IfNotPresent"

[andrewballantyne]

This is still having friction and I'm bumping the priority so we can get to this sooner vs later.