Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for building multi-CPU architecture images #1130

Closed
wants to merge 3 commits into from

Conversation

paulfantom
Copy link
Contributor

What this PR does / why we need it:

  • finalizing support for creating multi-arch images (Makefile already had some targets to build images for multiple architectures)
  • adding support for creating a container manifest joining images into one
  • fixing propagation of ARCH variable in kube-state-metrics target
  • removed the containerized build process as there is no benefit of it when using go modules.

Tested by running:

make release-images REGISTRY=quay.io/paulfantom TAG=$(git rev-parse --short HEAD)

Output can be seen in https://quay.io/repository/paulfantom/kube-state-metrics?tab=tags

Images validated on ARMv7 and AMD64 systems.

NOTE: This changes how images are published to quay.io. After merging, publishing images is done with make release-images. Added info about it in RELEASE.md

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1037

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 29, 2020
@paulfantom paulfantom changed the title *: add support for building multi-CPU architecture images add support for building multi-CPU architecture images Apr 29, 2020
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: paulfantom
To complete the pull request process, please assign tariq1890
You can assign the PR to them by writing /assign @tariq1890 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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 k8s-ci-robot requested review from andyxning and lilic April 29, 2020 12:17
@brancz
Copy link
Member

brancz commented Apr 29, 2020

There is some work on publishing the images to k8s.gcr.io automatically, at that point we will probably no longer support the quay repo. I'm not sure how much work we should put into it. I'm fine with this, but be aware that it might disappear soon, and I don't know if the auto publishing workflow will support multi-arch.

@lilic
Copy link
Member

lilic commented Apr 29, 2020

There is some work on publishing the images to k8s.gcr.io automatically, at that point we will probably no longer support the quay repo

Yes we discussed with Pawel :)

I'm fine with this, but be aware that it might disappear soon, and I don't know if the auto publishing workflow will support multi-arch.

Not true, we will still publish the coreos images, no?

@paulfantom
Copy link
Contributor Author

paulfantom commented Apr 29, 2020

/hold

Apparently docker on travis has permissions problems when trying to create manifests.

Apparently this is fixed but not really: docker/for-linux#396

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2020
@paulfantom
Copy link
Contributor Author

/unhold

moved to use manifest-tool tool instead of experimental docker feature to solve travis issue with docker permissions. Also simplifies Makefile a bit.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2020
${DOCKER_CLI} tag $(MULTI_ARCH_IMG):$(TAG) $(MULTI_ARCH_IMG):latest
rm -rf "${TEMP_DIR}"
manifest-tool:
curl -fsSL https://github.com/estesp/manifest-tool/releases/download/v1.0.2/manifest-tool-linux-amd64 > ./manifest-tool
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move the manifest-tool version to a variable

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also have a bin existence check, so that we don't run this every time. Something like this:

HAS_GOLANGCI := $(shell which golangci-lint) which is used in the lint make target

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simple checking for binary existence is a contrast to requiring a version of a binary. Consider having a pre-existing system-wide binary in version A, but specifying version B in Makefile. In such scenario you would need to add another check to validate globally installed binary version against version requested in Makefile. It is a question of code simplicity vs first execution speed.

@@ -84,41 +80,57 @@ TEMP_DIR := $(shell mktemp -d)
all: all-container

sub-container-%:
$(MAKE) --no-print-directory ARCH=$* container
# 'clean' is necessary since build process needs to overwrite kube-state-metrics binary. Otherwise all images end up with binary made for one architecture
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not have clean as a pre-requisite to the sub-container-% target?

Copy link
Contributor Author

@paulfantom paulfantom May 7, 2020

Choose a reason for hiding this comment

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

That was the first thing I did, but it didn't work 🤷


sub-push-%:
$(MAKE) --no-print-directory ARCH=$* push
# 'clean' is necessary since build process needs to overwrite kube-state-metrics binary. Otherwise all images end up with binary made for one architecture
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not have clean as a pre-requisite to the sub-push-% target?

@tariq1890
Copy link
Contributor

Thanks for the PR @paulfantom :). Provided some review comments

@mrueg
Copy link
Member

mrueg commented Jun 29, 2020

@tariq1890 @paulfantom since I'm using kube-state-metrics on arm64, I would love to see this change in as it will simplifying deployment a lot. Anything I can help out with here to move it forward?

@paulfantom
Copy link
Contributor Author

@mrueg AFAIK right now kube-state-metrics is moving container builds to gcr (#1155). After that move is done I don't this PR is necessary anymore (definitely not in this form).

Since right now I don't have time or desire to investigate this issue again, I am closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ARM images to quay.io image
6 participants