Skip to content

Consider XCOM sidecar container during pod cleanup#64962

Merged
jscheffl merged 7 commits into
apache:mainfrom
boschglobal:feature/consider-xcom-sidecar-container-during-cleanup
Apr 15, 2026
Merged

Consider XCOM sidecar container during pod cleanup#64962
jscheffl merged 7 commits into
apache:mainfrom
boschglobal:feature/consider-xcom-sidecar-container-during-cleanup

Conversation

@wolfdn

@wolfdn wolfdn commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Problem

When using do_xcom_push=True with the KubernetesPodOperator, an xcom sidecar container is injected into the pod to facilitate XCom value transfer. If communication with the Kubernetes API fails during cleanup (e.g. the kill command for the sidecar cannot be sent), the xcom sidecar container may remain running even after the base container has completed.

In this scenario, await_pod_completion waits indefinitely for the pod to reach a terminal phase — which never happens because the still-running xcom sidecar keeps the pod in the Running phase.

Additionally, the cleanup method determines task success/failure based on the overall pod phase rather than the base container's status. When the xcom sidecar prevents the pod from reaching Succeeded, a task whose base container completed successfully is incorrectly marked as failed.

Solution

This change extends the existing istio sidecar handling to also cover the xcom sidecar container:

  1. await_pod_completion (pod_manager.py): When do_xcom_push=True, break out of the pod-completion wait loop as soon as the base container has completed — just like the existing behavior for istio sidecars. A log message is emitted to make this early exit visible.

  2. cleanup (pod.py): When do_xcom_push=True, determine task success/failure based on the base container's exit status rather than the overall pod phase. This prevents a lingering xcom sidecar from causing a false failure.

Changes

  • providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py

    • Added do_xcom_push parameter to await_pod_completion
    • Extended the sidecar-aware early exit condition from istio_enabled to istio_enabled or do_xcom_push
    • Added informational log message when breaking out early due to a sidecar
  • providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py

    • Pass self.do_xcom_push to await_pod_completion in _clean
    • Refactored failed determination in cleanup to check base container status when do_xcom_push is enabled

Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)
    GitHub Copilot - Claude Opus 4.6

  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

@boring-cyborg boring-cyborg Bot added area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues labels Apr 9, 2026

@jscheffl jscheffl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation looks good but some tests are failing.

@wolfdn

wolfdn commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

Implementation looks good but some tests are failing.

Just fixed the failing unit tests.

@kaxil
kaxil requested a review from Copilot April 10, 2026 19:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates KubernetesPodOperator pod monitoring/cleanup to better handle the XCom sidecar case where the base container completes but the pod remains in Running, preventing indefinite waits and avoiding incorrect task failure decisions.

Changes:

  • Extend PodManager.await_pod_completion to optionally stop waiting once the base container completes when an XCom sidecar is expected.
  • Update deferrable operator cleanup to pass do_xcom_push into pod completion waiting and to use base container status for success/failure when sidecars are involved.
  • Add unit tests covering the new XCom-sidecar behavior for both await_pod_completion and cleanup.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py Adds do_xcom_push flag to allow early exit when base container completes but pod phase stays non-terminal due to a sidecar.
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py Threads do_xcom_push into the deferrable cleanup wait path and refactors failure detection for sidecar scenarios.
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/utils/test_pod_manager.py Adds tests validating early-exit behavior for XCom sidecar vs. no-sidecar behavior.
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py Adds a cleanup test asserting task outcome is based on base container exit code when an XCom sidecar keeps the pod running.

Comment thread providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py Outdated
Comment thread providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py Outdated
@jscheffl

Copy link
Copy Markdown
Contributor

No luck with CI - can you apply some more fixes to tests?

@jscheffl jscheffl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@jscheffl
jscheffl merged commit da87130 into apache:main Apr 15, 2026
111 checks passed
karenbraganz pushed a commit to karenbraganz/airflow that referenced this pull request Apr 16, 2026
* Consider XCOM sidecar container during pod cleanup

* Fix failing unit tests

* Apply Copilot review findings

* Implement Copilot review finding regarding failed status

* Fix some more unit tests

* More fixes for unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants