Multi ARCH build amd64, arm64 #698

zvonkok · 2021-12-10T12:19:17Z

Sample buildx Makefile for building for various platfroms

ArangoGutierrez · 2021-12-10T13:40:41Z

Thanks for this PR @zvonkok , I started to play around buildx a time ago see here : #582

happy to close that PR and merge efforts here

zvonkok · 2021-12-10T14:02:14Z

@marquiz PTAL, should we try to keep the old structure with build and push command? Should we consider how podman fits into this picture (@ArangoGutierrez)? I think as a start we can keep amd64 and arm64 and then extend if needed.

ArangoGutierrez · 2021-12-10T14:28:26Z

/assign marquiz ArangoGutierrez

zvonkok · 2021-12-10T15:19:55Z

Just checked the building would work with podman as eays as:

IMAGE_BUILD_CMD ?= podman build --platform=${PLATFORMS} $(OUTPUT) --progress=$(PROGRESS) --pull

Podman does a sequential build of each platform supplied:

➜  node-feature-discovery git:(multi-arch) ✗ make image
namespace: node-feature-discovery
image: k8s.gcr.io/nfd/node-feature-discovery:v0.2.0-914-gcde21fe-dirty
./hack/init-buildx.sh
podman build --platform=linux/amd64,linux/arm64  --progress=auto --pull --build-arg VERSION=v0.2.0-914-gcde21fe-dirty \
    --target full \
    --build-arg HOSTMOUNT_PREFIX=/host- \
    --build-arg BASE_IMAGE_FULL=debian:buster-slim \
    --build-arg BASE_IMAGE_MINIMAL=gcr.io/distroless/base \
    -t k8s.gcr.io/nfd/node-feature-discovery:v0.2.0-914-gcde21fe-dirty \
     \
     ./
[linux/arm64] [1/2] STEP 1/10: FROM golang:1.17.2-buster AS builder
Resolved "golang" as an alias (/home/zvonkok/.cache/containers/short-name-aliases.conf)
Trying to pull docker.io/library/golang:1.17.2-buster...
Getting image source signatures
Copying blob e7c82db586c3 done  
Copying blob 2ff6d7a9e7d7 done  
Copying blob 267d85d2d4f5 done  
Copying blob b7324ea40984 done  
Copying blob 4e213c33a073 done  
Copying blob 16d1041ad0e9 done  
Copying blob cefbd31cd5ba done  
Copying config f341e2ad98 done  
Writing manifest to image destination
Storing signatures
[linux/arm64] [1/2] STEP 2/10: RUN GRPC_HEALTH_PROBE_VERSION=v0.3.1 &&     wget -qO/bin/grpc_health_probe https://github.com/grpc-ecosystem/grpc-health-probe/releases/download/${GRPC_HEALTH_PROBE_VERSION}/grpc_health_probe-linux-amd64 &&     chmod +x /bin/grpc_health_probe
--> d6d6e21226e
[linux/arm64] [1/2] STEP 3/10: COPY go.mod go.sum /go/node-feature-discovery/
--> c62b32dc0d7
[linux/arm64] [1/2] STEP 4/10: WORKDIR /go/node-feature-discovery
--> 30de786f60f

marquiz · 2021-12-11T08:37:15Z

@marquiz PTAL, should we try to keep the old structure with build and push command? Should we consider how podman fits into this picture (@ArangoGutierrez)? I think as a start we can keep amd64 and arm64 and then extend if needed.

Thanks for working on this @zvonkok! Good questions above. I think it would probably be handy to have separate build targets for building only one image (defaulting to the current os/arch) and all archs, i.e. make imageand make image-all or smth (with possible pattern rule for image-$(os)/$(arch): targets). This would be handy for development. Also, would there be a nice way to avoid buildx for native builds?

My test build fails, probably because of some http(s) proxy issue. Need to investigate/fix that:

 => ERROR [linux/arm64 internal] load metadata for docker.io/library/debian:buster-slim                                                                       30.0s
 => CANCELED [linux/arm64 internal] load metadata for docker.io/library/golang:1.17.2-buster                                                                  30.0s
 => CANCELED [linux/amd64 internal] load metadata for docker.io/library/golang:1.17.2-buster                                                                  29.9s
 => CANCELED [linux/amd64 internal] load metadata for docker.io/library/debian:buster-slim                                                                    29.9s
------
 > [linux/arm64 internal] load metadata for docker.io/library/debian:buster-slim:
------
Dockerfile:30
--------------------
  28 |     
  29 |     # Create full variant of the production image
  30 | >>> FROM ${BASE_IMAGE_FULL} as full
  31 |     
  32 |     # Run as unprivileged user
--------------------
error: failed to solve: debian:buster-slim: failed to do request: Head "https://registry-1.docker.io/v2/library/debian/manifests/buster-slim": dial tcp 34.238.187.50:443: i/o timeout
make: *** [Makefile:91: image] Error 1

zvonkok · 2021-12-13T10:08:29Z

@marquiz What about keeping the old Makefile intact. This enables native builds if you're e.g running on arm64 you do just a make image etc. and those commands would stay the same.

Additionally to that we can create as you suggested some new targets for cross builds with buildx. I will push another version soon.

zvonkok · 2021-12-13T10:51:12Z

This keeps the original Makefile almost completely intact, we're adding an include for the multi arch Makefile.

# Build all preconfigured OS/ARCH combinations (linux + amd64, arm64) 
make image-all
# Push all
make push-all


# Build a specific OS/ARCH combination 
OS=linux ARCH=arm64 make image-${OS}/${ARCH}
# Push linux/arm64
OS=linux ARCH=arm64 make push-${OS}/${ARCH}

zvonkok · 2021-12-13T10:56:09Z

Actually we do not need the extra targets. We could simply do:

PLATFORMS=linux/amd64 make image-all
# or only arm64
PLATFORMS=linux/arm64 make image-all
# or arm64,amd64,s390x
PLATFORMS=linux/arm64,linux/amd64,linux/s390x make image-all

zvonkok · 2021-12-13T10:56:32Z

@marquiz Which one do you prefer?

zvonkok · 2021-12-13T10:56:45Z

@ArangoGutierrez Any comments from your side?

ArangoGutierrez · 2021-12-13T16:51:46Z

Actually we do not need the extra targets. We could simply do:

PLATFORMS=linux/amd64 make image-all
# or only arm64
PLATFORMS=linux/arm64 make image-all
# or arm64,amd64,s390x
PLATFORMS=linux/arm64,linux/amd64,linux/s390x make image-all

I would only set the Arch, unless we are planning on adding support for non-linux NFD deployments (we don't wanna do there right?)

For the rest I like the idea of the ENV VAR

ARCH=arm64 make image-${ARCH}

zvonkok · 2021-12-14T09:28:54Z

@ArangoGutierrez I would leave it as

PLATFORMS=linux/arm64,linux/amd64,linux/s390x make image-all

The PLATFORM is the cmdline for docker and podman no changes needed for --platform=$(PLATFORM)
If we only go for arch e.g. ARCH=amd64,arm64 make image-${ARCH} we would need any combination of archs as targets.
I do not want to implement some parsing list handling in the Makefile.

@marquiz WDYT?

marquiz · 2021-12-14T11:11:26Z

Hmm, I now took a closer look at this. I don't mind refactoring the existing Makefile. It's not perfect by any means 😇

Looking at this I think that it might be better to have separate make image-cross (or make image-multiarch or smth.) target instead of exploiting the existing make image target. Just to keep it simple and easier to understand (and maintain). We could have a separate hack/build.sh script or capture the common builder args into IMAGE_BUILD_ARGS variable or similar.

Also, I'm not sure how/if the image promotion in https://github.com/kubernetes/k8s.io will work 🧐 Maybe we should build explicit node-feature-discovery-$(ARCH):$(tag) (and node-feature-discovery-$(ARCH):$(tag)-minimal) images instead? In practice I think, this would mean having pattern rules like image-cross-%, push-cross-%. Then we'd have something like

image-cross-all: $(foreach arch,$(ARCHS),image-cross-$(arch))

image-cross-%: yamls
        $(IMAGE_BUILD_CMD_CROSS) 
        ...

BTW: shouldn't the GRPC probes be updated? 🤔

A bit unrelated note: if multiarch push also builds the image, I'd be inclined to change the normal make push to behave similarly. This can addressed in a separate PR, though.

Any thoughts?

zvonkok · 2021-12-14T12:08:40Z

For mulitarch the image registries support manifest lists so essentially you have one tag that represents several architectures. Depending on the arch you're running on a

<WHATEVER> pull node-feature-discovery:vX.Y

will just work.

ArangoGutierrez · 2021-12-21T15:36:41Z

We also need a solution to address #563 when doing Multi-Arch builds, I propose not pulling a binary but pulling the source, and compiling during build.

fcami · 2021-12-21T15:38:17Z

The quick solution to always get the tip of the master branch like this:

Make sure you have git and go installed.
Run: go get github.com/grpc-ecosystem/grpc-health-probe
This will compile the binary into your $GOPATH/bin (or $HOME/go/bin).

As listed in the grpc-health-probe documentation.

fcami · 2021-12-21T15:39:59Z

The longer solution is to have a variable with the version we're interested in and pull from that git tag.

marquiz · 2021-12-21T16:23:47Z

We definitely want a specific version, for reproducibility, like we currently do. I guess the simplest thing to do is just do:

RUN go install github.com/grpc-ecosystem/grpc-health-probe@${GRPC_HEALTH_PROBE_VERSION}

in the dockerfile

marquiz · 2022-01-13T09:26:02Z

Issue with gprc probes has been addressed. Also, the image promotion of multiarch images (which I was worried about) is not problem.

ArangoGutierrez · 2022-01-18T14:38:54Z

hack/init-buildx.sh

@@ -0,0 +1,41 @@
+#!/usr/bin/env bash
+# Copyright 2020 The Kubernetes Authors.


Update year

ArangoGutierrez · 2022-01-18T14:40:03Z

After cutting v0.10 let's re test
/test all

marquiz · 2022-01-19T14:45:59Z

Makefile

@@ -32,7 +32,7 @@ JEKYLL_OPTS := -d '$(SITE_DESTDIR)' $(if $(SITE_BASEURL),-b '$(SITE_BASEURL)',)

 VERSION := $(shell git describe --tags --dirty --always)

-IMAGE_REGISTRY ?= k8s.gcr.io/nfd
+IMAGE_REGISTRY ?= quay.io/zvonkok


Zvonko secretly moving us to a new world

I'd suggest to change this back to k8s.gcr.io/nfd 😉

marquiz · 2022-01-19T14:50:18Z

Makefile

+image-all: OUTPUT=--pull
+image-all: IMAGE_BUILD_CMD=$(IMAGE_BUILDX_CMD)
+image-all: ensure-buildx yamls
+	$(IMAGE_BUILD_CMD) $(IMAGE_BUILD_ARGS) $(IMAGE_TAG_FULL)


We could possibly get rid of the OUTPUT variable. WDYT?

Suggested change

image-all: OUTPUT=--pull

image-all: IMAGE_BUILD_CMD=$(IMAGE_BUILDX_CMD)

image-all: ensure-buildx yamls

$(IMAGE_BUILD_CMD) $(IMAGE_BUILD_ARGS) $(IMAGE_TAG_FULL)

image-all: IMAGE_BUILD_CMD=$(IMAGE_BUILDX_CMD)

image-all: ensure-buildx yamls

$(IMAGE_BUILD_CMD) $(IMAGE_BUILD_ARGS) $(IMAGE_TAG_FULL) --pull

Makefile

marquiz · 2022-01-19T16:39:03Z

Makefile

@@ -32,7 +32,7 @@ JEKYLL_OPTS := -d '$(SITE_DESTDIR)' $(if $(SITE_BASEURL),-b '$(SITE_BASEURL)',)

 VERSION := $(shell git describe --tags --dirty --always)

-IMAGE_REGISTRY ?= k8s.gcr.io/nfd
+IMAGE_REGISTRY ?= quay.io/zvonkok


I'd suggest to change this back to k8s.gcr.io/nfd 😉

scripts/test-infra/push-image.sh

Makefile

zvonkok · 2022-01-19T20:09:40Z

Just confused two parameters, the --pull just says to pull the latest images that are used during the build. The --load parameter makes the images available to e.g. docker images, manifest-lists are not yet implemented for docker see docker/buildx#59 so we're omitting it during the build. The registry supports manifest-lists so we can push.

marquiz

Nice @zvonkok, I think we're almost there 😸

One thing is that it would be nice to get some mention about the multiarch builds in docs/advanced/developer-guide.md. And document the new variables like IMAGE_BUILDX_CMD there, too.

Makefile

marquiz

Thanks for the update! Few more small comments

And the docs...

Makefile

scripts/test-infra/push-image.sh

marquiz · 2022-01-20T12:50:17Z

docs/advanced/developer-guide.md

+
+

mdlint hates extra empty lines 😄

Suggested change

Yikes I should take a look at my warnings .... Sorry about that!

marquiz

One nit to make mdlint happy. Other than that, I think you could squash these into one commit and we could get it merged 👍

marquiz · 2022-01-20T14:07:19Z

/retest

marquiz

Could you squash a bit more, one commit should be enough (the second commit Added additional Makefile for mulit arch builds doesn't make much sense) 😉

zvonkok · 2022-01-20T14:42:13Z

@marquiz should be good now? :)

marquiz

Thanks @zvonkok for the effort! This has been on the wishlist of people for quite some time
/lgtm

k8s-ci-robot · 2022-01-20T15:21:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz, zvonkok

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~~ [marquiz]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 10, 2021

k8s-ci-robot requested review from adrianchiris and kad December 10, 2021 12:19

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 10, 2021

zvonkok force-pushed the multi-arch branch 2 times, most recently from 44fb7a4 to 22aeaa1 Compare December 10, 2021 12:44

zvonkok changed the title ~~Multi ARCH build amd64 for now~~ Multi ARCH build amd64, arm64, s390x Dec 10, 2021

zvonkok changed the title ~~Multi ARCH build amd64, arm64, s390x~~ WIP Multi ARCH build amd64, arm64, s390x Dec 10, 2021

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 10, 2021

zvonkok force-pushed the multi-arch branch from 22aeaa1 to 253bb29 Compare December 10, 2021 13:16

zvonkok changed the title ~~WIP Multi ARCH build amd64, arm64, s390x~~ WIP Multi ARCH build amd64, arm64 Dec 10, 2021

zvonkok force-pushed the multi-arch branch from 253bb29 to cde21fe Compare December 10, 2021 13:29

zvonkok changed the title ~~WIP Multi ARCH build amd64, arm64~~ Multi ARCH build amd64, arm64 Dec 10, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 10, 2021

k8s-ci-robot assigned ArangoGutierrez and marquiz Dec 10, 2021

marquiz mentioned this pull request Jan 13, 2022

Add support for arm64, armV7, armV8 #426

Closed

ArangoGutierrez requested changes Jan 18, 2022

View reviewed changes

hack/init-buildx.sh Outdated

@@ -0,0 +1,41 @@

#!/usr/bin/env bash

# Copyright 2020 The Kubernetes Authors.

Copy link

Contributor

ArangoGutierrez Jan 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update year

ArangoGutierrez mentioned this pull request Jan 18, 2022

WIP: Test if buildx is enabled at Prow #582

Closed

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 19, 2022

marquiz reviewed Jan 19, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

marquiz reviewed Jan 20, 2022

View reviewed changes

zvonkok force-pushed the multi-arch branch from a699966 to a426fe5 Compare January 20, 2022 13:12

marquiz reviewed Jan 20, 2022

View reviewed changes

Iniital multi arch build

c69e47a

zvonkok force-pushed the multi-arch branch from a426fe5 to c69e47a Compare January 20, 2022 14:41

marquiz approved these changes Jan 20, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2022

k8s-ci-robot merged commit 8ea6b76 into kubernetes-sigs:master Jan 20, 2022

marquiz mentioned this pull request Mar 17, 2022

Release v0.11.0 #786

Closed

22 tasks

		@@ -0,0 +1,41 @@
		#!/usr/bin/env bash
		# Copyright 2020 The Kubernetes Authors.

Multi ARCH build amd64, arm64 #698

Multi ARCH build amd64, arm64 #698

Conversation

zvonkok commented Dec 10, 2021 • edited Loading

ArangoGutierrez commented Dec 10, 2021

zvonkok commented Dec 10, 2021

ArangoGutierrez commented Dec 10, 2021

zvonkok commented Dec 10, 2021 • edited Loading

marquiz commented Dec 11, 2021

zvonkok commented Dec 13, 2021

zvonkok commented Dec 13, 2021 • edited Loading

zvonkok commented Dec 13, 2021

zvonkok commented Dec 13, 2021

zvonkok commented Dec 13, 2021

ArangoGutierrez commented Dec 13, 2021

zvonkok commented Dec 14, 2021

marquiz commented Dec 14, 2021

zvonkok commented Dec 14, 2021

ArangoGutierrez commented Dec 21, 2021

fcami commented Dec 21, 2021 • edited Loading

fcami commented Dec 21, 2021

marquiz commented Dec 21, 2021

marquiz commented Jan 13, 2022

ArangoGutierrez Jan 18, 2022

Choose a reason for hiding this comment

ArangoGutierrez commented Jan 18, 2022

marquiz Jan 19, 2022

Choose a reason for hiding this comment

ArangoGutierrez Jan 19, 2022

Choose a reason for hiding this comment

marquiz Jan 19, 2022

Choose a reason for hiding this comment

marquiz Jan 19, 2022

Choose a reason for hiding this comment

marquiz Jan 19, 2022

Choose a reason for hiding this comment

zvonkok commented Jan 19, 2022

marquiz left a comment

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

marquiz Jan 20, 2022

Choose a reason for hiding this comment

zvonkok Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

marquiz commented Jan 20, 2022

marquiz left a comment

Choose a reason for hiding this comment

zvonkok commented Jan 20, 2022

marquiz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 20, 2022

zvonkok commented Dec 10, 2021 •

edited

Loading

zvonkok commented Dec 10, 2021 •

edited

Loading

zvonkok commented Dec 13, 2021 •

edited

Loading

fcami commented Dec 21, 2021 •

edited

Loading

zvonkok Jan 20, 2022 •

edited

Loading