Add support to create airflow pools from chart values #15093

FloChehab · 2021-03-30T16:52:53Z

I finally had time to look into #11707 ; for this issue I have introduced 4 of my soon-to-be ex-collaborators to this chart so maybe you will have new contributors (you should expect a bit less contributions from myself in the coming months).

Changes

Pools can now be added through the airflowPools value
Docs also updated in that regard

Done in collaboration with @abrasseu @Jattakuy @JonathanCiglia @manzkel .

Closes #11707

Todos

I have left a couple of TODOs mostly in docs as I was having issues cross-referencing docs locally.

^ 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 UPDATING.md.

@abrasseu

* Pools can now be added throught the airflowPools value * Docs also updated in that regard Done in collaboration with @abrasseu @Jattakuy @JonathanCiglia @manzkel . Closes apache#11707

ashb · 2021-03-30T21:15:35Z

chart/templates/create-pools-job.yaml

+          args:
+            - "bash"
+            - "-c"
+            - 'airflow pools import {{ template "airflow_pools_path" . }}'


Suggested change

args:

- "bash"

- "-c"

- 'airflow pools import {{ template "airflow_pools_path" . }}'

args:

- "airflow"

- "pools"

- "import"

- {{ template "airflow_pools_path" . }}

I think

I'm afraid that won't work with some Docker images. It's a similar problem to: #11129

@mik-laj are we good as is ? (there are several location in the charts where airflow is called through bash -c)

ashb · 2021-03-30T21:16:36Z

docs/helm-chart/airflow-configuration.rst

  set within the image, so you need to override it when deploying the chart.
+
+..
+   Uncomment before merge


Should probably do this then, shouldn't we :)

Yes! As mentionned in the PR description, I am having issues cross-referencing documentation and would need help here.

mik-laj · 2021-03-30T23:42:30Z

docs/helm-chart/airflow-configuration.rst

+   Airflow Variables, Connections and Pools
+   ----------------------------------------
+
+   Airflow variables and connections may be set directly with environment variables (see :ref:`apache-airflow:cli-and-env-variables-ref:_env_variables`). You can set them by using the chart values ``extraEnv``, or a combination of ``extraConfigMaps``/``extraSecrets`` and ``extraEnvFrom`` (see :doc:`Parameters reference <parameters-ref>`). You may also use a secret backend (**TODO, before merge. Need clarification: I am not familiar with this.**)


it would be helpful to add an example for lazy people showing how to set up an environment variable that uses a secret.

mik-laj · 2021-03-30T23:52:13Z

chart/templates/create-pools-job.yaml

+          args:
+            - "bash"
+            - "-c"
+            - 'airflow pools import {{ template "airflow_pools_path" . }}'


What do you think to add support for Airflow 1.10.13? This version uses airflow pool --import command.

if airflow pools import --help; then exec airflow pools import .... else: exec airflow pool --import ... fi

This should be doable, I haven't followed much the latest discussions arround the chart but out of curiosity when will the support for 1.10.x be dropped ?

The Chart, for now, will continue supporting 1.10.x too

FloChehab

Hello @mik-laj and @ashb !

Thanks for the reviews and the feedback. Sorry for the little response delays, I was on a break. I should be able to be a bit more responsive in the coming days.

FloChehab · 2021-04-29T07:49:33Z

chart/templates/create-pools-job.yaml

+          args:
+            - "bash"
+            - "-c"
+            - 'airflow pools import {{ template "airflow_pools_path" . }}'


@mik-laj are we good as is ? (there are several location in the charts where airflow is called through bash -c)

FloChehab · 2021-04-29T07:50:42Z

chart/templates/create-pools-job.yaml

+          args:
+            - "bash"
+            - "-c"
+            - 'airflow pools import {{ template "airflow_pools_path" . }}'


This should be doable, I haven't followed much the latest discussions arround the chart but out of curiosity when will the support for 1.10.x be dropped ?

FloChehab · 2021-04-29T07:51:44Z

docs/helm-chart/airflow-configuration.rst

  set within the image, so you need to override it when deploying the chart.
+
+..
+   Uncomment before merge


Yes! As mentionned in the PR description, I am having issues cross-referencing documentation and would need help here.

FloChehab · 2021-04-29T07:52:13Z

docs/helm-chart/airflow-configuration.rst

+   Airflow Variables, Connections and Pools
+   ----------------------------------------
+
+   Airflow variables and connections may be set directly with environment variables (see :ref:`apache-airflow:cli-and-env-variables-ref:_env_variables`). You can set them by using the chart values ``extraEnv``, or a combination of ``extraConfigMaps``/``extraSecrets`` and ``extraEnvFrom`` (see :doc:`Parameters reference <parameters-ref>`). You may also use a secret backend (**TODO, before merge. Need clarification: I am not familiar with this.**)


dnskr · 2021-05-17T11:03:41Z

Any progress on this feature? Looking forward to have it merged :)

FloChehab · 2021-06-06T18:06:10Z

Hello there !
I just wanted to tell you that I am busy and my interest has shifted a bit outside of airflow.
If anyone wants to move this PR forward, feel free to go ahead (from a new one?).

Best of luck !

github-actions · 2021-08-02T00:05:16Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

Add support to create airflow pools from chart values

be3646a

* Pools can now be added throught the airflowPools value * Docs also updated in that regard Done in collaboration with @abrasseu @Jattakuy @JonathanCiglia @manzkel . Closes apache#11707

FloChehab requested review from ashb, dimberman, kaxil, mik-laj and vikramkoka as code owners March 30, 2021 16:52

boring-cyborg bot added area:CLI area:helm-chart Airflow Helm Chart kind:documentation labels Mar 30, 2021

ashb reviewed Mar 30, 2021

View reviewed changes

mik-laj reviewed Mar 30, 2021

View reviewed changes

FloChehab commented Apr 29, 2021

View reviewed changes

github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 2, 2021

github-actions bot closed this Aug 8, 2021

csp33 mentioned this pull request Feb 7, 2023

Configure pools via Helm chart #29401

Closed

Add support to create airflow pools from chart values #15093

Add support to create airflow pools from chart values #15093

Uh oh!

Conversation

FloChehab commented Mar 30, 2021

Changes

Todos

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mik-laj Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mik-laj Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FloChehab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnskr commented May 17, 2021

Uh oh!

FloChehab commented Jun 6, 2021

Uh oh!

github-actions bot commented Aug 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mik-laj Mar 30, 2021 •

edited

Loading

mik-laj Mar 30, 2021 •

edited

Loading