-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨Add an option to recover panic for controller reconcile #1627
✨Add an option to recover panic for controller reconcile #1627
Conversation
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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
@@ -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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I have added tests and changed all recover()
to utilruntime.HandleCrash()
, so that developers can set it via utilruntime.ReallyCrash
.
3cffa52
to
76cdea9
Compare
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should also have an option to set this up and by default panic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. It is hard to add the option for webhook. Maybe we can firstly add an option for controller reconcile.
76cdea9
to
8e0707e
Compare
for _, fn := range utilruntime.PanicHandlers { | ||
fn(r) | ||
} | ||
err = fmt.Errorf("panic: %v [recovered]", r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add the stack here, otherwise this is almost impossible to debug? From runtime/debug.Stack()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}) | ||
_, _ = ctrl.Reconcile(ctx, | ||
reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "foo", Name: "bar"}}) | ||
By("this should not be called") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Is this needed? The defer
should make the test fail if it doesn't work or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have removed this.
Signed-off-by: FillZpp <FillZpp.pub@gmail.com>
8e0707e
to
9a2a972
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
[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:
Approvers can indicate their approval by writing |
…s-sigs#1627) Signed-off-by: FillZpp <FillZpp.pub@gmail.com>
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