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.

Sign up for GitHub

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

Jump to bottom

Optional finalize for parent reconciler #204

Closed
wants to merge 4 commits into from
Closed

Optional finalize for parent reconciler #204

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b8fd9d1
Add optional Finalize to ParentReconciler.
gmrodgers Apr 6, 2022
e692211
Be specific in finalizer update err and events.
gmrodgers Apr 7, 2022
47f6d8b
Add test case for finalizer update erroring.
gmrodgers Apr 8, 2022
a2b3906
Move Finalize method to SyncReconciler.
gmrodgers Apr 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Expand Up @@ -42,9 +42,11 @@ The parent is responsible for:
- updates the resource status if it was modified
- logging the reconcilers activities
- records events for mutations and errors
- adding and removing a finalizer if needed

The implementor is responsible for:
- defining the set of sub reconcilers
- providing instructions on how to clean up on deletion if needed

**Example:**

Expand Down
78 changes: 77 additions & 1 deletion reconcilers/reconcilers.go
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@ import (
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/webhook"

Expand Down Expand Up @@ -70,9 +71,18 @@ type ParentReconciler struct {

// Reconciler is called for each reconciler request with the parent
// resource being reconciled. Typically, Reconciler is a Sequence of
// multiple SubReconcilers.
// multiple SubReconcilers. It is not called on a delete request.
Reconciler SubReconciler

// Finalize is called on a delete request. Typically, Finalize cleans
// up any state, created by previous reconciles, of which the parent is
// not a Kubernetes owner.
//
// Expected function signature:
// func(ctx context.Context, parent client.Object) error
// +optional
Finalize interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@gmrodgers gmrodgers Apr 8, 2022 •
edited

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@gmrodgers gmrodgers Apr 15, 2022 •
edited

Choose a reason for hiding this comment

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

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!


Config
}

Expand All @@ -83,9 +93,32 @@ func (r *ParentReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manage
if err := r.Reconciler.SetupWithManager(ctx, mgr, bldr); err != nil {
return err
}

if err := r.validate(ctx); err != nil {
return err
}

return bldr.Complete(r)
}

func (r *ParentReconciler) validate(ctx context.Context) error {
castParentType := RetrieveCastParentType(ctx)
// validate Finalize function signature:
// nil
// func(ctx context.Context, parent client.Object) error
if r.Finalize != nil {
fn := reflect.TypeOf(r.Finalize)
if fn.NumIn() != 2 || fn.NumOut() != 1 ||
!reflect.TypeOf((*context.Context)(nil)).Elem().AssignableTo(fn.In(0)) ||
!reflect.TypeOf(castParentType).AssignableTo(fn.In(1)) ||
!reflect.TypeOf((*error)(nil)).Elem().AssignableTo(fn.Out(0)) {
return fmt.Errorf("ParentReconciler Finalize must have correct signature: func(context.Context, %s) error, found: %s", reflect.TypeOf(castParentType), fn)
}
}

return nil
}

func (r *ParentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
ctx = WithStash(ctx)
log := r.Log.WithValues("request", req.NamespacedName)
Expand Down Expand Up @@ -117,6 +150,7 @@ func (r *ParentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
initializeConditions.Call([]reflect.Value{})
}
}

result, err := r.reconcile(ctx, parent)

if r.hasStatus(originalParent) {
Expand All @@ -137,12 +171,41 @@ func (r *ParentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
"Updated status")
}
}

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

log.Error(updateErr, "unable to update", typeName(r.Type), parent)
r.Recorder.Eventf(parent, corev1.EventTypeWarning, "UpdateFailed",
"Failed to update: %v", updateErr)
gmrodgers marked this conversation as resolved.
Show resolved Hide resolved
return ctrl.Result{}, updateErr
}
r.Recorder.Eventf(parent, corev1.EventTypeNormal, "Updated", "Updated")
gmrodgers marked this conversation as resolved.
Show resolved Hide resolved
}

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if r.Finalize == nil {
controllerutil.RemoveFinalizer(parent, finalizer)
} else {
controllerutil.AddFinalizer(parent, finalizer)
}

if r.Finalize == nil && parent.GetDeletionTimestamp() != nil {
return ctrl.Result{}, nil
}

if parent.GetDeletionTimestamp() != nil {
if err := r.finalize(ctx, parent); err != nil {
return ctrl.Result{}, err
}

controllerutil.RemoveFinalizer(parent, finalizer)

return ctrl.Result{}, nil
}

Expand All @@ -154,6 +217,19 @@ func (r *ParentReconciler) reconcile(ctx context.Context, parent client.Object)
r.copyGeneration(parent)
return result, nil
}

func (r *ParentReconciler) finalize(ctx context.Context, parent client.Object) error {
fn := reflect.ValueOf(r.Finalize)
out := fn.Call([]reflect.Value{
reflect.ValueOf(ctx),
reflect.ValueOf(parent),
})
if !out[0].IsNil() {
return out[0].Interface().(error)
}
return nil
}

func (r *ParentReconciler) hasStatus(obj client.Object) bool {
return reflect.ValueOf(obj).Elem().FieldByName("Status").IsValid()
}
Expand Down
159 changes: 158 additions & 1 deletion reconcilers/reconcilers_test.go
View file
Open in desktop
Expand Up @@ -95,6 +95,7 @@ func TestParentReconciler(t *testing.T) {
)
})
deletedAt := metav1.NewTime(time.UnixMilli(2000))
const finalizer = "testing.reconciler.runtime/reconciler-runtime-finalize"

rts := rtesting.ReconcilerTestSuite{{
Name: "resource does not exist",
Expand Down Expand Up @@ -302,14 +303,170 @@ func TestParentReconciler(t *testing.T) {
}
},
},
}, {
Name: "has Finalize",
Key: testKey,
GivenObjects: []client.Object{
resource,
},
ExpectUpdates: []client.Object{
resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) {
d.Finalizers(finalizer)
}).SpecDie(func(d *dies.TestResourceSpecDie) {
d.AddField("Defaulter", "ran")
}),
},
ExpectEvents: []rtesting.Event{
rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Updated", `Updated`),
},
Metadata: map[string]interface{}{
"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
return &reconcilers.Sequence{}
},
"Finalize": func(ctx context.Context, parent *resources.TestResource) error {
return nil
},
},
}, {
Name: "has finalizer",
Key: testKey,
GivenObjects: []client.Object{
resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) {
d.Finalizers(finalizer)
}),
},
ExpectUpdates: []client.Object{
resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) {
d.Finalizers()
}).SpecDie(func(d *dies.TestResourceSpecDie) {
d.AddField("Defaulter", "ran")
}),
},
ExpectEvents: []rtesting.Event{
rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Updated", `Updated`),
},
Metadata: map[string]interface{}{
"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
return &reconcilers.Sequence{}
},
},
}, {
Name: "has Finalize and has finalizer",
Key: testKey,
GivenObjects: []client.Object{
resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) {
d.Finalizers(finalizer)
}),
},
Metadata: map[string]interface{}{
"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
return &reconcilers.Sequence{}
},
"Finalize": func(ctx context.Context, parent *resources.TestResource) error {
return nil
},
},
}, {
Name: "has finalize, has finalizer, and is deleted",
Key: testKey,
GivenObjects: []client.Object{
resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) {
d.DeletionTimestamp(&deletedAt)
d.Finalizers(finalizer)
}),
},
ExpectEvents: []rtesting.Event{
rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Updated", `Updated`),
},
ExpectUpdates: []client.Object{
resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) {
d.DeletionTimestamp(&deletedAt)
d.Finalizers()
}).SpecDie(func(d *dies.TestResourceSpecDie) {
d.AddField("Defaulter", "ran")
}),
},
Metadata: map[string]interface{}{
"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
return &reconcilers.Sequence{}
},
"Finalize": func(ctx context.Context, parent *resources.TestResource) error {
return nil
},
},
}, {
Name: "Finalize erred",
Key: testKey,
GivenObjects: []client.Object{
resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) {
d.DeletionTimestamp(&deletedAt)
d.Finalizers(finalizer)
}),
},
Metadata: map[string]interface{}{
"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
return &reconcilers.Sequence{}
},
"Finalize": func(ctx context.Context, parent *resources.TestResource) error {
return fmt.Errorf("Finalize error")
},
},
ShouldErr: true,
}, {
Name: "has Finalize and status",
Key: testKey,
GivenObjects: []client.Object{
resource,
},
ExpectUpdates: []client.Object{
resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) {
d.Finalizers(finalizer)
}).SpecDie(func(d *dies.TestResourceSpecDie) {
d.AddField("Defaulter", "ran")
}).StatusDie(func(d *dies.TestResourceStatusDie) {
d.AddField("Reconciler", "ran")
}),
},
ExpectEvents: []rtesting.Event{
rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "StatusUpdated",
`Updated status`),
rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Updated", `Updated`),
},
ExpectStatusUpdates: []client.Object{
resource.StatusDie(func(d *dies.TestResourceStatusDie) {
d.AddField("Reconciler", "ran")
}),
},
Metadata: map[string]interface{}{
"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
return &reconcilers.SyncReconciler{
Config: c,
Sync: func(ctx context.Context, parent *resources.TestResource) error {
if parent.Status.Fields == nil {
parent.Status.Fields = map[string]string{}
}
parent.Status.Fields["Reconciler"] = "ran"
return nil
},
}
},
"Finalize": func(ctx context.Context, parent *resources.TestResource) error {
return nil
},
},
}}

rts.Test(t, scheme, func(t *testing.T, rtc *rtesting.ReconcilerTestCase, c reconcilers.Config) reconcile.Reconciler {
return &reconcilers.ParentReconciler{
reconciler := &reconcilers.ParentReconciler{
Type: &resources.TestResource{},
Reconciler: rtc.Metadata["SubReconciler"].(func(*testing.T, reconcilers.Config) reconcilers.SubReconciler)(t, c),
Config: c,
}
if _, ok := rtc.Metadata["Finalize"]; ok {
reconciler.Finalize = rtc.Metadata["Finalize"].(func(context.Context, *resources.TestResource) error)
}

return reconciler
})
}

Expand Down