Skip to content

Use proper default airflow_constraints_reference#26148

Merged
potiuk merged 1 commit intoapache:mainfrom
potiuk:add-proper-default-airflow-constraints
Sep 4, 2022
Merged

Use proper default airflow_constraints_reference#26148
potiuk merged 1 commit intoapache:mainfrom
potiuk:add-proper-default-airflow-constraints

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Sep 3, 2022

In case image was built by breeze and not by build command.
default value of the arg was wrong (empty) rather than default
constraint branch. That led to early cache invalidation and much
longer image build than necessary.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

In case image was built by `breeze` and not by `build` command.
default value of the arg was wrong (empty) rather than default
constraint branch. That led to early cache invalidation and much
longer image build than necessary.
@potiuk
Copy link
Member Author

potiuk commented Sep 3, 2022

cc: @o-nikolas @mik-laj -> this was another reason for longer builds than necessary (but only when plain breeze command was used. When you did breeze ci-image build or old breeze build-image - it was fast, but when you run breeze and chose to build image, the build could have taken longer because of this small bug,

@potiuk
Copy link
Member Author

potiuk commented Sep 3, 2022

After we merge this one, I think all of the problems with "sometimes longer build than needed" should be solved

@potiuk potiuk merged commit f74a223 into apache:main Sep 4, 2022
@potiuk potiuk deleted the add-proper-default-airflow-constraints branch September 4, 2022 08:33
@potiuk
Copy link
Member Author

potiuk commented Sep 4, 2022

Also added CI test that will give us an early warning in case of our "canary" builds in main in #26150

potiuk added a commit to potiuk/airflow that referenced this pull request Sep 4, 2022
This is a follow-up after apache#26148 where we found out that basic
breeze command took far more time than it should when user
answered "yes" to rebuild the image.

This PR adds `--max-time` parameter to `breze shell` command which
forces "exit" to be executed as command and fails if the command
does not complete in time specified.

This command is run in "early build cache" so failure there will
not fail the whole build (the job has "continue on error" set),
but we will see the "red" status of the `main` workflow in case
the build takes longer than 120 seconds - indicating that the
build takes too long.

We only run it for `amd` images because if we have a problem
there, we will have similar problem in arm images.

We make sure to clear the docker cache before the command is run
to make sure that it runs in "clean" state.
potiuk added a commit that referenced this pull request Sep 4, 2022
This is a follow-up after #26148 where we found out that basic
breeze command took far more time than it should when user
answered "yes" to rebuild the image.

This PR adds `--max-time` parameter to `breze shell` command which
forces "exit" to be executed as command and fails if the command
does not complete in time specified.

This command is run in "early build cache" so failure there will
not fail the whole build (the job has "continue on error" set),
but we will see the "red" status of the `main` workflow in case
the build takes longer than 120 seconds - indicating that the
build takes too long.

We only run it for `amd` images because if we have a problem
there, we will have similar problem in arm images.

We make sure to clear the docker cache before the command is run
to make sure that it runs in "clean" state.
@o-nikolas
Copy link
Contributor

cc: @o-nikolas @mik-laj -> this was another reason for longer builds than necessary (but only when plain breeze command was used. When you did breeze ci-image build or old breeze build-image - it was fast, but when you run breeze and chose to build image, the build could have taken longer because of this small bug,

I'm late to this party since I was on vacation, but this is awesome! Thanks for doubling down on speeding up Airflow builds @potiuk 👍 😄

@potiuk
Copy link
Member Author

potiuk commented Sep 12, 2022

I'm late to this party since I was on vacation, but this is awesome! Thanks for doubling down on speeding up Airflow builds @potiuk 👍 😄

Yeah. I think I finally nailed all the edge cases for the builds that caused occasional (but extremely annoying) cases where the image was built for a longer time than needed. I have not seen recently any case where things are taking long time and also once we merge the last fix: #26351 ( just realized we had a bug there while answering this comment) we have an actual CI test in our "canary" builds that test if the build is "quick" - so we should have an early warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants