From 89da7342ec90ddbfb13bb613a87afd80c8475eb7 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Thu, 20 Aug 2026 11:15:56 -0700 Subject: [PATCH] Fix LoadAffinity mutation accumulating OS node selector terms (#10342) * Fix LoadAffinity mutation accumulating OS node selector terms The node-agent parses the loadAffinity configuration once at startup and keeps it in memory. GetLoadAffinityByStorageClass returned a pointer to one of the elements of that cached list rather than a copy, so the exposers, which append a kubernetes.io/os match expression to the returned affinity, were mutating the shared configuration. Every DataUpload or DataDownload appended another OS term, growing the data mover pod spec until it could eventually exceed the object size limit. Return a deep copy from GetLoadAffinityByStorageClass so that callers can safely modify the result. A shallow copy is not enough because the MatchExpressions slice header would still be shared with the source. Fixes #10341 Signed-off-by: Shubham Pampattiwar * Add changelog Signed-off-by: Shubham Pampattiwar --------- Signed-off-by: Shubham Pampattiwar (cherry picked from commit f27a4ad8c0823d5d6617f2450139f36e71551650) Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com> --- .../unreleased/10342-shubham-pampattiwar | 1 + pkg/util/kube/pod.go | 21 ++++- pkg/util/kube/pod_test.go | 79 +++++++++++++++++++ 3 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/10342-shubham-pampattiwar diff --git a/changelogs/unreleased/10342-shubham-pampattiwar b/changelogs/unreleased/10342-shubham-pampattiwar new file mode 100644 index 000000000..adc848948 --- /dev/null +++ b/changelogs/unreleased/10342-shubham-pampattiwar @@ -0,0 +1 @@ +Fix issue #10341, avoid mutating the cached node-agent LoadAffinity so the OS node selector term is not appended repeatedly to data mover pods diff --git a/pkg/util/kube/pod.go b/pkg/util/kube/pod.go index 76af3022f..e117c2234 100644 --- a/pkg/util/kube/pod.go +++ b/pkg/util/kube/pod.go @@ -319,9 +319,26 @@ func ExitPodWithMessage(logger logrus.FieldLogger, succeed bool, message string, funcExit(exitCode) } +// deepCopy returns a deep copy of the LoadAffinity, so that the returned value +// can be safely modified without affecting the source. +func (a *LoadAffinity) deepCopy() *LoadAffinity { + if a == nil { + return nil + } + + result := &LoadAffinity{ + StorageClass: a.StorageClass, + } + a.NodeSelector.DeepCopyInto(&result.NodeSelector) + + return result +} + // GetLoadAffinityByStorageClass retrieves the LoadAffinity from the parameter affinityList. // The function first try to find by the scName. If there is no such LoadAffinity, // it will try to get the LoadAffinity whose StorageClass has no value. +// The returned LoadAffinity is a deep copy of the matched element, so that the +// callers can modify it without corrupting the shared node-agent configuration. func GetLoadAffinityByStorageClass( affinityList []*LoadAffinity, scName string, @@ -332,7 +349,7 @@ func GetLoadAffinityByStorageClass( for _, affinity := range affinityList { if affinity.StorageClass == scName { logger.WithField("StorageClass", scName).Info("Found pod's affinity setting per StorageClass.") - return affinity + return affinity.deepCopy() } if affinity.StorageClass == "" && globalAffinity == nil { @@ -346,5 +363,5 @@ func GetLoadAffinityByStorageClass( logger.Info("No Affinity is found for pod.") } - return globalAffinity + return globalAffinity.deepCopy() } diff --git a/pkg/util/kube/pod_test.go b/pkg/util/kube/pod_test.go index 1d54071c3..2591f5caa 100644 --- a/pkg/util/kube/pod_test.go +++ b/pkg/util/kube/pod_test.go @@ -1545,3 +1545,82 @@ func TestGetLoadAffinityByStorageClass(t *testing.T) { }) } } + +func TestGetLoadAffinityByStorageClassReturnsCopy(t *testing.T) { + newAffinityList := func() []*LoadAffinity { + return []*LoadAffinity{ + { + NodeSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"pool": "backup"}, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: corev1api.LabelArchStable, + Operator: metav1.LabelSelectorOpIn, + Values: []string{"amd64"}, + }, + }, + }, + }, + { + NodeSelector: metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: corev1api.LabelArchStable, + Operator: metav1.LabelSelectorOpIn, + Values: []string{"arm64"}, + }, + }, + }, + StorageClass: "storage-class-01", + }, + } + } + + tests := []struct { + name string + scName string + }{ + { + name: "global affinity", + scName: "no-such-storage-class", + }, + { + name: "affinity matched by StorageClass", + scName: "storage-class-01", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + affinityList := newAffinityList() + + // Simulate the exposers, which append an OS related term to the returned + // affinity on every expose call. The source list must not be affected. + for range 3 { + result := GetLoadAffinityByStorageClass(affinityList, test.scName, velerotest.NewLogger()) + require.NotNil(t, result) + + result.NodeSelector.MatchExpressions = append(result.NodeSelector.MatchExpressions, metav1.LabelSelectorRequirement{ + Key: NodeOSLabel, + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{NodeOSWindows}, + }) + + assert.Len(t, result.NodeSelector.MatchExpressions, 2) + } + + assert.Equal(t, newAffinityList(), affinityList) + + // The other fields must be copied as well. + result := GetLoadAffinityByStorageClass(affinityList, test.scName, velerotest.NewLogger()) + require.NotNil(t, result) + result.StorageClass = "modified" + result.NodeSelector.MatchExpressions[0].Values[0] = "modified" + if result.NodeSelector.MatchLabels != nil { + result.NodeSelector.MatchLabels["pool"] = "modified" + } + + assert.Equal(t, newAffinityList(), affinityList) + }) + } +}