RecoverPanic - true by default

[RealHarshThakur]

Just curious why this isn't set to true by default. Would anyone object if I made a PR to enable by default? https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/config/controller.go#L44

[vincepri]

Not sure of the original reasoning, it might make sense to have it always on by default; at the same time, I can see that folks might prefer that in case of a panic in the reconciler code, the whole application should shutdown.

@kubernetes-sigs/controller-runtime-maintainers thoughts?

[sbueringer]

I personally like if the controller is not killed through panics.

I think that's a nice behavior that e.g. the Go http server also has if a handler panics (IIRC).

It makes it harder to find panics, but with proper logging etc it shouldn't be too hard (we could also expose an additional metric if we want to).

I think with recover panic per default controllers become more robust, because a panic which might only occur when reconciling one special object doesn't influence reconciliations of other objects.

[vincepri]

To add onto that, would it make sense to remove the option, and always recover the panic?

[RealHarshThakur]

I'm also looking at how to set up alerts for these. Seems like something that should be log-based due to high cardinality but also, this is currently incrementing the errors metric(we don't alert on this as we expect the error to be solved eventually in most cases and have alerts sets on particular status conditions which we care about). How do you all feel about having a dedicated metric for panics?

Update: I see you've mentioned it already @sbueringer . My bad for skimming :p

[sbueringer]

To add onto that, would it make sense to remove the option, and always recover the panic?

I think that would be reasonable, but I don't know if others want to panic.

How do you all feel about having a dedicated metric for panics?
Update: I see you've mentioned it already @sbueringer . My bad for skimming :p

No problem. Sounds like a good idea to me.

Seems like something that should be log-based due to high cardinality but also

When we introduce a dedicated panic metric we can choose the cardinality. I guess a single metric for the entire manager could be enough, maybe we want a label for the controller or something. But I think in general an alert for panics > 0 is already a win and it might not be necessary to distinguish which controller, for which reconciled object, ... paniced

I would also hope that normal controllers don't panic that often that it has a chance to become a high cardinality metric :)

[RealHarshThakur]

I think that would be reasonable, but I don't know if others want to panic.

I am having this conversation with a couple of people, they were hesitant at first but after bringing up "What do you expect your web framework to do when a panic occurs?", they agreed that it should be the default. Although, I can imagine OSS project maintainers may prefer crash looping as they wouldn't expect their users to set up alerts for it and their alert mechanism is Github issues, essentially :p .

[vincepri]

Let's default it to true, in the next version, and keep the option; but also add a metric to track reconciler panics

[RealHarshThakur]

I would also hope that normal controllers don't panic that often that it has a chance to become a high cardinality metric :)

I think if the metric has the key-value pairs as the logs do, we'll be good i.e GVK , CR name and namespace and the panic error( ex : " index out of range [0] with length 0 "). Nothing about the stack trace should be in the metric labels I think. People can grab the panic error from the alert and dig through logs.

[sbueringer]

Not sure if we should include the panic string. That definitely sounds a bit strange for a metric.

[alvaroaleman]

Yeah no, error messages which is essentially what the panic is do not belong into metrics. Metrics are supposed to tell that there is a problem and how frequently it occurs, they should not this level of detail, that is what logs are for.

[alvaroaleman]

I'd honestly just add the controller name to the metric. GVK is then implicit (also: Not all controllers are specific to one GVK) and ppl should fetch all the other info from the logs, just like they do for any other error.

[RealHarshThakur]

The below SS is from Honeybadger, they manage stacktraces as a service and mention it on the list page of panics and when we click through, they let us view the stack trace. I'm thinking of metrics as the list page here and the show page as the logs. Service monitors can be configured to not index it if people don't want it. Maybe we can at least have runtime.errorString bit as a label?

[alvaroaleman]

Maybe we can at least have runtime.errorString bit as a label?

No, that it was a panic is implicit by the fact that this is a metric about panics. Any further detail except the reconciler name doesn't belong into metrics. We had explicit changes in the past to remove metrics with too high cardinality and adding more details would put this into that category.

If you really want all the details in a metric, you can recover the panic yourself and create a metric out of this.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale