Add pre-Airflow-2-7 hardcoded defaults for config for older providers #32775
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
boring-cyborg
bot
added
area:CLI
provider:cncf-kubernetes
potiuk
requested review from
gmcdonald,
ephraimbuddy,
hussein-awala,
eladkal,
o-nikolas and
pierrejeambrun
|
Based on #32765 so only last commit counts. |
potiuk
force-pushed
the
harcode-pre-2-7-defaults
branch
from
f993b8e
to
6a5a46e
Compare
potiuk
changed the title
This was referenced Jul 22, 2023
Merged
potiuk
force-pushed
the
harcode-pre-2-7-defaults
branch
2 times, most recently
from
5bc91c1
to
08320da
Compare
potiuk
force-pushed
the
harcode-pre-2-7-defaults
branch
2 times, most recently
from
84bf2d5
to
40b4178
Compare
|
Only last commit counts. The others are pre-requisite. |
The test has been recently failing with deadlock (see apache#32778) and needs thorough looking at if we want to find the root cause/remedium. In the meantime it looks like a niche case connected with Dask Executor that is rather obsure and we have no expertise in solving problems with and diagnosing, so until the problem is diagnosed it might be a long time (and maybe even we decide not to care about it and let Dask community take a look and either fix or ignore it. We aim to have a very low number of those Quarantined tests (currently we have 1 and we have not run it for a while as this was a mysql test run on Postgres) but we have now the opportunity to also improve the quarantined tests framework. This test will be run together with other (1) quarantined test and: * they will not be run in our regular tests * they are run sequentially not in parallel with all other tests * they are run for all 4 backends but only for the default versions of those backends * failure of the quarantined tests will not cause failure of the whole job or limit constraints from being generated and updated
During thorough testing and review of moving configuration to provoders I realised that there was a case that was not handled properly. In some cases some providers and DAGs could rely on some default values being available as default, but when we move them from core, and use older version of provider those defaults were not available: * they were remove as defaults in core * the old providers did not have "config" section to contribute the defaults This would be a breaking change and old providers (Celery, K8s) could fail - as it happened in some tests. This PR implements a nice solution to that, also allowing to remove some manual fallbacks in Celery and Kubernetes executor code. The solution is to add a hard-coded "pre-2.7" configuration which would only contain "provider" pre-2.7 hard-coded defaults and make it a fallback option if the values are neither set nor defaults contributed by the providers. We do not have to maintain those - the defaults are "frozen" effectively at the values available just before 2.7. The nice side effect is that we can remove a number of fallbacks, because this hard-coded configuration becomes the fallback automatically, That entirely solves the case where you want to install older providers on 2.7 where config.yml does not contain those provider values.
potiuk
force-pushed
the
harcode-pre-2-7-defaults
branch
from
40b4178
to
dfbb6ad
Compare
jedcunningham
approved these changes
Jul 24, 2023
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
o-nikolas
added a commit
to aws-mwaa/upstream-to-airflow
that referenced
this pull request
Jul 24, 2023
There was a code comment warning that the fallbacks for celery cli commands should not be removed until some conditions are met, but the fallbacks were removed in apache#32775
jedcunningham
pushed a commit
that referenced
this pull request
Jul 24, 2023
There was a code comment warning that the fallbacks for celery cli commands should not be removed until some conditions are met, but the fallbacks were removed in #32775
69 tasks
ephraimbuddy
added
the
changelog:skip
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
During thorough testing and review of moving configuration to provoders
I realised that there was a case that was not handled properly. In some
cases some providers and DAGs could rely on some default values being
available as default, but when we move them from core, and use older
version of provider those defaults were not available:
defaults
This would be a breaking change and old providers (Celery, K8s) could
fail - as it happened in some tests.
This PR implements a nice solution to that, also allowing to remove
some manual fallbacks in Celery and Kubernetes executor code.
The solution is to add a hard-coded "pre-2.7" configuration which
would only contain "provider" pre-2.7 hard-coded defaults and make
it a fallback option if the values are neither set nor defaults
contributed by the providers.
We do not have to maintain those - the defaults are "frozen"
effectively at the values available just before 2.7. The nice side
effect is that we can remove a number of fallbacks, because this
hard-coded configuration becomes the fallback automatically,
That entirely solves the case where you want to install older
providers on 2.7 where config.yml does not contain those provider
values.
^ 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.rstor{issue_number}.significant.rst, in newsfragments.