From a2e80c53d485db1a159751a7d29eba20f2c8176c Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 17 Mar 2023 12:46:33 +0100 Subject: [PATCH] [v4.4.1-rhel] fix --health-on-failure=restart in transient unit As described in #17777, the `restart` on-failure action did not behave correctly when the health check is being run by a transient systemd unit. It ran just fine when being executed outside such a unit, for instance, manually or, as done in the system tests, in a scripted fashion. There were two issue causing the `restart` on-failure action to misbehave: 1) The transient systemd units used the default `KillMode=cgroup` which will nuke all processes in the specific cgroup including the recently restarted container/conmon once the main `podman healthcheck run` process exits. 2) Podman attempted to remove the transient systemd unit and timer during restart. That is perfectly fine when manually restarting the container but not when the restart itself is being executed inside such a transient unit. Ultimately, Podman tried to shoot itself in the foot. Fix both issues by moving the restart logic in the cleanup process. Instead of restarting the container, the `healthcheck run` will just stop the container and the cleanup process will restart the container once it has turned unhealthy. Backport of commit 95634154303f5b8c3d5c92820e2a3545c54f0bc8. Fixes: #17777 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2180125 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2180126 Signed-off-by: Valentin Rothberg --- libpod/container_internal.go | 16 +++++++++ libpod/healthcheck.go | 20 ++++++++++-- test/system/220-healthcheck.bats | 56 +++++++++++++++++++++++++++----- 3 files changed, 81 insertions(+), 11 deletions(-) diff --git a/libpod/container_internal.go b/libpod/container_internal.go index ee6a7b6cc2..f6e64594a3 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -228,6 +228,15 @@ func (c *Container) handleExitFile(exitFile string, fi os.FileInfo) error { } func (c *Container) shouldRestart() bool { + if c.config.HealthCheckOnFailureAction == define.HealthCheckOnFailureActionRestart { + isUnhealthy, err := c.isUnhealthy() + if err != nil { + logrus.Errorf("Checking if container is unhealthy: %v", err) + } else if isUnhealthy { + return true + } + } + // If we did not get a restart policy match, return false // Do the same if we're not a policy that restarts. if !c.state.RestartPolicyMatch || @@ -267,6 +276,12 @@ func (c *Container) handleRestartPolicy(ctx context.Context) (_ bool, retErr err return false, err } + if c.config.HealthCheckConfig != nil { + if err := c.removeTransientFiles(ctx, c.config.StartupHealthCheckConfig != nil && !c.state.StartupHCPassed); err != nil { + return false, err + } + } + // Is the container running again? // If so, we don't have to do anything if c.ensureState(define.ContainerStateRunning, define.ContainerStatePaused) { @@ -1431,6 +1446,7 @@ func (c *Container) restartWithTimeout(ctx context.Context, timeout uint) (retEr if err := c.stop(timeout); err != nil { return err } + if c.config.HealthCheckConfig != nil { if err := c.removeTransientFiles(context.Background(), c.config.StartupHealthCheckConfig != nil && !c.state.StartupHCPassed); err != nil { logrus.Error(err.Error()) diff --git a/libpod/healthcheck.go b/libpod/healthcheck.go index 63f22ef9fb..987b02d934 100644 --- a/libpod/healthcheck.go +++ b/libpod/healthcheck.go @@ -184,8 +184,12 @@ func (c *Container) processHealthCheckStatus(status string) error { } case define.HealthCheckOnFailureActionRestart: - if err := c.RestartWithTimeout(context.Background(), c.config.StopTimeout); err != nil { - return fmt.Errorf("restarting container after health-check turned unhealthy: %w", err) + // We let the cleanup process handle the restart. Otherwise + // the container would be restarted in the context of a + // transient systemd unit which may cause undesired side + // effects. + if err := c.Stop(); err != nil { + return fmt.Errorf("restarting/stopping container after health-check turned unhealthy: %w", err) } case define.HealthCheckOnFailureActionStop: @@ -346,6 +350,18 @@ func (c *Container) updateHealthStatus(status string) error { return os.WriteFile(c.healthCheckLogPath(), newResults, 0700) } +// isUnhealthy returns if the current health check status in unhealthy. +func (c *Container) isUnhealthy() (bool, error) { + if !c.HasHealthCheck() { + return false, nil + } + healthCheck, err := c.getHealthCheckLog() + if err != nil { + return false, err + } + return healthCheck.Status == define.HealthCheckUnhealthy, nil +} + // UpdateHealthCheckLog parses the health check results and writes the log func (c *Container) updateHealthCheckLog(hcl define.HealthCheckLog, inStartPeriod bool) (string, error) { c.lock.Lock() diff --git a/test/system/220-healthcheck.bats b/test/system/220-healthcheck.bats index 1ff26f2e6a..b8d70fe707 100644 --- a/test/system/220-healthcheck.bats +++ b/test/system/220-healthcheck.bats @@ -139,20 +139,23 @@ Log[-1].Output | \"Uh-oh on stdout!\\\nUh-oh on stderr!\" run_podman 1 healthcheck run $ctr is "$output" "unhealthy" "output from 'podman healthcheck run' (policy: $policy)" - run_podman inspect $ctr --format "{{.State.Status}} {{.Config.HealthcheckOnFailureAction}}" - if [[ $policy == "restart" ]];then - # Container has been restarted and health check works again - is "$output" "running $policy" "container has been restarted" - run_podman container inspect $ctr --format "{{.State.Healthcheck.FailingStreak}}" - is "$output" "0" "Failing streak of restarted container should be 0 again" - run_podman healthcheck run $ctr + if [[ $policy == "restart" ]];then + # Make sure the container transitions back to running + run_podman wait --condition=running $ctr + run_podman inspect $ctr --format "{{.RestartCount}}" + assert "${#lines[@]}" != 0 "Container has been restarted at least once" + run_podman container inspect $ctr --format "{{.State.Healthcheck.FailingStreak}}" + is "$output" "0" "Failing streak of restarted container should be 0 again" + run_podman healthcheck run $ctr elif [[ $policy == "none" ]];then + run_podman inspect $ctr --format "{{.State.Status}} {{.Config.HealthcheckOnFailureAction}}" # Container is still running and health check still broken is "$output" "running $policy" "container continued running" run_podman 1 healthcheck run $ctr is "$output" "unhealthy" "output from 'podman healthcheck run' (policy: $policy)" - else - # kill and stop yield the container into a non-running state + else + run_podman inspect $ctr --format "{{.State.Status}} {{.Config.HealthcheckOnFailureAction}}" + # kill and stop yield the container into a non-running state is "$output" ".* $policy" "container was stopped/killed (policy: $policy)" assert "$output" != "running $policy" # also make sure that it's not stuck in the stopping state @@ -164,4 +167,39 @@ Log[-1].Output | \"Uh-oh on stdout!\\\nUh-oh on stderr!\" done } +@test "podman healthcheck --health-on-failure with interval" { + ctr="healthcheck_c" + + for policy in stop kill restart ;do + t0=$(date --iso-8601=seconds) + run_podman run -d --name $ctr \ + --health-cmd /bin/false \ + --health-retries=1 \ + --health-on-failure=$policy \ + --health-interval=1s \ + $IMAGE top + + if [[ $policy == "restart" ]];then + # Sleeping for 2 seconds makes the test much faster than using + # podman-wait which would compete with the container getting + # restarted. + sleep 2 + # Make sure the container transitions back to running + run_podman wait --condition=running $ctr + run_podman inspect $ctr --format "{{.RestartCount}}" + assert "${#lines[@]}" != 0 "Container has been restarted at least once" + else + # kill and stop yield the container into a non-running state + run_podman wait $ctr + run_podman inspect $ctr --format "{{.State.Status}} {{.Config.HealthcheckOnFailureAction}}" + is "$output" ".* $policy" "container was stopped/killed (policy: $policy)" + assert "$output" != "running $policy" + # also make sure that it's not stuck in the stopping state + assert "$output" != "stopping $policy" + fi + + run_podman rm -f -t0 $ctr + done +} + # vim: filetype=sh