CA: stop mutating the result of .TemplateNodeInfo() in SanitizedTemplateNodeInfoFromNodeGroup

SanitizedTemplateNodeInfoFromNodeGroup() calls .TemplateNodeInfo(),
adds deprecated labels to the result, then sanitizes it - which includes
deep-copying.

This commit moves adding the deprecated labels after the sanitization
process, directly to the deep-copied result.

If .TemplateNodeInfo() caches its result internally, and is called
from a CloudProvider-specific goroutine at the same time as
SanitizedTemplateNodeInfoFromNodeGroup(), CA panics because of a
concurrent map read/write. This change removes the race condition.

Author: towca

cluster-autoscaler/simulator/node_info_utils.go

@@ -49,9 +49,12 @@ func SanitizedTemplateNodeInfoFromNodeGroup(nodeGroup nodeGroupTemplateNodeInfoG
 	if err != nil {
 		return nil, errors.ToAutoscalerError(errors.CloudProviderError, err).AddPrefix("failed to obtain template NodeInfo from node group %q: ", nodeGroup.Id())
 	}
-	labels.UpdateDeprecatedLabels(baseNodeInfo.Node().ObjectMeta.Labels)
-
-	return SanitizedTemplateNodeInfoFromNodeInfo(baseNodeInfo, nodeGroup.Id(), daemonsets, true, taintConfig)
+	sanitizedNodeInfo, aErr := SanitizedTemplateNodeInfoFromNodeInfo(baseNodeInfo, nodeGroup.Id(), daemonsets, true, taintConfig)
+	if aErr != nil {
+		return nil, aErr
+	}
+	labels.UpdateDeprecatedLabels(sanitizedNodeInfo.Node().Labels)
+	return sanitizedNodeInfo, nil
 }
 
 // SanitizedTemplateNodeInfoFromNodeInfo returns a template NodeInfo object based on a real example NodeInfo from the cluster. The template is sanitized, and only

cluster-autoscaler/simulator/node_info_utils_test.go

@@ -93,6 +93,11 @@ func TestSanitizedTemplateNodeInfoFromNodeGroup(t *testing.T) {
 	exampleNode.Spec.Taints = []apiv1.Taint{
 		{Key: taints.ToBeDeletedTaint, Value: "2312532423", Effect: apiv1.TaintEffectNoSchedule},
 	}
+	exampleNode.Labels = map[string]string{
+		"custom":                      "label",
+		apiv1.LabelInstanceTypeStable: "some-instance",
+		apiv1.LabelTopologyRegion:     "some-region",
+	}
 
 	for _, tc := range []struct {
 		testName  string
@@ -155,7 +160,7 @@ func TestSanitizedTemplateNodeInfoFromNodeGroup(t *testing.T) {
 			// Pass empty string as nameSuffix so that it's auto-determined from the sanitized templateNodeInfo, because
 			// TemplateNodeInfoFromNodeGroupTemplate randomizes the suffix.
 			// Pass non-empty expectedPods to verify that the set of pods is changed as expected (e.g. DS pods added, non-DS/deleted pods removed).
-			if err := verifyNodeInfoSanitization(tc.nodeGroup.templateNodeInfoResult, templateNodeInfo, tc.wantPods, "template-node-for-"+tc.nodeGroup.id, "", nil); err != nil {
+			if err := verifyNodeInfoSanitization(tc.nodeGroup.templateNodeInfoResult, templateNodeInfo, tc.wantPods, "template-node-for-"+tc.nodeGroup.id, "", true, nil); err != nil {
 				t.Fatalf("TemplateNodeInfoFromExampleNodeInfo(): NodeInfo wasn't properly sanitized: %v", err)
 			}
 		})
@@ -167,6 +172,11 @@ func TestSanitizedTemplateNodeInfoFromNodeInfo(t *testing.T) {
 	exampleNode.Spec.Taints = []apiv1.Taint{
 		{Key: taints.ToBeDeletedTaint, Value: "2312532423", Effect: apiv1.TaintEffectNoSchedule},
 	}
+	exampleNode.Labels = map[string]string{
+		"custom":                      "label",
+		apiv1.LabelInstanceTypeStable: "some-instance",
+		apiv1.LabelTopologyRegion:     "some-region",
+	}
 
 	testCases := []struct {
 		name       string
@@ -317,7 +327,7 @@ func TestSanitizedTemplateNodeInfoFromNodeInfo(t *testing.T) {
 			// Pass empty string as nameSuffix so that it's auto-determined from the sanitized templateNodeInfo, because
 			// TemplateNodeInfoFromExampleNodeInfo randomizes the suffix.
 			// Pass non-empty expectedPods to verify that the set of pods is changed as expected (e.g. DS pods added, non-DS/deleted pods removed).
-			if err := verifyNodeInfoSanitization(exampleNodeInfo, templateNodeInfo, tc.wantPods, "template-node-for-"+nodeGroupId, "", nil); err != nil {
+			if err := verifyNodeInfoSanitization(exampleNodeInfo, templateNodeInfo, tc.wantPods, "template-node-for-"+nodeGroupId, "", false, nil); err != nil {
 				t.Fatalf("TemplateNodeInfoFromExampleNodeInfo(): NodeInfo wasn't properly sanitized: %v", err)
 			}
 		})
@@ -332,6 +342,12 @@ func TestSanitizedNodeInfo(t *testing.T) {
 		{Key: taints.ToBeDeletedTaint, Value: "2312532423", Effect: apiv1.TaintEffectNoSchedule},
 		{Key: "a", Value: "b", Effect: apiv1.TaintEffectNoSchedule},
 	}
+	templateNode.Labels = map[string]string{
+		"custom":                      "label",
+		apiv1.LabelInstanceTypeStable: "some-instance",
+		apiv1.LabelTopologyRegion:     "some-region",
+	}
+
 	pods := []*framework.PodInfo{
 		{Pod: BuildTestPod("p1", 80, 0, WithNodeName(nodeName))},
 		{Pod: BuildTestPod("p2", 80, 0, WithNodeName(nodeName))},
@@ -346,7 +362,7 @@ func TestSanitizedNodeInfo(t *testing.T) {
 	// Verify that the taints are not sanitized (they should be sanitized in the template already).
 	// Verify that the NodeInfo is sanitized using the template Node name as base.
 	initialTaints := templateNodeInfo.Node().Spec.Taints
-	if err := verifyNodeInfoSanitization(templateNodeInfo, freshNodeInfo, nil, templateNodeInfo.Node().Name, suffix, initialTaints); err != nil {
+	if err := verifyNodeInfoSanitization(templateNodeInfo, freshNodeInfo, nil, templateNodeInfo.Node().Name, suffix, false, initialTaints); err != nil {
 		t.Fatalf("FreshNodeInfoFromTemplateNodeInfo(): NodeInfo wasn't properly sanitized: %v", err)
 	}
 }
@@ -357,9 +373,11 @@ func TestCreateSanitizedNodeInfo(t *testing.T) {
 
 	labelsNode := basicNode.DeepCopy()
 	labelsNode.Labels = map[string]string{
-		apiv1.LabelHostname: oldNodeName,
-		"a":                 "b",
-		"x":                 "y",
+		apiv1.LabelHostname:           oldNodeName,
+		"a":                           "b",
+		"x":                           "y",
+		apiv1.LabelInstanceTypeStable: "some-instance",
+		apiv1.LabelTopologyRegion:     "some-region",
 	}
 
 	taintsNode := basicNode.DeepCopy()
@@ -491,7 +509,7 @@ func TestCreateSanitizedNodeInfo(t *testing.T) {
 			if err != nil {
 				t.Fatalf("sanitizeNodeInfo(): want nil error, got %v", err)
 			}
-			if err := verifyNodeInfoSanitization(tc.nodeInfo, nodeInfo, nil, newNameBase, suffix, tc.wantTaints); err != nil {
+			if err := verifyNodeInfoSanitization(tc.nodeInfo, nodeInfo, nil, newNameBase, suffix, false, tc.wantTaints); err != nil {
 				t.Fatalf("sanitizeNodeInfo(): NodeInfo wasn't properly sanitized: %v", err)
 			}
 		})
@@ -506,7 +524,7 @@ func TestCreateSanitizedNodeInfo(t *testing.T) {
 //
 // If expectedPods is nil, the set of pods is expected not to change between initialNodeInfo and sanitizedNodeInfo. If the sanitization is
 // expected to change the set of pods, the expected set should be passed to expectedPods.
-func verifyNodeInfoSanitization(initialNodeInfo, sanitizedNodeInfo *framework.NodeInfo, expectedPods []*apiv1.Pod, nameBase, nameSuffix string, wantTaints []apiv1.Taint) error {
+func verifyNodeInfoSanitization(initialNodeInfo, sanitizedNodeInfo *framework.NodeInfo, expectedPods []*apiv1.Pod, nameBase, nameSuffix string, wantDeprecatedLabels bool, wantTaints []apiv1.Taint) error {
 	if nameSuffix == "" {
 		// Determine the suffix from the provided sanitized NodeInfo - it should be the last part of a dash-separated name.
 		nameParts := strings.Split(sanitizedNodeInfo.Node().Name, "-")
@@ -526,7 +544,7 @@ func verifyNodeInfoSanitization(initialNodeInfo, sanitizedNodeInfo *framework.No
 
 	// Verification below assumes the same set of pods between initialNodeInfo and sanitizedNodeInfo.
 	wantNodeName := fmt.Sprintf("%s-%s", nameBase, nameSuffix)
-	if err := verifySanitizedNode(initialNodeInfo.Node(), sanitizedNodeInfo.Node(), wantNodeName, wantTaints); err != nil {
+	if err := verifySanitizedNode(initialNodeInfo.Node(), sanitizedNodeInfo.Node(), wantNodeName, wantDeprecatedLabels, wantTaints); err != nil {
 		return err
 	}
 	if err := verifySanitizedNodeResourceSlices(initialNodeInfo.LocalResourceSlices, sanitizedNodeInfo.LocalResourceSlices, nameSuffix); err != nil {
@@ -539,7 +557,7 @@ func verifyNodeInfoSanitization(initialNodeInfo, sanitizedNodeInfo *framework.No
 	return nil
 }
 
-func verifySanitizedNode(initialNode, sanitizedNode *apiv1.Node, wantNodeName string, wantTaints []apiv1.Taint) error {
+func verifySanitizedNode(initialNode, sanitizedNode *apiv1.Node, wantNodeName string, wantDeprecatedLabels bool, wantTaints []apiv1.Taint) error {
 	if gotName := sanitizedNode.Name; gotName != wantNodeName {
 		return fmt.Errorf("want sanitized Node name %q, got %q", wantNodeName, gotName)
 	}
@@ -552,6 +570,9 @@ func verifySanitizedNode(initialNode, sanitizedNode *apiv1.Node, wantNodeName st
 		wantLabels[k] = v
 	}
 	wantLabels[apiv1.LabelHostname] = wantNodeName
+	if wantDeprecatedLabels {
+		labels.UpdateDeprecatedLabels(wantLabels)
+	}
 	if diff := cmp.Diff(wantLabels, sanitizedNode.Labels); diff != "" {
 		return fmt.Errorf("sanitized Node labels unexpected, diff (-want +got): %s", diff)
 	}