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

fix(arm64): use qemu to do the packaging step for arm64 images refs: #ENGEN-715 #520

Merged
merged 12 commits into from
Aug 3, 2022

Conversation

hutchic
Copy link
Contributor

@hutchic hutchic commented Jul 27, 2022

Goal of the PR is the addition of release-kong-docker-images make task that uses build-test-container to build production worthy docker images and push them (leaving the task of renaming build-test-container and a few misnamed environment variables for a subsequent PR)

Putting this capability into kong-build-tools as it's the sensible location to share the logic between Kong and Kong Enterprise without having to copy-pasta between the two repositories. The build-test-container image name change is backwards compatible as it was assigned a variable everywhere it was used.

Previous result

make package-kong && make build-test-container
localhost:3000/kong-ubuntu-20.04

Adjusted result

export DOCKER_RELEASE_REPOSITORY=kong/kong #default
make package-kong && make build-test-container
# locally available docker images
kong/kong:3.0.0-ubuntu-20.04 # multi-arch if building arm
kong/kong:amd64-3.0.0-ubuntu-20.04
kong/kong:arm64-3.0.0-ubuntu-20.04 # only if building arm

New behaviour

export ADDITIONAL_TAGS="hello world"
export DOCKER_RELEASE_REPOSITORY=foo/bar
make package-kong && make release-kong-docker-images
# pushed docker images
foo/bar:3.0.0-ubuntu-20.04 # multi-arch if building arm
foo/bar:amd64-3.0.0-ubuntu-20.04
foo/bar:arm64-3.0.0-ubuntu-20.04 # only if building arm

foo/bar:hello
foo/bar:amd64-hello
foo/bar:arm64-hello

foo/bar:world
foo/bar:amd64-world
foo/bar:arm64-world

@hutchic hutchic force-pushed the fix/arm-images branch 4 times, most recently from ab15393 to ec0d62d Compare July 27, 2022 19:20
@hutchic hutchic force-pushed the fix/arm-images branch 3 times, most recently from 648d71b to cd6639c Compare July 28, 2022 18:09
@hutchic hutchic marked this pull request as ready for review July 29, 2022 12:50
@@ -43,11 +43,12 @@ OFFICIAL_RELEASE ?= true
PACKAGE_CONFLICTS ?= `grep PACKAGE_CONFLICTS $(KONG_SOURCE_LOCATION)/.requirements | awk -F"=" '{print $$2}'`
PACKAGE_PROVIDES ?= `grep PACKAGE_PROVIDES $(KONG_SOURCE_LOCATION)/.requirements | awk -F"=" '{print $$2}'`
PACKAGE_REPLACES ?= `grep PACKAGE_REPLACES $(KONG_SOURCE_LOCATION)/.requirements | awk -F"=" '{print $$2}'`
DOCKER_RELEASE_REPOSITORY ?= `grep DOCKER_RELEASE_REPOSITORY $(KONG_SOURCE_LOCATION)/.requirements | awk -F"=" '{print $$2}'`
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this get tested with kong-ee? I am skeptical this would correctly overwrite with the variable's value in .requirements (https://github.com/Kong/kong-ee/blob/c2d854b8dea021ab581588b8e758665a519e4746/.requirements#L9)

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 couldn't find a way to keep this PR backwards compatible with current Kong CE which doesn't have a value for DOCKER_RELEASE_REPOSITORY.

Someway of in order of precedence:

  1. use an environment variable
  2. use the DOCKER_RELEASE_REPOSITORY value from .requirements
  3. some sensible default

Copy link
Contributor

Choose a reason for hiding this comment

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

I think our build infrastructure for the gateway product already presumes that default values pertain to the building of CE. So option 3, a sensible default for the value required for the CE build, makes sense to me.

DOCKER_TLS_VERIFY=1 \
DOCKER_HOST=$(shell docker-machine env $(DOCKER_MACHINE_ARM64_NAME) | grep 'DOCKER_HOST=".*"' | cut -d\" -f2) \
DOCKER_CERT_PATH=$(shell docker-machine env $(DOCKER_MACHINE_ARM64_NAME) | grep 'DOCKER_CERT_PATH=".*"' | cut -d\" -f2) \
docker run --rm --privileged multiarch/qemu-user-static --reset -p yes && \
Copy link
Contributor

Choose a reason for hiding this comment

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

So, does this PR do away with docker-machine? If so, we should also strip out the docker-machine install.

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 does not.

We use docker-machine / a remote arm worker for the purposes of building Kong arm64. The final step of packaing and placing said package onto the host system output/ directory is via qemu.

README.md Show resolved Hide resolved
test/tests/01-package/run.sh Outdated Show resolved Hide resolved
.ci/setup_ci.sh Show resolved Hide resolved
test/tests/01-package/run.sh Outdated Show resolved Hide resolved
test/tests/01-package/run.sh Outdated Show resolved Hide resolved
@hutchic hutchic changed the title fix(arm64): use qemu to do the packaging step for arm64 images fix(arm64): use qemu to do the packaging step for arm64 images refs: #ENGEN-715 Aug 3, 2022
@curiositycasualty curiositycasualty merged commit 86e23c3 into master Aug 3, 2022
@curiositycasualty curiositycasualty deleted the fix/arm-images branch August 3, 2022 20:43
@github-actions
Copy link

github-actions bot commented Aug 3, 2022

🎉 This PR is included in version 4.33.8 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

hutchic added a commit that referenced this pull request Nov 8, 2022
…#ENGEN-715 (#520)

* fix(arm64): use qemu to do the packaging step for arm64 images

* chore(revert): revert me

* Revert "chore(revert): revert me"

This reverts commit 3913aaa.

* chore(src): dont do anything when we're doing a source code release

* small tweaks

* fix(arm64): s/BUILDPLATFORM/TARGETPLATFORM/g

* docs(README): update and refresh the README.md

* test(wip): run only the oss alpine build #revertme

* Revert "test(wip): run only the oss alpine build #revertme"

This reverts commit 17444f5.

* chore(fix): facepalm

* fix(ci): ADDITIONAL_TAGS applies to non arm manifests as well

* Update test/tests/01-package/run.sh
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.

2 participants