Do not attempt restore resource with no available GVK in cluster #7322

kaovilai · 2024-01-16T20:53:03Z

It is possible to end up in a situation where CRD was not restored due to an already existing CRD with a newer GVK without prior GVK. When restoring CR with older GVK, this PR causes item restore to error early and partially fail.

In the future, it might be desirable to merge CRD.spec.versions from backup with in-cluster if it can be done without conflicts but is out of scope of this PR.

Signed-off-by: Tiger Kaovilai tkaovila@redhat.com

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #7319

Please indicate you've done the following:

Accepted the DCO. Commits without the DCO will delay acceptance.
Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
Updated the corresponding documentation in site/content/docs/main.

kaovilai · 2024-01-16T22:36:05Z

pkg/restore/restore.go

+	if !clusterHasKind  {
+		ctx.log.Errorf("Cannot restore %s because GVK doesn't exist in the cluster", groupResource)
+		// Adding to errs should cause restore to partially fail
+		errs.Add(namespace, fmt.Errorf("cannot restore %s because gvk doesn't exist in the cluster", groupResource))


This should cause restore to partially fail, but the error message maybe different than before. Alternatively, another commits attempts to preserve prior error message for when informer cache is disabled. Let me know which is preferred.

ywk253100 · 2024-01-17T00:22:27Z

pkg/restore/restore.go

+	// Check if cluster has GVK required to restore resource, otherwise return err.
+	// Placed here because the object apiVersion might get modified by a RestorePlugin
+	// So we check for the GVK after the RestorePlugin has run.
+	_, _, err = ctx.discoveryHelper.KindFor(schema.GroupVersionKind{


I'm afraid this change isn't the correct fix because the informer syncing will still stuck if the CRDs don't exist or aren't restored: https://github.com/vmware-tanzu/velero/blob/main/pkg/restore/restore.go#L602

The second commit is the good one

Thanks. So maybe hybrid of both?

IMHO, the change in the second commit is enough. Seems the change here doesn't help resovling the issue, correct me if I'm wrong.

kaovilai · 2024-01-17T03:28:53Z

I think pkg/discovery and Fakes may not have been implemented fully.. will continue tomorrow

kaovilai · 2024-01-18T20:28:13Z

pkg/discovery/helper.go

@@ -193,7 +196,8 @@ func (h *helper) Refresh() error {

 		for _, resource := range resourceGroup.APIResources {
 			gvr := gv.WithResource(resource.Name)
-			gvk := gv.WithKind(resource.Kind)
+			// gvk kind is lowercased for consistency of hits in KindFor()
+			gvk := gv.WithKind(strings.ToLower(resource.Kind))


This and related changes fixed the unit tests. Will have to try a run live in a real environment to confirm everything is working.

codecov · 2024-01-18T20:28:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a81e049) 61.75% compared to head (1ece005) 61.76%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7322      +/-   ##
==========================================
+ Coverage   61.75%   61.76%   +0.01%     
==========================================
  Files         262      262              
  Lines       28419    28433      +14     
==========================================
+ Hits        17549    17563      +14     
  Misses       9640     9640              
  Partials     1230     1230

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ywk253100 · 2024-01-19T05:33:21Z

pkg/discovery/helper.go

@@ -117,6 +119,7 @@ func (h *helper) ResourceFor(input schema.GroupVersionResource) (schema.GroupVer
 	return gvr, apiResource, nil
 }

+// KindFor expects input where kind is lowercased to hit the map


The reason that Kind doesn't match is the test code converts the Resource to Kind incorrectly:

velero/pkg/test/discovery_client.go

Line 79 in 33bc0f1

Kind: cases.Title(language.Und).String(strings.TrimSuffix(resource.Name, "s")),

This shouldn't happen in the real world.

So let's specify the Kind explicitly in the test code, I create a PR for this, free feel to copy the changes into your PR if it isn't merged

Thanks! Will check

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> do not attempt restore resource with no available GVK in cluster Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

…esource if nil Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

…are-tanzu#7322) Check for GVK before attempting restore. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

… cluster (#7336) * Specify the Kind explicitly in the API resource Specify the Kind explicitly in the API resource to avoid wrong Kind conversion * Do not attempt restore resource with no available GVK in cluster (#7322) Check for GVK before attempting restore. --------- Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> Co-authored-by: Tiger Kaovilai <tkaovila@redhat.com>

github-actions bot requested review from blackpiglet and sseago January 16, 2024 20:53

github-actions bot assigned kaovilai Jan 16, 2024

kaovilai marked this pull request as draft January 16, 2024 20:55

kaovilai force-pushed the do-not-create-informer-for-non-existent-GVK branch 7 times, most recently from 784599c to a7de73e Compare January 16, 2024 22:33

github-actions bot added the has-changelog label Jan 16, 2024

kaovilai commented Jan 16, 2024

View reviewed changes

kaovilai changed the title ~~do not attempt restore resource with no available GVK in cluster~~ Do not attempt restore resource with no available GVK in cluster Jan 16, 2024

kaovilai force-pushed the do-not-create-informer-for-non-existent-GVK branch from a7de73e to 642efc8 Compare January 16, 2024 22:41

ywk253100 reviewed Jan 17, 2024

View reviewed changes

kaovilai force-pushed the do-not-create-informer-for-non-existent-GVK branch from 740bbd7 to 13c777d Compare January 17, 2024 01:56

kaovilai force-pushed the do-not-create-informer-for-non-existent-GVK branch 2 times, most recently from 94f9e89 to a94ccb4 Compare January 18, 2024 20:19

kaovilai commented Jan 18, 2024

View reviewed changes

kaovilai marked this pull request as ready for review January 18, 2024 20:35

github-actions bot requested a review from reasonerjt January 18, 2024 20:35

ywk253100 reviewed Jan 19, 2024

View reviewed changes

ywk253100 mentioned this pull request Jan 19, 2024

Specify the Kind explicitly in the API resource #7334

Merged

3 tasks

kaovilai added 2 commits January 19, 2024 10:48

Check for GVK before attempting restore.

5d21385

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> do not attempt restore resource with no available GVK in cluster Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Perform GVK check in getGenericInformerInternal, return error in getR…

adb0f46

…esource if nil Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

kaovilai force-pushed the do-not-create-informer-for-non-existent-GVK branch 2 times, most recently from ffb8357 to fab9fa0 Compare January 19, 2024 16:15

Revert unit test kind case workaround after vmware-tanzu#7334

1ece005

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

kaovilai force-pushed the do-not-create-informer-for-non-existent-GVK branch from fab9fa0 to 1ece005 Compare January 19, 2024 16:35

shubham-pampattiwar approved these changes Jan 19, 2024

View reviewed changes

ywk253100 approved these changes Jan 22, 2024

View reviewed changes

ywk253100 merged commit 270b1de into vmware-tanzu:main Jan 22, 2024
24 checks passed

ywk253100 pushed a commit to ywk253100/velero that referenced this pull request Jan 22, 2024

Do not attempt restore resource with no available GVK in cluster (vmw…

b432679

…are-tanzu#7322) Check for GVK before attempting restore. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

ywk253100 pushed a commit to ywk253100/velero that referenced this pull request Jan 22, 2024

Do not attempt restore resource with no available GVK in cluster (vmw…

3b9384c

…are-tanzu#7322) Check for GVK before attempting restore. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

ywk253100 pushed a commit to ywk253100/velero that referenced this pull request Jan 22, 2024

Do not attempt restore resource with no available GVK in cluster (vmw…

c74a237

…are-tanzu#7322) Check for GVK before attempting restore. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

ywk253100 pushed a commit to ywk253100/velero that referenced this pull request Jan 22, 2024

Do not attempt restore resource with no available GVK in cluster (vmw…

bd0ea3a

…are-tanzu#7322) Check for GVK before attempting restore. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not attempt restore resource with no available GVK in cluster #7322

Do not attempt restore resource with no available GVK in cluster #7322

kaovilai commented Jan 16, 2024 •

edited

Loading

kaovilai Jan 16, 2024

ywk253100 Jan 17, 2024

kaovilai Jan 17, 2024

ywk253100 Jan 17, 2024

kaovilai commented Jan 17, 2024

kaovilai Jan 18, 2024

codecov bot commented Jan 18, 2024 •

edited

Loading

ywk253100 Jan 19, 2024 •

edited

Loading

kaovilai Jan 19, 2024

Do not attempt restore resource with no available GVK in cluster #7322

Do not attempt restore resource with no available GVK in cluster #7322

Conversation

kaovilai commented Jan 16, 2024 • edited Loading

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

kaovilai Jan 16, 2024

Choose a reason for hiding this comment

ywk253100 Jan 17, 2024

Choose a reason for hiding this comment

kaovilai Jan 17, 2024

Choose a reason for hiding this comment

ywk253100 Jan 17, 2024

Choose a reason for hiding this comment

kaovilai commented Jan 17, 2024

kaovilai Jan 18, 2024

Choose a reason for hiding this comment

codecov bot commented Jan 18, 2024 • edited Loading

Codecov Report

ywk253100 Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

kaovilai Jan 19, 2024

Choose a reason for hiding this comment

kaovilai commented Jan 16, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading

ywk253100 Jan 19, 2024 •

edited

Loading