shahar1 commented on code in PR #76393:
URL: https://github.com/apache/airflow/pull/76393#discussion_r4502239059
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/utils/test_pod_manager.py:
##########
@@ -225,14 +247,17 @@ def
test_detect_pod_terminate_early_issues_returns_none_for_transient_messages(r
total wait. Returning None here keeps the monitoring loop going so those
retries have a chance to succeed.
"""
- pod = _pod_with_waiting_container(reason, message)
+ pod = _build_pod_with_waiting_container(reason, message)
assert detect_pod_terminate_early_issues(pod) is None
[email protected]("init", [False, True], ids=["main_container",
"init_container"])
@pytest.mark.parametrize(
("reason", "message"),
[
("InvalidImageName", "Failed to apply default image tag"),
+ # The image name contains the transient pattern "timeout".
+ ("InvalidImageName", 'Failed to apply default image tag
"timeout-worker:Bad Tag"'),
Review Comment:
**nit**: The `main_container` variant of this case already passes on `main`,
because `FATAL_STATES` was checked before the transient patterns there as well.
The repo's testing rule (AGENTS.md) says:
> Every changed or added behaviour must have a test; every test must fail
without the PR's change.
If it is meant as a guard for the merged condition, keep it and ignore this
nit. Otherwise drop the case, because the `init_container` variants already
cover the new path.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -254,30 +254,31 @@ def detect_pod_terminate_early_issues(pod: V1Pod) -> str
| None:
FATAL_STATES = ["InvalidImageName", "ErrImageNeverPull"]
TRANSIENT_STATES = ["ErrImagePull", "ImagePullBackOff"]
- ERROR_MESSAGE = "Image cannot be pulled, unable to start:
{reason}\n{message}"
+ ERROR_MESSAGE = "Image for container '{name}' cannot be pulled, unable to
start: {reason}\n{message}"
pod_status = pod.status
- if not pod_status.container_statuses:
- return None
-
- for container_status in pod_status.container_statuses:
+ # Init containers first: while one of them is stuck pulling, the main
+ # container only reports ``PodInitializing``, which matches no state below.
+ container_statuses = [
+ *(pod_status.init_container_statuses or []),
Review Comment:
**minor**: native sidecars after start-up
`init_container_statuses` also lists native sidecars, which are init
containers with `restartPolicy: Always`. A sidecar can restart while the pod
runs. So after this change, `detect_pod_terminate_early_issues` can return an
error long after start-up. Two callers run at that point:
`PodManager.await_pod_completion` (`if
detect_pod_terminate_early_issues(remote_pod): break`) and the re-defer guard
in `trigger_reentry`.
Example:
1. A sidecar crashes mid-run.
2. Its restart hits `ImagePullBackOff` with the bare `Back-off pulling image
"..."` message. This happens on a kubelet older than 1.32 after a registry
problem, or with `imagePullPolicy: Always` and an expired pull secret.
3. The base container exits 0.
4. `await_pod_completion` reads the pod while the phase is still `Running`,
sees the sidecar and breaks.
5. Without istio or `do_xcom_push`, `cleanup` sees `pod_phase !=
PodPhase.SUCCEEDED` and raises `Pod ... returned a failure`. The task's work
succeeded.
Before this change the loop kept polling until the phase was `Succeeded`.
Could the init container scan run only during start-up? For example, include
`init_container_statuses` only while the pod's `Initialized` condition is not
`True`. A native sidecar that cannot pull at start-up keeps `Initialized`
false, so the fail-fast case this PR targets still fails fast. A test with a
running pod and a sidecar in back-off would cover it.
--
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]
