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

Nox session to build and push multiplatform images #3324

Merged
merged 16 commits into from
May 29, 2023

Conversation

SteveDMurphy
Copy link
Contributor

@SteveDMurphy SteveDMurphy commented May 18, 2023

Closes tbd

Used this article for some guidance: https://www.thorsten-hans.com/how-to-build-multi-arch-docker-images-with-ease/#verify-multiple-architectures-in-docker-hub

Code Changes

  • refactor push to use discrete commands for each parameter (makes the function much more readable/maintainable
  • push will now also handle builds, as this is a requirement when doing multiplatform images.
  • Add some tests for the new buildx command generation logic
  • remove the build step from the image publication workflow
  • move the nox tests to run with the static checks for simplicity
  • add the docker buildx create command in case CI needs it

Steps to Confirm

  • run nox -s "push(dev)" and validate that two images are built
  • Do this in CI and validate the warnings are no longer present on an M1

Pre-Merge Checklist

Description Of Changes

This PR is a big overhaul of the push nox session that will allow us to publish multi-arch images and hopefully in a reasonably elegant way. It leverages buildx (part of docker experimental features) to achieve this and led to cleaning up a lot of the push-related logic

@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.09 ⚠️

Comparison is base (bd90888) 87.22% compared to head (ff64e6a) 87.13%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3324      +/-   ##
==========================================
- Coverage   87.22%   87.13%   -0.09%     
==========================================
  Files         311      312       +1     
  Lines       18586    18659      +73     
  Branches     2368     2377       +9     
==========================================
+ Hits        16212    16259      +47     
- Misses       1958     1980      +22     
- Partials      416      420       +4     
Impacted Files Coverage Δ
src/fides/api/db/util.py 25.00% <100.00%> (ø)
src/fides/api/models/privacy_request.py 96.23% <100.00%> (ø)

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ThomasLaPiana
Copy link
Contributor

ThomasLaPiana commented May 19, 2023

This is making me realize we shouldn't really be doing the silicon image in plus but instead pushing the same image with other other build targets 🤔

https://www.thorsten-hans.com/how-to-build-multi-arch-docker-images-with-ease/

https://www.docker.com/blog/multi-arch-build-and-images-the-simple-way/

Great finds here @SteveDMurphy ! Seems like buildx was the magic sauce we were missing all along.

I'll dig into this and clean it up a bit, but maybe this will finally solve our woes, at which point we can port it over to plus as well 🙂

@SteveDMurphy
Copy link
Contributor Author

I'll dig into this and clean it up a bit, but maybe this will finally solve our woes, at which point we can port it over to plus as well 🙂

Awesome, thanks @ThomasLaPiana ! Let me know if you want to dabble together or need an M1 to test with 🙌🏽

This is making me realize we shouldn't really be doing the silicon image in plus but instead pushing the same image with other other build targets 🤔

I think the only reason we ended up with the silicon image in plus was because of the way in which the arm image is built in GitHub Actions instead of on an M1 - something about that ended up being the difference (from what I could see at the time anyways). There may have been a concern as well as to what impacts may exist for other ARM platforms if an M1 build is pushed up instead

@ThomasLaPiana
Copy link
Contributor

ThomasLaPiana commented May 22, 2023

@SteveDMurphy afaik Macs (M1, M2, soon to be M3) are the only ARM machines that really matter currently

There is "ARM for windows" but it's, as I understand it, pretty much totally neglected. Otherwise there are some servers that use ARM but they're in the minority and very much more special use-cases

@cypress
Copy link

cypress bot commented May 25, 2023

Passing run #2229 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge ff64e6a into f05c436...
Project: fides Commit: 70a202e510 ℹ️
Status: Passed Duration: 00:45 💡
Started: May 29, 2023 1:01 AM Ended: May 29, 2023 1:02 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@ThomasLaPiana
Copy link
Contributor

@SteveDMurphy i'm digging into this deep today (got good vibes) and found this lol: docker/buildx#166

Apparently you can not build and push separately. Hence the refactor I did here, and will also be updating push so that it builds and pushes at once

@ThomasLaPiana
Copy link
Contributor

@SteveDMurphy I'm pretty satisfied with where I'm getting this, testing it now (which takes forever though because of emulation used during multiplat builds)

Feel free to take a look and lmk if this route makes sense

@SteveDMurphy
Copy link
Contributor Author

Apparently you can not build and push separately. Hence the refactor I did here, and will also be updating push so that it builds and pushes at once

Yes the build/push requirement got me early on too ! I thought I had added something about it somewhere, my bad 😳

@ThomasLaPiana
Copy link
Contributor

ThomasLaPiana commented May 25, 2023

Apparently you can not build and push separately. Hence the refactor I did here, and will also be updating push so that it builds and pushes at once

Yes the build/push requirement got me early on too ! I thought I had added something about it somewhere, my bad 😳

I ran this locally and everything seemed ok....I'm going to push a dev version for real to make sure everything works

Feel free to start digging in!

@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review May 25, 2023 11:53
@ThomasLaPiana
Copy link
Contributor

I ran a proper nox -s "push(dev)" and it succeeded 🥳

@TheAndrewJackson
Copy link
Contributor

This issue came up when running nox -s "push(dev)" on my machine.

I think it failed at this step => ERROR [linux/amd64 compile_image 8/8] RUN pip install --user -U pip --no-cache-dir install -r requirements.txt

with this error (I've removed some of the stacktrace)

#0 310.9       gcc: internal compiler error: Segmentation fault signal terminated program cc1plus
#0 310.9       Please submit a full bug report,
#0 310.9       with preprocessed source if appropriate.
#0 310.9       See <file:///usr/share/doc/gcc-10/README.Bugs> for instructions.
#0 310.9       error: command '/usr/bin/gcc' failed with exit code 4
#0 310.9       [end of output]
#0 310.9   
#0 310.9   note: This error originates from a subprocess, and is likely not a problem with pip.
#0 311.0   ERROR: Failed building wheel for cchardet
#0 311.0   Running setup.py clean for cchardet
#0 311.9 Successfully built fideslang fideslog multidimensional_urlencode sqlalchemy-citext validators pyyaml starkbank-ecdsa flatdict
#0 311.9 Failed to build cchardet
#0 311.9 ERROR: Could not build wheels for cchardet, which is required to install pyproject.toml-based projects
------
Dockerfile:28
--------------------
  26 |     
  27 |     COPY requirements.txt .
  28 | >>> RUN pip install --user -U pip --no-cache-dir install -r requirements.txt
  29 |     
  30 |     ##################
--------------------
ERROR: failed to solve: process "/dev/.buildkit_qemu_emulator /bin/sh -c pip install --user -U pip --no-cache-dir install -r requirements.txt" did not complete successfully: exit code: 1
nox > Command docker buildx build --push --target=prod --platform linux/amd64,linux/arm64 . --tag ethyca/fides:dev failed with exit code 1
nox > Session push(dev) failed.

Did you run into this issue on your M1 @SteveDMurphy?

@ThomasLaPiana
Copy link
Contributor

This issue came up when running nox -s "push(dev)" on my machine.

I think it failed at this step => ERROR [linux/amd64 compile_image 8/8] RUN pip install --user -U pip --no-cache-dir install -r requirements.txt

with this error (I've removed some of the stacktrace)

#0 310.9       gcc: internal compiler error: Segmentation fault signal terminated program cc1plus
#0 310.9       Please submit a full bug report,
#0 310.9       with preprocessed source if appropriate.
#0 310.9       See <file:///usr/share/doc/gcc-10/README.Bugs> for instructions.
#0 310.9       error: command '/usr/bin/gcc' failed with exit code 4
#0 310.9       [end of output]
#0 310.9   
#0 310.9   note: This error originates from a subprocess, and is likely not a problem with pip.
#0 311.0   ERROR: Failed building wheel for cchardet
#0 311.0   Running setup.py clean for cchardet
#0 311.9 Successfully built fideslang fideslog multidimensional_urlencode sqlalchemy-citext validators pyyaml starkbank-ecdsa flatdict
#0 311.9 Failed to build cchardet
#0 311.9 ERROR: Could not build wheels for cchardet, which is required to install pyproject.toml-based projects
------
Dockerfile:28
--------------------
  26 |     
  27 |     COPY requirements.txt .
  28 | >>> RUN pip install --user -U pip --no-cache-dir install -r requirements.txt
  29 |     
  30 |     ##################
--------------------
ERROR: failed to solve: process "/dev/.buildkit_qemu_emulator /bin/sh -c pip install --user -U pip --no-cache-dir install -r requirements.txt" did not complete successfully: exit code: 1
nox > Command docker buildx build --push --target=prod --platform linux/amd64,linux/arm64 . --tag ethyca/fides:dev failed with exit code 1
nox > Session push(dev) failed.

Did you run into this issue on your M1 @SteveDMurphy?

Interesting 🤔

As long as it runs in CI though, this doesn't really make a difference, but it is interesting to know that it might not work on M1 machines. CI runs amd/64 so I think it'd be ok there, but honestly it is hard to test without running it for real in CI

@SteveDMurphy
Copy link
Contributor Author

This issue came up when running nox -s "push(dev)" on my machine.

I think it failed at this step => ERROR [linux/amd64 compile_image 8/8] RUN pip install --user -U pip --no-cache-dir install -r requirements.txt

with this error (I've removed some of the stacktrace)

@TheAndrewJackson I hadn't tested these in a bit (@ThomasLaPiana did all the real work!) but was able to confirm this morning that the commands ran for me on an M1. I'm pretty sure I have seen that same error message in the past however, but struggling to remember what I did to sort it out

image

@TheAndrewJackson
Copy link
Contributor

I tried again and I was able to successfully run the command after clearing my docker cache 🤷‍♂️

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

i like the changes a lot! besides the obvious benefit of allowing us to build images for multiple architectures, the required refactors are sensible - they leave the touched pieces of the nox codebase more modular, easier to follow and test.

just left a couple of minor nits for cleanup of docstrings. i tested nox -s "push(dev)" locally on my M1 and things worked well (though that did take a while!).

(one thing that may be nice as an additional manual confirmation step, if we haven't done it already -- we could push an alpha/feature tag to this branch with nox -s "tag(push)" and just ensure that the git-tag push job still works as it always has to push a tagged "feature" image. this would be a simple and low-impact way to be more certain the refactors haven't caused any regressions on this codepath.)

noxfiles/docker_nox.py Outdated Show resolved Hide resolved
noxfiles/docker_nox.py Outdated Show resolved Hide resolved
@ThomasLaPiana ThomasLaPiana changed the title nox function to build and push multi-arch images Nox session to build and push multi-arch images May 27, 2023
@ThomasLaPiana ThomasLaPiana changed the title Nox session to build and push multi-arch images Nox session to build and push multiplatform images May 27, 2023
@ThomasLaPiana
Copy link
Contributor

i like the changes a lot! besides the obvious benefit of allowing us to build images for multiple architectures, the required refactors are sensible - they leave the touched pieces of the nox codebase more modular, easier to follow and test.

just left a couple of minor nits for cleanup of docstrings. i tested nox -s "push(dev)" locally on my M1 and things worked well (though that did take a while!).

(one thing that may be nice as an additional manual confirmation step, if we haven't done it already -- we could push an alpha/feature tag to this branch with nox -s "tag(push)" and just ensure that the git-tag push job still works as it always has to push a tagged "feature" image. this would be a simple and low-impact way to be more certain the refactors haven't caused any regressions on this codepath.)

that's a great idea, I definitely don't want to break your awesome work 🙂 will test that now

@ThomasLaPiana
Copy link
Contributor

retesting after the latest changes...

  1. nox -s "push(dev)" - ✅
  2. nox -s "push(git-tag)" - ✅

image

🥳

@ThomasLaPiana
Copy link
Contributor

so the push job definitely takes forever, but all is working as expected!

@ThomasLaPiana ThomasLaPiana merged commit 8d424d3 into main May 29, 2023
@ThomasLaPiana ThomasLaPiana deleted the multi-arch-support branch May 29, 2023 04:31
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.

4 participants