Fix namespace metadata not being restored with existing-resource-policy #8607
Conversation
|
@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! |
pkg/restore/restore.go
Outdated
| 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?
There was a problem hiding this comment.
It was more of defensive programming to cover any unknown scenarios where the patch might fail. I'll remove it.
pkg/restore/restore.go
Outdated
| 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.
There was a problem hiding this comment.
Accepted. Will make the change.
pkg/restore/restore.go
Outdated
| @@ -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.
There was a problem hiding this comment.
Accept. Will change this to a single line.
|
Thanks for the review. I shall be updating the code based on the latest review comments. Awakening this PR after a long time, but will try to finish it now. |
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 <[email protected]>
Thanks. Accept this suggestion Co-authored-by: Tiger Kaovilai <[email protected]> Signed-off-by: sshende-catalogicsoftware <[email protected]>
Thanks. Accept this suggestion as well. Co-authored-by: Tiger Kaovilai <[email protected]> Signed-off-by: sshende-catalogicsoftware <[email protected]>
sshende-catalogicsoftware
force-pushed
the
7519-namespace-metadata-restore
branch
2 times, most recently
from
1ce3fdd to
4e7e853
Compare
|
@reasonerjt : thanks for your review comments again. I have implemented the suggested changes in the latest commit : 4e7e853 @reasonerjt , @kaovilai : Can you please let me know if the changes seem correct and OK to be merged? |
|
@sshende-catalogicsoftware DCO your commits :) |
- Removed the fallback mechanism and rely upon the PATCH operation to add the labels and annotations to the namespace resoure. - Since we are relying on the PATCH operation, we need to add restore labels to backup namespace object so generatePatch detects intended changes. - Cosmetic changes: Remove certain log statements that could cause some confusion. Signed-off-by: Swanand Shende <[email protected]>
sshende-catalogicsoftware
force-pushed
the
7519-namespace-metadata-restore
branch
from
4e7e853 to
507391a
Compare
|
Done, thanks for bringing it to my notice @kaovilai. I have added the signature to the latest commit. |
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.