add healthcheck orchestration logic #4427

coderbirju · 2025-07-28T03:49:51Z

PR to add orchestration logic to nerdctl healthchecks

This PR uses systemd timers to orchestrate healthcheks in nerdctl without the need for a daemon process. Feature currently scoped for rootful linux systems.

coderbirju · 2025-08-27T19:04:18Z

@swagatbora90 @Shubhranshu153 @AkihiroSuda PTAL when you get a chance, open to improvements

AkihiroSuda · 2025-08-28T08:45:41Z

pkg/healthcheck/healthcheck_manager_linux.go

+	logrus.Debugf("Creating healthcheck timer unit: %s", hcName)
+
+	cmd := []string{}
+	if rootlessutil.IsRootless() {


The function seems to already support rootless.
Any reason to skip tests on rootless?

ChengyuZhu6 · 2025-08-28T10:40:19Z

pkg/containerutil/containerutil.go

+	// Clean up healthcheck units if configured.
+	// if err := healthcheck.RemoveTransientHealthCheckFiles(ctx, container); err != nil {
+	// 	return fmt.Errorf("failed to clean up healthcheck units for container %s", container.ID())
+	// }
+


What’s the purpose of this commented code?

ChengyuZhu6 · 2025-08-28T10:40:34Z

pkg/containerutil/containerutil.go

+	// Clean up healthcheck units if configured.
+	// if err := healthcheck.RemoveTransientHealthCheckFiles(ctx, container); err != nil {
+	// 	return fmt.Errorf("failed to clean up healthcheck units for container %s", container.ID())
+	// }
+


What’s the purpose of this commented code?

ChengyuZhu6 · 2025-08-28T10:47:06Z

go.mod

@@ -120,7 +120,7 @@ require (
 	github.com/santhosh-tekuri/jsonschema/v6 v6.0.1 // indirect
 	github.com/sasha-s/go-deadlock v0.3.5 // indirect
 	//gomodjail:unconfined
-	github.com/sirupsen/logrus v1.9.3 // indirect
+	github.com/sirupsen/logrus v1.9.3


Reason for this change?

Shubhranshu153 · 2025-08-29T00:33:43Z

cmd/nerdctl/container/container_health_check_linux_test.go

 	"github.com/containerd/nerdctl/v2/pkg/testutil"
 	"github.com/containerd/nerdctl/v2/pkg/testutil/nerdtest"
 )

 func TestContainerHealthCheckBasic(t *testing.T) {
+	if rootlessutil.IsRootless() {


any reason we cant support rootless?

Shubhranshu153 · 2025-08-29T00:34:24Z

cmd/nerdctl/container/container_health_check_linux_test.go

@@ -391,43 +399,6 @@ func TestContainerHealthCheckAdvance(t *testing.T) {
 				}
 			},
 		},
-		{


we needed to remove the test?

Shubhranshu153 · 2025-08-29T00:35:19Z

cmd/nerdctl/container/container_health_check_linux_test.go

+					"--health-interval", "2s",
+					testutil.CommonImage, "sleep", "30")
+				nerdtest.EnsureContainerStarted(helpers, data.Identifier())
+				// Wait for a healthcheck to execute


why we need to wait, ensure container started makes sure the container has started.

Shubhranshu153 · 2025-08-29T00:35:39Z

cmd/nerdctl/container/container_health_check_linux_test.go

+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				// Ensure proper cleanup of systemd units
+				helpers.Anyhow("stop", data.Identifier())
+				time.Sleep(500 * time.Millisecond) // Allow systemd cleanup


same here wait should not be needed when you are doing rm -f

Shubhranshu153 · 2025-08-29T00:39:04Z

cmd/nerdctl/container/container_health_check_linux_test.go

+			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
+				return &test.Expected{
+					ExitCode: 0,
+					Output: expect.All(func(stdout string, t tig.T) {


i think this portion needs a retry to for container to be in a healthy state.
Initially it should be starting and we can retry, if unhealthy we break with error, if healthy success.
And we retry a few times.

Shubhranshu153 · 2025-08-29T00:39:58Z

cmd/nerdctl/container/container_health_check_linux_test.go

+					"--health-interval", "1s",
+					testutil.CommonImage, "sleep", "30")
+				nerdtest.EnsureContainerStarted(helpers, data.Identifier())
+				time.Sleep(2 * time.Second)               // Wait for at least one health check to execute


same comment as above.

Shubhranshu153 · 2025-08-29T00:43:14Z

cmd/nerdctl/container/container_health_check_linux_test.go

+						containerEnd, err := time.Parse(time.RFC3339Nano, inspect.State.FinishedAt)
+						assert.NilError(t, err, "parsing container FinishedAt")
+
+						// Assert all healthcheck log start times are before container finished


can we check we remove the systemd timer on a kill signal?
Integ test should try to see the intended steady state condition in case of an event.

docs/healthchecks.md

Shubhranshu153 · 2025-08-29T00:49:04Z

docs/healthchecks.md


 Health checks can be configured in multiple ways:

-1. At container creation time using nerdctl run or nerdctl create with `--health-*` flags
+1. At container creation time using `nerdctl run` or `nerdctl create` with these flags:


we should specify if an option is not supported. Have we checked all options are supported? if not we cannot make the claims. Test should cover it, some may be part of the previous PR, but lets do a sanity check on what exists and what is not there.

pkg/api/types/container_types.go

Shubhranshu153 · 2025-08-29T01:03:30Z

pkg/cmd/container/kill.go

@@ -111,6 +112,11 @@ func killContainer(ctx context.Context, container containerd.Container, signal s
 		return err
 	}

+	// Clean up healthcheck systemd units
+	if err := healthcheck.RemoveTransientHealthCheckFiles(ctx, container); err != nil {


this seems like a risky function to be called in kill. Is there a possibility the function hangs?
It should also follow the pattern similar to the signal in question. Majorly SIGKILL and SIGTERM should try to be forced removal of the files.

Shubhranshu153 · 2025-08-29T01:05:24Z

pkg/cmd/container/remove.go

@@ -179,6 +180,11 @@ func RemoveContainer(ctx context.Context, c containerd.Container, globalOptions
 		// Otherwise, nil the error so that we do not write the error label on the container
 		retErr = nil

+		// Clean up healthcheck systemd units


same comment as above.

Shubhranshu153 · 2025-08-29T01:06:20Z

pkg/cmd/container/remove.go

@@ -179,6 +180,11 @@ func RemoveContainer(ctx context.Context, c containerd.Container, globalOptions
 		// Otherwise, nil the error so that we do not write the error label on the container
 		retErr = nil

+		// Clean up healthcheck systemd units


needs a integ test for remove case (kill is there but i didnt see remove and stop)

Shubhranshu153 · 2025-08-29T01:08:44Z

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,


would like to avoid logical assertion in assignment. It might confuse the actual intent of the operation

is the startPeriod tested?

Shubhranshu153 · 2025-08-29T01:11:05Z

pkg/healthcheck/executor.go

-			currentHealth.Status = Unhealthy
+
+	// Check if we're in start period workflow
+	inStartPeriodTime := hcResult.Start.Sub(containerCreated) < hcConfig.StartPeriod


would suggest to have unit test for this logic, seems to have a few cases

Shubhranshu153 · 2025-08-29T01:13:39Z

pkg/healthcheck/healthcheck_manager_linux.go

+}
+
+// StartTimer starts the healthcheck timer unit.
+// TODO if we persist hcName to container state, pass that to this function.


if we are adding TODO we can create a ticket for future follow up.

Shubhranshu153 · 2025-08-29T01:14:57Z

pkg/healthcheck/healthcheck_manager_linux.go

+// RemoveTransientHealthCheckFilesByID stops and cleans up the transient timer and service using just the container ID.
+func RemoveTransientHealthCheckFilesByID(ctx context.Context, containerID string) error {
+	// Don't proceed if systemd is unavailable or disabled
+	if !defaults.IsSystemdAvailable() || os.Getenv("DISABLE_HC_SYSTEMD") == "true" {


this is already checked in shouldSkipHealthCheckSystemd

Shubhranshu153 · 2025-08-29T01:16:55Z

pkg/healthcheck/healthcheck_manager_linux.go

+	}
+
+	// Reset failed units
+	_ = conn.ResetFailedUnitContext(context.Background(), service)


should we reset in all cases

Shubhranshu153 · 2025-08-29T01:17:25Z

pkg/healthcheck/healthcheck_manager_linux.go

+	// Stop timer
+	tChan := make(chan string)
+	if _, err := conn.StopUnitContext(context.Background(), timer, "ignore-dependencies", tChan); err == nil {
+		if msg := <-tChan; msg != "done" {


blocking calls in removing workflows is dangerous.

docs/healthchecks.md

swagatbora90 · 2025-08-28T18:22:42Z

cmd/nerdctl/container/container_health_check_linux_test.go

@@ -391,43 +399,6 @@ func TestContainerHealthCheckAdvance(t *testing.T) {
 				}
 			},
 		},
-		{
-			Description: "Healthcheck emits large output repeatedly",


why remove this test?

cmd/nerdctl/container/container_health_check_linux_test.go

cmd/nerdctl/container/container_run.go

Signed-off-by: Arjun Raja Yogidas <[email protected]>

coderbirju force-pushed the automate_health_checks_feat branch 15 times, most recently from a4676a1 to bba6059 Compare August 8, 2025 20:24

coderbirju force-pushed the automate_health_checks_feat branch from bba6059 to 97643eb Compare August 11, 2025 00:23

coderbirju force-pushed the automate_health_checks_feat branch 3 times, most recently from f4fc171 to f12e708 Compare August 27, 2025 18:49

coderbirju marked this pull request as ready for review August 27, 2025 19:03

AkihiroSuda added this to the v2.1.4 milestone Aug 28, 2025

AkihiroSuda reviewed Aug 28, 2025

View reviewed changes

ChengyuZhu6 reviewed Aug 28, 2025

View reviewed changes

Shubhranshu153 reviewed Aug 29, 2025

View reviewed changes

docs/healthchecks.md Outdated Show resolved Hide resolved

Shubhranshu153 reviewed Aug 29, 2025

View reviewed changes

docs/healthchecks.md Outdated Show resolved Hide resolved

Shubhranshu153 reviewed Aug 29, 2025

View reviewed changes

pkg/api/types/container_types.go Show resolved Hide resolved

Shubhranshu153 reviewed Aug 29, 2025

View reviewed changes

swagatbora90 reviewed Aug 29, 2025

View reviewed changes

coderbirju added 3 commits September 1, 2025 07:52

add healthcheck orchestration logic

993b7db

Signed-off-by: Arjun Raja Yogidas <[email protected]>

Update documentation

5eb08bc

Signed-off-by: Arjun Raja Yogidas <[email protected]>

Address PR comments

2a8f99d

Signed-off-by: Arjun Raja Yogidas <[email protected]>

coderbirju force-pushed the automate_health_checks_feat branch from f12e708 to 2a8f99d Compare September 1, 2025 07:56

@@ @@ -391,43 +399,6 @@ func TestContainerHealthCheckAdvance(t *testing.T) { @@
               				}
               			},
               		},
-              		{

add healthcheck orchestration logic #4427

Are you sure you want to change the base?

add healthcheck orchestration logic #4427

Conversation

coderbirju commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderbirju commented Aug 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderbirju commented Jul 28, 2025 •

edited

Loading