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

Disabling a custom Notebook Image from view makes its Workbenches display an Unknown image #1520

Closed
erwangranger opened this issue Jun 28, 2023 · 24 comments · Fixed by #1872
Closed
Assignees
Labels
feature/ds-projects Data Science Projects feature (formerly Data Science Groupings - DSG) field-priority Flag to track improvements that are for stability -- effort to put in front of new functionality kind/bug Something isn't working priority/high Important issue that needs to be resolved asap. Releases should not have too many of these. rhods-2.5

Comments

@erwangranger
Copy link

erwangranger commented Jun 28, 2023

Status

Might be resolved by:

If not, this ticket will need attention.


After disabling a custom image, it's still visible in the workbench, but its image is "unknown".
image

To Reproduce:

  1. add a new custom image. Call it "CUSTOM - my image"
  2. create a DS project
  3. Create a workbench in the project, using that image
  4. Project page displays the right Image name
  5. Now, go back to the admin section and disable (not delete) the image
  6. Go back to the data science project. (and/or reload the page)
  7. The "notebook Image Column now displays "Unknown".
  8. But you can still start that workbench.
  9. You just cannot create a new workbench with the same Image anymore as it is disabled.

additional notes:

  • re-enabling the image makes its name show up in the project again.
  • fully deleting the image in the admin section for Custom images, also makes the DS project display "unknown", but makes it (expectedly) impossible to spin up the workbench:

image

@andrewballantyne
Copy link
Member

andrewballantyne commented Jun 29, 2023

cc @harshad16

We are talking about how we can fix the naming of Unknown, and there were a few mentions of not forcing pulling every image -- which could help a little bit with the ImagePullBackOff problem.
The discussions happened here: #1370

Our solution today looks to be the Dashboard just tries a little harder to hold onto the name and give a better idea of it's still existence.

Does beg the question, could we keep tags for a long while around being pulled but no longer selected. The UI can show that we have moved away from that image tag, but your notebooks would still spawn with an "unavailable" image stream tag.

@andrewballantyne
Copy link
Member

That being said -- obv deleting the image (in this case our BYON images -- not the OOTB ones) should prevent it from spinning up I imagine. Because if you deleted the image stream from the cluster, I think it is fair to not allow it to spin up.

@shalberd
Copy link
Contributor

shalberd commented Jun 30, 2023

could we keep tags for a long while around being pulled but no longer selected

At least for the image digest reference that is possible. When deleting or modifying a tag (the tag, not from.name of it) in the imagestream spec, the old tag with the image reference is persisted in the imagestream status section. So notebooks keep running (already spun-up workbenches in dashboard lingo).

If the entire imagestream (image in dashboard lingo) is not deleted, then the name from that annotation in imagestream could be put into a Notebook CR annotation and then persisted that way. All tag details (software package versions and so on) are lost, though, in case an imagestream tag changes.

I am talking about my PR-800 approach here, have tried it out.

Yes, deleting the imagestream / image (imagestream mods) between selection and startup keeps it from spinning up anew, but the already running notebooks are not affected, they run either with their internal docker image registry reference still there (unless pruned) or with the external docker image reference in sha256 format that then links via a different mechanism to the enterprise docker registry (on a scenario where internal docker registry is not used).

obv deleting the image (in this case our BYON images -- not the OOTB ones) should prevent it from spinning up I imagine

Yeah, one cannot delete or modify the OOTB imagestreams, they come with odh-manifests and are synced up by ODH operator.

The list of images and tags available in dashboard, if it gets refreshed in real-time, we will never have an issue with a user selecting an image whose imagestream or specific version / tag has already been deleted or disabled.

@shalberd
Copy link
Contributor

shalberd commented Jun 30, 2023

Great idea about changing imagePullPolicy to "IfNotPresent". Also, it would be good if BYON imagestreams always had sha256 digest notation in tag.from.name, regardless of whether a user provides a BYON image url with a tag or with a digest.

If that lookup to digest is not possible in odh dashboard code, that is not so bad. Because the image change trigger annotation way of looking up the container image value for a given imagestream name and tag always resolves the image-field on the container to sha256 digest notation, even if tag.from.name string / url is in tag-format.

If the user provided a digest format image url, then there'd have to open an input textbox allowing the user to set the wished tag (of the imagestream).

Have a good weekend :-)

@harshad16
Copy link
Member

@andrewballantyne , i feel , we could probably do
soft delete of tag, include an annotation , image-deleted: boolean?
based on this , from UI if we delete or disable the image,
we can switch to not showing the tag in UI and then the user won't be able to view it.

from the notebook itself, i m not sure, how we can do this.
as BYON is currently just importing the imagestream and executing that particular execution.

@shalberd
Copy link
Contributor

shalberd commented Jul 3, 2023

kinda like a mark-for-deletion of a given tag, yes, not actually deleting the tag from spec at first. And then in a different GUI, once it is certain no notebooks running on that tag are there anymore, delete the marked-for-deletion tag at a later point in time. Good idea.

@andrewballantyne
Copy link
Member

I'm going to move this to the Dashboard repo. I suspect the solution will be handled by the "store the notebook name" efforts we have already talked about (linked in an earlier chat) -- at least it won't be called "Unknown" at the worse case.

I'll leave it open in case the solution to the other issue doesn't quite solve this ideally.

@github-project-automation github-project-automation bot moved this to Needs prioritization in ODH Dashboard Planning Jul 14, 2023
@andrewballantyne andrewballantyne transferred this issue from opendatahub-io/opendatahub-community Jul 14, 2023
@andrewballantyne andrewballantyne added needs-info Further information is requested from the reporter or from another source feature/ds-projects Data Science Projects feature (formerly Data Science Groupings - DSG) untriaged Indicates the newly create issue has not been triaged yet labels Jul 14, 2023
@manaswinidas manaswinidas added priority/high Important issue that needs to be resolved asap. Releases should not have too many of these. field-priority Flag to track improvements that are for stability -- effort to put in front of new functionality and removed untriaged Indicates the newly create issue has not been triaged yet labels Jul 18, 2023
@manaswinidas manaswinidas moved this from Needs prioritization to To do in ODH Dashboard Planning Jul 18, 2023
@manaswinidas manaswinidas added the kind/bug Something isn't working label Jul 18, 2023
@lucferbux lucferbux modified the milestones: Current Release, Upcoming Release Aug 9, 2023
@andrewballantyne andrewballantyne removed this from the Current Release milestone Sep 15, 2023
@DaoDaoNoCode
Copy link
Member

@Gkrumbach07 Can you check if your PR #1872 could also resolve this issue?

@Gkrumbach07
Copy link
Member

  1. assign a workbench to a custom notebook image
  2. disable the image
  3. image will no longer be shown as Unknown, but as Image Display Name (deprecated)
  4. editing the workbench requires you to set a new image before submiting
  5. re-enabling the image removes the (deprecated) text
Screenshot 2023-10-02 at 4 44 08 PM

@erwangranger does this fix the bug you described?

@andrewballantyne
Copy link
Member

@kywalker-rh @xianli123 thoughts on the double brackets 🤔

@erwangranger
Copy link
Author

@Gkrumbach07 , yes, this is definitely better than the existing behavior.
so, I'm ok with that behavior, as-is, to be the first (and as-soon-as-we-can-have-it) step.

So, I say, ship it, as-is.

But if we wanted to then open another quick issue to make this even better, I would say the following should be its description.

  • using "deprecated" is a bit vague, because the image could be either disabled by the admin, or wholly deleted. So I'd recommend that we display a more meaningful word depending on the situation.
  • A disabled image could be re-enabled easily.
  • A deleted image could be re-created, but not easily.
  • While we inform the user of this, we could/should also inform the Admin of the impact of their changes. Sentences like "Disabling an image will mean that new Workbenches will not be able to use it anymore, but existing Workbenches will still be able to use them, although with a [disabled] markers added to them". And "Deleting an image will .... etc..."
  • it is understandable that if I change the image of the workbench, I can not pick one that's been disabled. It would be nice however to keep using even a [disabled] image if I'm just changing the size of the container. If I'm using a workbench that is usually "small" but on Fridays I need to make it "Large", if someone disables the image, I'm stuck on whatever size was used last. If I need to change the size, I have to "let go" of that image and use something else. And if I do, there is no way to go back to that image without asking the admin to re-enable it.

If there is time to take some of this into account without adding delay to the resolution, great. If not, the rest can wait.

@Gkrumbach07
Copy link
Member

Maybe not in time for this sprint, because UX will need to go over the proposed changes.

UX Context

@kywalker-rh, @xianli123, @vconzola This proposed change is a continuation to #1370 where we added the wording (deprecated) to images in the workbench table row if they could not be found (deleted or disabled). This issue is suggesting the following:

  • [@kywalker-rh, @xianli123] add Image Name (disabled) along with Image Name (deprecated) to the workbench table row.
  • [@vconzola] adding additional wording to the disable and deleting functions of custom notebooks to inform admins of the impact of their changes (delete or disable)
  • [@kywalker-rh, @xianli123] on edit of a workbench using a disabled image, allow the user to make edits without forcing them to select a new image

UX let me know if you have follow up questions

@erwangranger Does this capture what you were hoping to see?

@erwangranger
Copy link
Author

It does, yes. Thanks.

@kywalker-rh
Copy link

@Gkrumbach07 and @erwangranger... @xianli123 and @vconzola are out until next week. I would typically defer to them, but in order move this along a bit in the meantime, I agree with the changes for the first and the third bullet. The second one will take some exploration to figure out the best place to surface that text.

I support you moving forward with all the changes except adding the additional wording.

@kywalker-rh @xianli123 thoughts on the double brackets 🤔

@andrewballantyne It is a little strange, maybe a - deprecated would be more appropriate. I didn't realize we had more brackets in that field.

@andrewballantyne
Copy link
Member

@Gkrumbach07 give - deprecated a shot in your PR... see if that makes things look nicer.

Also I think we can combine these two issues together and merge your PR for both. If we need additional cleanup, we can log those separate. I think this PR context is a lot overlapping with your PR efforts.

@Gkrumbach07
Copy link
Member

@kywalker-rh why are we trying to work within text by using - deprecated. Would using a PF label be best? or will that make the row become to busy?

@andrewballantyne
Copy link
Member

@Gkrumbach07 mock it up 🙂

@Gkrumbach07
Copy link
Member

Screenshot 2023-10-04 at 5 49 30 PM Screenshot 2023-10-04 at 5 49 23 PM Screenshot 2023-10-04 at 5 50 41 PM Screenshot 2023-10-04 at 5 50 36 PM

@erwangranger
Copy link
Author

I like the red icon thing. (is that what PF label is?)

Everything else looks like it could be part of the image name. Which would also be problematic in case someone actually renames the image with - deprecated at the end of the display name.

@Gkrumbach07
Copy link
Member

@erwangranger Yes this is the docs for it. I also made the label compact for reference to UX.

We can also switch up the colors and make deprecated and disabled different colors (grey and red)
Screenshot 2023-10-05 at 8 05 15 AM

@kywalker-rh
Copy link

Thanks @Gkrumbach07... I really like the label, thank you for proposing it! I want to check on colors, red is definitely very obvious, but that may be good or bad. I'm going to check with the rest of the UX team and respond later today.

@xianli123
Copy link

Hi @Gkrumbach07 @kywalker-rh @andrewballantyne . I've looked through this issue and all discussions. I prefer labels to double brackets.

As for the wording, I think "Disabled" would be better than "Deprecated". The reason is that Disabled has an obvious antonym called Enabled. Users can easily understand that they or someone else can enable this image again. "Deprecated" is more like the result that the user may have violated some rule. Let's wait for others' opinions.

With regard to the label colors, PF mentioned that:

We recommend you avoid using the red label unless it indicates danger or an error state.

We usually use grey labels in our design to indicate the Disabled status. As for the Deleted label, we can use the label in orange or red.

In addition, I am not sure if it's possible or necessary to add a popover as shown below to tell users why the image is in that status. (The content of the popover is a rough placeholder.) What do you think?
Screenshot 2023-10-07 at 17 21 17

@xianli123
Copy link

This is the design based on our discussion in this issue and Slack thread. Please take a look. If there are any suggestions, please leave your comments.

To summarize the solution:

  • Add 2 labels named Disabled and Deleted. In addition, each label is clickable and can trigger a popover to explain the labels and tell users what they can do. These wordings were reviewed and suggested by @kaedward, our content designer.

  • When the notebook image has been deleted and the name is not able to track. It will use the “unknown” in grey color as the name.

@xianli123 xianli123 moved this from UX In Progress to Dev Ready in ODH Dashboard Planning Oct 13, 2023
@andrewballantyne andrewballantyne linked a pull request Nov 9, 2023 that will close this issue
7 tasks
@andrewballantyne
Copy link
Member

Completed by #1872

@github-project-automation github-project-automation bot moved this from Dev Ready to Done in ODH Dashboard Planning Nov 9, 2023
@andrewballantyne andrewballantyne added rhods-2.5 and removed needs-info Further information is requested from the reporter or from another source labels Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/ds-projects Data Science Projects feature (formerly Data Science Groupings - DSG) field-priority Flag to track improvements that are for stability -- effort to put in front of new functionality kind/bug Something isn't working priority/high Important issue that needs to be resolved asap. Releases should not have too many of these. rhods-2.5
Projects
Status: Done
Status: Dashboard
Archived in project
Development

Successfully merging a pull request may close this issue.

10 participants