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

add restore finalizer to clean up external resources #6479

Merged
merged 1 commit into from
Jul 20, 2023

Conversation

allenxu404
Copy link
Contributor

Thank you for contributing to Velero!

Please add a summary of your change

  • Add restore finalizer to clean up external resources when restore is being deleted

Does your change fix a particular issue?

Fixes #2697

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.

@codecov-commenter
Copy link

codecov-commenter commented Jul 11, 2023

Codecov Report

Merging #6479 (1fbc1ac) into main (0945879) will decrease coverage by 0.27%.
The diff coverage is 53.42%.

@@            Coverage Diff             @@
##             main    #6479      +/-   ##
==========================================
- Coverage   60.31%   60.04%   -0.27%     
==========================================
  Files         229      229              
  Lines       24319    24552     +233     
==========================================
+ Hits        14667    14742      +75     
- Misses       8647     8791     +144     
- Partials     1005     1019      +14     
Impacted Files Coverage Δ
pkg/controller/restore_controller.go 64.53% <53.42%> (-0.31%) ⬇️

... and 4 files with indirect coverage changes

@ywk253100
Copy link
Contributor

@allenxu404 Does this PR cover the upgrade case? What will happen to the existing Restores?

@allenxu404
Copy link
Contributor Author

@allenxu404 Does this PR cover the upgrade case? What will happen to the existing Restores?

@ywk253100 "The existing restores will not have a finalizer added and they will be deleted directly the way it was.

@ywk253100
Copy link
Contributor

@allenxu404 Current changes that filter based on different events are complicated, how about that we handle the restore directly without considering the event type as follows:

  1. Get the restore
  2. If the restore doesn't exist, return directly
  3. If the deletion timestamp isn't empty, clean up the resource in store, remove the finalizer, and return
  4. Reconcile the finalizer if the restore isn't set. - This covers the upgrade case
  5. Process the restore if it is new as previous

An example is https://github.com/kubernetes-sigs/cluster-api/blob/main/internal/controllers/machine/machine_controller.go

@ywk253100
Copy link
Contributor

And please also the new code is covered by the unit test

@allenxu404 allenxu404 force-pushed the i2697 branch 4 times, most recently from 8e49629 to 3b0db77 Compare July 17, 2023 10:52
@allenxu404
Copy link
Contributor Author

@allenxu404 Current changes that filter based on different events are complicated, how about that we handle the restore directly without considering the event type as follows:

1. Get the restore

2. If the restore doesn't exist, return directly

3. If the deletion timestamp isn't empty, clean up the resource in store, remove the finalizer, and return

4. Reconcile the finalizer if the restore isn't set. - This covers the upgrade case

5. Process the restore if it is new as previous

An example is https://github.com/kubernetes-sigs/cluster-api/blob/main/internal/controllers/machine/machine_controller.go

@ywk253100 Yeah, make sense. I made an enhancement to the code and it functions following the procedures outlined aboved.

}
}

fmt.Println("Waiting for resource with finalizer attached to be deleted")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's replace resource with finalizer attached with restores as the finalizer is the internal mechanism

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we output "Waiting for restore to be deleted" to the users, users might be confused about the need for an additional operation to delete the restores. What's your thought?

pkg/cmd/cli/uninstall/uninstall.go Outdated Show resolved Hide resolved
pkg/cmd/cli/uninstall/uninstall.go Outdated Show resolved Hide resolved
pkg/controller/restore_controller.go Outdated Show resolved Hide resolved
pkg/controller/restore_controller.go Outdated Show resolved Hide resolved
@allenxu404 allenxu404 force-pushed the i2697 branch 2 times, most recently from 83a6895 to b034880 Compare July 19, 2023 08:48
Signed-off-by: allenxu404 <qix2@vmware.com>
@qiuming-best qiuming-best merged commit 32262ba into vmware-tanzu:main Jul 20, 2023
22 checks passed
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 velero restore objects from the cloud storage
4 participants