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

Remove the "in-use" Finalizer #265

Merged
merged 3 commits into from
Jun 5, 2024
Merged

Remove the "in-use" Finalizer #265

merged 3 commits into from
Jun 5, 2024

Conversation

nolancon
Copy link
Collaborator

@nolancon nolancon commented Jun 5, 2024

Description of your changes

The purpose of the bucket-in-use finalizer was to ensure that (1) a Bucket CR could not be removed whilst it still has corresponding S3 buckets existing on Ceph backends and (2) in a system where Bucket CRs are managed by a separate controller or component, a separate actor could not simply delete a Bucket CR (by accident or otherwise).

However, scenario (1) is covered by the Crossplane managed finalizer.
Scenario (2) is not covered by the managed finalizer, but upon reflection it should not be the responsibility of Provider Ceph to add an "in-use" finalizer. It should instead be the responsibility of the controller/component that creates the Bucket CR.

Provider Ceph managing it's own "in-use" finalizer causes an issue whereby failure to remove the finalizer after successful deletion is not requeued by Crossplane as it assumes that the only finalizer it should care about is its own managed finalizer.
This can lead to a scenario where deleting a Bucket CR can result in the successful removal of all buckets from backends, however the kube API update to remove the finalizer fails and the CR is left hanging forever.

I have:

  • Run make reviewable to ensure this PR is ready for review.
  • Run make ceph-chainsaw to validate these changes against Ceph. This step is not always necessary. However, for changes related to S3 calls it is sensible to validate against an actual Ceph cluster. Localstack is used in our CI Chainsaw suite for convenience and there can be disparity in S3 behaviours betwee it and Ceph. See docs/TESTING.md for information on how to run tests against a Ceph cluster.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

CI + e2e

@nolancon nolancon marked this pull request as ready for review June 5, 2024 14:07
@nolancon nolancon requested review from mhmxs and Shunpoco and removed request for mhmxs June 5, 2024 14:07
@nolancon nolancon merged commit 1832017 into main Jun 5, 2024
9 of 10 checks passed
@nolancon nolancon deleted the delete-bucket-debug branch June 5, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants