From 4620e91f86e7a6130cb8d8e37bdea9d0d90ea557 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 7 Aug 2024 16:33:38 +0200 Subject: [PATCH] podman container cleanup: ignore common errors The podman container cleanup command is not really intended for human use. Instead each conmon will spawn this command after the container exit to make sure we can cleanup resources asynchronously. However this command will always race against other foreground process such as podman rm -fa. Therefore it is possible that the ctr was already removed and we should not log errors in this case. While these errors are normally not seen as the command is int he background you can see it if you enable syslog logging and then they just spam the log with useless errors so just ignore them. Signed-off-by: Paul Holzinger --- pkg/domain/infra/abi/containers.go | 23 ++++++++++++++--------- test/e2e/cleanup_test.go | 20 ++++++++++++++++---- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index b60a4a8e8f..9afed94b86 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -1267,6 +1267,10 @@ func (ic *ContainerEngine) ContainerLogs(ctx context.Context, namesOrIds []strin func (ic *ContainerEngine) ContainerCleanup(ctx context.Context, namesOrIds []string, options entities.ContainerCleanupOptions) ([]*entities.ContainerCleanupReport, error) { containers, err := getContainers(ic.Libpod, getContainersOptions{all: options.All, latest: options.Latest, names: namesOrIds}) if err != nil { + // cleanup command spawned by conmon lost race as another process already removed the ctr + if errors.Is(err, define.ErrNoSuchCtr) { + return nil, nil + } return nil, err } reports := []*entities.ContainerCleanupReport{} @@ -1276,13 +1280,13 @@ func (ic *ContainerEngine) ContainerCleanup(ctx context.Context, namesOrIds []st if options.Exec != "" { if options.Remove { - if err := ctr.ExecRemove(options.Exec, false); err != nil { - return nil, err - } + err = ctr.ExecRemove(options.Exec, false) } else { - if err := ctr.ExecCleanup(options.Exec); err != nil { - return nil, err - } + err = ctr.ExecCleanup(options.Exec) + } + // If ErrNoSuchExecSession then the exec session was already removed so do not report an error. + if err != nil && !errors.Is(err, define.ErrNoSuchExecSession) { + return nil, err } return []*entities.ContainerCleanupReport{}, nil } @@ -1290,12 +1294,13 @@ func (ic *ContainerEngine) ContainerCleanup(ctx context.Context, namesOrIds []st if options.Remove && !ctr.ShouldRestart(ctx) { var timeout *uint err = ic.Libpod.RemoveContainer(ctx, ctr.Container, false, true, timeout) - if err != nil { + if err != nil && !errors.Is(err, define.ErrNoSuchCtr) { report.RmErr = fmt.Errorf("failed to clean up and remove container %v: %w", ctr.ID(), err) } } else { err := ctr.Cleanup(ctx) - if err != nil { + // ignore error if ctr is removed or cannot be cleaned up, likely the ctr was already restarted by another process + if err != nil && !errors.Is(err, define.ErrNoSuchCtr) && !errors.Is(err, define.ErrCtrStateInvalid) { report.CleanErr = fmt.Errorf("failed to clean up container %v: %w", ctr.ID(), err) } } @@ -1303,7 +1308,7 @@ func (ic *ContainerEngine) ContainerCleanup(ctx context.Context, namesOrIds []st if options.RemoveImage { _, imageName := ctr.Image() imageEngine := ImageEngine{Libpod: ic.Libpod} - _, rmErrors := imageEngine.Remove(ctx, []string{imageName}, entities.ImageRemoveOptions{}) + _, rmErrors := imageEngine.Remove(ctx, []string{imageName}, entities.ImageRemoveOptions{Ignore: true}) report.RmiErr = errorhandling.JoinErrors(rmErrors) } diff --git a/test/e2e/cleanup_test.go b/test/e2e/cleanup_test.go index 10bdd9d92c..aa48fab3c3 100644 --- a/test/e2e/cleanup_test.go +++ b/test/e2e/cleanup_test.go @@ -12,10 +12,10 @@ var _ = Describe("Podman container cleanup", func() { SkipIfRemote("podman container cleanup is not supported in remote") }) - It("podman cleanup bogus container", func() { + It("podman cleanup bogus container should not error", func() { session := podmanTest.Podman([]string{"container", "cleanup", "foobar"}) session.WaitWithDefaultTimeout() - Expect(session).Should(ExitWithError(125, `no container with name or ID "foobar" found: no such container`)) + Expect(session).Should(ExitCleanly()) }) It("podman cleanup container by id", func() { @@ -86,7 +86,13 @@ var _ = Describe("Podman container cleanup", func() { Expect(session).Should(ExitCleanly()) session = podmanTest.Podman([]string{"container", "cleanup", "running"}) session.WaitWithDefaultTimeout() - Expect(session).Should(ExitWithError(125, "is running or paused, refusing to clean up: container state improper")) + Expect(session).Should(ExitCleanly()) + + // cleanup should be a NOP here, ctr must still be running + session = podmanTest.Podman([]string{"container", "inspect", "--format={{.State.Status}}", "running"}) + session.WaitWithDefaultTimeout() + Expect(session).Should(ExitCleanly()) + Expect(session.OutputToString()).To(Equal("running")) }) It("podman cleanup paused container", func() { @@ -99,7 +105,13 @@ var _ = Describe("Podman container cleanup", func() { Expect(session).Should(ExitCleanly()) session = podmanTest.Podman([]string{"container", "cleanup", "paused"}) session.WaitWithDefaultTimeout() - Expect(session).Should(ExitWithError(125, "is running or paused, refusing to clean up: container state improper")) + Expect(session).Should(ExitCleanly()) + + // cleanup should be a NOP here, ctr must still be paused + session = podmanTest.Podman([]string{"container", "inspect", "--format={{.State.Status}}", "paused"}) + session.WaitWithDefaultTimeout() + Expect(session).Should(ExitCleanly()) + Expect(session.OutputToString()).To(Equal("paused")) // unpause so that the cleanup can stop the container, // otherwise it fails with container state improper