From c0e374824ad1fd6f2c54abbf8194ac9de1681a93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 13 Feb 2024 01:32:00 +0100 Subject: [PATCH] Actually make reusing layers found by TOC work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Look up the layer by TOC; and don't abort when diffID is not around. We could, instead, look up the layers only in tryReusingBlobAsPending, and record the layer metadata at that time. That would be simpler, but it would also widen a race with concurrent image pulls/deletions: the current code can find one layer (ChainID) to reuse, and when that layer is deleted, it can find some other layer (ChainID) to actually consume. The time between tryReusingBlobAsPending and createNewLayer can be fairly significant, so opening a ~determinitic race singificantly more might lead to reproducible issues. Even if anyone encountering such issues has fundamental workflow problems that should be fixed. Signed-off-by: Miloslav Trmač --- storage/storage_dest.go | 45 ++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/storage/storage_dest.go b/storage/storage_dest.go index 8a85a63916..eb92b036d7 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -806,45 +806,48 @@ func (s *storageImageDestination) createNewLayer(index int, layerDigest digest.D return layer, nil } - s.lock.Lock() - diffID, ok := s.lockProtected.blobDiffIDs[layerDigest] - s.lock.Unlock() - if !ok { - return nil, fmt.Errorf("failed to find diffID for layer: %q", layerDigest) - } - optionalDiffID := diffID // FIXME - // Check if we previously cached a file with that blob's contents. If we didn't, // then we need to read the desired contents from a layer. var trustedUncompressedDigest, trustedOriginalDigest digest.Digest // For storage.LayerOptions s.lock.Lock() + tocDigest := s.lockProtected.indexToTOCDigest[index] // "" if not set + optionalDiffID := s.lockProtected.blobDiffIDs[layerDigest] // "" if not set filename, gotFilename := s.lockProtected.filenames[layerDigest] - _, layerIdentifiedByTOC := s.lockProtected.indexToTOCDigest[index] s.lock.Unlock() - if gotFilename && !layerIdentifiedByTOC { - // If layerIdentifiedByTOC, if we now happen to find a layerDigest match, the newLayerID has already been computed as TOC-based, + if gotFilename && tocDigest == "" { + // If tocDigest != "", if we now happen to find a layerDigest match, the newLayerID has already been computed as TOC-based, // and we don't know the relationship of the layerDigest and TOC digest. // We could recompute newLayerID to be DiffID-based and use the file, but such a within-image layer // reuse is expected to be pretty rare; instead, ignore the unexpected file match and proceed to the // originally-planned TOC match. - // Because !layerIdentifiedByTOC, optionaldiffID must have been set; and even if it weren’t, PutLayer will recompute the digest from the stream. + // Because tocDigest == "", optionaldiffID must have been set; and even if it weren’t, PutLayer will recompute the digest from the stream. trustedUncompressedDigest = optionalDiffID trustedOriginalDigest = layerDigest // The code setting .filenames[layerDigest] is responsible for the contents matching. } else { - // Try to find the layer with contents matching that blobsum. + // Try to find the layer with contents matching the data we use. var layer *storage.Layer // = nil - layers, err2 := s.imageRef.transport.store.LayersByUncompressedDigest(diffID) - if err2 == nil && len(layers) > 0 { - layer = &layers[0] + if tocDigest != "" { + layers, err2 := s.imageRef.transport.store.LayersByTOCDigest(tocDigest) + if err2 == nil && len(layers) > 0 { + layer = &layers[0] + } else { + return nil, fmt.Errorf("locating layer for TOC digest %q: %w", tocDigest, err2) + } } else { - layers, err2 = s.imageRef.transport.store.LayersByCompressedDigest(layerDigest) + // Because tocDigest == "", optionaldiffID must have been set + layers, err2 := s.imageRef.transport.store.LayersByUncompressedDigest(optionalDiffID) if err2 == nil && len(layers) > 0 { layer = &layers[0] + } else { + layers, err2 = s.imageRef.transport.store.LayersByCompressedDigest(layerDigest) + if err2 == nil && len(layers) > 0 { + layer = &layers[0] + } + } + if layer == nil { + return nil, fmt.Errorf("locating layer for blob %q: %w", layerDigest, err2) } - } - if layer == nil { - return nil, fmt.Errorf("locating layer for blob %q: %w", layerDigest, err2) } // Read the layer's contents. noCompression := archive.Uncompressed @@ -876,7 +879,7 @@ func (s *storageImageDestination) createNewLayer(index int, layerDigest digest.D } // FIXME CLEAN THIS UP - if !layerIdentifiedByTOC { + if tocDigest == "" { // If we found the layer using LayersByUncompressedDigest(diffID): // - trustedUncompressedDigest = diffID by definition, and diffID is set // - trustedOriginalDigest = layerDigest is trusted by construction of blobDiffIDs (probably from BlobInfoCache, which is trusted)