Fixes #3086 - amd64 images are pulled on all the architectures #3337

bahetiamit · 2020-10-06T09:57:22Z

Changes

This PR fixes an issue wherein even if the multi-arch image is available, always amd64 image is getting pulled (#3086)

This happens as go-containerregistry package pulls amd64 image by default (Reference - https://github.com/google/go-containerregistry/blob/9f2724c4b962e4eb0630f8611f9382fb83e4e2e7/pkg/v1/remote/options.go#L113 )

Although the change is generic and applicable for all other architectures, I was able to successfully test using following e2e test cases on POWER (ppc64le) platform for which ubuntu, busybox and alpine/git images are required and they are multi-arch images.

TestWorkingDirCreated
TestWorkingDirIgnoredNonSlashWorkspace
TestEntrypointRunningStepsInOrder
TestGitPipelineRun
TestGitPipelineRunFail
TestTaskRun_EmbeddedResource

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

The entrypoint lookup will pull the image for the controller's architecture instead of amd64 to better support different architectures

linux-foundation-easycla · 2020-10-06T09:57:24Z

The committers are authorized under a signed CLA.

✅ bahetiamit (e987483)

tekton-robot · 2020-10-06T09:57:37Z

Hi @bahetiamit. Thanks for your PR.

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

vdemeester

/cc @jonjohnsonjr @mattmoor

mattmoor · 2020-10-06T13:53:44Z

I literally hit this last night and was going to debug and fix this today. I love OSS :)

mattmoor · 2020-10-06T13:55:18Z

pkg/pod/entrypoint_lookup_impl.go

+	//By default go-containerregistry pulls amd64 images.
+	//Setting correct image pull architecture based on the underlying platform
+	var pf = v1.Platform {
+        		Architecture: runtime.GOARCH ,
+	        	OS :          "linux",
+        	}


nit: gofmt -s -w this file

Formatted the change.

mattmoor · 2020-10-06T13:59:56Z

pkg/pod/entrypoint_lookup_impl.go

+        		Architecture: runtime.GOARCH ,
+	        	OS :          "linux",
+        	}
+	img, err := remote.Image(ref, remote.WithAuthFromKeychain(mkc), remote.WithPlatform(pf))


This is probably the right band-aid for supporting multi-arch on homogeneous clusters short-term, but we should probably open an issue to further discuss heterogeneous clusters (where the controller may be running on a different platform than the pod gets scheduled on).

One problem with this in general is that different architectures could (technically) have different entrypoints, so to properly support multi-arch on heterogeneous clusters, I think we will need to pass through all of the arch-specific configs and have the endpoint select the appropriate one based on where it schedules.

To give an example of a subtle, but significant variation in config, when playing with multi-arch kaniko I rebuilt with Bazel and ended up with /kaniko/executor_{arch} as the entrypoint in the respective architecture images. So while my early intuition was that these should largely be the same, I don't think we can assume that.

Again: this probably shouldn't hold up this tactical change for homogeneous clusters, but is something that should be considered for heterogeneous multi-arch support.

@mattmoor right, but we do fetch the image here to get the entrypoint from the config. So if the entrypoint is correctly set in the image to /kaniko/executor_{arch}, it shouldn't be a problem, should it ? 🤔

it shouldn't be a problem, should it ?

Only if the arch where the pod will be scheduled matches the arch of the node where this code is running to look up the entrypoint. If you have a controller on amd64 and schedule a task on arm, you could run into issues (assuming I understand correctly what this code is doing).

Yeah, that's exactly the scenario I'm talking about. This is better than what's there today, so we should clean it up and merge it, but it doesn't close the book on multi-arch support :D

If you have a controller on amd64 and schedule a task on arm, you could run into issues (assuming I understand correctly what this code is doing).

Ah, indeed, I didn't thought of that one 😓

I think it's worth calling out this behavior in a comment in the code. Something like:

// By default go-containerregistry pulls amd64 images. // Setting correct image pull architecture based on the underlying platform // _of the node that Tekton's controller is running on_. If the cluster // is comprised of nodes of heterogeneous architectures, this might cause issues.

Make a suggested edit with this above, since I was adding a nit for spacing anyhow.

mattmoor · 2020-10-06T14:00:11Z

/cc @imjasonh

imjasonh · 2020-10-06T14:09:38Z

/kind feature

mattmoor · 2020-10-06T15:00:01Z

pkg/pod/entrypoint_lookup_impl.go

+	//Setting correct image pull architecture based on the underlying platform
+	var pf = v1.Platform {
+        		Architecture: runtime.GOARCH ,
+	        	OS :          "linux",


Perhaps for good measure this should also be runtime.GOOS?

Good suggestion. Made the required changes.

mattmoor · 2020-10-06T20:20:49Z

FWIW with this and some other changes I was able to perform a kaniko build on my RPi4 🤩

It was crazy slow, but I will try graviton next 😉

imjasonh · 2020-10-07T13:00:01Z

@bahetiamit thanks for making this change! You'll need to sign the LF CLA to get this merged.

bahetiamit · 2020-10-07T13:16:13Z

@bahetiamit thanks for making this change! You'll need to sign the LF CLA to get this merged.

@imjasonh My bad. I am working towards it. As this needs to be corporate CLA, it is taking some time.

snehlatamohite · 2020-10-07T13:55:03Z

We have CLA approvals just need to confirm manager at our end, I have started confirmation on this.

mattmoor · 2020-10-07T14:31:48Z

pkg/pod/entrypoint_lookup_impl.go

 	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/google/go-containerregistry/pkg/authn/k8schain"
 	"github.com/google/go-containerregistry/pkg/name"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 	lru "github.com/hashicorp/golang-lru"
 	"k8s.io/client-go/kubernetes"
+	"runtime"


technically goimports wants this up under fmt with a line of whitespace before the github.com imports.

That's because of gofmt - golang/go#37719. Do you want me to change ordering? For this repo, we use goimports tool?

gofmt moved it down because you removed the blank line. May as well stick it back up at the top and add the newline while we are waiting on CLA 😅

Sort of annoying that different go tools futz with the same parts, but in inconsistent ways... thanks 🙏

pkg/pod/entrypoint_lookup_impl.go

…rlying system architecture

mattmoor · 2020-10-08T15:10:50Z

This LGTM. Any update on the CLA @bahetiamit ?

bahetiamit · 2020-10-08T16:31:06Z

This LGTM. Any update on the CLA @bahetiamit ?

@mattmoor Request to CLA manager is sent. May be in next few hours I will be in whitelist & we should be good to go.

bahetiamit · 2020-10-09T02:28:48Z

@mattmoor EasyCLA is now in place.

mattmoor

/ok-to-test
/lgtm

mattmoor · 2020-10-09T02:50:24Z

/assign @imjasonh @vdemeester

imjasonh

/lgtm

vdemeester

Added a release-note part 😉
/meow

tekton-robot · 2020-10-09T12:09:52Z

@vdemeester:

In response to this:

Added a release-note part 😉
/meow

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.

tekton-robot · 2020-10-09T12:09:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, vdemeester

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

- bump alpine-git version to the latest one to be able to use multi-arch image - remove some s390x e2e and examples tests from excluded list, the bug was fixed by tektoncd#3337 - add kubectl image replacement for s390x Signed-off-by: Yulia Gaponenko <[email protected]>

- bump alpine-git version to the latest one to be able to use multi-arch image - remove some s390x e2e and examples tests from excluded list, the bug was fixed by #3337 - add kubectl image replacement for s390x Signed-off-by: Yulia Gaponenko <[email protected]>

tekton-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Oct 6, 2020

tekton-robot requested review from dibyom and vdemeester October 6, 2020 09:57

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 6, 2020

vdemeester reviewed Oct 6, 2020

View reviewed changes

tekton-robot requested review from jonjohnsonjr and mattmoor October 6, 2020 10:49

mattmoor reviewed Oct 6, 2020

View reviewed changes

tekton-robot requested a review from imjasonh October 6, 2020 14:00

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 6, 2020

mattmoor reviewed Oct 6, 2020

View reviewed changes

bahetiamit force-pushed the multiarch_image branch from c38aaff to 5ec8000 Compare October 7, 2020 06:05

mattmoor reviewed Oct 7, 2020

View reviewed changes

pkg/pod/entrypoint_lookup_impl.go Outdated Show resolved Hide resolved

Fixes tektoncd#3086 - amd64 images always pulled irrespective of unde…

e987483

…rlying system architecture

bahetiamit force-pushed the multiarch_image branch from 8a94134 to e987483 Compare October 8, 2020 15:03

mattmoor approved these changes Oct 9, 2020

View reviewed changes

tekton-robot assigned mattmoor Oct 9, 2020

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm Indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 9, 2020

tekton-robot assigned imjasonh and vdemeester Oct 9, 2020

imjasonh reviewed Oct 9, 2020

View reviewed changes

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 9, 2020

vdemeester approved these changes Oct 9, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 9, 2020

tekton-robot merged commit e2f528b into tektoncd:master Oct 9, 2020

barthy1 mentioned this pull request Oct 12, 2020

Refresh list of s390x excluded tests #3372

Merged

4 tasks

barthy1 mentioned this pull request Oct 19, 2020

s390x related fixes for tekton pipeline #3064

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #3086 - amd64 images are pulled on all the architectures #3337

Fixes #3086 - amd64 images are pulled on all the architectures #3337

bahetiamit commented Oct 6, 2020 •

edited by vdemeester

Loading

linux-foundation-easycla bot commented Oct 6, 2020 •

edited

Loading

tekton-robot commented Oct 6, 2020

vdemeester left a comment

mattmoor commented Oct 6, 2020

mattmoor Oct 6, 2020

bahetiamit Oct 7, 2020

mattmoor Oct 6, 2020

mattmoor Oct 6, 2020

vdemeester Oct 6, 2020

jonjohnsonjr Oct 6, 2020

mattmoor Oct 6, 2020

vdemeester Oct 7, 2020 •

edited

Loading

imjasonh Oct 7, 2020

mattmoor Oct 7, 2020

mattmoor commented Oct 6, 2020

imjasonh commented Oct 6, 2020

mattmoor Oct 6, 2020

bahetiamit Oct 7, 2020

mattmoor commented Oct 6, 2020

imjasonh commented Oct 7, 2020

bahetiamit commented Oct 7, 2020

snehlatamohite commented Oct 7, 2020

mattmoor Oct 7, 2020

bahetiamit Oct 7, 2020

mattmoor Oct 7, 2020

mattmoor commented Oct 8, 2020

bahetiamit commented Oct 8, 2020

bahetiamit commented Oct 9, 2020

mattmoor left a comment

mattmoor commented Oct 9, 2020

imjasonh left a comment

vdemeester left a comment

tekton-robot commented Oct 9, 2020

tekton-robot commented Oct 9, 2020

Fixes #3086 - amd64 images are pulled on all the architectures #3337

Fixes #3086 - amd64 images are pulled on all the architectures #3337

Conversation

bahetiamit commented Oct 6, 2020 • edited by vdemeester Loading

Changes

Submitter Checklist

linux-foundation-easycla bot commented Oct 6, 2020 • edited Loading

tekton-robot commented Oct 6, 2020

vdemeester left a comment

Choose a reason for hiding this comment

mattmoor commented Oct 6, 2020

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

vdemeester Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor commented Oct 6, 2020

imjasonh commented Oct 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor commented Oct 6, 2020

imjasonh commented Oct 7, 2020

bahetiamit commented Oct 7, 2020

snehlatamohite commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor commented Oct 8, 2020

bahetiamit commented Oct 8, 2020

bahetiamit commented Oct 9, 2020

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor commented Oct 9, 2020

imjasonh left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 9, 2020

tekton-robot commented Oct 9, 2020

bahetiamit commented Oct 6, 2020 •

edited by vdemeester

Loading

linux-foundation-easycla bot commented Oct 6, 2020 •

edited

Loading

vdemeester Oct 7, 2020 •

edited

Loading