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

add support for multi-architecture docker images #22456

Open
wants to merge 10 commits into
base: master
from

Conversation

@xUnholy
Copy link

xUnholy commented Mar 25, 2020

[X] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

xUnholy added 3 commits Mar 25, 2020
…OS and GOARCH in setup_env.sh
@xUnholy xUnholy requested a review from istio/wg-test-and-release-maintainers as a code owner Mar 25, 2020
@googlebot

This comment has been minimized.

Copy link
Collaborator

googlebot commented Mar 25, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no label Mar 25, 2020
@istio-testing

This comment has been minimized.

Copy link
Collaborator

istio-testing commented Mar 25, 2020

Hi @xUnholy. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@@ -66,6 +66,7 @@ target "$image-$variant" {
proxy_version = "istio-proxy:${PROXY_REPO_SHA}"
istio_version = "${VERSION}"
}
platforms = ["linux/arm/v7"]

This comment has been minimized.

Copy link
@xUnholy

xUnholy Mar 25, 2020

Author

Agreed, this should neither be hardcoded, I do however want to parameterize this field although will need some assistance on a best possible solution.

This comment has been minimized.

Copy link
@xUnholy

xUnholy Mar 26, 2020

Author

@sdake perhaps you might have a suggestion on this front? I was considering we could use the ${TARGET_OS} ${TARGET_ARCH} values and have them passed into this file?

@howardjohn

This comment has been minimized.

Copy link
Member

howardjohn commented Mar 25, 2020

/ok-to-test

Copy link
Member

sdake left a comment

Thanks for the PR. WIll be pretty sweet to have a standard mechanism to add multiple architectures. I am curious how well this works with CGO cross-platform.

@@ -26,13 +26,13 @@ set -e
LOCAL_ARCH=$(uname -m)
export LOCAL_ARCH

This comment has been minimized.

Copy link
@sdake

sdake Mar 25, 2020

Member

I spent a good 5 minutes staring at this change to understand what you were after here :) You could simplify:

# Pass environment set target architecture to build system
if [[ -z ${TARGET_ARCH} ]]; then
    export TARGET_ARCH
elif ...
# Otherwise set TARGET_ARCH based upon LOCAL_ARCH

and keep lines 29-35 intact.

This comment has been minimized.

Copy link
@xUnholy

xUnholy Mar 26, 2020

Author

Correct, I was working on a PR where this logic was still in the makefile so I was using the same logic that was in the original makefile where it would set the var otherwise use the default.

I definitely like this suggestion though!

@@ -41,10 +41,10 @@ fi
LOCAL_OS=$(uname)
export LOCAL_OS
if [[ $LOCAL_OS == Linux ]]; then

This comment has been minimized.

Copy link
@sdake

sdake Mar 25, 2020

Member

Same simplification applies. Some day we want additional OS support.

@@ -26,13 +26,13 @@ set -e
LOCAL_ARCH=$(uname -m)

This comment has been minimized.

Copy link
@sdake

sdake Mar 25, 2020

Member

Once this is passing CI properly, please submit the PR to https://github.com/istio/common-files. The PR will then be replicated to all of our repos and automatically merged after approval.

This comment has been minimized.

Copy link
@xUnholy

xUnholy Mar 26, 2020

Author

Will do!

@xUnholy

This comment has been minimized.

Copy link
Author

xUnholy commented Mar 26, 2020

Based on the CI failures I believe it's the builder instance driver that has not been configured to use multiple of the selected architectures.

https://github.com/docker/buildx#working-with-builder-instances

@istio-testing

This comment has been minimized.

Copy link
Collaborator

istio-testing commented Mar 26, 2020

@xUnholy: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
integ-local-tests_istio 418d848 link /test integ-local-tests_istio
release-test_istio 418d848 link /test release-test_istio
integ-telemetry-k8s-tests_istio 418d848 link /test integ-telemetry-k8s-tests_istio
integ-security-k8s-tests_istio 418d848 link /test integ-security-k8s-tests_istio
integ-mixer-k8s-tests_istio 418d848 link /test integ-mixer-k8s-tests_istio
integ-pilot-k8s-tests_istio 418d848 link /test integ-pilot-k8s-tests_istio
integ-distroless-k8s-tests_istio 418d848 link /test integ-distroless-k8s-tests_istio
integ-conformance-k8s-tests_istio 418d848 link /test integ-conformance-k8s-tests_istio
integ-galley-k8s-tests_istio 418d848 link /test integ-galley-k8s-tests_istio
integ-multicluster-k8s-tests_istio 418d848 link /test integ-multicluster-k8s-tests_istio

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.

Copy link
Member

howardjohn left a comment

What is the actual end goal of this? Do you want to be able to build on your arm images on your local machine, have them built in CI, have them built in CI+Published, or have them built in CI+Published+tested?

I think that will guide some of the choices we make here

# Then a new build target can be created such as build-container-bin that builds these
# various platform images.
.PHONY: build-linux
build-linux: depend
STATIC=0 GOOS=linux GOARCH=amd64 LDFLAGS='-extldflags -static -s -w' common/scripts/gobuild.sh $(ISTIO_OUT_LINUX)/ $(BINARIES)
STATIC=0 GOOS=$(GOOS_LOCAL) GOARCH=$(GOARCH_LOCAL) LDFLAGS='-extldflags -static -s -w' common/scripts/gobuild.sh $(ISTIO_OUT_LINUX)/ $(BINARIES)

This comment has been minimized.

Copy link
@howardjohn

howardjohn Mar 26, 2020

Member

This means a user on OSX will build mac binaries which will then be packaged into the docker files which will not work. Thats why we have a specific build-linux target in addition to build

This comment has been minimized.

Copy link
@xUnholy

xUnholy Mar 26, 2020

Author

Somewhat, the changes made in the file common/scripts/setup_env.sh allows for overriding these variables so if a user wanted to build on an OSX but compile for ARM then it would still be possible, essentially allowing for all combinations by simply passing the right params

@xUnholy

This comment has been minimized.

Copy link
Author

xUnholy commented Mar 26, 2020

Do you want to be able to build on your arm images on your local machine, have them built in CI, have them built in CI+Published, or have them built in CI+Published+tested?

Ideally, the end goal would be to have them built-in CI, Tested, and Published. I understand that will require a lot of additional effort, therefore my initial thought was to tackle it in phases, firstly just getting CI to build the images, then I'd like to collaborate with the test and release workgroup on a good testing strategy going forward that will also work for other architectures, and once that is worked out, I would then try to get these images published. I knew this wouldn't be a simple fix and would require several changes.

@howardjohn

This comment has been minimized.

Copy link
Member

howardjohn commented Mar 26, 2020

@clarketm any idea if Kubernetes run any multi-arch testing in Prow?

@clarketm

This comment has been minimized.

Copy link
Member

clarketm commented Mar 26, 2020

@clarketm any idea if Kubernetes run any multi-arch testing in Prow?

I am not certain. @kubernetes/sig-testing @BenTheElder @Katharine @spiffxp how does K8s currently test multi-arch?

@BenTheElder

This comment has been minimized.

Copy link

BenTheElder commented Mar 26, 2020

We don't *

We have some supported some third parties publishing results to testgrid but they haven't been the most stable...

There's some interest in this currently from vendors but it's relatively unstaffed.

* we do test compilation upstream. the rest is third party.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.