RUN: only use an overlay for --mount=type=bind,rw on overlay #6126

nalind · 2025-04-15T21:07:02Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Only use an overlay for --mounts of type=bind,rw, where changes should be discarded, if the storage driver is overlay. Otherwise, make a temporary copy.

How to verify it

Updated CI to test {vfs|overlay} over {vfs|overlay}, which would have caught this.

Which issue(s) this PR fixes:

Should fix #5988.

Special notes for your reviewer:

Basically what #5988 (comment) suggested.

Does this PR introduce a user-facing change?

When not using overlay for storage, running `buildah run` or executing a `buildah build` RUN instruction with the `--mount=type=bind,rw` flag will now use a temporary copy to ensure that writes are discarded, instead of attempting to mount an overlay filesystem using the source location as a "lower".

packit-as-a-service · 2025-04-17T18:23:15Z

Ephemeral COPR build failed. @containers/packit-build please check.

tests/run.bats

cgwalters

I only gave this a quick skim and I don't know the code really, but FWIW looks sane to me.

cgwalters · 2025-05-02T13:54:23Z

internal/volumes/volumes.go

@@ -118,6 +120,87 @@ func convertToOverlay(m specs.Mount, store storage.Store, mountLabel, tmpDir str
 	return mountThisInstead, overlayDir, nil
 }

+func makeTempCopy(m specs.Mount, tmpDir string, uid, gid int) (specs.Mount, string, error) {


This function seems like it could use a doc comment.

tests/chroot.bats

.cirrus.yml

TomSweeneyRedHat · 2025-06-04T19:28:45Z

@nalind looks lke you need an update here.

Honny1 · 2025-06-05T12:57:22Z

tests/run.bats

@@ -420,23 +420,23 @@ function configure_and_check_user() {
 	cid=$output
 	mkdir -p ${TEST_SCRATCH_DIR}/was:empty
 	# As a baseline, this should succeed.
-	run_buildah run --mount type=tmpfs,dst=/var/tmpfs-not-empty                                                      $cid touch /var/tmpfs-not-empty/testfile
+	run_buildah run --mount type=tmpfs,dst=/var/tmpfs-not-empty                                                            $cid touch /var/tmpfs-not-empty/testfile


Not blocking: I'm not sure if the spaces are intentional, but if they are to improve readability. It might be better to move the value of the --mount argument to a separate variable.

Ah, that does help. Thanks!

Honny1

Thanks, LGTM. I haven't noticed any problems. I think a rebase will be necessary.

/approve

openshift-ci · 2025-06-05T12:59:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Honny1, nalind

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [nalind]

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

Expand our task that runs the integration tests inside of a container from running buildah-using-vfs-inside-podman-using-vfs and buildah-using-overlay-inside-podman-using-overlay to also test vfs over overlay, and overlay over vfs. Signed-off-by: Nalin Dahyabhai <[email protected]>

Only use an overlay for --mounts of type=bind,rw, where changes should be discarded, if the storage driver is overlay. Otherwise, make a temporary copy and use a bind mount to the copy. Signed-off-by: Nalin Dahyabhai <[email protected]>

If we're forcing a mount using overlay, but we fail, and our chosen upper directory is on overlay, try using fuse-overlayfs. Signed-off-by: Nalin Dahyabhai <[email protected]>

Force the mount of the overlay in runSetupVolumeMounts() so that we can take advantage of logic that tries to use fuse-overlayfs if we happen to be on top of overlay, which would otherwise fail. Signed-off-by: Nalin Dahyabhai <[email protected]>

Force the mount of the overlay in volumeCacheSaveOverlay(), and clean it up in volumeCacheRestoreOverlay(), so that we can take advantage of logic that tries to use fuse-overlayfs if we happen to be on top of overlay, which would otherwise fail. Signed-off-by: Nalin Dahyabhai <[email protected]>

Drop the conditional ,z that we were trying to add when testing the -v flag with the "O" option, the combination of which is not allowed by github.com/containers/common/pkg/parse.ValidateVolumeOpts(). Signed-off-by: Nalin Dahyabhai <[email protected]>

When grepping the mounts list for the root mount, also accept the entry if "rw" is the only flag. Signed-off-by: Nalin Dahyabhai <[email protected]>

If we're on overlayfs, we can't use the directory name that we'd like to use as part of the test and expect things to succeed, even if we try to work around it using fuse-overlayfs. Signed-off-by: Nalin Dahyabhai <[email protected]>

When setting up a chroot using overlay, if the intended upper directory is already on an overlayfs, mount a tmpfs onto it so that we can finish setting up the chroot that's an overlayfs that we'll actually test in, and copy the base image into the storage that it'll use. Signed-off-by: Nalin Dahyabhai <[email protected]>

mtrmac

I don’t know enough to judge the volumeCache… part.

Overall, I suppose the added test coverage should be decisive.

mtrmac · 2025-06-06T01:30:26Z

internal/volumes/volumes.go

-		if newMount, overlayDir, err = convertToOverlay(newMount, store, mountLabel, tmpDir, 0, 0); err != nil {
-			return newMount, "", "", "", err
+		switch store.GraphDriverName() {
+		case "overlay":


(There is precedent for hard-coding a condition like this … but maybe the checks could all be consolidated into a single function.)

mtrmac · 2025-06-06T01:48:17Z

run_linux.go

+				ForceMount:             true,
+				MountLabel:             b.MountLabel,


Force the mount of the overlay in runSetupVolumeMounts() so that we can
take advantage of logic that tries to use fuse-overlayfs if we happen to
be on top of overlay, which would otherwise fail.

I read that as potentially unclean… if using fuse-overlay this way is expected for an “overlay” mount, shouldn’t the consumer of the created specs.Mount be able to handle that universally, instead of us special-casing it and adding an extra mount operation every time?

*shrug* I suppose changing all relevant OCI runtimes is unpalatable?

With this, every single caller in Buildah except for run_freebsd.go (but not the callers in podman/libpod) sets ForceMount. Is that correct, or should the option be removed entirely?

github-actions · 2025-07-07T00:17:23Z

A friendly reminder that this PR had no activity for 30 days.

TomSweeneyRedHat · 2025-07-10T20:33:39Z

@nalind merge conflict here that needs some massaging.

openshift-ci bot added do-not-merge/work-in-progress kind/bug Categorizes issue or PR as related to a bug. approved labels Apr 15, 2025

nalind force-pushed the overlay-copy branch 7 times, most recently from 6084d5f to b81b006 Compare April 17, 2025 18:20

nalind force-pushed the overlay-copy branch 2 times, most recently from 86b6879 to f0e6488 Compare April 23, 2025 21:57

nalind force-pushed the overlay-copy branch 6 times, most recently from 04b80e1 to e62f77c Compare April 25, 2025 20:55

siteshwar reviewed Apr 26, 2025

View reviewed changes

tests/run.bats Outdated Show resolved Hide resolved

nalind force-pushed the overlay-copy branch 7 times, most recently from dad02e0 to 5dd5313 Compare April 28, 2025 21:26

nalind marked this pull request as ready for review April 28, 2025 23:52

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 28, 2025

nalind mentioned this pull request May 2, 2025

buildah build: use the same overlay for the context directory for the whole build #5975

Open

cgwalters reviewed May 2, 2025

View reviewed changes

nalind force-pushed the overlay-copy branch from 5dd5313 to 2ed4845 Compare May 2, 2025 14:19

cevich mentioned this pull request May 5, 2025

ERRO[0025] unlinkat /var/tmp/buildah2410054376/mounts3022885724/bind626918239: device or resource busy #5988

Open

cevich reviewed May 5, 2025

View reviewed changes

tests/chroot.bats Outdated Show resolved Hide resolved

cevich reviewed May 5, 2025

View reviewed changes

.cirrus.yml Outdated Show resolved Hide resolved

siteshwar mentioned this pull request May 6, 2025

buildah: ignore tests and vendor directories openscanhub/known-false-positives#30

Merged

cevich mentioned this pull request May 8, 2025

Add vfs/chroot build test to aio container containers/image_build#44

Open

nalind force-pushed the overlay-copy branch from 2ed4845 to f93c977 Compare June 4, 2025 14:40

Honny1 reviewed Jun 5, 2025

View reviewed changes

Honny1 approved these changes Jun 5, 2025

View reviewed changes

nalind added 10 commits June 5, 2025 10:01

pkg/overlay: fall back to trying fuse-overlayfs

86c0b35

If we're forcing a mount using overlay, but we fail, and our chosen upper directory is on overlay, try using fuse-overlayfs. Signed-off-by: Nalin Dahyabhai <[email protected]>

run.bats(root fs only mounted once): also accept rw by itself

d3595ff

When grepping the mounts list for the root mount, also accept the entry if "rw" is the only flag. Signed-off-by: Nalin Dahyabhai <[email protected]>

run.bats(run --mount): skip if we're on overlay

b4837fd

If we're on overlayfs, we can't use the directory name that we'd like to use as part of the test and expect things to succeed, even if we try to work around it using fuse-overlayfs. Signed-off-by: Nalin Dahyabhai <[email protected]>

nalind force-pushed the overlay-copy branch from f93c977 to b2b76a7 Compare June 5, 2025 14:07

mtrmac reviewed Jun 6, 2025

View reviewed changes

github-actions bot added the stale-pr label Jul 7, 2025

github-actions bot removed the stale-pr label Jul 11, 2025

RUN: only use an overlay for --mount=type=bind,rw on overlay #6126

Are you sure you want to change the base?

RUN: only use an overlay for --mount=type=bind,rw on overlay #6126

Uh oh!

Conversation

nalind commented Apr 15, 2025

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

packit-as-a-service bot commented Apr 17, 2025

Uh oh!

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

cgwalters May 2, 2025

Choose a reason for hiding this comment

Uh oh!

nalind May 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TomSweeneyRedHat commented Jun 4, 2025

Uh oh!

Honny1 Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalind Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

Honny1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jun 5, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

mtrmac Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 7, 2025

Uh oh!

TomSweeneyRedHat commented Jul 10, 2025

Uh oh!

Uh oh!

Honny1 Jun 5, 2025 •

edited

Loading

Honny1 left a comment •

edited

Loading

mtrmac Jun 6, 2025 •

edited

Loading