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

Fix delete dataupload datadownload failure when Velero uninstall #6689

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

qiuming-best
Copy link
Contributor

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)
#6687

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.

@qiuming-best qiuming-best added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Aug 22, 2023
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #6689 (7f3b7fe) into main (3e61386) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #6689   +/-   ##
=======================================
  Coverage   60.36%   60.36%           
=======================================
  Files         242      242           
  Lines       25982    25982           
=======================================
  Hits        15683    15683           
  Misses       9196     9196           
  Partials     1103     1103           
Files Changed Coverage Δ
pkg/controller/data_download_controller.go 75.18% <100.00%> (ø)
pkg/controller/data_upload_controller.go 68.25% <100.00%> (ø)

@@ -293,10 +343,53 @@ func gracefullyDeleteRestore(ctx context.Context, kbClient kbclient.Client, name
return err
}

func forcedlyDeleteRestore(ctx context.Context, kbClient kbclient.Client, namespace string) error {
func gracefullyWaitDataUpload(ctx context.Context, kbClient kbclient.Client, namespace string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Creating each function for Restore, DataUpload, and DataDownload seems a bit redundant.
Could we use a unified function for all three resources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used the reflect mechanism in Golang to restructure it.

@qiuming-best qiuming-best force-pushed the uninstall-fix branch 2 times, most recently from 9688d69 to b3609e5 Compare August 23, 2023 05:47
return removeResourceFinalizer(ctx, kbClient, namespace)
}

func removeResourceFinalizer(ctx context.Context, kbClient kbclient.Client, namespace string) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this function, I try to use reflect in Golang, It always panics with panic: interface conversion: v2alpha1.DataUpload is not client.Object: missing method DeepCopyObject. I could not figure it out in a short time. so I keep it with redundancy codes

func removeFinalizer(ctx context.Context, kbClient kbclient.Client, namespace string, resourceList kbclient.ObjectList, finalizer string) error {
	listOptions := &kbclient.ListOptions{Namespace: namespace}
	if err := kbClient.List(ctx, resourceList, listOptions); err != nil {
		return errors.Wrap(err, fmt.Sprintf("Error getting resources of type %T during force deletion", resourceList))
	}

	itemsValue := reflect.ValueOf(resourceList).Elem().FieldByName("Items")
	for i := 0; i < itemsValue.Len(); i++ {
		item := itemsValue.Index(i).Interface().(kbclient.Object)
		if controllerutil.ContainsFinalizer(item, finalizer) {
			update := item.DeepCopyObject().(kbclient.Object)
			original := item.DeepCopyObject().(kbclient.Object)
			controllerutil.RemoveFinalizer(update, finalizer)
			if err := kubeutil.PatchResource(original, update, kbClient); err != nil {
				return errors.Wrap(err, fmt.Sprintf("Error removing finalizer %q during force deletion", finalizer))
			}
		}
	}
	return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

If our type does not conform to that interface, I think this could be a larger problem for anyone working with these API's. I think to fix, you have to run the controller-tools tool to generate these methods for the API.

https://book.kubebuilder.io/reference/markers/object.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comment @shawn-hurley, I've fixed it.
I should not covert interface into kbclient.Objecttype in item := itemsValue.Index(i).Interface().(kbclient.Object)

As here is the definition of DatauploadList

type DataUploadList struct {
    metav1.TypeMeta `json:",inline"`

    // +optional
    metav1.ListMeta `json:"metadata,omitempty"`

    Items []DataUpload `json:"items"`
}

And we have the DeepCopyObject function for *DataUpload type not DataUpload:

func (in *DataUpload) DeepCopyObject() runtime.Object {
	if c := in.DeepCopy(); c != nil {
		return c
	}
	return nil
}

The Item in DataUploadList is DataUpload but not *DataUpload, so it panic with v2alpha1.DataUpload is not client.Object: missing method DeepCopyObject

@qiuming-best qiuming-best force-pushed the uninstall-fix branch 4 times, most recently from 1a9e644 to 49b9638 Compare August 23, 2023 06:41
@qiuming-best qiuming-best merged commit 164431b into vmware-tanzu:main Aug 25, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-unit-tests kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants