ostree-ext: Match on diff_ids as well as layer digest on import#2081
ostree-ext: Match on diff_ids as well as layer digest on import#2081alexlarsson wants to merge 1 commit intobootc-dev:mainfrom
Conversation
Layer digest can vary for a layer due to e.g. recompression, so at the start of an import we enumerate all cached images and make a diff_id to layer digest map. Then when pulling an image if the layer digest ref doesn't exist, we try to lookup in the map and use that if it exists. We also create new refs for all such reused layer commits. This fixes bootc-dev#2078 Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Alexander Larsson <alexl@redhat.com>
|
This is an alternative to #2080 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a valuable layer deduplication mechanism by mapping "diff_id"s to blob digests. This enhancement allows for more efficient reuse of existing layers, even when their digests change due to factors like recompression. The integration of this logic into the ImageImporter struct and its associated methods, such as query_layer and ensure_ref_for_layer, is well-executed. This change significantly improves storage management and import efficiency.
| map.entry(diff_id_str) | ||
| .or_insert_with(|| layer_desc.digest().to_string()); |
There was a problem hiding this comment.
The build_diffid_to_digest_map function uses or_insert_with to store only the first encountered blob_digest for a given diff_id. While this approach is valid for deduplication (as any valid digest for the content is sufficient), it might be beneficial to add a comment explaining this design choice. This clarifies that if multiple images have the same diff_id but different blob_digests (e.g., due to varying compression), only one of them will be used for future lookups. This is consistent with the PR description's goal of handling recompression.
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
I think this would be relatively easy to generate a test for in tests/it/main.rs which fetches content from OCI dirs where we can easily control this (1 layer gzip compressed, anotehr one zstd compressed) etc.
| // Try to find a layer with the same diff_id but different blob digest | ||
| if let Some(existing_digest) = self.find_digest_by_diffid(manifest, config, layer) { | ||
| let existing_ref = ref_for_blob_digest(existing_digest)?; | ||
| if let Some(existing_commit) = self |
There was a problem hiding this comment.
Pretty deep nesting here, I think we could combinator chain this to reduce nesting with .or_else(|| find_digest_by_diffid()) or so - we'd probably need to Ok wrap the first case and also .transpose()?
2 participants
Layer digest can vary for a layer due to e.g. recompression, so at the start of an import we enumerate all cached images and make a diff_id to layer digest map. Then when pulling an image if the layer digest ref doesn't exist, we try to lookup in the map and use that if it exists.
We also create new refs for all such reused layer commits.
This fixes #2078
Assisted-by: Claude Code (Sonnet 4.5)