From a6cf8bfd6a62e73a73db1c8acbbefd69db42f240 Mon Sep 17 00:00:00 2001 From: Scott Andrews Date: Sat, 16 Apr 2022 11:50:49 -0400 Subject: [PATCH 1/3] Finalizers 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 --- README.md | 48 +++ reconcilers/reconcilers.go | 235 ++++++++++++-- reconcilers/reconcilers_test.go | 386 +++++++++++++++++++++++ reconcilers/reconcilers_validate_test.go | 88 ++++++ testing/subreconciler.go | 8 + 5 files changed, 738 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index bb9e544..32cc78f 100644 --- a/README.md +++ b/README.md @@ -25,6 +25,7 @@ - [Stash](#stash) - [Tracker](#tracker) - [Status](#status) + - [Finalizers](#finalizers) - [Contributing](#contributing) - [Acknowledgements](#acknowledgements) - [License](#license) @@ -75,6 +76,8 @@ The [`SubReconciler`](https://pkg.go.dev/github.com/vmware-labs/reconciler-runti The [`SyncReconciler`](https://pkg.go.dev/github.com/vmware-labs/reconciler-runtime/reconcilers#SyncReconciler) is the minimal type-aware sub reconciler. It is used to manage a portion of the parent's reconciliation that is custom, or whose behavior is not covered by another sub reconciler type. Common uses include looking up reference data for the reconciliation, or controlling resources that are not kubernetes resources. +When a resource is deleted that has pending finalizers, the Finalize method is called instead of the Sync method. If the SyncDuringFinalization field is true, the Sync method will also by called. If creating state that must be manually cleaned up, it is the users responsibility to define and clear finalizers. Using the [parent finalizer helper methods](#finalizers) is strongly encouraged with working under a [ParentReconciler](#parentreconciler). + **Example:** While sync reconcilers have the ability to do anything a reconciler can do, it's best to keep them focused on a single goal, letting the parent reconciler structure multiple sub reconcilers together. In this case, we use the parent resource and the client to resolve the target image and stash the value on the parent's status. The status is a good place to stash simple values that can be made public. More [advanced forms of stashing](#stash) are also available. Learn more about [status and its contract](#status). @@ -110,6 +113,7 @@ The ChildReconciler is responsible for: - logging the reconcilers activities - recording child mutations and errors for the parent resource - adapting to child resource changes applied by mutating webhooks +- adding and clearing of a finalizer, if specified The implementor is responsible for: - defining the desired resource @@ -118,6 +122,10 @@ The implementor is responsible for: - updating the parent's status from the child - defining the status subresource [according to the contract](#status) +When a finalizer is defined, the parent resource is patched to add the finalizer before creating the child; it is removed after the child is deleted. If the parent resource is pending deletion, the desired child method is not called, and existing children are deleted. + +> Warning: It is crucial that each ChildReconciler using a finalizer have a unique and stable finalizer name. Two reconcilers that use the same finalizer, or a reconciler that changed the name of its finalizer, may leak the child resource when the parent is deleted, or the parent resource may never terminate. + **Example:** Now it's time to create the child Image resource that will do the work of building our Function. This reconciler looks more more complex than what we have seen so far, each function on the reconciler provides a focused hook into the lifecycle being orchestrated by the ChildReconciler. @@ -516,6 +524,46 @@ type MyResource struct { } ``` +### Finalizers + +[Finalizers](https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers/) allow a reconciler to clean up state for a resource that has been deleted by a client, and not yet fully removed. Terminating resources have `.metadata.deletionTimestamp` set. Resources with finalizers will stay in this terminating state until all finalizers are cleared from the resource. While using the [Kubernetes garbage collector](https://kubernetes.io/docs/concepts/architecture/garbage-collection/) is recommended when possible, finalizer are useful for cases when state exists outside of the same cluster, scope, and namespace of the parent resource that needs to be cleaned up when no longer used. + +Deleting a resource that uses finalizers requires the controller to be running. + +The [AddParentFinalizer](https://pkg.go.dev/github.com/vmware-labs/reconciler-runtime/reconcilers#AddParentFinalizer) and [ClearParentFinalizer](https://pkg.go.dev/github.com/vmware-labs/reconciler-runtime/reconcilers#ClearParentFinalizer) functions patch the parent resource to update its finalizers. These methods work with [CastParents](#castparent) resources and use the same client the [ParentReconciler](#parentreconciler) used to originally load the parent resource. They can be called inside [SubReconcilers](#subreconciler) that may use a different client. + +When an update is required, only the `.metadata.finalizers` field is patched. The parent's `.metadata.resourceVersion` is used as an optimistic concurrency lock, and is updated with the value returned from the server. Any error from the server will cause the resource reconciliation to err. When testing with the [SubReconcilerTestSuite](#subreconcilertestsuite), the resource version of the parent defaults to `"999"`, the patch bytes include the resource version and the response increments the parent's resource version. For a parent with the default resource version that patches a finalizer, the expected parent will have a resource version of `"1000"`. + +A minimal test case for a sub reconciler that adds a finalizer may look like: + +```go + ... + { + Name: "add 'test.finalizer' finalizer", + Parent: resourceDie, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resourceDie, scheme, corev1.EventTypeNormal, "FinalizerPatched", + `Patched finalizer %q`, "test.finalizer"), + }, + ExpectParent: resourceDie. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers("test.finalizer") + d.ResourceVersion("1000") + }), + ExpectPatches: []rtesting.PatchRef{ + { + Group: "testing.reconciler.runtime", + Kind: "TestResource", + Namespace: resourceDie.GetNamespace(), + Name: resourceDie.GetName(), + PatchType: types.MergePatchType, + Patch: []byte(`{"metadata":{"finalizers":["test.finalizer"],"resourceVersion":"999"}}`), + }, + }, + }, + ... +``` + ## Contributing The reconciler-runtime project team welcomes contributions from the community. If you wish to contribute code and you have not signed our contributor license agreement (CLA), our bot will update the issue when you open a Pull Request. For any questions about the CLA process, please refer to our [FAQ](https://cla.vmware.com/faq). For more detailed information, refer to [CONTRIBUTING.md](CONTRIBUTING.md). diff --git a/reconcilers/reconcilers.go b/reconcilers/reconcilers.go index 29b563b..49a12b5 100644 --- a/reconcilers/reconcilers.go +++ b/reconcilers/reconcilers.go @@ -28,6 +28,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/cluster" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -71,12 +72,9 @@ func NewConfig(mgr ctrl.Manager, apiType client.Object, syncPeriod time.Duration name := typeName(apiType) log := newWarnOnceLogger(ctrl.Log.WithName("controllers").WithName(name)) return Config{ - Client: mgr.GetClient(), - APIReader: mgr.GetAPIReader(), - Recorder: mgr.GetEventRecorderFor("controller"), - Log: log, - Tracker: tracker.New(syncPeriod), - } + Log: log, + Tracker: tracker.New(syncPeriod), + }.WithCluster(mgr) } // ParentReconciler is a controller-runtime reconciler that reconciles a given @@ -187,7 +185,8 @@ func (r *ParentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr } func (r *ParentReconciler) reconcile(ctx context.Context, parent client.Object) (ctrl.Result, error) { - if parent.GetDeletionTimestamp() != nil { + if parent.GetDeletionTimestamp() != nil && len(parent.GetFinalizers()) == 0 { + // resource is being deleted and has no pending finalizers, nothing to do return ctrl.Result{}, nil } @@ -348,12 +347,29 @@ type SyncReconciler struct { // +optional Setup func(ctx context.Context, mgr ctrl.Manager, bldr *builder.Builder) error - // Sync does whatever work is necessary for the reconciler + // SyncDuringFinalization indicates the the Sync method should be called regardless of whether + SyncDuringFinalization bool + + // Sync does whatever work is necessary for the reconciler. + // + // If SyncDuringFinalization is true this method is called when the resource is pending + // deletion. This is useful if the reconciler is managing reference data. // // Expected function signature: // func(ctx context.Context, parent client.Object) error // func(ctx context.Context, parent client.Object) (ctrl.Result, error) Sync interface{} + + // Finalize does whatever work is necessary for the reconciler when the resource is pending + // deletion. If this reconciler sets a finalizer it should do the necessary work to clean up + // state the finalizer represents and then clear the finalizer. + // + // Expected function signature: + // func(ctx context.Context, parent client.Object) error + // func(ctx context.Context, parent client.Object) (ctrl.Result, error) + // + // +optional + Finalize interface{} } func (r *SyncReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, bldr *builder.Builder) error { @@ -402,6 +418,34 @@ func (r *SyncReconciler) validate(ctx context.Context) error { } } + // validate Finalize function signature: + // nil + // func(ctx context.Context, parent client.Object) error + // func(ctx context.Context, parent client.Object) (ctrl.Result, error) + if r.Finalize != nil { + castParentType := RetrieveCastParentType(ctx) + fn := reflect.TypeOf(r.Finalize) + err := fmt.Errorf("SyncReconciler must implement Finalize: nil | func(context.Context, %s) error | func(context.Context, %s) (ctrl.Result, error), found: %s", reflect.TypeOf(castParentType), reflect.TypeOf(castParentType), fn) + if fn.NumIn() != 2 || + !reflect.TypeOf((*context.Context)(nil)).Elem().AssignableTo(fn.In(0)) || + !reflect.TypeOf(castParentType).AssignableTo(fn.In(1)) { + return err + } + switch fn.NumOut() { + case 1: + if !reflect.TypeOf((*error)(nil)).Elem().AssignableTo(fn.Out(0)) { + return err + } + case 2: + if !reflect.TypeOf(ctrl.Result{}).AssignableTo(fn.Out(0)) || + !reflect.TypeOf((*error)(nil)).Elem().AssignableTo(fn.Out(1)) { + return err + } + default: + return err + } + } + return nil } @@ -412,10 +456,24 @@ func (r *SyncReconciler) Reconcile(ctx context.Context, parent client.Object) (c } ctx = logr.NewContext(ctx, log) - result, err := r.sync(ctx, parent) - if err != nil { - log.Error(err, "unable to sync") - return ctrl.Result{}, err + result := ctrl.Result{} + + if parent.GetDeletionTimestamp() == nil || r.SyncDuringFinalization { + syncResult, err := r.sync(ctx, parent) + if err != nil { + log.Error(err, "unable to sync") + return ctrl.Result{}, err + } + result = AggregateResults(result, syncResult) + } + + if parent.GetDeletionTimestamp() != nil { + finalizeResult, err := r.finalize(ctx, parent) + if err != nil { + log.Error(err, "unable to finalize") + return ctrl.Result{}, err + } + result = AggregateResults(result, finalizeResult) } return result, nil @@ -443,6 +501,32 @@ func (r *SyncReconciler) sync(ctx context.Context, parent client.Object) (ctrl.R return result, err } +func (r *SyncReconciler) finalize(ctx context.Context, parent client.Object) (ctrl.Result, error) { + if r.Finalize == nil { + return ctrl.Result{}, nil + } + + fn := reflect.ValueOf(r.Finalize) + out := fn.Call([]reflect.Value{ + reflect.ValueOf(ctx), + reflect.ValueOf(parent), + }) + result := ctrl.Result{} + var err error + switch len(out) { + case 2: + result = out[0].Interface().(ctrl.Result) + if !out[1].IsNil() { + err = out[1].Interface().(error) + } + case 1: + if !out[0].IsNil() { + err = out[0].Interface().(error) + } + } + return result, err +} + var ( OnlyReconcileChildStatus = errors.New("skip reconciler create/update/delete behavior for the child resource, while still reflecting the existing child's status on the parent") ) @@ -479,6 +563,18 @@ type ChildReconciler struct { // PodList is the list type for Pod ChildListType client.ObjectList + // Finalizer is set on the parent resource before a child resource is created, and cleared + // after a child resource is deleted. The value must be unique to this specific reconciler + // instance and not shared. Reusing a value may result in orphaned resources when the parent + // resource is deleted. + // + // Using a finalizer is encouraged when the Kubernetes garbage collector is unable to delete + // the child resource automatically, like when the parent and child are in different + // namespaces, scopes or clusters. + // + // +optional + Finalizer string + // Setup performs initialization on the manager and builder this reconciler // will run with. It's common to setup field indexes and watch resources. // @@ -707,7 +803,7 @@ func (r *ChildReconciler) Reconcile(ctx context.Context, parent client.Object) ( // the created child from a previous turn may be slow to appear in the informer cache, but shouldn't appear // on the parent as being not ready. apierr := err.(apierrs.APIStatus) - conflicted := r.ChildType.DeepCopyObject().(client.Object) + conflicted := newEmpty(r.ChildType).(client.Object) _ = c.APIReader.Get(ctx, types.NamespacedName{Namespace: parent.GetNamespace(), Name: apierr.Status().Details.Name}, conflicted) if r.ourChild(parent, conflicted) { // skip updating the parent's status, fail and try again @@ -730,8 +826,8 @@ func (r *ChildReconciler) reconcile(ctx context.Context, parent client.Object) ( pc := RetrieveParentConfigOrDie(ctx) c := RetrieveConfigOrDie(ctx) - actual := r.ChildType.DeepCopyObject().(client.Object) - children := r.ChildListType.DeepCopyObject().(client.ObjectList) + actual := newEmpty(r.ChildType).(client.Object) + children := newEmpty(r.ChildListType).(client.ObjectList) if err := c.List(ctx, children, client.InNamespace(parent.GetNamespace())); err != nil { return nil, err } @@ -780,10 +876,18 @@ func (r *ChildReconciler) reconcile(ctx context.Context, parent client.Object) ( } pc.Recorder.Eventf(parent, corev1.EventTypeNormal, "Deleted", "Deleted %s %q", typeName(r.ChildType), actual.GetName()) + + if err := ClearParentFinalizer(ctx, parent, r.Finalizer); err != nil { + return nil, err + } } return nil, nil } + if err := AddParentFinalizer(ctx, parent, r.Finalizer); err != nil { + return nil, err + } + // create child if it doesn't exist if actual.GetName() == "" { log.Info("creating child", "child", r.sanitize(desired)) @@ -856,6 +960,11 @@ func (r *ChildReconciler) semanticEquals(a1, a2 client.Object) bool { } func (r *ChildReconciler) desiredChild(ctx context.Context, parent client.Object) (client.Object, error) { + if parent.GetDeletionTimestamp() != nil { + // the parent is pending deletion, cleanup the child resource + return nil, nil + } + fn := reflect.ValueOf(r.DesiredChild) out := fn.Call([]reflect.Value{ reflect.ValueOf(ctx), @@ -981,23 +1090,12 @@ func (r Sequence) Reconcile(ctx context.Context, parent client.Object) (ctrl.Res if err != nil { return ctrl.Result{}, err } - aggregateResult = r.aggregateResult(result, aggregateResult) + aggregateResult = AggregateResults(result, aggregateResult) } return aggregateResult, nil } -func (r Sequence) aggregateResult(result, aggregate ctrl.Result) ctrl.Result { - if result.RequeueAfter != 0 && (aggregate.RequeueAfter == 0 || result.RequeueAfter < aggregate.RequeueAfter) { - aggregate.RequeueAfter = result.RequeueAfter - } - if result.Requeue { - aggregate.Requeue = true - } - - return aggregate -} - // CastParent casts the ParentReconciler's type by projecting the resource data // onto a new struct. Casting the parent resource is useful to create cross // cutting reconcilers that can operate on common portion of multiple parent @@ -1163,6 +1261,89 @@ func namespaceName(obj client.Object) types.NamespacedName { } } +// AddParentFinalizer ensures the desired finalizer exists on the parent resource. The client that +// loaded the parent resource is used to patch it with the finalizer if not already set. +func AddParentFinalizer(ctx context.Context, parent client.Object, finalizer string) error { + return ensureParentFinalizer(ctx, parent, finalizer, true) +} + +// ClearParentFinalizer ensures the desired finalizer does not exist on the parent resource. The +// client that loaded the parent resource is used to patch it with the finalizer if set. +func ClearParentFinalizer(ctx context.Context, parent client.Object, finalizer string) error { + return ensureParentFinalizer(ctx, parent, finalizer, false) +} + +func ensureParentFinalizer(ctx context.Context, parent client.Object, finalizer string, add bool) error { + config := RetrieveParentConfig(ctx) + if config.IsEmpty() { + panic(fmt.Errorf("parent config must exist on the context. Check that the context from a ParentReconciler")) + } + parentType := RetrieveParentType(ctx) + if parentType == nil { + panic(fmt.Errorf("parent type must exist on the context. Check that the context from a ParentReconciler")) + } + + if finalizer == "" || controllerutil.ContainsFinalizer(parent, finalizer) == add { + // nothing to do + return nil + } + + // cast the current object back to the parent so scheme-aware, typed client can operate on it + cast := &CastParent{ + Type: parentType, + Reconciler: &SyncReconciler{ + SyncDuringFinalization: true, + Sync: func(ctx context.Context, current client.Object) error { + log := logr.FromContextOrDiscard(ctx) + + desired := current.DeepCopyObject().(client.Object) + if add { + log.Info("adding parent finalizer", "finalizer", finalizer) + controllerutil.AddFinalizer(desired, finalizer) + } else { + log.Info("removing parent finalizer", "finalizer", finalizer) + controllerutil.RemoveFinalizer(desired, finalizer) + } + + patch := client.MergeFromWithOptions(current, client.MergeFromWithOptimisticLock{}) + if err := config.Patch(ctx, desired, patch); err != nil { + log.Error(err, "unable to patch parent finalizers", "finalizer", finalizer) + config.Recorder.Eventf(current, corev1.EventTypeWarning, "FinalizerPatchFailed", + "Failed to patch finalizer %q: %s", finalizer, err) + return err + } + config.Recorder.Eventf(current, corev1.EventTypeNormal, "FinalizerPatched", + "Patched finalizer %q", finalizer) + + // update current object with values from the api server after patching + current.SetFinalizers(desired.GetFinalizers()) + current.SetResourceVersion(desired.GetResourceVersion()) + current.SetGeneration(desired.GetGeneration()) + + return nil + }, + }, + } + + _, err := cast.Reconcile(ctx, parent) + return err +} + +// AggregateResults combines multiple results into a single result. If any result requests +// requeue, the aggregate is requeued. The shortest non-zero requeue after is the aggregate value. +func AggregateResults(results ...ctrl.Result) ctrl.Result { + aggregate := ctrl.Result{} + for _, result := range results { + if result.RequeueAfter != 0 && (aggregate.RequeueAfter == 0 || result.RequeueAfter < aggregate.RequeueAfter) { + aggregate.RequeueAfter = result.RequeueAfter + } + if result.Requeue { + aggregate.Requeue = true + } + } + return aggregate +} + // MergeMaps flattens a sequence of maps into a single map. Keys in latter maps // overwrite previous keys. None of the arguments are mutated. func MergeMaps(maps ...map[string]string) map[string]string { diff --git a/reconcilers/reconcilers_test.go b/reconcilers/reconcilers_test.go index c7964b5..ddaf430 100644 --- a/reconcilers/reconcilers_test.go +++ b/reconcilers/reconcilers_test.go @@ -350,6 +350,8 @@ func TestSyncReconciler(t *testing.T) { testNamespace := "test-namespace" testName := "test-resource" + now := metav1.Now() + scheme := runtime.NewScheme() _ = resources.AddToScheme(scheme) @@ -414,6 +416,117 @@ func TestSyncReconciler(t *testing.T) { }, }, ShouldPanic: true, + }, { + Name: "should not finalize non-deleted resources", + Parent: resource, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return &reconcilers.SyncReconciler{ + Sync: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + return ctrl.Result{RequeueAfter: 2 * time.Hour}, nil + }, + Finalize: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + t.Errorf("reconciler should not call finalize for non-deleted resources") + return ctrl.Result{RequeueAfter: 3 * time.Hour}, nil + }, + } + }, + }, + ExpectedResult: reconcile.Result{RequeueAfter: 2 * time.Hour}, + }, { + Name: "should not finalize deleted resources", + Parent: resource. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&now) + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return &reconcilers.SyncReconciler{ + Sync: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + t.Errorf("reconciler should not call sync for deleted resources") + return ctrl.Result{RequeueAfter: 2 * time.Hour}, nil + }, + Finalize: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + return ctrl.Result{RequeueAfter: 3 * time.Hour}, nil + }, + } + }, + }, + ExpectedResult: reconcile.Result{RequeueAfter: 3 * time.Hour}, + }, { + Name: "should finalize and sync deleted resources when asked to", + Parent: resource. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&now) + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return &reconcilers.SyncReconciler{ + SyncDuringFinalization: true, + Sync: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + return ctrl.Result{RequeueAfter: 2 * time.Hour}, nil + }, + Finalize: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + return ctrl.Result{RequeueAfter: 3 * time.Hour}, nil + }, + } + }, + }, + ExpectedResult: reconcile.Result{RequeueAfter: 2 * time.Hour}, + }, { + Name: "should finalize and sync deleted resources when asked to, shorter resync wins", + Parent: resource. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&now) + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return &reconcilers.SyncReconciler{ + SyncDuringFinalization: true, + Sync: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + return ctrl.Result{RequeueAfter: 3 * time.Hour}, nil + }, + Finalize: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + return ctrl.Result{RequeueAfter: 2 * time.Hour}, nil + }, + } + }, + }, + ExpectedResult: reconcile.Result{RequeueAfter: 2 * time.Hour}, + }, { + Name: "finalize is optional", + Parent: resource. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&now) + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return &reconcilers.SyncReconciler{ + Sync: func(ctx context.Context, parent *resources.TestResource) error { + return nil + }, + } + }, + }, + }, { + Name: "finalize error", + Parent: resource. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&now) + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return &reconcilers.SyncReconciler{ + Sync: func(ctx context.Context, parent *resources.TestResource) error { + return nil + }, + Finalize: func(ctx context.Context, parent *resources.TestResource) error { + return fmt.Errorf("syncreconciler finalize error") + }, + } + }, + }, + ShouldErr: true, }} rts.Test(t, scheme, func(t *testing.T, rtc *rtesting.SubReconcilerTestCase, c reconcilers.Config) reconcilers.SubReconciler { @@ -424,6 +537,7 @@ func TestSyncReconciler(t *testing.T) { func TestChildReconciler(t *testing.T) { testNamespace := "test-namespace" testName := "test-resource" + testFinalizer := "test.finalizer" scheme := runtime.NewScheme() _ = resources.AddToScheme(scheme) @@ -552,6 +666,49 @@ func TestChildReconciler(t *testing.T) { ExpectCreates: []client.Object{ configMapCreate, }, + }, { + Name: "create child with finalizer", + Parent: resource. + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "FinalizerPatched", + `Patched finalizer %q`, testFinalizer), + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Created", + `Created ConfigMap %q`, testName), + }, + ExpectParent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers(testFinalizer) + d.ResourceVersion("1000") + }). + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + }). + StatusDie(func(d *dies.TestResourceStatusDie) { + d.AddField("foo", "bar") + }), + ExpectCreates: []client.Object{ + configMapCreate, + }, + ExpectPatches: []rtesting.PatchRef{ + { + Group: "testing.reconciler.runtime", + Kind: "TestResource", + Namespace: testNamespace, + Name: testName, + PatchType: types.MergePatchType, + Patch: []byte(`{"metadata":{"finalizers":["test.finalizer"],"resourceVersion":"999"}}`), + }, + }, }, { Name: "update child", Parent: resourceReady. @@ -587,6 +744,102 @@ func TestChildReconciler(t *testing.T) { configMapGiven. AddData("new", "field"), }, + }, { + Name: "update child, preserve finalizers", + Parent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers(testFinalizer, "some.other.finalizer") + }). + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + d.AddField("new", "field") + }). + StatusDie(func(d *dies.TestResourceStatusDie) { + d.AddField("foo", "bar") + }), + GivenObjects: []client.Object{ + configMapGiven, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Updated", + `Updated ConfigMap %q`, testName), + }, + ExpectParent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers(testFinalizer, "some.other.finalizer") + }). + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + d.AddField("new", "field") + }). + StatusDie(func(d *dies.TestResourceStatusDie) { + d.AddField("foo", "bar") + d.AddField("new", "field") + }), + ExpectUpdates: []client.Object{ + configMapGiven. + AddData("new", "field"), + }, + }, { + Name: "update child, restoring missing finalizer", + Parent: resourceReady. + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + d.AddField("new", "field") + }). + StatusDie(func(d *dies.TestResourceStatusDie) { + d.AddField("foo", "bar") + }), + GivenObjects: []client.Object{ + configMapGiven, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "FinalizerPatched", + `Patched finalizer %q`, testFinalizer), + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Updated", + `Updated ConfigMap %q`, testName), + }, + ExpectParent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers(testFinalizer) + d.ResourceVersion("1000") + }). + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + d.AddField("new", "field") + }). + StatusDie(func(d *dies.TestResourceStatusDie) { + d.AddField("foo", "bar") + d.AddField("new", "field") + }), + ExpectUpdates: []client.Object{ + configMapGiven. + AddData("new", "field"), + }, + ExpectPatches: []rtesting.PatchRef{ + { + Group: "testing.reconciler.runtime", + Kind: "TestResource", + Namespace: testNamespace, + Name: testName, + PatchType: types.MergePatchType, + Patch: []byte(`{"metadata":{"finalizers":["test.finalizer"],"resourceVersion":"999"}}`), + }, + }, }, { Name: "delete child", Parent: resourceReady, @@ -605,6 +858,69 @@ func TestChildReconciler(t *testing.T) { ExpectDeletes: []rtesting.DeleteRef{ {Group: "", Kind: "ConfigMap", Namespace: testNamespace, Name: testName}, }, + }, { + Name: "delete child, clearing finalizer", + Parent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers(testFinalizer) + }), + GivenObjects: []client.Object{ + configMapGiven, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Deleted", + `Deleted ConfigMap %q`, testName), + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "FinalizerPatched", + `Patched finalizer %q`, testFinalizer), + }, + ExpectParent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers() + d.ResourceVersion("1000") + }), + ExpectDeletes: []rtesting.DeleteRef{ + {Group: "", Kind: "ConfigMap", Namespace: testNamespace, Name: testName}, + }, + ExpectPatches: []rtesting.PatchRef{ + { + Group: "testing.reconciler.runtime", + Kind: "TestResource", + Namespace: testNamespace, + Name: testName, + PatchType: types.MergePatchType, + Patch: []byte(`{"metadata":{"finalizers":null,"resourceVersion":"999"}}`), + }, + }, + }, { + Name: "delete child, preserve other finalizer", + Parent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers("some.other.finalizer") + }), + GivenObjects: []client.Object{ + configMapGiven, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Deleted", + `Deleted ConfigMap %q`, testName), + }, + ExpectDeletes: []rtesting.DeleteRef{ + {Group: "", Kind: "ConfigMap", Namespace: testNamespace, Name: testName}, + }, }, { Name: "ignore extraneous children", Parent: resourceReady, @@ -828,6 +1144,76 @@ func TestChildReconciler(t *testing.T) { configMapCreate, }, ShouldErr: true, + }, { + Name: "error adding finalizer", + Parent: resource. + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + WithReactors: []rtesting.ReactionFunc{ + rtesting.InduceFailure("patch", "TestResource"), + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeWarning, "FinalizerPatchFailed", + `Failed to patch finalizer %q: inducing failure for patch TestResource`, testFinalizer), + }, + ExpectPatches: []rtesting.PatchRef{ + { + Group: "testing.reconciler.runtime", + Kind: "TestResource", + Namespace: testNamespace, + Name: testName, + PatchType: types.MergePatchType, + Patch: []byte(`{"metadata":{"finalizers":["test.finalizer"],"resourceVersion":"999"}}`), + }, + }, + ShouldErr: true, + }, { + Name: "error clearing finalizer", + Parent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers(testFinalizer) + }), + GivenObjects: []client.Object{ + configMapGiven, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + WithReactors: []rtesting.ReactionFunc{ + rtesting.InduceFailure("patch", "TestResource"), + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Deleted", + `Deleted ConfigMap %q`, testName), + rtesting.NewEvent(resource, scheme, corev1.EventTypeWarning, "FinalizerPatchFailed", + `Failed to patch finalizer %q: inducing failure for patch TestResource`, testFinalizer), + }, + ExpectDeletes: []rtesting.DeleteRef{ + {Group: "", Kind: "ConfigMap", Namespace: testNamespace, Name: testName}, + }, + ExpectPatches: []rtesting.PatchRef{ + { + Group: "testing.reconciler.runtime", + Kind: "TestResource", + Namespace: testNamespace, + Name: testName, + PatchType: types.MergePatchType, + Patch: []byte(`{"metadata":{"finalizers":null,"resourceVersion":"999"}}`), + }, + }, + ShouldErr: true, }, { Name: "error updating child", Parent: resourceReady. diff --git a/reconcilers/reconcilers_validate_test.go b/reconcilers/reconcilers_validate_test.go index b4a802d..858cf62 100644 --- a/reconcilers/reconcilers_validate_test.go +++ b/reconcilers/reconcilers_validate_test.go @@ -95,6 +95,94 @@ func TestSyncReconciler_validate(t *testing.T) { }, shouldErr: "SyncReconciler must implement Sync: func(context.Context, *v1.ConfigMap) error | func(context.Context, *v1.ConfigMap) (ctrl.Result, error), found: func(context.Context, *v1.ConfigMap) (reconcile.Result, string)", }, + { + name: "valid Finalize", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + }, + }, + { + name: "valid Finalize with result", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func(ctx context.Context, parent *corev1.ConfigMap) (ctrl.Result, error) { + return ctrl.Result{}, nil + }, + }, + }, + { + name: "Finalize num in", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func() error { + return nil + }, + }, + shouldErr: "SyncReconciler must implement Finalize: nil | func(context.Context, *v1.ConfigMap) error | func(context.Context, *v1.ConfigMap) (ctrl.Result, error), found: func() error", + }, + { + name: "Finalize in 1", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func(ctx context.Context, parent *corev1.Secret) error { + return nil + }, + }, + shouldErr: "SyncReconciler must implement Finalize: nil | func(context.Context, *v1.ConfigMap) error | func(context.Context, *v1.ConfigMap) (ctrl.Result, error), found: func(context.Context, *v1.Secret) error", + }, + { + name: "Finalize num out", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func(ctx context.Context, parent *corev1.ConfigMap) { + }, + }, + shouldErr: "SyncReconciler must implement Finalize: nil | func(context.Context, *v1.ConfigMap) error | func(context.Context, *v1.ConfigMap) (ctrl.Result, error), found: func(context.Context, *v1.ConfigMap)", + }, + { + name: "Finalize out 1", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func(ctx context.Context, parent *corev1.ConfigMap) string { + return "" + }, + }, + shouldErr: "SyncReconciler must implement Finalize: nil | func(context.Context, *v1.ConfigMap) error | func(context.Context, *v1.ConfigMap) (ctrl.Result, error), found: func(context.Context, *v1.ConfigMap) string", + }, + { + name: "Finalize result out 1", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func(ctx context.Context, parent *corev1.ConfigMap) (ctrl.Result, string) { + return ctrl.Result{}, "" + }, + }, + shouldErr: "SyncReconciler must implement Finalize: nil | func(context.Context, *v1.ConfigMap) error | func(context.Context, *v1.ConfigMap) (ctrl.Result, error), found: func(context.Context, *v1.ConfigMap) (reconcile.Result, string)", + }, } for _, c := range tests { diff --git a/testing/subreconciler.go b/testing/subreconciler.go index d794808..5e6f16c 100644 --- a/testing/subreconciler.go +++ b/testing/subreconciler.go @@ -166,6 +166,10 @@ func (tc *SubReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, facto ctx = reconcilers.StashParentConfig(ctx, c) parent := tc.Parent.DeepCopyObject().(client.Object) + if parent.GetResourceVersion() == "" { + // this value is also set by the test client when resource are added as givens + parent.SetResourceVersion("999") + } ctx = reconcilers.StashParentType(ctx, parent.DeepCopyObject().(client.Object)) ctx = reconcilers.StashCastParentType(ctx, parent.DeepCopyObject().(client.Object)) @@ -201,6 +205,10 @@ func (tc *SubReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, facto if tc.ExpectParent != nil { expectedParent = tc.ExpectParent.DeepCopyObject().(client.Object) } + if expectedParent.GetResourceVersion() == "" { + // mirror defaulting of the parent + expectedParent.SetResourceVersion("999") + } if diff := cmp.Diff(expectedParent, parent, IgnoreLastTransitionTime, SafeDeployDiff, IgnoreTypeMeta, cmpopts.EquateEmpty()); diff != "" { t.Errorf("Unexpected parent mutations(-expected, +actual): %s", diff) } From ed8b20245264e5a5218b0eb75c27d5a5cc03c902 Mon Sep 17 00:00:00 2001 From: Scott Andrews Date: Tue, 3 May 2022 10:40:25 -0400 Subject: [PATCH 2/3] review feedback Signed-off-by: Scott Andrews --- reconcilers/reconcilers.go | 3 ++ reconcilers/reconcilers_test.go | 60 ++++++++++++++++++++++++++++----- testing/reconciler.go | 14 ++++++++ 3 files changed, 68 insertions(+), 9 deletions(-) diff --git a/reconcilers/reconcilers.go b/reconcilers/reconcilers.go index 49a12b5..d1ea260 100644 --- a/reconcilers/reconcilers.go +++ b/reconcilers/reconcilers.go @@ -797,6 +797,9 @@ func (r *ChildReconciler) Reconcile(ctx context.Context, parent client.Object) ( } child, err := r.reconcile(ctx, parent) + if parent.GetDeletionTimestamp() != nil { + return ctrl.Result{}, err + } if err != nil { if apierrs.IsAlreadyExists(err) { // check if the resource blocking create is owned by the parent. diff --git a/reconcilers/reconcilers_test.go b/reconcilers/reconcilers_test.go index ddaf430..bb4fc8f 100644 --- a/reconcilers/reconcilers_test.go +++ b/reconcilers/reconcilers_test.go @@ -131,9 +131,7 @@ func TestParentReconciler(t *testing.T) { Name: "error fetching resource", Key: testKey, GivenObjects: []client.Object{ - resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) { - d.DeletionTimestamp(&deletedAt) - }), + resource, }, WithReactors: []rtesting.ReactionFunc{ rtesting.InduceFailure("get", "TestResource"), @@ -434,7 +432,7 @@ func TestSyncReconciler(t *testing.T) { }, ExpectedResult: reconcile.Result{RequeueAfter: 2 * time.Hour}, }, { - Name: "should not finalize deleted resources", + Name: "should finalize deleted resources", Parent: resource. MetadataDie(func(d *diemetav1.ObjectMetaDie) { d.DeletionTimestamp(&now) @@ -539,6 +537,8 @@ func TestChildReconciler(t *testing.T) { testName := "test-resource" testFinalizer := "test.finalizer" + now := metav1.NewTime(time.Now().Truncate(time.Second)) + scheme := runtime.NewScheme() _ = resources.AddToScheme(scheme) _ = clientgoscheme.AddToScheme(scheme) @@ -856,7 +856,7 @@ func TestChildReconciler(t *testing.T) { `Deleted ConfigMap %q`, testName), }, ExpectDeletes: []rtesting.DeleteRef{ - {Group: "", Kind: "ConfigMap", Namespace: testNamespace, Name: testName}, + rtesting.NewDeleteRefFromObject(configMapGiven, scheme), }, }, { Name: "delete child, clearing finalizer", @@ -886,7 +886,7 @@ func TestChildReconciler(t *testing.T) { d.ResourceVersion("1000") }), ExpectDeletes: []rtesting.DeleteRef{ - {Group: "", Kind: "ConfigMap", Namespace: testNamespace, Name: testName}, + rtesting.NewDeleteRefFromObject(configMapGiven, scheme), }, ExpectPatches: []rtesting.PatchRef{ { @@ -919,7 +919,7 @@ func TestChildReconciler(t *testing.T) { `Deleted ConfigMap %q`, testName), }, ExpectDeletes: []rtesting.DeleteRef{ - {Group: "", Kind: "ConfigMap", Namespace: testNamespace, Name: testName}, + rtesting.NewDeleteRefFromObject(configMapGiven, scheme), }, }, { Name: "ignore extraneous children", @@ -979,6 +979,48 @@ func TestChildReconciler(t *testing.T) { ExpectCreates: []client.Object{ configMapCreate, }, + }, { + Name: "delete child durring finalization", + Parent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&now) + d.Finalizers(testFinalizer) + }), + GivenObjects: []client.Object{ + configMapGiven, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Deleted", + `Deleted ConfigMap %q`, testName), + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "FinalizerPatched", + `Patched finalizer %q`, testFinalizer), + }, + ExpectParent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&now) + d.Finalizers() + d.ResourceVersion("1000") + }), + ExpectDeletes: []rtesting.DeleteRef{ + rtesting.NewDeleteRefFromObject(configMapGiven, scheme), + }, + ExpectPatches: []rtesting.PatchRef{ + { + Group: "testing.reconciler.runtime", + Kind: "TestResource", + Namespace: testNamespace, + Name: testName, + PatchType: types.MergePatchType, + Patch: []byte(`{"metadata":{"finalizers":null,"resourceVersion":"999"}}`), + }, + }, }, { Name: "child name collision", Parent: resourceReady. @@ -1201,7 +1243,7 @@ func TestChildReconciler(t *testing.T) { `Failed to patch finalizer %q: inducing failure for patch TestResource`, testFinalizer), }, ExpectDeletes: []rtesting.DeleteRef{ - {Group: "", Kind: "ConfigMap", Namespace: testNamespace, Name: testName}, + rtesting.NewDeleteRefFromObject(configMapGiven, scheme), }, ExpectPatches: []rtesting.PatchRef{ { @@ -1263,7 +1305,7 @@ func TestChildReconciler(t *testing.T) { `Failed to delete ConfigMap %q: inducing failure for delete ConfigMap`, testName), }, ExpectDeletes: []rtesting.DeleteRef{ - {Group: "", Kind: "ConfigMap", Namespace: testNamespace, Name: testName}, + rtesting.NewDeleteRefFromObject(configMapGiven, scheme), }, ShouldErr: true, }, { diff --git a/testing/reconciler.go b/testing/reconciler.go index 04bcceb..89adef0 100644 --- a/testing/reconciler.go +++ b/testing/reconciler.go @@ -423,6 +423,20 @@ func NewDeleteRef(action DeleteAction) DeleteRef { } } +func NewDeleteRefFromObject(obj client.Object, scheme *runtime.Scheme) DeleteRef { + gvks, _, err := scheme.ObjectKinds(obj.DeepCopyObject()) + if err != nil { + panic(err) + } + + return DeleteRef{ + Group: gvks[0].Group, + Kind: gvks[0].Kind, + Namespace: obj.GetNamespace(), + Name: obj.GetName(), + } +} + type DeleteCollectionRef struct { Group string Kind string From 7f2cfffe6ca5e6d6d17e10f37064dcc3b34bfaf9 Mon Sep 17 00:00:00 2001 From: Scott Andrews Date: Fri, 6 May 2022 06:53:13 -0400 Subject: [PATCH 3/3] Update reconcilers/reconcilers.go Co-authored-by: Max Brauer --- reconcilers/reconcilers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reconcilers/reconcilers.go b/reconcilers/reconcilers.go index d1ea260..791dfdd 100644 --- a/reconcilers/reconcilers.go +++ b/reconcilers/reconcilers.go @@ -347,7 +347,7 @@ type SyncReconciler struct { // +optional Setup func(ctx context.Context, mgr ctrl.Manager, bldr *builder.Builder) error - // SyncDuringFinalization indicates the the Sync method should be called regardless of whether + // SyncDuringFinalization indicates the Sync method should be called when the resource is pending deletion. SyncDuringFinalization bool // Sync does whatever work is necessary for the reconciler.