Fixup arm64 build; Remove amd64 hardcode from Makefile to make it more flexible;

[brezerk]

HW: Rock64 (arm64v8)
OS: Gentoo

Hi

Thank you for nice DevOps tool. I was looking forward to build if for my arm64 k8s cluster and got some issues.

Here you are the changes I had to make to compile the binaries and build the images.

Also, i have noticed comments/discussions around #1761 , #1411 and #1474 so i have tried to update Makefile accordingly.

[2opremio]

Thanks for your PR! Let me take a look.

[2opremio]

I am fine with a change which lets you compile for arm64 but that it can't disrupt the current development environments nor break CI or the produced docker image. As it is, this PR doesn't comply with the requirements mentioned.

For instance, you could set an environment variable indicating the target architecture when running make. Without the env variable, compilation happens as usual (compiling fluxd for linux/amd64).

You should also choose the correct base docker image (matching the target architecture).

[brezerk]

Hi @2opremio ,

I have updated PR according to the comments: new ARCH variable has been added. It has default value set to amd64. make ARCH=<arch> could be used to choose the desired target arch.

You should also choose the correct base docker image (matching the target architecture).

The base alpine images are multi-arch ones by default. So no need to change anything here. I have tested the generated images and they looks pretty fine for me:

[ marauder ] brezerk@pts/3:77  ~/develop/helm/flux $
 04/16/19 12:47:18 EEST > kubectl -n flux get pods -o wide
NAME                                  READY   STATUS    RESTARTS   AGE    IP             NODE         NOMINATED NODE   READINESS GATES
flux-6d694c876d-bnhws                 1/1     Running   0          126m   10.244.1.225   cerberus02   <none>           <none>
flux-helm-operator-6db4677799-d4wg8   1/1     Running   0          126m   10.244.2.211   cerberus01   <none>           <none>
flux-memcached-757756884-7czts        1/1     Running   0          142m   10.244.4.132   brentoo01    <none>           <none>
You have new mail.
[ marauder ] brezerk@pts/3:78  ~/develop/helm/flux $
 04/16/19 14:37:27 EEST > kubectl get nodes -l beta.kubernetes.io/arch=arm64 
NAME         STATUS   ROLES    AGE    VERSION
cerberus01   Ready    <none>   169d   v1.13.4
cerberus02   Ready    <none>   21d    v1.13.4
marauder     Ready    master   170d   v1.13.4
[ marauder ] brezerk@pts/3:79  ~/develop/helm/flux $
 04/16/19 14:37:37 EEST > 

Note: if at some point you would like to build official arm/arm64 images you may need to adopt binfmt_misc approach as your current circle docker executor doesn't natively support building ARM images. See prometheus/prometheus#5031 for details.

Thanks

[2opremio]

Can you update bin/upload-binaries to include arm/arm64?

Also, optionally:

1. I am happy with the ARCH env approach but I would be even happier if we produced a fluxd multiarch docker image that we could release. The multi-arch docker build should only be enabled for releases, since producing a multiarch docker image just slows down the normal development cycle.
2. For extra points, could you make sure that the ARCH binaries of kubectl and helm are checksummed and verified before being put into the docker image? Otherwise we should refuse to build.

(2) probably doesn't make much sense without (1). And, as mentioned, they are both optional but desirable, you (or anybody interested) can do it in a later PR if you preferred.

EDIT: reformatted the comment

[brezerk]

Sure. Good points. Will take a look.

[2opremio]

Great, can you also squash the commits once you are done?

[brezerk]

Sure. One question tho...

if [ `basename $@` = "build" -a $(CURRENT_OS_ARCH) = "linux-amd64" ]; then strip $@; fi
[ $* != "linux-amd64" ] || echo "$(KUBECTL_CHECKSUM)  cache/$*/kubectl-$(KUBECTL_VERSION).tar.gz" | shasum -a 256 -c

is it b/c of windows and macos systems used for development purposes do not have strip/shasum binaries? will it be ok if i just change it to $GOOS = "linux" instead?

[2opremio]

if [ `basename $@` = "build" -a $(CURRENT_OS_ARCH) = "linux-amd64" ]; then strip $@; fi
[ $* != "linux-amd64" ] || echo "$(KUBECTL_CHECKSUM)  cache/$*/kubectl-$(KUBECTL_VERSION).tar.gz" | shasum -a 256 -c

is it b/c of windows and macos systems used for development purposes do not have strip/shasum binaries? will it be ok if i just change it to $GOOS = "linux" instead?

Only partly. It's because strip won't match the architecure/OS if the binary being built. With your current change I think it should be [ $* != "${ARCH}-linux" ] .... But you may need to change this when doing multiarch docker images.

[brezerk]

hm...

component=client url=https://quay.io/v2/ status="401 UNAUTHORIZED"
component=client repo=quay.io/weaveworks/kured auth="<zero creds>" api=https://quay.io/v2/
component=client url="https://quay.io/v2/auth?scope=repository%3Aweaveworks%2Fkured%3Apull&service=quay.io" status="400 BAD REQUEST"
component=warmer canonical_name=quay.io/weaveworks/kured auth={map[]} err="requesting tags: Get https://quay.io/v2/weaveworks/kured/tags/list: unknown: Namespace weaveworks has been disabled. Please contact a system administrator."
component=warmer stopping=true
--- FAIL: TestWarming_WarmerWriteCacheRead (10.00s)

does not seems to be an issue related to my changes tho....

[2opremio]

No, that's #1948 :S

[brezerk]

Mkay. After merge with master tests are passed. But now it looks like I squashed the commits in an odd way so PR looks... weird? :)

[hiddeco]

@brezerk it would be better to rebase on origin/master (or probably upstream/master in your case) instead of merging master into your branch, this way you omit a merge commit. Also, the commit message is a bit long, <=50 characters is better.

We just made some changes to the tests to pull images from Docker Hub instead of Quay.io due to it being unavailable to us. So an additional rebase is necessary. Thank you 🌷

[brezerk]

git rebase... ok will take a look, thanks. I am not git wizard yet, sorry for all this spam >_<

[2opremio]

@brezerk did you close the PR intentionally?

[brezerk]

@2opremio Well... I had to reset master to upstream/master and PR got closed automatically sine there was no changes. I have created a new one #1950