[v2] Graceful shutdown #659
Conversation
blampe
added
the
impact/no-changelog-required
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #659 +/- ##
===========================================
+ Coverage 33.11% 47.27% +14.15%
===========================================
Files 27 26 -1
Lines 4161 2807 -1354
===========================================
- Hits 1378 1327 -51
+ Misses 2610 1316 -1294
+ Partials 173 164 -9 ☔ View full report in Codecov by Sentry. |
…o blampe/v2-graceful
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Curious, something happened?
There was a problem hiding this comment.
No, but we shouldn't ignore the error even if we don't expect it.
| _workDir string | ||
| _skipInstall bool | ||
| _stack string | ||
| _host string | ||
| _port int |
There was a problem hiding this comment.
What is the "underscore" convention?
There was a problem hiding this comment.
https://github.com/uber-go/guide/blob/master/style.md#prefix-unexported-globals-with-_
Makes global symbols (and their mutation) more obvious to readers.
| log.Infow("starting the RPC server", "address", address) | ||
| serverOpts := []grpc_zap.Option{ |
There was a problem hiding this comment.
I felt better with this setup code being in serve.go rather than the server package, because a) the setup code tends to be parameterized by flags, b) there might be other RPC services to be mixed into the server, and c) code that watches signals and calls os.Exit should be in the cmd package. Moving the code has made this PR harder to understand.
There was a problem hiding this comment.
Totally right about os.Exit belonging in cmd -- I moved that back.
In general I treat cmd as the place for executables (package main) and try to limit their scope to flag parsing and terminal output. With cobra especially it helps to keep Run functions as small wrappers around other library calls -- for example in this case I need to extract the gRPC setup into something a test can re-use. The server can still be customized with minimal changes to NewGRPC.
…o blampe/v2-graceful
This implements graceful shutdown by propagating interrupts to our child processes.
Killing a workspace pod in the middle of an update will mark the Update as failed ("update canceled" error), and subsequent Updates will resume where it left off as you would expect.
Fixes #607.