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

cross build s390x kind binary and associated images by default #1705

Closed
wants to merge 1 commit into from

Conversation

guirish
Copy link
Contributor

@guirish guirish commented Jul 6, 2020

Added support for cross building s390x kind binary and associated images namely base, kindnetd and haproxy.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
  • Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 6, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @guirish!

It looks like this is your first PR to kubernetes-sigs/kind 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kind has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @guirish. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: guirish
To complete the pull request process, please assign munnerz
You can assign the PR to them by writing /assign @munnerz in a comment when ready.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 6, 2020
@k8s-ci-robot k8s-ci-robot requested review from amwat and munnerz July 6, 2020 14:59
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 6, 2020
@BenTheElder
Copy link
Member

You can override these at build time already and include this if you wish.

Why should we include this by default.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 6, 2020
@guirish
Copy link
Contributor Author

guirish commented Jul 6, 2020

Hi @BenTheElder , we are planning to setup our own local CI to test and publish Kubernetes Integration and Conformance test results specific to s390x. We have raised this PR so that we could have the cross built kind binary and its associated multi-arch images as releases, which would in-turn help us in our CI setup.

@BenTheElder
Copy link
Member

Hi @BenTheElder , we are planning to setup our own local CI to test and publish Kubernetes Integration and Conformance test results specific to s390x. We have raised this PR so that we could have the cross built kind binary and its associated multi-arch images as releases, which would in-turn help us in our CI setup.

Currently there's no continuous testing of s390x in any form in the Kubernetes project, adding this might be valuable to someone.

Can you elaborate on your intentions here and who / what groups you're working with within the Kubernetes project?

Is this going to be continuous testing? I.E. not Kubernetes tagged releases, but ongoing development versions (HEAD)?
Are you allocating resources to test all supported branches and working with SIG release / the CI signal team to get monitored results?

If this is just downstream conformance testing releases or something along those lines, I'm not sure this is the best use of our resources, given that you can already build custom images for your own usage. Building for more platforms is very slow compared to the native platform build.

@cwsolee
Copy link

cwsolee commented Jul 8, 2020

Hi @BenTheElder, Let me provide some background and answer to your questions. @guirish feel free to add any comments too.
Q: Can you elaborate on your intentions here and who / what groups you're working with within the Kubernetes project?
A: Guirish is working in my team, we're focusing on porting various open source packages to LoZ(s390x), Kubernetes is one of the package that we worked on, more details on what we've been ported/validated could be found here https://www.ibm.com/community/z/open-source-software. I saw your thread https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!topic/kubernetes-sig-architecture/eHBFgfd6Qxg weeks ago which is what trigger this. We did try to add e2e test a while back but got blocked, we pretty much want to try again and see what type of test make sense for s390x and how far we could get. Our main intention is to validate the released cross built binaries by performing Integration and Conformance tests specific to s390x on required branches/releases and publish our logs on gcloud bucket which could be used later to display  results on dashboard. Just as fyi, we internally start validating Kubernetes active release(1.17.x, 1.18.x & future 1.19.x) since we came across that s390x specific proxy problem too but we've no way to expose our test result.
Q: Is this going to be continuous testing? I.E. not Kubernetes tagged releases, but ongoing development versions (HEAD)?
A: Yes we will doing testing periodically(Daily) on kubernetes tagged releases as well as master branch and development versions (HEAD).
Q: Are you allocating resources to test all supported branches and working with SIG release / the CI signal team to get monitored results?
A: That's not what we plan to do, more trying to provide minimal test to make sure s390x is ok, that is why we want to focus on master and development versions only.
Q: If this is just downstream conformance testing releases or something along those lines, I'm not sure this is the best use of our resources, given that you can already build custom images for your own usage. Building for more platforms is very slow compared to the native platform build.
A: We're working on conformance test because we thought other non-intel platform are doing conformance test too, pls. advise if it's not the case. We're aware of integration and e2e test but didn't spend too much time on it.

@aledbf
Copy link
Member

aledbf commented Jul 8, 2020

Currently there's no continuous testing of s390x in any form in the Kubernetes project, adding this might be valuable to someone.

@cwsolee why not try to help the sig-testing team with the required resources to help with this ^^^ task upstream?

I ask because I am also interested in using kind on s390x to test ingress-nginx

Note: the changes in this PR are not enough to build the kindest/node image. This is what I changed to build the image

@guirish
Copy link
Contributor Author

guirish commented Jul 9, 2020

Note: the changes in this PR are not enough to build the kindest/node image. This is what I changed to build the image

@aledbf Thanks for highlighting the changes required for building node image, this PR is not specific to adding support to build node image. I'm planning to PR node specific changes after this PR. I will look into your changes.

@BenTheElder BenTheElder removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 7, 2020
@BenTheElder BenTheElder changed the title Added support for cross building s390x kind binary and associated images cross build s390x kind binary and associated images by default Aug 7, 2020
@BenTheElder
Copy link
Member

support already existed without this change (PLATFORMS is ?=, so you can supply it and it will accept the value you set instead, the and the go binary / CLI is trivially compileable on / for s390x without the release / cross build script).

what this change actually does is build them by default.

building them by default is NOT a blocker to using kind on this platform, with respect to kubernetes/kubernetes#93621

conformance is a pretty good start, though we usually aim to run more e2e test cases than that.
integration etc. are also useful, but maybe not as much for this case.

I have been re-working the node image build to support more platforms more easily. It has been slow going with other things going on, particularly kubernetes/test-infra#18551

@guirish
Copy link
Contributor Author

guirish commented Aug 7, 2020

Yes @BenTheElder , I agree that we can cross build kind binary and associated images excluding node by setting PLATFORM value to respective disto/ARCH, my intention here for raising this PR was to get s390x binary as release similar to amd64,arm64,arm and ppc64le. We are currently trying to build node image using kind and bazel locally, as node image is required in running conformance tests.

@BenTheElder BenTheElder added this to the v0.10.0 milestone Sep 1, 2020
@BenTheElder
Copy link
Member

I just realized I didn't respond to this last comment, sorry.

my intention here for raising this PR was to get s390x binary as release similar to amd64,arm64,arm and ppc64le. We are currently trying to build node image using kind and bazel locally, as node image is required in running conformance tests.

Yes, but I'm not overly interested in releasing for another platform that isn't actually functional. We're not removing the current platforms so as not to break anyone already choosing to depend on these. but I'm going to start noting the level of support for these.

building the base image has gotten so much slower with these additional platforms and they're of little use and demand to us at the moment. arm at least has some growing developer machine usage and provided some upstream CI signal.

I will revisit this in the 0.10 timeframe, we're trying to finish up the 0.9 release. In the meantime you can build images and binaries for this platform without these changes.

@barthy1
Copy link

barthy1 commented Oct 28, 2020

Hi @BenTheElder are there any updates for this topic?
We'd like to use kind to run knative tests for s390x. I was able to build and start kind cluster locally, but it would be great if kindest/kindnetd and kindest/base will be available for s390x to reduce customizations and code changes..

@BenTheElder BenTheElder modified the milestones: v0.10.0, v0.11.0 Jan 25, 2021
@BenTheElder
Copy link
Member

see #2024: a better plan for multi-arch is on the maintainers agenda for the next release (previous release just this past friday). this PR or a similar change should likely fall in v0.11

Base automatically changed from master to main March 16, 2021 23:22
@BenTheElder BenTheElder modified the milestones: v0.11.0, v0.12.0 May 18, 2021
@BenTheElder BenTheElder added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 24, 2021
@BenTheElder
Copy link
Member

not sure why this never ran in CI, I think the label got lost

@BenTheElder
Copy link
Member

#25 2.119 go: github.com/coreos/[email protected]: Get "https://proxy.golang.org/github.com/coreos/go-iptables/@v/v0.4.5.mod": tls: invalid signature by the server certificate: ECDSA verification failure
#25 ERROR: executor failed running [/bin/sh -c go mod download]: exit code: 1

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/1705/pull-kind-build/1407940393344962560

this came up once before in kubernetes also with an s390x cross build I think, but I can't seem to recall what the issue is.

sorry for the ridiculous delay here, I've set aside time tonight to burn through the PR backlog as opposed to incoming stuff and only just noticed this one was still lingering

@BenTheElder
Copy link
Member

golang/go#40949 seems last time we decided it was qemu. will open another PR to upgrade qemu

@BenTheElder
Copy link
Member

possibly #2324

@BenTheElder
Copy link
Member

/retest
see if #2324 did the trick

@k8s-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
pull-kind-build a3895f2 link /test pull-kind-build

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@guirish
Copy link
Contributor Author

guirish commented Jun 24, 2021

@BenTheElder We are investigating on the build failure specific to s390x with respective QEMU versions.

@BenTheElder
Copy link
Member

there is another popular image for binfmt_misc qemu now, but it also seems to lack a LICENSE tonistiigi/binfmt#46

we are on the latest version of the current image AFAICT, though perhaps an older version works better?

@BenTheElder
Copy link
Member

tongstii/binfmt was updated, I think we should give it a shot here, the other image was last updated 6 months ago but qemu released as recently as april 29 for 6.0.0

@guirish
Copy link
Contributor Author

guirish commented Jun 28, 2021

Hi @BenTheElder , we were able to resolve the build failure for s390x using tonistiigi/binfmt:qemu-v6.0.0image with a workaround. We have introduced a change in kindnetd Dockerfile using GODEBUG asyncpreemptoff=1 go ENV variable, without it the build fails for s390x. Kindly see below the additional changes that were required as a fix and provide your valuable feedback on same.

diff --git a/hack/build/init-buildx.sh b/hack/build/init-buildx.sh
index 6fd26e25..ee3b7f80 100755
--- a/hack/build/init-buildx.sh
+++ b/hack/build/init-buildx.sh
@@ -32,7 +32,9 @@ fi
 # We only need to do this setup on linux hosts
 if [ "$(uname)" == 'Linux' ]; then
   # NOTE: this is pinned to a digest for a reason!
-  docker run --rm --privileged multiarch/qemu-user-static:register@sha256:0434e870ebbe9d28d254c1e65b46969bef458490be5df04d3266ef87380518e2 --reset -p yes
+  docker run --rm --privileged tonistiigi/binfmt:qemu-v6.0.0 --install all
 fi


diff --git a/images/kindnetd/Dockerfile b/images/kindnetd/Dockerfile
index 9d87b16e..040e8d31 100644
--- a/images/kindnetd/Dockerfile
+++ b/images/kindnetd/Dockerfile
@@ -14,14 +14,18 @@

 # first stage build kindnetd binary
# NOTE: tentatively follow upstream kubernetes go version based on k8s in go.mod
-FROM golang:1.15
+FROM --platform=$TARGETPLATFORM golang:1.15
+ARG TARGETPLATFORM
+
WORKDIR /go/src
# make deps fetching cacheable
COPY go.mod go.sum ./
-RUN go mod download
+
+RUN if [ "$TARGETPLATFORM" != "linux/s390x" ] ; then go mod download ; else GODEBUG=asyncpreemptoff=1 go mod download ; fi
+
# build
COPY . .
-RUN CGO_ENABLED=0 go build -o ./kindnetd ./cmd/kindnetd
+RUN if [ "$TARGETPLATFORM" != "linux/s390x" ] ; then CGO_ENABLED=0 go build -o ./kindnetd ./cmd/kindnetd ; else CGO_ENABLED=0 GODEBUG=asyncpreemptoff=1 go build -o ./kindnetd ./cmd/kindnetd ; fi

 # build real kindnetd image
FROM k8s.gcr.io/build-image/debian-iptables:buster-v1.6.3

@BenTheElder
Copy link
Member

@aojea WDYT about generally updating kindnetd deps to 1.16 instead? IIRC 1.15 had these async issues.

I think we should also do a PR to update the qemu image, regardless 👍

@aojea
Copy link
Contributor

aojea commented Jun 29, 2021

@aojea WDYT about generally updating kindnetd deps to 1.16 instead? IIRC 1.15 had these async issues.

👍 are you asking me to do it or is a general question? 😄

@BenTheElder
Copy link
Member

👍 are you asking me to do it or is a general question? 😄

not asking to do it, just asking if you think this is a good plan, I haven't kept up closely enough with kindnetd dependencies recently, I think we should be fine to roll everything to the latest k8s etc.?

@aojea
Copy link
Contributor

aojea commented Jun 29, 2021

+1 are you asking me to do it or is a general question? smile

not asking to do it, just asking if you think this is a good plan, I haven't kept up closely enough with kindnetd dependencies recently, I think we should be fine to roll everything to the latest k8s etc.?

#2342

@BenTheElder
Copy link
Member

#2342 <-- kindnetd go 1.16
#2350 <-- qemu v6

@k8s-ci-robot
Copy link
Contributor

@guirish: 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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2021
@guirish
Copy link
Contributor Author

guirish commented Jul 21, 2021

We are keeping the task of Rebasing the PR changes on hold for now, due to below mentioned reason.

We are awaiting for the new release of tonistiigi/binfmt image to handle the cross image build failure specific to s390x that we are observing currently with latest QEMU stable version. The issue has been fixed in the latest master HEAD for QEMU, I have verified it locally with kindnetd cross image build. Once new image version is released we will retest and rebase the code with associated changes.

@aojea
Copy link
Contributor

aojea commented Oct 8, 2021

@guirish where are we standing here?
do you still want to get this in 0.12 version?

@guirish
Copy link
Contributor Author

guirish commented Oct 8, 2021

@guirish where are we standing here? do you still want to get this in 0.12 version?

@aojea I need to add some additional changes specific to QEMU and also rebase the existing commit. I will retest associated changes and raise a new PR or update the existing PR by Monday.

@guirish
Copy link
Contributor Author

guirish commented Oct 12, 2021

@BenTheElder @aojea I am closing this PR, I have added changes in a new PR #2494 .

@aojea
Copy link
Contributor

aojea commented Oct 12, 2021

@BenTheElder @aojea I am closing this PR, I have added changes in a new PR #2494 .

@aojea aojea closed this Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants