Skip to content
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

Parse resource from backup tarball directly rather than resolving it via discovery service to avoid #4009 #4398

Merged
merged 1 commit into from
Dec 21, 2021

Conversation

ywk253100
Copy link
Contributor

@ywk253100 ywk253100 commented Nov 25, 2021

If the resource is a CR and the CRD is removed, the resource cannot be fully resolved via discovery and the backup cannot be deleted anymore. This commit parses the resource from backup tarball directly to avoid this

Fixes #4009

Signed-off-by: Wenkai Yin(尹文开) yinw@vmware.com

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

Comment on lines 90 to 92
gvr = schema.ParseGroupResource(resource).WithVersion("")

ctx.Log.Infof("got \"no matches\" error when trying to resolve the resource %s via discovery, use ParseGroupResource directly, gvr: %s", resource, gvr.String())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we can't use the gvr variable on line 81 directly, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to replace gvr, _, err := ctx.DiscoveryHelper.ResourceFor(schema.ParseGroupResource(resource).WithVersion("")) with gvr := schema.ParseGroupResource(resource).WithVersion("") in line 81?
I'm not sure whether the input resource is fully resolved or not. Handle this when getting errors may be more safe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same question, @ywk253100 could you please check with Nolan.
Seems the resources in the backup tarball should be resolved already?

@ywk253100
Copy link
Contributor Author

ywk253100 commented Dec 20, 2021

@reasonerjt @jenting Confirmed the fix with @nrb, please take a look again

jenting
jenting previously approved these changes Dec 20, 2021
@reasonerjt
Copy link
Contributor

I think the code change makes sense to me, but the commit message no longer matches the code change?
It's currently skip doing any resolution, instead of ignoring the errors.

@reasonerjt reasonerjt added this to the v1.8.0 milestone Dec 21, 2021
@ywk253100 ywk253100 changed the title Ignore the no match error when trying to resolve the resource via discovery Parse resource from backup tarball directly rather than resolving it via discovery service to avoid #4009 Dec 21, 2021
…via discovery service to avoid vmware-tanzu#4009

If the resource is a CR and the CRD is removed, the resource cannot be fully resolved via discovery and the backup cannot be deleted anymore. This commit parses the resource from backup tarball directly to avoid this

Fixes vmware-tanzu#4009

Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>
Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jenting jenting merged commit 72fc1d2 into vmware-tanzu:main Dec 21, 2021
danfengliu pushed a commit to danfengliu/velero that referenced this pull request Jan 25, 2022
gyaozhou pushed a commit to gyaozhou/velero-read that referenced this pull request May 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to delete backup after application is uninstalled
3 participants