[flyteadmin] Refactor panic recovery into middleware

[Sovietaced]

What changes were proposed in this pull request?

Previously all gRPC handlers would handle panics inside each RPC handler. This added a lot of repetitive boilerplate to all RPC handlers that was pretty fragile to maintain. This pull request introduces recovery middleware that will recover from panics for all RPCs mounted to the RPC server.

This pull request also proposes a change to the panic recovery logic.

I made a change to the recovery logic such that it logs the panic at the error level instead of the fatal level. The previous fatal error level would call os.Exit(1) which immediately terminates the program ungracefully. My suspicion is that this made the existing prometheus panic metrics effectively useless given that prometheus metrics are polled on an interval and the server was likely killed when the metrics would normally be polled. (Arguably the panic metrics could be removed now).

IMO, it's better for high availability to have an RPC server that is alive and sending errors back (and reporting error metrics) than one that gets killed and is unresponsive until Kubernetes decides to boot up another healthy pod. As such, I have changed the behavior to return gRPC INTERNAL status codes instead of terminating the server. I'm open to debating this change so feel free to share your opinion.

How was this patch tested?

Unit tests

Setup process

Screenshots

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

Related PRs

Docs link

[Sovietaced]

I'm open to changing where this lives but it feels like there should be a middleware package, and any interceptors should move here imo.

[katrogan]

Sounds good!

[codecov]

Codecov Report

Attention: Patch coverage is 54.05405% with 17 lines in your changes missing coverage. Please review.

Project coverage is 36.17%. Comparing base (025296a) to head (ec7ba89).
Report is 129 commits behind head on master.

Files with missing lines	Patch %	Lines
flyteadmin/pkg/server/service.go	0.00%	17 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5546      +/-   ##
==========================================
+ Coverage   35.89%   36.17%   +0.27%     
==========================================
  Files        1301     1302       +1     
  Lines      109419   109388      -31     
==========================================
+ Hits        39281    39570     +289     
+ Misses      66041    65683     -358     
- Partials     4097     4135      +38     

Flag	Coverage Δ
unittests-datacatalog	51.37% <ø> (ø)
unittests-flyteadmin	55.30% <54.05%> (+1.60%)	⬆️
unittests-flytecopilot	12.17% <ø> (ø)
unittests-flytectl	62.28% <ø> (ø)
unittests-flyteidl	7.09% <ø> (ø)
unittests-flyteplugins	53.31% <ø> (ø)
unittests-flytepropeller	41.75% <ø> (ø)
unittests-flytestdlib	55.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[katrogan]

looks good, thank you for refactoring and the detailed PR explanation!

[katrogan]

Sounds good!

[katrogan]

cc @eapolinario who is looking into test failures