PB-5834: API Changes -> DIRECT_KDMP for Backup Object Level #235
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: 26 |
|
License Evaluation Results:
Total License Issues: 0 |
dbinnal-px
approved these changes
Feb 26, 2024
prashanthpx
reviewed
Feb 26, 2024
| @@ -351,6 +351,8 @@ message BackupScheduleInfo { | |||
| bool skip_vm_auto_exec_rules = 27; | |||
| // volume snapshot class mapping for csi based backup <provisioner(string), volumesnapshotclass(string)> (optional) | |||
| map<string, string> volume_snapshot_class_mapping = 28; | |||
| // option to take backup as direct kdmp | |||
| bool direct_kdmp = 29; | |||
There was a problem hiding this comment.
Any reason we need this to be part of backupschedule object as backup driver info already has driver type.
There was a problem hiding this comment.
BackupSchedule gives values to Backup. So for per backup level direct kdmp we need to add in BackupSchdeuleInfo too
| @@ -534,6 +536,8 @@ message BackupInfo { | |||
| bool skip_vm_auto_exec_rules = 36; | |||
| // volume snapshot class mapping for csi based backup <provisioner(string), volumesnapshotclass(string)> (optional) | |||
| map<string, string> volume_snapshot_class_mapping = 37; | |||
| // option to take backup as direct kdmp | |||
| bool direct_kdmp = 38; | |||
There was a problem hiding this comment.
Now its per backup level direct kdmp we should store the value to understand driver easily
pkg/apis/v1/api.proto
Outdated
| @@ -1052,6 +1059,8 @@ message BackupScheduleUpdateRequest { | |||
| ObjectRef post_exec_rule_ref = 16; | |||
| // volume snapshot class mapping for csi based backup <provisioner(string), volumesnapshotclass(string)> (optional) | |||
| map<string, string> volume_snapshot_class_mapping = 17; | |||
| // option to take backup as direct kdmp | |||
| bool direct_kdmp = 18; | |||
There was a problem hiding this comment.
One confusion which might cause here is if a user was on PX vol and had incremental backups and in between he switches to direct KDMP then it might lead to confusing as what incremental mean now where already we have confusion with generic backup. I am thinking if a user wants to switch, we should ask them to create new schedule.
There was a problem hiding this comment.
If we change the driver for backup schedule now. Each new backup after change will be full backup and those previous incremental backup will be irrelevant . This should be documented
|
OSS Scan Results:
Total issues: 26 |
|
License Evaluation Results:
Total License Issues: 0 |
|
OSS Scan Results:
Total issues: 26 |
|
License Evaluation Results:
Total License Issues: 0 |
|
OSS Scan Results:
Total issues: 26 |
|
License Evaluation Results:
Total License Issues: 0 |
siva-portworx
approved these changes
Feb 29, 2024
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:
Which issue(s) this PR fixes (optional)
Closes #
Special notes for your reviewer: