[v4.9-rhel] libpod: do not leak systemd hc startup unit timer

This fixes a regression added in commit 4fd8419, because the name was
overwritten by the createTimer() timer call the removeTransientFiles()
call removed the new timer and not the startup healthcheck. And then
when the container was stopped we leaked it as the wrong unit name was
in the state.

A new test has been added to ensure the logic works and we never leak
the system timers.

Fixes containers#22884

Signed-off-by: Paul Holzinger <[email protected]>
Signed-off-by: tomsweeneyredhat <[email protected]>

libpod/container_internal.go

@@ -262,7 +262,9 @@ func (c *Container) handleRestartPolicy(ctx context.Context) (_ bool, retErr err
 	}
 
 	if c.config.HealthCheckConfig != nil {
-		if err := c.removeTransientFiles(ctx, c.config.StartupHealthCheckConfig != nil && !c.state.StartupHCPassed); err != nil {
+		if err := c.removeTransientFiles(ctx,
+			c.config.StartupHealthCheckConfig != nil && !c.state.StartupHCPassed,
+			c.state.HCUnitName); err != nil {
 			return false, err
 		}
 	}
@@ -1558,7 +1560,9 @@ func (c *Container) restartWithTimeout(ctx context.Context, timeout uint) (retEr
 		}
 
 		if c.config.HealthCheckConfig != nil {
-			if err := c.removeTransientFiles(context.Background(), c.config.StartupHealthCheckConfig != nil && !c.state.StartupHCPassed); err != nil {
+			if err := c.removeTransientFiles(context.Background(),
+				c.config.StartupHealthCheckConfig != nil && !c.state.StartupHCPassed,
+				c.state.HCUnitName); err != nil {
 				logrus.Error(err.Error())
 			}
 		}
@@ -2055,7 +2059,9 @@ func (c *Container) cleanup(ctx context.Context) error {
 
 	// Remove healthcheck unit/timer file if it execs
 	if c.config.HealthCheckConfig != nil {
-		if err := c.removeTransientFiles(ctx, c.config.StartupHealthCheckConfig != nil && !c.state.StartupHCPassed); err != nil {
+		if err := c.removeTransientFiles(ctx,
+			c.config.StartupHealthCheckConfig != nil && !c.state.StartupHCPassed,
+			c.state.HCUnitName); err != nil {
 			logrus.Errorf("Removing timer for container %s healthcheck: %v", c.ID(), err)
 		}
 	}

libpod/healthcheck.go

@@ -277,6 +277,7 @@ func (c *Container) incrementStartupHCSuccessCounter(ctx context.Context) {
 	if recreateTimer {
 		logrus.Infof("Startup healthcheck for container %s passed, recreating timer", c.ID())
 
+		oldUnit := c.state.HCUnitName
 		// Create the new, standard healthcheck timer first.
 		if err := c.createTimer(c.HealthCheckConfig().Interval.String(), false); err != nil {
 			logrus.Errorf("Error recreating container %s healthcheck: %v", c.ID(), err)
@@ -290,7 +291,7 @@ func (c *Container) incrementStartupHCSuccessCounter(ctx context.Context) {
 		// Which happens to be us.
 		// So this has to be last - after this, systemd serves us a
 		// SIGTERM and we exit.
-		if err := c.removeTransientFiles(ctx, true); err != nil {
+		if err := c.removeTransientFiles(ctx, true, oldUnit); err != nil {
 			logrus.Errorf("Error removing container %s healthcheck: %v", c.ID(), err)
 			return
 		}

libpod/healthcheck_linux.go

@@ -109,7 +109,7 @@ func (c *Container) startTimer(isStartup bool) error {
 
 // removeTransientFiles removes the systemd timer and unit files
 // for the container
-func (c *Container) removeTransientFiles(ctx context.Context, isStartup bool) error {
+func (c *Container) removeTransientFiles(ctx context.Context, isStartup bool, unitName string) error {
 	if c.disableHealthCheckSystemd(isStartup) {
 		return nil
 	}
@@ -123,7 +123,6 @@ func (c *Container) removeTransientFiles(ctx context.Context, isStartup bool) er
 	// clean up as much as possible.
 	stopErrors := []error{}
 
-	unitName := c.state.HCUnitName
 	if unitName == "" {
 		unitName = c.hcUnitName(isStartup, true)
 	}

libpod/healthcheck_nosystemd_linux.go

@@ -19,6 +19,6 @@ func (c *Container) startTimer(isStartup bool) error {
 
 // removeTransientFiles removes the systemd timer and unit files
 // for the container
-func (c *Container) removeTransientFiles(ctx context.Context, isStartup bool) error {
+func (c *Container) removeTransientFiles(ctx context.Context, isStartup bool, unitName string) error {
 	return nil
 }

libpod/healthcheck_unsupported.go

@@ -19,6 +19,6 @@ func (c *Container) startTimer(isStartup bool) error {
 
 // removeTransientFiles removes the systemd timer and unit files
 // for the container
-func (c *Container) removeTransientFiles(ctx context.Context, isStartup bool) error {
+func (c *Container) removeTransientFiles(ctx context.Context, isStartup bool, unitName string) error {
 	return nil
 }

test/system/220-healthcheck.bats

@@ -5,6 +5,7 @@
 #
 
 load helpers
+load helpers.systemd
 
 
 # Helper function: run 'podman inspect' and check various given fields
@@ -34,14 +35,15 @@ function _check_health {
 
 @test "podman healthcheck" {
     _build_health_check_image healthcheck_i
-
-    # Run that healthcheck image.
-    run_podman run -d --name healthcheck_c \
-               --health-cmd /healthcheck   \
-               --health-interval 1s        \
-               --health-retries 3          \
-               --health-on-failure=kill    \
+    run_podman run -d --name healthcheck_c             \
+               --health-cmd /healthcheck               \
+               --health-interval 1s                    \
+               --health-retries 3                      \
+               --health-on-failure=kill                \
+               --health-startup-cmd /healthcheck       \
+               --health-startup-interval 1s            \
                healthcheck_i
+    cid="$output"
 
     run_podman inspect healthcheck_c --format "{{.Config.HealthcheckOnFailureAction}}"
     is "$output" "kill" "on-failure action is set to kill"
@@ -73,6 +75,10 @@ Log[-1].ExitCode | 1
 Log[-1].Output   | \"Uh-oh on stdout!\\\nUh-oh on stderr!\"
 " "$current_time" "healthy"
 
+    # Check that we now we do have valid podman units with this
+    # name so that the leak check below does not turn into a NOP without noticing.
+    assert "$(systemctl list-units --type timer | grep $cid)" =~ "podman" "Healthcheck systemd unit exists"
+
     current_time=$(date --iso-8601=seconds)
     # After three successive failures, container should no longer be healthy
     sleep 5
@@ -89,6 +95,10 @@ Log[-1].Output   | \"Uh-oh on stdout!\\\nUh-oh on stderr!\"
     # Clean up
     run_podman rm -t 0 -f healthcheck_c
     run_podman rmi   healthcheck_i
+
+    # Important check for https://github.com/containers/podman/issues/22884
+    # We never should leak the unit files, healthcheck uses the cid in name so just grep that.
+    assert "$(systemctl list-units --type timer | grep $cid)" == "" "Healthcheck systemd unit cleanup"
 }
 
 @test "podman healthcheck - restart cleans up old state" {