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
vwa(front): Prevent PVCs from being deleted when there is a corresponding notebook #6899
Merged
google-oss-prow
merged 7 commits into
kubeflow:master
from
arrikto:feature-orfeas-vwa-prevent-pvc-being-deleted
Jan 30, 2023
Merged
vwa(front): Prevent PVCs from being deleted when there is a corresponding notebook #6899
google-oss-prow
merged 7 commits into
kubeflow:master
from
arrikto:feature-orfeas-vwa-prevent-pvc-being-deleted
Jan 30, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Append notebooks using each PVC when getting and parsing all PVCs. Refs arrikto/dev#2017 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Create custom delete column component that extends ActionComponent from Kubeflow common code and adds the following functionalities: * Disable the button when a row's notebooks array is not empty * Display an appropriate message including the notebooks' names Refs arrikto/dev#2017 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Implement custom delete column in the table in VWA's index page. Refs arrikto/dev#2017 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Add a Used by column in volumes table of VWA's index page in order to link to the notebooks that are using each PVC. Refs arrikto/dev#2017 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Update mock request for PVCs Refs arrikto/dev#2017 Refs arrikto/dev#2135 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>
Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>
orfeas-k
force-pushed
the
feature-orfeas-vwa-prevent-pvc-being-deleted
branch
from
January 19, 2023 10:07
7b6e55b
to
e142f72
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kimwnasptd The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kimwnasptd
pushed a commit
to arrikto/kubeflow
that referenced
this pull request
Feb 15, 2023
…ding notebook (kubeflow#6899) * vwa(back): Append notebooks using each PVC Append notebooks using each PVC when getting and parsing all PVCs. Refs arrikto/dev#2017 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * vwa(front): Create custom delete action column Create custom delete column component that extends ActionComponent from Kubeflow common code and adds the following functionalities: * Disable the button when a row's notebooks array is not empty * Display an appropriate message including the notebooks' names Refs arrikto/dev#2017 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * vwa(front): Add delete button component in index page Implement custom delete column in the table in VWA's index page. Refs arrikto/dev#2017 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * vwa(front): Add Used by column in volumes table Add a Used by column in volumes table of VWA's index page in order to link to the notebooks that are using each PVC. Refs arrikto/dev#2017 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * vwa(front): Fix integration tests Update mock request for PVCs Refs arrikto/dev#2017 Refs arrikto/dev#2135 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * vwa(front): Fix format error Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> * web-apps(front): Fix linting errors Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> --------- Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>
kimwnasptd
pushed a commit
to arrikto/kubeflow
that referenced
this pull request
Feb 15, 2023
…ding notebook (kubeflow#6899) * vwa(back): Append notebooks using each PVC Append notebooks using each PVC when getting and parsing all PVCs. Refs arrikto/dev#2017 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * vwa(front): Create custom delete action column Create custom delete column component that extends ActionComponent from Kubeflow common code and adds the following functionalities: * Disable the button when a row's notebooks array is not empty * Display an appropriate message including the notebooks' names Refs arrikto/dev#2017 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * vwa(front): Add delete button component in index page Implement custom delete column in the table in VWA's index page. Refs arrikto/dev#2017 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * vwa(front): Add Used by column in volumes table Add a Used by column in volumes table of VWA's index page in order to link to the notebooks that are using each PVC. Refs arrikto/dev#2017 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * vwa(front): Fix integration tests Update mock request for PVCs Refs arrikto/dev#2017 Refs arrikto/dev#2135 Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * vwa(front): Fix format error Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> * web-apps(front): Fix linting errors Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com> --------- Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Right now, VWA checks whether a PVC is mounted to a Pod. If not, then it allows users to delete the PVC. When a user stops a notebook server, then the notebook Pod gets deleted. Thus, VWA will allow a user to delete a PVC that used to be mounted to the notebook. This results in the corresponding notebook not being able to start.
Goal
We want to prevent a user from being able to delete a PVC, even when the corresponding notebook is stopped and there is no Pod currently mounted to it.
UX
The ideal UX would be to check for all PVCs which of them can be deleted and disable the delete buttons for the ones that the user cannot delete, so they cannot click an action that we are able to know beforehand it will fail. However, checking if a PVC can be deleted requires checking for mounted pods, as well as CRs (notebooks, inference services etc) that may use the specific PVC even if there is no pod right now (e.g. a stopped notebook). Currently, checking only for mounted pods and notebook CRs takes around 1-1.5s. Since, this approach, in VWA's index page, would require checking for all PVCs beforehand, we see that this implementation may create a huge delay. We would be able to overcome this delay if we had some caching done in the backend. Since, then, we cannot have the ideal UX effectively, this PR gets as close we can to it. That means that it implements the disable beforehand delete icons approach but we will be checking only for notebook CRs using a PVC. This means, that delete buttons will be disabled for PVCs that are being used by at least one notebook (even a stopped one). At the same time, we will expose the following tooltip over a disabled delete icon
while we will also add a
Used by
column, that will contain links to the notebooks using each PVC.Screenshots
Disabled butons for PVCs used by notebooks +
Used by
column with truncated links to these notebooksTooltip for disabled buttons
Popover card tooltip for truncated links