[Additional Layer Store] Use TOCDigest as ID of each layer (patch for c/image) #2416
Conversation
ktock
force-pushed
the
store-tocdigest-id
branch
2 times, most recently
from
76644c2
to
773cb67
Compare
|
@ktock You're fighting some lint errors on this one. |
|
Thanks for the comment. Fixed the linter error. |
| if err != nil && !errors.Is(err, storage.ErrLayerUnknown) { | ||
| return false, private.ReusedBlob{}, fmt.Errorf(`looking for compressed layers with digest %q and labels: %w`, blobDigest, err) | ||
| } else if err == nil { | ||
| s.lockProtected.blobDiffIDs[blobDigest] = aLayer.UncompressedDigest() | ||
| s.lockProtected.blobAdditionalLayer[blobDigest] = aLayer | ||
| d := aLayer.TOCDigest() |
There was a problem hiding this comment.
We have passed down the expected TOC digest in LookupAdditionalLayer. Would this ever return a different value? If so, what does that mean / what is the guaranteed relationship between the two TOCs?
There was a problem hiding this comment.
Looking at the implementation in containerd/stargz-snapshotter#1673 , this seems to always return the digest that is used in the FUSE request, i.e. it is exactly the value we passed in … unless the FUSE server is old and does not return the value at all; and in that case we don’t want to use any layers in the ALS — the FUSE server would be looking up a layer with compressed (non-TOC) digest options.TOCDigest.
There was a problem hiding this comment.
#2428 modifies this to only trust options.TOCDigest (but to require the one from the ALS to match).
| s.lockProtected.blobDiffIDs[blobDigest] = aLayer.UncompressedDigest() | ||
| s.lockProtected.blobAdditionalLayer[blobDigest] = aLayer | ||
| d := aLayer.TOCDigest() | ||
| if d == "" { |
There was a problem hiding this comment.
When can this happen? Wouldn’t it be appropriate to pull the layer in the ordinary way, instead of failing?
There was a problem hiding this comment.
Per the other comment, this can happen if the FUSE server is old and not expecting TOCs at all.
| return -1, fmt.Errorf("size for layer %q is unknown, failing getSize()", layerID) | ||
| } | ||
| if layer.UncompressedSize < 0 { | ||
| sum = 0 |
There was a problem hiding this comment.
This means that if there 3 layers, and the middle one has an unknown size, the total returned size of the image is… some value larger than the true size, but the particular value can AFAICS not be reasonably used for anything.
Why is that better than returning -1 to indicate “unknown”, or than returning an error? (One possible answer might be “Podman callers assume this always succeeds and returns a non--1 value, I didn’t check whether that’s the case. But I think, if that were true, the callers would need to be updated to deal with images where the size is unknown, instead.)
There was a problem hiding this comment.
#2428 modifies this to treat the unknown-size layer as zero-size, while returning the size of the other components.
| return -1, fmt.Errorf("size for layer %q is unknown, failing getSize()", layerID) | ||
| } | ||
| if layer.UncompressedSize < 0 { |
There was a problem hiding this comment.
If a TOC-identified layer can have UncompressedSize == -1, that breaks more assumptions. At least the LayersByTOCDigest path in tryReusingBlobAsPending is using the UncompressedSize value, and the value can’t be “unknown” on that path.
… but, also, note to self: is a new behavior of the AdditionalLayerStore code? We did previously assume that the blobDigest-identified ALS layers did have a valid UncompressedSize. Was that always broken?
There was a problem hiding this comment.
The commit says
Layers from additional layer store possibly doesn't know the uncompressed size until it fully downloads the entire contents to the node.
but we collect all metadata at the time of layer creation (layerStore.PutAdditionalLayer reads AdditionalLayer.Info for that), and don’t update it afterwards. Does that mean that the UncompressedSize is typically not available?
Actually, in https://github.com/containerd/stargz-snapshotter/pull/1673/files , it is now reported always as -1.
There was a problem hiding this comment.
At least the
LayersByTOCDigestpath intryReusingBlobAsPendingis using theUncompressedSizevalue, and the value can’t be “unknown” on that path.
That’s actually fine because that is already conditional on UncompressedDigest != "", and that’s false for recent ALS layers.
There was a problem hiding this comment.
… and I don’t know whether UncompressedSize was always returned with the older version of the FUSE server, but that’s not a concern going forward, anyway.
Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
Layers from additional layer store possibly doesn't know the uncompressed size until it fully downloads the entire contents to the node. Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
mtrmac
force-pushed
the
store-tocdigest-id
branch
from
a1e2fff
to
52101a0
Compare
There was a problem hiding this comment.
Merging now because otherwise c/image does not compile after the c/storage API change; and because, as of today, containerd/stargz-snapshotter#1673 is not merged, so we actually never create ALS layers after merging this, and most of the concerns become hypothetical (in the short term).
I’ll immediately follow up with a PR that mitigates some of the concerns, or at least adds explicit FIXMEs. That’s not to preclude any other work on improving those aspects, just to get us back to a production-ready state.
Needs: containers/storage#1924
This commit changes the store implementation to use TOCDigset as the ID of each layer. This ensures the runtime to pass TOCDigest to the FUSE backend everytime requiring a layer.
This commit modifies c/storage's
LookupAdditionalLayerto receive a TOCDigest as one of the arguments. Then that TOCDigest is passed to Additional Layer Store as one of the path elements of the FUSE fs. For example, the extracted view of a layer diff contents is provided in the following path:Additional Layer Store implementation attempts lazy pulling of the layer that has the specified TOCDigest in the image.
If that image doesn't contain the layer matching to the TOCDigest, accessing to the layer directly results in an error.
Example draft implementation of Additional Layer Store is at containerd/stargz-snapshotter#1673