Finalizers

[scothis]

Finalizers allow a reconciler to clean up state for a resource that has
been deleted by a client, and not yet fully removed. Resources with
finalizers will stay in this terminating state until all finalizers are
cleared from the resource.

ParentReconcilers will now reconcile resources that are terminating as
long as they also have a pending finalizer.

SyncReconcilers have a new, optional, Finalize method that is called
when the resource is terminating. The Sync method will not be called
unless SyncDuringFinalization is true. Users are responsible to add and
clear finalizers from resources.

The AddParentFinalizer and ClearParentFinalizer functions will patch the
resource. These method work with cast parents objects and use the same
client the ParentReconciler used to originally load the parent resource,
so they can be called inside SubReconcilers that use a different client.

ChildReconcilers have a Finalizer field that when set will ensure that
value is set on the parent resource finalizers before creating/updating
a child resource. It will also clear that finalizer after the child is
deleted. When the parent is terminating, the DesiredChild method is not
called and existing children are deleted.

Signed-off-by: Scott Andrews andrewssc@vmware.com

[codecov]

Codecov Report

Merging #207 (7f2cfff) into main (bb1d838) will increase coverage by 8.84%.
The diff coverage is 94.77%.

@@            Coverage Diff             @@
##             main     #207      +/-   ##
==========================================
+ Coverage   62.09%   70.94%   +8.84%     
==========================================
  Files           9        9              
  Lines         905     1277     +372     
==========================================
+ Hits          562      906     +344     
- Misses        325      351      +26     
- Partials       18       20       +2     

Impacted Files	Coverage Δ
reconcilers/reconcilers.go	84.90% <94.77%> (+4.94%)	⬆️

---

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 bb1d838...7f2cfff. Read the comment docs.

[mamachanko]

Took your branch for a spin. Here's what I found.

To unit test adding of a finalizer, the parent needs to have a ResourceVersion(e.g. "123") and the expected parent
will have that incremented(e.g. "123"). That's not a problem, but while it's putting the burden on the implementer of
the tests they may benefit from better docs here. The test should expect the patch of the parent to add the finalizer
and an additional event "FinalizerPatched".

Unit testing clearing of a finalizer happens by setting the DeletionTimestamp. That may be helpful to document as
well. The patch's body then looks something like:

//...
ExpectPatches: []rtesting.PatchRef{
    {
        Group:     "my.group",
        Kind:      "Thing",
        Namespace: "test-ns",
        Name:      "test-client",
        PatchType: "application/strategic-merge-patch+json",
        Patch:     []byte(`{"metadata":{"finalizers":null,"resourceVersion":"123"}}`),
    },
},
//...

In action, however, reconcilers.AddParentFinalizer breaks down for me with

{
	"level": "error",
	"ts": "2022-04-21T10:07:03.738701466Z",
	"logger": "controller.thing",
	"msg": "Reconciler error",
	"reconciler group": "my.group",
	"reconciler kind": "Thing",
	"name": "max",
	"namespace": "default",
	"error": "the body of the request was in an unknown format - accepted media types include: application/json-patch+json, application/merge-patch+json, application/apply-patch+yaml",
	"stacktrace": "sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\tsigs.k8s.io/controller-runtime@v0.11.2/pkg/internal/controller/controller.go:266\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\tsigs.k8s.io/controller-runtime@v0.11.2/pkg/internal/controller/controller.go:227"
}

I think that's because PatchType: "application/strategic-merge-patch+json" is not available for CRDs, or is it?

[scothis]

To unit test adding of a finalizer, the parent needs to have a ResourceVersion(e.g. "123") and the expected parent will have that incremented(e.g. "123"). That's not a problem, but while it's putting the burden on the implementer of the tests they may benefit from better docs here. The test should expect the patch of the parent to add the finalizer and an additional event "FinalizerPatched".

The ResourceVersion is used as an optimistic concurrency lock. If someone else modified the resource since we loaded it, the request will fail. Since all resources on the api server have a resource version, the test client will default the resource version to "999", we could do the same thing for the parent resource since it is also assumed to exist on the api server. This version shows up in the patch bytes, and the expected parent will have a bumped resource version, so it's still something a user will need to have some knowledge of.

Unit testing clearing of a finalizer happens by setting the DeletionTimestamp. That may be helpful to document as well.

This is how k8s defines termination, but yea, I agree it's worth documenting here.

I think that's because PatchType: "application/strategic-merge-patch+json" is not available for CRDs, or is it?

Yep, the strategic merge patch isn't available for CRDs, switching to regular merge patch.

[mamachanko]

[question]: The doc for Metadata says "Metadata contains arbitrary value that are stored with the test case". Why attach a function here?

[scothis]

this allows us to define the reconciler under test dynamically. The alternative is to setup a new table for each test case. This is a bit of a special case as we're trying to test the subreconciler edge behavior and not a user's business logic.

[mamachanko]

[question]: Can you help me understand why this is a deletion case? I can't tell by looking at the preconditions Parent and GivenObjects. 🤔

[scothis]

this behavior is part of the business logic for the particular reconciler under test. The test case immediate above results in an existing child resource being no longer desired, and deleted without involving a finalizer. This test case is laregly a copy of that test adding in a finalizer.

The delete behavior is driven from

reconciler-runtime/reconcilers/reconcilers_test.go

Lines 468 to 470 in dba890c

	if len(parent.Spec.Fields) == 0 {
	return nil, nil
	}

[mamachanko]

[off-topic]:

• Had to catch up on config and logger being in the context now. The config-related commit says "SubReconcilers that need to interact with the parent resource should use the parent config via RetrieveParentConfig
  method.", but it's fine to use config.Client for generic, parent-unrelated API interaction too, isn't it?

[on-topic]:

• Had to adjust RBAC for deletion of unwanted children. Before I was simply relying on garbage collection to clean away unwanted children. I am not sure I understand how rr figures out when to delete a child.
• Had to adjust the child reconciler which was using ReflectChildStatusOnParent. Once the unwanted child got deleted,
  the child passed to ReflectChildStatusOnParent is nil and the controller may fall over. It's reasonable to check for a
  nil child, but it becomes much more important now I think. On the other hand I wonder if a ChildReconciler should be concerned about deletion at all.
• Ouch, finalizers bite. Once the controller starts falling over, k8s needs some negotiating to delete the resource. kubectl edit ftw.
• It works, @scothis! ⭐

[scothis]

• Had to catch up on config and logger being in the context now.

Yea, I rebased the PR

The config-related commit says "SubReconcilers that need to interact with the parent resource should use the parent config via RetrieveParentConfig
method.", but it's fine to use config.Client for generic, parent-unrelated API interaction too, isn't it?

That's will work when the clients are the same, which is the default. The parent and current client may be different if the child resource is being created using a client with a different service account, or on a different cluster. In these cases using the correct client will be important. Because it's a subtile change that may bite you later when the context shifts, loading the "correct" config now is recommended.

• Had to adjust RBAC for deletion of unwanted children. Before I was simply relying on garbage collection to clean away unwanted children. I am not sure I understand how rr figures out when to delete a child.

The ChildReconciler should always have delete permission to the child resource. Before this PR, the ChildReconciler will delete child resources when there are multiple potential children (rare but can happen if the informers are lagging and there are two controllers running without leader election), or when the desired child is nil. After this change, the ChildReconciler will also delete the child resource if the parent resource is terminating as the desired child is implicitly nil.

We can document this better, opened #217 to track.

• Had to adjust the child reconciler which was using ReflectChildStatusOnParent. Once the unwanted child got deleted,
  the child passed to ReflectChildStatusOnParent is nil and the controller may fall over. It's reasonable to check for a
  nil child, but it becomes much more important now I think. On the other hand I wonder if a ChildReconciler should be concerned about deletion at all.

At the point the parent resource is terminating, the status won't be updated, so there's no point in calling ReflectChildStatusOnParent, we can skip that.

• Ouch, finalizers bite. Once the controller starts falling over, k8s needs some negotiating to delete the resource. kubectl edit ftw.

Yea, don't use finalizers unless you really need them. Something else to be careful of, when uninstalling a controller, delete the CRD before the controller, sa, and rbac permissions to make sure that any existing resources can be deleted cleanly. kapp change groups are a great way to handle this.

We can add that warning to the readme.

[gmrodgers]

question: I'm unsure of the need for the SyncDuringFinalization field, can you give me some usecases?

Is it to save folks from manually calling Sync during Finalize? Which given they would probably have compatible signatures, would be fairly easy.

[scothis]

The main intent is to avoid needing to duplicate the same function for both Sync and Finalize.

You'd want to use the same logic when the sync reconciler is looking up reference data that is used during regular processing and finalization. Imagine a child reconciler that creates a resource in a different cluster than the parent, and the credentials are in a secret referenced by the parent resource. A sync reconciler can lookup the credentials and then stash the value to be used later. Since this logic is needed for both the Sync (create/update) and Finalize (delete) phases, it's cleaner to define it once and then call the method for both.

[mamachanko]

Deployed our kit with this branch. It checks out! Looking forward to using it in production.

LGTM @scothis 👌🏻

[mamachanko]

[question, non-blocking]: Why is AggregateResults exported? When would a consumer of rr use it?

[scothis]

Could be useful to someone implementing a composed SubReconciler