search

siderolabs / omni

SaaS-simple deployment of Kubernetes - on your own hardware.
Other
395 stars 24 forks source link

feat: handle all goroutine panics gracefully #389

Closed utkuozdemir closed 1 week ago

utkuozdemir commented 1 week ago

Convert goroutine panics to errors or error logs.

Disallow usage of golang.org/x/sync/errgroup package in the backend by depguard linter. This linter configuration depends on: https://github.com/siderolabs/kres/pull/417

Rekres the project to include the feature (also bump Go to 1.22.4), but revert PROTOBUF_GO_VERSION and GRPC_GATEWAY_VERSION manually to not break the frontend.

Disallowing the named go statement was not possible at the moment using existing linters, raised an issue in forbidigo for it: https://github.com/ashanbrown/forbidigo/issues/47

Closes https://github.com/siderolabs/omni/issues/373.

DmitriyMV commented 1 week ago

Hmm, I think we can do better:

  1. Create our own errgroup.Group implementation which does what we want.
  2. Forbid the usage of errgroup using https://golangci-lint.run/usage/linters/#depguard
  3. Forbid the usage of go statement using https://golangci-lint.run/usage/linters/#forbidigo
utkuozdemir commented 1 week ago

Hmm, I think we can do better:

  1. Create our own errgroup.Group implementation which does what we want.
  2. Forbid the usage of errgroup using golangci-lint.run/usage/linters/#depguard
  3. Forbid the usage of go statement using golangci-lint.run/usage/linters/#forbidigo

ah this makes perfect sense, thanks.

utkuozdemir commented 1 week ago

Hmm, I think we can do better:

  1. Create our own errgroup.Group implementation which does what we want.
  2. Forbid the usage of errgroup using golangci-lint.run/usage/linters/#depguard
  3. Forbid the usage of go statement using golangci-lint.run/usage/linters/#forbidigo

done something along the lines of your suggestion and our discussion.

Unix4ever commented 1 week ago

Rekres the project to include the feature (also bump Go to 1.22.4), but revert PROTOBUF_GO_VERSION and GRPC_GATEWAY_VERSION manually to not break the frontend.

Does it still break the frontend, I thought it was fixed?

utkuozdemir commented 1 week ago

Rekres the project to include the feature (also bump Go to 1.22.4), but revert PROTOBUF_GO_VERSION and GRPC_GATEWAY_VERSION manually to not break the frontend.

Does it still break the frontend, I thought it was fixed?

Actually, everything seemed to be working when I ran locally with newer versions and generated sources, but I saw some 500s for the watch requests in the browser developer console, so to be on the safe side I reverted manually anyway. So, not 100% sure. @DmitriyMV?

utkuozdemir commented 1 week ago

I think we should proceed and look into those 500s if they become a problem.

Ok, but I'll still merge this PR as-is, and make rekres a separate PR to not block this with any potential issues.

utkuozdemir commented 1 week ago

/m