proxy-lifecycle: add HTTP Server with endpoints for proxy lifecycle shutdown #115

mikemorris · 2023-05-15T19:13:50Z

Refs hashicorp/consul-k8s#536, hashicorp/consul-k8s#650

Adds a proxy lifecycle management server and starts it from the consul-dataplane main process. This server exposes an HTTP endpoint (configurable, defaulting to /graceful_shutdown on port 20300) to optionally start draining inbound (external) connections to the managed Envoy proxy, while allowing outbound requests from the application for which this proxy is acting as a sidecar to continue, up to a configurable grace period timeout, to facilitate application shutdown.

Refactors the envoy package proxy manager to introduce new states (stateDraining and stateExited) and new methods (Drain(), Quit() and renaming Stop() to Kill() to avoid confusion and better describe the actual implementation) and implement an interface to allow a mock implementation for testing the lifecycle management server in isolation.

Notes for reviewers

This behavior can currently only be triggered by calling the /graceful_shutdown endpoint explicitly (which will be necessary for handling job termination from a preStop hook) in the future.
- proxy-lifecycle: catch SIGTERM and initiate graceful shutdown #130 will catch SIGTERM to handle shutdown gracefully if configured, and will include an addition to the integration test.
The additional functionality added to the envoy package does not have proper test coverage yet - I'm hoping to add this as a followup by replacing the current fake-envoy implementation with a Go-based version to include an HTTP server mocking the /drain_listeners and /quitquitquit Envoy admin API endpoints.
Should bootstrap tests be added for the Envoy drain time and strategy passthrough configuration?
envoyExtraArgs refactor will likely conflict with fix: log level present in extra args using envoy-extra-args annotation : NET-2190 #133 and need to be resolved

Links

mikemorris · 2023-05-30T16:06:00Z

pkg/consuldp/consul_dataplane.go

 		case <-cdp.xdsServerExited():
-			if err := proxy.Stop(); err != nil {
+			// Initiate graceful shutdown of Envoy, kill if error
+			if err := proxy.Quit(); err != nil {
 				cdp.logger.Error("failed to stop proxy", "error", err)
+				if err := proxy.Kill(); err != nil {
+					cdp.logger.Error("failed to kill proxy", "error", err)
+				}
 			}
 			doneCh <- errors.New("xDS server exited unexpectedly")
 		case <-cdp.metricsConfig.metricsServerExited():
 			doneCh <- errors.New("metrics server exited unexpectedly")
+		case <-cdp.lifecycleConfig.lifecycleServerExited():
+			// Initiate graceful shutdown of Envoy, kill if error
+			if err := proxy.Quit(); err != nil {
+				cdp.logger.Error("failed to stop proxy", "error", err)
+				if err := proxy.Kill(); err != nil {
+					cdp.logger.Error("failed to kill proxy", "error", err)
+				}
+			}
+			doneCh <- errors.New("proxy lifecycle maangement server exited unexpectedly")


This should be the only change to existing logic aside from introducing the new proxy lifecycle management server exposing the /shutdown endpoint - attempting to cleanly exit the Envoy process if a subprocess exits unexpectedly instead of the prior hard kill.

cmd/consul-dataplane/main.go

mikemorris · 2023-05-30T18:33:50Z

pkg/consuldp/lifecycle_test.go

+			url := fmt.Sprintf("http://127.0.0.1:%d%s", port, m.gracefulShutdownPath)
+			log.Printf("sending request to %s\n", url)
+
+			resp, err := http.Get(url)


Could possibly add some way to check that the lifecycle management server blocks for c.shutdownGracePeriod seconds here before calling Proxy.Quit(), but not sure how to best implement that in a non-flaky way.

nathancoleman · 2023-05-31T15:15:09Z

.github/workflows/consul-dataplane-checks.yaml

@@ -32,7 +32,7 @@ jobs:
      - uses: actions/setup-go@4d34df0c2316fe8122ab82dc22947d607c0c91f9 # v4.0.0
        with:
          go-version: ${{ needs.get-go-version.outputs.go-version }}
-      - run: go test ./...
+      - run: go test ./... -p 1 # disable parallelism to avoid port conflicts from default metrics and lifecycle server configuration


What kind of impact does this have on the runtime for these tests?

I'd like to clean this up, but skipped for now in the interest of expediency. It didn't feel substantial enough to warrant the effort at this time, as the full suite still completes in under a minute.

pkg/consuldp/config.go

pkg/consuldp/consul_dataplane.go

nathancoleman · 2023-05-31T15:32:43Z

pkg/consuldp/consul_dataplane.go

+		AdminAddr:       cdp.cfg.Envoy.AdminBindAddress,
+		AdminBindPort:   cdp.cfg.Envoy.AdminBindPort,


Curious why these are showing up in this PR

It felt reasonable to pull these out of the consul-dataplane Envoy config at this point when creating the config to pass as the only argument into envoy.NewProxy (happy to change if they're already somewhere else I didn't notice) to make them accessible within pkg/envoy/proxy.go where they're needed for the HTTP calls to Envoy's admin API for the Drain() and Quit() methods.

This was not needed previously, as the Envoy process was just terminated with a process kill signal.

pglass

Looks good, and this is super useful! I added some nits and non-blocking questions.

pkg/consuldp/lifecycle.go

curtbushko

Looking pretty good! I can tell that you put a lot of thought and effort into this!

I only have a few comments, but nothing that I will stop you on.

Note: There is going to be another release of consul-dataplane tomorrow (Thursday, June 1st) so I am going to block here so this doesn't get merged in by accident.

cmd/consul-dataplane/main.go

pkg/consuldp/lifecycle.go

mikemorris · 2023-06-06T12:39:44Z

Addressed feedback and pushed some naming changes up into #100, which I'd like to merge before this to get the chain started.

Co-authored-by: Paul Glass <[email protected]>

…tenersEnabled after rebase

…ecycle shutdown (#115)" This reverts commit 0047e65.

* Revert "proxy-lifecycle: catch SIGTERM and initiate graceful shutdown (#130)" This reverts commit 40c99dc. * Revert "proxy-lifecycle: add HTTP Server with endpoints for proxy lifecycle shutdown (#115)" This reverts commit 0047e65. * Revert "cmd: add CLI flags for proxy shutdown lifecycle management (#100)" This reverts commit 3d37b9f.

…hutdown (#115) * envoy: set drain time and strategy passthrough config with sensible defaults for sidecar proxy shutdown lifecycle * pkg/consuldp/lifecycle: wire up lifecycle mgmt server into consul-dataplane main config * pkg/envoy: add Drain and Quit methods, rename Stop to Kill * pkg/consuldp/lifecycle: gracefully shutdown Envoy if xDS or lifecycle mgmt server exits unexpectedly * ci: disable parallelism in unit tests to avoid port conflicts * pkg/envoy: add http client to dial Envoy admin interface * pkg/consuldp/lifecycle: replace http client with proxy manager interface and mock * test/lifecycle: pick an available port if gracefulPort is unspecified * pkg/consuldp/lifecycle: check errors and close errorExitCh if any problems gracefully shutting down Envoy * add changelong --------- Co-authored-by: Nathan Coleman <[email protected]> Co-authored-by: Paul Glass <[email protected]>

… lifecycle shutdown into release/1.0.x (#182) * proxy-lifecycle: add HTTP Server with endpoints for proxy lifecycle shutdown (#115) * envoy: set drain time and strategy passthrough config with sensible defaults for sidecar proxy shutdown lifecycle * pkg/consuldp/lifecycle: wire up lifecycle mgmt server into consul-dataplane main config * pkg/envoy: add Drain and Quit methods, rename Stop to Kill * pkg/consuldp/lifecycle: gracefully shutdown Envoy if xDS or lifecycle mgmt server exits unexpectedly * ci: disable parallelism in unit tests to avoid port conflicts * pkg/envoy: add http client to dial Envoy admin interface * pkg/consuldp/lifecycle: replace http client with proxy manager interface and mock * test/lifecycle: pick an available port if gracefulPort is unspecified * pkg/consuldp/lifecycle: check errors and close errorExitCh if any problems gracefully shutting down Envoy * add changelong --------- Co-authored-by: Nathan Coleman <[email protected]> Co-authored-by: Paul Glass <[email protected]> * fix automation * fix unit tests --------- Co-authored-by: Mike Morris <[email protected]> Co-authored-by: Nathan Coleman <[email protected]> Co-authored-by: Paul Glass <[email protected]>

mikemorris force-pushed the proxy-lcm/shutdown branch from 5a3d46d to 5be431b Compare May 18, 2023 19:14

mikemorris force-pushed the proxy-lcm/shutdown-http-endpoints branch from ada8d83 to 47d0424 Compare May 18, 2023 19:15

mikemorris force-pushed the proxy-lcm/shutdown branch from 5be431b to 1ffb8ac Compare May 23, 2023 18:44

mikemorris force-pushed the proxy-lcm/shutdown-http-endpoints branch 2 times, most recently from a5bd187 to a6e6f0a Compare May 23, 2023 18:46

mikemorris mentioned this pull request May 23, 2023

cmd: add CLI flags for proxy shutdown lifecycle management #100

Merged

4 tasks

mikemorris force-pushed the proxy-lcm/shutdown-http-endpoints branch from e824175 to 52c214d Compare May 30, 2023 14:24

mikemorris commented May 30, 2023

View reviewed changes

cmd/consul-dataplane/main.go Outdated Show resolved Hide resolved

mikemorris marked this pull request as ready for review May 30, 2023 16:24

mikemorris requested a review from a team as a code owner May 30, 2023 16:24

mikemorris requested review from a team, erichaberkorn, curtbushko and pglass May 30, 2023 16:25

mikemorris commented May 30, 2023

View reviewed changes

mikemorris changed the title ~~pkg/consuldp: add HTTP Server with endpoints for proxy lifecycle shutdown~~ proxy-lifecycle: add HTTP Server with endpoints for proxy lifecycle shutdown May 30, 2023

nathancoleman requested changes May 31, 2023

View reviewed changes

mikemorris mentioned this pull request May 31, 2023

proxy-lifecycle: catch SIGTERM and initiate graceful shutdown #130

Merged

pglass approved these changes May 31, 2023

View reviewed changes

nathancoleman approved these changes May 31, 2023

View reviewed changes

curtbushko requested changes Jun 1, 2023

View reviewed changes

cmd/consul-dataplane/main.go Outdated Show resolved Hide resolved

pkg/consuldp/lifecycle.go Outdated Show resolved Hide resolved

pkg/consuldp/lifecycle.go Show resolved Hide resolved

mikemorris force-pushed the proxy-lcm/shutdown branch from 1ffb8ac to 04e73f2 Compare June 6, 2023 11:15

mikemorris force-pushed the proxy-lcm/shutdown-http-endpoints branch 2 times, most recently from d548340 to 8bc0b14 Compare June 6, 2023 12:37

mikemorris requested review from curtbushko, nathancoleman and pglass June 6, 2023 12:38

pglass approved these changes Jun 6, 2023

View reviewed changes

mikemorris and others added 4 commits June 6, 2023 15:18

pkg/lifecycle: wrap graceful shutdown path config printf in logger

52b4557

Apply suggestions from code review

21595f0

Co-authored-by: Paul Glass <[email protected]>

rename -shutdown-grace-period to -shutdown-grace-period-seconds

b5e3aea

finish renaming to shutdownDrainListenersEnabled and ShutdownDrainLis…

bf8f0c8

…tenersEnabled after rebase

mikemorris force-pushed the proxy-lcm/shutdown-http-endpoints branch from 268e173 to bf8f0c8 Compare June 6, 2023 19:18

add changelong

790881e

curtbushko approved these changes Jun 6, 2023

View reviewed changes

mikemorris merged commit 0047e65 into main Jun 6, 2023

mikemorris deleted the proxy-lcm/shutdown-http-endpoints branch June 6, 2023 19:50

mikemorris mentioned this pull request Jun 6, 2023

fix: log level present in extra args using envoy-extra-args annotation : NET-2190 #133

Merged

DanStough added a commit that referenced this pull request Jun 14, 2023

Revert "proxy-lifecycle: add HTTP Server with endpoints for proxy lif…

bd1da00

…ecycle shutdown (#115)" This reverts commit 0047e65.

DanStough mentioned this pull request Jun 14, 2023

Revert Proxy Lifecycle PRs #152

Merged

curtbushko self-assigned this Jun 27, 2023

This was referenced Jun 27, 2023

Backport of cmd: add CLI flags for proxy shutdown lifecycle management into release/1.1.x #176

Merged

Backport of cmd: add CLI flags for proxy shutdown lifecycle management into release/1.0.x #177

Merged

curtbushko added backport/1.0 backport/1.1 Changes are backported to 1.1 labels Jun 27, 2023

This was referenced Jun 27, 2023

Backport of proxy-lifecycle: add HTTP Server with endpoints for proxy lifecycle shutdown into release/1.0.x #178

Closed

Backport of proxy-lifecycle: add HTTP Server with endpoints for proxy lifecycle shutdown into release/1.1.x #179

Closed

curtbushko added backport/1.0 backport/1.1 Changes are backported to 1.1 and removed backport/1.0 backport/1.1 Changes are backported to 1.1 labels Jun 27, 2023

This was referenced Jun 27, 2023

Backport of proxy-lifecycle: add HTTP Server with endpoints for proxy lifecycle shutdown into release/1.0.x #180

Closed

Backport of proxy-lifecycle: add HTTP Server with endpoints for proxy lifecycle shutdown into release/1.1.x #181

Merged

curtbushko mentioned this pull request Jun 27, 2023

Backport of proxy-lifecycle: add HTTP Server with endpoints for proxy lifecycle shutdown into release/1.0.x #182

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proxy-lifecycle: add HTTP Server with endpoints for proxy lifecycle shutdown #115

proxy-lifecycle: add HTTP Server with endpoints for proxy lifecycle shutdown #115

mikemorris commented May 15, 2023 •

edited

Loading

mikemorris May 30, 2023 •

edited

Loading

mikemorris May 30, 2023 •

edited

Loading

nathancoleman May 31, 2023

mikemorris May 31, 2023

nathancoleman May 31, 2023

mikemorris May 31, 2023 •

edited

Loading

pglass left a comment

curtbushko left a comment

mikemorris commented Jun 6, 2023 •

edited

Loading

		AdminAddr: cdp.cfg.Envoy.AdminBindAddress,
		AdminBindPort: cdp.cfg.Envoy.AdminBindPort,

proxy-lifecycle: add HTTP Server with endpoints for proxy lifecycle shutdown #115

proxy-lifecycle: add HTTP Server with endpoints for proxy lifecycle shutdown #115

Conversation

mikemorris commented May 15, 2023 • edited Loading

Notes for reviewers

Links

mikemorris May 30, 2023 • edited Loading

Choose a reason for hiding this comment

mikemorris May 30, 2023 • edited Loading

Choose a reason for hiding this comment

nathancoleman May 31, 2023

Choose a reason for hiding this comment

mikemorris May 31, 2023

Choose a reason for hiding this comment

nathancoleman May 31, 2023

Choose a reason for hiding this comment

mikemorris May 31, 2023 • edited Loading

Choose a reason for hiding this comment

pglass left a comment

Choose a reason for hiding this comment

curtbushko left a comment

Choose a reason for hiding this comment

mikemorris commented Jun 6, 2023 • edited Loading

mikemorris commented May 15, 2023 •

edited

Loading

mikemorris May 30, 2023 •

edited

Loading

mikemorris May 30, 2023 •

edited

Loading

mikemorris May 31, 2023 •

edited

Loading

mikemorris commented Jun 6, 2023 •

edited

Loading