Clean up after unexpectedly terminated build #25102

Honny1 · 2025-01-23T09:41:47Z

The podman system prune command can remove build containers that were created during the build but were not removed because the build terminated unexpectedly.

By default, build containers are not removed to prevent interference with builds in progress. Use the --build flag when running the command to remove build containers as well.

Reproducer:

Containerfile:

FROM ubi8/ubi
RUN truncate -s 10G out
RUN echo "Hi"
RUN sleep infinity

Test script run.sh:

#!/usr/bin/env bash

podman build -f Containerfile -t podmanleaker &
sleep 60 && kill -9 $!

measure the size of current images, containers, etc... Before build

podman unshare du -sh ~/.local/share/containers/

Test script (Note: requires disk space of about 32 GB)

./run.sh

measure the size of current images, containers, etc... After termination of build

podman unshare du -sh ~/.local/share/containers/

Clean up leftovers after build

podman system prune --build -f

measure the size of current images, containers, etc... After cleanup build

podman unshare du -sh ~/.local/share/containers/

The size should be the same as the first measurement but could be different if a base image is present in the system.

Fixes: #14523
Fixes: #23683
Fixes: https://issues.redhat.com/browse/RHEL-62009

Does this PR introduce a user-facing change?

The `podman system prune` command now supports removing build containers with the new `--build` option.

openshift-ci · 2025-01-23T09:41:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Honny1
Once this PR has been reviewed and has the lgtm label, please assign giuseppe for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Luap99 · 2025-01-23T10:35:48Z

libpod/runtime.go

+		if err != nil {
+			return stageContainersPruneReports, err
+		}
+		if _, err := os.Stat(filepath.Join(path, "buildah.json")); errors.Is(err, fs.ErrNotExist) {


use fileutils.Exists()

@nalind Is there another (better?) way to check if a storage container is from buildah?

Luap99 · 2025-01-23T10:37:09Z

libpod/runtime.go

+		size, err := r.store.ContainerSize(container.ID)
+		if err != nil {
+			report.Err = err
+			logrus.Warnf("Failed to get size of build stage container %s: %v", container.ID, err)


do not report the error to the caller and also print a warning.
Just reporting the errors back to the caller is good enough. The caller can then log once.

Luap99 · 2025-01-23T10:37:32Z

libpod/runtime.go

+			report.Err = err
+			logrus.Warnf("Failed to remove build stage container %s: %v", container.ID, err)


Luap99 · 2025-01-23T10:39:33Z

pkg/domain/infra/abi/system.go

+
+	reclaimedSpace := (uint64)(0)
+	found := true
+	for found {


Why is this a loop? Running once should be enough.

Luap99 · 2025-01-23T10:44:08Z

pkg/domain/infra/abi/system.go

+		stageContainersPruneReports, err := ic.Libpod.PruneStageContainers()
+		if err != nil {
+			return nil, err
+		}
+		if len(stageContainersPruneReports) > 0 {
+			found = true
+		}
+		reclaimedSpace += reports.PruneReportsSize(stageContainersPruneReports)
+		systemPruneReport.ContainerPruneReports = append(systemPruneReport.ContainerPruneReports, stageContainersPruneReports...)
+
+		// Prune Images
+		imagePruneOptions := entities.ImagePruneOptions{
+			External:   true,
+			BuildCache: true,
+		}
+		imageEngine := ImageEngine{Libpod: ic.Libpod}
+		imagePruneReports, err := imageEngine.Prune(ctx, imagePruneOptions)
+		if err != nil {
+			return nil, err
+		}
+		if len(imagePruneReports) > 0 {
+			found = true
+		}
+		reclaimedSpace += reports.PruneReportsSize(imagePruneReports)
+		systemPruneReport.ImagePruneReports = append(systemPruneReport.ImagePruneReports, imagePruneReports...)


This seems inconsistent with the documentation, from the docs you wrote it sounds like it only prune the build containers but this prunes everything other containers and images as well.
I don't think this is desirable.
If we want that behavior then the code should not cause a conflict with --build option and only do this in addition to the existing code in SystemPrune() instead of duplicating the image logic here.

Luap99 · 2025-01-23T10:47:34Z

test/e2e/prune_test.go

+		hasNone, result := none.GrepString("<none>")
+		Expect(result).To(HaveLen(1))
+		Expect(hasNone).To(BeTrue())
+


This leads to horrible Expected false to be true errors.

Please use the proper matchers, something like Expect(none.OutputToString()).To(ContanSubstring("none"))

Luap99 · 2025-01-23T10:49:58Z

test/e2e/prune_test.go

+		dirents, err := os.ReadDir(containerStorageDir)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(dirents).To(HaveLen(6))


Does this matter? This peaks at rather internal storage details which can change requiring the test to be updated often. Would it not be better to run buildah containers instead? Then in the end ensure it is removed there?

Luap99 · 2025-01-23T10:51:36Z

test/e2e/prune_test.go

+		after := podmanTest.Podman([]string{"images", "-a"})
+		after.WaitWithDefaultTimeout()
+		Expect(after).Should(ExitCleanly())
+		Expect(len(after.OutputToStringArray())).To(BeNumerically(">", 1))


Not sure what this should check, AFAIK in the test/e2e setup we will always have extra images from the additional store shown.

Luap99 · 2025-01-23T10:51:47Z

test/e2e/prune_test.go

+		hasNoneAfter, result := after.GrepString("<none>")
+		Expect(result).To(BeEmpty())
+		Expect(hasNoneAfter).To(BeFalse())
+		hasNotLeakerImager, _ := after.GrepString("notleaker")
+		Expect(hasNotLeakerImager).To(BeTrue())


also needs to use proper matchers

Luap99 · 2025-01-23T10:52:08Z

test/e2e/prune_test.go

+		// still have: volatile-containers.json, containers.json, containers.lock and container dir
+		dirents, err = os.ReadDir(containerStorageDir)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(dirents).To(HaveLen(4))


same comment I would use buildah containers to check

Luap99 · 2025-01-23T10:56:13Z

Also what this doesn't answer what happens if the build process is still running? And another thing that this does not cleanup is the networking, not sure if there is a good way for that but if you kill the build and run with bridge networking the interfaces/firewall rules and ipam db allocations are still leaked. They should be cleaned after reboot so not that bad like the storage leak and certainly not a blocker for this here. I just mention it as there are other leaks to consider too.

Honny1 · 2025-01-23T14:50:12Z

@Luap99 I have incorporated your review. I've changed the approach and the --build flag allows the removal of build/stage containers for the podman system prune command, so it works the same as --volumes. The networking should also be cleaned.

Luap99

Thanks, code wise this seems good to me. just a minor comment on the man page and we need to make sure the test does not run forever

Luap99 · 2025-01-24T16:24:51Z

docs/source/markdown/podman-system-prune.1.md

+
+Removes any build containers that were created during the build, but were not removed because the build was unexpectedly terminated.
+
+> **This is not safe operation and should be executed only when no builds are in progress. It can interfere with builds in progress.**


I think the general style in the man pages was to write Note: ... or something like that. I don't think we have used > elsewhere. I don't mind it for the web view but I have not yet looked at how it looks in the rendered man page.

Luap99 · 2025-01-24T16:27:52Z

test/e2e/prune_test.go

+			if build.LineInOutputContains("Please use signal 9") {
+				break
+			}


this is great but maybe add a small sleep for like 10ms or something to not busy poll which also eats a lot of resources.

Second this needs a timeout, if the build process fails and never prints the output this would loop forever causing hard to debug issues in CI.

The `podman system prune` command is able to remove build containers that were created during the build, but were not removed because the build terminated unexpectedly. By default, build containers are not removed to prevent interference with builds in progress. Use the **--build** flag when running the command to remove build containers as well. Fixes: https://issues.redhat.com/browse/RHEL-62009 Signed-off-by: Jan Rodák <[email protected]>

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Jan 23, 2025

Honny1 force-pushed the prune branch 2 times, most recently from 596c6bb to c607887 Compare January 23, 2025 10:34

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Jan 23, 2025

Luap99 reviewed Jan 23, 2025

View reviewed changes

Honny1 force-pushed the prune branch from c607887 to 99aa16d Compare January 23, 2025 14:42

Honny1 marked this pull request as ready for review January 23, 2025 15:21

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2025

Honny1 requested a review from Luap99 January 23, 2025 15:46

Luap99 reviewed Jan 24, 2025

View reviewed changes

Honny1 force-pushed the prune branch from 99aa16d to cb1e549 Compare January 25, 2025 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up after unexpectedly terminated build #25102

Clean up after unexpectedly terminated build #25102

Honny1 commented Jan 23, 2025 •

edited

Loading

openshift-ci bot commented Jan 23, 2025

Luap99 Jan 23, 2025

Luap99 Jan 23, 2025

Luap99 Jan 23, 2025

Luap99 Jan 23, 2025

Luap99 Jan 23, 2025

Luap99 Jan 23, 2025

Luap99 Jan 23, 2025

Luap99 Jan 23, 2025

Luap99 Jan 23, 2025

Luap99 Jan 23, 2025

Luap99 commented Jan 23, 2025

Honny1 commented Jan 23, 2025 •

edited

Loading

Luap99 left a comment

Luap99 Jan 24, 2025

Luap99 Jan 24, 2025

		report.Err = err
		logrus.Warnf("Failed to remove build stage container %s: %v", container.ID, err)


		Removes any build containers that were created during the build, but were not removed because the build was unexpectedly terminated.

		> This is not safe operation and should be executed only when no builds are in progress. It can interfere with builds in progress.

Clean up after unexpectedly terminated build #25102

Are you sure you want to change the base?

Clean up after unexpectedly terminated build #25102

Conversation

Honny1 commented Jan 23, 2025 • edited Loading

Reproducer:

Does this PR introduce a user-facing change?

openshift-ci bot commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 commented Jan 23, 2025

Honny1 commented Jan 23, 2025 • edited Loading

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Honny1 commented Jan 23, 2025 •

edited

Loading

Honny1 commented Jan 23, 2025 •

edited

Loading