Update healthcheck orchestration logic

coderbirju · coderbirju · commit c5f711b89467 · 2025-07-26T02:17:59.000Z
Signed-off-by: Arjun Raja Yogidas &lt;arjunry@amazon.com&gt;
diff --git a/cmd/nerdctl/container/container_create.go b/cmd/nerdctl/container/container_create.go
@@ -279,10 +279,6 @@ func createOptions(cmd *cobra.Command) (types.ContainerCreateOptions, error) {
 	if err != nil {
 		return opt, err
 	}
-	opt.HealthStartInterval, err = cmd.Flags().GetDuration("health-start-interval")
-	if err != nil {
-		return opt, err
-	}
 	opt.NoHealthcheck, err = cmd.Flags().GetBool("no-healthcheck")
 	if err != nil {
 		return opt, err
diff --git a/cmd/nerdctl/container/container_run.go b/cmd/nerdctl/container/container_run.go
@@ -19,10 +19,11 @@ package container
 import (
 	"errors"
 	"fmt"
-	"github.com/containerd/nerdctl/v2/pkg/healthcheck"
 	"runtime"
 	"strings"
 
+	"github.com/containerd/nerdctl/v2/pkg/healthcheck"
+
 	"github.com/spf13/cobra"
 	"golang.org/x/term"
 
@@ -241,7 +242,6 @@ func setCreateFlags(cmd *cobra.Command) {
 	cmd.Flags().Duration("health-timeout", 0, "Maximum time to allow one check to run (default: 30s)")
 	cmd.Flags().Int("health-retries", 0, "Consecutive failures needed to report unhealthy (default: 3)")
 	cmd.Flags().Duration("health-start-period", 0, "Start period for the container to initialize before starting health-retries countdown")
-	cmd.Flags().Duration("health-start-interval", 0, "Time between running the checks during the start period")
 	cmd.Flags().Bool("no-healthcheck", false, "Disable any container-specified HEALTHCHECK")
 
 	// #region env flags
diff --git a/cmd/nerdctl/helpers/flagutil.go b/cmd/nerdctl/helpers/flagutil.go
@@ -52,8 +52,7 @@ func ValidateHealthcheckFlags(options types.ContainerCreateOptions) error {
 		options.HealthInterval != 0 ||
 			options.HealthTimeout != 0 ||
 			options.HealthRetries != 0 ||
-			options.HealthStartPeriod != 0 ||
-			options.HealthStartInterval != 0
+			options.HealthStartPeriod != 0
 
 	if options.NoHealthcheck {
 		if options.HealthCmd != "" || healthFlagsSet {
@@ -74,9 +73,6 @@ func ValidateHealthcheckFlags(options types.ContainerCreateOptions) error {
 	if options.HealthStartPeriod < 0 {
 		return fmt.Errorf("--health-start-period cannot be negative")
 	}
-	if options.HealthStartInterval < 0 {
-		return fmt.Errorf("--health-start-interval cannot be negative")
-	}
 	return nil
 }
 
diff --git a/pkg/api/types/container_types.go b/pkg/api/types/container_types.go
@@ -285,13 +285,12 @@ type ContainerCreateOptions struct {
 	ImagePullOpt ImagePullOptions
 
 	// Healthcheck related fields
-	HealthCmd           string
-	HealthInterval      time.Duration
-	HealthTimeout       time.Duration
-	HealthRetries       int
-	HealthStartPeriod   time.Duration
-	HealthStartInterval time.Duration
-	NoHealthcheck       bool
+	HealthCmd         string
+	HealthInterval    time.Duration
+	HealthTimeout     time.Duration
+	HealthRetries     int
+	HealthStartPeriod time.Duration
+	NoHealthcheck     bool
 
 	// UserNS name for user namespace mapping of container
 	UserNS string
diff --git a/pkg/cmd/container/create.go b/pkg/cmd/container/create.go
@@ -891,9 +891,6 @@ func withHealthcheck(options types.ContainerCreateOptions, ensuredImage *imgutil
 	if options.HealthStartPeriod != 0 {
 		hc.StartPeriod = options.HealthStartPeriod
 	}
-	if options.HealthStartInterval != 0 {
-		hc.StartInterval = options.HealthStartInterval
-	}
 
 	// If no healthcheck config is set (via CLI or image), return empty string so we skip adding to container config.
 	if reflect.DeepEqual(hc, &healthcheck.Healthcheck{}) {
diff --git a/pkg/cmd/container/health_check.go b/pkg/cmd/container/health_check.go
@@ -59,7 +59,6 @@ func HealthCheck(ctx context.Context, client *containerd.Client, container conta
 	hcConfig.Interval = timeoutWithDefault(hcConfig.Interval, healthcheck.DefaultProbeInterval)
 	hcConfig.Timeout = timeoutWithDefault(hcConfig.Timeout, healthcheck.DefaultProbeTimeout)
 	hcConfig.StartPeriod = timeoutWithDefault(hcConfig.StartPeriod, healthcheck.DefaultStartPeriod)
-	hcConfig.StartInterval = timeoutWithDefault(hcConfig.StartInterval, healthcheck.DefaultStartInterval)
 	if hcConfig.Retries == 0 {
 		hcConfig.Retries = healthcheck.DefaultProbeRetries
 	}
diff --git a/pkg/cmd/container/kill.go b/pkg/cmd/container/kill.go
@@ -35,6 +35,7 @@ import (
 	"github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/containerd/nerdctl/v2/pkg/clientutil"
 	"github.com/containerd/nerdctl/v2/pkg/containerutil"
+	"github.com/containerd/nerdctl/v2/pkg/healthcheck"
 	"github.com/containerd/nerdctl/v2/pkg/idutil/containerwalker"
 	"github.com/containerd/nerdctl/v2/pkg/labels"
 	"github.com/containerd/nerdctl/v2/pkg/netutil"
@@ -112,9 +113,9 @@ func killContainer(ctx context.Context, container containerd.Container, signal s
 	}
 
 	// Clean up healthcheck systemd units
-	// if err := healthcheck.RemoveTransientHealthCheckFiles(ctx, container); err != nil {
-	// 	log.G(ctx).Warnf("failed to clean up healthcheck units for container %s: %s", container.ID(), err)
-	// }
+	if err := healthcheck.RemoveTransientHealthCheckFiles(ctx, container); err != nil {
+		log.G(ctx).Warnf("failed to clean up healthcheck units for container %s: %s", container.ID(), err)
+	}
 
 	// signal will be sent once resume is finished
 	if paused {
diff --git a/pkg/cmd/container/remove.go b/pkg/cmd/container/remove.go
@@ -34,6 +34,7 @@ import (
 	"github.com/containerd/nerdctl/v2/pkg/clientutil"
 	"github.com/containerd/nerdctl/v2/pkg/containerutil"
 	"github.com/containerd/nerdctl/v2/pkg/dnsutil/hostsstore"
+	"github.com/containerd/nerdctl/v2/pkg/healthcheck"
 	"github.com/containerd/nerdctl/v2/pkg/idutil/containerwalker"
 	"github.com/containerd/nerdctl/v2/pkg/ipcutil"
 	"github.com/containerd/nerdctl/v2/pkg/labels"
@@ -180,9 +181,9 @@ func RemoveContainer(ctx context.Context, c containerd.Container, globalOptions
 		retErr = nil
 
 		// Clean up healthcheck systemd units
-		// if err := healthcheck.RemoveTransientHealthCheckFiles(ctx, c); err != nil {
-		// 	log.G(ctx).WithError(err).Warnf("failed to clean up healthcheck units for container %q", id)
-		// }
+		if err := healthcheck.RemoveTransientHealthCheckFiles(ctx, c); err != nil {
+			log.G(ctx).WithError(err).Warnf("failed to clean up healthcheck units for container %q", id)
+		}
 
 		// Now, delete the actual container
 		var delOpts []containerd.DeleteOpts
diff --git a/pkg/cmd/container/stop.go b/pkg/cmd/container/stop.go
@@ -25,6 +25,7 @@ import (
 
 	"github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/containerd/nerdctl/v2/pkg/containerutil"
+	"github.com/containerd/nerdctl/v2/pkg/healthcheck"
 	"github.com/containerd/nerdctl/v2/pkg/idutil/containerwalker"
 )
 
@@ -39,6 +40,9 @@ func Stop(ctx context.Context, client *containerd.Client, reqs []string, opt typ
 			if err := cleanupNetwork(ctx, found.Container, opt.GOptions); err != nil {
 				return fmt.Errorf("unable to cleanup network for container: %s", found.Req)
 			}
+			if err := healthcheck.RemoveTransientHealthCheckFiles(ctx, found.Container); err != nil {
+				return fmt.Errorf("unable to cleanup healthcheck timer for container: %s: %w", found.Req, err)
+			}
 			if err := containerutil.Stop(ctx, found.Container, opt.Timeout, opt.Signal); err != nil {
 				if errdefs.IsNotFound(err) {
 					fmt.Fprintf(opt.Stderr, "No such container: %s\n", found.Req)
diff --git a/pkg/healthcheck/executor.go b/pkg/healthcheck/executor.go
@@ -128,26 +128,42 @@ func updateHealthStatus(ctx context.Context, container containerd.Container, hcC
 		currentHealth = &HealthState{
 			Status:        Starting,
 			FailingStreak: 0,
+			StartPeriod:   hcConfig.StartPeriod > 0,
 		}
 	}
 
-	// Check if still within start period
-	startPeriod := hcConfig.StartPeriod
+	// Get container info for start period check
 	info, err := container.Info(ctx)
 	if err != nil {
 		return fmt.Errorf("failed to get container info: %w", err)
 	}
 	containerCreated := info.CreatedAt
-	stillInStartPeriod := hcResult.Start.Sub(containerCreated) < startPeriod
-
-	// Update health status based on exit code
-	if hcResult.ExitCode == 0 {
-		currentHealth.Status = Healthy
-		currentHealth.FailingStreak = 0
-	} else if !stillInStartPeriod {
-		currentHealth.FailingStreak++
-		if currentHealth.FailingStreak >= hcConfig.Retries {
-			currentHealth.Status = Unhealthy
+
+	// Check if we're in start period workflow
+	inStartPeriodTime := hcResult.Start.Sub(containerCreated) < hcConfig.StartPeriod
+	inStartPeriodState := currentHealth.StartPeriod
+
+	if inStartPeriodTime && inStartPeriodState {
+		// Start Period Workflow
+		if hcResult.ExitCode == 0 {
+			// First healthy result transitions us out of start period
+			currentHealth.Status = Healthy
+			currentHealth.FailingStreak = 0
+			currentHealth.StartPeriod = false
+		}
+		// Ignore unhealthy results during start period
+	} else {
+		// Health Interval Workflow
+		if hcResult.ExitCode == 0 {
+			if currentHealth.Status != Healthy {
+				currentHealth.Status = Healthy
+				currentHealth.FailingStreak = 0
+			}
+		} else {
+			currentHealth.FailingStreak++
+			if currentHealth.FailingStreak >= hcConfig.Retries && currentHealth.Status != Unhealthy {
+				currentHealth.Status = Unhealthy
+			}
 		}
 	}
 
diff --git a/pkg/healthcheck/health.go b/pkg/healthcheck/health.go
@@ -43,7 +43,6 @@ const (
 	DefaultProbeInterval   = 30 * time.Second // Default interval between probe runs. Also applies before the first probe.
 	DefaultProbeTimeout    = 30 * time.Second // Max duration a single probe run may take before it's considered failed.
 	DefaultStartPeriod     = 0 * time.Second  // Grace period for container startup before health checks count as failures.
-	DefaultStartInterval   = 5 * time.Second  // Interval between checks during the start period.
 	DefaultProbeRetries    = 3                // Number of consecutive failures before marking container as unhealthy.
 	MaxLogEntries          = 5                // Maximum number of health check log entries to keep.
 	MaxOutputLenForInspect = 4096             // Max output length (in bytes) stored in health check logs during inspect. Longer outputs are truncated.
@@ -70,18 +69,18 @@ type HealthcheckResult struct {
 
 // Healthcheck represents the health check configuration
 type Healthcheck struct {
-	Test          []string      `json:"Test,omitempty"`          // Test is the check to perform that the container is healthy
-	Interval      time.Duration `json:"Interval,omitempty"`      // Interval is the time to wait between checks
-	Timeout       time.Duration `json:"Timeout,omitempty"`       // Timeout is the time to wait before considering the check to have hung
-	Retries       int           `json:"Retries,omitempty"`       // Retries is the number of consecutive failures needed to consider a container as unhealthy
-	StartPeriod   time.Duration `json:"StartPeriod,omitempty"`   // StartPeriod is the period for the container to initialize before the health check starts
-	StartInterval time.Duration `json:"StartInterval,omitempty"` // StartInterval is the time between health checks during the start period
+	Test        []string      `json:"Test,omitempty"`        // Test is the check to perform that the container is healthy
+	Interval    time.Duration `json:"Interval,omitempty"`    // Interval is the time to wait between checks
+	Timeout     time.Duration `json:"Timeout,omitempty"`     // Timeout is the time to wait before considering the check to have hung
+	Retries     int           `json:"Retries,omitempty"`     // Retries is the number of consecutive failures needed to consider a container as unhealthy
+	StartPeriod time.Duration `json:"StartPeriod,omitempty"` // StartPeriod is the period for the container to initialize before the health check starts
 }
 
 // HealthState stores the current health state of a container
 type HealthState struct {
 	Status        HealthStatus // Status is one of [Starting], [Healthy] or [Unhealthy]
 	FailingStreak int          // FailingStreak is the number of consecutive failures
+	StartPeriod   bool         // StartPeriod indicates if we're in the start period workflow
 }
 
 // ToJSONString serializes HealthState to a JSON string for label storage
diff --git a/pkg/healthcheck/healthcheck_integration_test.go b/pkg/healthcheck/healthcheck_integration_test.go
@@ -14,24 +14,119 @@
    limitations under the License.
 */
 
-package healthcheck
+package healthcheck_test
 
 import (
+	"regexp"
+	"testing"
+	"time"
+
 	"github.com/containerd/nerdctl/mod/tigron/expect"
 	"github.com/containerd/nerdctl/mod/tigron/require"
 	"github.com/containerd/nerdctl/mod/tigron/test"
 	"github.com/containerd/nerdctl/v2/pkg/testutil"
 	"github.com/containerd/nerdctl/v2/pkg/testutil/nerdtest"
-	"gotest.tools/v3/assert"
-	"testing"
-	"time"
 )
 
-func TestHealthCheck_SystemdIntegration_Basic(t *testing.T) {
+func TestHealthCheck_SystemdIntegration_AllScenarios(t *testing.T) {
 	testCase := nerdtest.Setup()
 	testCase.Require = require.Not(nerdtest.Docker)
 
 	testCase.SubTests = []*test.Case{
+		{
+			Description: "Start period workflow - unhealthy results ignored during grace period",
+			Setup: func(data test.Data, helpers test.Helpers) {
+				helpers.Ensure("run", "-d", "--name", data.Identifier(),
+					"--health-cmd", "exit 1",
+					"--health-interval", "2s",
+					"--health-start-period", "10s",
+					testutil.CommonImage, "sleep", "30")
+				// Wait for a few health checks during start period
+				time.Sleep(6 * time.Second)
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				helpers.Anyhow("rm", "-f", data.Identifier())
+			},
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
+				return helpers.Command("inspect", data.Identifier())
+			},
+			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
+				return &test.Expected{
+					ExitCode: 0,
+					Output:   expect.Match(regexp.MustCompile(`.*"Status":"starting".*`)),
+				}
+			},
+		},
+		{
+			Description: "Health interval workflow - retries and failure",
+			Setup: func(data test.Data, helpers test.Helpers) {
+				helpers.Ensure("run", "-d", "--name", data.Identifier(),
+					"--health-cmd", "exit 1",
+					"--health-interval", "2s",
+					"--health-retries", "2",
+					testutil.CommonImage, "sleep", "30")
+				// Wait for retries to exceed limit
+				time.Sleep(6 * time.Second)
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				helpers.Anyhow("rm", "-f", data.Identifier())
+			},
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
+				return helpers.Command("inspect", data.Identifier())
+			},
+			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
+				return &test.Expected{
+					ExitCode: 0,
+					Output:   expect.Match(regexp.MustCompile(`.*"Status":"unhealthy".*`)),
+				}
+			},
+		},
+		{
+			Description: "Timer cleanup on container stop",
+			Setup: func(data test.Data, helpers test.Helpers) {
+				helpers.Ensure("run", "-d", "--name", data.Identifier(),
+					"--health-cmd", "exit 0",
+					"--health-interval", "2s",
+					testutil.CommonImage, "sleep", "30")
+				time.Sleep(3 * time.Second)
+				helpers.Ensure("stop", data.Identifier())
+			},
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
+				return helpers.Command("exec", "sh", "-c", "ls /run/systemd/system/"+data.Identifier()+".timer 2>/dev/null || echo 'timer removed'")
+			},
+			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
+				return &test.Expected{
+					ExitCode: 0,
+					Output:   expect.Contains("timer removed"),
+				}
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				helpers.Anyhow("rm", "-f", data.Identifier())
+			},
+		},
+		{
+			Description: "Pause/unpause behavior - timer preserved",
+			Setup: func(data test.Data, helpers test.Helpers) {
+				helpers.Ensure("run", "-d", "--name", data.Identifier(),
+					"--health-cmd", "exit 0",
+					"--health-interval", "2s",
+					testutil.CommonImage, "sleep", "30")
+				time.Sleep(3 * time.Second)
+				helpers.Ensure("pause", data.Identifier())
+			},
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
+				return helpers.Command("exec", "sh", "-c", "ls /run/systemd/system/"+data.Identifier()+".timer")
+			},
+			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
+				return &test.Expected{
+					ExitCode: 0,
+				}
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				helpers.Anyhow("unpause", data.Identifier())
+				helpers.Anyhow("rm", "-f", data.Identifier())
+			},
+		},
 		{
 			Description: "Basic healthy container with systemd-triggered healthcheck",
 			Setup: func(data test.Data, helpers test.Helpers) {
@@ -51,13 +146,7 @@ func TestHealthCheck_SystemdIntegration_Basic(t *testing.T) {
 			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
 				return &test.Expected{
 					ExitCode: 0,
-					Output: expect.All(func(stdout, _ string, t *testing.T) {
-						inspect := nerdtest.InspectContainer(helpers, data.Identifier())
-						h := inspect.State.Health
-						assert.Assert(t, h != nil, "expected health state to be present")
-						assert.Equal(t, h.Status, "healthy")
-						assert.Assert(t, len(h.Log) > 0, "expected at least one health check log entry")
-					}),
+					Output:   expect.Match(regexp.MustCompile(`.*"Status":"healthy".*`)),
 				}
 			},
 		},
diff --git a/pkg/healthcheck/healthcheck_manager_linux.go b/pkg/healthcheck/healthcheck_manager_linux.go

Original file line number	Diff line number	Diff line change
`@@ -891,9 +891,6 @@ func withHealthcheck(options types.ContainerCreateOptions, ensuredImage *imgutil`
`891`	`891`	`if options.HealthStartPeriod != 0 {`
`892`	`892`	`hc.StartPeriod = options.HealthStartPeriod`
`893`	`893`	`}`
`894`		`- if options.HealthStartInterval != 0 {`
`895`		`- hc.StartInterval = options.HealthStartInterval`
`896`		`- }`
`897`	`894`
`898`	`895`	`// If no healthcheck config is set (via CLI or image), return empty string so we skip adding to container config.`
`899`	`896`	`if reflect.DeepEqual(hc, &healthcheck.Healthcheck{}) {`
Original file line number	Diff line number	Diff line change
`@@ -59,7 +59,6 @@ func HealthCheck(ctx context.Context, client *containerd.Client, container conta`
`59`	`59`	`hcConfig.Interval = timeoutWithDefault(hcConfig.Interval, healthcheck.DefaultProbeInterval)`
`60`	`60`	`hcConfig.Timeout = timeoutWithDefault(hcConfig.Timeout, healthcheck.DefaultProbeTimeout)`
`61`	`61`	`hcConfig.StartPeriod = timeoutWithDefault(hcConfig.StartPeriod, healthcheck.DefaultStartPeriod)`
`62`		`- hcConfig.StartInterval = timeoutWithDefault(hcConfig.StartInterval, healthcheck.DefaultStartInterval)`
`63`	`62`	`if hcConfig.Retries == 0 {`
`64`	`63`	`hcConfig.Retries = healthcheck.DefaultProbeRetries`
`65`	`64`	`}`