SameerMesiah97 commented on code in PR #62401:
URL: https://github.com/apache/airflow/pull/62401#discussion_r2850621943
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/triggers/pod.py:
##########
@@ -334,6 +360,109 @@ def hook(self) -> AsyncKubernetesHook:
def pod_manager(self) -> AsyncPodManager:
return AsyncPodManager(async_hook=self.hook)
+ if not AIRFLOW_V_3_0_PLUS:
+
+ @provide_session
+ def get_task_instance(self, session: Session) -> TaskInstance:
+ """Get the task instance for this trigger from the database
(Airflow 2.x only)."""
+ task_instance = session.scalar(
+ select(TaskInstance).where(
+ TaskInstance.dag_id == self.task_instance.dag_id,
+ TaskInstance.task_id == self.task_instance.task_id,
+ TaskInstance.run_id == self.task_instance.run_id,
+ TaskInstance.map_index == self.task_instance.map_index,
+ )
+ )
Review Comment:
The Airflow 3.x path is fine but is this session here async safe? This looks
like it is doing a sync call to metadata db which is blocking. Considering that
this is farily narrow query, it is not too egregious and we do have other
triggers that are not 100% async safe but is there no way to avoid this? I see
that further down you used `sync_to_async` for 3.x. Why not for legacy?
##########
providers/cncf/kubernetes/docs/operators.rst:
##########
@@ -155,6 +155,37 @@ Example to fetch and display container log periodically
:end-before: [END howto_operator_async_log]
+Pod cleanup on kill
+^^^^^^^^^^^^^^^^^^^
+
+The ``cancel_on_kill`` parameter controls whether the Kubernetes pod is
deleted when a
+running task is killed (e.g. manually marked as success or failed from the
Airflow UI).
+
+In **sync mode**, ``cancel_on_kill`` gates the ``on_kill`` callback: when
``True`` (the
+default), killing the task deletes the pod; when ``False``, the pod is left
running.
+
+In **deferrable mode**, ``cancel_on_kill`` is forwarded to the trigger. When
the trigger
+is cancelled it will clean up the pod according to the ``on_finish_action``
setting:
+
+- ``on_finish_action="delete_pod"`` (default): the pod is deleted
+- ``on_finish_action="keep_pod"``: the pod is left running
+- ``on_finish_action="delete_succeeded_pod"``: the pod is left running
Review Comment:
Add this:
``on_finish_action="delete_active_pod"``: the pod is deleted if it is in
state 'PENDING' or 'RUNNING;.
And for `delete_succeeded_pod`, you shoud specify the pod is deleted if it
is in state 'SUCCEEDED'.
##########
providers/cncf/kubernetes/docs/operators.rst:
##########
@@ -155,6 +155,37 @@ Example to fetch and display container log periodically
:end-before: [END howto_operator_async_log]
+Pod cleanup on kill
+^^^^^^^^^^^^^^^^^^^
+
+The ``cancel_on_kill`` parameter controls whether the Kubernetes pod is
deleted when a
+running task is killed (e.g. manually marked as success or failed from the
Airflow UI).
+
+In **sync mode**, ``cancel_on_kill`` gates the ``on_kill`` callback: when
``True`` (the
+default), killing the task deletes the pod; when ``False``, the pod is left
running.
+
+In **deferrable mode**, ``cancel_on_kill`` is forwarded to the trigger. When
the trigger
+is cancelled it will clean up the pod according to the ``on_finish_action``
setting:
Review Comment:
Could you clarify why you are deferring to `on_finish_action` in deferrable
mode but subverting it in sync mode?
--
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]
