preserve original compressed layer blobs alongside extracted content - #1012
preserve original compressed layer blobs alongside extracted content#1012sohankunkerkar wants to merge 2 commits into
Conversation
github-actions
Bot
added
storage
There was a problem hiding this comment.
Pull request overview
Adds an opt-in mechanism to preserve the original compressed layer blob bytes alongside the unpacked layer contents in storage (currently supported by the overlay driver), and wires this through image pull configuration so consumers (e.g., CRI-O) can enable it.
Changes:
- Introduces
LayerOptions.PreserveCompressedBloband plumbs it through the layer apply/extract pipeline to tee and persist the incoming compressed stream. - Exposes
Store.CompressedBlobPath(layerID)to retrieve the on-disk path to the preserved blob (or""when unavailable). - Adds overlay-driver support (
CompressedBlobDir) and unit tests covering applyDiff blob preservation behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/store.go | Adds Store.CompressedBlobPath and LayerOptions.PreserveCompressedBlob, plus store implementation to locate preserved blobs. |
| storage/layers.go | Implements blob preservation in applyDiff, stages/copies blob for unlocked staging path, and defines driver capability interface. |
| storage/drivers/overlay/overlay.go | Implements the optional CompressedBlobDir capability for overlay. |
| storage/compressed_blob_test.go | Adds unit tests verifying preserve/disable/uncompressed/error cases for blob preservation in applyDiff. |
| image/types/types.go | Adds SystemContext.PreserveCompressedBlobs to enable feature during image pulls. |
| image/storage/storage_dest.go | Plumbs SystemContext.PreserveCompressedBlobs into LayerOptions.PreserveCompressedBlob during layer creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…acted content Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
94aae67 to
b459d3d
Compare
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
First see #1011 about prioritization / the whole idea (summary: I’m pretty strongly opposed).
What about all the other ways a layer can be created, like partial pulls (where the puller ideally never sees the full uncompressed blob), or ALS (possibly on-demand) mounts/accesses?
This is an extremely narrow combination of factors that actually records data (and if anything does not match, silently nothing happens); the API does not give users any kind of reliable way to achieve recording the data — presumably that actually matters to the user.
| // fileutils.CopyFile is used instead of os.Rename because the staging dir | ||
| // (overlay-layers/tmp/) and the layer dir (overlay/<id>/) may be on different filesystems. |
There was a problem hiding this comment.
?! Can’t possibly be the case.
| } | ||
| r.applyDiffResultToLayer(layer, contents.stagedLayerExtraction.result) | ||
| } else { | ||
| // The diff was not staged, apply it now here instead. |
There was a problem hiding this comment.
… and no implementation on this code path at all, either.
| if blobDir := provider.CompressedBlobDir(layer.ID); blobDir != "" { | ||
| blobDst := filepath.Join(blobDir, compressedBlobFilename) |
There was a problem hiding this comment.
So the driver specifies a directory but the generic code specifies a path within? That’s rather tightly coupled.
… and either way the poor user is left hard-coding implementation details of the overlay layout which we never publicly commit to. (I can imagine some sort of API where the driver owns the file and can provide a path when asked … that would require plumbing all through the call stack.)
|
|
||
| // compressedBlobDirProvider is an optional driver capability for preserving | ||
| // the original compressed stream alongside the extracted layer. | ||
| type compressedBlobDirProvider interface { |
There was a problem hiding this comment.
No-one will find the interface here and connect it with the driver interface / semantics. A driver can’t even unit-test its conformance to it!
| // CompressedBlobPath returns the path to the preserved compressed blob for the | ||
| // given layer. Returns "" if no blob is preserved (feature not enabled, driver | ||
| // does not support it, or the layer was extracted from an uncompressed stream). | ||
| func (s *store) CompressedBlobPath(id string) string { |
There was a problem hiding this comment.
Oh and this has a fundamental design problem: There can be several compressed representations of the same uncompressed layer.
That potentially multiplies the cost of storing the compressed versions; and the caller would need a fairly specific API to allow matching the expected digests. (The caller isn’t even told the digest in this version.)
Fixes #1011