IR-57: API changes for manifest list support #635

ricardomaraschini · 2021-02-08T09:56:46Z

This commits adds a proposal for Openshift's manifest list support.

dmage

Will we have changes in ImageStreamImport? ImageStreamLayers? Will it be possible to get ImageStreamImages only for top-level SHAs (i.e. for image A) or will it work for children manifests too (i.e. for images B, C, D)?

dmage · 2021-02-08T10:53:17Z

enhancements/manifestlist/manifestlist-support.md

+
+### Non-Goals
+
+- Modify OpenShift's web console to support manifest lists.


I'd say it should also be a goal. As well as improving oc describe. If we allow customers to create some objects, we should allow them to inspect them.

A non-goal: provide facilities to build manifest lists.

dmage · 2021-02-08T10:54:04Z

enhancements/manifestlist/manifestlist-support.md

+
+- Allow users to import manifest lists using `oc image-import`.
+- Allow users to execute pull through using manifest lists.
+- Make image pruner aware of manifest lists.


I would add:

Allow users to push manifest lists into the integrated registry.

dmage · 2021-02-08T10:58:27Z

enhancements/manifestlist/manifestlist-support.md

+in order to present them to users. When dealing with manifest lists not all
+information will be presented back to the console (the Config portion of an
+image is absent in a manifest list). Depending on how manifest lists support
+is going to be implemented in the console a second request to the API may be


I'd say the goal of this proposal is to define these API details.

Could we tag someone from Console team's here? I need to get someone from that side involved.

dmage · 2021-02-08T11:02:40Z

enhancements/manifestlist/manifestlist-support.md

+	metav1.TypeMeta
+	metav1.ObjectMeta
+	DockerImageReference string
+	DockerImageMetadata DockerImage


DockerImageMetadata is runtime.RawExtension: https://github.com/openshift/api/blob/a9e731090f5ed361e5ab887d0ccd55c1db7fc633/image/v1/types.go#L41

Addressed. Not sure from where I copy this anymore.

dmage · 2021-02-08T11:07:36Z

enhancements/manifestlist/manifestlist-support.md

+(i.e. Config) we can see nowadays in a regular Image.
+
+Not all fields in an Image are relevant for a manifest list, therefore other
+proposed change is to add an "omitempty" flag on `DockerImageLayers` property


I don't think this is a backward compatible change. Old clients (oc v4.(y-1).z) may expect them to be always present.

What is the main source of concern here?

DockerImageLayers being an empty slice or

DockerImageLayers not even being presented

dmage · 2021-02-08T11:10:24Z

enhancements/manifestlist/manifestlist-support.md

+
+A second test would be very similar to this one but at this time leveraging
+pull through. Podman currently supports managing manifest lists and we can
+use its internals for step 2.


We also need to test that a manifest list can be mirrored into the integrated registry using oc image mirror.

adambkaplan · 2021-02-12T20:28:30Z

enhancements/manifestlist/manifestlist-support.md

+
+### Non-Goals
+
+- Modify OpenShift's web console to support manifest lists.


Another non-goal - Multiarch OpenShift builds which push a manifest list as output.

Added, thanks.

adambkaplan · 2021-02-12T20:31:08Z

enhancements/manifestlist/manifestlist-support.md

+1. Deploy a temporary image registry.
+2. Publish a manifest list containing images for different platforms.
+3. Create an ImageStream object to import the published manifest list.
+4. Create a Deployment using the ImageStream.


I think builds need to be included here as well. I'm pretty sure buildah can handle building from an image that references a manifest list, but we should verify that.

"Make sure a build that leverages an image using ManifestList works"
Something like this?

yeah. more importantly, "using a manifestlist hosted in the internal registry" (it better already work for manifestlists in general)

I have added a new section with a test to validate this. Thanks.

ricardomaraschini · 2021-03-23T13:52:47Z

Will we have changes in ImageStreamImport? ImageStreamLayers? Will it be possible to get ImageStreamImages only for top-level SHAs (i.e. for image A) or will it work for children manifests too (i.e. for images B, C, D)?

With regards to ImageStreamImport

I have proposed a change to the output of an ImageStreamImport, the proposal is to return the "child images" on the Status field, please take a look.

With regards to ImageStreamLayers

I have added a note as we gonna have to include layers from the child images on this API as well. PTAL.

With regards to ImageStreamImage

As far as I understood this is used when "tagging" one ImageStreamTag from one ImageStream into another (or between two distinct ImageStreamTags in the same ImageStream), in my view it would stay as it is, we would allow this tagging to happen only at the upmost level (at the ManifestList level). We will need to allow users to describe the "child images" as well (e.g. oc get -o yaml isimage <imagestream>@<sha of a child image>) and I have added this to the proposal. What do you reckon on this regard?

enhancements/manifestlist/manifestlist-support.md

dmage · 2021-03-25T14:07:05Z

enhancements/manifestlist/manifestlist-support.md

+Openshifft's internal registry will need to accept image pushes by SHA, not only by tag. The
+difference here is that the registry is not supposed to create an Image object every time it
+sees a push by SHA but only when it sees a push by tag. In the latter scenario the registry may
+need to create more than one image (if the pushed manifest is of type manifest list).


Risk: the pruner won't see these images, the hard pruner doesn't know about its age and whether this manifest has been just uploaded for a manifest list.

dmage · 2021-03-25T14:09:29Z

I think this proposal is ready for a review by a broader audience.

/assign @bparees @smarterclayton

enhancements/manifestlist/manifestlist-support.md

bparees · 2021-03-25T18:40:20Z

enhancements/manifestlist/manifestlist-support.md

+
+As an OpenShift user
+I want to be able to automatically clean unused manifest lists
+So that I can keep my storage resource usage under control


need more detail on how prune will work. If any image within the manifestlist is in use, will the full manifestlist be kept around? Or only if the manifestlist itself is referenced the manifestlist will be kept around?

or something in between? (if 2 of 3 images in the manifestlist are in use, the manifestlist is kept around but the unused image is removed from the manifestlist)

I think that the best way of dealing with this would be to maintain the whole struct consistent by not removing anything that is part of the manifest list. I have added a section to the proposal specifying this, PTAL.

/cc @dmage you may have some input on this regard.

keeping the entire manifestlist intact if any image it contains is ref'd makes sense to me.

bparees · 2021-03-25T18:49:41Z

enhancements/manifestlist/manifestlist-support.md

+1. Deploy a temporary image registry.
+2. Publish a manifest list containing images for different platforms.
+3. Create an ImageStream object to import the published manifest list.
+4. Create a Deployment using the ImageStream.


yeah. more importantly, "using a manifestlist hosted in the internal registry" (it better already work for manifestlists in general)

This commits adds a proposal for Openshift's manifest list support.

bparees · 2021-03-30T17:17:57Z

enhancements/manifestlist/manifestlist-support.md

+ImageStream X -> Image A -> Image B
+                            Image C
+                            Image D
+


it's not clear to me what the relationship is between A,B,C,and D here.

are they all part of the same manifestlist? is B the base image for A? Is A a manifestlist that contains B,C, and D?

I think it's that last option, but it'd help to clarify the scenario.

Yes, you are right. I will clarify it. The last option is the correct one.

bparees · 2021-03-31T18:32:44Z

/approve

openshift-ci-robot · 2021-03-31T18:32:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bparees]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dmage · 2021-04-06T14:20:00Z

enhancements/manifestlist/manifestlist-support.md

+```
+
+The output of the `oc describe isimage` command line will need to be changed to display
+information present in the DockerImageManifests property of the parent Image.


/cc @soltysh

fyi, we want to update oc describe for some image related types

No objections from my end.

dmage · 2021-04-06T14:24:03Z

enhancements/manifestlist/manifestlist-support.md

+
+Here the image that refers to the manifest list would be presented on `Image` property (as it is
+today for regular manifests) while all inner manifests within the manifest list would be listed
+in the `Manifests` slice.


/cc @spadgett

fyi, ImageStreamImports are going to show more information about manifest lists, afaik Console uses ImageStreamImports.

dmage · 2021-04-26T14:05:16Z

No objections, so let's merge it.
/lgtm

openshift-ci-robot requested review from enxebre and mrunalp February 8, 2021 09:57

dmage suggested changes Feb 8, 2021

View reviewed changes

adambkaplan reviewed Feb 12, 2021

View reviewed changes

ricardomaraschini changed the title ~~IR-57: API changes for manifest list support~~ WIP - IR-57: API changes for manifest list support Mar 19, 2021

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 19, 2021

ricardomaraschini changed the title ~~WIP - IR-57: API changes for manifest list support~~ IR-57: API changes for manifest list support Mar 23, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 23, 2021

dmage reviewed Mar 25, 2021

View reviewed changes

enhancements/manifestlist/manifestlist-support.md Outdated Show resolved Hide resolved

dmage reviewed Mar 25, 2021

View reviewed changes

enhancements/manifestlist/manifestlist-support.md Outdated Show resolved Hide resolved

dmage reviewed Mar 25, 2021

View reviewed changes

openshift-ci-robot assigned bparees and smarterclayton Mar 25, 2021

bparees reviewed Mar 25, 2021

View reviewed changes

ricardomaraschini added 2 commits March 26, 2021 11:27

API changes for manifest list support

c95fa50

This commits adds a proposal for Openshift's manifest list support.

Adding more details on pruner changes

f622b43

bparees reviewed Mar 30, 2021

View reviewed changes

Adding graph explanation

3a5fce7

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2021

dmage reviewed Apr 6, 2021

View reviewed changes

openshift-ci-robot requested a review from soltysh April 6, 2021 14:20

dmage reviewed Apr 6, 2021

View reviewed changes

openshift-ci-robot requested a review from spadgett April 6, 2021 14:25

openshift-ci-robot assigned dmage Apr 26, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 26, 2021

openshift-merge-robot merged commit 52eb78e into openshift:master Apr 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IR-57: API changes for manifest list support #635

IR-57: API changes for manifest list support #635

ricardomaraschini commented Feb 8, 2021

dmage left a comment

dmage Feb 8, 2021

ricardomaraschini Mar 26, 2021

dmage Feb 8, 2021

ricardomaraschini Mar 18, 2021

dmage Feb 8, 2021

ricardomaraschini Mar 18, 2021

dmage Feb 8, 2021

ricardomaraschini Mar 18, 2021

dmage Feb 8, 2021

ricardomaraschini Mar 18, 2021

dmage Feb 8, 2021

ricardomaraschini Mar 18, 2021

adambkaplan Feb 12, 2021

ricardomaraschini Mar 18, 2021

adambkaplan Feb 12, 2021

ricardomaraschini Mar 18, 2021

bparees Mar 25, 2021

ricardomaraschini Mar 26, 2021

ricardomaraschini commented Mar 23, 2021

dmage Mar 25, 2021

dmage commented Mar 25, 2021

bparees Mar 25, 2021

ricardomaraschini Mar 30, 2021

bparees Mar 30, 2021

bparees Mar 25, 2021

bparees Mar 30, 2021

ricardomaraschini Mar 31, 2021

bparees commented Mar 31, 2021

openshift-ci-robot commented Mar 31, 2021

dmage Apr 6, 2021

soltysh Apr 20, 2021

dmage Apr 6, 2021 •

edited

Loading

dmage commented Apr 26, 2021


		### Non-Goals

		- Modify OpenShift's web console to support manifest lists.

IR-57: API changes for manifest list support #635

IR-57: API changes for manifest list support #635

Conversation

ricardomaraschini commented Feb 8, 2021

dmage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardomaraschini commented Mar 23, 2021

With regards to ImageStreamImport

With regards to ImageStreamLayers

With regards to ImageStreamImage

Choose a reason for hiding this comment

dmage commented Mar 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Mar 31, 2021

openshift-ci-robot commented Mar 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmage Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

dmage commented Apr 26, 2021

dmage Apr 6, 2021 •

edited

Loading