Preserve layer annotations from parent images during commit - #6972
Preserve layer annotations from parent images during commit#6972simek-m wants to merge 1 commit into
Conversation
|
|
||
| @test "commit preserves layer annotations" { | ||
| # Build a minimal OCI image with layer annotations. | ||
| mkdir -p $TEST_SCRATCH_DIR/blobs/sha256 |
There was a problem hiding this comment.
I wanted to use quay.io/fedora/fedora-bootc, but I've been running into resource limitations - the image is large and the test was slow. Unfortunately, I couldn't find a smaller existing image with layer annotations.
| ctx := context.TODO() | ||
| graphDriverName := os.Getenv("STORAGE_DRIVER") | ||
| if graphDriverName == "" { | ||
| graphDriverName = "vfs" | ||
| } | ||
| t.Logf("using storage driver %q", graphDriverName) | ||
| store, err := storage.GetStore(storageTypes.StoreOptions{ | ||
| RunRoot: t.TempDir(), | ||
| GraphRoot: t.TempDir(), | ||
| GraphDriverName: graphDriverName, | ||
| }) | ||
| require.NoError(t, err, "initializing storage") | ||
| t.Cleanup(func() { _, err := store.Shutdown(true); assert.NoError(t, err) }) | ||
|
|
||
| // Build a source image with one layer. | ||
| b, err := NewBuilder(ctx, store, BuilderOptions{ |
There was a problem hiding this comment.
The whole setup is repeated multiple times in the tests and could be extracted. I didn't want to do it in the scope of this change.
| } | ||
|
|
||
| // Get layer annotations if present. | ||
| if len(v1Manifest.Layers) > len(b.OCIv1.RootFS.DiffIDs) { |
There was a problem hiding this comment.
I'm not confident this is a correct way of getting the annotations (well, it works), but couldn't think of anything better.
The length check should not be normally needed, but I wanted to be safe.
There was a problem hiding this comment.
I wonder if using the blob info cache (go.podman.io/image/v5/pkg/blobinfocache.DefaultCache()) to try to correlate digest values from the manifest and diffID values from the config blob would be more resilient. It would certainly be more complicated.
There was a problem hiding this comment.
Sorry, missed this comment.
Interesting, I can try to implement it to see what it practically means. It definitely looks more complicated and I'm not sure if there are cases when the cache would get a miss, while this index-based approach would work (maybe there are none).
There was a problem hiding this comment.
I tried that and it looks easier than I expected. Tests pass, so kept it with a fallback to the index-based lookup.
This should be a user-facing change, right? The user will see that the bug is no longer there.
Typo. |
50d72c3 to
15ae7d8
Compare
Thank you, fixed.
I'm not sure. Looking at past PRs, I can see both. It won't hurt so I added the change log. |
nalind
left a comment
nalind
left a comment
There was a problem hiding this comment.
This would need some documentation, and possibly a flag to revert to discarding these annotations, as --inherit-annotations offers control of those at the level of the image manifest.
The docs would also be a good place to note that these aren't (can't be?) preserved when the diffIDs for layers change, for example when --rewrite-timestamp is used, or when all layers are being squashed, as with --cw.
15ae7d8 to
85ceb8a
Compare
Thank you for the feedback. I've addressed the obvious ones. Regarding these points, I'm thinking how it should work regarding flags to control annotation preservation. With image annotations I:
I think that there should be a separate flag like
Yes, the implementation is definitely limited and it needs to be documented. |
|
The Likewise, The I don't know what the CLI would look like for exposing an ability to set or unset annotations for individual layers in a multi-layer So, shorter version of the above: a |
9825a0a to
285f4b3
Compare
Thank you for this thorough answer. It is even more complex than I initially thought. I updated this PR and added a I looked at your draft in #6322 and I think it's the way to go.
What do you think? Does it make any sense like this and would it be sufficient? |
|
Yes, I think that all makes sense. It's more than I think the original issue requested, but it should meet their needs, too. |
285f4b3 to
61ad151
Compare
| mb.addLayer(destHasher.Digest(), size, srcHasher.Digest()) | ||
|
|
||
| diffID := srcHasher.Digest() | ||
| if layerAnnotations == nil && i.layerAnnotations != nil { |
There was a problem hiding this comment.
The second check here shouldn't be necessary.
@nalind Thank you, the However, when implementing the CLI, I realized that {"ostree.components":"python3-botocore"}
{"ostree.components":"nvidia-gpu-firmware"}
{"ostree.components":"kernel-modules"}
{"ostree.components":"kernel-modules-core"}
{"ostree.components":"linux-firmware"}
{"ostree.components":"python3-libs"}
{"ostree.components":"podman"}
{"ostree.components":"atheros-firmware"}
A hacky way could allow removing a specific annotation matching a key-value pair regardless of a layer like |
c8ea96c to
d6b6021
Compare
|
I updated the PR with the I haven't implemented the |
simonbrauner
left a comment
simonbrauner
left a comment
There was a problem hiding this comment.
I added two comments.
Other than that, LGTM
| if layerAnnotations == nil { | ||
| layerAnnotations = make(map[string]string) | ||
| } |
There was a problem hiding this comment.
I'm not sure whether this could cause problems in this particular case, but wouldn't it be better practice to clone the map instead of rewriting the original, as the structure i refers to outlives that code block?
| if layerAnnotations == nil { | |
| layerAnnotations = make(map[string]string) | |
| } | |
| if layerAnnotations == nil { | |
| layerAnnotations = make(map[string]string) | |
| } else { | |
| layerAnnotations = maps.Clone(layerAnnotations) | |
| } |
There was a problem hiding this comment.
Yes, thank you, great catch.
It's cloned in other places and it should be done here too. I just did it where it's populated:
+++ b/image.go
@@ -1040,7 +1040,7 @@ func (i *containerImageRef) NewImageSource(ctx context.Context, _ *types.SystemC
layerUncompressedSize = layer.UncompressedSize
}
// Get annotations for the layer if present.
- layerAnnotations := i.layerAnnotations[layerUncompressedDigest]
+ layerAnnotations := maps.Clone(i.layerAnnotations[layerUncompressedDigest])
// We already know the digest of the contents of parent layers,
// so if this is a parent layer, and we know its digest, reuse
// its blobsum, diff ID, and size.
@@ -1223,7 +1223,7 @@ func (i *containerImageRef) NewImageSource(ctx context.Context, _ *types.SystemC
diffID := srcHasher.Digest()
if layerAnnotations == nil {
- layerAnnotations = i.layerAnnotations[diffID]
+ layerAnnotations = maps.Clone(i.layerAnnotations[diffID])
}
// Set layer annotations for the top layer.
if layerID == i.layerID && len(i.topLayerAnnotations) > 0 {(I also fixed some more instances of commited in the config.bats tests.)
| } | ||
| // Add new annotations to the last step. | ||
| for _, annotationSpec := range s.executor.annotations { | ||
| annotationk, annotationv, _ := strings.Cut(annotationSpec, "=") |
There was a problem hiding this comment.
There seems to be an introduced inconsistency between commands. Perhaps it would be a good idea to unify the behavior?
buildah config --annotation layer:key=val
Adds the annotation to a layer.
cid=$(./bin/buildah from --quiet scratch)
./bin/buildah config --annotation layer:mykey=myval "$cid"
./bin/buildah copy "$cid" /dev/null /somefile
./bin/buildah commit "$cid" repro-config
echo "=== Image Annotations ==="
./bin/buildah inspect --type=image --format '{{.Manifest}}' repro-config | jq '.annotations'
echo "=== Layer Annotations ==="
./bin/buildah inspect --type=image --format '{{.Manifest}}' repro-config | jq '.layers[].annotations'
./bin/buildah rm "$cid"
./bin/buildah rmi repro-config=== Image Annotations ===
{
"org.opencontainers.image.base.digest": "",
"org.opencontainers.image.base.name": "",
"org.opencontainers.image.created": "2026-08-10T14:09:24.9320504Z"
}
=== Layer Annotations ===
{
"mykey": "myval"
}buildah build --annotation layer:key=val
Adds the annotation to an image, treats layer: prefix as part of the key.
dir=$(mktemp -d)
echo hello > "$dir/somefile"
cat > "$dir/Containerfile" <<EOF
FROM scratch
COPY somefile /
EOF
./bin/buildah build --annotation layer:mykey=myval -t repro-build "$dir"
echo "=== Image Annotations ==="
./bin/buildah inspect --type=image --format '{{.Manifest}}' repro-build | jq '.annotations'
echo "=== Layer Annotations ==="
./bin/buildah inspect --type=image --format '{{.Manifest}}' repro-build | jq '.layers[].annotations'
./bin/buildah rmi repro-build
rm -rf "$dir"=== Image Annotations ===
{
"layer:mykey": "myval",
"org.opencontainers.image.base.digest": "",
"org.opencontainers.image.base.name": "",
"org.opencontainers.image.created": "2026-08-10T14:12:17.63929102Z"
}
=== Layer Annotations ===
nullThere was a problem hiding this comment.
Thank you, that's a very good point and you're right that this is a confusing behavior. Unfortunately, there are other inconsistencies too.
I'm afraid it's a bit more tricky with buildah build, though. The layer: prefix in buildah config refers to the top-most layer and it makes sense there.
However, there can be multiple layers with buildah build and IMHO the options are:
- Document properly that it's not supported
- Warn if the
layer:prefix is used (documentation too) - Refer only to the top-most layer for the annotations like with
config - Do it properly and let the user to refer to specific layers.
The last option (discussed in this PR) is most complex, because it's less clear how to design the CLI for it to be user-friendly (layer[idx]: or layer[digest]:) and I wanted to leave it out of scope of this PR. I think it's quite a niche feature and I'm not that sure of actual use cases needed to support.
@nalind What do you think?
When building an image using FROM with a source that contains layer annotations, the final committed image loses those annotations. Propagate the per-layer annotations to addLayer() for OCI images to be present in the final committed image. The annotations are only preserved if the layer content doesn't change as their lookup relies on the layer uncompressed digest not changing. Introduce a new flag --inherit-layer-annotations for the `buildah build` command to control inheritance of per-layer annotations (default=true). Extend the existing `buildah config --annotation` flag to support "layer:" prefix for per-layer annotations on the top-most (read-write) layer. The flag works the same for per-layer annotation as it does for image ones (set/unset key/clear all). Fixes: podman-container-tools#6652 Fixes: https://redhat.atlassian.net/browse/RUN-5088 Signed-off-by: Marek Simek <msimek@redhat.com>
d6b6021 to
326bc45
Compare
|
You might consider tweaking the release note to include something about |
| of any images which will be built using the specified container or add a per-layer | ||
| annotation for the top-most layer with the "layer:" prefix before the key. Can be used | ||
| multiple times. If *annotation* has a trailing `-`, then the *annotation* is removed | ||
| from the config. If the *annotation* is set to "-" then all annotations are removed |
There was a problem hiding this comment.
| from the config. If the *annotation* is set to "-" then all annotations are removed | |
| from the config. If the *annotation* is set to "-" then all annotations at that level are removed |
I think
4 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
How to verify it
fromit:buildah from quay.io/fedora/fedora-bootc buildah run fedora-bootc-working-container touch /var/foo img=$(buildah commit fedora-bootc-working-container)Which issue(s) this PR fixes:
Fixes #6652
Special notes for your reviewer:
I'm adding notes inline.
Does this PR introduce a user-facing change?