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

Optional finalize for parent reconciler #204

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
93 changes: 88 additions & 5 deletions reconcilers/reconcilers.go
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,7 +71,7 @@ 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

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

return bldr.Complete(r)
}

Expand Down Expand Up @@ -117,6 +119,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,15 +140,23 @@ 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 finalizers: %v", updateErr)
return ctrl.Result{}, updateErr
}
r.Recorder.Eventf(parent, corev1.EventTypeNormal, "Updated", "Updated finalizers")
}

// return original reconcile result
return result, err
}

func (r *ParentReconciler) reconcile(ctx context.Context, parent client.Object) (ctrl.Result, error) {
if parent.GetDeletionTimestamp() != nil {
return ctrl.Result{}, nil
}

result, err := r.Reconciler.Reconcile(ctx, parent)
if err != nil {
return ctrl.Result{}, err
Expand All @@ -154,6 +165,7 @@ func (r *ParentReconciler) reconcile(ctx context.Context, parent client.Object)
r.copyGeneration(parent)
return result, nil
}

func (r *ParentReconciler) hasStatus(obj client.Object) bool {
return reflect.ValueOf(obj).Elem().FieldByName("Status").IsValid()
}
Expand Down Expand Up @@ -263,6 +275,15 @@ type SyncReconciler struct {
// func(ctx context.Context, parent client.Object) (ctrl.Result, error)
Sync interface{}

// 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{}

Config
}

Expand Down Expand Up @@ -306,10 +327,56 @@ func (r *SyncReconciler) validate(ctx context.Context) error {
}
}

// validate Finalize function signature:
// nil
// func(ctx context.Context, parent client.Object) error
if r.Finalize != nil {
castParentType := RetrieveCastParentType(ctx)
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("SyncReconciler Finalize must have correct signature: func(context.Context, %s) error, found: %s", reflect.TypeOf(castParentType), fn)
}
}

return nil
}

func UniqueFinalizer(ctx context.Context, group string) string {
finalizerCount, ok := RetrieveValue(ctx, "finalizer-count").(int)
if !ok {
finalizerCount = 1
}
finalizer := fmt.Sprintf("%s/%d-finalizer", group, finalizerCount)
StashValue(ctx, "finalizer-count", finalizerCount+1)

return finalizer
}

func (r *SyncReconciler) Reconcile(ctx context.Context, parent client.Object) (ctrl.Result, error) {
finalizer := UniqueFinalizer(ctx, parent.GetObjectKind().GroupVersionKind().Group)
if r.Finalize != nil {
controllerutil.AddFinalizer(parent, finalizer)
} else {
controllerutil.RemoveFinalizer(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
}

result, err := r.sync(ctx, parent)
if err != nil {
r.Log.Error(err, "unable to sync", typeName(parent), parent)
Expand Down Expand Up @@ -341,6 +408,18 @@ 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) 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
}

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")
)
Expand Down Expand Up @@ -576,6 +655,10 @@ func (r *ChildReconciler) Reconcile(ctx context.Context, parent client.Object) (
r.mutationCache = cache.NewExpiring()
}

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

child, err := r.reconcile(ctx, parent)
if err != nil {
parentType := RetrieveParentType(ctx)
Expand Down