Support architecture targeted builds by jahkeup · Pull Request #837 · aws/amazon-vpc-cni-k8s

jahkeup · 2020-02-11T00:31:17Z

Issue #, if available:

Related: #687

Description of changes:

These changes clean up the Makefile in order to widely update references for building cross-arch artifacts and images in some configurations. I've added many docstrings to the updated sections as well as the left in place sections, hopefully this a welcome addition - please let me know if I've misunderstood or misinterpreted usages in any way!

With these changes in place I have been able to build container images for both aarch64 and x86_64 (on the same host using QEMU as binfmt handlers!) as well as the bare executables by providing the GOARCH styled architecture names as ARCH:

# Build container images for aarch64 (either *on* aarch64 or using `binfmt` method)
make all ARCH=arm64
# and the executables:
make build-linux ARCH=arm64

# Build container images for x86_64 explicitly
make all ARCH=amd64
# and the executables:
make build-linux ARCH=amd64

The updated targets also take into account the currently running host and enforce the use of the native build where required (such as testing with -race enabled).

I've updated the Circle CI builds to utilize these code paths that build for aarch64 and x86_64, though I have not yet tested this and would like help to validate this proposed change.

cc: @mogren

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

mogren

@jahkeup Nice! Thanks a lot for fixing this. Just some minor nitpicks and comments...

Makefile

scripts/dockerfiles/Dockerfile.release

mogren

@jahkeup Thanks a lot, this all looks a lot better!

Makefile

jaypipes

Hi @jahkeup! Thanks very much for this contribution!

Just FYI for future PRs, I would prefer that code changes be tightly scoped to a specific feature described in the PR/commit summary. This change introduces a wholesale set of feature changes, style changes, cleanups and more, which usually makes reviewing PRs like this more difficult (because code reviewers need to examine each line in the PR to determine whether that line correlates to something the PR claims to add or whether the line is merely a style-only change).

I don't mind style-only changes, of course, and most of the style-only changes you've included in here are awesome. It's just for future PRs, please try to keep those in a separate PR entirely so reviewers can more easily group code changes in their head :)

Anyway, the only thing that I'd like you to functionally change in this PR is the handling of the golint stuff. Please see inline comment for an explanation.

Thanks!
-jay

jaypipes · 2020-02-21T14:10:14Z

Makefile

+		build-linux docker \
+		unit-test unit-test-race build-docker-test docker-func-test \
+		build-metrics docker-metrics \
+		metrics-unit-test docker-metrics-test


missing portmap above.

portmap isn't phony, its a file that's created by downloading it - if portmap were to be listed here it will be downloaded every time the target is activated.

If the goal is to be sure that we're getting the Makefile's specific binary, then Makefile can be added as dependency for portmap which would cause portmap to be considered stale and then redownloaded. I don't see usages of portmap outside of the container build at the moment (which doesn't get stale where Make can help) so I'd recommend not doing that at this time.

portmap isn't phony, its a file that's created by downloading it - if portmap were to be listed here it will be downloaded every time the target is activated.

ack, yes, good point.

Makefile

jaypipes · 2020-02-21T14:19:30Z

Makefile

+	docker save $(METRICS_IMAGE_NAME) | gzip > $(METRICS_IMAGE_DIST)

-# Default to build the Linux binary
+# Build the VPC CNI plugin agent using host Go toolchain.


s/using host Go toolchain/using the Linux Go toolchain/

..since GOOS is always linux, right?

..since GOOS is always linux, right?

I'm not sure if you're implying something here, the Makefile does set GOOS=linux for builds.

Not that this is what's at hand, but I personally think that -linux suffix is unnecessary. Any additional GOOS/GOARCH combinations can be specified to influence the build when make is invoked. To me, then, this target should be named build (another OS/Arch combination can be built using eg: make build GOOS=windows[1]). I chose to leave build-linux in place as its a target that's very likely to be referenced in places I'm not aware of. With the appropriate variables set in the Makefile, ie: GOOS, the effect/output of build-linux remains the same as it was before.

s/using host Go toolchain/using the Linux Go toolchain/

I'd argue that it is still accurate to say using the host Go toolchain as it'll be up to the host's Go toolchain to build for GOOS=linux. It's certainly feasible that builds are possible on native Mac OS X for instance using a multi-arch equipped distribution of Go. The syscall provided by the stdlib should allow for the binary to build on developer's native machines (with the caveat that their Go toolchains have the support).

[1]: definitely would not build because of syscall dependencies deeply rooted in Linux land.

jaypipes · 2020-02-21T14:24:56Z

Makefile

+	go test -v -mod=readonly -cover -race -timeout 10s  ./pkg/networkutils/...
+	go test -v -mod=readonly -cover -race -timeout 10s  ./pkg/utils/...
+	go test -v -mod=readonly -cover -race -timeout 10s  ./pkg/eniconfig/...
+	go test -v -mod=readonly -cover -race -timeout 10s  ./pkg/ipamd/...


Might clean this up in a future commit with:

unit-test-race: COVER_ARGS = -v -mod=readonly -cover -race go test $(COVER_ARGS) -timeout 10s ./cmd/*/... ...

Is this an ask? Or a note for another time? Happy to make the change now if that's welcome.

it's a note for a future PR :) no need to make the change here.

jaypipes · 2020-02-21T14:33:20Z

Makefile

+	  -type d \
+	  -not -path ./rpc -not -path ./pkg -not -path ./cmd -not -path './.*' \
+	  -not -name mocks -not -name mock \
+	  -not -name 'mock_*' -not -name 'mocks_*' \


the above includes the docs/, testdata/, misc/, /config and /scripts directories which should be excluded from Go linting.

Probably better to just search for Go files and exclude mocks:

find . -name *.go -not -name *mock*

but be aware that there are linter failures in the tree's current code, so we'll need to clean these up before adding lint to the check target that gates CI:

jaypipes@udb42383a8e5155:~/go/src/github.com/aws/amazon-vpc-cni-k8s$ for f in `find . -name *.go -not -name *mock*`; do GO111MODULE=on golint $f; done ./pkg/awsutils/awsutils.go:194:1: exported method ENIMetadata.PrimaryIPv4Address should have comment or be unexported ./pkg/ec2metadatawrapper/ec2metadatawrapper.go:17:6: type HttpClient should be HTTPClient ./pkg/ipwrapper/ip.go:23:6: exported type IP should have comment or be unexported ./pkg/ipwrapper/ip.go:30:1: exported function NewIP should have comment or be unexported ./pkg/eniconfig/eniconfig.go:50:6: exported type ENIConfig should have comment or be unexported ./pkg/eniconfig/eniconfig.go:55:5: exported var ErrNoENIConfig should have comment or be unexported ./pkg/eniconfig/eniconfig.go:58:6: type name will be used as eniconfig.ENIConfigController by other packages, and that stutters; consider calling this Controller ./pkg/eniconfig/eniconfig.go:68:6: type name will be used as eniconfig.ENIConfigInfo by other packages, and that stutters; consider calling this Info ./pkg/eniconfig/eniconfig.go:162:1: exported method ENIConfigController.Getter should have comment or be unexported ./pkg/utils/ttime/ttime.go:14:6: exported type Timer should have comment or be unexported ./pkg/utils/retry/retry.go:27:6: func name will be used as retry.RetryWithBackoff by other packages, and that stutters; consider calling this WithBackoff ./pkg/utils/retry/retry.go:35:6: func name will be used as retry.RetryWithBackoffCtx by other packages, and that stutters; consider calling this WithBackoffCtx ./pkg/utils/retry/retry.go:57:6: func name will be used as retry.RetryNWithBackoff by other packages, and that stutters; consider calling this NWithBackoff ./pkg/utils/retry/retry.go:66:6: func name will be used as retry.RetryNWithBackoffCtx by other packages, and that stutters; consider calling this NWithBackoffCtx ./pkg/utils/retry/backoff.go:23:6: exported type Backoff should have comment or be unexported ./pkg/utils/retry/backoff.go:28:6: exported type SimpleBackoff should have comment or be unexported ./pkg/utils/retry/backoff.go:51:1: exported method SimpleBackoff.Duration should have comment or be unexported ./pkg/utils/retry/backoff.go:59:1: exported method SimpleBackoff.Reset should have comment or be unexported ./pkg/utils/retry/errors.go:21:6: exported type Retriable should have comment or be unexported ./pkg/utils/retry/errors.go:25:6: exported type DefaultRetriable should have comment or be unexported ./pkg/utils/retry/errors.go:29:1: exported method DefaultRetriable.Retry should have comment or be unexported ./pkg/utils/retry/errors.go:33:1: exported function NewRetriable should have comment or be unexported ./pkg/utils/retry/errors.go:39:6: exported type RetriableError should have comment or be unexported ./pkg/utils/retry/errors.go:44:6: exported type DefaultRetriableError should have comment or be unexported ./pkg/utils/retry/errors.go:49:1: exported function NewRetriableError should have comment or be unexported ./pkg/utils/retry/errors.go:56:6: exported type AttributeError should have comment or be unexported ./pkg/typeswrapper/client.go:20:6: exported type CNITYPES should have comment or be unexported ./pkg/typeswrapper/client.go:27:1: exported function New should have comment or be unexported ./pkg/apis/crd/v1alpha1/register.go:17:2: exported var SchemeBuilder should have comment or be unexported ./pkg/apis/crd/v1alpha1/types.go:9:6: exported type ENIConfigList should have comment or be unexported ./pkg/apis/crd/v1alpha1/types.go:17:6: exported type ENIConfig should have comment or be unexported ./pkg/apis/crd/v1alpha1/types.go:24:6: exported type ENIConfigSpec should have comment or be unexported ./pkg/apis/crd/v1alpha1/types.go:28:6: exported type ENIConfigStatus should have comment or be unexported ./pkg/rpcwrapper/client.go:21:6: exported type RPC should have comment or be unexported ./pkg/rpcwrapper/client.go:27:1: exported function New should have comment or be unexported ./pkg/cri/cri.go:27:6: exported type APIs should have comment or be unexported ./pkg/cri/cri.go:31:6: exported type Client should have comment or be unexported ./pkg/cri/cri.go:33:1: exported function New should have comment or be unexported ./pkg/cri/cri.go:37:1: exported method Client.GetRunningPodSandboxes should have comment or be unexported ./pkg/ipamd/ipamd.go:188:1: comment on exported type ReconcileCooldownCache should be of the form "ReconcileCooldownCache ..." (with optional leading article) ./pkg/ipamd/ipamd_test.go:275:2: var warmIpTarget should be warmIPTarget ./pkg/grpcwrapper/client.go:20:6: exported type GRPC should have comment or be unexported ./pkg/grpcwrapper/client.go:26:1: exported function New should have comment or be unexported ./pkg/k8sapi/discovery.go:120:1: comment on exported method Controller.DiscoverCNIK8SPods should be of the form "DiscoverCNIK8SPods ..." ./pkg/ec2wrapper/ec2wrapper.go:1:1: package comment should be of the form "Package ec2wrapper ..." ./pkg/ec2wrapper/ec2wrapper.go:30:1: comment on exported function NewMetricsClient should be of the form "NewMetricsClient ..." ./pkg/ec2wrapper/client.go:23:6: exported type EC2 should have comment or be unexported ./pkg/ec2wrapper/client.go:37:1: exported function New should have comment or be unexported ./pkg/nswrapper/ns.go:20:6: exported type NS should have comment or be unexported ./pkg/nswrapper/ns.go:27:1: exported function NewNS should have comment or be unexported ./cmd/routed-eni-cni-plugin/cni.go:82:2: don't use ALL_CAPS in Go names; use CamelCase ./cmd/routed-eni-cni-plugin/cni.go:85:2: don't use ALL_CAPS in Go names; use CamelCase ./cmd/routed-eni-cni-plugin/cni.go:88:2: don't use ALL_CAPS in Go names; use CamelCase ./cmd/routed-eni-cni-plugin/cni.go:288:10: if block ends with a return statement, so drop this else and outdent its block ./cmd/routed-eni-cni-plugin/cni.go:301:2: var deletedPodIp should be deletedPodIP

but be aware that there are linter failures in the tree's current code, so we'll need to clean these up before adding lint to the check target that gates CI:

Yeah, this is why its configured as a soft-fail/no-fail in CI. The lint failures will print without causing the step to exit non-zero. Locally, this target will report lint failures with a non-zero exit code by default (unless disabled as it is in the CI script).

jaypipes@udb42383a8e5155:~/go/src/github.com/aws/amazon-vpc-cni-k8s$ for f in find . -name *.go -not -name mock; do GO111MODULE=on golint $f; done

Ah, so we'll have to do it a file at a time then. The reason for the interesting find is that invoking golint on a set of files that cross package boundaries is not supported and so this find is/was to avoid invoking golint for each file. I'll rework this with the approach you've suggested.

jaypipes · 2020-02-21T14:50:07Z

scripts/dockerfiles/Dockerfile.metrics

+FROM $docker_arch/amazonlinux:2
 RUN yum update -y && \
-    yum clean all
+	yum clean all


do you need to put the below in this Dockerfile? (you added it to the release Dockerfile below)

ENV GOARCH=$arch

Good catch, yes, this'll need to be plumbed in (in the builder stage). Will update.

A side note:

With a static build, I think that the CNI metrics helper's container image can be shrunk down into a final image of:

FROM scratch # Copy our bundled certs to the first place go will check: see # https://golang.org/src/pkg/crypto/x509/root_unix.go COPY ./misc/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt COPY --from=builder /go/src/github.com/aws/amazon-vpc-cni-k8s/cni-metrics-helper /app

That's a change for another time though.

This reworks much of the Makefile and, to a lesser extent, the Dockerfiles for building the CNI plugins for other platforms. Additional clean up work is included as well as this was combed through to support these builds. Including: - ARCH resolver to identify the ARCH (GOARCH styled) to pass around - Updated ARCH and GOARCH references throughout to support against other ARCHes - Make base container build appropriately for ARCH - Docker explicit multiarch references in targets and Dockerfiles - Updated related container images to allow testing where possible - Eliminate ./tmp usage with tar unpacking from a pipe - Clean up and consistent call sites all around - Reconciled image naming - Updated stale golang images references - Added docstrings for reference

Though the tests don't require CGO with cross-compilation support, the tests do still require support to exec the golang test binaries.

jahkeup · 2020-02-22T00:31:25Z

Just FYI for future PRs, I would prefer that code changes be tightly scoped to a specific feature described in the PR/commit summary. This change introduces a wholesale set of feature changes, style changes, cleanups and more, which usually makes reviewing PRs like this more difficult (because code reviewers need to examine each line in the PR to determine whether that line correlates to something the PR claims to add or whether the line is merely a style-only change).

My apologies for the scope this grew to - most of these changes I had discussed with @mogren offline about and also sent along early versions of the patches before opening this PR. My understanding was that the set of changes was welcome and welcome to be bundled together - with the tangentially related changes included (which admittedly, yes, is wide reaching).

I don't mind style-only changes, of course, and most of the style-only changes you've included in here are awesome. It's just for future PRs, please try to keep those in a separate PR entirely so reviewers can more easily group code changes in their head :)

I'll keep this in mind for the next time when scoping the work out in my mind and in my posted changes 👍

jaypipes

Great stuff @jahkeup :) Thanks very much!

-jay

jaypipes · 2020-02-25T14:27:10Z

Makefile

+		build-linux docker \
+		unit-test unit-test-race build-docker-test docker-func-test \
+		build-metrics docker-metrics \
+		metrics-unit-test docker-metrics-test


portmap isn't phony, its a file that's created by downloading it - if portmap were to be listed here it will be downloaded every time the target is activated.

ack, yes, good point.

jaypipes · 2020-02-25T14:27:45Z

Makefile

+	go test -v -mod=readonly -cover -race -timeout 10s  ./pkg/networkutils/...
+	go test -v -mod=readonly -cover -race -timeout 10s  ./pkg/utils/...
+	go test -v -mod=readonly -cover -race -timeout 10s  ./pkg/eniconfig/...
+	go test -v -mod=readonly -cover -race -timeout 10s  ./pkg/ipamd/...


it's a note for a future PR :) no need to make the change here.

mogren · 2020-03-05T19:55:28Z

@jahkeup Hey, sorry this never got merged earlier. It seems like #851 introduced some incompatible changes since it no longer builds. Do you have bandwidth to resolve the rebase issue?

jaypipes · 2020-03-11T12:43:42Z

Makefile

+# Run unit tests
 unit-test:
-	GOOS=linux CGO_ENABLED=1 go test -v -cover $(ALLPKGS)
+	go test -mod=readonly -v -cover $(ALLPKGS)


We will need to run go mod tidy before using -mod=readonly...

https://www.reddit.com/r/golang/comments/c2ckjz/why_does_my_gosum_file_keep_changing/

jahkeup · 2020-03-11T13:34:21Z

@mogren sorry I don't currently have the bandwidth to circle back this week. I'll take a look at what's going on if I end up with time on my hands or early next week!

jaypipes · 2020-03-11T14:48:41Z

@mogren sorry I don't currently have the bandwidth to circle back this week. I'll take a look at what's going on if I end up with time on my hands or early next week!

Not a problem @jahkeup! Either Claes or myself can push a commit that addresses the issue.

mogren

@jaypipes Thanks for fixing. We need to update the merge checks after this gets merged, since the name of the build task has changed.

ci/circleci: build Expected — Waiting for status to be reported

We need to check for build_x86_64 and build_aarch64 instead.

jaypipes · 2020-03-13T15:18:53Z

.circleci/config.yml


-jobs:
-  build:
+references:


I actually realized what was wrong with this this morning, when I noticed CircleCI was waiting for a "build" job to report a successful status even though there was no build job defined any more.

Then I remembered CircleCI requires a job named "build" when you do not use workflows. I'm wondering if there's something messed up by not having a job or a workflow called "build".

If we can't change that name in CircleCI, we could just rename the build_x86_64 to build again, since that would be the default.

If we can't change that name in CircleCI, we could just rename the build_x86_64 to build again, since that would be the default.

I'm going to see if this issue just goes away. I think maybe (?) the old config YAML was cached somehow and that was the reason for the empty "build" step status check being expected...

This reworks much of the Makefile and, to a lesser extent, the Dockerfiles for building the CNI plugins for other platforms. Additional clean up work is included as well as this was combed through to support these builds. Including: - ARCH resolver to identify the ARCH (GOARCH styled) to pass around - Updated ARCH and GOARCH references throughout to support against other ARCHes - Make base container build appropriately for ARCH - Docker explicit multiarch references in targets and Dockerfiles - Updated related container images to allow testing where possible - Eliminate ./tmp usage with tar unpacking from a pipe - Clean up and consistent call sites all around - Reconciled image naming - Updated stale golang images references - Added docstrings for reference

gitignore: update to latest output name

a88575a

jahkeup requested a review from mogren February 11, 2020 00:31

jahkeup force-pushed the arches branch from 0d13f6e to ebf5b38 Compare February 11, 2020 00:51

mogren added the enhancement label Feb 11, 2020

mogren suggested changes Feb 11, 2020

View reviewed changes

mogren reviewed Feb 11, 2020

View reviewed changes

scripts/dockerfiles/Dockerfile.release Outdated Show resolved Hide resolved

jahkeup force-pushed the arches branch 2 times, most recently from ec6f884 to ce2b3a8 Compare February 12, 2020 01:18

jahkeup requested a review from mogren February 12, 2020 16:50

mogren approved these changes Feb 12, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

jahkeup force-pushed the arches branch from ce2b3a8 to cf08aa8 Compare February 12, 2020 20:26

mogren requested a review from jaypipes February 21, 2020 13:09

jaypipes self-assigned this Feb 21, 2020

jaypipes suggested changes Feb 21, 2020

View reviewed changes

jahkeup and others added 4 commits February 21, 2020 15:54

build: break out circleci jobs for arches

513af33

build: force Circle CI to use amd64 for unit tests

a250d67

Though the tests don't require CGO with cross-compilation support, the tests do still require support to exec the golang test binaries.

build: make CI builds treat lint as non-failing

ae4f6a6

build: wire GOARCH through metrics helper image

f96e485

jahkeup force-pushed the arches branch from cf08aa8 to f96e485 Compare February 22, 2020 01:04

Merge branch 'master' into arches

fab138d

jaypipes approved these changes Feb 25, 2020

View reviewed changes

jaypipes mentioned this pull request Feb 25, 2020

Run conformance test as part of PR/Release certification #851

Merged

jaypipes and others added 2 commits February 28, 2020 08:58

Merge branch 'master' into arches

3246cc3

Merge branch 'master' into arches

7dadfe8

Merge branch 'master' into arches

517242a

jaypipes suggested changes Mar 11, 2020

View reviewed changes

jaypipes added 3 commits March 12, 2020 15:49

Merge branch 'master' into arches

416ba31

Remove -mod=readonly until figure out go mod issues

87f40f7

Merge branch 'master' into arches

c921939

jaypipes approved these changes Mar 12, 2020

View reviewed changes

mogren approved these changes Mar 13, 2020

View reviewed changes

jaypipes reviewed Mar 13, 2020

View reviewed changes

jaypipes merged commit a4dc9f5 into aws:master Mar 13, 2020

tjkirch mentioned this pull request Mar 13, 2020

Verify ARM64 support bottlerocket-os/bottlerocket#468

Closed

6 tasks

mogren mentioned this pull request Apr 17, 2020

Ensure makefile exits with non-zero code when code is unformatted #912

Merged

Conversation

jahkeup commented Feb 11, 2020

Uh oh!

mogren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mogren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaypipes left a comment

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!

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!

jaypipes Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jahkeup commented Feb 22, 2020

Uh oh!

jaypipes left a comment

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!

mogren commented Mar 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jahkeup commented Mar 11, 2020

Uh oh!

jaypipes commented Mar 11, 2020

Uh oh!

mogren left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jaypipes Feb 21, 2020 •

edited

Loading