From 8cd9f8351d814ba950e5d5648de70de8392da097 Mon Sep 17 00:00:00 2001 From: Edoardo Tenani Date: Thu, 3 Feb 2022 16:30:48 +0100 Subject: [PATCH 1/6] add timeout to WaitForHealthy `WaitForHealthy()` does not implement any logic to timeout if the healthy condition is never met. By adding a timeout we make sure this function ends if the expected condition cannot be met in a certain (known) time frame. --- internal/compose/compose.go | 74 +++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 35 deletions(-) diff --git a/internal/compose/compose.go b/internal/compose/compose.go index 7ed0bc92f..47566f88f 100644 --- a/internal/compose/compose.go +++ b/internal/compose/compose.go @@ -275,54 +275,58 @@ func (p *Project) WaitForHealthy(opts CommandOptions) error { return err } + timeout := time.After(10 * time.Minute) + ticker := time.NewTicker(1 * time.Second) + containerIDs := strings.Split(strings.TrimSpace(b.String()), "\n") for { if signal.SIGINT() { return errors.New("SIGINT: cancel waiting for policy assigned") } - healthy := true - - logger.Debugf("Wait for healthy containers: %s", strings.Join(containerIDs, ",")) - descriptions, err := docker.InspectContainers(containerIDs...) - if err != nil { - return err - } + select { + case <-timeout: + return errors.New("time out waiting for healthy container") + case <-ticker.C: + healthy := true - for _, containerDescription := range descriptions { - logger.Debugf("Container status: %s", containerDescription.String()) - - // No healthcheck defined for service - if containerDescription.State.Status == "running" && containerDescription.State.Health == nil { - continue + logger.Debugf("Wait for healthy containers: %s", strings.Join(containerIDs, ",")) + descriptions, err := docker.InspectContainers(containerIDs...) + if err != nil { + return err } - - // Service is up and running and it's healthy - if containerDescription.State.Status == "running" && containerDescription.State.Health.Status == "healthy" { - continue + for _, containerDescription := range descriptions { + logger.Debugf("Container status: %s", containerDescription.String()) + + // No healthcheck defined for service + if containerDescription.State.Status == "running" && containerDescription.State.Health == nil { + continue + } + + // Service is up and running and it's healthy + if containerDescription.State.Status == "running" && containerDescription.State.Health.Status == "healthy" { + continue + } + + // Container started and finished with exit code 0 + if containerDescription.State.Status == "exited" && containerDescription.State.ExitCode == 0 { + continue + } + + // Container exited with code > 0 + if containerDescription.State.Status == "exited" && containerDescription.State.ExitCode > 0 { + return fmt.Errorf("container (ID: %s) exited with code %d", containerDescription.ID, containerDescription.State.ExitCode) + } + + // Any different status is considered unhealthy + healthy = false } - // Container started and finished with exit code 0 - if containerDescription.State.Status == "exited" && containerDescription.State.ExitCode == 0 { - continue + if healthy { + return nil } - - // Container exited with code > 0 - if containerDescription.State.Status == "exited" && containerDescription.State.ExitCode > 0 { - return fmt.Errorf("container (ID: %s) exited with code %d", containerDescription.ID, containerDescription.State.ExitCode) - } - - // Any different status is considered unhealthy - healthy = false - } - - if healthy { - break } - - time.Sleep(time.Second) } - return nil } func (p *Project) baseArgs() []string { From 0cb68922cdbb9f3882ef10f6318e3361156ac73d Mon Sep 17 00:00:00 2001 From: Edoardo Tenani Date: Mon, 7 Feb 2022 16:04:07 +0100 Subject: [PATCH 2/6] refactor removing time.Ticker --- internal/compose/compose.go | 86 ++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 39 deletions(-) diff --git a/internal/compose/compose.go b/internal/compose/compose.go index 47566f88f..db446fe81 100644 --- a/internal/compose/compose.go +++ b/internal/compose/compose.go @@ -23,6 +23,8 @@ import ( "github.com/elastic/elastic-package/internal/signal" ) +const waitForHealthyTimeout = 10 * time.Minute + // Project represents a Docker Compose project. type Project struct { name string @@ -275,58 +277,64 @@ func (p *Project) WaitForHealthy(opts CommandOptions) error { return err } - timeout := time.After(10 * time.Minute) - ticker := time.NewTicker(1 * time.Second) + timeout := time.Duration(waitForHealthyTimeout) + startTime := time.Now() + timeoutTime := startTime.Add(timeout) + interval := time.Duration(1 * time.Second) + healthy := true containerIDs := strings.Split(strings.TrimSpace(b.String()), "\n") - for { + for time.Now().Before(timeoutTime) { if signal.SIGINT() { return errors.New("SIGINT: cancel waiting for policy assigned") } - select { - case <-timeout: - return errors.New("time out waiting for healthy container") - case <-ticker.C: - healthy := true + logger.Debugf("Wait for healthy containers: %s", strings.Join(containerIDs, ",")) + descriptions, err := docker.InspectContainers(containerIDs...) + if err != nil { + return err + } + for _, containerDescription := range descriptions { + logger.Debugf("Container status: %s", containerDescription.String()) - logger.Debugf("Wait for healthy containers: %s", strings.Join(containerIDs, ",")) - descriptions, err := docker.InspectContainers(containerIDs...) - if err != nil { - return err + // No healthcheck defined for service + if containerDescription.State.Status == "running" && containerDescription.State.Health == nil { + continue } - for _, containerDescription := range descriptions { - logger.Debugf("Container status: %s", containerDescription.String()) - - // No healthcheck defined for service - if containerDescription.State.Status == "running" && containerDescription.State.Health == nil { - continue - } - - // Service is up and running and it's healthy - if containerDescription.State.Status == "running" && containerDescription.State.Health.Status == "healthy" { - continue - } - - // Container started and finished with exit code 0 - if containerDescription.State.Status == "exited" && containerDescription.State.ExitCode == 0 { - continue - } - - // Container exited with code > 0 - if containerDescription.State.Status == "exited" && containerDescription.State.ExitCode > 0 { - return fmt.Errorf("container (ID: %s) exited with code %d", containerDescription.ID, containerDescription.State.ExitCode) - } - - // Any different status is considered unhealthy - healthy = false + + // Service is up and running and it's healthy + if containerDescription.State.Status == "running" && containerDescription.State.Health.Status == "healthy" { + continue } - if healthy { - return nil + // Container started and finished with exit code 0 + if containerDescription.State.Status == "exited" && containerDescription.State.ExitCode == 0 { + continue } + + // Container exited with code > 0 + if containerDescription.State.Status == "exited" && containerDescription.State.ExitCode > 0 { + return fmt.Errorf("container (ID: %s) exited with code %d", containerDescription.ID, containerDescription.State.ExitCode) + } + + // Any different status is considered unhealthy + healthy = false } + + // end loop before timeout if healthy + if healthy { + break + } + + // NOTE: using sleep does not guarantee interval but it's ok for this use case + time.Sleep(interval) } + + if !healthy { + return errors.New("timeout waiting for healthy container") + } + + return nil } func (p *Project) baseArgs() []string { From 36c410add355531964a5f8b55229906bc8cd9afa Mon Sep 17 00:00:00 2001 From: Edoardo Tenani Date: Tue, 8 Feb 2022 11:40:17 +0100 Subject: [PATCH 3/6] refactor waitForHealthyTimeout --- internal/compose/compose.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/compose/compose.go b/internal/compose/compose.go index db446fe81..65b27d247 100644 --- a/internal/compose/compose.go +++ b/internal/compose/compose.go @@ -23,7 +23,8 @@ import ( "github.com/elastic/elastic-package/internal/signal" ) -const waitForHealthyTimeout = 10 * time.Minute +// waitForHealthyTimeout is the maximum duration for WaitForHealthy(). +var waitForHealthyTimeout = time.Duration(10 * time.Minute) // Project represents a Docker Compose project. type Project struct { @@ -277,14 +278,13 @@ func (p *Project) WaitForHealthy(opts CommandOptions) error { return err } - timeout := time.Duration(waitForHealthyTimeout) startTime := time.Now() - timeoutTime := startTime.Add(timeout) + timeout := startTime.Add(waitForHealthyTimeout) interval := time.Duration(1 * time.Second) healthy := true containerIDs := strings.Split(strings.TrimSpace(b.String()), "\n") - for time.Now().Before(timeoutTime) { + for time.Now().Before(timeout) { if signal.SIGINT() { return errors.New("SIGINT: cancel waiting for policy assigned") } From 960acdd218e714d1760727fe70b8589f15b06179 Mon Sep 17 00:00:00 2001 From: Edoardo Tenani Date: Tue, 8 Feb 2022 11:35:43 +0100 Subject: [PATCH 4/6] fix healthy bug --- internal/compose/compose.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/compose/compose.go b/internal/compose/compose.go index 65b27d247..f3fefd977 100644 --- a/internal/compose/compose.go +++ b/internal/compose/compose.go @@ -281,7 +281,6 @@ func (p *Project) WaitForHealthy(opts CommandOptions) error { startTime := time.Now() timeout := startTime.Add(waitForHealthyTimeout) interval := time.Duration(1 * time.Second) - healthy := true containerIDs := strings.Split(strings.TrimSpace(b.String()), "\n") for time.Now().Before(timeout) { @@ -289,6 +288,9 @@ func (p *Project) WaitForHealthy(opts CommandOptions) error { return errors.New("SIGINT: cancel waiting for policy assigned") } + // NOTE: healthy must be reinitialized at each iteration + healthy := true + logger.Debugf("Wait for healthy containers: %s", strings.Join(containerIDs, ",")) descriptions, err := docker.InspectContainers(containerIDs...) if err != nil { @@ -330,7 +332,7 @@ func (p *Project) WaitForHealthy(opts CommandOptions) error { time.Sleep(interval) } - if !healthy { + if time.Now().After(timeout) { return errors.New("timeout waiting for healthy container") } From 24ac388d9e1ee29d2148e5123328f0416d9690c0 Mon Sep 17 00:00:00 2001 From: Edoardo Tenani Date: Tue, 8 Feb 2022 11:46:48 +0100 Subject: [PATCH 5/6] refactor waitForHealthyInterval --- internal/compose/compose.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/compose/compose.go b/internal/compose/compose.go index f3fefd977..f22fa5c6f 100644 --- a/internal/compose/compose.go +++ b/internal/compose/compose.go @@ -26,6 +26,9 @@ import ( // waitForHealthyTimeout is the maximum duration for WaitForHealthy(). var waitForHealthyTimeout = time.Duration(10 * time.Minute) +// waitForHealthyInterval is the check interval for WaitForHealthy(). +const waitForHealthyInterval = time.Duration(1 * time.Second) + // Project represents a Docker Compose project. type Project struct { name string @@ -280,7 +283,6 @@ func (p *Project) WaitForHealthy(opts CommandOptions) error { startTime := time.Now() timeout := startTime.Add(waitForHealthyTimeout) - interval := time.Duration(1 * time.Second) containerIDs := strings.Split(strings.TrimSpace(b.String()), "\n") for time.Now().Before(timeout) { @@ -329,7 +331,7 @@ func (p *Project) WaitForHealthy(opts CommandOptions) error { } // NOTE: using sleep does not guarantee interval but it's ok for this use case - time.Sleep(interval) + time.Sleep(waitForHealthyInterval) } if time.Now().After(timeout) { From 18fccc71aa10e6d9d93fd778828af0790e12dcd8 Mon Sep 17 00:00:00 2001 From: Edoardo Tenani Date: Wed, 9 Feb 2022 10:46:49 +0100 Subject: [PATCH 6/6] code review suggestions --- internal/compose/compose.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/internal/compose/compose.go b/internal/compose/compose.go index f22fa5c6f..3cd329662 100644 --- a/internal/compose/compose.go +++ b/internal/compose/compose.go @@ -23,11 +23,12 @@ import ( "github.com/elastic/elastic-package/internal/signal" ) -// waitForHealthyTimeout is the maximum duration for WaitForHealthy(). -var waitForHealthyTimeout = time.Duration(10 * time.Minute) - -// waitForHealthyInterval is the check interval for WaitForHealthy(). -const waitForHealthyInterval = time.Duration(1 * time.Second) +const ( + // waitForHealthyTimeout is the maximum duration for WaitForHealthy(). + waitForHealthyTimeout = 10 * time.Minute + // waitForHealthyInterval is the check interval for WaitForHealthy(). + waitForHealthyInterval = 1 * time.Second +) // Project represents a Docker Compose project. type Project struct { @@ -285,7 +286,11 @@ func (p *Project) WaitForHealthy(opts CommandOptions) error { timeout := startTime.Add(waitForHealthyTimeout) containerIDs := strings.Split(strings.TrimSpace(b.String()), "\n") - for time.Now().Before(timeout) { + for { + if time.Now().After(timeout) { + return errors.New("timeout waiting for healthy container") + } + if signal.SIGINT() { return errors.New("SIGINT: cancel waiting for policy assigned") } @@ -298,6 +303,7 @@ func (p *Project) WaitForHealthy(opts CommandOptions) error { if err != nil { return err } + for _, containerDescription := range descriptions { logger.Debugf("Container status: %s", containerDescription.String()) @@ -334,10 +340,6 @@ func (p *Project) WaitForHealthy(opts CommandOptions) error { time.Sleep(waitForHealthyInterval) } - if time.Now().After(timeout) { - return errors.New("timeout waiting for healthy container") - } - return nil }