noamst-monday commented on PR #70595:
URL: https://github.com/apache/airflow/pull/70595#issuecomment-5116036330
**Note on evicted pod behaviour change**
With `default=1` and the conditions fallback in `_get_pod_completion_time`,
evicted pods are now also subject to the min-age check. Previously they were
deleted immediately. The conditions fallback returns an accurate eviction time
(`max(conditions[*].last_transition_time)`, which kubelet updates at eviction),
so the guard works correctly — an evicted pod is held for
`min_completed_minutes` then cleaned up.
This is technically a behaviour change for evicted pods: they now wait 1
minute (by default) instead of being deleted immediately. Since evicted pods
don't cause the KPO polling race (KPO observes the `Failed` phase directly),
there is no correctness benefit to the delay for them — it is just a side
effect of applying a uniform guard.
If this is undesirable, an alternative is to separate evicted pods out of
the age check entirely (Option A):
```python
is_evicted = pod_reason == pod_reason_evicted
is_terminal = pod_phase == pod_succeeded or (
pod_phase == pod_failed and pod_restart_policy ==
pod_restart_policy_never
)
terminal_old_enough = is_terminal and (
min_completed_minutes == 0
or current_time - _get_pod_completion_time(pod) >
timedelta(minutes=min_completed_minutes)
)
if (
is_evicted # always immediate — no KPO race, no reliable
finished_at
or terminal_old_enough
or (pod_phase == pod_pending and ...)
):
```
Happy to apply that if preferred. The current implementation is simpler and
the 1-minute delay for evicted pods is harmless, but wanted to flag the
difference explicitly.
--
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]
