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 finalizer with webhook instead of controllers #2080

Merged
merged 3 commits into from
Jul 24, 2023

Conversation

ejweber
Copy link
Contributor

@ejweber ejweber commented Jul 14, 2023

longhorn/longhorn#4872

This work was started in #1535. In that PR, we ensured that the webhook only added the finalizer during CREATE operations and not during UPDATE operations to avoid resources that cannot be deleted (e.g. we attempt to remove a resource's finalizer, but the finalizer is added back when we do so). After reading some discussion on the internet and thinking on it a bit, I decided a better approach would be for the webhook to always add the finalizer on both CREATE and UPDATE except when the deletion timestamp is set. After reviewing our code, every time longhorn-manager removes the finalizer, the deletion timestamp has already been set on an object. This approach has two benefits:

  • The finalizer cannot be accidentally deleted by a user unless the object is already being deleted. Previously, a user could update an object so that it did not have a finalizer. This could have been problematic later when the object was deleted.
  • Consistency/code cleanliness. We always do the same thing on CREATE and UPDATE and can share logic between the two.

Otherwise, I also tried to clean up the webhook resources slightly. Code was organized differently between resources and some code was duplicated. Since I was already doing a webhook refactor, I spent some time (hopefully) improving this.

Keeping this as a draft until regression tests pass: https://ci.longhorn.io/job/private/job/longhorn-tests-regression/4596/console.

@ejweber
Copy link
Contributor Author

ejweber commented Jul 14, 2023

I tested this PR by deploying an updated longhorn-manager and creating/editing/deleting resources. Generally, each resource should have a finalizer added on creation. It should be impossible to remove the finalizer while the resource's deletion timestamp is not set, and the resource should still clean up as normal. See specific results below:

backingimage
(created through UI with url: https://longhorn-backing-image.s3-us-west-1.amazonaws.com/parrot.qcow2 and checksum: bd79ab9e6d45abf4f3f0adf552a868074dd235c4698ce7258d521160e0ad79ffe555b94e7d4007add6e1a25f4526885eb25c53ce38f7d344dd4925b9f2cb5d3b)
has finalizer x
doesn't allow finalizer removal while not deleting x
cleans up when directly deleted x

backingimagedatasource
has finalizer x
doesn't allow finalizer removal while not deleting x
cleans up when backing image is deleted x

backingimagemanager
has finalizer x
doesn't allow finalizer removal while not deleting x
cleans up when backing image is deleted x

backup
has finalizer x
doesn't allow finalizer removal while not deleting x
cleans up when directly deleted x

backupvolume
has finalizer x
doesn't allow finalizer removal while not deleting x
cleans up when directly deleted x

engine
has finalizer x
doesn't allow finalizer removal while not deleting x
cleans up when volume deleted x

engineimage
has finalizer x
doesn't allow finalizer removal while not deleting x
cleans up during uninstall ?

node
has finalizer x
doesn't allow finalizer removal while not deleting x
cleans up when directly deleted x

replica
has finalizer x
doesn't allow finalizer removal while not deleting x
cleans up when volume deleted x

sharemanager
has finalizer x
doesn't allow finalizer removal while not deleting x
cleans up when rwx volume deleted x

snapshot
has finalizer x
doesn't allow finalizer removal while not deleting x

systembackup
has finalizer x
doesn't allow finalizer removal while not deleting x
cleans up when directly deleted x

volume
has finalizer x
doesn't allow finalizer removal while not deleting x
cleans up when directly deleted x

volumeattachment (lhva or volumeattachment.longhorn.io, NOT volumeattachment.storage.k8s.io)
has finalizer x
doesn't allow finalizer removal while not deleting x
cleans up when volume deleted x

Note: any time we edit a resource to remove its finalizer but don't delete it, we should see something like the following in the longhorn-manager logs:

time="2023-07-13T20:42:05Z" level=info msg="Request (user: user-gkb8z, longhorn.io/v1beta2, Kind=BackingImage, namespace: longhorn-system, name: test-bi, operation: UPDATE) patchOps: [{\"op\": \"replace\", \"path\": \"/metadata/finalizers\", \"value\": [\"longhorn.io\"]}]" service=admissionWebhook

@ejweber
Copy link
Contributor Author

ejweber commented Jul 17, 2023

Signed-off-by: Eric Weber <eric.weber@suse.com>
@ejweber ejweber force-pushed the 4872-add-finalizer-with-webhook branch from 562e029 to f87d716 Compare July 18, 2023 17:00
shuo-wu
shuo-wu previously approved these changes Jul 19, 2023
Copy link
Contributor

@shuo-wu shuo-wu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@PhanLe1010 PhanLe1010 left a comment

Choose a reason for hiding this comment

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

In general LGTM.

Can we make sure add this uninstallation test plan:

  1. Install Longhorn with all possible CRDs (volumes, snapshot, backups, backing image, recurring jobs, ...)
  2. Make sure Uninstallation is ok

webhook/resources/volume/mutator.go Show resolved Hide resolved
james-munson
james-munson previously approved these changes Jul 21, 2023
Copy link
Contributor

@james-munson james-munson left a comment

Choose a reason for hiding this comment

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

An idle question / whine, but LGTM.

webhook/resources/recurringjob/mutator.go Show resolved Hide resolved
Signed-off-by: Eric Weber <eric.weber@suse.com>
Signed-off-by: Eric Weber <eric.weber@suse.com>
@ejweber ejweber dismissed stale reviews from james-munson and shuo-wu via 0a0fd7d July 24, 2023 16:36
@ejweber ejweber force-pushed the 4872-add-finalizer-with-webhook branch from f87d716 to 0a0fd7d Compare July 24, 2023 16:36
Copy link
Contributor

@PhanLe1010 PhanLe1010 left a comment

Choose a reason for hiding this comment

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

LGTM

@PhanLe1010 PhanLe1010 merged commit 6b7d12b into longhorn:master Jul 24, 2023
3 checks passed
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.

5 participants