Reconciler panics should not crash the manager

[ekuefler]

Currently, an unhandled panic in a reconciler will not be recovered from, and will likely cause the manager binary to crash. This is a problem, since a panic might be triggered by a single resource in an unexpected state, so that one bad resource could prevent all other resources from being processed. Since Kubernetes is likely to restart the manager pod after a crash, this can also cause the manager to DOS the Kubernetes API server as it continually restarts.

In my project, I wrote this utility function:

func MakeSafe(r reconcile.Reconciler) reconcile.Reconciler {
	return safeReconciler{impl: r}
}

type safeReconciler struct {
	impl reconcile.Reconciler
}

func (r safeReconciler) Reconcile(request reconcile.Request) (result reconcile.Result, err error) {
	defer func() {
		if r := recover(); r != nil {
			result = reconcile.Result{}
			err = fmt.Errorf("panic: %v [recovered]\n\n%s", r, debug.Stack())
		}
	}()
	return r.impl.Reconcile(request)
}

Every time I pass a reconciler to Complete, I wrap it with this. It ensures that any panics raised by the reconciler are converted to normal errors.

[alvaroaleman]

Yeah, this would be great to have :)

[rajathagasthya]

Something like apimachinery's HandleCrash function would also work if we plug it in the right place: https://github.com/kubernetes/apimachinery/blob/3253b0a30d67e7e362b8615e463156bac729c82f/pkg/util/runtime/runtime.go#L45

[vincepri]

We can revisit the milestone if the design doesn't have breaking changes. Folks might be relying on panics to detect failures today.

/priority important-soon
/help
/kind design

[k8s-ci-robot]

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

We can revisit the milestone if the design doesn't have breaking changes. Folks might be relying on panics to detect failures today.

/priority important-soon
/help
/kind design

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[vincepri]

/lifecycle frozen

[varshaprasad96]

/assign

[FillZpp]

@varshaprasad96 Hi, are you still pursuing this? Mind if I take a stab at it :) ?

[varshaprasad96]

@FillZpp sure, please feel free to create a PR for this.

[FillZpp]

/assign @FillZpp