Helmchart: Provision Standalone Dag Processor #23711
Conversation
This comment was marked as spam.
This comment was marked as spam.
True, that makes sense, I can remove the persistent DAG volume depending on |
|
@jedcunningham rebased and incorporated the review suggestions. It's working as expected in a dev deployment: > cat values.yaml
[...]
dagProcessor:
enabled: true
[...]
> kubectl get pods
NAME READY STATUS RESTARTS AGE
airflow-cleanup-27546945-55mjl 0/1 Completed 0 41m
airflow-cleanup-27546960-mslp7 0/1 Completed 0 26m
airflow-cleanup-27546975-zvh75 0/1 Completed 0 11m
airflow-dag-processor-68479f4fb-8bfnx 1/1 Running 0 3m52s
airflow-scheduler-699b79bc7d-59wc2 2/2 Running 0 3m52s
airflow-scheduler-699b79bc7d-j7xb4 2/2 Running 0 53s
airflow-statsd-579db47f48-nms6x 1/1 Running 0 29h
airflow-triggerer-5df797bbf6-zrng6 1/1 Running 0 3m51s
airflow-webserver-78bcfc575-btwc4 1/1 Running 0 3m25s
airflow-webserver-78bcfc575-vjrv2 1/1 Running 0 3m50sThis deployment does not use gitsync or persistent volume mounted with the pods, but it should work according to the tests. |
|
Nice! @mhenc -> maybe you want to take a look too? |
github-actions
bot
added
the
full tests needed
|
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. |
|
Would love to get some feedback from our Helm people :) |
|
Related #23872 We might need this patch before running the standalone dag processor with the official image. |
|
@gmsantos, sorry for the delay, just got around to testing this. I overlooked the fact that with LocalExecutor, the scheduler is the worker 🤦♂️. So the condition to mount DAGs needs to take that into account as well. You can probably use |
|
@jedcunningham okay, let me take a look. |
|
@jedcunningham I have a question about the I'm assuming that we should include |
|
Yes, exactly. Sorry, that was a half baked comment, but that's what I was envisioning - expanding $local to cover both LocalExecutor and LocalKubernetesExecutor and using it to determine whether to mount DAGs in addition to the standalone flag. |
|
hey @jedcunningham, it should be ready to review. I just trying to make all the PR checks pass ✅ |
|
@jedcunningham I could use some help to retry the failed CI checks here, please :) |
With local executor, the scheduler is the worker and it needs access to the DAGs
see the explanation for this logic #23711 (comment)
|
@jedcunningham I added some combinations of DAG Processor and executors in c8c5265 I assumed that gitsync and persistence was always enabled to make the assertions easier to understand. |
|
Thanks @gmsantos 🎉🍺 |
|
Hi @gmsantos, Sorry the mention in a merged PR, but taking into account the new Thanks! |
|
Different subdirs are only available as of Airlfow 2.4.0 and this is not available yet in the Helm Chart. But feel free to add it as PR. |
I'm not completely sure how is the best way to implement this feature, but at least I'll start by opening the issue -> #26494 |
Initial support to deploy standalone dag processor from Helm on Airflow 2.3+
It is disabled by default for now.
^ 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.