Use the right image for the right platform in the e2e tests #49457

mkumatag · 2017-07-23T18:15:08Z

What this PR does / why we need it:
This PR is for enabling kubernetes tests for multi architecture platform
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #38067

Special notes for your reviewer:
This will enable conformance tests for all the supported architectures.
Release note:

Make all e2e tests lookup image to use from a centralized place. In that centralized place, add support for multiple platforms.

x-ref #38067

k8s-ci-robot · 2017-07-23T18:15:15Z

Hi @mkumatag. Thanks for your PR.

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

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

mkumatag · 2017-07-23T18:25:49Z

/assign
/cc @luxas

luxas

Thanks a LOT for this beast! 🎉

Some initial points to discuss... let's get feedback from @ixdy and @kubernetes/sig-testing-pr-reviews before making the rest of the changes so we have consensus on the direction.

Can you please structure your commits like this?

Adding the imagemanifest or imageutils package (net-new)
Redirecting all references in Go code to imageutils.GetE2EImage() (replace)
Remove all unnecessary constants that were used before
Replace everything templated in YAML to use {{ .Field }} instead of hard-coding
Updating bazel and other required and automated things

I'm actually fine with doing this in three PRs

1 -- only adding the new thing
2, 3 and 5 -- main piece of work
4 and 5 -- takes care of YAML files used in tests.

What do others think about that?

luxas · 2017-07-24T18:13:55Z

test/e2e/framework/volume_util.go

@@ -61,7 +61,6 @@ const (
 	GlusterfsServerImage string = "gcr.io/google_containers/volume-gluster:0.2"
 	CephServerImage      string = "gcr.io/google_containers/volume-ceph:0.1"
 	RbdServerImage       string = "gcr.io/google_containers/volume-rbd:0.1"


I guess these images will be cross-built in the future as well?

As these images are bit complex and need lot of effort porting them for other archs. Anyways those images are not used in conformance tests.

luxas · 2017-07-24T18:18:43Z

examples/guestbook/frontend-deployment.yaml

@@ -12,7 +12,7 @@ spec:
    spec:
      containers:
      - name: php-redis
-        image: gcr.io/google-samples/gb-frontend:v4
+        image: {{frontendImage}}


Leave out all examples/ changes, they have been moved to a new repo

luxas · 2017-07-24T18:21:07Z

test/utils/imagemanifest/manifest.go

+	redisslaveImageVersion         = "1.0"
+	resourceConsumerImageName      = "resource-consumer"
+	resourceConsumerImageVersion   = "1.0"
+	resourceControllerImageName    = "resource-consumer/controller"


is this actually right?

luxas · 2017-07-24T18:22:02Z

test/utils/imagemanifest/manifest.go

+)
+
+const (
+	testRegistry                   = "gcr.io/kubernetes-e2e-test-images"


suggest: e2eImagesRegistry

luxas · 2017-07-24T18:33:17Z

test/utils/imagemanifest/manifest.go

+}
+
+// GetclusterTesterImage returns multi-arch cluster-image docker image
+func GetclusterTesterImage() string {


Could we just structure it like this?

const ( Foo = "foo" Bar = "bar" Busybox = "busybox" ) var imageVersions = map[string]string{ Foo: "1.0", Bar: "1.1", } var imageExceptions = map[string](func() string){ BusyBox: getBusyBoxImage, } func GetE2EImage(image string) string { if nameFunc, ok := imageExceptions[image]; ok { return nameFunc() } return fmt.Sprintf("%s/%s-%s:%s", e2eImagesRegistry, image, runtime.GOARCH, imageVersions[image]) }

luxas · 2017-07-24T18:33:45Z

test/utils/imagemanifest/BUILD

@@ -0,0 +1,27 @@
+package(default_visibility = ["//visibility:public"])


can you update bazel in your last commit please so its easier to review?

luxas · 2017-07-24T18:35:42Z

test/fixtures/doc-yaml/user-guide/update-demo/kitten-rc.yaml

@@ -13,7 +13,7 @@ spec:
        version: kitten
    spec:
      containers:
-      - image: gcr.io/google_containers/update-demo:kitten
+      - image: {{kittenImage}}


Is this file actually used by e2e tests?
If it is a template, the filename must indicate that (may it be with .in or whatever)
If it's used by the e2e tests and intended to be a template, please use the normal {{ .Field }} syntax

This goes for all gcr.io/google_containers/foo:bar => {{fooIimage}} changes

luxas · 2017-07-24T18:37:40Z

test/utils/imagemanifest/manifest.go

+limitations under the License.
+*/
+
+package imagemanifest


@ixdy preference on name here?
What about just image and then import as imageutils?

luxas · 2017-07-24T18:39:10Z

test/e2e/kubectl/kubectl.go

+	busyboxImage  = imagemanifest.GetBusyboxImage()
+)
+
+var funcMap = template.FuncMap{


Can we use a normal map[string]string instead so we can use the more common {{ .Field }} format?

luxas · 2017-07-24T18:42:43Z

test/e2e/common/apparmor.go

@@ -84,7 +84,7 @@ done`, testCmd)
 		Spec: api.PodSpec{
 			Containers: []api.Container{{
 				Name:    "test",
-				Image:   "gcr.io/google_containers/busybox:1.24",
+				Image:   busyboxImage,


What do we feel about referencing some custom busyboxImage variable vs always calling imageutils?
To me it seems like always calling the SSOT makes sense, regardless of if we there prefix things with -GOARCH or not.

I don't think the busyboxImage shorthands makes things much shorter actually, as there are duplicates everywhere (a new BusyboxImage var has to be created in every new package, instead of always referencing the canonical place)

I also have a similar opinion, will update the tests to call GetE2EImage function

luxas · 2017-07-24T18:54:06Z

Also, this already needs a rebase as things are moving from test/e2e to test/e2e/$SIG

/release-note
/ok-to-test

mkumatag · 2017-07-26T12:00:00Z

Addressed most of the comments, PTAL at latest set of commits.

ixdy · 2017-07-27T06:42:00Z

test/e2e/apps/rc.go

@@ -45,7 +46,7 @@ var _ = SIGDescribe("ReplicationController", func() {
 		// requires private images
 		framework.SkipUnlessProviderIs("gce", "gke")

-		TestReplicationControllerServeImageOrFail(f, "private", "gcr.io/k8s-authenticated-test/serve_hostname:v1.4")
+		TestReplicationControllerServeImageOrFail(f, "private", imageutils.GetE2EImage(imageutils.ServeHostname))


this one looks wrong. it is intentionally testing a private repo, which gcr.io/kubernetes-e2e-test-images is not.

then we may need to push serve_hostname image to gcr.io/k8s-authenticated-test repo as well for all the architectures or do you have any other way to tackle this?

@ixdy as we already have a way to push these images to user defined repository, can you please run following command to push the images to gcr.io/k8s-authenticated-test repository

make all-push WHAT=serve-hostname REGISTRY=gcr.io/k8s-authenticated-test

ixdy · 2017-07-27T06:45:58Z

test/utils/image/manifest.go

@@ -0,0 +1,141 @@
+/*


I don't have a particularly strong opinion here, but should this live in test/images instead of adding a new test/utils/image directory?

As we are maintaining only docker images in test/images so IMO it will be more relevant in test/utils/images directory.

I'm okay with this

ixdy · 2017-07-27T06:48:03Z

test/utils/image/manifest.go

+
+const (
+	e2eImagesRegistry  = "gcr.io/kubernetes-e2e-test-images"
+	BusyBox            = "busybox"


maybe sort these into groups by which registry they're in? i.e. most are in kubernetes-e2e-test-images, but some are in google-containers, and the way they're written right now doesn't make that super obvious.

same thought goes for imageVersions.

I'm also worried about the vertical separation/disconnect between the image names, registry, and versions.

I'm starting to wonder if there should instead be more structure around the configs, literally using a struct or something, like so:

const ( e2eRegistry = "gcr.io/kubernetes-e2e-test-images" gcRegistry = "gcr.io/kubernetes-e2e-test-images" ) type ImageConfig struct { registry string name string version string } var ( BusyBox = ImageConfig{gcRegistry, "busybox", "1.24"} ClusterTester = ImageConfig{e2eRegistry, "clusterapi-tester", "1.0"} ... ) func GetArchImage(config ImageConfig) string { return fmt.Sprintf("%s/%s-%s:%s", config.registry, config.name, runtime.GOARCH, config.version) }

thoughts? it's a bit more succinct.

(I guess really kubernetes should figure out how to use the right arch image itself, which is probably something @luxas has written about and I haven't caught up on. :)

The above approach looks more cleaner and will reduce good amount of coding, @luxas what do you say about it?

@ixdy How gcr.io/google-containers/busybox:1.24 different from docker hub busybox? I'm wondering how to get busybox images for different architectures.!

I think gcr.io/google-containers/busybox:1.24 is just the official docker busybox:1.24 that was downloaded at some point and pushed back to gcr.io/google-containers, since gcr.io was/is more reliable than dockerhub (#13288).

as with many things, I'm not sure if this is still necessary.

in particular, busybox was #23248.

I'm fine with any option, what do you recommend?
Option 1 - Refer busybox from docker hub
Option 2 - Pull busybox images for all the architecture and keep it in gcr.

My vote is for option 1 to avoid any maintenance required for pushing same image to gcr for all the architecture.!

I think we should use Busybox from Docker Hub, yes. That will get us security patches automatically. I think Docker Hub is more reliable now... or?
cc @fejta

@ixdy We'll ideally, we'd use manifest lists. Plan B might be to lookup the API server platform by hitting /version on every image get, but that's kinda painful. Plan C (which this is, and is what I think we should stick with now) is to use runtime.GOARCH from the e2e binary. If you have an arm cluster, you have to make sure you call e2e tests from an arm machine as well, etc.

mkumatag · 2017-09-01T00:20:43Z

This cronjob pr created a lot of chaos today! btw : rebased the PR again.

@mkumatag looks like you need to rebase again. :( (the change to test/e2e/apps/types.go got reverted.)

mkumatag · 2017-09-01T01:31:12Z

/test pull-kubernetes-e2e-gce-etcd3

mkumatag · 2017-09-01T03:52:37Z

@ixdy @luxas because of rebase lgtm got removed, can you please gimme lgtm again to merge this PR.

luxas · 2017-09-01T08:12:32Z

/lgtm

k8s-github-robot · 2017-09-01T08:36:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ixdy, luxas, mkumatag

Associated issue: 38067

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~hack/OWNERS~~ [ixdy]
~~test/OWNERS~~ [ixdy]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

mkumatag · 2017-09-01T11:43:09Z

/retest

mkumatag · 2017-09-01T12:34:04Z

/test pull-kubernetes-kubemark-e2e-gce

luxas · 2017-09-01T13:45:45Z

/retest

mkumatag · 2017-09-01T15:42:28Z

/retest

mkumatag · 2017-09-01T18:32:33Z

/retest

mkumatag · 2017-09-02T05:28:52Z

/retest

mkumatag · 2017-09-02T08:24:20Z

/retest

mkumatag · 2017-09-02T13:23:01Z

/retest

mkumatag · 2017-09-02T16:27:23Z

/retest

mkumatag · 2017-09-02T18:57:53Z

/retest

luxas · 2017-09-02T20:28:05Z

/retest

k8s-github-robot · 2017-09-02T20:47:43Z

/test all [submit-queue is verifying that this PR is safe to merge]

mkumatag · 2017-09-02T21:31:52Z

/retest

k8s-ci-robot · 2017-09-02T21:45:21Z

@mkumatag: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`ee4d54c`	link	`/test pull-kubernetes-e2e-kops-aws`

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.

k8s-github-robot · 2017-09-02T22:18:10Z

Automatic merge from submit-queue

mkumatag · 2017-09-03T00:33:20Z

Finally got merged after so many retests... Thanks @ixdy @luxas

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 23, 2017

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 23, 2017

k8s-github-robot assigned madhusudancs and deads2k Jul 23, 2017

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jul 23, 2017

mkumatag force-pushed the tests_multiarch branch from 7355885 to bf0a637 Compare July 23, 2017 18:24

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2017

k8s-ci-robot assigned mkumatag Jul 23, 2017

k8s-ci-robot requested a review from luxas July 23, 2017 18:25

deads2k removed their assignment Jul 24, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2017

luxas reviewed Jul 24, 2017

View reviewed changes

mkumatag force-pushed the tests_multiarch branch from bf0a637 to 952698d Compare July 26, 2017 06:21

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2017

mkumatag force-pushed the tests_multiarch branch from 952698d to 5fc0259 Compare July 26, 2017 06:58

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 26, 2017

mkumatag force-pushed the tests_multiarch branch from 5fc0259 to bcefdf8 Compare July 26, 2017 11:48

mkumatag mentioned this pull request Jul 27, 2017

[WIP] Image catalog for multiarch images #45074

Closed

ixdy reviewed Jul 27, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2017

mkumatag added 4 commits September 1, 2017 05:39

Test image utils for multi arch test images

6f55de1

Update the yaml file with multiarch images

87e2321

Fix bazel

22c3a59

Port e2e tests for multi architecture

ee4d54c

mkumatag force-pushed the tests_multiarch branch from 418fb21 to ee4d54c Compare September 1, 2017 00:11

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017

luxas added this to the v1.8 milestone Sep 1, 2017

k8s-github-robot merged commit da7ee10 into kubernetes:master Sep 2, 2017

		@@ -0,0 +1,27 @@
		package(default_visibility = ["//visibility:public"])

Use the right image for the right platform in the e2e tests #49457

Use the right image for the right platform in the e2e tests #49457

Conversation

mkumatag commented Jul 23, 2017 • edited Loading

k8s-ci-robot commented Jul 23, 2017

mkumatag commented Jul 23, 2017

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas commented Jul 24, 2017

mkumatag commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkumatag Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkumatag Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkumatag commented Sep 1, 2017

mkumatag commented Sep 1, 2017

mkumatag commented Sep 1, 2017

luxas commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

mkumatag commented Sep 1, 2017

mkumatag commented Sep 1, 2017

luxas commented Sep 1, 2017

mkumatag commented Sep 1, 2017

mkumatag commented Sep 1, 2017

mkumatag commented Sep 2, 2017

mkumatag commented Sep 2, 2017

mkumatag commented Sep 2, 2017

mkumatag commented Sep 2, 2017

mkumatag commented Sep 2, 2017

luxas commented Sep 2, 2017

k8s-github-robot commented Sep 2, 2017

mkumatag commented Sep 2, 2017

k8s-ci-robot commented Sep 2, 2017 • edited Loading

k8s-github-robot commented Sep 2, 2017

mkumatag commented Sep 3, 2017

mkumatag commented Jul 23, 2017 •

edited

Loading

mkumatag Aug 3, 2017 •

edited

Loading

mkumatag Jul 27, 2017 •

edited

Loading

k8s-ci-robot commented Sep 2, 2017 •

edited

Loading