feat(controller): add stack dump for better debugging

[loafoe]

Encountered a nil error in controller in current version. Using tilt up does not immediately show the location of the issue, so added a stack dump using runtime/debug Go package to make this visible.

[netlify]

✅ Deploy Preview for docs-kargo-akuity-io ready!

Name	Link
🔨 Latest commit	39cebf7
🔍 Latest deploy log	https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/66a945bc32c111000882ea1a
😎 Deploy Preview	https://deploy-preview-2360.kargo.akuity.io
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[hiddeco]

I wonder if it would not be better to configure this by globally enabling recovery from panics (see: https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.18.4/pkg/controller#Options -> RecoverPanic), but I have to admit that I lack historical insight here (cc: @krancour).

[codecov]

Codecov Report

Attention: Patch coverage is 5.00000% with 19 lines in your changes missing coverage. Please review.

Project coverage is 47.58%. Comparing base (aff97d8) to head (39cebf7).
Report is 55 commits behind head on main.

Files	Patch %	Lines
cmd/controlplane/controller.go	0.00%	6 Missing ⚠️
cmd/controlplane/api.go	0.00%	3 Missing ⚠️
cmd/controlplane/garbage_collector.go	0.00%	3 Missing ⚠️
cmd/controlplane/management_controller.go	0.00%	3 Missing ⚠️
cmd/controlplane/webhooks.go	0.00%	3 Missing ⚠️
internal/controller/promotions/promotions.go	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2360      +/-   ##
==========================================
- Coverage   47.64%   47.58%   -0.07%     
==========================================
  Files         244      244              
  Lines       17392    17414      +22     
==========================================
  Hits         8287     8287              
- Misses       8694     8716      +22     
  Partials      411      411              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[krancour]

@hiddeco I would prefer that as well. I'm not sure why the current panic recovery is limited to this one spot.

[hiddeco]

@loafoe do you feel up for making this suggested change?

It should be introduced at these points: https://github.com/search?q=repo%3Aakuity%2Fkargo%20NewManager&type=code. Where the ctrl.Options can take a Controller struct, which should have something along the lines of:

config.Controller{
	RecoverPanic: ptr.To(true),
}

as a value.

[loafoe]

Sure, I'll have a look

[loafoe]

@hiddeco @krancour added the controller Config across. However, the panic test function now fails as thepanic is not recovered. I lack insights into exactly what is happening. Also, no idea where to add the stack dump code, as that is the original intent of this change. Any further pointers appreciated, thx.

[hiddeco]

@loafoe the test fails because the wrapping of the panic now happens at the point where Reconcile is called, and is thus no longer captured by the test.

However, for this "special" use case, we shell out to a series of other binaries as part of the promotion mechanics. Given this, it would be OK to keep the panic logic in place to surface the error (and be able to persist it to the Kubernetes API).

With the new addition, we can now guarantee the controller will not end up in a crash loop if we make a mistake anywhere else.

[loafoe]

However, for this "special" use case, we shell out to a series of other binaries as part of the promotion mechanics. Given this, it would be OK to keep the panic logic in place to surface the error (and be able to persist it to the Kubernetes API).

Okay, I'll bring the wrapper code back and do some linting, thanks

[loafoe]

@hiddeco fixed linter issues and all unit tests pass again

[hiddeco]

Thanks for following up with the requested changes! 🙇

[hiddeco]

With the upgrade to controller-runtime v0.19.0 (via #2431), this has now become obsolete as it has been enabled by default upstream.

I want to thank you nonetheless for the work you put in @loafoe. 🙇