Re: [PR] Add cancel_on_kill and safe_to_cancel support to KubernetesPodOperator and trigger [airflow]

["manipatnam (via GitHub)" <gi...@apache.org>]

manipatnam commented on code in PR #62401:
URL: https://github.com/apache/airflow/pull/62401#discussion_r2876053980


##########
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:
   Since I modified the flow I think we no longer need to add this
   
   New flow:
   In **deferrable mode**, ``cancel_on_kill`` is forwarded to the trigger. When the trigger is cancelled (e.g. the deferred task is manually marked as success or failed), the pod is deleted. The ``on_finish_action`` parameter is **not** consulted during a kill -- it only governs cleanup after normal task completion.



##########
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:
   There's no constraint that requires different behaviour between modes. You're right that it should be symmetric.
   
   I've updated the implementation so both modes are now consistent: `cancel_on_kill` is the sole control for kill behaviour, and `on_finish_action` is not consulted during a kill in either mode. It only governs cleanup after normal task completion.
   
   So the behaviour is now:
   Kill path (sync `on_kill()` / deferrable `cleanup()`): controlled exclusively by `cancel_on_kill`. When True, the pod is always deleted regardless of `on_finish_action`. When False, the pod is left running.
   Normal completion path: controlled by `on_finish_action` as before (delete_pod, keep_pod, delete_succeeded_pod, delete_active_pod).
   This gives a clean separation -- `cancel_on_kill` for kills, `on_finish_action` for normal completion.



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org