Ensure teardown tasks are executed when DAG run is set to failed #45530
Conversation
jscheffl
added
the
backport-to-v2-10-test
jscheffl
force-pushed
the
bugfix/ensure-teardown-is-executed-when-dag-set-failed
branch
from
b9d8425 to
196b89c
Compare
There was a problem hiding this comment.
Nice catch!
One edge case that I thought about though* - what if the corresponding setup task hasn't finished running yet? (if such exists ofc)
For example:
If you set the DAG run to failed before the cluster was created, the delete_cluster should be skipped.
* Maybe the current architecture already handles, but it's worth checking before merging this PR.
jscheffl
force-pushed
the
bugfix/ensure-teardown-is-executed-when-dag-set-failed
branch
from
196b89c to
031d7af
Compare
Oh, yeah. Tested this explicitly. And actually the Scheduler had this feature before (added this to docs as well, was also new to me): If the setup task is skipped (which is the state when you mark as failed and it was not running) then the teardown is also skipped, |
|
Added some docs as I thought about that slightly the behavior changes - to ensure it is properly documented. |
…o failed (#45530) * Ensure teardown tasks are executed when DAG run is set to failed * Also handle the case of setting DAG to success * Add some documentation to behavior changes * Add some documentation to behavior changes (cherry picked from commit 1e8977a) Co-authored-by: Jens Scheffler <[email protected]>
Backport successfully created: v2-10-test
|
…o failed (apache#45530) * Ensure teardown tasks are executed when DAG run is set to failed * Also handle the case of setting DAG to success * Add some documentation to behavior changes * Add some documentation to behavior changes (cherry picked from commit 1e8977a) Co-authored-by: Jens Scheffler <[email protected]>
…o failed (#45530) (#45581) * [v2-10-test] Ensure teardown tasks are executed when DAG run is set to failed (#45530) * Ensure teardown tasks are executed when DAG run is set to failed * Also handle the case of setting DAG to success * Add some documentation to behavior changes * Add some documentation to behavior changes (cherry picked from commit 1e8977a) Co-authored-by: Jens Scheffler <[email protected]> * Remove type hints only working in Airflow 3 --------- Co-authored-by: Jens Scheffler <[email protected]> Co-authored-by: Jens Scheffler <[email protected]>
…che#45530) * Ensure teardown tasks are executed when DAG run is set to failed * Also handle the case of setting DAG to success * Add some documentation to behavior changes * Add some documentation to behavior changes
…che#45530) * Ensure teardown tasks are executed when DAG run is set to failed * Also handle the case of setting DAG to success * Add some documentation to behavior changes * Add some documentation to behavior changes
|
just notice this could potentially be a breaking change, but doesn't look like something we need a migration rule 🤔 would like to confirm whether we might want to have such a rule? |
I'd say it's a bugfix. Not all "change behaviour" is "breaking change". Every bugfix is a "behavioural change" in essence, the important thing is what is the feature intention and whether the change is "fixing" things or "changing intention on how things should work". SemVer is all about intentions and not about whether behaviour changes or not - see https://semver.org/ In this case it seems reasonable to assume that this behaviour was intention of the "teardown" behaviour - so technically it's a bugfix (and it's been already backported to 2.10 it seems. It might require a newsfragment, but IMHO - that's more than enough. |
For sure it does not need migration rules. No code change needed. Yeah and thought also a bit about if it is breaking, asked in #random / Slack for feedback and nobody except @dstandish was responding. Then after passing... I think it is really in the intend for why we have Setup/Teardown. Behaviour change is "just" because of the side effect that the DAG is not immediately failed if you set it to failed as Teardown need to be scheduled. |
…che#45530) * Ensure teardown tasks are executed when DAG run is set to failed * Also handle the case of setting DAG to success * Add some documentation to behavior changes * Add some documentation to behavior changes
…che#45530) * Ensure teardown tasks are executed when DAG run is set to failed * Also handle the case of setting DAG to success * Add some documentation to behavior changes * Add some documentation to behavior changes
…che#45530) * Ensure teardown tasks are executed when DAG run is set to failed * Also handle the case of setting DAG to success * Add some documentation to behavior changes * Add some documentation to behavior changes
Related to Slack topic: https://apache-airflow.slack.com/archives/CCR6P6JRL/p1736440079894049
We noticed that if a DAG run is set to failed, all tasks are either set to failed or skipped. But if Teardown Tasks are used in a DAG, they are not executed. This could lead to infrastructure or external dependencies not properly cleaned-up.
This PR changes the behavior and does NOT fail/skip teardown tasks if a DAG is set to failed.
A side effect as consequence might be that the DAG is after the call NOT failed, else if it would set it to failed, then any teardown task (even if not skipped/failed) will not scheduled anymore.