Skip to content

Fix pod mutation bug in add_xcom_sidecar and add unit tests for Kubernetes xcom sidecar helper - #68786

Closed
kesem0811 wants to merge 1 commit into
apache:mainfrom
kesem0811:add-kubernetes-xcom-sidecar-unit-tests
Closed

kesem0811 wants to merge 1 commit into
apache:mainfrom
kesem0811:add-kubernetes-xcom-sidecar-unit-tests

Conversation

@kesem0811

@kesem0811 kesem0811 commented Jun 20, 2026 •

Copy link
Copy Markdown
Contributor

This PR fixes a silent bug in add_xcom_sidecar where the deepcopy contract was accidentally broken for pods that already contained volumes.

Previously, add_xcom_sidecar (used by KubernetesPodOperator when do_xcom_push=True to attach the XCom sidecar and shared volume) inadvertently mutated the caller's original pod due to how the volume list was assigned. This PR fixes the mutation issue and adds comprehensive unit tests to ensure add_xcom_sidecar behaves correctly and prevents future regressions.

@boring-cyborg boring-cyborg Bot added area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues labels Jun 20, 2026
@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jun 22, 2026
@potiuk

potiuk commented Aug 2, 2026

Copy link
Copy Markdown
Member

The tests are welcome, but the one-line change in this PR is doing more than the title and description suggest, and I think it deserves to be the headline.

pod_cp = copy.deepcopy(pod)
pod_cp.spec.volumes = pod.spec.volumes or []   # reads the original, not the copy
pod_cp.spec.volumes.insert(0, PodDefaults.VOLUME)

Rebinding to the original's list defeats the deepcopy for that attribute, so the insert mutates the caller's pod:

before: caller pod after add_xcom_sidecar() -> ['XCOM_VOLUME', 'user-volume']
after:  caller pod after add_xcom_sidecar() -> ['user-volume']

add_xcom_sidecar deepcopies specifically so it does not touch its input, and that contract was silently broken for any pod that already had volumes. It stays invisible when there are none, because or [] hands back a fresh list.

Could you retitle and describe this as the bug fix it is, with the tests as supporting work? The description becomes the squash commit message, and as it stands someone bisecting a pod-mutation problem would never find this commit.

Two practical notes, neither about the content.

This branch has drifted a long way behind main — 1047 commits — and is now conflicting, so it needs a rebase before it can go anywhere.

And this and #68788 both edit xcom_sidecar.py and test_xcom_sidecar.py, so whichever merges first will leave the other needing a second rebase. My suggestion is to land this one first: it is small and independently correct, and #68788's larger PodDefaults restructuring then rebases onto it rather than the other way round. I have left a note there too.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

@kesem0811 kesem0811 changed the title Add unit tests for Kubernetes xcom sidecar helper Fix pod mutation bug in add_xcom_sidecar and add unit tests for Kubernetes xcom sidecar helper Aug 3, 2026
@potiuk

potiuk commented Oct 5, 2026

Copy link
Copy Markdown
Member

Thanks for finding and fixing this! The same bug was fixed in #72522, merged on 2026-09-09: it changes only the deep copy and also copies PodDefaults.VOLUME, with tests in the same test_xcom_sidecar.py. That's why this branch now conflicts with main. Closing as superseded; your fix was spot on.


Drafted-by: Claude Code (Opus 5.5); reviewed by @potiuk before posting

@potiuk potiuk closed this Oct 5, 2026
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 ready for maintainer review Set after triaging when all criteria pass.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants