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

Use sql_alchemy_conn for celery result backend when result_backend is not set #24496

Merged
merged 2 commits into from
Jun 23, 2022

Conversation

Aakcht
Copy link
Contributor

@Aakcht Aakcht commented Jun 16, 2022

This PR adds possibility to use the same connection for celery result_backend and metadata by automatically adding db+ for result_backend connection.

closes #18283


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 newsfragement file, named {pr_number}.significant.rst, in newsfragments.

@wip ready for review

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Jun 16, 2022
@dstandish dstandish changed the title Add possibility to use the same url for result backend and metadata connections Add db+ prefix if missing for celery result backend Jun 16, 2022
@dstandish
Copy link
Contributor

dstandish commented Jun 17, 2022

A little unsure about this one. On one hand I get the pain, and like the idea of making it more convenient. On the other hand it's good to be explicit. Another way of approaching this... what if we just made it so that if celery result backend is not configured, then it would take the database as the default (with prefix)?

So then, if it's configured, we use what we are given (even if it's wrong because missing db prefix, thus forcing the user to update the config to be correct). But if it's not configured, i.e. so that there is no value for conf.get('celery', 'RESULT_BACKEND'), then we use the database sql_alchemy_conn value and add prefix.

cc @jedcunningham

@Aakcht
Copy link
Contributor Author

Aakcht commented Jun 17, 2022

@dstandish For me the issue is mostly related to airflow helm chart/kubernetes: I have the same problem as described in #18283 - I want to use the same pre-created secret for both metadata and resultBackendConnection in helm chart, see https://github.com/apache/airflow/blob/main/chart/values.yaml#L322 . Currently it's not possible because one of the secrets has to start with "db+".

I suppose using sql_alchemy_conn when result_backend is not configured would work, but in this case some changes will be required in the helm chart too, will it be ok?

For example currently with celery executor this secret for result_backend connection is created every time when custom result backend secret is not provided and the env for the connection is always defined.

@dstandish
Copy link
Contributor

i see re https://github.com/apache/airflow/blob/main/chart/templates/secrets/result-backend-connection-secret.yaml#L21 we create the secret when none referenced in your config. good point.

so yeah if you like this suggestion, i.e. to deal with this in airflow config, then i think in the chart we just have to look at airflow version and if greater than 2.4 (or whenever this hypothetical change implemented) then don't create the secret (because sql_alchemy_conn will be used by default anyway)

@Aakcht
Copy link
Contributor Author

Aakcht commented Jun 18, 2022

Sure, I'll try implementing the change so sql_alchemy_conn is used when result_backend is not set. Looks like I'll have to change default value of result_backend to None (currently it is set to db+postgresql://postgres:airflow@postgres/airflow - hopefully it won't be too much of a problem.

@Aakcht Aakcht marked this pull request as draft June 18, 2022 18:55
@Aakcht Aakcht changed the title Add db+ prefix if missing for celery result backend WIP: Use sql_alchemy_conn for celery result backend when result_backend is not set Jun 18, 2022
@Aakcht Aakcht force-pushed the result_backend_db_formatting branch 2 times, most recently from 498efd3 to 2a89d02 Compare June 19, 2022 13:26
@Aakcht Aakcht force-pushed the result_backend_db_formatting branch 8 times, most recently from 1f43dce to 26cec60 Compare June 20, 2022 19:54
@Aakcht Aakcht marked this pull request as ready for review June 20, 2022 20:49
@Aakcht Aakcht changed the title WIP: Use sql_alchemy_conn for celery result backend when result_backend is not set Use sql_alchemy_conn for celery result backend when result_backend is not set Jun 20, 2022
@Aakcht
Copy link
Contributor Author

Aakcht commented Jun 20, 2022

The PR is ready for review - not sure why WIP check is still in progress :/ I can retrigger the checks if needed.

As @potiuk suggested, I modified helm chart tests with parameterized to make it easier to update helm charts to a future airflow version.

@Aakcht Aakcht requested a review from dstandish June 20, 2022 21:08
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jun 21, 2022
@github-actions
Copy link

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.

@Aakcht Aakcht force-pushed the result_backend_db_formatting branch from 26cec60 to 6426d27 Compare June 21, 2022 09:33
@potiuk
Copy link
Member

potiuk commented Jun 21, 2022

Nice! You made it with parameterizzed tests :)

@dstandish dstandish force-pushed the result_backend_db_formatting branch from 087d0cb to 6c3cfe0 Compare June 21, 2022 16:42
@Aakcht Aakcht force-pushed the result_backend_db_formatting branch from 6c3cfe0 to 890749c Compare June 21, 2022 19:07
@Aakcht
Copy link
Contributor Author

Aakcht commented Jun 21, 2022

All the tests have finally passed 😌

airflow/config_templates/default_celery.py Outdated Show resolved Hide resolved
chart/templates/_helpers.yaml Outdated Show resolved Hide resolved
chart/values.yaml Show resolved Hide resolved
@Aakcht Aakcht force-pushed the result_backend_db_formatting branch from d8df6ba to ea893ac Compare June 22, 2022 11:56
@Aakcht Aakcht force-pushed the result_backend_db_formatting branch from ea893ac to c6e201e Compare June 22, 2022 12:42
chart/values.yaml Outdated Show resolved Hide resolved
chart/values.yaml Outdated Show resolved Hide resolved
airflow/config_templates/config.yml Outdated Show resolved Hide resolved
@Aakcht Aakcht force-pushed the result_backend_db_formatting branch from c6e201e to f2d7d84 Compare June 23, 2022 00:17
@Aakcht Aakcht force-pushed the result_backend_db_formatting branch from f2d7d84 to e033b68 Compare June 23, 2022 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart full tests needed We need to run full set of tests for this PR to merge type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

db+ string in result backend but not metadata secret
6 participants