From 5021a7a229812d73e0d5ad9e4d1e24791a8a5c92 Mon Sep 17 00:00:00 2001 From: Plamen Bardarov Date: Fri, 23 May 2025 09:36:35 +0300 Subject: [PATCH 1/3] add configuration for default declarative healthcheck timeout --- depot/transformer/transformer.go | 26 ++++++++++++++++---------- depot/transformer/transformer_test.go | 17 +++++++++-------- initializer/initializer.go | 6 +++++- 3 files changed, 30 insertions(+), 19 deletions(-) diff --git a/depot/transformer/transformer.go b/depot/transformer/transformer.go index 55cf1e5d..fb43b54d 100644 --- a/depot/transformer/transformer.go +++ b/depot/transformer/transformer.go @@ -55,13 +55,14 @@ type transformer struct { tempDir string clock clock.Clock - sidecarRootFS string - useDeclarativeHealthCheck bool - emitHealthCheckMetrics bool - healthyMonitoringInterval time.Duration - unhealthyMonitoringInterval time.Duration - gracefulShutdownInterval time.Duration - healthCheckWorkPool *workpool.WorkPool + sidecarRootFS string + useDeclarativeHealthCheck bool + declarativeHealthCheckDefaultTimeout time.Duration + emitHealthCheckMetrics bool + healthyMonitoringInterval time.Duration + unhealthyMonitoringInterval time.Duration + gracefulShutdownInterval time.Duration + healthCheckWorkPool *workpool.WorkPool useContainerProxy bool drainWait time.Duration @@ -82,9 +83,10 @@ func WithSidecarRootfs( } } -func WithDeclarativeHealthchecks() Option { +func WithDeclarativeHealthchecks(timeout time.Duration) Option { return func(t *transformer) { t.useDeclarativeHealthCheck = true + t.declarativeHealthCheckDefaultTimeout = timeout } } @@ -377,7 +379,6 @@ func (t *transformer) StepsRunner( gardenContainer garden.Container, logStreamer log_streamer.LogStreamer, config Config, - // ) (ifrit.Runner, chan steps.ReadinessState, error) { ) (ifrit.Runner, chan steps.ReadinessState, error) { var setup, action, postSetup, monitor, readinessMonitor, longLivedAction ifrit.Runner var substeps []ifrit.Runner @@ -820,9 +821,14 @@ func (t *transformer) transformReadinessCheckDefinition( } func (t *transformer) applyCheckDefaults(timeout int, interval time.Duration, path string) (int, time.Duration, string) { - if timeout == 0 { + if timeout <= 0 { + timeout = int(t.declarativeHealthCheckDefaultTimeout / time.Millisecond) + } + if timeout <= 0 { + // fallback to 1000 if still invalid timeout = DefaultDeclarativeHealthcheckRequestTimeout } + if path == "" { path = "/" } diff --git a/depot/transformer/transformer_test.go b/depot/transformer/transformer_test.go index 2ab7c118..25d7e3b4 100644 --- a/depot/transformer/transformer_test.go +++ b/depot/transformer/transformer_test.go @@ -642,7 +642,8 @@ var _ = Describe("Transformer", func() { Context("when declarative healthchecks are enabled", func() { BeforeEach(func() { - options = append(options, transformer.WithDeclarativeHealthchecks()) + declarativeHealthCheckTimeout := 42 * time.Second + options = append(options, transformer.WithDeclarativeHealthchecks(declarativeHealthCheckTimeout)) container.StartTimeoutMs = 1000 }) @@ -848,7 +849,7 @@ var _ = Describe("Transformer", func() { Path: filepath.Join(transformer.HealthCheckDstPath, "healthcheck"), Args: []string{ "-port=8989", - "-timeout=1000ms", + "-timeout=42000ms", "-uri=/", fmt.Sprintf("-until-ready-interval=%s", unhealthyMonitoringInterval), }, @@ -1099,12 +1100,12 @@ var _ = Describe("Transformer", func() { Expect(paths).To(ContainElement(filepath.Join(transformer.HealthCheckDstPath, "healthcheck"))) Expect(args).To(ContainElement([]string{ "-port=5432", - "-timeout=1000ms", + "-timeout=42000ms", "-until-ready-interval=1ms", })) Expect(args).To(ContainElement([]string{ "-port=5432", - "-timeout=1000ms", + "-timeout=42000ms", "-readiness-interval=1s", })) }) @@ -1400,7 +1401,7 @@ var _ = Describe("Transformer", func() { Expect(paths).To(ContainElement(filepath.Join(transformer.HealthCheckDstPath, "healthcheck"))) Expect(args).To(ContainElement([]string{ "-port=6432", - "-timeout=1000ms", + "-timeout=42000ms", "-uri=/", "-startup-interval=1ms", "-startup-timeout=1s", @@ -1640,7 +1641,7 @@ var _ = Describe("Transformer", func() { Expect(paths).To(ContainElement(filepath.Join(transformer.HealthCheckDstPath, "healthcheck"))) Expect(args).To(ContainElement([]string{ "-port=6432", - "-timeout=1000ms", + "-timeout=42000ms", "-uri=/", "-liveness-interval=1s", })) @@ -1745,7 +1746,7 @@ var _ = Describe("Transformer", func() { Expect(paths).To(ContainElement(filepath.Join(transformer.HealthCheckDstPath, "healthcheck"))) Expect(args).To(ContainElement([]string{ "-port=6432", - "-timeout=1000ms", + "-timeout=42000ms", "-startup-interval=1ms", "-startup-timeout=1s", })) @@ -1829,7 +1830,7 @@ var _ = Describe("Transformer", func() { Expect(paths).To(ContainElement(filepath.Join(transformer.HealthCheckDstPath, "healthcheck"))) Expect(args).To(ContainElement([]string{ "-port=6432", - "-timeout=1000ms", + "-timeout=42000ms", "-liveness-interval=1s", })) }) diff --git a/initializer/initializer.go b/initializer/initializer.go index 14052e35..522c50c6 100644 --- a/initializer/initializer.go +++ b/initializer/initializer.go @@ -104,6 +104,7 @@ type ExecutorConfig struct { EnableContainerProxy bool `json:"enable_container_proxy,omitempty"` EnableContainerProxyHealthChecks bool `json:"enable_container_proxy_healthcheck,omitempty"` EnableDeclarativeHealthcheck bool `json:"enable_declarative_healthcheck,omitempty"` + DeclarativeHealthCheckDefaultTimeout durationjson.Duration `json:"declarative_healthcheck_default_timeout,omitempty"` EnableHealtcheckMetrics bool `json:"enable_healthcheck_metrics,omitempty"` EnableUnproxiedPortMappings bool `json:"enable_unproxied_port_mappings"` EnvoyConfigRefreshDelay durationjson.Duration `json:"envoy_config_refresh_delay"` @@ -268,6 +269,7 @@ func Initialize( time.Duration(config.EnvoyDrainTimeout), config.EnableContainerProxyHealthChecks, time.Duration(config.ProxyHealthCheckInterval), + time.Duration(config.DeclarativeHealthCheckDefaultTimeout), ) hub := event.NewHub() @@ -574,6 +576,8 @@ func initializeTransformer( drainWait time.Duration, enableProxyHealthChecks bool, proxyHealthCheckInterval time.Duration, + declarativeHealthCheckDefaultTimeout time.Duration, + ) transformer.Transformer { var options []transformer.Option compressor := compressor.NewTgz() @@ -581,7 +585,7 @@ func initializeTransformer( options = append(options, transformer.WithSidecarRootfs(declarativeHealthcheckRootFS)) if useDeclarativeHealthCheck { - options = append(options, transformer.WithDeclarativeHealthchecks()) + options = append(options, transformer.WithDeclarativeHealthchecks(declarativeHealthCheckDefaultTimeout)) } if emitHealthCheckMetrics { From 4f1eeb36b6cf6fdff8c1df7fc9a7015756e6181c Mon Sep 17 00:00:00 2001 From: Plamen Bardarov Date: Fri, 23 May 2025 10:05:47 +0300 Subject: [PATCH 2/3] add test case for when the default timeout is not in the config(<=0) --- depot/transformer/transformer_test.go | 31 +++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/depot/transformer/transformer_test.go b/depot/transformer/transformer_test.go index 25d7e3b4..3adc4afa 100644 --- a/depot/transformer/transformer_test.go +++ b/depot/transformer/transformer_test.go @@ -1,6 +1,7 @@ package transformer_test import ( + "code.cloudfoundry.org/durationjson" "errors" "fmt" "io" @@ -1407,6 +1408,36 @@ var _ = Describe("Transformer", func() { "-startup-timeout=1s", })) }) + + Context("and the default declarative healthcheck timeout is not set in the spec", func() { + BeforeEach(func() { + var emptyJsonTime durationjson.Duration + declarativeHealthCheckTimeout := time.Duration(emptyJsonTime) + + // This option will override the previously configured default timeout + options = append(options, transformer.WithDeclarativeHealthchecks(declarativeHealthCheckTimeout)) + }) + + It("uses the hardcoded 1s default for the timeout", func() { + Eventually(gardenContainer.RunCallCount).Should(Equal(2)) + paths := []string{} + args := [][]string{} + for i := 0; i < gardenContainer.RunCallCount(); i++ { + spec, _ := gardenContainer.RunArgsForCall(i) + paths = append(paths, spec.Path) + args = append(args, spec.Args) + } + + Expect(paths).To(ContainElement(filepath.Join(transformer.HealthCheckDstPath, "healthcheck"))) + Expect(args).To(ContainElement([]string{ + "-port=6432", + "-timeout=1000ms", + "-uri=/", + "-startup-interval=1ms", + "-startup-timeout=1s", + })) + }) + }) }) It("uses the check definition", func() { From 7e3b5e33b104b653eb8dd6b1c9813f421c3d6a2f Mon Sep 17 00:00:00 2001 From: Plamen Bardarov Date: Thu, 29 May 2025 10:43:14 +0300 Subject: [PATCH 3/3] add validation for `enable_declarative_healthcheck` add tests for negative or zero values for `enable_declarative_healthcheck` move default timeout setting to initialization --- depot/transformer/transformer.go | 10 ++++---- depot/transformer/transformer_test.go | 35 ++++++++++++++++++++++++--- initializer/initializer.go | 7 +++++- 3 files changed, 43 insertions(+), 9 deletions(-) diff --git a/depot/transformer/transformer.go b/depot/transformer/transformer.go index fb43b54d..366f4fe1 100644 --- a/depot/transformer/transformer.go +++ b/depot/transformer/transformer.go @@ -83,9 +83,13 @@ func WithSidecarRootfs( } } -func WithDeclarativeHealthchecks(timeout time.Duration) Option { +func WithDeclarativeHealthChecks(timeout time.Duration) Option { return func(t *transformer) { t.useDeclarativeHealthCheck = true + + if timeout <= 0 { + timeout = time.Duration(DefaultDeclarativeHealthcheckRequestTimeout) * time.Millisecond + } t.declarativeHealthCheckDefaultTimeout = timeout } } @@ -824,10 +828,6 @@ func (t *transformer) applyCheckDefaults(timeout int, interval time.Duration, pa if timeout <= 0 { timeout = int(t.declarativeHealthCheckDefaultTimeout / time.Millisecond) } - if timeout <= 0 { - // fallback to 1000 if still invalid - timeout = DefaultDeclarativeHealthcheckRequestTimeout - } if path == "" { path = "/" diff --git a/depot/transformer/transformer_test.go b/depot/transformer/transformer_test.go index 3adc4afa..fa6abd61 100644 --- a/depot/transformer/transformer_test.go +++ b/depot/transformer/transformer_test.go @@ -644,7 +644,7 @@ var _ = Describe("Transformer", func() { Context("when declarative healthchecks are enabled", func() { BeforeEach(func() { declarativeHealthCheckTimeout := 42 * time.Second - options = append(options, transformer.WithDeclarativeHealthchecks(declarativeHealthCheckTimeout)) + options = append(options, transformer.WithDeclarativeHealthChecks(declarativeHealthCheckTimeout)) container.StartTimeoutMs = 1000 }) @@ -1415,10 +1415,39 @@ var _ = Describe("Transformer", func() { declarativeHealthCheckTimeout := time.Duration(emptyJsonTime) // This option will override the previously configured default timeout - options = append(options, transformer.WithDeclarativeHealthchecks(declarativeHealthCheckTimeout)) + options = append(options, transformer.WithDeclarativeHealthChecks(declarativeHealthCheckTimeout)) }) - It("uses the hardcoded 1s default for the timeout", func() { + It("uses the 1s default for the timeout", func() { + Eventually(gardenContainer.RunCallCount).Should(Equal(2)) + paths := []string{} + args := [][]string{} + for i := 0; i < gardenContainer.RunCallCount(); i++ { + spec, _ := gardenContainer.RunArgsForCall(i) + paths = append(paths, spec.Path) + args = append(args, spec.Args) + } + + Expect(paths).To(ContainElement(filepath.Join(transformer.HealthCheckDstPath, "healthcheck"))) + Expect(args).To(ContainElement([]string{ + "-port=6432", + "-timeout=1000ms", + "-uri=/", + "-startup-interval=1ms", + "-startup-timeout=1s", + })) + }) + }) + + Context("and the declarative healthcheck timeout is set to a value <= 0", func() { + BeforeEach(func() { + declarativeHealthCheckTimeout, _ := time.ParseDuration("-24s") + + // This option will override the previously configured default timeout + options = append(options, transformer.WithDeclarativeHealthChecks(declarativeHealthCheckTimeout)) + }) + + It("uses the 1s default for the timeout", func() { Eventually(gardenContainer.RunCallCount).Should(Equal(2)) paths := []string{} args := [][]string{} diff --git a/initializer/initializer.go b/initializer/initializer.go index 522c50c6..30e90e1e 100644 --- a/initializer/initializer.go +++ b/initializer/initializer.go @@ -585,7 +585,7 @@ func initializeTransformer( options = append(options, transformer.WithSidecarRootfs(declarativeHealthcheckRootFS)) if useDeclarativeHealthCheck { - options = append(options, transformer.WithDeclarativeHealthchecks(declarativeHealthCheckDefaultTimeout)) + options = append(options, transformer.WithDeclarativeHealthChecks(declarativeHealthCheckDefaultTimeout)) } if emitHealthCheckMetrics { @@ -771,6 +771,11 @@ func (config *ExecutorConfig) Validate(logger lager.Logger) bool { valid = false } + if config.EnableDeclarativeHealthcheck && time.Duration(config.DeclarativeHealthCheckDefaultTimeout) <= 0 { + logger.Error("declarative_healthcheck_default_timeout", nil) + valid = false + } + return valid }