Skip to content
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.

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

.*: Add new http-grace-period flag #1680

Merged
merged 5 commits into from
Oct 24, 2019
Merged

Conversation

kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Oct 24, 2019

This PR adds a new CLI flag to components which serve HTTP, called --http-grace-period.
Also refactors existing code and introduces a new HTTP server package, to increase the clarity of the run.Group scheduling.

cc @bwplotka @FUSAKLA

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

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

Changes

  • Adds new --http-grace-period CLI flag to components which serve HTTP.
  • Removes scheduleHTTPServer function and introduces a new package to handle HTTP serving.

Verification

  • make local-test
  • ./scripts/quickstart.sh

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! One comment and LGTM, maybe we could increase timeout a bit.

Wonder can we have similiar for gRPC 🤔 ? We would need something like grpc/grpc-go#2448

Also we used to have no timeout I guess? So adding 5s make sense. We were not draining even really.

We would need something like grpc/grpc-go#2448

pkg/server/http.go Outdated Show resolved Hide resolved
@kakkoyun
Copy link
Member Author

@bwplotka Yes, definitely. Great idea, I'll add a grace period and draining logic for gRPC as well in another PR.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, LGTM. 👍

Thanks!

@bwplotka
Copy link
Member

bwplotka commented Oct 24, 2019

you need to sign commits + CI failure

kakkoyun and others added 5 commits October 24, 2019 17:44
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Co-Authored-By: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@bwplotka bwplotka merged commit 9c2fe03 into thanos-io:master Oct 24, 2019
@kakkoyun kakkoyun deleted the grace_period branch October 24, 2019 22:13
vmg added a commit to vmg/thanos that referenced this pull request Oct 28, 2019
PR thanos-io#1680 introduced graceful handling for the HTTP server in Thanos, but
the graceful `Shutdown` call was being performed on an `http.Server`
instance that was *not* running at all. The actual server that was
listening for requests was started through `http.Serve`, so there was no
reference to the server struct that we could use to shut it down. This
was causing all of Thanos to freeze after receiving an exit signal,
because the run-group for the HTTP server would never finalize.

This seems like an oversight because the `(*Server).srv` field was being
properly initialized with an HTTP server. Fix this by calling
`ListenAndServe` on our initialized server.

Signed-off-by: Vicent Marti <vmg@strn.cat>
@vmg vmg mentioned this pull request Oct 28, 2019
2 tasks
bwplotka pushed a commit that referenced this pull request Oct 28, 2019
PR #1680 introduced graceful handling for the HTTP server in Thanos, but
the graceful `Shutdown` call was being performed on an `http.Server`
instance that was *not* running at all. The actual server that was
listening for requests was started through `http.Serve`, so there was no
reference to the server struct that we could use to shut it down. This
was causing all of Thanos to freeze after receiving an exit signal,
because the run-group for the HTTP server would never finalize.

This seems like an oversight because the `(*Server).srv` field was being
properly initialized with an HTTP server. Fix this by calling
`ListenAndServe` on our initialized server.

Signed-off-by: Vicent Marti <vmg@strn.cat>
GiedriusS pushed a commit that referenced this pull request Oct 28, 2019
* Add new http-grace-period flag

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Update CHANGELOG

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Update docs

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Update pkg/server/http.go

Co-Authored-By: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Rename initializer for HTTP server

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
GiedriusS pushed a commit that referenced this pull request Oct 28, 2019
PR #1680 introduced graceful handling for the HTTP server in Thanos, but
the graceful `Shutdown` call was being performed on an `http.Server`
instance that was *not* running at all. The actual server that was
listening for requests was started through `http.Serve`, so there was no
reference to the server struct that we could use to shut it down. This
was causing all of Thanos to freeze after receiving an exit signal,
because the run-group for the HTTP server would never finalize.

This seems like an oversight because the `(*Server).srv` field was being
properly initialized with an HTTP server. Fix this by calling
`ListenAndServe` on our initialized server.

Signed-off-by: Vicent Marti <vmg@strn.cat>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
@FUSAKLA
Copy link
Member

FUSAKLA commented Oct 29, 2019

This is great! I really like the new package and grace period is definitely good thing to have 👍
Sorry for the delay, I was short of time lately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants