Skip to content

Conversation

pstibrany
Copy link
Contributor

@pstibrany pstibrany commented May 7, 2021

Build-image can be built for linux/amd64 (as before) and also linux/arm64 platform.

Added new Makefile target to build multi-arch build image. Such image can only be pushed to repository, and not used locally by Docker. Docker only fetches platform-specific image, when it doesn't have one locally.

To use the feature, one needs to enable buildkit feature in docker config. Building amd64 and arm64 should work regardless of host platform -- docker will simply run non-native platform under qemu.

Added new Makefile target to build multi-arch
build image.

Signed-off-by: Peter Štibraný <[email protected]>
tar xzvf hugo.tar.gz -C /usr/bin && \
chmod +x /usr/bin/hugo

ENV SHFMT_VERSION=3.2.4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bumped version, as 3.1.0 had no arm64 binary.


# Install faillint used to lint go imports in CI.
ENV FAILLINT_VERSION=1.5.0
RUN GO111MODULE=on go get github.com/fatih/faillint@v${FAILLINT_VERSION} && \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved faillint to shared go get build.


# Install embedmd used to embed content into markdown files.
ENV EMBEDMD_VERSION=1.0.0
RUN GO111MODULE=on go get github.com/campoy/embedmd@v${EMBEDMD_VERSION} && \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved embedmd to shared go get build.

Signed-off-by: Peter Štibraný <[email protected]>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks Peter for working on this! Another nice test to do could be pushing the image and updating it in CI workflows, so we can test CI running on the new build-image.

%/$(UPTODATE): %/Dockerfile
@echo
$(SUDO) docker build --build-arg=revision=$(GIT_REVISION) --build-arg=goproxyValue=$(GOPROXY_VALUE) -t $(IMAGE_PREFIX)$(shell basename $(@D)) $(@D)/
$(SUDO) docker tag $(IMAGE_PREFIX)$(shell basename $(@D)) $(IMAGE_PREFIX)$(shell basename $(@D)):$(IMAGE_TAG)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this removed? I think we need it to keep CI working, am I wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's back now... in single docker build command, with multiple -t flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted it back, due to upcoming change in #4211, which builds images for both platforms, but only tags native one with $(IMAGE_PREFIX)$(shell basename $(@D)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to revert that change in #4211, as Github Actions don't support --platform argument when running docker :(

mv shfmt /usr/bin

RUN curl -sfL https://github.com/raw/golangci/golangci-lint/master/install.sh| sh -s -- -b /usr/bin v1.27.0
RUN GO111MODULE=on go get -tags netgo \
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the impact of removing -tags netgo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to https://golang.org/doc/go1.5, netgo is no longer necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In practice, I haven't noticed any difference.

Copy link
Contributor Author

@pstibrany pstibrany May 21, 2021

Choose a reason for hiding this comment

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

Another nice test to do could be pushing the image and updating it in CI workflows, so we can test CI running on the new build-image.

I've updated this PR to use newly built image from this PR.

pstibrany added 7 commits May 21, 2021 13:32
Signed-off-by: Peter Štibraný <[email protected]>
We need 'extended' version, and binaries for this
version are not available for our archs.

Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Changes to build-image/Dockerfile LGTM! I think I'm just missing who is expected to run make push-multiarch-build-image. Could you share more details, please?

@echo Please use push-multiarch-build-image to build and push build image for all supported architectures.
touch $@

push-multiarch-build-image:
Copy link
Contributor

Choose a reason for hiding this comment

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

Who is expected to run this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This target is to be used by maintainer building a new build-image. I've updated how-to-update-the-build-image.md file to make it clear. Note that it should work regardless of platform that dev is using.

pstibrany added 3 commits May 24, 2021 17:04
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany pstibrany merged commit fb14b14 into cortexproject:master May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants