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

Multi Arch Image Build #8989

Merged
merged 4 commits into from
Feb 8, 2023
Merged

Conversation

rthallisey
Copy link
Contributor

@rthallisey rthallisey commented Dec 21, 2022

Bazel doesn't support creating a multi-arch manifest, so we can work around that by using a container runtime to build a multiple architecture manifest.

  • Users can use a comma-separated list of architectures in BUILD_ARCH and bazel recipes will support the build
  • Building the container manifest is done from the local host so will require a container runtime
BUILD_ARCH=aarch64,x86_64 make bazel-build-images
BUILD_ARCH=aarch64,x86_64 make bazel-push-images

Sample output from BUILD_ARCH=aarch64,x86_64 make bazel-push-images:

2023/01/03 15:11:30 Successfully pushed Docker image to docker.io/rthallisey/virt-handler:local_latest_test-x86_64
BUILD_ARCH=aarch64,x86_64 DOCKER_PREFIX=docker.io/rthallisey DOCKER_TAG=local_latest_test hack/push-container-manifest.sh
docker manifest create docker.io/rthallisey/virt-api:local_latest_test --amend docker.io/rthallisey/virt-api@sha256:0d04a7fe6cd70dece2b68ae6dd5fa1fee4a57de09f4950f63c0175a71be3c83e --amend docker.io/rthallisey/virt-api@sha256:01158984de3e743d07bc1b63085b1f94404a31250eea92dd23debe2b6ccda99d
Created manifest list docker.io/rthallisey/virt-api:local_latest_test
sha256:2dc788b897f4de0880f687eeb5203b24aa97fb33d510b7106ea39ef5acdfbcb2
docker manifest create docker.io/rthallisey/virt-controller:local_latest_test --amend docker.io/rthallisey/virt-controller@sha256:43d6c594c99730c1353005561ad4bd1a168b273a7537bf5523360cf24e42b64e --amend docker.io/rthallisey/virt-controller@sha256:8c0d8b2677100b5882a509beb7c894e234a67f893e8d73c1ec0569ba77b4110a
Created manifest list docker.io/rthallisey/virt-controller:local_latest_test
sha256:c671859ac61e62f75e160e98e850dbb5c58cc3cf083add4d2a4d0194092c8f1d
docker manifest create docker.io/rthallisey/virt-handler:local_latest_test --amend docker.io/rthallisey/virt-handler@sha256:d0dfc28be685ccecf361de5c1dc6cab312d52fef52b53712e2fc43fccd3ed4cb --amend docker.io/rthallisey/virt-handler@sha256:d79bca811b206ed9e113dcac93bba47664da8b4ef52c027a1a00478bd367fa35
Created manifest list docker.io/rthallisey/virt-handler:local_latest_test
sha256:d96b57659dc0dec63a010dd066bce429ead7a72dc82330ee154dea9585f1dfb5
docker manifest create docker.io/rthallisey/virt-operator:local_latest_test --amend docker.io/rthallisey/virt-operator@sha256:cea1c97e2cf426efd6afcaee50a31d9c77867df23739de180561e8ba051f5e7c --amend docker.io/rthallisey/virt-operator@sha256:7162539bf44d5bd4754f4f462aa5ce4f24c0418cc4a5f62138eed8d52d79174d
Created manifest list docker.io/rthallisey/virt-operator:local_latest_test
sha256:9875547a2e2b14ab5fad7a44d4956c18945a2f43cd41849bd152f584c86856fb

Inspecting the result:

$ docker manifest inspect docker.io/rthallisey/virt-operator:local_latest_test
{
   "schemaVersion": 2,
   "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
   "manifests": [
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1076,
         "digest": "sha256:7162539bf44d5bd4754f4f462aa5ce4f24c0418cc4a5f62138eed8d52d79174d",
         "platform": {
            "architecture": "amd64",
            "os": "linux"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1076,
         "digest": "sha256:cea1c97e2cf426efd6afcaee50a31d9c77867df23739de180561e8ba051f5e7c",
         "platform": {
            "architecture": "arm64",
            "os": "linux"
         }
      }
   ]
}

Failed builds look like:

docker manifest create quay.io/ryan/virt-handler:testbuild-x86_64-multi-arch --amend quay.io/ryan/virt-handler:testbuild-x86_64 --amend quay.io/ryan/virt-handler:testbuild-aarch64
no such manifest: quay.io/ryan/virt-handler:testbuild-aarch64
docker manifest create quay.io/ryan/virt-launcher:testbuild-x86_64-multi-arch --amend quay.io/ryan/virt-launcher:testbuild-x86_64 --amend quay.io/ryan/virt-launcher:testbuild-aarch64
no such manifest: quay.io/ryan/virt-launcher:testbuild-x86_64

Signed-off-by: Ryan Hallisey rhallisey@nvidia.com

Integrate multi-architecture container manifests into the bazel make recipes

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels Dec 21, 2022
@rthallisey
Copy link
Contributor Author

/cc @rmohr @zhlhahaha

@rthallisey
Copy link
Contributor Author

There's a bunch of directions we can take this, e.g. how we can push these multi-arch manifests and how would the e2e automation look. Starting off with just building the manifest first to see if we can get alignment before going too far.

@rthallisey
Copy link
Contributor Author

related to #3558

@zhlhahaha zhlhahaha mentioned this pull request Dec 22, 2022
32 tasks
Copy link
Member

@rmohr rmohr left a comment

Choose a reason for hiding this comment

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

I added an alterantive suggestion. Let me know what you think.

Makefile Outdated Show resolved Hide resolved
@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 3, 2023
@rthallisey rthallisey force-pushed the multi-arch-image branch 2 times, most recently from 707e0b8 to da289ee Compare January 3, 2023 18:08
@rthallisey
Copy link
Contributor Author

/retest-required

@zhlhahaha
Copy link
Contributor

Hi @rthallisey , thanks for your PR.
I am a little busy today and will give a more careful review on this PR tomorrow. I roughly read this PR. There is an issue.
When doing cross compile, we set environmental variable BUILD_ARCH=crossbuild-aarch64. Then bazel build would apply the configuration in the file .bazelrc and using cross compile toolchain.

build:crossbuild-aarch64 --incompatible_enable_cc_toolchain_resolution --platforms=//bazel/platforms:aarch64-none-linux-gnu --platforms=@io_bazel_rules_go//go/toolchain:linux_arm64_cgo
run:crossbuild-aarch64  --incompatible_enable_cc_toolchain_resolution --platforms=//bazel/platforms:aarch64-none-linux-gnu --platforms=@io_bazel_rules_go//go/toolchain:linux_arm64_cgo
test:crossbuild-aarch64 --platforms=@io_bazel_rules_go//go/toolchain:linux_arm64_cgo --host_javabase=@local_jdk//:jdk

However, if we set BUILD_ARCH=aarch64,x86_64, the cross compile toolchain would not be applied.

If we modify .bazelrc file and change crossbuild-aarch64 to aarch64, this would impact native compile on Arm64 server.

@rthallisey
Copy link
Contributor Author

@zhlhahaha the cross compile would still work because all I'm doing is calling a bazel build recipe. The args would look like:

BUILD_ARCH=crossbuild-aarch64,x86_64

The only difference is crossbuild-aarch64 would have its own digest directory instead of sharing one with aarch64. Meaning someone could theoretically do BUILD_ARCH=crossbuild-aarch64,aarch64.., which is useless but would be allowed.

@zhlhahaha
Copy link
Contributor

Verified on Arm64 server, it works fine:

wget https://github.com/kubevirt/kubevirt/pull/8989.patch
git am 8989.patch
make cluster-sync

Verified cross-build on x86_64 server, it works fine:

wget https://github.com/kubevirt/kubevirt/pull/8989.patch
git am 8989.patch
BUILD_ARCH=crossbuild-aarch64,x86_64 DOCKER_PREFIX=zhlhahaha DOCKER_TAG=multi-arch make bazel-push-images

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2023
@zhlhahaha
Copy link
Contributor

/hold
It seems that there are some issue on operator tests.

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 5, 2023
@zhlhahaha
Copy link
Contributor

/retest

@zhlhahaha
Copy link
Contributor

/test pull-kubevirt-e2e-k8s-1.24-operator

@zhlhahaha
Copy link
Contributor

I applied the PR and create a kind cluster, then run the operator tests. All tests passed.
I only have a x86_64 desktop, the memory of which is too low to run operator tests in nested VM.

@rthallisey
Copy link
Contributor Author

Ya the failure is in the pretests virt-operator should use sha. Still investigating why..

Bazel doesn't support creating a multi-arch manifest, so we can work around
that by using a container tool to add multiple architectures to a single manifest.

Signed-off-by: Ryan Hallisey <rhallisey@nvidia.com>
@kubevirt-bot kubevirt-bot added size/L and removed size/M labels Jan 6, 2023
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jan 6, 2023
Signed-off-by: Ryan Hallisey <rhallisey@nvidia.com>
@kubevirt-bot kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jan 7, 2023
Signed-off-by: Ryan Hallisey <rhallisey@nvidia.com>
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jan 7, 2023
@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Jan 7, 2023

@rthallisey: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-e2e-k8s-1.23-operator da289ee link true /test pull-kubevirt-e2e-k8s-1.23-operator

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. I understand the commands that are listed here.

@rthallisey
Copy link
Contributor Author

/retest

@zhlhahaha
Copy link
Contributor

/lgtm
Thanks Ryan

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2023
@rthallisey
Copy link
Contributor Author

/retest

@rmohr
Copy link
Member

rmohr commented Feb 8, 2023

/approve

Is the reason for the hold still an issue?

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr

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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2023
@rthallisey
Copy link
Contributor Author

/unhold
It was because some of the tests weren't passing. That's been addressed.

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 8, 2023
@kubevirt-bot kubevirt-bot merged commit eea5574 into kubevirt:main Feb 8, 2023
@rthallisey rthallisey mentioned this pull request May 17, 2023
3 tasks
@enp0s3
Copy link
Contributor

enp0s3 commented Jun 20, 2023

Hi @rthallisey @rmohr , can you please assist with making container manifest also for the utility containers (those that are )? because in the current situation, as 1.0.0 released, now we have the utility containers with the arch tag in Quay, something that wasn't before.
@nunnatsa FYI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants