feat: PB-7779 implement cluster delete to support cluster share feature #252
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
|
OSS Scan Results:
Total issues: 43 |
|
License Evaluation Results:
Total License Issues: 0 |
t-nidhin
approved these changes
Aug 9, 2024
siva-portworx
approved these changes
Aug 9, 2024
| @@ -1323,6 +1329,9 @@ message ClusterDeleteRequest { | |||
| bool delete_restores = 4; | |||
| // indicate the uid of the particular object | |||
| string uid = 5; | |||
| // delete_all_cluster_backup if set will delete all backups on the cluster | |||
| // allowed only to super admin Role | |||
There was a problem hiding this comment.
I feel, we need to change the comment of the delete_backups as well. Also comment looks more generic for delete_all_cluster_backup. I pefer,
// delete_backups indicates whether all the backup created by cluster owner and belong to the cluster need to
// be deleted or retained. This can be set, when cluster delete was trigged by user with
// any role
bool delete_backups = 3;
// delete_all_cluster_backups indicates whether all the backup owned by cluster owner and belong to the
// cluster need to be deleted or retained. This will be set only, when cluster delete was trigged by user with
// SuperAdmin role
bool delete_all_cluster_backups = 6;
There was a problem hiding this comment.
Some thougths
- Are we going to handle the case when both are set? What would be the behaviour?
- If a superadim creates the cluster, can he use any of the flags?
- When a super_admin is using the
delete_all_cluster_backupsflag, should we show warning msg that other users also have created the backup and that would be deleted? Just a warning to the super_admin but not stop him from deleting.
There was a problem hiding this comment.
delete_backups - this will be used by any user ( superAdmin or non superAdmin user). With this flag, we will delete only the backup created by the user and that belongs to the cluster.
delete_all_cluster_backups - this will be used by only SuperAdmin user. With this flag we will delete all the backup that belongs to the cluster, even if it is not created by the user who initiates the delete. We will return error, if non-admin user set the delete_all_cluster_backups. If Super Admin set both the flags, delete_all_cluster_backups will take priority.
Also warning can be given only in the UI and in the UI, currently these flags are not used.
Warning in the API is not possible. So for API, we should document the purpose of the flags clearly.
There was a problem hiding this comment.
Agree with @siva-portworx. This is not supported from UI as of now. Only from API and more over this is not default. It is something that super admin uses it cauteously. Because for default its false and for deletion of his own backups is delete_backups which has clear swagger message and further on the delete_all_cluster_backups is explicit flag allowed only to super admin. With all these I dont think we should have warnings.
There was a problem hiding this comment.
As we know from UI it will never be used, taking a step back I m thinking do we need to really support this (even the older one). It might be good time now to clean it up. As we don't know who is using it(I doubt anyone is using it) we can always call out this is deprecated. Lot of engineering to support which is hardly used
Signed-off-by: vsundarraj-px <[email protected]>
|
OSS Scan Results:
Total issues: 43 |
|
License Evaluation Results:
Total License Issues: 0 |
vsundarraj-px
added a commit
that referenced
this pull request
Sep 2, 2024
…re (#252) Signed-off-by: vsundarraj-px <[email protected]>
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.
What this PR does / why we need it:
This PR add two new flags to ClusterInfo deleted_by and delete_all_cluster_backup.
WIth cluster share we allow cluster deletion by cluster owner and also SuperAdmin. This deleted_by will identify whose backups should we delete while deleting the cluster with option deleteBackups.
Similarly delete_all_cluster_backups field is introduced to cleanup backups created by all shared users including the other super admin user. this field is allowed to be executed only by super admin.
Which issue(s) this PR fixes (optional)
Closes #
PB-7779
Special notes for your reviewer:
Unit testing results will be shared with px-backup PR