From 0a53800a735e196fa1ee836ae6f64f20892cae89 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Mon, 9 Sep 2024 17:02:51 -0400 Subject: [PATCH] imagebuildah.StageExecutor: clean up volumes/volumeCache Clean up the distinctions between the volumes slice and the volumeCache and volumeCacheInfo maps so that --compat-volumes will work correctly when we're building with multiple layers. Signed-off-by: Nalin Dahyabhai --- cmd/buildah/build.go | 3 +- imagebuildah/build.go | 2 +- imagebuildah/executor.go | 2 +- imagebuildah/stage_executor.go | 115 +++++++++++--------------- tests/bud.bats | 76 ++++++++++++----- tests/bud/preserve-volumes/Dockerfile | 20 +++-- tests/bud/volume-perms/Dockerfile | 8 +- 7 files changed, 123 insertions(+), 103 deletions(-) diff --git a/cmd/buildah/build.go b/cmd/buildah/build.go index 47251a94208..290bc0f9802 100644 --- a/cmd/buildah/build.go +++ b/cmd/buildah/build.go @@ -1,6 +1,7 @@ package main import ( + "fmt" "os" "github.com/containers/buildah/imagebuildah" @@ -73,7 +74,7 @@ func buildCmd(c *cobra.Command, inputArgs []string, iopts buildahcli.BuildOption if c.Flag("logfile").Changed { logfile, err := os.OpenFile(iopts.Logfile, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0o600) if err != nil { - return err + return fmt.Errorf("opening log file: %w", err) } iopts.Logwriter = logfile defer iopts.Logwriter.Close() diff --git a/imagebuildah/build.go b/imagebuildah/build.go index d545c83d794..7384e8acd4f 100644 --- a/imagebuildah/build.go +++ b/imagebuildah/build.go @@ -143,7 +143,7 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options define.B } contents, err = os.Open(dfile) if err != nil { - return "", nil, err + return "", nil, fmt.Errorf("reading build instructions: %w", err) } dinfo, err = contents.Stat() if err != nil { diff --git a/imagebuildah/executor.go b/imagebuildah/executor.go index f78b191e232..b2526d0390b 100644 --- a/imagebuildah/executor.go +++ b/imagebuildah/executor.go @@ -223,7 +223,7 @@ func newExecutor(logger *logrus.Logger, logPrefix string, store storage.Store, o } else { rusageLogFile, err = os.OpenFile(options.RusageLogFile, os.O_TRUNC|os.O_CREATE|os.O_WRONLY, 0o644) if err != nil { - return nil, err + return nil, fmt.Errorf("creating file to store rusage logs: %w", err) } } } diff --git a/imagebuildah/stage_executor.go b/imagebuildah/stage_executor.go index 60d5a9a9ead..320eca1b3dd 100644 --- a/imagebuildah/stage_executor.go +++ b/imagebuildah/stage_executor.go @@ -67,9 +67,9 @@ type StageExecutor struct { name string builder *buildah.Builder preserved int - volumes imagebuilder.VolumeSet - volumeCache map[string]string - volumeCacheInfo map[string]os.FileInfo + volumes imagebuilder.VolumeSet // list of directories which are volumes + volumeCache map[string]string // mapping from volume directories to cache archives (used by vfs method) + volumeCacheInfo map[string]os.FileInfo // mapping from volume directories to perms/datestamps to reset after restoring mountPoint string output string containerIDs []string @@ -92,7 +92,7 @@ type StageExecutor struct { // writeable while the RUN instruction is being handled, even if any changes // made within the directory are ultimately discarded. func (s *StageExecutor) Preserve(path string) error { - logrus.Debugf("PRESERVE %q in %q", path, s.builder.ContainerID) + logrus.Debugf("PRESERVE %q in %q (already preserving %v)", path, s.builder.ContainerID, s.volumes) // Try and resolve the symlink (if one exists) // Set archivedPath and path based on whether a symlink is found or not @@ -111,71 +111,61 @@ func (s *StageExecutor) Preserve(path string) error { return fmt.Errorf("evaluating path %q: %w", path, err) } + // Whether or not we're caching and restoring the contents of this + // directory, we need to ensure it exists now. const createdDirPerms = os.FileMode(0o755) - if s.executor.compatVolumes != types.OptionalBoolTrue { - logrus.Debugf("ensuring volume path %q exists", path) + st, err := os.Stat(archivedPath) + if errors.Is(err, os.ErrNotExist) { + // Yup, we do have to create it. That means it's not in any + // cached copy of the path that covers it, so we have to + // invalidate such cached copy. + logrus.Debugf("have to create volume %q", path) createdDirPerms := createdDirPerms if err := copier.Mkdir(s.mountPoint, archivedPath, copier.MkdirOptions{ChmodNew: &createdDirPerms}); err != nil { return fmt.Errorf("ensuring volume path exists: %w", err) } - logrus.Debugf("not doing volume save-and-restore of %q in %q", path, s.builder.ContainerID) - return nil + if err := s.volumeCacheInvalidate(path); err != nil { + return fmt.Errorf("ensuring volume path %q is preserved: %w", filepath.Join(s.mountPoint, path), err) + } + if st, err = os.Stat(archivedPath); err != nil { + return fmt.Errorf("checking on just-created volume path: %w", err) + } + } + if err != nil { + return fmt.Errorf("reading info cache for volume at %q: %w", path, err) } if s.volumes.Covers(path) { // This path is a subdirectory of a volume path that we're - // already preserving, so there's nothing new to be done except - // ensure that it exists. - st, err := os.Stat(archivedPath) - if errors.Is(err, os.ErrNotExist) { - // We do have to create it. That means it's not in any - // cached copy of the path that covers it, so we have - // to invalidate such cached copy. - logrus.Debugf("have to create volume %q", path) - createdDirPerms := createdDirPerms - if err := copier.Mkdir(s.mountPoint, filepath.Join(s.mountPoint, path), copier.MkdirOptions{ChmodNew: &createdDirPerms}); err != nil { - return fmt.Errorf("ensuring volume path exists: %w", err) - } - if err := s.volumeCacheInvalidate(path); err != nil { - return fmt.Errorf("ensuring volume path %q is preserved: %w", filepath.Join(s.mountPoint, path), err) - } - if st, err = os.Stat(archivedPath); err != nil { - return fmt.Errorf("checking on just-created volume path: %w", err) - } - } + // already preserving, so there's nothing new to be done now + // that we've ensured that it exists. s.volumeCacheInfo[path] = st return nil } - // Figure out where the cache for this volume would be stored. - s.preserved++ - cacheDir, err := s.executor.store.ContainerDirectory(s.builder.ContainerID) - if err != nil { - return fmt.Errorf("unable to locate temporary directory for container") - } - cacheFile := filepath.Join(cacheDir, fmt.Sprintf("volume%d.tar", s.preserved)) - - // Save info about the top level of the location that we'll be archiving. - st, err := os.Stat(archivedPath) - if errors.Is(err, os.ErrNotExist) { - logrus.Debugf("have to create volume %q", path) - createdDirPerms := os.FileMode(0o755) - if err = copier.Mkdir(s.mountPoint, archivedPath, copier.MkdirOptions{ChmodNew: &createdDirPerms}); err != nil { - return fmt.Errorf("ensuring volume path exists: %w", err) - } - st, err = os.Stat(archivedPath) - } - if err != nil { - logrus.Debugf("error reading info about %q: %v", archivedPath, err) - return err - } - s.volumeCacheInfo[path] = st + // Add the new volume path to the ones that we're tracking. if !s.volumes.Add(path) { // This path is not a subdirectory of a volume path that we're // already preserving, so adding it to the list should have // worked. return fmt.Errorf("adding %q to the volume cache", path) } + s.volumeCacheInfo[path] = st + + // If we're not doing save/restore, we're done, since volumeCache + // should be empty. + if s.executor.compatVolumes != types.OptionalBoolTrue { + logrus.Debugf("not doing volume save-and-restore of %q in %q", path, s.builder.ContainerID) + return nil + } + + // Decide where the cache for this volume will be stored. + s.preserved++ + cacheDir, err := s.executor.store.ContainerDirectory(s.builder.ContainerID) + if err != nil { + return fmt.Errorf("unable to locate temporary directory for container") + } + cacheFile := filepath.Join(cacheDir, fmt.Sprintf("volume%d.tar", s.preserved)) s.volumeCache[path] = cacheFile // Now prune cache files for volumes that are newly supplanted by this one. @@ -206,7 +196,7 @@ func (s *StageExecutor) Preserve(path string) error { if errors.Is(err, os.ErrNotExist) { continue } - return err + return fmt.Errorf("removing cache of %q: %w", archivedPath, err) } delete(s.volumeCache, cachedPath) } @@ -256,16 +246,12 @@ func (s *StageExecutor) volumeCacheSaveVFS() (mounts []specs.Mount, err error) { continue } if !errors.Is(err, os.ErrNotExist) { - return nil, err - } - createdDirPerms := os.FileMode(0o755) - if err := copier.Mkdir(s.mountPoint, archivedPath, copier.MkdirOptions{ChmodNew: &createdDirPerms}); err != nil { - return nil, fmt.Errorf("ensuring volume path exists: %w", err) + return nil, fmt.Errorf("checking for presence of a cached copy of %q at %q: %w", cachedPath, cacheFile, err) } logrus.Debugf("caching contents of volume %q in %q", archivedPath, cacheFile) cache, err := os.Create(cacheFile) if err != nil { - return nil, err + return nil, fmt.Errorf("creating cache for volume %q: %w", archivedPath, err) } defer cache.Close() rc, err := chrootarchive.Tar(archivedPath, nil, s.mountPoint) @@ -298,16 +284,12 @@ func (s *StageExecutor) volumeCacheRestoreVFS() (err error) { logrus.Debugf("restoring contents of volume %q from %q", archivedPath, cacheFile) cache, err := os.Open(cacheFile) if err != nil { - return err + return fmt.Errorf("restoring contents of volume %q: %w", archivedPath, err) } defer cache.Close() if err := copier.Remove(s.mountPoint, archivedPath, copier.RemoveOptions{All: true}); err != nil { return err } - createdDirPerms := os.FileMode(0o755) - if err := copier.Mkdir(s.mountPoint, archivedPath, copier.MkdirOptions{ChmodNew: &createdDirPerms}); err != nil { - return err - } err = chrootarchive.Untar(cache, archivedPath, nil) if err != nil { return fmt.Errorf("extracting archive at %q: %w", archivedPath, err) @@ -334,13 +316,11 @@ func (s *StageExecutor) volumeCacheRestoreVFS() (err error) { } // Save the contents of each of the executor's list of volumes for which we -// don't already have a cache file. +// don't already have a cache file. For overlay, we "save" and "restore" by +// using it as a lower for an overlay mount in the same location, and then +// discarding the upper. func (s *StageExecutor) volumeCacheSaveOverlay() (mounts []specs.Mount, err error) { for cachedPath := range s.volumeCache { - err = copier.Mkdir(s.mountPoint, filepath.Join(s.mountPoint, cachedPath), copier.MkdirOptions{}) - if err != nil { - return nil, fmt.Errorf("ensuring volume exists: %w", err) - } volumePath := filepath.Join(s.mountPoint, cachedPath) mount := specs.Mount{ Source: volumePath, @@ -1089,6 +1069,7 @@ func (s *StageExecutor) prepare(ctx context.Context, from string, initializeIBCo s.mountPoint = mountPoint s.builder = builder // Now that the rootfs is mounted, set up handling of volumes from the base image. + s.volumes = make([]string, 0, len(s.volumes)) s.volumeCache = make(map[string]string) s.volumeCacheInfo = make(map[string]os.FileInfo) for _, v := range builder.Volumes() { diff --git a/tests/bud.bats b/tests/bud.bats index dea9ccb21a0..240a6b88641 100644 --- a/tests/bud.bats +++ b/tests/bud.bats @@ -2468,18 +2468,33 @@ _EOF skip_if_no_runtime _prefetch alpine - target=volume-image - run_buildah build $WITH_POLICY_JSON -t ${target} --compat-volumes $BUDFILES/preserve-volumes - run_buildah from --quiet ${target} - cid=$output - run_buildah mount ${cid} - root=$output - test -s $root/vol/subvol/subsubvol/subsubvolfile - test ! -s $root/vol/subvol/subvolfile - test -s $root/vol/volfile - test -s $root/vol/Dockerfile - test -s $root/vol/Dockerfile2 - test ! -s $root/vol/anothervolfile + for layers in "" --layers ; do + for compat in "" --compat-volumes ; do + target=volume-image$compat$layers + run_buildah build $WITH_POLICY_JSON -t ${target} ${layers} ${compat} $BUDFILES/preserve-volumes + run_buildah from --quiet ${target} + cid=$output + run_buildah mount ${cid} + root=$output + # these files were created before VOLUME instructions froze the directories that contained them + test -s $root/vol/subvol/subsubvol/subsubvolfile + test -s $root/vol/volfile + if test "$compat" != "" ; then + # true, these files should have been discarded after they were created by RUN instructions + test ! -s $root/vol/subvol/subvolfile + test ! -s $root/vol/anothervolfile + else + # false, these files should not have been discarded, despite being created by RUN instructions + test -s $root/vol/subvol/subvolfile + test -s $root/vol/anothervolfile + fi + # and these were ADDed + test -s $root/vol/Dockerfile + test -s $root/vol/Dockerfile2 + run_buildah rm ${cid} + run_buildah rmi ${target} + done + done } # Helper function for several of the tests which pull from http. @@ -2701,16 +2716,33 @@ function validate_instance_compression { skip_if_no_runtime _prefetch alpine - target=volume-image - run_buildah build $WITH_POLICY_JSON -t ${target} --compat-volumes=true $BUDFILES/volume-perms - run_buildah from --quiet $WITH_POLICY_JSON ${target} - cid=$output - run_buildah mount ${cid} - root=$output - test ! -s $root/vol/subvol/subvolfile - run stat -c %f $root/vol/subvol - assert "$status" -eq 0 "status code from stat $root/vol/subvol" - expect_output "41ed" "stat($root/vol/subvol) [0x41ed = 040755]" + for layers in "" --layers ; do + for compat in "" --compat-volumes ; do + target=volume-image$compat$layers + run_buildah build $WITH_POLICY_JSON -t ${target} ${layers} ${compat} $BUDFILES/volume-perms + run_buildah from --quiet $WITH_POLICY_JSON ${target} + cid=$output + run_buildah mount ${cid} + root=$output + if test "$compat" != "" ; then + # true, /vol/subvol should not have contents, and its permissions should be the default 0755 + test -d $root/vol/subvol + test ! -s $root/vol/subvol/subvolfile + run stat -c %a $root/vol/subvol + assert "$status" -eq 0 "status code from stat $root/vol/subvol" + expect_output "755" "stat($root/vol/subvol)" + else + # true, /vol/subvol should have contents, and its permissions should be the changed 0711 + test -d $root/vol/subvol + test -s $root/vol/subvol/subvolfile + run stat -c %a $root/vol/subvol + assert "$status" -eq 0 "status code from stat $root/vol/subvol" + expect_output "711" "stat($root/vol/subvol)" + fi + run_buildah rm ${cid} + run_buildah rmi ${target} + done + done } @test "bud-volume-ownership" { diff --git a/tests/bud/preserve-volumes/Dockerfile b/tests/bud/preserve-volumes/Dockerfile index df959086d68..ec1fb36e4c2 100644 --- a/tests/bud/preserve-volumes/Dockerfile +++ b/tests/bud/preserve-volumes/Dockerfile @@ -2,23 +2,27 @@ FROM alpine RUN mkdir -p /vol/subvol/subsubvol RUN dd if=/dev/zero bs=512 count=1 of=/vol/subvol/subsubvol/subsubvolfile VOLUME /vol/subvol -# At this point, the contents below /vol/subvol should be frozen. +# At this point, the contents below /vol/subvol may be frozen, so try to create +# something that will be discarded if it was. RUN dd if=/dev/zero bs=512 count=1 of=/vol/subvol/subvolfile -# In particular, /vol/subvol/subvolfile should be wiped out. +# In particular, /vol/subvol/subvolfile should be wiped out if --compat-volumes +# behavior was selected. RUN dd if=/dev/zero bs=512 count=1 of=/vol/volfile -# However, /vol/volfile should exist. +# However, /vol/volfile should always exist, since /vol was not a volume, but +# we're making it one here. VOLUME /vol # And this should be redundant. VOLUME /vol/subvol -# And now we've frozen /vol. +# And now that we've frozen /vol, --compat-volumes should make this disappear, +# too. RUN dd if=/dev/zero bs=512 count=1 of=/vol/anothervolfile -# Which means that in the image we're about to commit, /vol/anothervolfile -# shouldn't exist, either. -# ADD files which should persist. +# ADD files which should persist, regardless of the --compat-volumes setting. ADD Dockerfile /vol/Dockerfile RUN stat /vol/Dockerfile ADD Dockerfile /vol/Dockerfile2 RUN stat /vol/Dockerfile2 -# We should still be saving and restoring volume caches. + +# This directory should still exist, since we cached /vol once it was declared +# a VOLUME, and /vol/subvol was created before that (as a VOLUME, but still). RUN dd if=/dev/zero bs=512 count=1 of=/vol/subvol/subvolfile diff --git a/tests/bud/volume-perms/Dockerfile b/tests/bud/volume-perms/Dockerfile index 4dced773817..2690444cea1 100644 --- a/tests/bud/volume-perms/Dockerfile +++ b/tests/bud/volume-perms/Dockerfile @@ -1,6 +1,8 @@ FROM alpine VOLUME /vol/subvol -# At this point, the directory should exist, with default permissions 0755, the -# contents below /vol/subvol should be frozen, and we shouldn't get an error -# from trying to write to it because we it was created automatically. +# At this point, the directory should exist, and it should have default +# permissions 0755, and we shouldn't get an error from trying to write to it +# because we it was created automatically. If this image is built with the +# --compat-volumes flag, everything done after this point will be discarded. +RUN chmod 0711 /vol/subvol RUN dd if=/dev/zero bs=512 count=1 of=/vol/subvol/subvolfile