Optional finalize for parent reconciler #204

gmrodgers · 2022-04-06T14:29:28Z

Currently, the ParentReconciler does not reconcile on a delete request. This leads to some reconcilers not being implementable in reconciler-runtime if they want to do cleanup on a delete (outside of owner reference garbage collection).

The typical way to do this in Kubernetes is to have the reconciler add a finalizer to the resource on first reconcile which blocks deletion, and when it reconciles on deletion, it cleans up and removes said finalizer (see: Finalizers).

This PR aims to create an abstraction over the finalizers, seeing them as an implementation detail. It proposes the following:

An optional Finalize method on the Parent reconciler with signature:
```
func(ctx context.Context, parent client.Object) error
```
This method would be called on a delete reconcile and if successful, finalizer is removed.
Allow delete reconciles for ParentReconciler if the Finalize method is present.
Add the a deterministic finalizer on non-delete reconciles of ParentReconciler if the Finalize method is present.

codecov · 2022-04-06T14:32:57Z

Codecov Report

Merging #204 (c8e660d) into main (0248645) will increase coverage by 1.66%.
The diff coverage is 95.65%.

❗ Current head c8e660d differs from pull request most recent head a2b3906. Consider uploading reports for the commit a2b3906 to get more accurate results

@@            Coverage Diff             @@
##             main     #204      +/-   ##
==========================================
+ Coverage   64.71%   66.37%   +1.66%     
==========================================
  Files           8        8              
  Lines         768      812      +44     
==========================================
+ Hits          497      539      +42     
- Misses        256      257       +1     
- Partials       15       16       +1

Impacted Files	Coverage Δ
reconcilers/reconcilers.go	`85.91% <95.65%> (+0.91%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0248645...a2b3906. Read the comment docs.

- Used to clean up unowned resources on a delete reconcile.

scothis

Thanks for kicking off this work. Depending on your level of interest there are a couple paths we can explore based on inline comments/questions.

While I think this approach will work for the vast majority of cases, I want to consider what it would mean to bake finalizers a bit deeper into the composition model.

scothis · 2022-04-06T21:51:44Z

reconcilers/reconcilers.go

+	// Expected function signature:
+	//     func(ctx context.Context, parent client.Object) error
+	// +optional
+	Finalize interface{}


I wonder if Finalize should itself be a SubReconciler? That would give us a natural way to compose multiple finalizers. We'd consider the finalizer clearable when it returns an empty result and no error.

Another option would be to add a Finalize method to the SubReconciler interface (or create an extended interface to not break existing types).

I thought about this too but wonder how some subreconcilers would behave in the case of deletion. SyncReconcilers are a natural fit (even share the same signature as what I implemented). ChildReconcilers seem to be somewhere we do not want to have on deletion. If we were to go down that route, would we prevent ChildReconcilers some how or just rely on user restraint?

I think we'd want finalizer support for each built-in sub reconciler. A ChildReconciler that creates a resource in a different namespace should still be able to clean up that resource upon deletion of the parent. The sub reconciler, especially when composed, will have more context about the state that needs to be cleaned up than the parent reconciler.

Interesting, so to check I understand, given SyncReconciler and ChildReconciler as examples:

The ChildReconciler would create a finalizer if a) itself is a namespaced scope resource, and b) it's child is either clusterscoped or in a different namespace. On a deletion, it would then have remove the parent finalizer, and the child resource using an explicit Delete.

The SyncReconciler would need to have its functionality exposed via a Finalize like method which if present, a finalizer is added to the parent. On a deletion, it would then remove the finalizer after running Finalize.

And by necessity, the ParentReconciler will now allow deletion requests through to subreconcilers (unless it were to check all whether all subreconcilers need to create finalizers).

Ive pushed a version that does part of the above:

SyncReconciler adds a finalizer and has the new method

For now, ChildReconciler just ignores it and will assume same namespace tidy up via ownerreferences

The tricky bit I found was how to allow for a unique finalizer for each Sync (or any other reconciler that wanted one). I came up with the following options:

Hash the contents of the Finalize function

I don't think this works the Finalize functions shared across SubReconciler

The name of Subreconciler via reflection

Problem since you could also have same named reconcilers but different packages, or inlined SubReconcilers

Have user pass in their own custom finalizers

feels like breaking the abstraction by allowing selection of finalizers perhaps?

Easiest solution that would probably cover most usecases

Have unique finalizer generated per reconciler via incrementing a count in the stash (I tried implementing this)

Had to create a UniqueFinalizer() func to handle this and make it public so other Subreconcilers could use it.

Open to thoughts on the options or suggestions of others!

reconcilers/reconcilers.go

scothis · 2022-04-06T21:56:22Z

reconcilers/reconcilers.go

+
+	if !equality.Semantic.DeepEqual(parent.GetFinalizers(), originalParent.GetFinalizers()) {
+		log.Info("updating finalizers", "diff", cmp.Diff(parent.GetFinalizers(), originalParent.GetFinalizers()))
+		if updateErr := r.Update(ctx, parent); updateErr != nil {


A patch request that only updates the finalizers would be better, as to avoid any accidental mutations to other parts of the resource. Early patch support is in #202

This is fine as a stopgap, until that merges.

scothis · 2022-04-06T21:58:01Z

reconcilers/reconcilers.go

 	// return original reconcile result
 	return result, err
 }

 func (r *ParentReconciler) reconcile(ctx context.Context, parent client.Object) (ctrl.Result, error) {
+	finalizer := fmt.Sprintf("%s/reconciler-runtime-finalize", parent.GetObjectKind().GroupVersionKind().Group)


Do you think there is any need to allow a fully custom finalizer name?

There's a corner case for "v1" resources that are part of the "core" api group, but are represented by an empty group string in the gvk. To avoid a finalizer that starts with a "/", we should expand "" to "core".

Suggested change

finalizer := fmt.Sprintf("%s/reconciler-runtime-finalize", parent.GetObjectKind().GroupVersionKind().Group)

finalizer := fmt.Sprintf("%s/reconciler-runtime-finalizer", parent.GetObjectKind().GroupVersionKind().Group)

To clarify, as in allow the writer of the ParentReconciler to specify a custom finalizer? I guess they would want this if a) they need more than one finalizer or b) they need a finalizer with a specific name. I have not seen a) or b) in the wild and therefore am unsure if it is a usecase or if it is best practice or not.

A resource can define an arbitrary set of finalizers, k8s will only finish the deletion once all of the finalizers have been removed.

Co-authored-by: Scott Andrews <scott@andrews.me>

- ParentReconciler allows delete reconciles - ChildReconciler ignores them - Each syncReconciler adds a unique finalizer (supplied via stash)

scothis · 2022-04-17T16:05:29Z

I started to play around with pushing the bulk of finalizer support into the edges of the reconciler tree, allowing each child reconciler to manage its own finalizer on the parent. There's certainly a trade off as a poorly crafted reconciler could causes a child to be orphaned or a resource to be stuck terminating.

The approach you took in this PR is still useful, but it may may more sense as a higher order sub reconciler. I'm imaging something with a similar shape to the CastParent reconciler:

when the parent resource is not terminating and the finalizer is not set, add it
call the nested sub reconciler
when the parent resource is terminating and the nested sub reconciler did not return an error, clear the finalizer

This pattern could be used to create arbitrary finalization boundaries anywhere in the sub reconciler hierarchy. There are a couple helper utilities in my PR that make adding/clearing a finalizer on the parent resource much easier.

scothis · 2022-05-06T16:05:31Z

@gmrodgers thank you so much for opening this PR and kick-starting the conversation around finalizers. I'm going to close this as it largely conflicts with #207

There are a couple good ideas in this PR that are not present in #207, specifically being able to define adding/removing a finalizer around a reconciliation. Where #207 adds finalizer support for the leaves, we could also support finalizers at the trunk of the reconciler tree.

vmwclabot added the cla-not-required label Apr 6, 2022

Add optional Finalize to ParentReconciler.

b8fd9d1

- Used to clean up unowned resources on a delete reconcile.

gmrodgers force-pushed the optional-finalize-for-parent-reconciler branch from bfacff2 to b8fd9d1 Compare April 6, 2022 21:31

scothis reviewed Apr 6, 2022

View reviewed changes

gmrodgers and others added 2 commits April 8, 2022 14:37

Be specific in finalizer update err and events.

e692211

Co-authored-by: Scott Andrews <scott@andrews.me>

Add test case for finalizer update erroring.

47f6d8b

gmrodgers force-pushed the optional-finalize-for-parent-reconciler branch from 7f06178 to 47f6d8b Compare April 8, 2022 14:38

Move Finalize method to SyncReconciler.

a2b3906

- ParentReconciler allows delete reconciles - ChildReconciler ignores them - Each syncReconciler adds a unique finalizer (supplied via stash)

scothis closed this May 6, 2022

scothis mentioned this pull request Mar 26, 2024

Proxy vmware-labs/reconciler-runtime reconcilerio/runtime#204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional finalize for parent reconciler #204

Optional finalize for parent reconciler #204

gmrodgers commented Apr 6, 2022 •

edited

codecov bot commented Apr 6, 2022 •

edited

scothis left a comment

scothis Apr 6, 2022

gmrodgers Apr 7, 2022

scothis Apr 7, 2022

gmrodgers Apr 8, 2022 •

edited

gmrodgers Apr 15, 2022 •

edited

scothis Apr 6, 2022

scothis Apr 6, 2022

gmrodgers Apr 7, 2022

scothis Apr 7, 2022

scothis commented Apr 17, 2022

scothis commented May 6, 2022

	finalizer := fmt.Sprintf("%s/reconciler-runtime-finalize", parent.GetObjectKind().GroupVersionKind().Group)
	finalizer := fmt.Sprintf("%s/reconciler-runtime-finalizer", parent.GetObjectKind().GroupVersionKind().Group)

Optional finalize for parent reconciler #204

Optional finalize for parent reconciler #204

Conversation

gmrodgers commented Apr 6, 2022 • edited

codecov bot commented Apr 6, 2022 • edited

Codecov Report

scothis left a comment

Choose a reason for hiding this comment

scothis Apr 6, 2022

Choose a reason for hiding this comment

gmrodgers Apr 7, 2022

Choose a reason for hiding this comment

scothis Apr 7, 2022

Choose a reason for hiding this comment

gmrodgers Apr 8, 2022 • edited

Choose a reason for hiding this comment

gmrodgers Apr 15, 2022 • edited

Choose a reason for hiding this comment

scothis Apr 6, 2022

Choose a reason for hiding this comment

scothis Apr 6, 2022

Choose a reason for hiding this comment

gmrodgers Apr 7, 2022

Choose a reason for hiding this comment

scothis Apr 7, 2022

Choose a reason for hiding this comment

scothis commented Apr 17, 2022

scothis commented May 6, 2022

gmrodgers commented Apr 6, 2022 •

edited

codecov bot commented Apr 6, 2022 •

edited

gmrodgers Apr 8, 2022 •

edited

gmrodgers Apr 15, 2022 •

edited