Skip to content
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

Generalize container-push to support multi-arch images and varying artifacts #2828

Closed
wants to merge 1 commit into from

Conversation

rmohr
Copy link
Member

@rmohr rmohr commented Apr 22, 2022

The container-push command now works like this:

  • It checks the meta.json file of every built architecture referenced in builds.json
  • It creates with buildah a multi-arch image and pushes it.
  • It determines the final digest values for the remove manifests per arch (to be independend of the push format) and updates meta.json of each architecture.

Right now the command works with ostree and kubevirt. If not arthifact is specified it falls back to ostree to not interrupt any build pipelines wich assume that it solely works for ostree.

Pushing ostree:

cosa push-container quay.io/myorg/ostree:testing --authfile=/home/builder/.docker/config.json --format oci ostree

Push kubevirt:

cosa push-container quay.io/myorg/kubevirt:testing --authfile=/home/builder/.docker/config.json --format oci kubevirt

Note: Since this modifie meta.json stream metadata needs to be regenerated after all artifacts are pushed.

Background discussion happened at coreos/fedora-coreos-pipeline#515 (comment).

)


class MetadataNavigator:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is at least the 3rd version of this we have in tree now...there's also class Builds etc.

for manifest in inspect_result["manifests"]:
arch = manifest["platform"]["architecture"]
if arch == "amd64":
arch = "x86_64"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have this bit in the mantle code, but fine as is.

@cgwalters
Copy link
Member

Thanks for working on this BTW!

@openshift-ci
Copy link

openshift-ci bot commented May 3, 2022

@rmohr: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@cgwalters
Copy link
Member

cgwalters commented Jun 7, 2022

I am uncertain about this PR...it ties into the bigger multiarch discussion.

What I don't quite understand is this command seems to assume the containers have already been built for multiple architectures and stored in meta.json, and that the local build has fetched all those architectures.

I think we should more clearly separate "multi-arch unification" flows from the "individual build command".

The "multi-arch unification" (i.e. convert arch-specific container builds into a standard manifest list, or convert arch-specific cosa disk image builds into stream metadata) I think should be a separate command.

I pushed some (textually but not logically) conflicting code in #2907 FWIW.

@ravanelli
Copy link
Member

ravanelli commented Jun 13, 2022

What I don't quite understand is this command seems to assume the containers have already been built for multiple architectures and stored in meta.json, and that the local build has fetched all those architectures.

Yes, in theory gangplank would allow us to have all multi-arch artifacts and the unified meta.json in the same place. You wouldn't need to fetch/build it as part of container-push

Since we are probably dropping gangplank, we may think if that's what we really want, have everything unified in the sam e place? If so, it could be some enhancement for podman-remote part @dustymabe is starting.

For the manifest part, we really don't need to have the containers in the host itself. We can push them directly from the remote to a registry and then create a manifest pointing to it.

We can still change this PR to look for both cases.

@rmohr
Copy link
Member Author

rmohr commented Jul 15, 2022

Closing this one since I am not actively working on it. Doing some cleanup. Thanks everyone for your help.

@rmohr rmohr closed this Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants