feat: support graceful shutdown on sigint/sigterm #385
Conversation
| time.Sleep(2 * time.Second) | ||
| syscall.Kill(syscall.Getpid(), syscall.SIGINT) | ||
| }() | ||
| if err := Run(ctx, "tcp", serviceConfig); err != nil { |
There was a problem hiding this comment.
Do you think it's feasible to test the status change from SERVING to NOT_SERVING? Looks like GracefulStop doesn't return until all RPC calls are served, so timing wise it's a bit tricky.
There was a problem hiding this comment.
I could, but I added the test to TestOpenSaves_HealthCheck
internal/app/server/server.go
Outdated
| } | ||
| healthcheck.SetServingStatus(serviceName, healthgrpc.HealthCheckResponse_NOT_SERVING) | ||
| s.GracefulStop() | ||
| log.Infoln("stopping open saves server gracefully") |
There was a problem hiding this comment.
This log output should be between L72-73.
Log ("stopping") -> change status -> gracefulstop -> Log ("Stopped")
| @@ -99,6 +112,31 @@ func TestOpenSaves_HealthCheck(t *testing.T) { | |||
| if want := healthgrpc.HealthCheckResponse_SERVING; got.Status != want { | |||
| t.Errorf("hc.Check got: %v, want: %v", got.Status, want) | |||
| } | |||
| cancel() | |||
There was a problem hiding this comment.
Why do we need to cancel the context? Do we want to test the behavior of the server after cancel()?
There was a problem hiding this comment.
I added a comment here to address this. I also added tests after this to test NOT_SERVING.
| } | ||
|
|
||
| func TestOpenSaves_RunServer(t *testing.T) { | ||
| serviceConfig, err := getServiceConfig() |
There was a problem hiding this comment.
I don't want to run these tests in parallel since they use the same port. The timeouts are short enough that there shouldn't be much delay here anyway. Same response for the remaining tests.
| } | ||
|
|
||
| func TestOpenSaves_HealthCheck(t *testing.T) { | ||
| serviceConfig, err := getServiceConfig() |
| } | ||
| ctx := context.Background() | ||
| t.Run("cancel_context", func(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(ctx, 2*time.Second) |
| } | ||
| }) | ||
| t.Run("sigint", func(t *testing.T) { | ||
| go func() { |
| select { | ||
| case s := <-sigs: | ||
| log.Infof("received signal: %v\n", s) | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
It's not clear when you need to cancel the context. Currently it's only in the tests, but do you have any plan to support cancellation?
There was a problem hiding this comment.
I'm supporting this here since we pass in a ctx to Run. There's no immediate plan to support this. If you prefer, I can remove it.
| @@ -99,6 +112,31 @@ func TestOpenSaves_HealthCheck(t *testing.T) { | |||
| if want := healthgrpc.HealthCheckResponse_SERVING; got.Status != want { | |||
| t.Errorf("hc.Check got: %v, want: %v", got.Status, want) | |||
| } | |||
| cancel() | |||
| } | ||
|
|
||
| func TestOpenSaves_RunServer(t *testing.T) { | ||
| serviceConfig, err := getServiceConfig() |
| select { | ||
| case s := <-sigs: | ||
| log.Infof("received signal: %v\n", s) | ||
| case <-ctx.Done(): |
ghost
mentioned this pull request
What type of PR is this?
/kind feat
What this PR does / Why we need it:
Allows grpc server to gracefully shutdown when encountering interruption/termination signals.
Which issue(s) this PR fixes:
Closes #379
Special notes for your reviewer: