From 446ee4e5df05008b1dd92187188e8f4cabd3560b Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Thu, 27 Jul 2023 14:34:44 -0500 Subject: [PATCH 01/18] flag setup, function signatures --- .DS_Store | Bin 0 -> 6148 bytes cmd/consul-dataplane/config.go | 9 ++++++++- cmd/consul-dataplane/main.go | 4 +++- pkg/consuldp/config.go | 5 +++++ pkg/consuldp/lifecycle.go | 24 ++++++++++++++++++++---- 5 files changed, 36 insertions(+), 6 deletions(-) create mode 100644 .DS_Store diff --git a/.DS_Store b/.DS_Store new file mode 100644 index 0000000000000000000000000000000000000000..425a07f72165835d7464dbfa22c0ed7a588fe42e GIT binary patch literal 6148 zcmeHK!EVz)5S>jzhT>+EkS)E=igQDp8PX1P4GNuH9h8u_N0dhbU6+ z@D2O|XMTi#;RJ8i8x%V&w<170((c<`&zs$S8GExtB-#^am#9ia1`1C1*6Y|K1_XCRQ zj3V^Llu(>we#tSf(-A$P7CocK)TO8N9JoUxaZSpno4a}BN)FTMBw$w&(0`VrQiD7(|D}D)?e+h@5M>I{!J8$rE42y zqik#$Z>{&?)JpovByRVk*W7y%hQ7UQ_wARSJMC3(9fd*C_X0PP1kZ8d^5&HnIN`J% zP68*ETq}KGRE$cmdUrP4J#6gN%*Mg~yk^c0_v$rszi}|1SB%^DA3i$m4gFCN@+M#c z$BVRWg-iH^#+ncxtSIoqfPYHnm634ANWd=A&!TQlCQg#9OeYYNQ{?glS@kdC1sKvG z8WGl)!4e~iFACefL7q2Q-Hq@CJJLy;UY!( z=|Cl~0KgK4rJ*hV`U6cK05%maQp5;Mq*P!^73zv1)XhPpbUbe=|3!)^orIhjan#I0 z-B5(OcnDLvlW3->YpsA*Age$z?N<5x?_PfY&ysXcE1(tluM`laj@4;nOX_T0+Z>;@ vCdxY$HuhVjC@HAaaV#5r6yHIShG!->fK7#q6fpwRe*~lqy3z{#Q3ZYiR=xvz literal 0 HcmV?d00001 diff --git a/cmd/consul-dataplane/config.go b/cmd/consul-dataplane/config.go index 867439bb..040a43fa 100644 --- a/cmd/consul-dataplane/config.go +++ b/cmd/consul-dataplane/config.go @@ -121,6 +121,9 @@ type EnvoyFlags struct { GracefulShutdownPath *string `json:"gracefulShutdownPath,omitempty"` GracefulPort *int `json:"gracefulPort,omitempty"` DumpEnvoyConfigOnExitEnabled *bool `json:"dumpEnvoyConfigOnExitEnabled,omitempty"` + + StartupGracePeriodSeconds *int `json:"startupGracePeriodSeconds,omitempty"` + GracefulStartupPath *string `json:"gracefulStartupPath,omitempty"` } const ( @@ -217,7 +220,9 @@ func buildDefaultConsulDPFlags() (DataplaneConfigFlags, error) { "shutdownGracePeriodSeconds": 0, "gracefulShutdownPath": "/graceful_shutdown", "gracefulPort": 20300, - "dumpEnvoyConfigOnExitEnabled": false + "dumpEnvoyConfigOnExitEnabled": false, + "gracefulStartupPath": "/graceful_startup", + "startupGracePeriodSeconds": 0 }, "xdsServer": { "bindAddress": "127.0.0.1", @@ -295,6 +300,8 @@ func constructRuntimeConfig(cfg DataplaneConfigFlags, extraArgs []string) (*cons DumpEnvoyConfigOnExitEnabled: boolVal(cfg.Envoy.DumpEnvoyConfigOnExitEnabled), GracefulShutdownPath: stringVal(cfg.Envoy.GracefulShutdownPath), GracefulPort: intVal(cfg.Envoy.GracefulPort), + StartupGracePeriodSeconds: intVal(cfg.Envoy.StartupGracePeriodSeconds), + GracefulStartupPath: stringVal(cfg.Envoy.GracefulStartupPath), ExtraArgs: extraArgs, }, Telemetry: &consuldp.TelemetryConfig{ diff --git a/cmd/consul-dataplane/main.go b/cmd/consul-dataplane/main.go index 5988d2ca..d5098e5a 100644 --- a/cmd/consul-dataplane/main.go +++ b/cmd/consul-dataplane/main.go @@ -102,7 +102,9 @@ func init() { // Default is false, may be useful for debugging unexpected termination. BoolVar(flags, &flagOpts.dataplaneConfig.Envoy.DumpEnvoyConfigOnExitEnabled, "dump-envoy-config-on-exit", "DP_DUMP_ENVOY_CONFIG_ON_EXIT", "Call the Envoy /config_dump endpoint during consul-dataplane controlled shutdown.") - + //######################### + IntVar(flags, &flagOpts.dataplaneConfig.Envoy.StartupGracePeriodSeconds, "startup-grace-period-seconds", "DP_STARTUP_GRACE_PERIOD_SECONDS", "Amount of time to wait for consul-dataplane startup.") + StringVar(flags, &flagOpts.dataplaneConfig.Envoy.GracefulStartupPath, "graceful-startup-path", "DP_GRACEFUL_STARTUP_PATH", "An HTTP path to serve the graceful startup endpoint.") flags.StringVar(&flagOpts.configFile, "config-file", "", "The json config file for configuring consul data plane") } diff --git a/pkg/consuldp/config.go b/pkg/consuldp/config.go index 44d1d966..c9353377 100644 --- a/pkg/consuldp/config.go +++ b/pkg/consuldp/config.go @@ -297,6 +297,11 @@ type EnvoyConfig struct { GracefulPort int // DumpEnvoyConfigOnExitEnabled configures whether to call Envoy's /config_dump endpoint during consul-dataplane controlled shutdown. DumpEnvoyConfigOnExitEnabled bool + //############## + //StartupGracePeriodSeconds is the amount of time to block application after startup for Envoy proxy to be ready. + StartupGracePeriodSeconds int + // GracefulStartupPath is the path on which the HTTP endpoint to initiate a graceful startup of Envoy is served + GracefulStartupPath string // ExtraArgs are the extra arguments passed to envoy at startup of the proxy ExtraArgs []string } diff --git a/pkg/consuldp/lifecycle.go b/pkg/consuldp/lifecycle.go index 68a6d600..cf11c616 100644 --- a/pkg/consuldp/lifecycle.go +++ b/pkg/consuldp/lifecycle.go @@ -36,8 +36,9 @@ type lifecycleConfig struct { shutdownGracePeriodSeconds int gracefulPort int gracefulShutdownPath string - - dumpEnvoyConfigOnExitEnabled bool + startupGracePeriodSeconds int + gracefulStartupPath string + dumpEnvoyConfigOnExitEnabled bool // manager for controlling the Envoy proxy process proxy envoy.ProxyManager @@ -58,8 +59,9 @@ func NewLifecycleConfig(cfg *Config, proxy envoy.ProxyManager) *lifecycleConfig gracefulPort: cfg.Envoy.GracefulPort, gracefulShutdownPath: cfg.Envoy.GracefulShutdownPath, dumpEnvoyConfigOnExitEnabled: cfg.Envoy.DumpEnvoyConfigOnExitEnabled, - - proxy: proxy, + startupGracePeriodSeconds: cfg.Envoy.StartupGracePeriodSeconds, + gracefulStartupPath: cfg.Envoy.GracefulStartupPath, + proxy: proxy, errorExitCh: make(chan struct{}, 1), mu: sync.Mutex{}, @@ -211,3 +213,17 @@ func (m *lifecycleConfig) gracefulShutdown() { // Wait for context timeout to elapse wg.Wait() } + +func (m *lifecycleConfig) gracefulStartupHandler(rw http.ResponseWriter, _ *http.Request) { + // Kick off graceful shutdown in a separate goroutine to avoid blocking + // sending an HTTP response + go func { + m.gracefulStartup() + rw.WriteHeader() + //Delay the +} +} +func (m *lifecycleConfig) gracefulStartup() { + m.logger.Info("Blocking container startup until Envoy ready") + +} From 3f6219771b26dc8e79a0ecf392575da490ab1b43 Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Mon, 31 Jul 2023 10:17:28 -0500 Subject: [PATCH 02/18] first draft graceful startup function and http handler --- pkg/consuldp/lifecycle.go | 61 ++++++++++++++++++++++++++++++++++----- 1 file changed, 53 insertions(+), 8 deletions(-) diff --git a/pkg/consuldp/lifecycle.go b/pkg/consuldp/lifecycle.go index cf11c616..ab732c6f 100644 --- a/pkg/consuldp/lifecycle.go +++ b/pkg/consuldp/lifecycle.go @@ -24,6 +24,7 @@ const ( cdpLifecycleUrl = "http://" + cdpLifecycleBindAddr defaultLifecycleShutdownPath = "/graceful_shutdown" + defaultLifecycleStartupPath = "/graceful_startup" ) // lifecycleConfig handles all configuration related to managing the Envoy proxy @@ -99,6 +100,8 @@ func (m *lifecycleConfig) startLifecycleManager(ctx context.Context) error { m.logger.Info(fmt.Sprintf("setting graceful shutdown path: %s\n", cdpLifecycleShutdownPath)) mux.HandleFunc(cdpLifecycleShutdownPath, m.gracefulShutdownHandler) + mux.HandleFunc(defaultLifecycleStartupPath, m.gracefulStartupHandler) + // Determine what the proxy lifecycle management server bind port is. It can be // set as a flag. cdpLifecycleBindPort := defaultLifecycleBindPort @@ -215,15 +218,57 @@ func (m *lifecycleConfig) gracefulShutdown() { } func (m *lifecycleConfig) gracefulStartupHandler(rw http.ResponseWriter, _ *http.Request) { - // Kick off graceful shutdown in a separate goroutine to avoid blocking - // sending an HTTP response - go func { - m.gracefulStartup() - rw.WriteHeader() - //Delay the -} + + m.gracefulStartup() + //Unlike in gracefulShutdown, we want to delay the OK response until envoy is ready + //in order to block application container. + rw.WriteHeader(http.StatusOK) + } func (m *lifecycleConfig) gracefulStartup() { - m.logger.Info("Blocking container startup until Envoy ready") + m.logger.Info("Blocking container startup until Envoy ready / grace period elapsed") + timeout := time.Duration(m.shutdownGracePeriodSeconds) * time.Second + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + envoyReady := false + var wg sync.WaitGroup + wg.Add(1) + envoyStatus := make(chan bool) + + //Move readiness check into ProxyManager? + //readyUrl := fmt.Sprintf("http://:%d/ready", adminBindAddress, adminBindPort) + + go func() { + defer wg.Done() + loop: + for { + select { + case <-ctx.Done(): + break loop + case envoyReady = <-envoyStatus: + if envoyReady { + break loop + } else { + go func() { + envoyStatus <- m.proxy.Ready() + }() + } + + } + } + + }() + + wg.Wait() + + // Block until context timeout has elapsed + + // Finish graceful shutdown, quit Envoy proxy + if !envoyReady { + m.logger.Info("startup grace period reached before envoy ready") + } + + // Wait for context timeout to elapse } From 44428ae5fad30726c89963468baaae284c1c3a88 Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Wed, 2 Aug 2023 12:10:43 -0500 Subject: [PATCH 03/18] proxy readiness check --- pkg/consuldp/lifecycle.go | 1 + pkg/envoy/proxy.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/pkg/consuldp/lifecycle.go b/pkg/consuldp/lifecycle.go index ab732c6f..94dc74b1 100644 --- a/pkg/consuldp/lifecycle.go +++ b/pkg/consuldp/lifecycle.go @@ -241,6 +241,7 @@ func (m *lifecycleConfig) gracefulStartup() { go func() { defer wg.Done() + envoyStatus <- m.proxy.Ready() loop: for { select { diff --git a/pkg/envoy/proxy.go b/pkg/envoy/proxy.go index 22fde437..64ef6c19 100644 --- a/pkg/envoy/proxy.go +++ b/pkg/envoy/proxy.go @@ -42,6 +42,7 @@ type ProxyManager interface { Quit() error Kill() error DumpConfig() error + Ready() bool } // Proxy manages an Envoy proxy process. @@ -422,3 +423,33 @@ func removeArgAndGetValue(stringAr []string, key string) ([]string, string) { } return stringAr, "" } + +func (p *Proxy) Ready() bool { + envoyReadyURL := fmt.Sprintf("http://%s:%v/ready", p.cfg.AdminAddr, p.cfg.AdminBindPort) + rsp, err := p.client.Get(envoyReadyURL) + if err != nil { + p.cfg.Logger.Error("envoy: admin endpoint not available", "error", err) + return false, err + } + defer rsp.Body.Close() + return rsp.StatusCode == 200 + /* + switch p.getState() { + case stateExited: + // Nothing to do! + return false + case stateStopped: + // Nothing to do! + return false + case stateDraining: + // Nothing to do! + return false + case stateRunning: + // Query ready endpoint to check if proxy is Ready + case initialState: + + default: + return errors.New("proxy must be running to be stopped") + } + */ +} From 108e41344985201788a7b7b16c6e5b7ca46aec38 Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Fri, 11 Aug 2023 14:34:22 -0500 Subject: [PATCH 04/18] readiness check in proxy manager --- pkg/consuldp/lifecycle.go | 14 ++++++-------- pkg/envoy/proxy.go | 26 +++++++++++--------------- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/pkg/consuldp/lifecycle.go b/pkg/consuldp/lifecycle.go index 94dc74b1..8152c2d8 100644 --- a/pkg/consuldp/lifecycle.go +++ b/pkg/consuldp/lifecycle.go @@ -225,6 +225,9 @@ func (m *lifecycleConfig) gracefulStartupHandler(rw http.ResponseWriter, _ *http rw.WriteHeader(http.StatusOK) } + +// gracefulStartup blocks until the startup grace period has elapsed or we have confirmed that +// Envoy proxy is ready. func (m *lifecycleConfig) gracefulStartup() { m.logger.Info("Blocking container startup until Envoy ready / grace period elapsed") timeout := time.Duration(m.shutdownGracePeriodSeconds) * time.Second @@ -236,12 +239,11 @@ func (m *lifecycleConfig) gracefulStartup() { wg.Add(1) envoyStatus := make(chan bool) - //Move readiness check into ProxyManager? - //readyUrl := fmt.Sprintf("http://:%d/ready", adminBindAddress, adminBindPort) - go func() { defer wg.Done() envoyStatus <- m.proxy.Ready() + + //Loop until either proxy is ready or timeout expires. loop: for { select { @@ -251,6 +253,7 @@ func (m *lifecycleConfig) gracefulStartup() { if envoyReady { break loop } else { + //Check if Envoy is ready, don't block here so that timeout break can still happen. go func() { envoyStatus <- m.proxy.Ready() }() @@ -263,13 +266,8 @@ func (m *lifecycleConfig) gracefulStartup() { wg.Wait() - // Block until context timeout has elapsed - - // Finish graceful shutdown, quit Envoy proxy if !envoyReady { m.logger.Info("startup grace period reached before envoy ready") } - // Wait for context timeout to elapse - } diff --git a/pkg/envoy/proxy.go b/pkg/envoy/proxy.go index 64ef6c19..1c5073ea 100644 --- a/pkg/envoy/proxy.go +++ b/pkg/envoy/proxy.go @@ -425,15 +425,7 @@ func removeArgAndGetValue(stringAr []string, key string) ([]string, string) { } func (p *Proxy) Ready() bool { - envoyReadyURL := fmt.Sprintf("http://%s:%v/ready", p.cfg.AdminAddr, p.cfg.AdminBindPort) - rsp, err := p.client.Get(envoyReadyURL) - if err != nil { - p.cfg.Logger.Error("envoy: admin endpoint not available", "error", err) - return false, err - } - defer rsp.Body.Close() - return rsp.StatusCode == 200 - /* + switch p.getState() { case stateExited: // Nothing to do! @@ -444,12 +436,16 @@ func (p *Proxy) Ready() bool { case stateDraining: // Nothing to do! return false - case stateRunning: + case stateRunning, stateInitial: // Query ready endpoint to check if proxy is Ready - case initialState: - - default: - return errors.New("proxy must be running to be stopped") + envoyReadyURL := fmt.Sprintf("http://%s:%v/ready", p.cfg.AdminAddr, p.cfg.AdminBindPort) + rsp, err := p.client.Get(envoyReadyURL) + defer rsp.Body.Close() + if err != nil { + p.cfg.Logger.Error("envoy: admin endpoint not available", "error", err) + return false + } + return rsp.StatusCode == 200 } - */ + } From 9a419fa9f4ff86a323e7d4df68085eb91d9b6453 Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Fri, 11 Aug 2023 14:39:55 -0500 Subject: [PATCH 05/18] fix readiness check --- pkg/consuldp/lifecycle.go | 6 ++-- pkg/consuldp/lifecycle_test.go | 3 ++ pkg/envoy/proxy.go | 50 ++++++++++++++++++---------------- 3 files changed, 33 insertions(+), 26 deletions(-) diff --git a/pkg/consuldp/lifecycle.go b/pkg/consuldp/lifecycle.go index 8152c2d8..da3cb7fe 100644 --- a/pkg/consuldp/lifecycle.go +++ b/pkg/consuldp/lifecycle.go @@ -241,7 +241,8 @@ func (m *lifecycleConfig) gracefulStartup() { go func() { defer wg.Done() - envoyStatus <- m.proxy.Ready() + envoyReady, _ = m.proxy.Ready() + envoyStatus <- envoyReady //Loop until either proxy is ready or timeout expires. loop: @@ -255,7 +256,8 @@ func (m *lifecycleConfig) gracefulStartup() { } else { //Check if Envoy is ready, don't block here so that timeout break can still happen. go func() { - envoyStatus <- m.proxy.Ready() + ready, _ := m.proxy.Ready() + envoyStatus <- ready }() } diff --git a/pkg/consuldp/lifecycle_test.go b/pkg/consuldp/lifecycle_test.go index 6ead73da..2f482411 100644 --- a/pkg/consuldp/lifecycle_test.go +++ b/pkg/consuldp/lifecycle_test.go @@ -213,3 +213,6 @@ func (p *mockProxy) Kill() error { func (p *mockProxy) DumpConfig() error { return nil } +func (p *mockProxy) Ready() (bool, error) { + return false, nil +} diff --git a/pkg/envoy/proxy.go b/pkg/envoy/proxy.go index 1c5073ea..47c082d8 100644 --- a/pkg/envoy/proxy.go +++ b/pkg/envoy/proxy.go @@ -42,7 +42,7 @@ type ProxyManager interface { Quit() error Kill() error DumpConfig() error - Ready() bool + Ready() (bool, error) } // Proxy manages an Envoy proxy process. @@ -424,28 +424,30 @@ func removeArgAndGetValue(stringAr []string, key string) ([]string, string) { return stringAr, "" } -func (p *Proxy) Ready() bool { - - switch p.getState() { - case stateExited: - // Nothing to do! - return false - case stateStopped: - // Nothing to do! - return false - case stateDraining: - // Nothing to do! - return false - case stateRunning, stateInitial: - // Query ready endpoint to check if proxy is Ready - envoyReadyURL := fmt.Sprintf("http://%s:%v/ready", p.cfg.AdminAddr, p.cfg.AdminBindPort) - rsp, err := p.client.Get(envoyReadyURL) - defer rsp.Body.Close() - if err != nil { - p.cfg.Logger.Error("envoy: admin endpoint not available", "error", err) - return false - } - return rsp.StatusCode == 200 +func (p *Proxy) Ready() (bool, error) { + + switch p.getState() { + case stateExited: + // Nothing to do! + return false, nil + case stateStopped: + // Nothing to do! + return false, nil + case stateDraining: + // Nothing to do! + return false, nil + case stateRunning, stateInitial: + // Query ready endpoint to check if proxy is Ready + envoyReadyURL := fmt.Sprintf("http://%s:%v/ready", p.cfg.AdminAddr, p.cfg.AdminBindPort) + rsp, err := p.client.Get(envoyReadyURL) + defer rsp.Body.Close() + if err != nil { + p.cfg.Logger.Error("envoy: admin endpoint not available", "error", err) + return false, err } - + return rsp.StatusCode == 200, nil + default: + return false, nil + } + } From 58644153ee5874e0e0f06d668bdbf7b21d217a15 Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Mon, 14 Aug 2023 11:25:57 -0500 Subject: [PATCH 06/18] changelog + config unit tests --- .changelog/239.txt | 3 +++ cmd/consul-dataplane/config_test.go | 4 ++++ 2 files changed, 7 insertions(+) create mode 100644 .changelog/239.txt diff --git a/.changelog/239.txt b/.changelog/239.txt new file mode 100644 index 00000000..d562d630 --- /dev/null +++ b/.changelog/239.txt @@ -0,0 +1,3 @@ +```release-note:improvement +Add graceful_startup endpoint and postStart hook in order to guarantee that dataplane starts up before application container. +``` diff --git a/cmd/consul-dataplane/config_test.go b/cmd/consul-dataplane/config_test.go index e6e356ac..e45e1bb2 100644 --- a/cmd/consul-dataplane/config_test.go +++ b/cmd/consul-dataplane/config_test.go @@ -86,6 +86,7 @@ func TestConfigGeneration(t *testing.T) { GracefulShutdownPath: "/graceful_shutdown", EnvoyDrainTimeSeconds: 30, GracefulPort: 20300, + GracefulStartupPath: "/graceful_startup", }, Telemetry: &consuldp.TelemetryConfig{ UseCentralConfig: true, @@ -189,6 +190,7 @@ func TestConfigGeneration(t *testing.T) { EnvoyDrainTimeSeconds: 30, GracefulPort: 20300, DumpEnvoyConfigOnExitEnabled: true, + GracefulStartupPath: "/graceful_startup", }, Telemetry: &consuldp.TelemetryConfig{ UseCentralConfig: true, @@ -287,6 +289,7 @@ func TestConfigGeneration(t *testing.T) { EnvoyDrainTimeSeconds: 30, GracefulPort: 20300, DumpEnvoyConfigOnExitEnabled: false, + GracefulStartupPath: "/graceful_startup", }, Telemetry: &consuldp.TelemetryConfig{ UseCentralConfig: true, @@ -410,6 +413,7 @@ func TestConfigGeneration(t *testing.T) { EnvoyDrainTimeSeconds: 30, GracefulPort: 20300, DumpEnvoyConfigOnExitEnabled: false, + GracefulStartupPath: "/graceful_startup", }, Telemetry: &consuldp.TelemetryConfig{ UseCentralConfig: true, From 7d76013225d4c75fb8fc7094e937ba5418480187 Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Mon, 14 Aug 2023 16:02:43 -0500 Subject: [PATCH 07/18] code cleanup, timeout fix log message change --- cmd/consul-dataplane/main.go | 7 +++---- pkg/consuldp/config.go | 9 ++++----- pkg/consuldp/lifecycle.go | 6 +++--- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/cmd/consul-dataplane/main.go b/cmd/consul-dataplane/main.go index d5098e5a..93cd40da 100644 --- a/cmd/consul-dataplane/main.go +++ b/cmd/consul-dataplane/main.go @@ -99,12 +99,11 @@ func init() { IntVar(flags, &flagOpts.dataplaneConfig.Envoy.ShutdownGracePeriodSeconds, "shutdown-grace-period-seconds", "DP_SHUTDOWN_GRACE_PERIOD_SECONDS", "Amount of time to wait after receiving a SIGTERM signal before terminating the proxy.") StringVar(flags, &flagOpts.dataplaneConfig.Envoy.GracefulShutdownPath, "graceful-shutdown-path", "DP_GRACEFUL_SHUTDOWN_PATH", "An HTTP path to serve the graceful shutdown endpoint.") IntVar(flags, &flagOpts.dataplaneConfig.Envoy.GracefulPort, "graceful-port", "DP_GRACEFUL_PORT", "A port to serve HTTP endpoints for graceful shutdown.") - - // Default is false, may be useful for debugging unexpected termination. - BoolVar(flags, &flagOpts.dataplaneConfig.Envoy.DumpEnvoyConfigOnExitEnabled, "dump-envoy-config-on-exit", "DP_DUMP_ENVOY_CONFIG_ON_EXIT", "Call the Envoy /config_dump endpoint during consul-dataplane controlled shutdown.") - //######################### IntVar(flags, &flagOpts.dataplaneConfig.Envoy.StartupGracePeriodSeconds, "startup-grace-period-seconds", "DP_STARTUP_GRACE_PERIOD_SECONDS", "Amount of time to wait for consul-dataplane startup.") StringVar(flags, &flagOpts.dataplaneConfig.Envoy.GracefulStartupPath, "graceful-startup-path", "DP_GRACEFUL_STARTUP_PATH", "An HTTP path to serve the graceful startup endpoint.") + // Default is false, may be useful for debugging unexpected termination. + BoolVar(flags, &flagOpts.dataplaneConfig.Envoy.DumpEnvoyConfigOnExitEnabled, "dump-envoy-config-on-exit", "DP_DUMP_ENVOY_CONFIG_ON_EXIT", "Call the Envoy /config_dump endpoint during consul-dataplane controlled shutdown.") + flags.StringVar(&flagOpts.configFile, "config-file", "", "The json config file for configuring consul data plane") } diff --git a/pkg/consuldp/config.go b/pkg/consuldp/config.go index c9353377..df7add46 100644 --- a/pkg/consuldp/config.go +++ b/pkg/consuldp/config.go @@ -293,15 +293,14 @@ type EnvoyConfig struct { ShutdownGracePeriodSeconds int // GracefulShutdownPath is the path on which the HTTP endpoint to initiate a graceful shutdown of Envoy is served GracefulShutdownPath string - // GracefulPort is the port on which the HTTP server for graceful shutdown endpoints will be available. - GracefulPort int - // DumpEnvoyConfigOnExitEnabled configures whether to call Envoy's /config_dump endpoint during consul-dataplane controlled shutdown. - DumpEnvoyConfigOnExitEnabled bool - //############## //StartupGracePeriodSeconds is the amount of time to block application after startup for Envoy proxy to be ready. StartupGracePeriodSeconds int // GracefulStartupPath is the path on which the HTTP endpoint to initiate a graceful startup of Envoy is served GracefulStartupPath string + // GracefulPort is the port on which the HTTP server for graceful shutdown endpoints will be available. + GracefulPort int + // DumpEnvoyConfigOnExitEnabled configures whether to call Envoy's /config_dump endpoint during consul-dataplane controlled shutdown. + DumpEnvoyConfigOnExitEnabled bool // ExtraArgs are the extra arguments passed to envoy at startup of the proxy ExtraArgs []string } diff --git a/pkg/consuldp/lifecycle.go b/pkg/consuldp/lifecycle.go index da3cb7fe..bc696d2b 100644 --- a/pkg/consuldp/lifecycle.go +++ b/pkg/consuldp/lifecycle.go @@ -219,9 +219,9 @@ func (m *lifecycleConfig) gracefulShutdown() { func (m *lifecycleConfig) gracefulStartupHandler(rw http.ResponseWriter, _ *http.Request) { - m.gracefulStartup() //Unlike in gracefulShutdown, we want to delay the OK response until envoy is ready //in order to block application container. + m.gracefulStartup() rw.WriteHeader(http.StatusOK) } @@ -229,8 +229,8 @@ func (m *lifecycleConfig) gracefulStartupHandler(rw http.ResponseWriter, _ *http // gracefulStartup blocks until the startup grace period has elapsed or we have confirmed that // Envoy proxy is ready. func (m *lifecycleConfig) gracefulStartup() { - m.logger.Info("Blocking container startup until Envoy ready / grace period elapsed") - timeout := time.Duration(m.shutdownGracePeriodSeconds) * time.Second + timeout := time.Duration(m.startupGracePeriodSeconds) * time.Second + m.logger.Info(fmt.Sprintf("blocking container startup until Envoy ready or grace period of %d seconds elapsed", m.startupGracePeriodSeconds)) ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() From d10d4b59927b183af743a6153b7bb3d39a3abf28 Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Tue, 15 Aug 2023 16:13:13 -0500 Subject: [PATCH 08/18] add testing for graceful startup fix test --- pkg/consuldp/lifecycle_test.go | 77 +++++++++++++++++++++++++--------- 1 file changed, 57 insertions(+), 20 deletions(-) diff --git a/pkg/consuldp/lifecycle_test.go b/pkg/consuldp/lifecycle_test.go index 2f482411..9816ca59 100644 --- a/pkg/consuldp/lifecycle_test.go +++ b/pkg/consuldp/lifecycle_test.go @@ -21,6 +21,8 @@ var ( envoyAdminAddr = "127.0.0.1" ) +// TestLifecycleServerClosed tests that the lifecycle manager properly starts up +// and shuts down when the context passed into it is cancelled. func TestLifecycleServerClosed(t *testing.T) { cfg := Config{ Envoy: &EnvoyConfig{ @@ -38,35 +40,35 @@ func TestLifecycleServerClosed(t *testing.T) { require.Eventually(t, func() bool { return !m.running }, time.Second*2, time.Second) - } -func TestLifecycleServerEnabled(t *testing.T) { +func TestLifecycleServer(t *testing.T) { cases := map[string]struct { shutdownDrainListenersEnabled bool shutdownGracePeriodSeconds int gracefulShutdownPath string + startupGracePeriodSeconds int + gracefulStartupPath string gracefulPort int + proxyStartupDelaySeconds int }{ - // TODO: testing the actual Envoy behavior here such as how open or new - // connections are handled should happpen in integration or acceptance tests - "connection draining disabled without grace period": { + "connection draining disabled without shutdown grace period": { // All inbound and outbound connections are terminated immediately. }, - "connection draining enabled without grace period": { + "connection draining enabled without shutdown grace period": { // This should immediately send "Connection: close" to inbound HTTP1 // connections, GOAWAY to inbound HTTP2, and terminate connections on // request completion. Outbound connections should start being rejected // immediately. shutdownDrainListenersEnabled: true, }, - "connection draining disabled with grace period": { + "connection draining disabled with shutdown grace period": { // This should immediately terminate any open inbound connections. // Outbound connections should be allowed until the grace period has // elapsed. shutdownGracePeriodSeconds: 5, }, - "connection draining enabled with grace period": { + "connection draining enabled with shutdown grace period": { // This should immediately send "Connection: close" to inbound HTTP1 // connections, GOAWAY to inbound HTTP2, and terminate connections on // request completion. @@ -80,11 +82,21 @@ func TestLifecycleServerEnabled(t *testing.T) { shutdownDrainListenersEnabled: true, shutdownGracePeriodSeconds: 5, gracefulShutdownPath: "/quit-nicely", - // TODO: should this be random or use freeport? logic disallows passing - // zero value explicitly - gracefulPort: 23108, + gracefulPort: 23108, + }, + "startup grace period with default path, no startup time": { + startupGracePeriodSeconds: 5, + }, + "startup grace period with default path, small startup time": { + startupGracePeriodSeconds: 10, + proxyStartupDelaySeconds: 5, + }, + "startup grace period with default path, large startup time": { + startupGracePeriodSeconds: 10, + proxyStartupDelaySeconds: 15, }, } + for name, c := range cases { c := c log.Printf("config = %v", c) @@ -145,12 +157,32 @@ func TestLifecycleServerEnabled(t *testing.T) { if c.gracefulShutdownPath != "" { require.Equal(t, m.gracefulShutdownPath, c.gracefulShutdownPath, "failed to set lifecycle server graceful shutdown HTTP endpoint path") } + shutdownUrl := fmt.Sprintf("http://127.0.0.1:%d%s", port, m.gracefulShutdownPath) - // Check lifecycle server graceful shutdown path configuration - url := fmt.Sprintf("http://127.0.0.1:%d%s", port, m.gracefulShutdownPath) - log.Printf("sending request to %s\n", url) + // Check lifecycle server graceful startup path configuration + if c.gracefulStartupPath != "" { + require.Equal(t, m.gracefulStartupPath, c.gracefulStartupPath, "failed to set lifecycle server graceful startup HTTP endpoint path") + } + startupURL := fmt.Sprintf("http://127.0.0.1:%d%s", port, m.gracefulStartupPath) + + // Start the mock proxy. + go m.proxy.Run(ctx) + start := time.Now() + log.Printf("sending startup check request to %s\n", startupURL) + resp, err := http.Get(startupURL) + require.NoError(t, err) + require.True(t, resp.StatusCode == 200) + duration := time.Since(start) + var expectedTime int + if c.proxyStartupDelaySeconds < c.startupGracePeriodSeconds { + expectedTime = c.proxyStartupDelaySeconds + } else { + expectedTime = c.startupGracePeriodSeconds + } + require.True(t, duration.Seconds()-float64(time.Duration(expectedTime)) < 1) - resp, err := http.Get(url) + log.Printf("sending request to %s\n", shutdownUrl) + resp, err = http.Get(shutdownUrl) // HTTP handler is not blocking, so need to wait and check mock // client for expected method calls to proxy manager within @@ -185,14 +217,19 @@ func TestLifecycleServerEnabled(t *testing.T) { } type mockProxy struct { - runCalled int - drainCalled int - quitCalled int - killCalled int + runCalled int + drainCalled int + quitCalled int + killCalled int + isReady bool + startupDelaySeconds int } func (p *mockProxy) Run(ctx context.Context) error { p.runCalled++ + time.Sleep(time.Duration(p.startupDelaySeconds) * time.Second) + p.isReady = true + return nil } @@ -214,5 +251,5 @@ func (p *mockProxy) DumpConfig() error { return nil } func (p *mockProxy) Ready() (bool, error) { - return false, nil + return p.isReady, nil } From bdf56a5a766eabd9bdf9dd72d6314df496341e52 Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Tue, 15 Aug 2023 16:13:57 -0500 Subject: [PATCH 09/18] logic cleanup --- pkg/consuldp/lifecycle.go | 35 ++++++++++++++++++++++------------- pkg/envoy/proxy.go | 13 ++----------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/pkg/consuldp/lifecycle.go b/pkg/consuldp/lifecycle.go index bc696d2b..713680b4 100644 --- a/pkg/consuldp/lifecycle.go +++ b/pkg/consuldp/lifecycle.go @@ -87,20 +87,11 @@ func (m *lifecycleConfig) startLifecycleManager(ctx context.Context) error { // management control mux := http.NewServeMux() - // Determine what HTTP endpoint paths to configure for the proxy lifecycle - // management server. These can be set as flags. - cdpLifecycleShutdownPath := defaultLifecycleShutdownPath - if m.gracefulShutdownPath != "" { - cdpLifecycleShutdownPath = m.gracefulShutdownPath - } - - // Set config to allow introspection of default path for testing - m.gracefulShutdownPath = cdpLifecycleShutdownPath - - m.logger.Info(fmt.Sprintf("setting graceful shutdown path: %s\n", cdpLifecycleShutdownPath)) - mux.HandleFunc(cdpLifecycleShutdownPath, m.gracefulShutdownHandler) + m.logger.Info(fmt.Sprintf("setting graceful shutdown path: %s\n", m.shutdownPath())) + mux.HandleFunc(m.shutdownPath(), m.gracefulShutdownHandler) - mux.HandleFunc(defaultLifecycleStartupPath, m.gracefulStartupHandler) + m.logger.Info(fmt.Sprintf("setting graceful startup path: %s\n", m.startupPath())) + mux.HandleFunc(m.startupPath(), m.gracefulStartupHandler) // Determine what the proxy lifecycle management server bind port is. It can be // set as a flag. @@ -273,3 +264,21 @@ func (m *lifecycleConfig) gracefulStartup() { } } + +func (m *lifecycleConfig) shutdownPath() string { + if m.gracefulShutdownPath == "" { + // Set config to allow introspection of default path for testing + m.gracefulShutdownPath = defaultLifecycleShutdownPath + } + + return m.gracefulShutdownPath +} + +func (m *lifecycleConfig) startupPath() string { + if m.gracefulStartupPath == "" { + // Set config to allow introspection of default path for testing + m.gracefulStartupPath = defaultLifecycleStartupPath + } + + return m.gracefulStartupPath +} diff --git a/pkg/envoy/proxy.go b/pkg/envoy/proxy.go index 47c082d8..a81e268c 100644 --- a/pkg/envoy/proxy.go +++ b/pkg/envoy/proxy.go @@ -225,10 +225,7 @@ func (p *Proxy) Quit() error { envoyShutdownUrl := fmt.Sprintf("http://%s:%v/quitquitquit", p.cfg.AdminAddr, p.cfg.AdminBindPort) switch p.getState() { - case stateExited: - // Nothing to do! - return nil - case stateStopped: + case stateExited, stateStopped: // Nothing to do! return nil case stateDraining: @@ -427,13 +424,7 @@ func removeArgAndGetValue(stringAr []string, key string) ([]string, string) { func (p *Proxy) Ready() (bool, error) { switch p.getState() { - case stateExited: - // Nothing to do! - return false, nil - case stateStopped: - // Nothing to do! - return false, nil - case stateDraining: + case stateExited, stateStopped, stateDraining: // Nothing to do! return false, nil case stateRunning, stateInitial: From 76700255d4273066b7f73afb1274af93ab3f72c1 Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Wed, 16 Aug 2023 09:36:01 -0500 Subject: [PATCH 10/18] graceful startup test cases --- pkg/consuldp/lifecycle_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/consuldp/lifecycle_test.go b/pkg/consuldp/lifecycle_test.go index 9816ca59..0ce12605 100644 --- a/pkg/consuldp/lifecycle_test.go +++ b/pkg/consuldp/lifecycle_test.go @@ -87,13 +87,16 @@ func TestLifecycleServer(t *testing.T) { "startup grace period with default path, no startup time": { startupGracePeriodSeconds: 5, }, - "startup grace period with default path, small startup time": { + "startup grace period with default path, no grace period": { + startupGracePeriodSeconds: 5, + }, + "startup grace period with default path, grace period > startup time": { startupGracePeriodSeconds: 10, proxyStartupDelaySeconds: 5, }, - "startup grace period with default path, large startup time": { - startupGracePeriodSeconds: 10, - proxyStartupDelaySeconds: 15, + "startup grace period with default path, grace period < startup time": { + startupGracePeriodSeconds: 5, + proxyStartupDelaySeconds: 10, }, } From 90ec82c2c598b222a91071165830f5047435d359 Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Wed, 16 Aug 2023 11:03:06 -0500 Subject: [PATCH 11/18] skip blocking if grace period = 0 --- cmd/consul-dataplane/config.go | 7 ++++--- pkg/consuldp/lifecycle.go | 3 +++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/cmd/consul-dataplane/config.go b/cmd/consul-dataplane/config.go index 040a43fa..30630a8f 100644 --- a/cmd/consul-dataplane/config.go +++ b/cmd/consul-dataplane/config.go @@ -121,9 +121,10 @@ type EnvoyFlags struct { GracefulShutdownPath *string `json:"gracefulShutdownPath,omitempty"` GracefulPort *int `json:"gracefulPort,omitempty"` DumpEnvoyConfigOnExitEnabled *bool `json:"dumpEnvoyConfigOnExitEnabled,omitempty"` - - StartupGracePeriodSeconds *int `json:"startupGracePeriodSeconds,omitempty"` - GracefulStartupPath *string `json:"gracefulStartupPath,omitempty"` + //Time in seconds to wait for dataplane to be ready. + StartupGracePeriodSeconds *int `json:"startupGracePeriodSeconds,omitempty"` + //Endpoint for graceful startup function. + GracefulStartupPath *string `json:"gracefulStartupPath,omitempty"` } const ( diff --git a/pkg/consuldp/lifecycle.go b/pkg/consuldp/lifecycle.go index 713680b4..b01a49e1 100644 --- a/pkg/consuldp/lifecycle.go +++ b/pkg/consuldp/lifecycle.go @@ -220,6 +220,9 @@ func (m *lifecycleConfig) gracefulStartupHandler(rw http.ResponseWriter, _ *http // gracefulStartup blocks until the startup grace period has elapsed or we have confirmed that // Envoy proxy is ready. func (m *lifecycleConfig) gracefulStartup() { + if m.startupGracePeriodSeconds == 0 { + return + } timeout := time.Duration(m.startupGracePeriodSeconds) * time.Second m.logger.Info(fmt.Sprintf("blocking container startup until Envoy ready or grace period of %d seconds elapsed", m.startupGracePeriodSeconds)) ctx, cancel := context.WithTimeout(context.Background(), timeout) From fda10d79fd13454227cbebaed3f219417b1c32f5 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Thu, 17 Aug 2023 13:23:36 -0400 Subject: [PATCH 12/18] Check the error from running the mock proxy --- pkg/consuldp/lifecycle_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/consuldp/lifecycle_test.go b/pkg/consuldp/lifecycle_test.go index 0ce12605..2fa04e92 100644 --- a/pkg/consuldp/lifecycle_test.go +++ b/pkg/consuldp/lifecycle_test.go @@ -169,7 +169,10 @@ func TestLifecycleServer(t *testing.T) { startupURL := fmt.Sprintf("http://127.0.0.1:%d%s", port, m.gracefulStartupPath) // Start the mock proxy. - go m.proxy.Run(ctx) + go func() { + err := m.proxy.Run(ctx) + require.NoError(t, err) + }() start := time.Now() log.Printf("sending startup check request to %s\n", startupURL) resp, err := http.Get(startupURL) From 8ba9005f13add79973f438d104b372e2a4a7c188 Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Fri, 18 Aug 2023 11:06:55 -0400 Subject: [PATCH 13/18] comments and err logging --- pkg/consuldp/config.go | 2 +- pkg/consuldp/lifecycle.go | 14 +++++++++++--- pkg/consuldp/lifecycle_test.go | 2 ++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/pkg/consuldp/config.go b/pkg/consuldp/config.go index df7add46..cc79dbcd 100644 --- a/pkg/consuldp/config.go +++ b/pkg/consuldp/config.go @@ -295,7 +295,7 @@ type EnvoyConfig struct { GracefulShutdownPath string //StartupGracePeriodSeconds is the amount of time to block application after startup for Envoy proxy to be ready. StartupGracePeriodSeconds int - // GracefulStartupPath is the path on which the HTTP endpoint to initiate a graceful startup of Envoy is served + // GracefulStartupPath is the path where the HTTP endpoint to initiate a graceful startup of Envoy is served GracefulStartupPath string // GracefulPort is the port on which the HTTP server for graceful shutdown endpoints will be available. GracefulPort int diff --git a/pkg/consuldp/lifecycle.go b/pkg/consuldp/lifecycle.go index b01a49e1..c7864b72 100644 --- a/pkg/consuldp/lifecycle.go +++ b/pkg/consuldp/lifecycle.go @@ -235,8 +235,13 @@ func (m *lifecycleConfig) gracefulStartup() { go func() { defer wg.Done() - envoyReady, _ = m.proxy.Ready() - envoyStatus <- envoyReady + go func() { + envoyReady, err := m.proxy.Ready() + if err != nil { + m.logger.Info(fmt.Sprintf("error when querying proxy readiness, %s", err.Error())) + } + envoyStatus <- envoyReady + }() //Loop until either proxy is ready or timeout expires. loop: @@ -250,7 +255,10 @@ func (m *lifecycleConfig) gracefulStartup() { } else { //Check if Envoy is ready, don't block here so that timeout break can still happen. go func() { - ready, _ := m.proxy.Ready() + ready, err := m.proxy.Ready() + if err != nil { + m.logger.Info(fmt.Sprintf("error when querying proxy readiness, %s", err.Error())) + } envoyStatus <- ready }() } diff --git a/pkg/consuldp/lifecycle_test.go b/pkg/consuldp/lifecycle_test.go index 2fa04e92..541aafe1 100644 --- a/pkg/consuldp/lifecycle_test.go +++ b/pkg/consuldp/lifecycle_test.go @@ -42,6 +42,8 @@ func TestLifecycleServerClosed(t *testing.T) { }, time.Second*2, time.Second) } +// TestLifecycleServer tests the different functions of the lifecycle server, +// using a mock proxy, including graceful_shutdown and graceful_startup. func TestLifecycleServer(t *testing.T) { cases := map[string]struct { shutdownDrainListenersEnabled bool From 719fe982b7cd55c8bb595c182017ae78bd55c29a Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Tue, 22 Aug 2023 13:53:43 -0500 Subject: [PATCH 14/18] changed graceful startup based on suggestion from curt --- pkg/consuldp/lifecycle.go | 54 ++++++++++----------------------------- 1 file changed, 14 insertions(+), 40 deletions(-) diff --git a/pkg/consuldp/lifecycle.go b/pkg/consuldp/lifecycle.go index c7864b72..70564902 100644 --- a/pkg/consuldp/lifecycle.go +++ b/pkg/consuldp/lifecycle.go @@ -9,6 +9,7 @@ import ( "net/http" "strconv" "sync" + "sync/atomic" "time" "github.com/hashicorp/go-hclog" @@ -223,57 +224,30 @@ func (m *lifecycleConfig) gracefulStartup() { if m.startupGracePeriodSeconds == 0 { return } - timeout := time.Duration(m.startupGracePeriodSeconds) * time.Second - m.logger.Info(fmt.Sprintf("blocking container startup until Envoy ready or grace period of %d seconds elapsed", m.startupGracePeriodSeconds)) - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() - envoyReady := false - var wg sync.WaitGroup - wg.Add(1) - envoyStatus := make(chan bool) + ctx, cancel := context.WithTimeout(context.Background(), time.Duration(m.startupGracePeriodSeconds)*time.Second) + defer cancel() + var ready atomic.Bool go func() { - defer wg.Done() - go func() { - envoyReady, err := m.proxy.Ready() + for ctx.Err() == nil { + r, err := m.proxy.Ready() if err != nil { m.logger.Info(fmt.Sprintf("error when querying proxy readiness, %s", err.Error())) } - envoyStatus <- envoyReady - }() - - //Loop until either proxy is ready or timeout expires. - loop: - for { - select { - case <-ctx.Done(): - break loop - case envoyReady = <-envoyStatus: - if envoyReady { - break loop - } else { - //Check if Envoy is ready, don't block here so that timeout break can still happen. - go func() { - ready, err := m.proxy.Ready() - if err != nil { - m.logger.Info(fmt.Sprintf("error when querying proxy readiness, %s", err.Error())) - } - envoyStatus <- ready - }() - } - + if r { + ready.Store(true) + cancel() + break } + time.Sleep(50 * time.Millisecond) } - }() - wg.Wait() - - if !envoyReady { - m.logger.Info("startup grace period reached before envoy ready") + <-ctx.Done() + if !ready.Load() { + m.logger.Info("grace period elapsed before proxy ready") } - } func (m *lifecycleConfig) shutdownPath() string { From 147e688b087c0cda3f78cefb9e9c650b18b1f20c Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Tue, 22 Aug 2023 13:54:07 -0500 Subject: [PATCH 15/18] split lifecycle tests split lifecycle tests into separate startup/shutdown --- pkg/consuldp/lifecycle_test.go | 159 +++++++++++++++++++++++++-------- 1 file changed, 121 insertions(+), 38 deletions(-) diff --git a/pkg/consuldp/lifecycle_test.go b/pkg/consuldp/lifecycle_test.go index 541aafe1..0b54072c 100644 --- a/pkg/consuldp/lifecycle_test.go +++ b/pkg/consuldp/lifecycle_test.go @@ -41,18 +41,134 @@ func TestLifecycleServerClosed(t *testing.T) { return !m.running }, time.Second*2, time.Second) } +func TestLifecycleServer_Startup(t *testing.T) { + cases := map[string]struct { + startupGracePeriodSeconds int + gracefulStartupPath string + gracefulPort int + proxyStartupDelaySeconds int + }{ + "startup grace period with default path, no startup time": { + startupGracePeriodSeconds: 5, + }, + "startup time with default path, no grace period": { + proxyStartupDelaySeconds: 5, + }, + "startup time and grace period with default path, grace period > startup time": { + startupGracePeriodSeconds: 10, + proxyStartupDelaySeconds: 5, + }, + "startup time and grace period with default path, grace period < startup time": { + startupGracePeriodSeconds: 5, + proxyStartupDelaySeconds: 10, + }, + "startup time and grace period with custom path, grace period < startup time": { + startupGracePeriodSeconds: 5, + proxyStartupDelaySeconds: 10, + gracefulStartupPath: "/custom_startup", + }, + } + for name, c := range cases { + c := c + log.Printf("config = %v", c) + + t.Run(name, func(t *testing.T) { + // Add a small margin of error for assertions checking expected + // behavior within the shutdown grace period window. + + cfg := Config{ + Envoy: &EnvoyConfig{ + AdminBindAddress: envoyAdminAddr, + AdminBindPort: envoyAdminPort, + GracefulPort: c.gracefulPort, + GracefulStartupPath: c.gracefulStartupPath, + StartupGracePeriodSeconds: c.startupGracePeriodSeconds, + }, + } + m := NewLifecycleConfig(&cfg, &mockProxy{ + startupDelaySeconds: c.proxyStartupDelaySeconds, + }) + + require.NotNil(t, m) + require.NotNil(t, m.proxy) + require.NotNil(t, m.errorExitCh) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + err := m.startLifecycleManager(ctx) + require.NoError(t, err) + + // Have consul-dataplane's lifecycle server start on an open port + // and figure out what port was used so we can make requests to it. + // Conveniently, this seems to wait until the server is ready for requests. + portCh := make(chan int, 1) + if c.gracefulPort == 0 { + m.lifecycleServer.Addr = "127.0.0.1:0" + } + m.lifecycleServer.BaseContext = func(l net.Listener) context.Context { + portCh <- l.Addr().(*net.TCPAddr).Port + return context.Background() + } + + var port int + select { + case port = <-portCh: + case <-time.After(5 * time.Second): + } + + // Check lifecycle server graceful port configuration + if c.gracefulPort != 0 { + require.Equal(t, c.gracefulPort, port, "failed to set lifecycle server port") + } else { + require.NotEqual(t, 0, port, "failed to figure out lifecycle server port") + } + log.Printf("port = %v\n", port) + + // Check lifecycle server graceful startup path configuration + if c.gracefulStartupPath != "" { + require.Equal(t, m.gracefulStartupPath, c.gracefulStartupPath, "failed to set lifecycle server graceful startup HTTP endpoint path") + } + startupURL := fmt.Sprintf("http://127.0.0.1:%d%s", port, m.gracefulStartupPath) + + // Start the mock proxy. + go func() { + fmt.Print("starting go func") + err := m.proxy.Run(ctx) + require.NoError(t, err) + fmt.Print("proxy should be running") + }() + start := time.Now() + log.Printf("sending startup check request to %s\n", startupURL) + resp, err := http.Get(startupURL) + require.NoError(t, err) + require.True(t, resp.StatusCode == 200) + duration := time.Since(start) + var expectedTime int + if c.proxyStartupDelaySeconds < c.startupGracePeriodSeconds { + expectedTime = c.proxyStartupDelaySeconds + } else { + expectedTime = c.startupGracePeriodSeconds + } + require.True(t, duration.Seconds()-float64(time.Duration(expectedTime)) < 1) + require.NoError(t, err) + require.NotNil(t, resp) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.NotNil(t, body) + }) + } + +} // TestLifecycleServer tests the different functions of the lifecycle server, // using a mock proxy, including graceful_shutdown and graceful_startup. -func TestLifecycleServer(t *testing.T) { +func TestLifecycleServer_Shutdown(t *testing.T) { cases := map[string]struct { shutdownDrainListenersEnabled bool shutdownGracePeriodSeconds int gracefulShutdownPath string - startupGracePeriodSeconds int - gracefulStartupPath string gracefulPort int - proxyStartupDelaySeconds int }{ "connection draining disabled without shutdown grace period": { // All inbound and outbound connections are terminated immediately. @@ -86,20 +202,6 @@ func TestLifecycleServer(t *testing.T) { gracefulShutdownPath: "/quit-nicely", gracefulPort: 23108, }, - "startup grace period with default path, no startup time": { - startupGracePeriodSeconds: 5, - }, - "startup grace period with default path, no grace period": { - startupGracePeriodSeconds: 5, - }, - "startup grace period with default path, grace period > startup time": { - startupGracePeriodSeconds: 10, - proxyStartupDelaySeconds: 5, - }, - "startup grace period with default path, grace period < startup time": { - startupGracePeriodSeconds: 5, - proxyStartupDelaySeconds: 10, - }, } for name, c := range cases { @@ -164,33 +266,14 @@ func TestLifecycleServer(t *testing.T) { } shutdownUrl := fmt.Sprintf("http://127.0.0.1:%d%s", port, m.gracefulShutdownPath) - // Check lifecycle server graceful startup path configuration - if c.gracefulStartupPath != "" { - require.Equal(t, m.gracefulStartupPath, c.gracefulStartupPath, "failed to set lifecycle server graceful startup HTTP endpoint path") - } - startupURL := fmt.Sprintf("http://127.0.0.1:%d%s", port, m.gracefulStartupPath) - // Start the mock proxy. go func() { err := m.proxy.Run(ctx) require.NoError(t, err) }() - start := time.Now() - log.Printf("sending startup check request to %s\n", startupURL) - resp, err := http.Get(startupURL) - require.NoError(t, err) - require.True(t, resp.StatusCode == 200) - duration := time.Since(start) - var expectedTime int - if c.proxyStartupDelaySeconds < c.startupGracePeriodSeconds { - expectedTime = c.proxyStartupDelaySeconds - } else { - expectedTime = c.startupGracePeriodSeconds - } - require.True(t, duration.Seconds()-float64(time.Duration(expectedTime)) < 1) log.Printf("sending request to %s\n", shutdownUrl) - resp, err = http.Get(shutdownUrl) + resp, err := http.Get(shutdownUrl) // HTTP handler is not blocking, so need to wait and check mock // client for expected method calls to proxy manager within From 8950d208bac02fc3f7723a9e45e7dffa548c01db Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Wed, 23 Aug 2023 09:05:21 -0500 Subject: [PATCH 16/18] Delete .DS_Store --- .DS_Store | Bin 6148 -> 0 bytes 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 .DS_Store diff --git a/.DS_Store b/.DS_Store deleted file mode 100644 index 425a07f72165835d7464dbfa22c0ed7a588fe42e..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 6148 zcmeHK!EVz)5S>jzhT>+EkS)E=igQDp8PX1P4GNuH9h8u_N0dhbU6+ z@D2O|XMTi#;RJ8i8x%V&w<170((c<`&zs$S8GExtB-#^am#9ia1`1C1*6Y|K1_XCRQ zj3V^Llu(>we#tSf(-A$P7CocK)TO8N9JoUxaZSpno4a}BN)FTMBw$w&(0`VrQiD7(|D}D)?e+h@5M>I{!J8$rE42y zqik#$Z>{&?)JpovByRVk*W7y%hQ7UQ_wARSJMC3(9fd*C_X0PP1kZ8d^5&HnIN`J% zP68*ETq}KGRE$cmdUrP4J#6gN%*Mg~yk^c0_v$rszi}|1SB%^DA3i$m4gFCN@+M#c z$BVRWg-iH^#+ncxtSIoqfPYHnm634ANWd=A&!TQlCQg#9OeYYNQ{?glS@kdC1sKvG z8WGl)!4e~iFACefL7q2Q-Hq@CJJLy;UY!( z=|Cl~0KgK4rJ*hV`U6cK05%maQp5;Mq*P!^73zv1)XhPpbUbe=|3!)^orIhjan#I0 z-B5(OcnDLvlW3->YpsA*Age$z?N<5x?_PfY&ysXcE1(tluM`laj@4;nOX_T0+Z>;@ vCdxY$HuhVjC@HAaaV#5r6yHIShG!->fK7#q6fpwRe*~lqy3z{#Q3ZYiR=xvz From d1f0f8c37f247c2633e39cec8619235e6ca775b5 Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Wed, 23 Aug 2023 09:09:57 -0500 Subject: [PATCH 17/18] upgrade logger to warn if proxy starts before ready --- pkg/consuldp/lifecycle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/consuldp/lifecycle.go b/pkg/consuldp/lifecycle.go index 70564902..0084cf38 100644 --- a/pkg/consuldp/lifecycle.go +++ b/pkg/consuldp/lifecycle.go @@ -246,7 +246,7 @@ func (m *lifecycleConfig) gracefulStartup() { <-ctx.Done() if !ready.Load() { - m.logger.Info("grace period elapsed before proxy ready") + m.logger.Warn("grace period elapsed before proxy ready") } } From f1bc5455a93c2af247748e347019612d4c93b9df Mon Sep 17 00:00:00 2001 From: trevorLeonHC Date: Wed, 23 Aug 2023 09:17:51 -0500 Subject: [PATCH 18/18] comments --- pkg/consuldp/config.go | 6 +++--- pkg/consuldp/lifecycle.go | 2 -- pkg/consuldp/lifecycle_test.go | 7 +++++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/pkg/consuldp/config.go b/pkg/consuldp/config.go index cc79dbcd..23977446 100644 --- a/pkg/consuldp/config.go +++ b/pkg/consuldp/config.go @@ -291,11 +291,11 @@ type EnvoyConfig struct { ShutdownDrainListenersEnabled bool // ShutdownGracePeriodSeconds is the amount of time to wait after receiving a SIGTERM before terminating the proxy container. ShutdownGracePeriodSeconds int - // GracefulShutdownPath is the path on which the HTTP endpoint to initiate a graceful shutdown of Envoy is served + // GracefulShutdownPath is the path on which the HTTP endpoint to initiate a graceful shutdown of Envoy is served. GracefulShutdownPath string - //StartupGracePeriodSeconds is the amount of time to block application after startup for Envoy proxy to be ready. + // StartupGracePeriodSeconds is the amount of time to block application after startup for Envoy proxy to be ready. StartupGracePeriodSeconds int - // GracefulStartupPath is the path where the HTTP endpoint to initiate a graceful startup of Envoy is served + // GracefulStartupPath is the path where the HTTP endpoint to initiate a graceful startup of Envoy is served. GracefulStartupPath string // GracefulPort is the port on which the HTTP server for graceful shutdown endpoints will be available. GracefulPort int diff --git a/pkg/consuldp/lifecycle.go b/pkg/consuldp/lifecycle.go index 0084cf38..beb00464 100644 --- a/pkg/consuldp/lifecycle.go +++ b/pkg/consuldp/lifecycle.go @@ -210,12 +210,10 @@ func (m *lifecycleConfig) gracefulShutdown() { } func (m *lifecycleConfig) gracefulStartupHandler(rw http.ResponseWriter, _ *http.Request) { - //Unlike in gracefulShutdown, we want to delay the OK response until envoy is ready //in order to block application container. m.gracefulStartup() rw.WriteHeader(http.StatusOK) - } // gracefulStartup blocks until the startup grace period has elapsed or we have confirmed that diff --git a/pkg/consuldp/lifecycle_test.go b/pkg/consuldp/lifecycle_test.go index 0b54072c..24d1a280 100644 --- a/pkg/consuldp/lifecycle_test.go +++ b/pkg/consuldp/lifecycle_test.go @@ -41,6 +41,9 @@ func TestLifecycleServerClosed(t *testing.T) { return !m.running }, time.Second*2, time.Second) } + +// TestLifecycleServer_Startup the graceful startup functionality of the dataplane +// using different grace period and simulated startup duration configurations. func TestLifecycleServer_Startup(t *testing.T) { cases := map[string]struct { startupGracePeriodSeconds int @@ -161,8 +164,8 @@ func TestLifecycleServer_Startup(t *testing.T) { } -// TestLifecycleServer tests the different functions of the lifecycle server, -// using a mock proxy, including graceful_shutdown and graceful_startup. +// TestLifecycleServer_Shutdown the graceful shutdown functionality of the dataplane +// with different grace period and listener draining configurations. func TestLifecycleServer_Shutdown(t *testing.T) { cases := map[string]struct { shutdownDrainListenersEnabled bool