feat: build and push image index #81

codingben · 2024-02-08T18:54:54Z

What this PR does / why we need it:

Push an image index to have multiple image manifests, instead of single image manifest when needed.

Jira-Url: https://issues.redhat.com/browse/CNV-38597

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Fixes #80

Release note:

Build and push image index

artifacts/centos/centos.go

zhlhahaha · 2024-02-11T03:16:10Z

Hi @codingben it is a good start, feel free to tell me if I can provide any help, I will review and verify the PR once the draft have done.
In terms of generate and publish multi-arch manifest, here is an shell script. https://github.com/kubevirt/kubevirtci/blob/main/cluster-provision/images/vm-image-builder/publish-multiarch-containerdisk.sh

codingben · 2024-02-11T13:01:16Z

Hi @codingben it is a good start, feel free to tell me if I can provide any help, I will review and verify the PR once the draft have done. In terms of generate and publish multi-arch manifest, here is an shell script. https://github.com/kubevirt/kubevirtci/blob/main/cluster-provision/images/vm-image-builder/publish-multiarch-containerdisk.sh

Thanks a lot, it's really appreciated and helpful. I'll keep you updated once we have something to review here.

Makefile

artifacts/centos/centos.go

pkg/build/build.go

codingben · 2024-02-26T16:00:26Z

@0xFelix What do you think about just implementing image index functionality here, to push it via existing pipelines, and then open another PR to add arm64 image manifests?

0xFelix · 2024-02-26T16:11:25Z

IMO you can do it in a single PR, until then let's stick to our simple manifest without indexes.

codingben · 2024-02-28T09:23:48Z

Hi @0xFelix, it's ready to be reviewed. I'll run local tests now. After it, I'll work on adding arm64 images (in another commit in this PR).

0xFelix · 2024-03-27T12:31:30Z

cmd/medius/common/registry.go

@@ -100,7 +104,7 @@ func gatherArtifacts(registry *[]Entry, gatherers []api.ArtifactsGatherer) {
 		} else {
 			for i := range artifacts {
 				*registry = append(*registry, Entry{
-					Artifact:     artifacts[i],


That still looks wrong. The gatherer needs to return artifacts with multiple architectures and this loop needs to append them to the registry. (Artifacts: artifacts[i])

I think you reviewed old revision. This is how it looks like today:

func gatherArtifacts(registry *[]Entry, gatherers []api.ArtifactsGatherer) { for _, gatherer := range gatherers { artifacts, err := gatherer.Gather() if err != nil { logrus.Warn("Failed to gather artifacts", err) } else { for i := range artifacts { *registry = append(*registry, Entry{ Artifacts: []api.Artifact{artifacts[0][i]}, UseForDocs: i == 0, UseForLatest: i == 0, }) } } } }

I reviewed the "left" side. The new is still wrong, since like this it probably run out of range. i is a range over the outer slice and you use it to access the inner slice. The artifacts returned by the gatherer should be [][]api.Artifact, then you can set each outer slice item as []api.Artifact in the loop below with artifacts[i]. IMO the gatherer should construct the multi-arch Artifact already. Like this you end up with a separate artifact for each architecture and version.

Ah got it, thanks Felix! :)

codingben · 2024-03-27T17:21:56Z

@codingben PR needs rebase

I see. I wasn't able to produce image index with OCI yet, I'll continue debugging it tomorrow.

It seems like it's producing and pushing OCI image index, but the container registry locally shows it as Docker schema and not OCI.

I'm trying to push the image index to Quay directly, but there is an error of unauthorized access to Quay during running Medius (either push or promote).

Seems like remote.WithAuthFromKeychain(authn.DefaultKeychain) was missing in the options in PushImageIndex, but even then after adding it, it still returns an unauthorized error.

codingben · 2024-03-27T17:23:41Z

Found another issue with uploads to quay.io:

felix@felix-p1 kubevirt/containerdisks (CNV-37406) » ./bin/medius images promote --dry-run=false --source-registry="$(./hack/kubevirtci.sh registry)" --target-registry=quay.io/fmatouschek --insecure-skip-tls --workers 3 --focus=cirros:6.1
INFO[0000] Copying localhost:45677/cirros:6.1-2403211715 -> quay.io/fmatouschek/cirros:6.1-2403211715  name=cirros version=6.1
ERRO[0018] Failed to copy image                          error="PUT https://quay.io/v2/fmatouschek/cirros/manifests/6.1-2403211715: MANIFEST_INVALID: manifest invalid; map[message:Unknown or unsupported manifest media type `application/vnd.docker.distribution.manifest.v2+json` not found in dict_keys(['application/vnd.oci.image.manifest.v1+json', 'application/vnd.oci.image.index.v1+json'])]" name=cirros version=6.1
ERRO[0018] PUT https://quay.io/v2/fmatouschek/cirros/manifests/6.1-2403211715: MANIFEST_INVALID: manifest invalid; map[message:Unknown or unsupported manifest media type `application/vnd.docker.distribution.manifest.v2+json` not found in dict_keys(['application/vnd.oci.image.manifest.v1+json', 'application/vnd.oci.image.index.v1+json'])]  name=cirros version=6.1
INFO[0018] Writing results file                         
FATA[0018] PUT https://quay.io/v2/fmatouschek/cirros/manifests/6.1-2403211715: MANIFEST_INVALID: manifest invalid; map[message:Unknown or unsupported manifest media type `application/vnd.docker.distribution.manifest.v2+json` not found in dict_keys(['application/vnd.oci.image.manifest.v1+json', 'application/vnd.oci.image.index.v1+json'])]

We need to update medius so it creates OCI image manifests and image indexes. Can you do that as a follow up first before continuing feature work please?

@0xFelix Can you please check if you tried to push an image index, or just single image manifest? Because I'm getting an unauthorized error when trying to push a new image index to Quay.

codingben · 2024-03-28T08:42:16Z

@0xFelix I can confirm that I was able to push image index, the issue was that it required to run Medius with sudo locally so it can get an access to read ./docker folder. It was complaining about unauthorized error and not about permission denied - seems like a bug in the go-containerregistry library.

0xFelix · 2024-03-28T10:19:01Z

@0xFelix I can confirm that I was able to push image index, the issue was that it required to run Medius with sudo locally so it can get an access to read ./docker folder. It was complaining about unauthorized error and not about permission denied - seems like a bug in the go-containerregistry library.

IMO this is very specific to Mac OS on which you are running medius on. Mac OS is known to deny access to files in the home directory if not explicitly allowed.

codingben · 2024-03-28T10:31:07Z

This PR is affected by this issue: containers/podman#22196 - We're not able to pull OCI image index via Podman, only Docker works correctly.

codingben · 2024-03-28T12:19:22Z

@0xFelix I've tested push to local registry and promote it from there to Quay container registry - It works without issues:

╰─$ sudo ./bin/medius images promote --dry-run=false --source-registry="localhost:5000" --target-registry=quay.io/boukhano --insecure-skip-tls --focus=cirros:6.1 --results-file="results.json"
INFO[0001] Copying localhost:5000/cirros:6.1-2403281407 -> quay.io/boukhano/cirros:6.1-2403281407  name=cirros version=6.1
INFO[0014] Copying localhost:5000/cirros:6.1-2403281407 -> quay.io/boukhano/cirros:6.1  name=cirros version=6.1
INFO[0016] Writing results file

Also podman pull works:

╰─$ sudo podman pull quay.io/boukhano/cirros:6.1                                                                                                          125 ↵
Trying to pull quay.io/boukhano/cirros:6.1...
Getting image source signatures
Copying blob sha256:982946d114cde2b91f867a5149ef078a2a5859c61769c5b495225d4fc4cd1ed0
Copying config sha256:b5af4dd60abc4e54a1acb5dd96d7e01a7890986c8b0973984296f29c4977e40c
Writing manifest to image destination
b5af4dd60abc4e54a1acb5dd96d7e01a7890986c8b0973984296f29c4977e40c

╰─$ sudo podman manifest inspect quay.io/boukhano/cirros:6.1 --tls-verify=false                                                                           125 ↵
{
    "schemaVersion": 2,
    "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
    "manifests": [
        {
            "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
            "size": 428,
            "digest": "sha256:3e2529ebfa5b79f2b10922b68b6a03e37d1f628536ce8e060892a31a15ac48f6",
            "platform": {
                "architecture": "amd64",
                "os": "linux"
            }
        },
        {
            "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
            "size": 428,
            "digest": "sha256:a30c9e081851feb38a90415d9dfab742faed9dd5db9df2bcef8a066af87623b1",
            "platform": {
                "architecture": "arm64",
                "os": "linux"
            }
        }
    ]
}

codingben · 2024-03-28T14:23:48Z

/unhold

It works now - I've tested changes locally by pushing and promoting it to Quay container registry. Thanks @0xFelix :)

codingben · 2024-03-28T14:34:40Z

Unit tests should work now. Forgot to update fedora_test.go.

pkg/repository/repository.go

Push an image index to have multiple image manifests, instead of single image manifest when needed. Jira-Url: https://issues.redhat.com/browse/CNV-38597 Signed-off-by: Ben Oukhanov <boukhanov@redhat.com>

Empty image sets Docker manifest schema and not OCI. Image manifests included in OCI image index, and to keep consistency all of them should have Docker media types. Signed-off-by: Ben Oukhanov <boukhanov@redhat.com>

0xFelix · 2024-03-28T15:15:50Z

Thanks @codingben, to me it looks good!

@ksimon1 can you give this a final review please?

codingben · 2024-03-28T15:21:05Z

@ksimon1 @akrejcir @jcanocan @lyarwood Hi, I'd appreciate /lgtm unless there is functionality that is broken. I also tested the code changes and it works, and I can improve code in another PR if needed.

codingben · 2024-03-28T15:24:05Z

/test pull-containerdisks-test

codingben · 2024-03-28T15:27:04Z

Hi @codingben it is a good start, feel free to tell me if I can provide any help, I will review and verify the PR once the draft have done. In terms of generate and publish multi-arch manifest, here is an shell script. https://github.com/kubevirt/kubevirtci/blob/main/cluster-provision/images/vm-image-builder/publish-multiarch-containerdisk.sh

@zhlhahaha Hi, can you please review? we're missing /lgtm label here.

ksimon1 · 2024-03-28T17:27:25Z

/lgtm

zhlhahaha · 2024-03-29T09:00:09Z

/lgtm
Thanks @codingben

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Feb 8, 2024

kubevirt-bot requested review from jcanocan and ksimon1 February 8, 2024 18:55

kubevirt-bot added the size/S label Feb 8, 2024

codingben marked this pull request as draft February 8, 2024 18:57

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 8, 2024

jcanocan reviewed Feb 9, 2024

View reviewed changes

artifacts/centos/centos.go Outdated Show resolved Hide resolved

0xFelix reviewed Feb 12, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

artifacts/centos/centos.go Outdated Show resolved Hide resolved

artifacts/centos/centos.go Outdated Show resolved Hide resolved

codingben force-pushed the CNV-37406 branch 3 times, most recently from 806f88e to fcad9d7 Compare February 21, 2024 14:24

kubevirt-bot added size/XS and removed size/S labels Feb 21, 2024

codingben force-pushed the CNV-37406 branch from fcad9d7 to ac9c4d5 Compare February 26, 2024 15:42

kubevirt-bot added size/M and removed size/XS labels Feb 26, 2024

0xFelix reviewed Feb 26, 2024

View reviewed changes

pkg/build/build.go Outdated Show resolved Hide resolved

codingben force-pushed the CNV-37406 branch from ac9c4d5 to 9963951 Compare February 26, 2024 16:10

codingben force-pushed the CNV-37406 branch 2 times, most recently from a4f4ac8 to 2ffb72a Compare February 28, 2024 09:19

codingben marked this pull request as ready for review February 28, 2024 09:44

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2024

kubevirt-bot requested review from akrejcir and jcanocan February 28, 2024 09:44

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2024

0xFelix reviewed Mar 27, 2024

View reviewed changes

codingben force-pushed the CNV-37406 branch 2 times, most recently from c39ddf6 to 62c2f39 Compare March 28, 2024 11:54

nalind mentioned this pull request Mar 28, 2024

podman manifest inspect showing unexpected values for mixed-schema image structures containers/podman#22196

Open

codingben force-pushed the CNV-37406 branch from 62c2f39 to d6cee1d Compare March 28, 2024 14:21

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 28, 2024

codingben force-pushed the CNV-37406 branch from d6cee1d to 6c2bc6b Compare March 28, 2024 14:34

0xFelix reviewed Mar 28, 2024

View reviewed changes

pkg/repository/repository.go Outdated Show resolved Hide resolved

codingben and others added 2 commits March 28, 2024 15:14

feat: build and push image index

4cb0967

Push an image index to have multiple image manifests, instead of single image manifest when needed. Jira-Url: https://issues.redhat.com/browse/CNV-38597 Signed-off-by: Ben Oukhanov <boukhanov@redhat.com>

fix: use Docker image index and manifests

7e7f181

Empty image sets Docker manifest schema and not OCI. Image manifests included in OCI image index, and to keep consistency all of them should have Docker media types. Signed-off-by: Ben Oukhanov <boukhanov@redhat.com>

codingben force-pushed the CNV-37406 branch from 6c2bc6b to 7e7f181 Compare March 28, 2024 15:14

kubevirt-bot assigned ksimon1 Mar 28, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2024

kubevirt-bot merged commit b48a3cd into kubevirt:main Mar 28, 2024
9 checks passed

codingben mentioned this pull request Apr 11, 2024

Enable building and running kubevirt on s390x kubevirt/kubevirt#10490

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: build and push image index #81

feat: build and push image index #81

codingben commented Feb 8, 2024 •

edited

Loading

zhlhahaha commented Feb 11, 2024

codingben commented Feb 11, 2024

codingben commented Feb 26, 2024

0xFelix commented Feb 26, 2024

codingben commented Feb 28, 2024

0xFelix Mar 27, 2024

codingben Mar 28, 2024

0xFelix Mar 28, 2024

codingben Mar 28, 2024

codingben commented Mar 27, 2024

codingben commented Mar 27, 2024

codingben commented Mar 28, 2024

0xFelix commented Mar 28, 2024

codingben commented Mar 28, 2024

codingben commented Mar 28, 2024 •

edited

Loading

codingben commented Mar 28, 2024

codingben commented Mar 28, 2024

0xFelix commented Mar 28, 2024

codingben commented Mar 28, 2024

codingben commented Mar 28, 2024

codingben commented Mar 28, 2024

ksimon1 commented Mar 28, 2024

zhlhahaha commented Mar 29, 2024

feat: build and push image index #81

feat: build and push image index #81

Conversation

codingben commented Feb 8, 2024 • edited Loading

zhlhahaha commented Feb 11, 2024

codingben commented Feb 11, 2024

codingben commented Feb 26, 2024

0xFelix commented Feb 26, 2024

codingben commented Feb 28, 2024

0xFelix Mar 27, 2024

Choose a reason for hiding this comment

codingben Mar 28, 2024

Choose a reason for hiding this comment

0xFelix Mar 28, 2024

Choose a reason for hiding this comment

codingben Mar 28, 2024

Choose a reason for hiding this comment

codingben commented Mar 27, 2024

codingben commented Mar 27, 2024

codingben commented Mar 28, 2024

0xFelix commented Mar 28, 2024

codingben commented Mar 28, 2024

codingben commented Mar 28, 2024 • edited Loading

codingben commented Mar 28, 2024

codingben commented Mar 28, 2024

0xFelix commented Mar 28, 2024

codingben commented Mar 28, 2024

codingben commented Mar 28, 2024

codingben commented Mar 28, 2024

ksimon1 commented Mar 28, 2024

zhlhahaha commented Mar 29, 2024

codingben commented Feb 8, 2024 •

edited

Loading

codingben commented Mar 28, 2024 •

edited

Loading