From b3f31026eecd5b9f49b8932f53556d3ea521ee13 Mon Sep 17 00:00:00 2001 From: Brent Baude Date: Thu, 16 Dec 2021 13:55:08 -0600 Subject: [PATCH 1/2] Removed .service file for healthchecks when a container with healthchecks exits due to stopping or failure, we need the cleanup process to remove both the timer file and the service file. Bz#:2024229 Signed-off-by: Brent Baude --- libpod/container_internal.go | 6 +++--- libpod/healthcheck_linux.go | 24 ++++++++++++++++-------- test/e2e/healthcheck_run_test.go | 16 ++++++++++++++++ 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 54d6b13039..c880326619 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -1814,7 +1814,7 @@ func (c *Container) cleanupStorage() error { return cleanupErr } -// Unmount the a container and free its resources +// Unmount the container and free its resources func (c *Container) cleanup(ctx context.Context) error { var lastError error @@ -1822,8 +1822,8 @@ func (c *Container) cleanup(ctx context.Context) error { // Remove healthcheck unit/timer file if it execs if c.config.HealthCheckConfig != nil { - if err := c.removeTimer(); err != nil { - logrus.Errorf("Error removing timer for container %s healthcheck: %v", c.ID(), err) + if err := c.removeTransientFiles(ctx); err != nil { + logrus.Errorf("Removing timer for container %s healthcheck: %v", c.ID(), err) } } diff --git a/libpod/healthcheck_linux.go b/libpod/healthcheck_linux.go index e08214809a..2c19e0a61d 100644 --- a/libpod/healthcheck_linux.go +++ b/libpod/healthcheck_linux.go @@ -1,6 +1,7 @@ package libpod import ( + "context" "fmt" "os" "os/exec" @@ -59,9 +60,9 @@ func (c *Container) startTimer() error { return err } -// removeTimer removes the systemd timer and unit files +// removeTransientFiles removes the systemd timer and unit files // for the container -func (c *Container) removeTimer() error { +func (c *Container) removeTransientFiles(ctx context.Context) error { if c.disableHealthCheckSystemd() { return nil } @@ -71,12 +72,19 @@ func (c *Container) removeTimer() error { } defer conn.Close() timerFile := fmt.Sprintf("%s.timer", c.ID()) - _, err = conn.StopUnit(timerFile, "fail", nil) - - // We want to ignore errors where the timer unit has already been removed. The error - // return is generic so we have to check against the string in the error - if err != nil && strings.HasSuffix(err.Error(), ".timer not loaded.") { - return nil + serviceFile := fmt.Sprintf("%s.service", c.ID()) + // We want to ignore errors where the timer unit and/or service unit has already + // been removed. The error return is generic so we have to check against the + // string in the error + if _, err = conn.StopUnitContext(ctx, serviceFile, "fail", nil); err != nil { + if !strings.HasSuffix(err.Error(), ".service not loaded.") { + return errors.Wrapf(err, "unable to remove service file") + } + } + if _, err = conn.StopUnitContext(ctx, timerFile, "fail", nil); err != nil { + if strings.HasSuffix(err.Error(), ".timer not loaded.") { + return nil + } } return err } diff --git a/test/e2e/healthcheck_run_test.go b/test/e2e/healthcheck_run_test.go index b30e8b810c..73af545c35 100644 --- a/test/e2e/healthcheck_run_test.go +++ b/test/e2e/healthcheck_run_test.go @@ -227,4 +227,20 @@ var _ = Describe("Podman healthcheck run", func() { Expect(len(ps.OutputToStringArray())).To(Equal(2)) Expect(ps.OutputToString()).To(ContainSubstring("hc")) }) + + It("stopping and then starting a container with healthcheck cmd", func() { + session := podmanTest.Podman([]string{"run", "-dt", "--name", "hc", "--health-cmd", "[\"ls\", \"/foo\"]", ALPINE, "top"}) + session.WaitWithDefaultTimeout() + Expect(session).Should(Exit(0)) + + stop := podmanTest.Podman([]string{"stop", "-t0", "hc"}) + stop.WaitWithDefaultTimeout() + Expect(stop).Should(Exit(0)) + + startAgain := podmanTest.Podman([]string{"start", "hc"}) + startAgain.WaitWithDefaultTimeout() + Expect(startAgain).Should(Exit(0)) + Expect(startAgain.OutputToString()).To(Equal("hc")) + Expect(startAgain.ErrorToString()).To(Equal("")) + }) }) From 241f26f934b92e87e4dec21a06937a865e6b597d Mon Sep 17 00:00:00 2001 From: Brent Baude Date: Thu, 23 Dec 2021 11:31:00 -0600 Subject: [PATCH 2/2] .service file removal on failure Prodding bz #2024229 a little more, it turns out the service file is NOT deleted when it is in a failed state (i.e the healtch check has failed for some reason). The state must be reset before the service is stopped on container removal and then the files will be removed properly. BZ#:2024229 [NO NEW TESTS NEEDED] Signed-off-by: Brent Baude --- libpod/healthcheck_linux.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/libpod/healthcheck_linux.go b/libpod/healthcheck_linux.go index 2c19e0a61d..a1f3e84911 100644 --- a/libpod/healthcheck_linux.go +++ b/libpod/healthcheck_linux.go @@ -73,6 +73,16 @@ func (c *Container) removeTransientFiles(ctx context.Context) error { defer conn.Close() timerFile := fmt.Sprintf("%s.timer", c.ID()) serviceFile := fmt.Sprintf("%s.service", c.ID()) + + // If the service has failed (the healthcheck has failed), then + // the .service file is not removed on stopping the unit file. If + // we check the properties of the service, it will automatically + // reset the state. But checking the state takes msecs vs usecs to + // blindly call reset. + if err := conn.ResetFailedUnitContext(ctx, serviceFile); err != nil { + logrus.Debugf("failed to reset unit file: %q", err) + } + // We want to ignore errors where the timer unit and/or service unit has already // been removed. The error return is generic so we have to check against the // string in the error