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/16 16:01:43 UTC
[GitHub] [airflow] Aakcht opened a new pull request, #24496: Add possibility to use the same url for result backend and metadata connections
Aakcht opened a new pull request, #24496:
URL: https://github.com/apache/airflow/pull/24496
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
<!--
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] dstandish commented on pull request #24496: Add `db+` prefix if missing for celery result backend
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #24496:
URL: https://github.com/apache/airflow/pull/24496#issuecomment-1159221977
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)?
--
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] dstandish commented on a diff in pull request #24496: WIP: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #24496:
URL: https://github.com/apache/airflow/pull/24496#discussion_r901107665
##########
airflow/config_templates/config.yml:
##########
@@ -1670,12 +1670,13 @@
or insert it into a database (depending of the backend)
This status is used by the scheduler to update the state of the task
The use of a database is highly recommended
+ When not specified, sql_alchemy_conn with db+ prefix will be used for connection
http://docs.celeryproject.org/en/latest/userguide/configuration.html#task-result-backend-settings
version_added: ~
type: string
sensitive: true
example: ~
- default: "db+postgresql://postgres:airflow@postgres/airflow"
Review Comment:
i think it would be nice to move this to `example:` so it will be preserved but commented out
--
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] Aakcht commented on a diff in pull request #24496: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
Aakcht commented on code in PR #24496:
URL: https://github.com/apache/airflow/pull/24496#discussion_r903694646
##########
airflow/config_templates/default_celery.py:
##########
@@ -36,6 +36,12 @@ def _broker_supports_visibility_timeout(url):
if _broker_supports_visibility_timeout(broker_url):
broker_transport_options['visibility_timeout'] = 21600
+if conf.has_option("celery", 'RESULT_BACKEND'):
+ result_backend = str(conf.get('celery', 'RESULT_BACKEND'))
Review Comment:
Fixed
--
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] uranusjr commented on a diff in pull request #24496: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24496:
URL: https://github.com/apache/airflow/pull/24496#discussion_r903680341
##########
airflow/config_templates/default_celery.py:
##########
@@ -36,6 +36,12 @@ def _broker_supports_visibility_timeout(url):
if _broker_supports_visibility_timeout(broker_url):
broker_transport_options['visibility_timeout'] = 21600
+if conf.has_option("celery", 'RESULT_BACKEND'):
+ result_backend = str(conf.get('celery', 'RESULT_BACKEND'))
Review Comment:
```suggestion
result_backend = conf.get_mandatory_value('celery', 'RESULT_BACKEND')
```
--
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] Aakcht commented on pull request #24496: Add `db+` prefix if missing for celery result backend
Posted by GitBox <gi...@apache.org>.
Aakcht commented on PR #24496:
URL: https://github.com/apache/airflow/pull/24496#issuecomment-1159536912
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](https://github.com/apache/airflow/blob/main/airflow/config_templates/default_airflow.cfg#L850) - hopefully it won't be too much of a problem.
--
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 #24496: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24496:
URL: https://github.com/apache/airflow/pull/24496#issuecomment-1161901647
Nice! You made it with parameterizzed tests :)
--
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 merged pull request #24496: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
jedcunningham merged PR #24496:
URL: https://github.com/apache/airflow/pull/24496
--
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] uranusjr commented on a diff in pull request #24496: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24496:
URL: https://github.com/apache/airflow/pull/24496#discussion_r902221995
##########
airflow/config_templates/default_celery.py:
##########
@@ -36,6 +36,12 @@ def _broker_supports_visibility_timeout(url):
if _broker_supports_visibility_timeout(broker_url):
broker_transport_options['visibility_timeout'] = 21600
+if conf.has_option("celery", 'RESULT_BACKEND'):
+ result_backend = str(conf.get('celery', 'RESULT_BACKEND'))
+else:
+ log.info("Value for celery result_backend not found. Using sql_alchemy_conn with db+ prefix.")
+ result_backend = f'db+{conf.get("database", "SQL_ALCHEMY_CONN")}'
Review Comment:
Let’s put this into a function. Logging on import time can be problematic due to startup order.
--
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] Aakcht commented on pull request #24496: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
Aakcht commented on PR #24496:
URL: https://github.com/apache/airflow/pull/24496#issuecomment-1162395466
All the tests have finally passed :relieved:
--
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 a diff in pull request #24496: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24496:
URL: https://github.com/apache/airflow/pull/24496#discussion_r904358371
##########
airflow/config_templates/config.yml:
##########
@@ -1670,12 +1670,13 @@
or insert it into a database (depending of the backend)
This status is used by the scheduler to update the state of the task
The use of a database is highly recommended
+ When not specified, sql_alchemy_conn with db+ prefix will be used for connection
Review Comment:
```suggestion
When not specified, sql_alchemy_conn with a db+ scheme prefix will be used
```
##########
chart/values.yaml:
##########
@@ -319,7 +319,24 @@ extraEnvFrom: ~
# Airflow database & redis config
data:
# If secret names are provided, use those secrets
+ # These secrets must be created manually, eg:
+ #
+ # kind: Secret
+ # apiVersion: v1
+ # metadata:
+ # name: custom-airflow-metadata-secret
+ # type: Opaque
+ # data:
+ # connection: base64_encoded_connection_string
+
metadataSecretName: ~
+ # When providing secret names and using the same database for metadata and
+ # result backend, for Airflow < 2.4.0 it is necessary to create a separate
+ # secret for result backend but with db+ prefix in connection. For example:
+ # data:
+ # connection: base64_encoded_connection_string_with_db+_prefix
Review Comment:
```suggestion
# secret for result backend but with a db+ scheme prefix.
```
I don't think the inline example really adds much value? I also changed the wording a bit (so it reads better to me at least).
##########
chart/values.yaml:
##########
@@ -319,7 +319,24 @@ extraEnvFrom: ~
# Airflow database & redis config
data:
# If secret names are provided, use those secrets
+ # These secrets must be created manually, eg:
+ #
+ # kind: Secret
+ # apiVersion: v1
+ # metadata:
+ # name: custom-airflow-metadata-secret
+ # type: Opaque
+ # data:
+ # connection: base64_encoded_connection_string
+
metadataSecretName: ~
+ # When providing secret names and using the same database for metadata and
+ # result backend, for Airflow < 2.4.0 it is necessary to create a separate
+ # secret for result backend but with db+ prefix in connection. For example:
+ # data:
+ # connection: base64_encoded_connection_string_with_db+_prefix
+ # For airflow >= 2.4.0 it is possible to not specify the secret,
+ # connection from metadataSecretName with db+ prefix will be used by default
Review Comment:
```suggestion
# For Airflow >= 2.4.0 it is possible to not specify the secret again,
# as Airflow will use sql_alchemy_conn with a db+ scheme prefix by default.
```
I think this reads better.
--
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] Aakcht commented on a diff in pull request #24496: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
Aakcht commented on code in PR #24496:
URL: https://github.com/apache/airflow/pull/24496#discussion_r903696635
##########
airflow/config_templates/default_celery.py:
##########
@@ -36,6 +36,12 @@ def _broker_supports_visibility_timeout(url):
if _broker_supports_visibility_timeout(broker_url):
broker_transport_options['visibility_timeout'] = 21600
+if conf.has_option("celery", 'RESULT_BACKEND'):
+ result_backend = str(conf.get('celery', 'RESULT_BACKEND'))
+else:
+ log.info("Value for celery result_backend not found. Using sql_alchemy_conn with db+ prefix.")
Review Comment:
Changed to debug
--
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 a diff in pull request #24496: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24496:
URL: https://github.com/apache/airflow/pull/24496#discussion_r903138270
##########
airflow/config_templates/default_celery.py:
##########
@@ -36,6 +36,12 @@ def _broker_supports_visibility_timeout(url):
if _broker_supports_visibility_timeout(broker_url):
broker_transport_options['visibility_timeout'] = 21600
+if conf.has_option("celery", 'RESULT_BACKEND'):
+ result_backend = str(conf.get('celery', 'RESULT_BACKEND'))
+else:
+ log.info("Value for celery result_backend not found. Using sql_alchemy_conn with db+ prefix.")
Review Comment:
I almost wonder if this should be debug?
##########
chart/templates/_helpers.yaml:
##########
@@ -74,6 +74,7 @@ If release name contains chart name it will be used as a full name.
key: webserver-secret-key
{{- end }}
{{- if or (eq .Values.executor "CeleryExecutor") (eq .Values.executor "CeleryKubernetesExecutor") }}
+ {{- if or (semverCompare "<=2.3.2" .Values.airflowVersion) (.Values.data.resultBackendSecretName) (.Values.data.resultBackendConnection) }}
Review Comment:
```suggestion
{{- if or (semverCompare "<2.4.0" .Values.airflowVersion) (.Values.data.resultBackendSecretName) (.Values.data.resultBackendConnection) }}
```
This change won't make it into a core release until 2.4.0.
##########
chart/values.yaml:
##########
@@ -319,6 +319,16 @@ extraEnvFrom: ~
# Airflow database & redis config
data:
# If secret names are provided, use those secrets
+ # These secrets must be created manually, eg:
+ #
+ # kind: Secret
+ # apiVersion: v1
+ # metadata:
+ # name: custom-airflow-metadata-secret
+ # type: Opaque
+ # data:
+ # connection: base64_encoded_connection_string
+
metadataSecretName: ~
resultBackendSecretName: ~
Review Comment:
Might be worth a comment that this will use `metadataSecretName`, if it's set.
##########
chart/templates/secrets/result-backend-connection-secret.yaml:
##########
@@ -20,6 +20,7 @@
#################################
{{- if not .Values.data.resultBackendSecretName }}
{{- if or (eq .Values.executor "CeleryExecutor") (eq .Values.executor "CeleryKubernetesExecutor") }}
+{{- if or (semverCompare "<=2.3.2" .Values.airflowVersion) (.Values.data.resultBackendSecretName) (.Values.data.resultBackendConnection) }}
Review Comment:
```suggestion
{{- if or (semverCompare "<2.4.0" .Values.airflowVersion) (and (semverCompare ">=2.4.0" .Values.airflowVersion) .Values.data.resultBackendConnection) }}
```
Either we are pre-2.4, or we are post 2.4 with a `resultBackendConnection`. We already check for `resultBackendSecretName`, so we can drop that portion.
--
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] Aakcht commented on a diff in pull request #24496: WIP: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
Aakcht commented on code in PR #24496:
URL: https://github.com/apache/airflow/pull/24496#discussion_r901152233
##########
airflow/config_templates/config.yml:
##########
@@ -1670,12 +1670,13 @@
or insert it into a database (depending of the backend)
This status is used by the scheduler to update the state of the task
The use of a database is highly recommended
+ When not specified, sql_alchemy_conn with db+ prefix will be used for connection
http://docs.celeryproject.org/en/latest/userguide/configuration.html#task-result-backend-settings
version_added: ~
type: string
sensitive: true
example: ~
- default: "db+postgresql://postgres:airflow@postgres/airflow"
Review Comment:
Good point, changed
--
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] dstandish commented on a diff in pull request #24496: Add `db+` prefix if missing for celery result backend
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #24496:
URL: https://github.com/apache/airflow/pull/24496#discussion_r900495836
##########
airflow/config_templates/default_celery.py:
##########
@@ -36,6 +36,14 @@ def _broker_supports_visibility_timeout(url):
if _broker_supports_visibility_timeout(broker_url):
broker_transport_options['visibility_timeout'] = 21600
+result_backend = str(conf.get('celery', 'RESULT_BACKEND'))
+if result_backend.startswith(('postgresql://', 'sqlite:///', 'mysql://')):
Review Comment:
we also support mssql: `mssql+pyodbc://`
and legacy postgres scheme (for older version of sqlalchemy) is i think `postgres://`
--
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] dstandish commented on pull request #24496: Add `db+` prefix if missing for celery result backend
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #24496:
URL: https://github.com/apache/airflow/pull/24496#issuecomment-1159524107
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)
--
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] Aakcht commented on a diff in pull request #24496: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
Aakcht commented on code in PR #24496:
URL: https://github.com/apache/airflow/pull/24496#discussion_r903695984
##########
chart/values.yaml:
##########
@@ -319,6 +319,16 @@ extraEnvFrom: ~
# Airflow database & redis config
data:
# If secret names are provided, use those secrets
+ # These secrets must be created manually, eg:
+ #
+ # kind: Secret
+ # apiVersion: v1
+ # metadata:
+ # name: custom-airflow-metadata-secret
+ # type: Opaque
+ # data:
+ # connection: base64_encoded_connection_string
+
metadataSecretName: ~
resultBackendSecretName: ~
Review Comment:
Currently it won't because you have to create two separate secrets that are almost the same except that one has db+ prefix - that's the reason for this pr :D Added info that for airflow 2.4.0 and above it will
--
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] Aakcht commented on pull request #24496: Add `db+` prefix if missing for celery result backend
Posted by GitBox <gi...@apache.org>.
Aakcht commented on PR #24496:
URL: https://github.com/apache/airflow/pull/24496#issuecomment-1159277582
@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](https://github.com/apache/airflow/blob/main/chart/templates/secrets/result-backend-connection-secret.yaml#L21) is created every time when custom result backend secret is not provided and the [env for the connection](https://github.com/apache/airflow/blob/main/chart/templates/_helpers.yaml#L76) is always defined.
--
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 #24496: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24496:
URL: https://github.com/apache/airflow/pull/24496#issuecomment-1161334267
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] Aakcht commented on pull request #24496: Use sql_alchemy_conn for celery result backend when result_backend is not set
Posted by GitBox <gi...@apache.org>.
Aakcht commented on PR #24496:
URL: https://github.com/apache/airflow/pull/24496#issuecomment-1160848382
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.
--
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