logdna / logspout

LogDNA Docker integration
MIT License
16 stars 20 forks source link

Build Docker images for multiple architectures (i.e. ARMv8) #25

Closed cederberg closed 3 years ago

cederberg commented 3 years ago

This PR is a proof of concept for how to build Docker images for multiple architectures. It only builds for x86-64 and ARM64 here (as that is what I can verify), but it should be possible to support any OS / architecture combination that is supported by Docker and Go.

A few things to note regarding the commits:

  1. The Dockerfile has been split to use stages. This allows us to cross-compile easily and also results in smaller images.
  2. The Docker image is now FROM scratch, meaning it will be smaller and won't contain any debugging tools.
  3. I've made no effort to update the CircleCI build machinery, but instead provide a Makefile. Should be easy enough to change to make build and make publish on the right places.
  4. Please read commit comments if this is of interest, as things are further explained there.

Ideally, the base Logspout image should be multi-architecture already. But seeing that nothing seems to happen there I opted for this approach instead. Whenever they fix their issues, this solution should be revisited.

darinspivey commented 3 years ago

Thanks for the contribution, @cederberg ! We will allocate some time to cleaning this project up a little bit, especially with regards to CircleCI (which we've largely moved off of).

darinspivey commented 3 years ago

Leaving a note for @smusali regarding the build. There's concern about this being tagged as latest as well, so let's talk about that offline before releasing.

cederberg commented 3 years ago

I've now fixed the two issues mentioned and rebased the branch to keep commit history clean and tidy. Note that I included a minor fix to .circleci/config.yml to change grep VERSION --> grep BUILD_VERSION to match the right line.

Regarding CircleCI, I hope you noticed that I didn't include make build or make publish there. Which would be needed to build the multi-architecture images. I'd recommend using make *** for all commands there, to simplify local testing. We use it everywhere. Makes switching tools a breeze.

On the topic of version numbers, please note that upstream ONBUILD sends a version number to go build. I replicated that in the Dockerfile by doing "-X main.Version=$(cat VERSION)-logdna" which includes a VERSION file from the upstream project. If you plan to re-introduce ARG VERSION, this should also be changed to automate all version numbers.

Finally, I can confirm that Docker manifest lists work just like you suggest. Try docker manifest inspect golang for instance and behold how you've been using this for years without being aware of it. 😃

darinspivey commented 3 years ago

Regarding CircleCI, I hope you noticed that I didn't include make build or make publish there.

Yep, all good! The way this works is typically to merge first, then someone tags, and that automatically triggers CircleCI to publish when the git tags are pushed. In this case, we'll do a PR right after this to make those changes, and once it's merged, we'll tag/push/publish.

Thanks!

darinspivey commented 3 years ago

"-X main.Version=$(cat VERSION)-logdna" which includes a VERSION file from the upstream project

Did you not commit the VERSION file? I don't see it. I don't think we'd re-introduce the ARG as a command-line build arg since the version number should always come from that static value. That being said, why isn't BUILD_VERSION declared at the top as a global ARG (with a default), then used throughout? As it sits now, the VERSION file and what's in BUILD_VERSION could be out of sync. How about something like this?

ARG BUILD_VERSION=1.2.0

FROM gliderlabs/logspout:latest AS builder
...
RUN go build -ldflags "-X main.Version=${BUILD_VERSION}-logdna" -o /bin/logspout
...

FROM scratch
...
ENV BUILD_VERSION=${BUILD_VERSION}

Having the build version inside of the running container I don't think is really used, but it's probably nice to have.

cederberg commented 3 years ago

Did you not commit the VERSION file? I don't see it.

Its here: https://github.com/gliderlabs/logspout/blob/master/VERSION

Its downloaded by go get just before the build.

I don't think we'd re-introduce the ARG as a command-line build arg since the version number should always come from that static value. That being said, why isn't BUILD_VERSION declared at the top as a global ARG (with a default), then used throughout? As it sits now, the VERSION file and what's in BUILD_VERSION could be out of sync.

I tried not to change too many things at once... Or perhaps I ran into some weird Docker issue and reverted back. Don't quite remember anymore. 🤷‍♂️

How about something like this?

ARG BUILD_VERSION=1.2.0

FROM gliderlabs/logspout:latest AS builder
...
RUN go build -ldflags "-X main.Version=${BUILD_VERSION}-logdna" -o /bin/logspout
...

FROM scratch
...
ENV BUILD_VERSION=${BUILD_VERSION}

Having the build version inside of the running container I don't think is really used, but it's probably nice to have.

👍

But I'd skip the default value (eventually), remove grep BUILD_VERSION from CircleCI and push a synchronized version everywhere via an env var. CIRCLE_TAG is already set, so I started using that in Makefile.

darinspivey commented 3 years ago

Its downloaded by go get just before the build.

Light bulb, thanks. I was unfamiliar with what go get did, and now it all makes sense! I had thought you had created your own static VERSION file since it was trying to cat it from locally.

darinspivey commented 3 years ago

But I'd skip the default value (eventually), remove grep BUILD_VERSION from CircleCI and push a synchronized version everywhere via an env var. CIRCLE_TAG is already set, so I started using that in Makefile.

Yep, I should have been more clean on our plans. In the future we will switch to our Jenkins CI server, and we will use semantic versioning in an automated fashion. This hardcoded version is something we've deprecated usage of, but still exists in some legacy projects like this one. The usage of CIRCLE_TAG should be fine for the time being while we're still on CircleCI.

darinspivey commented 3 years ago

@cederberg, we've completed the CI changes, and the new images have been published to dockerhub. You should be able to use :latest now on ARM and AMD archs. Thanks for your contribution!

cederberg commented 3 years ago

@darinspivey Thank you! I'll give it a spin and can finally retire my own repo. 👍

Also, you might want to close https://github.com/logdna/logspout/pull/12 since that addresses this same issue, but using a "docker buildx" solution instead. Something to consider once they're moving that out of "experimental stage".