-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Extend Volume Policies feature to support more actions #7664
Extend Volume Policies feature to support more actions #7664
Conversation
7f802b5
to
4e0de30
Compare
continue | ||
} | ||
// don't backup volumes that are included in the exclude list. | ||
if pdvolumeutil.Contains(volsToExclude, vol.Name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the condition pdvolumeutil.Contains
in this line of code duplicated with the same logic in ShouldIncludeVolumeInBackup
function? or it could not execute Line 129 for it will always execute L124 first if the matching condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shubham-pampattiwar please take a look before merging this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it should be like below?
for i, vol := range pod.Spec.Volumes {
if !v.ShouldIncludeVolumeInBackup(vol, volsToExclude, backupExcludePVC) {
// don't backup volumes that are included in the exclude list.
if util.Contains(volsToExclude, vol.Name) {
FSBackupNonActionMatchingVols = append(FSBackupNonActionMatchingVols, vol.Name)
}
continue
}
if vol.PersistentVolumeClaim != nil {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I have removed the volsToExclude
check from the volume policy processing logic as it is related to legacy annotations based approach.
To Clarify, Volume Policy existence will be checked first and processed first. The legacy annotations based fallback option will be used only in 2 cases:
- If no Volume Policy is specified by the user: Ref code link from PR
- If there are Volumes for which no Volume Policy action was found i.e. action was nil : Ref code link from PR
I have added comments in the code for the above process.
4e0de30
to
a01395b
Compare
363e4ff
to
fd00a0f
Compare
@shubham-pampattiwar Could help to address the comments so that we can further review and merge it? To meet FC, we hope this could be merged by the end of this week or early next week. |
3961dbb
to
10f1028
Compare
@Lyndon-Li addressed PR comments. |
10f1028
to
f96d570
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7664 +/- ##
==========================================
+ Coverage 58.51% 58.66% +0.14%
==========================================
Files 343 344 +1
Lines 28486 28723 +237
==========================================
+ Hits 16670 16849 +179
- Misses 10401 10445 +44
- Partials 1415 1429 +14 ☔ View full report in Codecov by Sentry. |
@shubham-pampattiwar |
@shubham-pampattiwar |
fcff28b
to
b4a300f
Compare
@Lyndon-Li @blackpiglet Addressed PR feedback and added tests. PTAL, Thanks ! |
b4a300f
to
b7e190c
Compare
Signed-off-by: Shubham Pampattiwar <[email protected]> fix volume policy action execution Signed-off-by: Shubham Pampattiwar <[email protected]> remove unused files Signed-off-by: Shubham Pampattiwar <[email protected]> add changelog file Signed-off-by: Shubham Pampattiwar <[email protected]> fix CI linter errors fix linter errors address pr review comments Signed-off-by: Shubham Pampattiwar <[email protected]> fix via make update cmd Signed-off-by: Shubham Pampattiwar <[email protected]> address PR feedback and add tests Signed-off-by: Shubham Pampattiwar <[email protected]> fix codespell Signed-off-by: Shubham Pampattiwar <[email protected]> fix ci linter checks Signed-off-by: Shubham Pampattiwar <[email protected]> remove volsToExclude processing from volume policy logic and add tests Signed-off-by: Shubham Pampattiwar <[email protected]> fix ci linter issue Signed-off-by: Shubham Pampattiwar <[email protected]>
b7e190c
to
8d2bef2
Compare
Thank you for contributing to Velero!
Please add a summary of your change
Implementation PR for Extend Volume Policies feature to support more actions
Related design: #6956
Does your change fix a particular issue?
Fixes #6640
Please indicate you've done the following:
/kind changelog-not-required
as a comment on this pull request.site/content/docs/main
.