[1.9.x] - Skip the exclusion check for additional resources returned by BIA #5406
Conversation
reasonerjt
requested review from
sseago,
shubham-pampattiwar and
blackpiglet
| if _, ok := u.GetAnnotations()[mustIncludeAdditionalItemAnnotation]; ok { | ||
| delete(u.GetAnnotations(), mustIncludeAdditionalItemAnnotation) | ||
| } |
There was a problem hiding this comment.
This annotation looks good to me. Prevent errors.
reasonerjt
force-pushed
the
skip-additional-resources-poc-1.9
branch
from
69a3010 to
2c657eb
Compare
There was a problem hiding this comment.
Looks good overall. One minor suggestion in the comments. Also, just to clarify at a high level -- the intent here is for a plugin to set the "must include" annotation on the modified resource for backup, not to add the annotation to the additional items in the cluster, right? That way the item backupper code can use the annotation and remove it before moving on to the next plugin, so this annotation never gets saved in the actual backup, since it's no longer needed.
pkg/backup/item_backupper.go
Outdated
| @@ -341,12 +348,16 @@ func (ib *itemBackupper) executeActions( | |||
| return nil, errors.WithStack(err) | |||
| } | |||
|
|
|||
| if _, err = ib.backupItem(log, item, gvr.GroupResource(), gvr); err != nil { | |||
| if _, err = ib.backupItem(log, item, gvr.GroupResource(), gvr, | |||
| u.GetAnnotations()[mustIncludeAdditionalItemAnnotation] == "true"); err != nil { | |||
There was a problem hiding this comment.
Rather than calling u.GetAnnotations() for each returned additional item, why not add this after line 324 above:
mustInclude := u..GetAnnotations()[mustIncludeAdditionalItemAnnotation] == "true")
and just use this bool var here to pass into backupItem
There was a problem hiding this comment.
@sseago Thanks.
The goal for this draft is to demonstrate the idea, if you are not against the idea, I will refine it and set it to ready for review.
pkg/backup/item_backupper.go
Outdated
| if mustInclude { | ||
| log.Infof("Skipping the exclusion checks for this resource") | ||
| } else { | ||
| if metadata.GetLabels()["velero.io/exclude-from-backup"] == "true" { |
There was a problem hiding this comment.
NIT: convert the label key velero.io/exclude-from-backup to const ?
There was a problem hiding this comment.
Thank you @reasonerjt for the PR, the annotation approach makes sense, this will help solve the CSI plugin issues as well, just added a NIT, else LGTM!
reasonerjt
force-pushed
the
skip-additional-resources-poc-1.9
branch
2 times, most recently
from
3f986af to
0bb456a
Compare
reasonerjt
changed the title
reasonerjt
changed the title
This commit provides a simple contract that if the BackupItemAction plugin sets an annotation in a resource it has handled, the additional items will considered "must include" i.e. each of them will skip the "include-exclude" filter, such that the plugin developer can make sure they are included in the backup disregarding the filter setting in the bakcup CR. Signed-off-by: Daniel Jiang <[email protected]>
reasonerjt
force-pushed
the
skip-additional-resources-poc-1.9
branch
from
0bb456a to
c6274c7
Compare
Introduce a simple contract so that BIA plugin developer can force the additional items be backed up
Signed-off-by: Daniel Jiang [email protected]
Thank you for contributing to Velero!
Please indicate you've done the following:
/kind changelog-not-requiredas a comment on this pull request.site/content/docs/main.