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".

openshift-ci · 2025-04-15T21:07:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

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.

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]>

cevich · 2025-05-05T13:07:25Z

tests/chroot.bats

+    if test -x /usr/bin/fuse-overlayfs ; then
+      local storage_opts="overlay.mount_program=/usr/bin/fuse-overlayfs"
+    else
+      skip trying to use overlay on top of overlay, but fuse-overlayfs is not present


Possibly minor: Everywhere else I looked the reason is passed as a single argument to skip by quoting it. I'm unsure if passing multiple arguments like this also works, but for consistency it would be nice to quote it. But this is such a minor thing, please don't re-push only to fix this.

cevich · 2025-05-05T13:13:58Z

.cirrus.yml

+    matrix:
+        - env:
+              STORAGE_DRIVER: 'vfs'
+        - env:
+              STORAGE_DRIVER: 'overlay'
+    matrix:
+        - env:
+              NESTED_STORAGE_DRIVER: 'vfs'
+        - env:
+              NESTED_STORAGE_DRIVER: 'overlay'


I believe cirrus will happily accept this invalid YAML, but I believe hack/get_ci_vm.sh will puke on it because it uses python-yaml to parse the file. If this functionality is important, I believe something like the following would work:

Suggested change

matrix:

- env:

STORAGE_DRIVER: 'vfs'

- env:

STORAGE_DRIVER: 'overlay'

matrix:

- env:

NESTED_STORAGE_DRIVER: 'vfs'

- env:

NESTED_STORAGE_DRIVER: 'overlay'

matrix:

- env:

STORAGE_DRIVER: 'vfs'

NESTED_STORAGE_DRIVER: 'vfs'

- env:

STORAGE_DRIVER: 'overlay'

NESTED_STORAGE_DRIVER: 'vfs'

- env:

STORAGE_DRIVER: 'vfs'

NESTED_STORAGE_DRIVER: 'overlay'

- env:

STORAGE_DRIVER: 'overlay'

NESTED_STORAGE_DRIVER: 'overlay'

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

openshift-ci bot added the approved label 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 added 10 commits May 2, 2025 10:04

pkg/overlay: fall back to trying fuse-overlayfs

c2f530c

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

b2b6d04

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

60e2359

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 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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nalind commented Apr 15, 2025

openshift-ci bot commented Apr 15, 2025

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

cgwalters left a comment

cgwalters May 2, 2025

nalind May 2, 2025

cevich May 5, 2025

cevich May 5, 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

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?

openshift-ci bot commented Apr 15, 2025

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

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters May 2, 2025

Choose a reason for hiding this comment

nalind May 2, 2025

Choose a reason for hiding this comment

cevich May 5, 2025

Choose a reason for hiding this comment

cevich May 5, 2025

Choose a reason for hiding this comment