✨Add an option to recover panic for controller reconcile #1627

FillZpp · 2021-08-09T05:50:25Z

Signed-off-by: FillZpp FillZpp.pub@gmail.com

Add an option to recover panic for controller reconcile. It ensures that any panics raised by controller reconciler are converted to normal errors.

fixes #797

k8s-ci-robot · 2021-08-09T05:50:33Z

Hi @FillZpp. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

FillZpp · 2021-08-09T06:44:41Z

/cc @alvaroaleman @joelanford

varshaprasad96 · 2021-08-09T06:48:25Z

/ok-to-test

alvaroaleman · 2021-08-15T14:25:02Z

pkg/internal/controller/controller.go

@@ -321,6 +322,15 @@ func (c *Controller) reconcileHandler(ctx context.Context, obj interface{}) {
 	}
 }

+func (c *Controller) doReconcile(ctx context.Context, req reconcile.Request) (_ reconcile.Result, err error) {
+	defer func() {


This should be optional and opt-in, similiar to what upstream does: https://github.com/kubernetes/apimachinery/blob/f916759cb6b8547418dc7708876ecab5c1961448/pkg/util/runtime/runtime.go#L29

Also, please add tests for this.

Okay. I have added tests and changed all recover() to utilruntime.HandleCrash(), so that developers can set it via utilruntime.ReallyCrash.

alvaroaleman · 2021-08-16T14:22:22Z

pkg/internal/controller/controller.go

@@ -95,7 +95,10 @@ type watchDescription struct {
 }

 // Reconcile implements reconcile.Reconciler.
-func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) {
+func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (_ reconcile.Result, err error) {


Sorry, what I meant by the other comment was:

Please add an option to Controller to make this configurable (i.E. RecoverPanic bool)

Make the handling here conditional (i.E. if c.RecoverPanic { defer recover() })

Expose the option in pkg/controller.Options

The reason for favoring that over the current approach is that with the current approach we require ppl to read internal packages to understand how they can get panics recovered, which is not good UX.

Sorry for being unclear about this before.

Understand, I will fix it. So what do you think of webhook handler? Should its panic default to be recovered or another option field to configure it?

It should also have an option to set this up and by default panic

Updated. It is hard to add the option for webhook. Maybe we can firstly add an option for controller reconcile.

alvaroaleman · 2021-08-18T15:19:04Z

pkg/internal/controller/controller.go

+				for _, fn := range utilruntime.PanicHandlers {
+					fn(r)
+				}
+				err = fmt.Errorf("panic: %v [recovered]", r)


Could we add the stack here, otherwise this is almost impossible to debug? From runtime/debug.Stack()

Maybe the utilruntime.PanicHandlers above can print the stack of panic more properly
https://github.com/kubernetes/apimachinery/blob/f916759cb6b8547418dc7708876ecab5c1961448/pkg/util/runtime/runtime.go#L61

alvaroaleman · 2021-08-18T15:19:49Z

pkg/internal/controller/controller_test.go

+			})
+			_, _ = ctrl.Reconcile(ctx,
+				reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "foo", Name: "bar"}})
+			By("this should not be called")


Nit: Is this needed? The defer should make the test fail if it doesn't work or not?

Have removed this.

Signed-off-by: FillZpp <FillZpp.pub@gmail.com>

alvaroaleman

Thank you!

k8s-ci-robot · 2021-08-19T13:21:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, FillZpp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…s-sigs#1627) Signed-off-by: FillZpp <FillZpp.pub@gmail.com>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 9, 2021

k8s-ci-robot requested review from varshaprasad96 and vincepri August 9, 2021 05:50

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 9, 2021

k8s-ci-robot requested review from alvaroaleman and joelanford August 9, 2021 06:44

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 9, 2021

alvaroaleman reviewed Aug 15, 2021

View reviewed changes

FillZpp force-pushed the handle-crash-for-controller-and-webhook branch from 3cffa52 to 76cdea9 Compare August 16, 2021 06:29

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 16, 2021

alvaroaleman reviewed Aug 16, 2021

View reviewed changes

FillZpp force-pushed the handle-crash-for-controller-and-webhook branch from 76cdea9 to 8e0707e Compare August 18, 2021 02:51

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 18, 2021

FillZpp changed the title ~~✨Recover panic for controller reconciler and webhook handler~~ ✨Add an option to recover panic for controller reconcile Aug 18, 2021

alvaroaleman reviewed Aug 18, 2021

View reviewed changes

Recover panic for controller reconcile and webhook handler

9a2a972

Signed-off-by: FillZpp <FillZpp.pub@gmail.com>

FillZpp force-pushed the handle-crash-for-controller-and-webhook branch from 8e0707e to 9a2a972 Compare August 19, 2021 02:47

alvaroaleman approved these changes Aug 19, 2021

View reviewed changes

k8s-ci-robot assigned alvaroaleman Aug 19, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 19, 2021

k8s-ci-robot merged commit 5de246b into kubernetes-sigs:master Aug 19, 2021

k8s-ci-robot added this to the v0.10.x milestone Aug 19, 2021

timebertt mentioned this pull request Oct 12, 2021

Upgrade k8s.io/* to v0.22, sigs.k8s.io/controller-runtime to v0.10 gardener/gardener#4772

Closed

6 tasks

FillZpp mentioned this pull request Nov 16, 2021

REQUEST: New membership for FillZpp kubernetes/org#3100

Closed

7 tasks

FillZpp mentioned this pull request Dec 22, 2021

🌱 Add FillZpp as a reviewer #1753

Merged

tamalsaha pushed a commit to kmodules/controller-runtime that referenced this pull request Feb 12, 2022

Recover panic for controller reconcile and webhook handler (kubernete…

811f7a3

…s-sigs#1627) Signed-off-by: FillZpp <FillZpp.pub@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨Add an option to recover panic for controller reconcile #1627

✨Add an option to recover panic for controller reconcile #1627

FillZpp commented Aug 9, 2021 •

edited

Loading

k8s-ci-robot commented Aug 9, 2021

FillZpp commented Aug 9, 2021

varshaprasad96 commented Aug 9, 2021

alvaroaleman Aug 15, 2021

FillZpp Aug 16, 2021 •

edited

Loading

alvaroaleman Aug 16, 2021

FillZpp Aug 17, 2021

alvaroaleman Aug 17, 2021

FillZpp Aug 18, 2021

alvaroaleman Aug 18, 2021

FillZpp Aug 19, 2021

alvaroaleman Aug 18, 2021

FillZpp Aug 19, 2021

alvaroaleman left a comment

k8s-ci-robot commented Aug 19, 2021

✨Add an option to recover panic for controller reconcile #1627

✨Add an option to recover panic for controller reconcile #1627

Conversation

FillZpp commented Aug 9, 2021 • edited Loading

k8s-ci-robot commented Aug 9, 2021

FillZpp commented Aug 9, 2021

varshaprasad96 commented Aug 9, 2021

Choose a reason for hiding this comment

FillZpp Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 19, 2021

FillZpp commented Aug 9, 2021 •

edited

Loading

FillZpp Aug 16, 2021 •

edited

Loading