Fix namespace metadata not being restored with existing-resource-policy #8607
+93
−2
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
This change ensures that when restoring namespaces with existing-resource-policy set to 'update', the namespace metadata (labels and annotations) from the backup bundle is properly restored. Fixes vmware-tanzu#7519 Signed-off-by: Swanand Shende <sshende@catalogicsoftware.com>
kaovilai
requested changes
Jan 15, 2025
Thanks. Accept this suggestion Co-authored-by: Tiger Kaovilai <passawit.kaovilai@gmail.com> Signed-off-by: sshende-catalogicsoftware <66183996+sshende-catalogicsoftware@users.noreply.github.com>
Thanks. Accept this suggestion as well. Co-authored-by: Tiger Kaovilai <passawit.kaovilai@gmail.com> Signed-off-by: sshende-catalogicsoftware <66183996+sshende-catalogicsoftware@users.noreply.github.com>
|
@kaovilai , Thanks for the suggestions! I've updated both comments to be clearer: Please let me know if you'd like to see any other improvements! |
kaovilai
approved these changes
Jan 15, 2025
reasonerjt
reviewed
Jan 20, 2025
| resourceClient, | ||
| ) | ||
|
|
||
| // Fall back to manual label/annotation update if the patch fails |
There was a problem hiding this comment.
Is this fallback necessary? In what case the patch will fail but the update will succeed?
| if groupResource == kuberesource.Namespaces { | ||
| if existingNamespaces.Has(targetNS) { |
There was a problem hiding this comment.
I think we can make condition like
if existingNamespaces.Has(targetNS) && velerov1api.PolicyTypeUpdate == ctx.restore.Spec.ExistingResourcePolicy {...}
and ignore printing the "Skipping ...." log message.
| @@ -2243,7 +2325,9 @@ func (ctx *restoreContext) getOrderedResourceCollection( | |||
| continue | |||
| } | |||
|
|
|||
| if namespace == "" && !boolptr.IsSetToTrue(ctx.restore.Spec.IncludeClusterResources) && !ctx.namespaceIncludesExcludes.IncludeEverything() { | |||
| if groupResource.Resource == "namespaces" { | |||
| ctx.log.Infof("Including resource namespaces despite being cluster-scoped") | |||
There was a problem hiding this comment.
is this log message needed?
It can also be confusing if ctx.restore.Spec.IncludeClusterResources is set to true
| @@ -0,0 +1,7 @@ | |||
| kind: bug | |||
There was a problem hiding this comment.
This is trivial but conventionally the changelog is a one-liner, otherwise the format of the combined changelogs may be inconsistent.
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.
Description
This PR fixes #7519 where namespace metadata (labels and annotations) from backup bundles was not being restored even when
--existing-resource-policywas set to "update".Fix Details
The fix adds logic to:
Testing Done
--existing-resource-policy=updateRelated PRs or Issues
Reviewer Instructions
Please pay special attention to:
Please indicate you've done the following:
make new-changelog) or comment/kind changelog-not-requiredon this PR.site/content/docs/main. ==> Not Applicable. No documentation updates needed.