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

Target multiple architectures (follow-up to #28) #31

Merged
merged 10 commits into from
Aug 3, 2020
Merged

Target multiple architectures (follow-up to #28) #31

merged 10 commits into from
Aug 3, 2020

Conversation

Filius-Patris
Copy link
Contributor

This is a follow-up to #28, I force-pushed to it... 🙄

Description:

This PR makes the CI (GitHub Actions) build docker images working on not just x86_64, but also armv7 and arm64 (for example Raspberry Pis).

Prerequisites:

This PR is based on #30. While you technically could just merge this one, I think it separates the commits into more logically complete units.

Caveats:

It increases the buildtimes by a huge amount.

x86_64 x86_64 + armv7 + arm64
~ 6min ~ 1h 25min

Build minutes on GitHub Actions are free for public repos though, so I think this would be fine.

See also:

Fixes issue #21.
Follow-up to PR #28.
PR #30 resolves problems with cypress.

Filius-Patris and others added 4 commits July 31, 2020 17:47
Commit 3198879 prepended the CYPRESS_INSTALL_BINARY=0 variable to
the install command in the ressources/assets directory, however
this component does not require cypress.

The root component however does. This environment variable should
be prepended to the second install command.
This error message shows it:
https://github.com/Filius-Patris/docker-koel/runs/915997770?check_suite_focus=true#step:5:8914

This issue comment suggests why:
sass/node-sass#1176 (comment)
TL;DR: Building some component requires python. This would normally be
precompiled and fetched, but arm doesn't have precompiled packages.
@Filius-Patris
Copy link
Contributor Author

Dude, whatthe..... why are these failing again....

@Filius-Patris Filius-Patris marked this pull request as draft July 31, 2020 17:33
The CICD errors seems to be tied to this:
yarnpkg/yarn#4890 (comment)
@Filius-Patris Filius-Patris marked this pull request as ready for review July 31, 2020 18:52
@Filius-Patris
Copy link
Contributor Author

Hmm... the goss test fail, but not because of some test cases, but rather because this:

docker: Error response from daemon: pull access denied for hyzual/koel-dev, repository does not exist or may require 'docker login': denied: requested access to the resource is denied.

@Hyzual could you look into it? Is it a login related issue?

@Hyzual
Copy link
Collaborator

Hyzual commented Aug 1, 2020

Hi, it's likely an issue with the Docker tag. I imagine the images now have tags to distinguish their supported architecture (amd64, arm) so we need to add the right tag to the command. I'll take a look later today.

@Filius-Patris
Copy link
Contributor Author

Filius-Patris commented Aug 1, 2020

Ok, I think I got the issue. buildx uses its own build cache. You can export it to a registry using --push or export it to the docker engine using --load. However --load only supports single architecture manifests (ie. tags) (as of now).

Now the workarounds are:

  • run tests are part of the build
  • build single arch image, test with docker run, then build multi-arch image. Cache from the first build will be reused.
  • use a local registry

See issue docker/buildx#59 (which is closed ironically)

To me the most reasonable approach would be option 2. It should be fairly easy to implement and probably won't increase the build times by a lot.


I have one more question though:
How are images pushed? It looks to happen in the make dgoss-ci stage, but what exactly does it do? I'm afraid that I won't get this step working as it is now. What are the actions taken in this step besides pushing the image?

If the make dgoss-ci stage only really pushes the image, I'd suggest following workflow:

  1. Install goss and dgoss
    using e1himself/goss-installation-action@v1
  2. Checkout
  3. Make test build (only x86_64)
    running make
  4. Run goss tests
    running dgoss run hyzual/koel-dev:latest
  5. Build & push release build (all 3 architectures)
    using ilteoood/docker_buildx@1.0.4

The CI tests then would omit step 5 and thus be as fast as before (~6min).

@Hyzual
Copy link
Collaborator

Hyzual commented Aug 1, 2020

Oops I just realized I made a mistake again... The stage is indeed named make dgoss-ci which is wrong. That command is meant to run dgoss tests and should be that instead of dgoss run hyzual/koel-dev:latest... 🤦 .
So, yes the wrongly named make dgoss-ci stage only really pushes the image to the docker hub registry.

I agree with you, option 2 looks the better choice. I am fine with the proposed workflow: Docker Image CI should only test x86_64 (same as today), and Docker Hub publish workflow should also build and push the 3 architectures.

This is meant to test the CI. If it works, it will be done in
publish.yml instead of dockerimage.yml.
@Filius-Patris
Copy link
Contributor Author

Wait, don't merge this. Publishing won't work. You'll need to use buildx with the --push flag.

I'll update the PR tomorrow. Are you leaning more towards using actions or using Makefile definitions?
Makefile definitions allow you to build the project locally more easily, however they're uglier when working with credentials.

@Hyzual
Copy link
Collaborator

Hyzual commented Aug 1, 2020

Ok thanks :). I was not going to merge this as it is anyway, we should edit publish.yml and move the last stage to it.
Let's write the command in the actions yaml directly. Publishing is not meant to be done locally so I don't see any benefit of using a Makefile definition for this.
What will it look like for people using the image ? Do they still pull hyzual/koel:latest or do they need to add a tag to pull the right architecture ?

@Hyzual
Copy link
Collaborator

Hyzual commented Aug 1, 2020

Side comment: I was wondering why the build time sky-rocketed from about 7 min to more than an hour. I just realized that the single step of building and minifiying koel's frontend assets takes 20 minutes on an arm / arm64 architecture. That's just crazy 😧

For more details, see the discussions on PR #31.

With this commit, some targets in the Makefile are unneeded, but
I left them anyway.
I wasn't understanding the docs correctly.
With this commit the pipeline completely cut reliance to the Makefile.
This helps to make everything more obvious by only looking at the pipeline
yaml file.
@Filius-Patris
Copy link
Contributor Author

Filius-Patris commented Aug 2, 2020

What will it look like for people using the image ? Do they still pull hyzual/koel:latest or do they need to add a tag to pull the right architecture ?

Nope, just pull hyzual/koel:latest and the docker engine will figure out which underlying image to pull.

I've made these changes to the pipeline:

  • Renamed files and pipeline names for more clarity
  • Use an Action for building the Docker file (x86_64)
  • Copy the dgoss-ci Makefile target directly into the pipeline yaml file.

I hope you're fine with them. If not, just mention it.


A further question:
It would offer better quality pipelines, if we made one pipeline just for testing and one just for deployment. Since it seems you commit new code directly to master, I'd suggest to run make sure this branch is tested before release too.

The structure mentioned here might be useful for this repo:
https://git.luolix.topmunity/t/is-there-a-way-to-depend-on-actions-from-another-file/16620/2

Should I implement this too in the same PR?

@Filius-Patris
Copy link
Contributor Author

I'd suggest one workflow file like this:

name: Continuous integration and deployment
on:
  push:
    branches: [master]
jobs:
  test:
    runs-on: ubuntu-latest
    steps:
      - name: Install goss and dgoss
        uses: e1himself/goss-installation-action@v1
        with:
          # Goss release version to install
          version: "v0.3.13"
      - name: Checkout code
        uses: actions/checkout@v2
      - name: Build the testing Docker image
        uses: docker/build-push-action@v1.1.0
        with:
          push: false
          repository: hyzual/koel
          tags: test
      - name: Run goss tests on the testing image
        run: dgoss run hyzual/koel:test
  deploy:
    runs-on: ubuntu-latest
    needs: [test]
    if: github.ref == 'refs/heads/master'
    steps:
      - name: Checkout code
        uses: actions/checkout@v2
      - name: Build and push production image
        uses: zmingxie/docker_buildx@v1.1
        with:
          publish: true
          imageName: hyzual/koel
          tag: latest
          dockerHubUser: ${{ secrets.DOCKER_HUB_USERNAME }}
          dockerHubPassword: ${{ secrets.DOCKER_HUB_PASSWORD }}

@Hyzual
Copy link
Collaborator

Hyzual commented Aug 3, 2020

Ok great ! Thanks for the changes, I am fine with them !
I had not thought to define both workflows in a single file. I did not know you could make one workflow depend on another, yeah the setup looks simpler. Let's do that in a dedicated PR though, I feel like this one has already been long-winded.

Thanks for all the work, I'll be merging this PR

@Hyzual Hyzual merged commit 46250ae into koel:master Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants