New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

.*: Introduce graceful shutdown for gRPC Servers #1687

Merged

bwplotka merged 11 commits into thanos-io:master from kakkoyun:grpc_grace_period

Nov 1, 2019

Member

kakkoyun commented Oct 25, 2019 •

edited

Loading

This PR,

introduces a grace period for components which use gRPC to wait (5s by default) for connections to drain before the shutdown.
adds a new --grpc-grace-period CLI option to specify the period.

Signed-off-by: Kemal Akkoyun [email protected]

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Add a new --grpc-grace-period CLI option to components which serve gRPC to set how long to wait until gRPC Server shuts down.
Components which use gRPC Server now wait (5s by default) for connections to drain before the shutdown.

Verification

make test-local
MINIO_ENABLED=1 ./scripts/quickstart.sh

Member Author

kakkoyun commented Oct 28, 2019

This should be rebased and include the fixes that will be introduced after #1689 merged.

kakkoyun force-pushed the grpc_grace_period branch from 3927f73 to 3c786f8 Compare

October 28, 2019 16:48

kakkoyun marked this pull request as ready for review

October 28, 2019 17:07

Member Author

kakkoyun commented Oct 28, 2019

cc @bwplotka @squat @brancz

I have tried to be extra careful, in order to prevent issues such as #1689, I'd be glad if you can double-check the work.

brancz approved these changes

View reviewed changes

Member

brancz left a comment

lgtm, but good to have another pair of eyes on this I think cc @squat @bwplotka

Member Author

kakkoyun commented Oct 30, 2019 •

edited

Loading

This PR might have conflicting changes with #1672

Member

brancz commented Oct 30, 2019

My personal opinion, this PR is larger and would be harder to rebase than #1672, so I'd prefer to merge this first.

squat reviewed

View reviewed changes

pkg/server/grpc/grpc.go Show resolved Hide resolved

squat reviewed

View reviewed changes

pkg/server/grpc/grpc.go Outdated Show resolved Hide resolved

squat reviewed

View reviewed changes

pkg/server/grpc/option.go Show resolved Hide resolved

squat reviewed

View reviewed changes

pkg/server/http/http.go Outdated Show resolved Hide resolved

squat reviewed

View reviewed changes

pkg/tls/options.go Outdated Show resolved Hide resolved

squat reviewed

View reviewed changes

pkg/tls/options.go Outdated Show resolved Hide resolved

squat reviewed

View reviewed changes

Member

squat left a comment

Great work, @kakkoyun :))) I have some nits but nothing major

squat reviewed

View reviewed changes

pkg/server/grpc/grpc.go Outdated Show resolved Hide resolved

squat reviewed

View reviewed changes

pkg/server/http/http.go Show resolved Hide resolved

squat reviewed

View reviewed changes

pkg/server/grpc/grpc.go Outdated Show resolved Hide resolved

bwplotka approved these changes

View reviewed changes

Member

bwplotka left a comment

Nice, it looks good, but have some minor comments (:

cmd/thanos/flags.go Outdated Show resolved Hide resolved

cmd/thanos/query.go Show resolved Hide resolved

cmd/thanos/rule.go Show resolved Hide resolved

cmd/thanos/sidecar.go Outdated Show resolved Hide resolved

cmd/thanos/sidecar.go Show resolved Hide resolved

pkg/server/grpc/grpc.go Show resolved Hide resolved

pkg/server/grpc/grpc.go Show resolved Hide resolved

pkg/server/grpc/option.go Show resolved Hide resolved

pkg/server/http/http.go Outdated Show resolved Hide resolved

kakkoyun changed the title ~~Introduce graceful shutdown for gRPC Servers~~ .*: Introduce graceful shutdown for gRPC Servers

bwplotka approved these changes

View reviewed changes

Member

bwplotka left a comment

Let's go with this! Good work, thanks 👍

Let's rebase and merge.

kakkoyun added 8 commits

October 31, 2019 19:42


          Inroduce graceful shutdown for gRPC

a543784

Signed-off-by: Kemal Akkoyun <[email protected]>


          Add missed cancel branch

162334a

Signed-off-by: Kemal Akkoyun <[email protected]>


          Remove stutter from server structs

edc6194

Signed-off-by: Kemal Akkoyun <[email protected]>


          Close servers immediately if grace period is not specified

fbf17e3

Signed-off-by: Kemal Akkoyun <[email protected]>


          Update CHANGELOG

66d2ec4

Signed-off-by: Kemal Akkoyun <[email protected]>


          Rename TLS methods, clarify log messages

f6395ff

Signed-off-by: Kemal Akkoyun <[email protected]>


          Document public functions

426ff70

Signed-off-by: Kemal Akkoyun <[email protected]>


          Fix review issues

e020f36

Signed-off-by: Kemal Akkoyun <[email protected]>

kakkoyun force-pushed the grpc_grace_period branch from d7609b4 to e020f36 Compare

November 1, 2019 10:16


          Update bucket docs

0db5de8

Signed-off-by: Kemal Akkoyun <[email protected]>

Member Author

kakkoyun commented Nov 1, 2019

@bwplotka Is CI failure a Netlify issue? Is it something related to these changes?

kakkoyun added 2 commits

November 1, 2019 13:51


          trigger checks

9a85e3c

Signed-off-by: Kemal Akkoyun <[email protected]>


          trigger checks

4bb46d9

Signed-off-by: Kemal Akkoyun <[email protected]>

bwplotka merged commit 9d4d0bf into thanos-io:master

gjtempleton mentioned this pull request

[prometheus-thanos] - Allow bucket web httpPort specification kiwigrid/helm-charts#287

Merged

4 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet