jscheffl commented on code in PR #65741:
URL: https://github.com/apache/airflow/pull/65741#discussion_r3140111143
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/triggers/pod.py:
##########
@@ -411,17 +414,52 @@ 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.
+ Cancel 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.
+
+ On Airflow 3.3.0+ the triggerer invokes this for user-initiated kills.
On older versions this
+ is a no-op; use ``cleanup()`` and ``safe_to_cancel()`` instead.
+ """
+ if not AIRFLOW_V_3_3_PLUS:
+ return
Review Comment:
I think this is not needed? This is never called when Airflow < 3.3.0
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/triggers/test_pod.py:
##########
@@ -561,7 +562,8 @@ async def test__get_pod_retries(
@pytest.mark.asyncio
@mock.patch(f"{TRIGGER_PATH}.hook")
- async def test_cleanup_does_not_delete_when_fired_event(self, mock_hook):
+ @mock.patch(f"{POD_TRIGGER_MODULE}.AIRFLOW_V_3_3_PLUS", True)
+ async def test_on_kill_does_not_delete_when_fired_event(self, mock_hook):
Review Comment:
We have back-compat tests and CI runs them on all (supported) Airflow
versions. Can you rather model it with `skipif=AIRFLOW_V_x_y_PLUS`? Then tests
are running automatically only in respective environment and no additional test
complexity needed.
--
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]
