Skip to content

Commit a7fe479

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #5934 from nalind/cache-mount-images
Accept image names as sources for cache mounts
2 parents ea50c7b + 3541a3c commit a7fe479

File tree

8 files changed

+237
-108
lines changed

8 files changed

+237
-108
lines changed

‎cmd/buildah/run.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ func runCmd(c *cobra.Command, args []string, iopts runInputOptions) error {
191191
if err != nil {
192192
return fmt.Errorf("building system context: %w", err)
193193
}
194-
mounts, mountedImages, intermediateMounts, _, targetLocks, err := volumes.GetVolumes(systemContext, store, builder.MountLabel, iopts.volumes, iopts.mounts, iopts.contextDir, iopts.workingDir, tmpDir)
194+
mounts, mountedImages, intermediateMounts, _, targetLocks, err := volumes.GetVolumes(systemContext, store, builder.MountLabel, iopts.volumes, iopts.mounts, iopts.contextDir, builder.IDMappingOptions, iopts.workingDir, tmpDir)
195195
if err != nil {
196196
return err
197197
}

‎internal/volumes/volumes.go

+122-74
Large diffs are not rendered by default.

‎internal/volumes/volumes_test.go

+9-6
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"github.com/containers/image/v5/types"
77
"github.com/containers/storage"
88
storageTypes "github.com/containers/storage/types"
9+
specs "github.com/opencontainers/runtime-spec/specs-go"
910
"github.com/stretchr/testify/assert"
1011
"github.com/stretchr/testify/require"
1112
)
@@ -14,6 +15,8 @@ func TestGetMount(t *testing.T) {
1415
tempDir := t.TempDir()
1516
rootDir := t.TempDir()
1617
runDir := t.TempDir()
18+
sys := &types.SystemContext{}
19+
var emptyIDmap []specs.LinuxIDMapping
1720

1821
store, err := storage.GetStore(storageTypes.StoreOptions{
1922
GraphDriverName: "vfs",
@@ -29,26 +32,26 @@ func TestGetMount(t *testing.T) {
2932

3033
t.Run("GetBindMount", func(t *testing.T) {
3134
for _, argNeeder := range []string{"from", "bind-propagation", "src", "source", "target", "dst", "destination", "relabel"} {
32-
_, _, _, _, err := GetBindMount(&types.SystemContext{}, []string{argNeeder}, tempDir, store, "", nil, tempDir, tempDir)
35+
_, _, _, _, err := GetBindMount(sys, []string{argNeeder}, tempDir, store, "", nil, tempDir, tempDir)
3336
assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder)
34-
_, _, _, _, err = GetBindMount(&types.SystemContext{}, []string{argNeeder + "="}, tempDir, store, "", nil, tempDir, tempDir)
37+
_, _, _, _, err = GetBindMount(sys, []string{argNeeder + "="}, tempDir, store, "", nil, tempDir, tempDir)
3538
assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder)
3639
}
3740
for _, argHater := range []string{"bind-nonrecursive", "nodev", "noexec", "nosuid", "ro", "readonly", "rw", "readwrite", "shared", "rshared", "private", "rprivate", "slave", "rslave", "Z", "z", "U", "no-dereference"} {
38-
_, _, _, _, err := GetBindMount(&types.SystemContext{}, []string{argHater + "=nonce"}, tempDir, store, "", nil, tempDir, tempDir)
41+
_, _, _, _, err := GetBindMount(sys, []string{argHater + "=nonce"}, tempDir, store, "", nil, tempDir, tempDir)
3942
assert.ErrorIsf(t, err, errBadOptionNoArg, "option %q is not supposed to have an arg, but wasn't flagged when it tried to supply one", argHater)
4043
}
4144
})
4245

4346
t.Run("GetCacheMount", func(t *testing.T) {
4447
for _, argNeeder := range []string{"sharing", "id", "from", "bind-propagation", "src", "source", "target", "dst", "destination", "mode", "uid", "gid"} {
45-
_, _, _, err := GetCacheMount([]string{argNeeder}, nil, tempDir, tempDir)
48+
_, _, _, _, _, err := GetCacheMount(sys, []string{argNeeder}, store, "", nil, emptyIDmap, emptyIDmap, tempDir, tempDir)
4649
assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder)
47-
_, _, _, err = GetCacheMount([]string{argNeeder + "="}, nil, tempDir, tempDir)
50+
_, _, _, _, _, err = GetCacheMount(sys, []string{argNeeder + "="}, store, "", nil, emptyIDmap, emptyIDmap, tempDir, tempDir)
4851
assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder)
4952
}
5053
for _, argHater := range []string{"nodev", "noexec", "nosuid", "U", "rw", "readwrite", "ro", "readonly", "shared", "Z", "z", "rshared", "private", "rprivate", "slave", "rslave"} {
51-
_, _, _, err := GetCacheMount([]string{argHater + "=nonce"}, nil, tempDir, tempDir)
54+
_, _, _, _, _, err := GetCacheMount(sys, []string{argHater + "=nonce"}, store, "", nil, emptyIDmap, emptyIDmap, tempDir, tempDir)
5255
assert.ErrorIsf(t, err, errBadOptionNoArg, "option %q is not supposed to have an arg, but wasn't flagged when it tried to supply one", argHater)
5356
}
5457
})

‎run_common.go

+36-6
Original file line numberDiff line numberDiff line change
@@ -1640,12 +1640,12 @@ func (b *Builder) runSetupRunMounts(mountPoint, bundlePath string, mounts []stri
16401640
if image != "" {
16411641
mountImages = append(mountImages, image)
16421642
}
1643-
if overlayDir != "" {
1644-
overlayDirs = append(overlayDirs, overlayDir)
1645-
}
16461643
if intermediateMount != "" {
16471644
intermediateMounts = append(intermediateMounts, intermediateMount)
16481645
}
1646+
if overlayDir != "" {
1647+
overlayDirs = append(overlayDirs, overlayDir)
1648+
}
16491649
finalMounts = append(finalMounts, *mountSpec)
16501650
case "tmpfs":
16511651
mountSpec, err = b.getTmpfsMount(tokens, idMaps, sources.WorkDir)
@@ -1659,13 +1659,19 @@ func (b *Builder) runSetupRunMounts(mountPoint, bundlePath string, mounts []stri
16591659
return nil, nil, err
16601660
}
16611661
}
1662-
mountSpec, intermediateMount, tl, err = b.getCacheMount(tokens, sources.StageMountPoints, idMaps, sources.WorkDir, bundleMountsDir)
1662+
mountSpec, image, intermediateMount, overlayDir, tl, err = b.getCacheMount(tokens, sources.SystemContext, sources.StageMountPoints, idMaps, sources.WorkDir, bundleMountsDir)
16631663
if err != nil {
16641664
return nil, nil, err
16651665
}
1666+
if image != "" {
1667+
mountImages = append(mountImages, image)
1668+
}
16661669
if intermediateMount != "" {
16671670
intermediateMounts = append(intermediateMounts, intermediateMount)
16681671
}
1672+
if overlayDir != "" {
1673+
overlayDirs = append(overlayDirs, overlayDir)
1674+
}
16691675
if tl != nil {
16701676
targetLocks = append(targetLocks, tl)
16711677
}
@@ -1706,15 +1712,39 @@ func (b *Builder) getBindMount(tokens []string, sys *types.SystemContext, contex
17061712
return nil, "", "", "", errors.New("context directory for current run invocation is not configured")
17071713
}
17081714
var optionMounts []specs.Mount
1709-
mount, image, intermediateMount, overlayMount, err := volumes.GetBindMount(sys, tokens, contextDir, b.store, b.MountLabel, stageMountPoints, workDir, tmpDir)
1715+
optionMount, image, intermediateMount, overlayMount, err := volumes.GetBindMount(sys, tokens, contextDir, b.store, b.MountLabel, stageMountPoints, workDir, tmpDir)
17101716
if err != nil {
17111717
return nil, "", "", "", err
17121718
}
1713-
optionMounts = append(optionMounts, mount)
1719+
succeeded := false
1720+
defer func() {
1721+
if !succeeded {
1722+
if overlayMount != "" {
1723+
if err := overlay.RemoveTemp(overlayMount); err != nil {
1724+
b.Logger.Debug(err.Error())
1725+
}
1726+
}
1727+
if intermediateMount != "" {
1728+
if err := mount.Unmount(intermediateMount); err != nil {
1729+
b.Logger.Debugf("unmounting %q: %v", intermediateMount, err)
1730+
}
1731+
if err := os.Remove(intermediateMount); err != nil {
1732+
b.Logger.Debugf("removing should-be-empty directory %q: %v", intermediateMount, err)
1733+
}
1734+
}
1735+
if image != "" {
1736+
if _, err := b.store.UnmountImage(image, false); err != nil {
1737+
b.Logger.Debugf("unmounting image %q: %v", image, err)
1738+
}
1739+
}
1740+
}
1741+
}()
1742+
optionMounts = append(optionMounts, optionMount)
17141743
volumes, err := b.runSetupVolumeMounts(b.MountLabel, nil, optionMounts, idMaps)
17151744
if err != nil {
17161745
return nil, "", "", "", err
17171746
}
1747+
succeeded = true
17181748
return &volumes[0], image, intermediateMount, overlayMount, nil
17191749
}
17201750

‎run_freebsd.go

+7-5
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
nettypes "github.com/containers/common/libnetwork/types"
2828
netUtil "github.com/containers/common/libnetwork/util"
2929
"github.com/containers/common/pkg/config"
30+
"github.com/containers/image/v5/types"
3031
"github.com/containers/storage/pkg/idtools"
3132
"github.com/containers/storage/pkg/lockfile"
3233
"github.com/containers/storage/pkg/stringid"
@@ -373,11 +374,12 @@ func setupSpecialMountSpecChanges(spec *specs.Spec, shmSize string) ([]specs.Mou
373374
}
374375

375376
// If this succeeded, the caller would be expected to, after the command which
376-
// uses the mount exits, unmount the mounted filesystem and remove its
377-
// mountpoint (if we provided the path to its mountpoint), and release the lock
378-
// (if we took one).
379-
func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps, workDir, tmpDir string) (*specs.Mount, string, *lockfile.LockFile, error) {
380-
return nil, "", nil, errors.New("cache mounts not supported on freebsd")
377+
// uses the mount exits, clean up the overlay filesystem (if we returned one),
378+
// unmount the mounted filesystem (if we provided the path to its mountpoint)
379+
// and remove its mountpoint, unmount the image (if we mounted one), and
380+
// release the lock (if we took one).
381+
func (b *Builder) getCacheMount(tokens []string, sys *types.SystemContext, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps, workDir, tmpDir string) (*specs.Mount, string, string, string, *lockfile.LockFile, error) {
382+
return nil, "", "", "", nil, errors.New("cache mounts not supported on freebsd")
381383
}
382384

383385
func (b *Builder) runSetupVolumeMounts(mountLabel string, volumeMounts []string, optionMounts []specs.Mount, idMaps IDMaps) (mounts []specs.Mount, Err error) {

‎run_linux.go

+23-9
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ import (
3636
"github.com/containers/common/pkg/config"
3737
"github.com/containers/common/pkg/hooks"
3838
hooksExec "github.com/containers/common/pkg/hooks/exec"
39+
"github.com/containers/image/v5/types"
3940
"github.com/containers/storage/pkg/fileutils"
4041
"github.com/containers/storage/pkg/idtools"
4142
"github.com/containers/storage/pkg/ioutils"
@@ -1427,21 +1428,29 @@ func checkIDsGreaterThan5(ids []specs.LinuxIDMapping) bool {
14271428
return false
14281429
}
14291430

1430-
// Returns a Mount to add to the runtime spec's list of mounts, an optional
1431-
// path of a mounted filesystem, unmounted, and an optional lock, or an error.
1431+
// Returns a Mount to add to the runtime spec's list of mounts, the ID of an
1432+
// image, the path to a mounted filesystem, and the path to an overlay
1433+
// filesystem, and an optional lock, or an error.
14321434
//
14331435
// The caller is expected to, after the command which uses the mount exits,
1434-
// unmount the mounted filesystem (if we provided the path to its mountpoint)
1435-
// and remove its mountpoint, , and release the lock (if we took one).
1436-
func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps, workDir, tmpDir string) (*specs.Mount, string, *lockfile.LockFile, error) {
1436+
// clean up the overlay filesystem (if we returned one), unmount the mounted
1437+
// filesystem (if we provided the path to its mountpoint) and remove its
1438+
// mountpoint, unmount the image (if we mounted one), and release the lock (if
1439+
// we took one).
1440+
func (b *Builder) getCacheMount(tokens []string, sys *types.SystemContext, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps, workDir, tmpDir string) (*specs.Mount, string, string, string, *lockfile.LockFile, error) {
14371441
var optionMounts []specs.Mount
1438-
optionMount, intermediateMount, targetLock, err := volumes.GetCacheMount(tokens, stageMountPoints, workDir, tmpDir)
1442+
optionMount, mountedImage, intermediateMount, overlayMount, targetLock, err := volumes.GetCacheMount(sys, tokens, b.store, b.MountLabel, stageMountPoints, idMaps.uidmap, idMaps.gidmap, workDir, tmpDir)
14391443
if err != nil {
1440-
return nil, "", nil, err
1444+
return nil, "", "", "", nil, err
14411445
}
14421446
succeeded := false
14431447
defer func() {
14441448
if !succeeded {
1449+
if overlayMount != "" {
1450+
if err := overlay.RemoveTemp(overlayMount); err != nil {
1451+
b.Logger.Debug(err.Error())
1452+
}
1453+
}
14451454
if intermediateMount != "" {
14461455
if err := mount.Unmount(intermediateMount); err != nil {
14471456
b.Logger.Debugf("unmounting %q: %v", intermediateMount, err)
@@ -1450,6 +1459,11 @@ func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]int
14501459
b.Logger.Debugf("removing should-be-empty directory %q: %v", intermediateMount, err)
14511460
}
14521461
}
1462+
if mountedImage != "" {
1463+
if _, err := b.store.UnmountImage(mountedImage, false); err != nil {
1464+
b.Logger.Debugf("unmounting image %q: %v", mountedImage, err)
1465+
}
1466+
}
14531467
if targetLock != nil {
14541468
targetLock.Unlock()
14551469
}
@@ -1458,8 +1472,8 @@ func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]int
14581472
optionMounts = append(optionMounts, optionMount)
14591473
volumes, err := b.runSetupVolumeMounts(b.MountLabel, nil, optionMounts, idMaps)
14601474
if err != nil {
1461-
return nil, "", nil, err
1475+
return nil, "", "", "", nil, err
14621476
}
14631477
succeeded = true
1464-
return &volumes[0], intermediateMount, targetLock, nil
1478+
return &volumes[0], mountedImage, intermediateMount, overlayMount, targetLock, nil
14651479
}

‎tests/bud.bats

+35-6
Original file line numberDiff line numberDiff line change
@@ -895,8 +895,40 @@ _EOF
895895
expect_output --substring "Groups: 1000"
896896
}
897897

898+
@test "build-mount-cache-with-id-mappings" {
899+
_prefetch alpine
900+
local contextdir=${TEST_SCRATCH_DIR}/context
901+
mkdir ${contextdir}
902+
903+
# with no ID mappings
904+
local cacheid=${SRANDOM}
905+
cat > ${contextdir}/Dockerfile << EOF
906+
FROM alpine
907+
USER 1000:1000
908+
RUN --mount=type=cache,id=${cacheid},target=/var/tmp,uid=1000,gid=1000 stat / /var/tmp
909+
RUN --mount=type=cache,id=${cacheid},target=/var/tmp,uid=1000,gid=1000 test \`stat -c %u /var/tmp\` -eq 1000
910+
RUN --mount=type=cache,id=${cacheid},target=/var/tmp,uid=1000,gid=1000 touch /var/tmp/should-be-able-to-write
911+
EOF
912+
run_buildah build $WITH_POLICY_JSON ${contextdir}
913+
914+
# with non-default ID mappings
915+
local cacheid=${SRANDOM}
916+
cat > ${contextdir}/Dockerfile << EOF
917+
FROM alpine
918+
USER 1000:1000
919+
RUN --mount=type=cache,id=${cacheid},target=/var/tmp,uid=1000,gid=1000 stat / /var/tmp
920+
RUN --mount=type=cache,id=${cacheid},target=/var/tmp,uid=1000,gid=1000 test \`stat -c %u /var/tmp\` -eq 1000
921+
RUN --mount=type=cache,id=${cacheid},target=/var/tmp,uid=1000,gid=1000 touch /var/tmp/should-be-able-to-write
922+
EOF
923+
if test `id -u` -eq 0 ; then
924+
run_buildah build --userns-uid-map 0:1:1023 --userns-gid-map 0:1:1023 $WITH_POLICY_JSON ${contextdir}
925+
else
926+
run_buildah build --userns auto:size=1023 $WITH_POLICY_JSON ${contextdir}
927+
fi
928+
}
929+
898930
@test "build test if supplemental groups has gid with --isolation chroot" {
899-
test -z "${BUILDAH_ISOLATION}" || skip "BUILDAH_ISOLATION=${BUILDAH_ISOLATION} overrides --isolation"
931+
test "${BUILDAH_ISOLATION}" != chroot || skip "BUILDAH_ISOLATION=${BUILDAH_ISOLATION} overrides --isolation"
900932

901933
_prefetch alpine
902934
run_buildah build --isolation chroot $WITH_POLICY_JSON -t source -f $BUDFILES/supplemental-groups/Dockerfile
@@ -6676,20 +6708,17 @@ _EOF
66766708
expect_output --substring "hello"
66776709
}
66786710

6679-
# following test must fail
66806711
@test "bud-with-mount-cache-image-from-like-buildkit" {
66816712
skip_if_no_runtime
66826713
skip_if_in_container
66836714
_prefetch alpine
6684-
local contextdir=${TEST_SCRATCH_DIR}/buildkit-mount-from
6685-
cp -R $BUDFILES/buildkit-mount-from $contextdir
6715+
local contextdir=${BUDFILES}/buildkit-mount-from
66866716

66876717
# build base image which we will use as our `from`
66886718
TMPDIR=${TEST_SCRATCH_DIR} run_buildah build -t buildkitbase $WITH_POLICY_JSON -f $contextdir/Dockerfilebuildkitbase $contextdir/
66896719

66906720
# try reading something from persistent cache in a different build
6691-
TMPDIR=${TEST_SCRATCH_DIR} run_buildah 125 build -t testbud $WITH_POLICY_JSON -f $contextdir/Dockerfilecachefromimage
6692-
expect_output --substring "no stage or additional build context found with name buildkitbase"
6721+
TMPDIR=${TEST_SCRATCH_DIR} run_buildah build -t testbud $WITH_POLICY_JSON -f $contextdir/Dockerfilecachefromimage
66936722
}
66946723

66956724
@test "bud-with-mount-cache-multiple-from-like-buildkit" {

‎tests/bud/buildkit-mount-from/Dockerfilecachefromimage

+4-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
FROM alpine
22
RUN mkdir /test
33
# use another image as cache source
4-
# following should fail as cache does not supports mounting image
54
RUN --mount=type=cache,from=buildkitbase,target=/test cat /test/hello
5+
# should be able to "write" to the cache
6+
RUN --mount=type=cache,from=buildkitbase,target=/test rm /test/hello
7+
# should be able to "write" to the cache again, since that last write was discarded
8+
RUN --mount=type=cache,from=buildkitbase,target=/test rm /test/hello

0 commit comments

Comments
 (0)