jedcunningham commented on code in PR #70595:
URL: https://github.com/apache/airflow/pull/70595#discussion_r3666354846
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/cli/definition.py:
##########
@@ -68,6 +68,19 @@
),
)
+ARG_MIN_COMPLETED_MINUTES = Arg(
+ ("--min-completed-minutes",),
+ default=0,
+ type=positive_int(allow_zero=True),
+ help=(
+ "Minimum age in minutes of a completed (Succeeded/Failed/Evicted) pod
before it is deleted. "
+ "Defaults to 0 (delete immediately, preserving current behaviour). "
+ "Set this to a positive value to prevent a race condition where the
cleanup job removes a "
+ "just-completed pod before KubernetesPodOperator has polled its
terminal phase, "
+ "causing a spurious task failure despite the pod having succeeded."
Review Comment:
We can probably shorten or remove this completely.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/cli/definition.py:
##########
@@ -68,6 +68,19 @@
),
)
+ARG_MIN_COMPLETED_MINUTES = Arg(
+ ("--min-completed-minutes",),
+ default=0,
+ type=positive_int(allow_zero=True),
+ help=(
+ "Minimum age in minutes of a completed (Succeeded/Failed/Evicted) pod
before it is deleted. "
+ "Defaults to 0 (delete immediately, preserving current behaviour). "
Review Comment:
Lets set a default age to avoid this race. No reason to maintain a race
condition :)
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/cli/kubernetes_command.py:
##########
@@ -119,6 +119,21 @@ def generate_pod_yaml(args):
print(f"YAML output can be found at {yaml_output_path}")
+def _get_pod_completion_time(pod):
+ """
+ Return the time the pod entered a terminal state, or its creation time as
fallback.
+
+ Uses the latest ``finished_at`` timestamp across all container statuses so
that pods
+ with multiple containers (e.g. an init container + a base container) are
judged by
+ the time the *last* container finished, not by when the pod was created.
+ """
+ times = []
+ for status in pod.status.container_statuses or []:
Review Comment:
Init container statuses live in a separate field,
`pod.status.init_container_statuses`, so they're never seen.
That matters because of the fallback direction. When an init container
fails, the pod goes Failed/restartPolicy: Never, but the base container never
starts, so it has no terminated state and times comes back empty. Then we
return creation_timestamp, which is always earlier than real completion, so the
age comes out inflated and the pod is deleted immediately. Same for pods
evicted before containers started. In other words the guard silently no-ops in
some of the exact terminal states the flag advertises, and it fails in the
unsafe direction (delete too early) rather than the safe one.
Two changes:
1. Scan init container statuses too:
`statuses = [*(pod.status.container_statuses or []),
*(pod.status.init_container_statuses or [])]`
2. Pick a fallback that isn't strictly older than completion.
`max(c.last_transition_time for c in pod.status.conditions)` is at-or-after the
terminal transition (the Ready condition flips on eviction) and is always
present for a scheduled pod.
Also, no test currently covers either behavior the helper exists for — every
case builds exactly one container status, so `max()` over multiple is
unexercised, and because _make_pod uses a bare `MagicMock()`, `status.state`
and `status.state.terminated` are auto-truthy and the None guards can't be
exercised as false. A small parametrized unit test on
`_get_pod_completion_time` with real `k8s.V1ContainerStatus` objects would
cover both plus the fallback. Or, cover it with more comprehensive coverage of
the cli command directly would work too.
--
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]
