-
Notifications
You must be signed in to change notification settings - Fork 39.7k
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
Improve the multiarch situation; armel => armhf; reenable pcc64le; remove the patched golang #38926
Conversation
The problem is here that we can't upgrade to go1.8 yet due to vendoring issues. I caught this at least:
Is there anyone from Google that is responsible for upgrading to go1.8 in time for v1.6? |
@luxas here's the latest status from @jessfraz #38228 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be worth fixing some of these things (e.g. moving everything to go1.7) while we're working through the go1.8 issues?
@@ -31,14 +31,14 @@ REGISTRY_TAG?=3.0.14 | |||
ARCH?=amd64 | |||
REGISTRY?=gcr.io/google_containers | |||
GOLANG_VERSION?=1.6.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we bump this one too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I'll do it
@@ -22,13 +22,14 @@ CNI_RELEASE?=07a8a28637e97b22eb8dfe710eeae1344f69d16e | |||
CNI_TARBALL=cni-$(ARCH)-$(CNI_RELEASE).tar.gz | |||
CUR_DIR=$(shell pwd) | |||
OUTPUT_DIR=$(CUR_DIR)/output | |||
GOLANG_VERSION=1.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to pin this to an explicit version? otherwise docker run
may not fetch a newer patch release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any way to make docker always pull on docker run?
I'm sure there is, but seems like I've forgotten it for the moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker build
has --pull
, but for run
you have to docker pull
then docker run
- see moby/moby#13331.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might still want to fix this. can we make this golang:1.7.4
explicitly?
@@ -25,15 +25,15 @@ ARCH?=amd64 | |||
TAG?=1.9 | |||
REGISTRY?=gcr.io/google_containers | |||
|
|||
GOLANG_VERSION=1.6 | |||
GOLANG_VERSION=1.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is even used here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope it isn't. this was just an automatic replace. will fix
@@ -29,7 +29,7 @@ ARCH ?= amd64 | |||
|
|||
ALL_ARCH = amd64 arm arm64 ppc64le s390x | |||
|
|||
GOARM=6 | |||
GOARM=7 | |||
TEMP_DIR := $(shell mktemp -d) | |||
GOLANG_VERSION = 1.6.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe update this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes gonna
@@ -79,15 +79,12 @@ RUN export ETCD_VERSION=v3.0.14; \ | |||
&& ln -s ../src/etcd/etcd-${ETCD_VERSION}-linux-amd64/etcd /usr/local/bin/ | |||
|
|||
# TODO: Remove the patched GOROOT when we have an official golang that has a working arm and ppc64le linker | |||
ENV K8S_PATCHED_GOLANG_VERSION=1.7.4 \ | |||
ENV K8S_PATCHED_GOLANG_VERSION=go1.8beta2 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd kinda like to see us get away from having two separate go versions. Wonder if go1.8 will land in time for k8s 1.6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I really hope it will. That's my intention and goal.
We have to validate it and do the hard work with the betas though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem, and the reason this is wip is that k8s is not working with go1.8 yet :(
I think I found the root cause, we might just have to bump runc: opencontainers/runc#1116 |
@runcom Even after upgrading runc I get the following when running on arm for all kube binaries on startup:
using go1.8, latest runc and running on ARM. Do you know what the cause could be? |
cc @vmarmol @crosbymichael @hqhq and @cyphar on the above Do you know what the cause could be? Why is this code running on |
@luxas How is |
@cyphar I believe the problem is that The problem is that I can't find how it's run at startup and why... |
What's even worse is that we don't even vendor |
@luxas Does the first argument equal |
&& GOROOT_FINAL=${K8S_PATCHED_GOROOT} GOROOT_BOOTSTRAP=/usr/local/go ./make.bash \ | ||
&& for platform in linux/arm; do GOOS=${platform%/*} GOARCH=${platform##*/} GOROOT=${K8S_PATCHED_GOROOT} go install std; done | ||
&& for platform in linux/arm linux/ppc64le; do GOOS=${platform%/*} GOARCH=${platform##*/} GOROOT=${K8S_PATCHED_GOROOT} go install std; done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think patching is not required for golang for ppc64le with go1.8. All the changes are already upstreamed in go1.8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR makes k8s not use a patched golang, but the name will still be K8S_PATCHED_GOROOT
until k8s uses go1.8 offically
b4e8a23
to
758b455
Compare
758b455
to
aa77111
Compare
aa77111
to
73d2053
Compare
yeah, easy repro outside of our containerized build is to use |
…1.8rc1 for arm and ppc64le instead. Reenable the ppc64le builds
…and goarm=6 => goarm=7 and use go 1.7.4
db48b8b
to
3229c5e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like you already fixed the one main issue. minor stylistic comment, otherwise I think this basically LG. I'll do one more full pass in a little bit.
V=2 kube::log::info "Env for ${platform}: GOOS=${GOOS-} GOARCH=${GOARCH-} GOROOT=${GOROOT-} CGO_ENABLED=${CGO_ENABLED-} CC=${CC-}" | ||
# Temporary workaround while we have two GOROOT's (which we'll get rid of as soon as we upgrade to go1.8 for amd64 as well) | ||
local GO=go | ||
if [[ ${GOROOT} == ${K8S_EDGE_GOROOT:-} ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: per style, we normally put vars inside double quotes. so e.g. here
if [[ "${GOROOT}" == "${K8S_EDGE_GOROOT:-}" ]]; then
# Temporary workaround while we have two GOROOT's (which we'll get rid of as soon as we upgrade to go1.8 for amd64 as well) | ||
local GO=go | ||
if [[ ${GOROOT} == ${K8S_EDGE_GOROOT:-} ]]; then | ||
GO=${K8S_EDGE_GOROOT}/bin/go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"${K8s_EDGE_GOROOT}/bin/go"
@@ -526,7 +532,7 @@ kube::golang::build_binaries_for_platform() { | |||
kube::log::progress " " | |||
for binary in "${statics[@]:+${statics[@]}}"; do | |||
local outfile=$(kube::golang::output_filename_for_binary "${binary}" "${platform}") | |||
CGO_ENABLED=0 go build -o "${outfile}" \ | |||
CGO_ENABLED=0 ${GO} build -o "${outfile}" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"${GO}"
etc
…go executable to EDGE_GOROOT/bin/go when using the edge version
3229c5e
to
6789d4e
Compare
@ixdy Fixed the nits. |
@ixdy Please lgtm now so we can get it in today. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED The following people have approved this PR: jbeda Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/lgtm |
@ixdy I accidently pushed addon-manager v6.3 to gcr.io two days ago and might overrided your push. Please re-push it before this merge. Sorry for the mistake :( |
I can repush after this merges. Nothing is actually using v6.3 yet, I think. |
Oh I just notice you are not updating the salt file. Yeah sounds good, thanks! |
@k8s-bot unit test this |
1 similar comment
@k8s-bot unit test this |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue |
What this PR does / why we need it:
Release note:
@kubernetes/sig-testing-misc @jessfraz @ixdy @jbeda @david-mcmahon @pwittrock