feat: add dag processor deployment #577
Conversation
Signed-off-by: Burak Karakan <burak.karakan@gmail.com>
…e values.yaml file Signed-off-by: Burak Karakan <burak.karakan@gmail.com>
…eadme Signed-off-by: Burak Karakan <burak.karakan@gmail.com>
karakanb
force-pushed
the
feature/add-new-deployment-for-dag-processor
branch
from
0557ffd
to
7e0a4a6
Compare
|
Waiting for this functionality! ) |
There was a problem hiding this comment.
@karakanb Thanks for this PR (sorry for the massive delay), I have left a few comments, but there are a few general things we need to discuss:
- Should we be enabling this feature by default?
- My initial instinct is no, as this increases the number of Pods, and users are probably not expecting it.
- However, we should include a new faq page about how to
Use Standalone Dag Processorfor those who want it.
- We should include some kind of liveness probe, but I am not sure if airflow has a "job" type that we can check the heartbeat of (similar to how we do for the scheduler)
| `dagProcessor.podAnnotations` | Pod annotations for the dag processor Deployment | `{}` | ||
| `dagProcessor.safeToEvict` | if we add the annotation: "cluster-autoscaler.kubernetes.io/safe-to-evict" = "true" | `true` | ||
| `dagProcessor.podDisruptionBudget.*` | configs for the PodDisruptionBudget of the dag processor Deployment | `<see values.yaml>` | ||
| `dagProcessor.livenessProbe.*` | liveness probe for the dag processor Pods | `<see values.yaml>` |
There was a problem hiding this comment.
If there is no livenessProbe we should remove this.
There was a problem hiding this comment.
I have added a liveness probe, please give it a look
| livenessProbe: | ||
| enabled: true | ||
| initialDelaySeconds: 10 | ||
| periodSeconds: 30 | ||
| timeoutSeconds: 60 | ||
| failureThreshold: 5 |
There was a problem hiding this comment.
We should not have these values if we don't actually use them.
| true | ||
| {{- end -}} | ||
| {{- else -}} | ||
| false |
There was a problem hiding this comment.
The truthiness of this airflow.dagProcessor.should_use is actually simply whether it is defined.
In this case, we actually do want to allow the dag processor (if we don't know what version the user is using). Because in that case, they are probably using a custom image. All this is to say, we should make this string say true.
thesuperzapper
moved this from PR | Needs Review
to PR | Waiting for Changes
in Issue Triage and PR Tracking
|
thanks for giving this a look, I wanted to discuss some of these points as well.
|
Signed-off-by: Burak Karakan <burak.karakan@gmail.com>
|
also, one question: is there a reason why you picked running Python code for the liveness probes instead of the airflow CLI commands? |
Signed-off-by: Burak Karakan <burak.karakan@gmail.com>
Signed-off-by: Burak Karakan <burak.karakan@gmail.com>
Signed-off-by: Burak Karakan <burak.karakan@gmail.com>
|
anything i can help with this? it would be very useful for my deployment. |
|
I have moved off of this chart to the official one, unfortunately I don't think I'll be able to maintain / follow-up on this PR, feel free to take over. |
thesuperzapper
changed the title
What issues does your PR fix?
As of version 2.3.0, Airflow allows running DagProcessorManager as standalone process. This PR adds the ability to run a separate deployment for the dag processor component to separate it from the scheduler.
What does your PR do?
This PR adds the deployment, PDB and docs for the new processor component. I couldn't figure out what to put in the liveness probe, so feedback is welcome.
Checklist
For all Pull Requests
For releasing ONLY