amoghrajesh commented on code in PR #65741:
URL: https://github.com/apache/airflow/pull/65741#discussion_r3158974207
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/triggers/pod.py:
##########
@@ -411,17 +414,50 @@ async def get_task_state(self):
async def safe_to_cancel(self) -> bool:
"""
- Whether it is safe to cancel the external job which is being executed
by this trigger.
+ Whether it is safe to delete the pod during trigger cleanup.
- Cancel is NOT safe when the task is still in DEFERRED state, because
it means the
- triggerer is redistributing triggers and the trigger will be recreated
on another triggerer.
- Cancel IS safe when the task state has changed (e.g. user marked it as
success/failed).
+ Used only on Airflow < 3.3.0 where the triggerer does not invoke
``on_kill()`` for user kills.
+ Deletion is NOT safe when the task is still in DEFERRED state
(triggerer restart).
"""
task_state = await self.get_task_state()
return task_state != TaskInstanceState.DEFERRED
+ async def on_kill(self) -> None:
+ """
+ Delete the pod when the trigger is cancelled by a user action.
+
+ The triggerer invokes this for user-initiated kills on Airflow 3.3.0+
only; on older versions
+ use ``cleanup()`` and ``safe_to_cancel()`` instead.
+ """
+ if self._fired_event:
+ self.log.debug("Skipping on_kill since an event has already been
fired.")
+ return
+
+ if self.on_kill_action == OnKillAction.KEEP_POD:
+ self.log.debug("Skipping on_kill since on_kill_action is set to
%r.", self.on_kill_action.value)
+ return
+
+ self.log.info("Deleting pod %s in namespace %s.", self.pod_name,
self.pod_namespace)
+ try:
+ await self.hook.delete_pod(
+ name=self.pod_name,
+ namespace=self.pod_namespace,
+ grace_period_seconds=self.termination_grace_period,
+ )
+ except Exception:
+ self.log.exception("Unexpected error while deleting pod %s",
self.pod_name)
+
async def cleanup(self) -> None:
- """Clean up the pod when the trigger is cancelled."""
+ """
+ Clean up the pod when the trigger exits.
+
+ On Airflow 3.3.0+ pod deletion on user kill is handled in
``on_kill()`` only; this avoids
+ deleting pods on triggerer restart. On older Airflow versions,
``cleanup()`` still uses
+ ``safe_to_cancel()`` because ``on_kill()`` is not wired for user kills.
+ """
+ if AIRFLOW_V_3_3_PLUS:
+ return
Review Comment:
Awesome, thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
