shahar1 commented on code in PR #76351:
URL: https://github.com/apache/airflow/pull/76351#discussion_r4495252631
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/triggers/pod.py:
##########
@@ -392,6 +393,25 @@ async def _wait_for_container_completion(self) ->
TriggerEvent:
)
pod = await self._get_pod()
pod_container_state = self.define_pod_container_state(pod)
+ if pod_container_state == ContainerState.UNDEFINED and not
self.has_base_container_in_spec(pod):
+ # Polling cannot make a container the pod never declares
appear, and
+ # ``_execution_deadline`` is only set when the task sets
``execution_timeout``,
+ # so without this the loop would keep polling until the pod
reaches a
+ # terminal phase.
+ return TriggerEvent(
Review Comment:
**major:** this event does not end the task while the pod's declared
containers keep running.
`trigger_reentry` raises on `failed`, but its `finally` first runs `_clean`
(`operators/pod.py:1288`), and `_clean` only skips `await_pod_completion` for
`timeout` (`operators/pod.py:1300`). `await_pod_completion` then polls until
the pod reaches a terminal phase. A base container that doesn't exist can never
satisfy its `container_is_completed` exit, and
`detect_pod_terminate_early_issues` returns `None` for a Running pod.
I ran the real `trigger_reentry` with this event, against a Running pod
whose only container is `sidecar` and with `time.sleep` patched. It logged the
error, then was still polling after 50 iterations with the `AirflowException`
pending. So unless `execution_timeout` interrupts it, the task stays running
and holds a worker slot until the pod exits. A long-running container means it
may never fail. That is the same wait the comment above describes, now in the
worker instead of the triggerer.
This is not a regression, because today's `error` event takes the same
`_clean` path. But it means the change doesn't yet deliver "instead of waiting
for a container that will never appear" from the description.
Suggestion: have `_clean` skip the wait when the base container isn't
declared, as it already does for `timeout`, and share the spec check with the
trigger. `cleanup` then applies `on_finish_action` and fails the task. Please
also add an operator test that this event on a Running pod doesn't call
`await_pod_completion`. The new tests stop at the emitted event, and the
project rule is to "Flag any changed or added behaviour without a corresponding
test".
--
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]
