-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preserve manifest list digest when embedding containers #1252
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL:DR; This approach is unworkable and must be rethought.
- The way we embed containers in images is by first pulling the container into a docker archive
skopeo copy <url>@<digest> docker-archive:<container>.tar
during the source download phase of the build and then we put it in the container storeskopeo copy docker-archive:<container>.tar <containerstore>:<url>
If at all possible, don’t do that. docker-archive:
, besides being slow, always loses information. Use dir:
if you absolutely need a non-registry file store, and want to preserve the image as it is on in-registry.
- Modify our
skopeo
stage (which doesskopeo copy
) to amend the image storeimages.json
Definitely don’t do that in production. The format of images.json
is an internal implementation detail and may change at any time (in fact, this month we have seriously discussed what it would take to move to SQLite).
The locking of images.json
is even more of an internal implementation detail, and critical to reliability. This code does not do any locking, and may therefore interfere with any running Podman/Skopeo/… processes in unanticipated ways.
The right way to add a name to an image is podman tag
, or, I suppose, skopeo copy containers-storage:$old containers-storage:$new
.
and add the digest that was used in the blueprint as a name for the container (this PR).
That won’t work anyway, because readers of those images will need to actually find the manifest data that matches that digest. Just adding a name does not record the manifest data.
So, the entire premise of using docker-archive
should be revisited. That’s the easiest way to get this done. (An alternative would be a custom-written tool to download the right manifests, and a custom-written Go tool to add the manifests to the store. Let’s not.)
- It's important to note that this digest is not the hash of anything that's on disk.
…- From what I understand, neither skopeo nor podman actually store any of this metadata.
This is incorrect. AFAICS all digest reference accesses like that will fail.
- I've read through the container library sources to make sure that modifying the
images.json
file directly is safe
It isn’t safe. It’s not up to me but speaking for myself, I would absolutely refuse to support any modifications not implemented by calling a (reasonably current) version of containers/storage.
The implementation is also not ensuring that each name is only assigned to only one image.
Thanks. I think this is probably the best way forward and probably wont require too much reworking.
I see. That's great to know. It wouldn't be a problem to rely on a shell utility that can properly write to the store, it would have to be something that's packaged for RHEL.
This is not an issue in our case (the stages are run in a clean bubblewrap container and no other processes should be running), but I understand the concern. Thank you.
We need an intermediate local storage because the download stage happens separately from the build stage. That said, if a
From some testing I did, I found that this does work (adding the name) and no manifest data was pulled (e.g., when doing
Good to know.
You're right, yes, this is an oversight. |
Adding some info after speaking with @mtrmac:
They're in the image's directory and their digests are in the
It shouldn't work without the data being there when the name contains a digest but it does. This is a bug. |
(I was thinking this could be done in the build stage; that’s just a way to add a tag without writing an external utility, e.g. with the |
|
b96cc4b
to
f9e1438
Compare
Everything changed. I've updated the PR description based on the solutions discussed with the containers team. |
f9e1438
to
a1e5a8e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I’m unfamiliar with the code base, this is just a quick skim of the container image-related aspects.)
a1e5a8e
to
abf603d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great and pretty clean 👍 It is great that you figured out a reasonable solution for this 🐛 . The only thing that I'm not sure about is the ability to specify the format for a stage input. I think that it just add new options to shoot oneself in the foot for not obvious (to me) benefit.
inputs/org.osbuild.containers
Outdated
}, | ||
"format": { | ||
"type": "string", | ||
"enum": ["docker-archive", "oci-archive", "dir"], | ||
"description": "The storage format of the container archive", | ||
"default": "oci-archive" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have basically the same question as for the source
input origin just with the difference of asking why we should not default to the format used to produce (store) the container in a pipeline or defaulting to the "most powerful" format for storing?
You're not wrong. It's a bit strange and creates a situation where the stage input needs to match the source. In fact, it's even weirder than that because you can specify the stage input to have a different format than the source, but if the input format was already downloaded and is in the store, it would work. My thinking on this was that I didn't want to introduce a behaviour change without introducing an option. On the other hand, this is an internal implementation detail and it might not matter. You're right that the most powerful format right now is the I thought I was making the most cautious choice here, but you're making me reconsider now. |
abf603d
to
ae3df8a
Compare
Rewrote with |
Updated the original comment again, though the Fedora digests are outdated now. |
ae3df8a
to
166a023
Compare
Fixed two small issues caught by pylint. |
Speaking with @thozza we just realised: we should change the ref ID that we use in the manifest (and cache) to something else, preferably the digest, because now we can download the same container using different manifest digests. This means that we can have the same container image ID with different metadata and this should be differentiated. |
166a023
to
5b6f213
Compare
Made the changes we discussed today. Manifest digest is now stored separately in the cache and it is only merged when the input option requests it. |
5b6f213
to
df05c28
Compare
Fixed a bug, added docstrings, and changed the name of the new option from |
cc8af87
to
c3fad90
Compare
The format so far was assumed to be `docker-archive` if the container was coming from a source and `oci-archive` if it was coming from a pipeline. The source format will now be changed to `dir` instead of `docker-archive`. The pipeline format remains `oci-archive`. With the new archive format being `dir`, the source can't be linked into the build root and is bind mounted instead with the use of a MountGuard created with the instance of the service, and torn down when the service is stopped. The _data field is removed from the map functions. It was unused and these functions aren't part of the abstract class so they don't need to have consistent signatures. Update the skopeo stage with support for the newly supported `dir` format.
Change the local storage format for containers to the `dir` format. The `dir` format will be used to retain signatures and manifests. The remove-signatures option is removed since the storage format now supports them. The final move (os.rename()) at the end of the fetch_one() method now creates the checksum directory if it doesn't exist and moves the child archive into it, adding to any existing archives that might exist in other formats (from a previous version downloading a `docker-archive`). Dropped the .tar suffix from the symlink in the skopeo stage since it's not necessary and the target of the link might be a directory now. The parent class exists() method checks if there is a *file* in the sources cache that matches the checksum. For containers, this used to be a file called container-image.tar under a directory that matches the checksum, so for containers it always returned False. Added an override for the skopeo source that checks for the new directory archive.
Extract the is_manifest_list() function from the ImageManifest object in osbuild-mpp into a util function to be reused by the skopeo source.
A new source module that can download a multi-image manifest list from a container registry. This module is very similar to the skopeo source, but instead downloads a manifest list with `--multi-arch=index-only`. The checksum of the source object must be the digest of the manifest list that will be stored and the manifest that is downloaded must be a manifest-list.
Add an extra optional input type to the skopeo stage called `manifest-lists`. This is a list of file-type inputs that must be a list of manifest lists, downloaded by the skopeo-index source. The manifests are parsed and automatically associated with an image from the required `images` inputs. If any manifest list is specified and not used, this is an error. Adding manifest-lists currently has no effect.
When a manifest list is matched with a container image, the skopeo stage will merge the specified manifest into the container image dir before copying it to the registry in the OS tree. If there is no manifest to merge, we maintain the old behaviour of symlinking the source to work around the ":" in filename issue. Otherwise, we copy the container directory so that we can merge the manifest in the new location.
Add support for resolving manifest lists in osbuild-mpp. Adds an `index` boolean field to the container image struct for mpp-resolve-images. When enabled, the preprocessor will also store the manifest-list digest as a separate skopeo-index source and add it to the skopeo stage under the `manifest-lists` input.
Added another skopeo stage to skopeo/a.mpp.json with a skopeo source for a container hosted on the osbuild-composer gitlab registry. The name points to a manifest list, which refers to two containers (amd64 and arm64) that contain a single text file (README.md). The `index` field is enabled to include the manifest-list as an extra input to the stage. The diff is updated with the new expected file list. The containers were created with buildah: amd=$(buildah from --arch=amd64 scratch) arm=$(buildah from --arch=arm64 scratch) buildah config --created-by "Achilleas Koutsou" "${amd}" buildah config --created-by "Achilleas Koutsou" "${arm}" buildah copy "${amd}" README.md buildah copy "${arm}" README.md amdid=$(buildah commit --format=docker --rm "${amd}") armid=$(buildah commit --format=docker --rm "${arm}") name="registry.gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/manifest-list-test" buildah manifest create "${name}" "${amdid}" "${armid}" podman manifest push --all "${name}" dir:container
c3fad90
to
75a2663
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work (on figuring our everything and also the test case), I love it 😍
Set the dependency to osbuild v83 which contains the new features for preserving manifest lists for containers. Added in osbuild/osbuild#1252
Set the dependency to osbuild v83 which contains the new features for preserving manifest lists for containers. Added in osbuild/osbuild#1252
Set the dependency to osbuild v83 which contains the new features for preserving manifest lists for containers. Added in osbuild/osbuild#1252
Set the dependency to osbuild v83 which contains the new features for preserving manifest lists for containers. Added in osbuild/osbuild#1252
Set the dependency to osbuild v83 which contains the new features for preserving manifest lists for containers. Added in osbuild/osbuild#1252
Set the dependency to osbuild v83 which contains the new features for preserving manifest lists for containers. Added in osbuild/osbuild#1252
Set the dependency to osbuild v83 which contains the new features for preserving manifest lists for containers. Added in osbuild/osbuild#1252
Set the dependency to osbuild v83 which contains the new features for preserving manifest lists for containers. Added in osbuild/osbuild#1252
Set the dependency to osbuild v83 which contains the new features for preserving manifest lists for containers. Added in osbuild/osbuild#1252
Skopeo source
This changes the intermediate format of a container source from
docker-archive
todir
, a directory format that's not yet standardised, but compatibility guidelines are currently being discussed (containers/image#1874).The
dir
format lets us merge in a multi-image manifest with a container so that we can preserve the manifest digest that was used to specify the image. This lets users run the image using the same digest that they used to specify it for pulling. See containers/skopeo#1935 for details on the merge method.Skopeo-index source
New source for specifying the manifest-list to download. This can then be used as a secondary (optional) input to the skopeo stage (see below). The output of the source is a single file, the manifest that the digest points to. If the digest points to any other type of object, the source fails.
Skopeo stage
The skopeo stage now takes an optional list of file references as input. These files must be manifest-lists. Each manifest-list is automatically paired with an image source based on the single-image manifest digest.
Example
Using the Fedora 37 image on Quay.io as an example, we can now specify the following sources:
sha256:d61102cd2dfcb5ac29f752554c8738631245acefb02cae741562349a471fc4d3
: Manifest-list digest. This is the multi-image manifest that (in this case) refers to four single-image manifests, one for each architecture. This is used to download the manifest-list as a single file by theskopeo-index
stage.sha256:b00bf6a9f41f2245ad0c7fccc3a948dbb6266ae808b3f9a8977e9634b3aabdcc
: Image manifest digest. This is the single-image manifest for theamd64
architecture. This is used to download the image by theskopeo
stage.sha256:38b8a6c320f32c32d41046e62496fdde0c712a6491a5327146fa7189a23399ad
: Image config digest (the image ID). This is the ID that appears inpodman image ls
for the container. This is the ID that's used to store the container in the local osbuild cache and the object ref for the container in the manifest.The skopeo stage will then look like this:
The manifest list is parsed for single-image manifest digests and the container is inspected to get its digest. If no matches are found between the two, the stage raises an exception.
manifest-lists
are not mandatory, but if they are specified, they should match a container in theimages
.Testing
I've tested the above example manually and also tested having multiple containers with matching manifest-lists.
I intend to add tests for the expected behaviour in osbuild-composer when we add support there.
Old summary. No longer relevant but kept for posterity.
This PR adds support for writing image names to the
names
array of the container store for images that are imported through skopeo.Problem
<url>@<digest>
and then run the container after build withpodman run <url>@<digest>
skopeo copy <url>@<digest> docker-archive:<container>.tar
during the source download phase of the build and then we put it in the container storeskopeo copy docker-archive:<container>.tar <containerstore>:<url>
podman run <url>@<digest>
after booting the OS doesn't work in many cases because the<digest>
is likely to change. This is especially the case when relying on the manifest-list digest (which Quay and the Red Hat catalogue present to users). This is a digest of a manifest that lists multiple container images (usually the same container for different architectures). When pulling a container using this digest, only the container for the host architecture is pulled. The local manifest is then completely different from the one used to query the registry.Observations
podman pull <url>@<digest>
, I see that podman puts the digest in theoverlay-images/images.json
as an "alias" for this container image.skopeo copy <url>@<digest> <containerstore>:<url>
.Solution
skopeo
stage (which doesskopeo copy
) to amend the image storeimages.json
and add the digest that was used in the blueprint as a name for the container (this PR).store_source_name
(or something similar) option in the container part of the blueprint (later PR in osbuild-composer).Questions/concerns
images.json
is just that, a name, a string that was used at some point to refer to an image that eventually resolved to the one in the store.images.json
file directly is safe and doesn't need any similar modifications in other places. I tested this a few times and it seems to simply work, but I'd like a sanity check from some of the container developers at the same time to make sure.names
array (and thenames-history
at the same time). If downloaded withskopeo copy
directly into the store, it's stored as thedigest
of the image. I chose to put it innames
, that feels less direct, but I'm open to suggestions.names-history
array at the same time.I've asked @runcom for some guidance here. I've looked at containers/image@e1ee980 and the linked issues, but that doesn't work because we rely on docker-archive for the local intermediate storage. That's one thing we can revisit in the future, whether a different local intermediate storage might make things easier, without having any negative side effects.