Make kubelet use an arch-specific pause image depending on GOARCH #23059

luxas · 2016-03-16T17:02:56Z

Related to: #22876, #22683 and #15140
@ixdy @pwittrock @brendandburns @mikedanese @yujuhong @thockin @zmerlynn

k8s-bot · 2016-03-16T17:04:50Z

GCE e2e build/test failed for commit 66751b4bb050120ac8ef4ccce6e8bb75768a6ff9.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

k8s-github-robot · 2016-03-16T17:16:12Z

Labelling this PR as size/S

mikedanese · 2016-03-16T17:35:12Z

pkg/kubelet/types/constants.go

+	if runtime.GOARCH == "amd64" {
+		return DefaultPodInfraContainerImageName + ":" + DefaultPodInfraContainerImageVersion
+	} else {
+		return DefaultPodInfraContainerImageName +  "-" + runtime.GOARCH + ":" + DefaultPodInfraContainerImageVersion


I know this pattern was not introduced here but I'm confused as to why we have been putting arch in the image name and not the build tag. It seems like it should be:

gcr.io/google_containers/hyperkube:v1.1.8 gcr.io/google_containers/hyperkube:v1.1.8-386 gcr.io/google_containers/hyperkube:v1.1.8-amd64 gcr.io/google_containers/hyperkube:v1.1.8-arm gcr.io/google_containers/hyperkube:v1.2.0 gcr.io/google_containers/hyperkube:v1.2.0-386 gcr.io/google_containers/hyperkube:v1.2.0-amd64 gcr.io/google_containers/hyperkube:v1.2.0-arm

Ask @brendandburns :)
I'm just following the conventions, but I do prefer binary-arch:version too, and it's implemented at quite many places atm.
I think we should go with that, but that's just my opinion :)

I'll try to convince @david-mcmahon next time i see him :)

Haha, make sure to have an issue or something if you're planning to change that, so you're listening to the community :)

However, the important thing is consistency. We must choose either and go with that all the time. binary-arch:version is already implemented in the codebase and for example arm images are pushed with every release with binary-arch:version naming, so I don't know if it's worth the effort to change.

I think our consistent should be consistent with other projects. Multi architecture support is recent enough IMO that we could still change it.

Yeah, it's work in progress. Images like etcd, hyperkube, kube-proxy are turned into etcd-amd64, hyperkube-amd64 and kube-proxy-amd64. The older naming convention (hyperkube is kept for backwards-compability for a while though)

@mikedanese What do you have to convince me of? I wasn't involved in any of this naming. My general guidance here is to follow a (sane) precedent and to be consistent with other projects.

So do you vote for binary-arch:version or binary:version-arch?
And no, you weren't involved in the naming, but now you are :)

What is typical for docker containers / other projects?

Docker at least uses armhf/, armel/, ppc64le/ and so on for the "official" images.
Not sure if that's related to this, but anyway

k8s-bot · 2016-03-16T17:49:08Z

GCE e2e build/test passed for commit a6368c13fd842401cee54f6ebd596a97ad62a4cb.

luxas · 2016-03-16T17:59:15Z

As #15140 points out, there is many places where the pause image should be standardized and there should be a constant somewhere that the whole codebase should use, but let's take one problem at a time.

vishh · 2016-03-16T18:28:29Z

pkg/kubelet/types/constants.go

 	// system default DNS resolver configuration
 	ResolvConfDefault = "/etc/resolv.conf"
+
+	DefaultPodInfraContainerImageName = "gcr.io/google_containers/pause"


nit: I'd prefer this logic staying out of the kubelet package. cmd/kubelet seems like a better location

I agree. we should toggle this when we create the flag so it shows up in --help.

https://github.com/kubernetes/kubernetes/blob/master/cmd/kubelet/app/options/options.go#L154

So where should I put it? cmd/kubelet/app/constants.go, cmd/kubelet/app/pause.go or cmd/kubelet/app/options/options.go, which do you prefer?

Another option would be breaking out all pause constants to some other place not directly tied to kubelet.

luxas · 2016-03-24T20:12:32Z

@vishh Updated. PTAL

vishh · 2016-03-24T20:48:02Z

pkg/kubelet/types/constants.go

@@ -17,7 +17,7 @@ limitations under the License.
 package types

 const (
-	PodInfraContainerImage = "gcr.io/google_containers/pause:2.0"
+


nit: Remove this empty line.

k8s-bot · 2016-03-24T20:56:12Z

GCE e2e build/test passed for commit b6f0da976ea49b595c8423c685099c78b77edebe.

luxas · 2016-03-24T21:35:19Z

@vishh Fixed the nit. Does this look good to you now?

k8s-bot · 2016-03-24T21:59:59Z

GCE e2e build/test passed for commit d8aafd222902d33df9edd138482ab723cfdd5cab.

vishh · 2016-03-25T17:58:06Z

LGTM.

vishh · 2016-03-25T17:58:42Z

@mikedanese: Do you have any other comments on this PR? If not, this is ready to be merged.

mikedanese · 2016-03-25T18:10:56Z

Even though I despise the image naming convention we've settled on (probably decided by brendan and joe years ago), I will most likely survive. lgtm

mikedanese · 2016-03-25T18:11:46Z

needs an update-generated-docs to update man pages

luxas · 2016-03-25T18:37:04Z

hack/update-generated-docs.sh didn't update anything and hack/verify-generated-docs.sh returned Generated docs are up to date..

Should I run something else?

mikedanese · 2016-03-25T18:39:44Z

Oops bad guess:

FAIL    k8s.io/kubernetes/pkg/kubelet/dockertools [build failed]

k8s-github-robot · 2016-03-25T21:55:04Z

Labelling this PR as size/M

k8s-bot · 2016-03-25T22:22:53Z

GCE e2e build/test passed for commit c6172e5.

k8s-github-robot · 2016-03-25T22:25:01Z

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

luxas · 2016-03-26T06:22:39Z

This works nicely on an ARM host, I tested it now.

$ kubelet --help
...
--pod-infra-container-image="gcr.io/google_containers/pause-arm:2.0": The image whose network/ipc namespaces containers in each pod will use.
...

And it starts pause-arm containers automatically.
On amd64 hosts, the default is still gcr.io/google_containers/pause:2.0 (which probably will be updated to gcr.io/google_containers/pause-amd64:3.0 soon)

thockin · 2016-03-26T20:12:46Z

Before we commit this:

Mike,

Why? Can you explain any value or cost to one over the other? To my eyes
foo:1.2-arm and foo:1.3-amd64 are two point releases in the same series (as
per semver-ish semantics). That's wrong. OTOH foo-arm:1.2 and
foo-amd64:1.2 are clearly apples and oranges. I still dislike the ad-hoc
nature of it.

Are there any other projects with multi-arch outputs? What are they
doing? What is Docker itself doing?
On Mar 25, 2016 11:11 AM, "Mike Danese" notifications@github.com wrote:

Even though I despise the image naming convention we've settled on
(probably decided by brendan and joe years ago), I will most likely
survive. lgtm

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#23059 (comment)

luxas · 2016-03-26T21:14:11Z

Are there any other projects with multi-arch outputs?

I'm sorry but I don't think there is any other large open-source Go project that is multiarch and comparable to Kubernetes.
Sample Github search

What is Docker itself doing?

moby/moby#8256

thockin · 2016-03-26T21:40:07Z

From that docker issue, do I understand that it is largely undecided yet?

I personally think arch has higher precedence than version, so given only
two fields (name and version) this belongs in the name.
On Mar 26, 2016 2:14 PM, "Lucas Käldström" notifications@github.com wrote:

Are there any other projects with multi-arch outputs?

I'm sorry but I don't think there is any other large open-source Go
project that is multiarch and compareable to Kubernetes.
Sample Github search
https://github.com/search?l=&o=desc&q=language%3AGo&ref=advsearch&s=stars&type=Repositories&utf8=%E2%9C%93

What is Docker itself doing?

moby/moby#8256 moby/moby#8256

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#23059 (comment)

luxas · 2016-03-27T09:40:11Z

Yes, I also see it as largely undecided, and I think it will take a while before docker really implement something usable.

I think these images should be multiarch primarily:
Get docker-multinode working:

gcr.io/google_containers/hyperkube, pending Cross-build hyperkube and debian-iptables for ARM. Also add a flannel image #21617
gcr.io/google_containers/etcd, fixed in Make the etcd image cross-platform #19216
gcr.io/google_containers/flannel, pending Cross-build hyperkube and debian-iptables for ARM. Also add a flannel image #21617
gcr.io/google_containers/pause, pending Reimplement 'pause' in C - smaller footprint all around #23009

Get DNS working:

gcr.io/google_containers/kube2sky, pending Make kube2sky and skydns docker images cross-platform #19376
gcr.io/google_containers/skydns, pending Make kube2sky and skydns docker images cross-platform #19376
gcr.io/google_containers/exechealthz pending Add cross-compiling support for exechealthz kubernetes-retired/contrib#646

Docker wrapped Kubernetes binaries:

gcr.io/google_containers/kube-proxy, fixed in Official ARM builds for every new release #19769
gcr.io/google_containers/kube-apiserver, fixed in Official ARM builds for every new release #19769
gcr.io/google_containers/kube-controller-manager, fixed in Official ARM builds for every new release #19769
gcr.io/google_containers/kube-scheduler, fixed in Official ARM builds for every new release #19769
gcr.io/google_containers/kubelet, doesn't really exist yet: Add docker images for kubelet and kubelet-checker. #19177

I worked with the dashboard team, and it has had multiarch support from the first release.
I'm maybe gonna add multiarch support for official heapster too, now when it has reached v1.
On kubernetes-on-arm, I already have heapster, influxdb and grafana nicely running as a cluster addon.

When we once has ported Kubernetes to ARM and decided about the naming, it's much easier to add support for ARM 64-bit, ppc64le and maybe some other arch, who knows?

luxas · 2016-03-29T18:46:14Z

@thockin @mikedanese @vishh Can we proceed with this one?
This PR isn't that tightly integrated with the naming as it uses two already existing images as the default (pause and pause-arm)
It this ok?

vishh · 2016-03-29T23:43:26Z

LGTM

vishh · 2016-03-29T23:43:51Z

@thockin

I personally think arch has higher precedence than version, so given only
two fields (name and version) this belongs in the name.

+1

k8s-github-robot · 2016-03-29T23:46:48Z

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

k8s-bot · 2016-03-30T00:29:51Z

GCE e2e build/test passed for commit c6172e5.

k8s-github-robot · 2016-03-30T05:56:04Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-03-30T06:25:50Z

GCE e2e build/test passed for commit c6172e5.

k8s-github-robot · 2016-03-30T06:27:42Z

Automatic merge from submit-queue

googlebot added the cla: yes label Mar 16, 2016

luxas force-pushed the auto_arch_pause branch from 66751b4 to a6368c1 Compare March 16, 2016 17:13

k8s-github-robot assigned vishh Mar 16, 2016

k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 16, 2016

mikedanese reviewed Mar 16, 2016
View reviewed changes

vishh reviewed Mar 16, 2016
View reviewed changes

luxas mentioned this pull request Mar 23, 2016

Reimplement 'pause' in C - smaller footprint all around #23009

Merged

luxas force-pushed the auto_arch_pause branch from a6368c1 to b6f0da9 Compare March 24, 2016 20:11

vishh reviewed Mar 24, 2016
View reviewed changes

luxas force-pushed the auto_arch_pause branch from b6f0da9 to d8aafd2 Compare March 24, 2016 21:27

Make kubelet use an arch-specific pause image depending on GOARCH

c6172e5

luxas force-pushed the auto_arch_pause branch from d8aafd2 to c6172e5 Compare March 25, 2016 21:46

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 25, 2016

k8s-github-robot added the needs-ok-to-merge label Mar 25, 2016

luxas mentioned this pull request Mar 28, 2016

[Proposal] Make Kubernetes run on ARM devices #17981

Closed

17 tasks

vishh added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge labels Mar 29, 2016

k8s-github-robot removed the needs-ok-to-merge label Mar 30, 2016

k8s-github-robot merged commit 2a690cc into kubernetes:master Mar 30, 2016

luxas mentioned this pull request Aug 19, 2016

Makefile: Make the ARCH part of the tag name not the image name and add pthread to busybox flannel-io/flannel#489

Merged

Make kubelet use an arch-specific pause image depending on GOARCH #23059

Make kubelet use an arch-specific pause image depending on GOARCH #23059

Uh oh!

Conversation

luxas commented Mar 16, 2016

Uh oh!

k8s-bot commented Mar 16, 2016

Uh oh!

k8s-github-robot commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-bot commented Mar 16, 2016

Uh oh!

luxas commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luxas commented Mar 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-bot commented Mar 24, 2016

Uh oh!

luxas commented Mar 24, 2016

Uh oh!

k8s-bot commented Mar 24, 2016

Uh oh!

vishh commented Mar 25, 2016

Uh oh!

vishh commented Mar 25, 2016

Uh oh!

mikedanese commented Mar 25, 2016

Uh oh!

mikedanese commented Mar 25, 2016

Uh oh!

luxas commented Mar 25, 2016

Uh oh!

mikedanese commented Mar 25, 2016

Uh oh!

k8s-github-robot commented Mar 25, 2016

Uh oh!

k8s-bot commented Mar 25, 2016

Uh oh!

k8s-github-robot commented Mar 25, 2016

Uh oh!

luxas commented Mar 26, 2016

Uh oh!

thockin commented Mar 26, 2016

Uh oh!

luxas commented Mar 26, 2016

Uh oh!

thockin commented Mar 26, 2016

Uh oh!

luxas commented Mar 27, 2016

Uh oh!

luxas commented Mar 29, 2016

Uh oh!