libimage.Image: add ConvertToManifestList()#2091
libimage.Image: add ConvertToManifestList()#2091openshift-merge-bot[bot] merged 3 commits intocontainers:mainfrom
Conversation
Reformat the godoc for the AddArtifactOptions type so that it gets wrapped correctly. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
|
We were not able to find or create Copr project Unless the HTTP status code above is >= 500, please check your configuration for:
|
libimage/manifest_list.go
Outdated
|
|
||
| // Write the "why yes, this is an OCI layout" file. | ||
| layoutFile := filepath.Join(tmp, "oci-layout") | ||
| if err := os.WriteFile(layoutFile, []byte(`{"imageLayoutVersion": "1.0.0"}`), 0o644); err != nil { |
There was a problem hiding this comment.
We do have a lot of constants we could be using here. Updated it.
libimage/manifest_list.go
Outdated
| // already also a list. An error is returned if the conversion fails. | ||
| func (i *Image) ConvertToManifestList(ctx context.Context) (*ManifestList, error) { | ||
| // If we don't need to do anything, don't do anything. | ||
| if list, err := i.ToManifestList(); err == nil { |
There was a problem hiding this comment.
Should this be
if list, err := i.ToManifestList(); !Errors.Is(err, ErrNotAManifestList) {
return list, err
}
There was a problem hiding this comment.
Changed it to return if err is nil or something other than a ErrNotAManifestList error.
nalind
force-pushed
the
manifest-convert
branch
2 times, most recently
from
1180035 to
fcad56b
Compare
There was a problem hiding this comment.
Overall this seems reasonable to me, but I’m afraid I’m not comfortably familiar with the libimage multi-platform code and I don’t know what assumptions this might be breaking. (Are there previous instances of a single image ID being both a per-platform image and a libimage manifest list? If this is the first time, ATM I have no idea whether that’s fine, and I’m afraid I can’t spend time today to learn.)
| return nil, fmt.Errorf("digesting image index: %w", err) | ||
| } | ||
|
|
||
| // Build an OCI layout containing the image index as the only item. |
There was a problem hiding this comment.
(This creates an OCI image index possibly pointing at a non-OCI per-platform image. That’s generally not expected by c/image, although in this case I can’t see anything that could break.)
There was a problem hiding this comment.
The intent is to force the list/index to be converted to the list format that corresponds to the image type during the copy.Image() call further down. The image library's storageImageDestination.Commit() implementation has a special case that writes both the list/index and the image manifest when it notices that the source has multiple instances and it's writing the image's manifest, and we're trying to take advantage of that to add the list/index to the image's set of data files.
There was a problem hiding this comment.
ACK, I think this does work right now. It’s just that, historically, when we did see c/image failures on mixed OCI/v2s2 data structures (and I forgot the exact details), the response usually was “fix your images, we are not going to add support”.
Assuming c/common will have tests that notice if things break, I’m fine with the mixed-format data here.
… uh, actually, wouldn’t it be a bit simpler to use dir: instead of oci/layout for the top-level source? That would
- not force the top-level item to use the OCI format; we could directly store the output of
list.Serialize, and expectc/image.Copyto record exactly those bytes - actually be simpler to implement, there are fewer data structures and subdirectories.
But then again, the oci/layout producer already exists, so I’m not asking for this to be changed.
(AFAICS storageImageDestination.Commit, ultimately, just adds the multi-platform manifest as a BigData item, which is not all that special — OTOH triggering the Commit is valuable in that it triggers exactly the same path as an ordinary pull, i.e. if Commit ever changed, this code would continue to work. On balance, I do like triggering the copy.Image here, despite all the effort.)
There was a problem hiding this comment.
I vaguely recall the "dir:" format disclaiming any guarantees about its details not changing going forward, which meant that a change in the image library's reading/writing logic would in turn require updates to logic that created them via other methods as this does, and that possibility being a reason to discourage using it for cases like this. But it's been a while since I think "dir:" actually last changed, so maybe that's less of a concern now?
There was a problem hiding this comment.
Yes, the dir: format is not formally documented. Pragmatically, we support its use in skopeo sync, and that alone implies a fair level of stability over some term of days/weeks(/months?) at least. But then again https://github.com/containers/image/issues/1876 remains outstanding, limiting any possible commitment to long-term stability as of today.
An alternative might be to use the dir ImageDestination to write the data, but writing a top-level manifest without any components is, per the terms of the API, probably (?) undefined (although it would work fine in practice). So that wouldn’t get us to any more documented stability per the state as of today.
One option is to document things. Probably changing the documentation of ImageDestination to explicitly allow creating sparse multi-platform images, if the destination supports them, would be easiest; c/image already has a long-standing RFE to support creating sparse images while mirroring.
(Just to be explicit, I’m personally more worried about this code introducing images which are both single-platform “data carrying” images and “multi-platform manifest” images, with a single storage ID; I vaguely seem to remember that the code assumes one or the other. Using mixed-format images for the ~sparse metadata-only copy is a concern, but much less of one, and it can be sufficiently resolved by a small number of tests.)
There was a problem hiding this comment.
that the code assumes one or the other
I mean libimage here.
Add libimage.Image.ConvertToManifestList(), which will convert an image to a manifest list if it isn't already possible to use it as one. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Instead of just writing a lot of the OCI layout using magic strings and names, use constants provided by the image-spec. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
|
Updated to set preferredListType correctly for Docker manifest types, and to drop tag names from the temporary OCI layouts the code creates. |
|
@mtrmac PTANL |
2 similar comments
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nalind, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
libimage.Image.ConvertToManifestList(), which will convert an image to a manifest list if it isn't already possible to use it as one.libimage/manifests.AddArtifactOptionstype so that it gets wrapped correctly for display.