You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/06/26 09:26:52 UTC
[GitHub] [airflow] potiuk opened a new pull request, #24664: Add ARM image building for regular PRs
potiuk opened a new pull request, #24664:
URL: https://github.com/apache/airflow/pull/24664
The image building for ARM is currently only done in the main build
only to refresh cache, however there are sometimes cases when
new dependency (for example #24635) broke ARM image build and it
was only discovered after merge.
This PR adds extra ARM-based build that should be run after
the AMD64 build. It should not influence the depending steps,
it should just signal failure of the PR if the ARM image cannot
be build.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragement file, named `{pr_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24664: Add ARM image building for regular PRs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24664:
URL: https://github.com/apache/airflow/pull/24664#issuecomment-1168093297
> @potiuk, just want to double check there is no security/resource considerations here with us spinning up ARM machines for normal, non-committer PRs, right?
Good questions. I :heart: that you asked them :). Here are my explanations and way how it is protected, but I would love a challenge/review and confirming that my thinking is right. Security is a teamwork and I don't trust myself, so I love someone can verify my apprach.
# Security
The ARM steps are executed in 2 caset:
a) build image workfow. Those are not "directly" affected by PR changes - the command will only run the "Dockerfile RUN commands" inside the ARM docker engine - Dockerfile is the only user-provided code that is executed during that build - and it is only executed as part of "docker build" (which runs inside the docker builder). You'd have to escape the container to get into the ARM instance. This is basically the same (or lower) risk we have currently - the Dockerfile RUN commands are executed inside the docker-engine of the S3 AMD instance we run. It's actually a bit lower because the ARM instances do not run the "runner" and the runner - it's a "bare" ARM instance with docker engine running with SSH-port-forwarded connection to it.
b) if you run PR from a branch in "apache" repo (like I did in this PR) - then the ARM instance will run in the CI worklfow. This is controlled by `if: needs.build-info.outputs.inWorkflowBuild == 'true'`. You need to have committer status to be able to push branch to Airflow Repo and make PR from it. Non-commiters can only run this CI workflow from fork, and even if they can change the ci.yaml there and change the `if: needs.build-info.outputs.inWorkflowBuild == 'true'` it will not work because fork-run ci.yaml workflow does not run on our machines and only our build machines have capability to spin the ARM instances. The"build_image" workflow does not run for those PR-s because they are skipped if pull request head is "apache/airlfow" (similarly as all the other "build" steps) https://github.com/apache/airflow/pull/24664/files#diff-a6ce9b5b0a53d8dc6dbd59d40b23994c3e68a91fac73c0d7eacdf373c95476c9R386.
The reason for having those duplicate "build" steps in CI workflow (in case of PRs from apache/airflow repo) is that I can actually test those ARM builds. When they are run from "CI" workflow, they run with "my code", when they are run in "build" workflow - they are run with "main" code and I only can test them when I merge them (which is main reason some of the workflows need a few merges to finally succed - as I have literally no way to test them before merge. For example the "build" workflow Start ARM instance/Stop ARM instance had never been tested, because the only way I can test it is by merging the code to main.
# Cost
Those ARM machines have auto-cancelling - no matter if succeded or failed: 50 minues [self-kill-switch](https://github.com/apache/airflow/blob/main/scripts/ci/images/self_terminate.sh) and "cancel" is run always: https://github.com/apache/airflow/pull/24664/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR417 - so even if someone presses cancel too many times or in case build "hangs" we got maximum ~ 1hr of instance run. The instances have [maximum bid spot price set]( https://github.com/apache/airflow/blob/48ceda22bdbee50b2d6ca24767164ce485f3c319/scripts/ci/images/ci_start_arm_instance_and_connect_to_docker.sh#L27) for requesting the instance. It is set to 0.1 USD /HR. Curent spot price in Ohio: `c6g.xlarge | $0.0399 per Hour` - so currently 1 h of running the instance is 0.04 USD - max 0.1 USD. Building the 4x images (when cache is refreshed - which is vast majority of times) is ~ 12 minutes. This means that running a single ARM instance step is 0.04
/ 5 ~ 0.008 USD (max 0,02 USD). We have ~ 100 image builds / day on average working day = 0.8 to 2 USD / day = 30 USD Month (with rather generous over-counting the days). this is ~ 30/1300 ~ 2% of current spending.
I personally think it's worth it as it saves a lot of our contributors from headaches on M1s - and this is not academic - this has already happened https://github.com/apache/airflow/pull/24635 and it was couple of hours lost for a few people. We have currently no protection mechanisms to prevent it from happening again - and taking into account that a number of deps (including mysql and mssql) do not yet have debian ARM package and fail when building, it is likely to happen again
BTW. Probably we loose way more but not merging some optimisations which are currently in the pipeline (for example this one https://github.com/apache/airflow/pull/24666 - I have not assesed that one because it is far more difficult but I would guess it allows to save ~ 3 minutes of build time x2 for AMD instances at 0.08 USD /hr per version in ~ 80% (wild guess) of our PRs which is already more than that cost of those ARM instances.
And I already saved quite a lot and more : https://github.com/apache/airflow/pull/24580 (and more to come with more breeze conversion) so I am on a cost-and-build-saving spree here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24664: Add ARM image building for regular PRs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24664:
URL: https://github.com/apache/airflow/pull/24664#issuecomment-1167446632
BTW. This one is crucial for those of us who use M1 on a daily basis and rely on the images continue to work for them @dstandish @bbovenzi :eyes:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #24664: Add ARM image building for regular PRs
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24664:
URL: https://github.com/apache/airflow/pull/24664#issuecomment-1167644031
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24664: Add ARM image building for regular PRs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24664:
URL: https://github.com/apache/airflow/pull/24664#issuecomment-1168445200
Actually @jedcunningham - after sleeping over it, it seems we can furter cut down the overhead and cost. It seems that in regular PRs we only need to build it when some depdnencies change. Generally there is no reason to breaj the ARM docker build with the incoming PR unless your change contains some changes to dependencies - which we already detect by "upgradeToNewerDepeendencies" flag.
So we can further reduce the overhead/cost by:
* only running the build in incoming PRs if they change dependencies
* always run it in "push" builds - which means that every time we merge to main or run scheduled build we run ARM build (this is double-checking for potentially automatically upgraded constraints/dependencies)
I'd guess this will be ~ 5% of the previous estimation - so some 1.5 USD/month overhead :). I think we can afford it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #24664: Add ARM image building for regular PRs
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #24664:
URL: https://github.com/apache/airflow/pull/24664#issuecomment-1167752309
@potiuk, just want to double check there is no security/resource considerations here with us spinning up ARM machines for normal, non-committer PRs, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #24664: Add ARM image building for regular PRs
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #24664:
URL: https://github.com/apache/airflow/pull/24664
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24664: Add ARM image building for regular PRs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24664:
URL: https://github.com/apache/airflow/pull/24664#issuecomment-1166467913
Testing execution of in-airlfow-repo PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #24664: Add ARM image building for regular PRs
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #24664:
URL: https://github.com/apache/airflow/pull/24664#issuecomment-1169055918
Thanks for the detailed reply @potiuk. All sounds good and makes sense to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24664: Add ARM image building for regular PRs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24664:
URL: https://github.com/apache/airflow/pull/24664#issuecomment-1168450379
Updated with only running stuff when we are upgrading to newer dependencies :). It should be super-optimized this way
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24664: Add ARM image building for regular PRs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24664:
URL: https://github.com/apache/airflow/pull/24664#issuecomment-1167191019
Seems it's going to work this time (I had to make the builds run sequentially, but it works now. just fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24664: Add ARM image building for regular PRs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24664:
URL: https://github.com/apache/airflow/pull/24664#issuecomment-1167173732
cc: @gmsantos - this will prevent the dramas you faced last week. I am running it from "apache" repo, to make sure it actually works. The nice thing is that for now we are **just** going to test test it and simply mark the build as failure. Nothing waits for the job so it shoudl just add few minutes of overhead/build time and single arm instance starting/stoppping to build all images (~ 2 minutes per image usually when cache is refeshed).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24664: Add ARM image building for regular PRs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24664:
URL: https://github.com/apache/airflow/pull/24664#issuecomment-1167444479
Nice. All ARM images built in under 10m :). Looks green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24664: Add ARM image building for regular PRs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24664:
URL: https://github.com/apache/airflow/pull/24664#issuecomment-1168098448
BTW. @jedcunningham - by writing this answer and looking at the workflows I realized that there is a potential for a small optimization (of cost). I added additional "needs" for the "arm" builds - so that they are executed as late as possible.
Previously they were executed always. bu in this case if either regular CI or PROD will fail, it will not be executed. Small cost reduction, but sttill a cool one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org