-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,9 @@ type Controller struct { | |
|
||
// Log is used to log messages to users during reconciliation, or for example when a watch is started. | ||
Log logr.Logger | ||
|
||
// RecoverPanic indicates whether the panic caused by reconcile should be recovered. | ||
RecoverPanic bool | ||
} | ||
|
||
// watchDescription contains all the information necessary to start a watch. | ||
|
@@ -95,7 +98,17 @@ 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) { | ||
if c.RecoverPanic { | ||
defer func() { | ||
if r := recover(); r != nil { | ||
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe the |
||
} | ||
}() | ||
} | ||
log := c.Log.WithValues("name", req.Name, "namespace", req.Namespace) | ||
ctx = logf.IntoContext(ctx, log) | ||
return c.Do.Reconcile(ctx, req) | ||
|
@@ -295,7 +308,7 @@ func (c *Controller) reconcileHandler(ctx context.Context, obj interface{}) { | |
|
||
// RunInformersAndControllers the syncHandler, passing it the Namespace/Name string of the | ||
// resource to be synced. | ||
result, err := c.Do.Reconcile(ctx, req) | ||
result, err := c.Reconcile(ctx, req) | ||
switch { | ||
case err != nil: | ||
c.Queue.AddRateLimited(req) | ||
|
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:
Controller
to make this configurable (i.E.RecoverPanic bool
)if c.RecoverPanic { defer recover() }
)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.