Add callback support on kubernetes executors - #70475
Conversation
Runs synchronous callbacks (e.g. Deadline Alerts) as supervised callback pods, mirroring LocalExecutor/CeleryExecutor. Adds callback dispatch in queue_workload/_process_workloads, a construct_callback_pod path, watcher key resolution via the callback_id annotation, WorkloadKey/WorkloadState type widening, and unit + k8s integration tests. Guarded by AIRFLOW_V_3_3_PLUS for provider backward compatibility.
local import to follow pre-existing file conventions
|
@ferruzzi Currently one of the big bug fixes was in |
| elif AIRFLOW_V_3_3_PLUS: | ||
| from airflow.utils.state import CallbackState | ||
|
|
||
| state = CallbackState.SUCCESS |
There was a problem hiding this comment.
Shouldn't this be wrapped in a state check if Callback.state is still QUEUED? Assuming SUCCESS feels dangerous.
There was a problem hiding this comment.
From how I understand it the callback state is never updated during the running so it will always be in the QUEUED state so adding the check wouldnt really change the behaviour of the code as far as i am aware
There was a problem hiding this comment.
And there is no state row in callback states for the db because there is a lot more cases that tasks can be than callbacks with callbacks only having 6 potential states and tasks having 12ish potential states.
There was a problem hiding this comment.
But if we do add the check it could help to catch anything in the future?
There was a problem hiding this comment.
Alright, I've been thinking about this. It looks like this is fine how it is for now, but it may need some more thinking later. Something feels off with the callback state flow here, but I can't place it right now.
Go ahead and resolve this when you read it. If you do go on to do the callback-retry work, maybe spend some time thinking this through... it seems like there's a step missing here with the state updates compared to the task state updates.
|
@JH-A-Kim It seems worth doing, but maybe in a different PR, this one is already pretty chonky and I don't believe any execujtors currently retry callbacks. I think the right answer there is to convert |
yeah that makes sense ill create an issue when I get the time to ask about potentially adding retry logic to callbacks, or should I create a discussion on the devlist? |
An Issue is fine for now, but mark is as dependent on this PR and on #63491 both being merged, that way we can reduce merge conflicts all around. |
Description
Related PR's: #67449
#63454
Related bug fix PR: #70148
To reiterate the main points this pr aims to implement support for callbacks on the Kubernetes executor because it currently only knows how to handle task instances. This PR makes it so it can also handle
ExecuteCallbackworkloads. These are keyed byCallbackKeyso that callbacks can execute as their own pod instead of requiring another mechanism.The core changes are the following:
AIRFLOW_V_3_3_PLUSso that behaviour older versions of Airflow can remain unchangedqueue_workloadandprocess_workloadsnow branch on the the workload is either aExecuteTaskorExecuteCallbackExecuteCallbackcalled_run_next_callbackwhich builds and submits the pod itself and returnsNone.PodGenerator.construct_callback_podwhich builds a minimal pod for a callback and tags it with acallback_idid annotation and aairflow-workload-type: callbacklabel so that the watcher can tell it apart from task podscallback_idthe watcher reconstructs aCallbackKeyinstead of aTaskInstanceKeyand state resolution treats a callback with no error asCallbackState.SUCCESSrather than querying the (nonexistent)TaskInstancerow.Was generative AI tooling used to co-author this PR?
Generated-by: Sonnet 5 following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.