yssxr opened a new pull request, #71452:
URL: https://github.com/apache/airflow/pull/71452
A `KubernetesPodOperator` pod that runs a sidecar alongside `base` never
finishes. The base
container exits, the sidecar keeps running, the pod stays in `Running`, and
the task waits on
it forever.
`await_pod_completion` already knew how to stop waiting on the base
container, but only when
istio or the XCom sidecar was involved. Any other sidecar fell through to
waiting on the pod
phase.
closes: #39693
### The second site
Fixing the wait loop alone makes things worse, so this touches two places.
`cleanup()` decided
success from the pod phase under the same `istio_enabled or do_xcom_push`
condition, so a pod
handed back in `Running` would have been read as failed — trading the hang
for a spurious
failure.
`istio_enabled` and `do_xcom_push` turn out to be two spellings of one
thing: there is a
sidecar, so the base container is what matters. `is_istio_enabled` is itself
just a check for
an extra container named `istio-proxy`. So the wait loop now asks that
question directly via
`has_sidecar_containers`, and istio and the XCom sidecar fall out as
ordinary cases.
Worth noting the deferrable path already worked this way — the triggerer's
`define_container_state` resolves state from `base_container_name` and stops
waiting when it
terminates. The synchronous path was the odd one out.
### What deliberately did not change
`cleanup()` is narrowed to pods that never reached a terminal phase rather
than to all
multi-container pods. Judging every sidecar pod by its base container would
also mean a
*failing* sidecar stops failing the task, which is a much bigger semantic
change than this bug
needs. Pods that do reach `Succeeded`/`Failed` keep exactly the outcome they
have today.
Init containers are reported under `spec.init_containers`, so they never
count as sidecars.
### Tests
- `test_await_pod_completion_breaks_on_user_sidecar_container_completed` —
the reported
scenario. Fails without the change (loops until it runs out of pods to
read).
- `test_cleanup_with_user_sidecar_uses_base_container_status` — mirrors the
existing XCom
sidecar test. `base-succeeded` fails without the change; `base-failed` is
the guard that the
fix does not over-correct into passing genuinely failed tasks.
- `test_has_sidecar_containers` — the helper, including init containers and
the empty/None
guards.
- `test_await_pod_completion_waits_for_pod_phase_without_sidecars` from
#64962 is unchanged and
still passes; a pod with only `base` still waits for the pod phase.
One fixture fix came along with this: `pod_factory` built container specs
with
`mock.MagicMock(name=container_name)`, where `name` is consumed by the
MagicMock constructor
rather than set as an attribute, so `spec.containers[*].name` read back as a
child mock instead
of the container name. Nothing depended on it before, but the sidecar check
does.
Ran the full `providers/cncf/kubernetes` unit suite plus the two dependent
providers that import
these modules (amazon `test_eks.py`, google `test_kubernetes_engine.py`) —
no regressions.
Happy to add a `docs/changelog.rst` entry if the behaviour change is worth
calling out; read it
as a routine bug fix, so left it out per the providers changelog guidance.
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Claude Code (Opus 5)
Generated-by: Claude Code (Opus 5) following [the
guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
--
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]
