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

Graceful shutdown #1799

Merged
merged 1 commit into from Mar 12, 2019

Conversation

2 participants
@pjdufour-truss
Copy link
Contributor

pjdufour-truss commented Feb 27, 2019

Description

This PR adds support for a graceful shutdown of the server. This has 2 benefits.

First, this PR will decrease our risk of data corruption during deployments. The server will now wait for all client connections to close and will then immediately shut down. Currently the server waits 30 seconds and then quits regardless if there are any open connections. This assumes any database transactions are complete within the HTTP request/response cycle.

Second, this PR will speed up our deployments since each container won't wait 30 seconds to begin shutdown, but will start to shutdown ASAP (assuming no open connections).

For context see:

https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_StopTask.html

Reviewer Notes

TBD: I'll add some notes on how to test thoroughly.

Setup

None

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • The requirements listed in
    Querying the Database Safely
    have been satisfied.
  • Any new migrations/schema changes:
    • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
    • Have been communicated to #dp3-engineering
    • Secure migrations have been tested using bin/run-prod-migrations
  • There are no aXe warnings for UI.
  • This works in IE.
  • Any new client dependencies (Google Analytics, hosted libraries, CDNs, etc) have been:
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • User facing changes have been reviewed by design.
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

Screenshots

None

@pjdufour-truss pjdufour-truss changed the title Graceful shutdown [WIP] Graceful shutdown Mar 4, 2019

@pjdufour-truss pjdufour-truss changed the title [WIP] Graceful shutdown [HOLD] Graceful shutdown Mar 4, 2019

@pjdufour-truss pjdufour-truss referenced this pull request Mar 4, 2019

Merged

Refactor Server #1809

0 of 13 tasks complete

@pjdufour-truss pjdufour-truss force-pushed the graceful_shutdown branch from 5eb4b65 to 2f7bc8c Mar 6, 2019

@pjdufour-truss pjdufour-truss changed the title [HOLD] Graceful shutdown Graceful shutdown Mar 6, 2019

@pjdufour-truss pjdufour-truss force-pushed the graceful_shutdown branch from 2f7bc8c to 7bd4776 Mar 6, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #1799 into master will decrease coverage by 0.13%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master    #1799      +/-   ##
==========================================
- Coverage   49.54%   49.41%   -0.14%     
==========================================
  Files         429      427       -2     
  Lines       18450    18338     -112     
  Branches     1636     1631       -5     
==========================================
- Hits         9141     9061      -80     
+ Misses       8507     8480      -27     
+ Partials      802      797       -5

@pjdufour-truss pjdufour-truss force-pushed the graceful_shutdown branch from 7bd4776 to 78301ce Mar 6, 2019

@pjdufour-truss pjdufour-truss requested a review from jim Mar 6, 2019

@@ -171,6 +175,7 @@ func initFlags(flag *pflag.FlagSet) {
flag.String("env", "development", "The environment to run in, which configures the database.")
flag.String("interface", "", "The interface spec to listen for connections on. Default is all.")
flag.String("service-name", "app", "The service name identifies the application for instrumentation.")
flag.Duration("graceful-shutdown-timeout", 25*time.Second, "The duration for which the server gracefully wait for existing connections to finish")

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 6, 2019

Contributor

We should add a comment that ECS only gives us 30 seconds, so any number less than 30 should be used.

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Mar 6, 2019

Author Contributor

Added to usage


shutdownError := false
shutdownErrors.Range(func(key, value interface{}) bool {
srv := key.(*server.NamedServer)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 6, 2019

Contributor

Should we catch errors here just to be safe? srv, ok := key.(*server.NamedServer)?

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Mar 6, 2019

Author Contributor

Sure, why not. :)


signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM)

sig := <-quit

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 6, 2019

Contributor

I'm still pretty new to channels. Does this essentially stop the code here and wait for the output from the quit channel?

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Mar 6, 2019

Author Contributor

Added comments to code.

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

🌟 - Nice job! I think I learned a couple new things reading this PR. Don't forget to remove your changes to circleci but I think this is ready. You can feel free to address my one comment or not, its up to your discretion.

@pjdufour-truss pjdufour-truss force-pushed the graceful_shutdown branch 4 times, most recently from 9bf34f3 to a2078e5 Mar 11, 2019

@pjdufour-truss pjdufour-truss force-pushed the graceful_shutdown branch from a2078e5 to 9d54267 Mar 11, 2019

@pjdufour-truss pjdufour-truss merged commit c523ecf into master Mar 12, 2019

19 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: client_test_coverage Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch 60% of diff hit (target 49.54%)
Details
codecov/project Absolute coverage decreased by -0.13% but relative coverage increased by +10.45% compared to 6d6421f
Details

@pjdufour-truss pjdufour-truss deleted the graceful_shutdown branch Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.