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

Publish Arm64 compatible images #2125

Merged
merged 11 commits into from
Apr 12, 2021
Merged

Publish Arm64 compatible images #2125

merged 11 commits into from
Apr 12, 2021

Conversation

manics
Copy link
Member

@manics manics commented Apr 1, 2021

Part 1 of #2119 (comment): Minor changes to get Docker images building on ARM64

  • hub: psycopg2-binary is not available as a precompiled binary, so libpq-dev needs to be installed for compilation. This increases the build time and the image size. An alternative would be to omit PostgreSQL support from the arm64 image, what do you think?

  • secret-sync: tini for the correct architecture needs to be downloaded.

  • This contains a temporary workflow just for testing, this will be removed. In the meantime you can pull the test images from docker pull ghcr.io/manics/z2jh-{hub,image-awaiter,network-tools,secret-sync}:dev. Not yet tested in a deployment

images/hub/Dockerfile Outdated Show resolved Hide resolved
context: images/${{ matrix.image }}
platforms: linux/amd64,linux/arm64
push: true
tags: ghcr.io/${{ github.repository_owner }}/z2jh-${{ matrix.image }}:dev
Copy link
Member

Choose a reason for hiding this comment

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

Ohhh wow how nice and clean these steps are.

We have some complexity if we want to use this instead of build args and a dynamic tag based on chartpress.yaml and git inspection logic in chartpress. This looks great though.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a quick and easy way to expose the built images for download. I should have a chartpress PR to replace some of this soon 😸

@manics manics changed the title Arm64 compatible images WIP: Arm64 compatible images Apr 2, 2021
@manics
Copy link
Member Author

manics commented Apr 10, 2021

Remaining requirements:

@manics
Copy link
Member Author

manics commented Apr 12, 2021

@consideRatio Do you want me to keep this new workflow? Or get rid of it and add the flags to:

@consideRatio
Copy link
Member

I'm not sure, I'd like the idea of having the logic to publish images in a single workflow file I think. What is your take?

@manics
Copy link
Member Author

manics commented Apr 12, 2021

Just realised the test-chart.yaml workflow won't work because you can't load multiple architectures into the host Docker daemon. I don't see a reason to have a separate push workflow though, so what probably makes sense is:

  • a workflow that builds but doesn't push all architectures, this can be either a new job in test-chart.yaml or this existing workflow with --push removed
  • add --platforms to ci/publish

@consideRatio
Copy link
Member

consideRatio commented Apr 12, 2021

Thinking about this a bit more now...

  • When we publish, we publish all images.
  • When we test the chart we normally test image build + chart install/startup tests
  • With the new arm64 addition, we are currently only able to test the build of arm64 images.
  • With a arm64 runner, we can test the image build + chart install/startup tests just and hopefully avoid breaking DRY doing so by generalizing the test image build + chart install/startup test

I'm very afraid of duplicating the test suite and really want to avoid breaking DRY in this case. I'm hoping we can have a arm64 based (self-hosted / circle-ci hosted) github actions runner or similar. If we assume that is plausible to accomplish, I'd be leaning towards:

  1. Embed the publish parts of arm64 images to the publish workflow
  2. We can retain this separate workflow test-arm64-image-build workflow until we have a arm64 based runner that can run the chart tests as well, at which point we let this logic merge into the test-chart workflow that will execute on both amd64 and arm64 runners and be able to adjust based on this.

@consideRatio
Copy link
Member

  • a workflow that builds but doesn't push all architectures, this can be either a new job in test-chart.yaml or this existing workflow with --push removed

Our test-chart workflow as it is already contains a build through chartpress, and it won't rebuild if it finds an existing image - but, the existing image will be in docker.io/jupyterhub/k8s-... rather than a local registry. Hmm...

I would be positive to have a dedicated build job if we don't just build in that dedicated build job AND the test-chart job that also builds images unless they are existing already.

I guess the ideal outcome given the complexity tradeoff doesn't make it non-ideal would be to:

  • have a dedicate image build step
  • have a dedicated test chart step using already built images
  • (not sure, but maybe...) have a dedicated publish step that re-use images tested rather than re-builds them again

@consideRatio
Copy link
Member

@manics nice work on this! The CHP images are published like a charm and I love the new actions job to extract relevant tags!

image

@manics manics changed the title WIP: Arm64 compatible images Arm64 compatible images Apr 12, 2021
@manics manics changed the title Arm64 compatible images Publish Arm64 compatible images Apr 12, 2021
@manics
Copy link
Member Author

manics commented Apr 12, 2021

I've updated the publish workflow to (hopefully) build and push the multiple architectures, and removed the push flag from the docker build workflow.

I don't think configuring circle-ci is too bad. The aim of the arm64 workflow will be to test the images not the helm chart, so we can skip most of the complexity. I've made a start in https://github.com/manics/zero-to-jupyterhub-k8s/blob/arm64-circleci/.circleci/config.yml

Removing pebble and network policies removes a lot of the complex setup. It'll require a flag in the tests to allow non-https requests to be made, but I think that's worthwhile anyway since it makes it easier to developers to test locally. If we want we could use this as an opportunity to test some other config that's not already tests.

@manics manics marked this pull request as ready for review April 12, 2021 21:03
@consideRatio consideRatio merged commit 6be4ace into jupyterhub:master Apr 12, 2021
@consideRatio
Copy link
Member

Wieee nice work on this @manics! What an effort!

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