diff --git a/changelogs/unreleased/10348-shubham-pampattiwar b/changelogs/unreleased/10348-shubham-pampattiwar new file mode 100644 index 000000000..adc848948 --- /dev/null +++ b/changelogs/unreleased/10348-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) + }) + } +}