From d76b3606fc9ca975bf436379f91105f0fac1555f Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Tue, 12 Apr 2022 10:20:26 -0400 Subject: [PATCH] Fully populate layer records for mapped image top layers When we create a copy of an image's top layer that's intended to be identical to the top layer, except for having some set of ID mappings already applied to it, copy over the template layer's compressed and uncompressed digest and size information, compression information, tar-split data, and lists of used UIDs and GIDs, if we have them. The lack of sizing information was forcing ImageSize() to regenerate the diffs to determine the size of the mapped layers, which shouldn't have been necessary. Teach the overlay DiffGetter to look for files in the diff directories of lower layers if we can't find them in the current layer, so that tar-split can retrieve content that we didn't have to pull up. Signed-off-by: Nalin Dahyabhai --- drivers/overlay/overlay.go | 29 +++++++++++++---- layers.go | 65 ++++++++++++++++++++++++++++++++------ tests/idmaps.bats | 41 +++++++++++++++--------- 3 files changed, 104 insertions(+), 31 deletions(-) diff --git a/drivers/overlay/overlay.go b/drivers/overlay/overlay.go index cea6ddaf86..c911acb5c3 100644 --- a/drivers/overlay/overlay.go +++ b/drivers/overlay/overlay.go @@ -39,7 +39,6 @@ import ( "github.com/opencontainers/selinux/go-selinux/label" "github.com/pkg/errors" "github.com/sirupsen/logrus" - "github.com/vbatts/tar-split/tar/storage" "golang.org/x/sys/unix" ) @@ -1743,11 +1742,24 @@ func (d *Driver) getWhiteoutFormat() archive.WhiteoutFormat { return whiteoutFormat } -type fileGetNilCloser struct { - storage.FileGetter +type overlayFileGetter struct { + diffDirs []string } -func (f fileGetNilCloser) Close() error { +func (g *overlayFileGetter) Get(path string) (io.ReadCloser, error) { + for _, d := range g.diffDirs { + f, err := os.Open(filepath.Join(d, path)) + if err == nil { + return f, nil + } + } + if len(g.diffDirs) > 0 { + return os.Open(filepath.Join(g.diffDirs[0], path)) + } + return nil, fmt.Errorf("%s: %w", path, os.ErrNotExist) +} + +func (g *overlayFileGetter) Close() error { return nil } @@ -1756,13 +1768,18 @@ func (d *Driver) getStagingDir() string { } // DiffGetter returns a FileGetCloser that can read files from the directory that -// contains files for the layer differences. Used for direct access for tar-split. +// contains files for the layer differences, either for this layer, or one of our +// lowers if we're just a template directory. Used for direct access for tar-split. func (d *Driver) DiffGetter(id string) (graphdriver.FileGetCloser, error) { p, err := d.getDiffPath(id) if err != nil { return nil, err } - return fileGetNilCloser{storage.NewPathFileGetter(p)}, nil + paths, err := d.getLowerDiffPaths(id) + if err != nil { + return nil, err + } + return &overlayFileGetter{diffDirs: append([]string{p}, paths...)}, nil } // CleanupStagingDirectory cleanups the staging directory. diff --git a/layers.go b/layers.go index 8a5616dfcb..5e9930ea78 100644 --- a/layers.go +++ b/layers.go @@ -725,12 +725,32 @@ func (r *layerStore) Put(id string, parentLayer *Layer, names []string, mountLab parent = parentLayer.ID } var parentMappings, templateIDMappings, oldMappings *idtools.IDMappings + var ( + templateMetadata string + templateCompressedDigest digest.Digest + templateCompressedSize int64 + templateUncompressedDigest digest.Digest + templateUncompressedSize int64 + templateCompressionType archive.Compression + templateUIDs, templateGIDs []uint32 + templateTSdata []byte + ) if moreOptions.TemplateLayer != "" { + var tserr error templateLayer, ok := r.lookup(moreOptions.TemplateLayer) if !ok { return nil, -1, ErrLayerUnknown } + templateMetadata = templateLayer.Metadata templateIDMappings = idtools.NewIDMappingsFromMaps(templateLayer.UIDMap, templateLayer.GIDMap) + templateCompressedDigest, templateCompressedSize = templateLayer.CompressedDigest, templateLayer.CompressedSize + templateUncompressedDigest, templateUncompressedSize = templateLayer.UncompressedDigest, templateLayer.UncompressedSize + templateCompressionType = templateLayer.CompressionType + templateUIDs, templateGIDs = append([]uint32{}, templateLayer.UIDs...), append([]uint32{}, templateLayer.GIDs...) + templateTSdata, tserr = ioutil.ReadFile(r.tspath(templateLayer.ID)) + if tserr != nil && !os.IsNotExist(tserr) { + return nil, -1, tserr + } } else { templateIDMappings = &idtools.IDMappings{} } @@ -775,17 +795,43 @@ func (r *layerStore) Put(id string, parentLayer *Layer, names []string, mountLab return nil, -1, err } } + if len(templateTSdata) > 0 { + if err := os.MkdirAll(filepath.Dir(r.tspath(id)), 0o700); err != nil { + // We don't have a record of this layer, but at least + // try to clean it up underneath us. + if err2 := r.driver.Remove(id); err2 != nil { + logrus.Errorf("While recovering from a failure creating in UpdateLayerIDMap, error deleting layer %#v: %v", id, err2) + } + return nil, -1, err + } + if err = ioutils.AtomicWriteFile(r.tspath(id), templateTSdata, 0o600); err != nil { + // We don't have a record of this layer, but at least + // try to clean it up underneath us. + if err2 := r.driver.Remove(id); err2 != nil { + logrus.Errorf("While recovering from a failure creating in UpdateLayerIDMap, error deleting layer %#v: %v", id, err2) + } + return nil, -1, err + } + } if err == nil { layer = &Layer{ - ID: id, - Parent: parent, - Names: names, - MountLabel: mountLabel, - Created: time.Now().UTC(), - Flags: make(map[string]interface{}), - UIDMap: copyIDMap(moreOptions.UIDMap), - GIDMap: copyIDMap(moreOptions.GIDMap), - BigDataNames: []string{}, + ID: id, + Parent: parent, + Names: names, + MountLabel: mountLabel, + Metadata: templateMetadata, + Created: time.Now().UTC(), + CompressedDigest: templateCompressedDigest, + CompressedSize: templateCompressedSize, + UncompressedDigest: templateUncompressedDigest, + UncompressedSize: templateUncompressedSize, + CompressionType: templateCompressionType, + UIDs: templateUIDs, + GIDs: templateGIDs, + Flags: make(map[string]interface{}), + UIDMap: copyIDMap(moreOptions.UIDMap), + GIDMap: copyIDMap(moreOptions.GIDMap), + BigDataNames: []string{}, } r.layers = append(r.layers, layer) r.idindex.Add(id) @@ -872,7 +918,6 @@ func (r *layerStore) Mounted(id string) (int, error) { } func (r *layerStore) Mount(id string, options drivers.MountOpts) (string, error) { - // check whether options include ro option hasReadOnlyOpt := func(opts []string) bool { for _, item := range opts { diff --git a/tests/idmaps.bats b/tests/idmaps.bats index b952e2de3f..7e21144179 100644 --- a/tests/idmaps.bats +++ b/tests/idmaps.bats @@ -560,10 +560,12 @@ load helpers esac n=5 host=2 + filelist= # Create some temporary files. for i in $(seq $n) ; do createrandom "$TESTDIR"/file$i chown ${i}:${i} "$TESTDIR"/file$i + filelist="$filelist file$i" done # Select some ID ranges. for i in $(seq $n) ; do @@ -576,24 +578,18 @@ load helpers [ "$status" -eq 0 ] [ "$output" != "" ] baselayer="$output" + # Create an empty layer blob and apply it to the layer. + dd if=/dev/zero bs=1k count=1 of="$TESTDIR"/layer.empty + run storage --debug=false applydiff -f "$TESTDIR"/layer.empty $baselayer # Create a layer using the host's mappings. run storage --debug=false create-layer --hostuidmap --hostgidmap $baselayer echo "$output" [ "$status" -eq 0 ] [ "$output" != "" ] lowerlayer="$output" - # Mount the layer. - run storage --debug=false mount $lowerlayer - echo "$output" - [ "$status" -eq 0 ] - [ "$output" != "" ] - lowermount="$output" - # Copy the files in (host mapping, so it's fine), and set ownerships on them. - cp -p "$TESTDIR"/file1 ${lowermount} - cp -p "$TESTDIR"/file2 ${lowermount} - cp -p "$TESTDIR"/file3 ${lowermount} - cp -p "$TESTDIR"/file4 ${lowermount} - cp -p "$TESTDIR"/file5 ${lowermount} + # Create a layer blob containing the files and apply it to the layer. + tar --directory "$TESTDIR" -cvf "$TESTDIR"/layer.tar $filelist + run storage --debug=false applydiff -f "$TESTDIR"/layer.tar $lowerlayer # Create an image record for this layer. run storage --debug=false create-image $lowerlayer echo "$output" @@ -679,15 +675,30 @@ load helpers echo nparents:$nparents [ $nparents -eq $n ] - # The image should have five top layers at this point. + # The image should have five top layers at this point, they should all + # have known sizes, and we should be able to diff them all. run storage --debug=false image $image echo "$output" [ "$status" -eq 0 ] [ "$output" != "" ] tops=$(grep '^Top Layer:' <<< "$output" | sed 's,^Top Layer: ,,g') + echo tops: "$tops" ntops=$(for p in $tops; do echo $p ; done | sort -u | wc -l) echo ntops:$ntops [ $ntops -eq $n ] + for p in $tops; do + rm -f "$TESTDIR"/diff.tar + storage --debug=false diff -u -f "$TESTDIR"/diff.tar "$p" + test -s "$TESTDIR"/diff.tar + expected=$(storage --debug=false layer --json $p | sed -r -e 's|.*"diff-size":([^",]*).*|\1|g') + actual=$(stat -c %s "$TESTDIR"/diff.tar) + echo expected diff size "$expected", got "$actual" + test $actual = $expected + expected=$(storage --debug=false layer --json $p | sed -r -e 's|.*"diff-digest":"?([^",]*).*|\1|g') + actual=sha256:$(sha256sum "$TESTDIR"/diff.tar | sed -e 's, .*,,g') + echo expected diff digest "$expected", got "$actual" + test $actual = $expected + done # Remove the containers and image and check that all of the layers we used got removed. for container in "${containers[@]}" ; do @@ -702,10 +713,10 @@ load helpers @test "idmaps-create-mapped-container" { case "$STORAGE_DRIVER" in - btrfs|devicemapper|overlay*|vfs|zfs) + overlay*|vfs) ;; *) - skip "not supported by driver $STORAGE_DRIVER" + skip "${STORAGE_DRIVER}.imagestore option not supported by driver ${STORAGE_DRIVER}" ;; esac case "$STORAGE_OPTION" in