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 --platform to breeze shell and start-airflow #24626

Merged
merged 1 commit into from
Jun 24, 2022

Conversation

uranusjr
Copy link
Member

This allows Breeze to run images of another platform, such as AMD64 on an ARM machine.

For some reason a dependency is not building correctly on ARM. I’ll probably look into that later, but this allows me to finish what I’m doing right now first.

This allows Breeze to run images of another platform, such as AMD64 on
an ARM machine.
@uranusjr uranusjr force-pushed the shell-for-platform branch from 4864e37 to 66f3cdc Compare June 24, 2022 07:20
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Wooohooo! Breeze contribution!

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jun 24, 2022
@github-actions
Copy link

The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@uranusjr
Copy link
Member Author

Hmm, I was under the impression breeze tests also needs this, but apparently it does not and “correctly” picks up the AMD-based image. This leads me to think maybe we are doing something wrong with the overall structure. Currently:

  1. shell and start-airflow only pick up an image if it matches the platform. In other words, if we only have an AMD64 base image available, they would attempt to rebuild the base image against ARM from scratch.
  2. tests automatically picks up the image for AMD64 and run tests against it.

So either shell and start-airflow should also automatically detect the base image is against AMD64 and build against it instead, or tests should not blindly pick up the AMD64 image, and instead build against ARM from scratch by default.

@potiuk
Copy link
Member

potiuk commented Jun 24, 2022

Hmm, I was under the impression breeze tests also needs this, but apparently it does not and “correctly” picks up the AMD-based image. This leads me to think maybe we are doing something wrong with the overall structure. Currently:

  1. shell and start-airflow only pick up an image if it matches the platform. In other words, if we only have an AMD64 base image available, they would attempt to rebuild the base image against ARM from scratch.
  2. tests automatically picks up the image for AMD64 and run tests against it.

So either shell and start-airflow should also automatically detect the base image is against AMD64 and build against it instead, or tests should not blindly pick up the AMD64 image, and instead build against ARM from scratch by default.

Let me take a closer look at that

@potiuk
Copy link
Member

potiuk commented Jun 24, 2022

Initially the idea was that you'd only ever need to "run" stuff on the platform you have - so all the "execution" shoudl by defaiult only be possible on the local platform of yours. Only the image building was considered as "multi-platform" capable. So I think the case here is that we probably do not properly detect the case where we do not have the local image available for the other-platform (because we do not check platform when we look for an image). Simply the "Image build" status is kept per "image" and not per "image+platform" combination.

Actualy now when I think about it- I am not entirely sure if it is actually good idea to execute the "different" platform image. It works only when you have qemu configured properly to emulate the images and I am not even 100% sure if you can "force" the different platform when you already have image with the local platform available.

@potiuk
Copy link
Member

potiuk commented Jun 24, 2022

but I might want to explore "fixing" the build status to be per image + platform combination. This seems like a useful thing to have.

@potiuk
Copy link
Member

potiuk commented Jun 24, 2022

(or you might want to look at it if you want - I can take a look and point out to where they shoudl be changed @uranusjr ). might be good learning experience :D

@uranusjr
Copy link
Member Author

I think retaining the logic is fine—but the problem is that shell (and other developer commands) currently has a different definition to “the platform you have” to tests. Personally I like the idea of allowing running things against a different platform in general, so adding a --platform flag (like in this PR) is what I would like. I’ll try figure out how this can be applied to tests.

@uranusjr uranusjr merged commit 5a8209e into apache:main Jun 24, 2022
@uranusjr uranusjr deleted the shell-for-platform branch June 24, 2022 10:12
@potiuk
Copy link
Member

potiuk commented Jun 24, 2022

Ah yeah. This might also be handy when it comes to running tests on ARM in CI when we convert our CI to use the new breeze for testing (that one is still pending).

but if it's only "tests" and "start-airflow" commands - then it 's easy to do so - a matter of adding platform: in the compose file and filling it with either default or chosen platform:

https://github.com/docker/docker.github.io/blob/master/compose/compose-file/compose-file-v2.md#platform

@potiuk
Copy link
Member

potiuk commented Jun 24, 2022

Tests run docker-compose under the hoood. You can run it with --dry-run and see the command and then it's easy to copy&paste manully after manually correcting the compose file. This is how I'd test it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants