UPSTREAM: <carry>: improve failed machine detection for hcp

This change allows the failed machine logic to be aware of the API
differences between MAPI and CAPI status fields related to failures and
errors. This change is being added to ensure that the autoscaler will
properly detect failed machines regardless of whether it is running on
OCP or HCP.

Author: elmiko

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go

@@ -67,6 +67,8 @@ const (
 	autoDiscovererTypeClusterAPI  = "clusterapi"
 	autoDiscovererClusterNameKey  = "clusterName"
 	autoDiscovererNamespaceKey    = "namespace"
+	CAPIFailedMessageField        = "failureMessage"
+	MAPIFailedMessageField        = "errorMessage"
 )
 
 // machineController watches for Nodes, Machines, MachinePools, MachineSets, and
@@ -625,17 +627,25 @@ func (c *machineController) findScalableResourceProviderIDs(scalableResource *un
 	}
 
 	for _, machine := range machines {
+		// Because the error messages are contained in different fields depending on the API (MAPI or CAPI), we
+		// need to make sure we grab the proper information so that the failure states are evaluated properly.
+		apiVersion := machine.GetAPIVersion()
+		failedMessageField := CAPIFailedMessageField
+		if strings.HasPrefix(apiVersion, openshiftMAPIGroup) {
+			failedMessageField = MAPIFailedMessageField
+		}
+
 		// In some cases it is possible for a machine to have acquired a provider ID from the infrastructure and
 		// then become failed later. We want to ensure that a failed machine is not counted towards the total
 		// number of nodes in the cluster, for this reason we will detect a failed machine first, regardless
 		// of provider ID, and give it a normalized provider ID with failure message prepended.
-		errorMessage, found, err := unstructured.NestedString(machine.UnstructuredContent(), "status", "errorMessage")
+		failureMessage, found, err := unstructured.NestedString(machine.UnstructuredContent(), "status", failedMessageField)
 		if err != nil {
 			return nil, err
 		}
 
 		if found {
-			klog.V(4).Infof("Status.ErrorMessage of machine %q is %q", machine.GetName(), errorMessage)
+			klog.V(4).Infof("status.%s of machine %q is %q", failedMessageField, machine.GetName(), failureMessage)
 			// Provide a fake ID to allow the autoscaler to track machines that will never
 			// become nodes and mark the nodegroup unhealthy after maxNodeProvisionTime.
 			// Fake ID needs to be recognised later and converted into a machine key.

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go

@@ -416,6 +416,7 @@ func TestNodeGroupDecreaseTargetSize(t *testing.T) {
 		includeFailedMachine               bool
 		includeFailedMachineWithProviderID bool
 		includePendingMachine              bool
+		failedMachineIsMAPI                bool
 	}
 
 	test := func(t *testing.T, tc *testCase, testConfig *testConfig) {
@@ -451,7 +452,13 @@ func TestNodeGroupDecreaseTargetSize(t *testing.T) {
 			if !tc.includeFailedMachineWithProviderID {
 				unstructured.RemoveNestedField(machine.Object, "spec", "providerID")
 			}
-			unstructured.SetNestedField(machine.Object, "ErrorMessage", "status", "errorMessage")
+
+			if tc.failedMachineIsMAPI {
+				machine.SetAPIVersion(fmt.Sprintf("%s/v1beta1", openshiftMAPIGroup))
+				unstructured.SetNestedField(machine.Object, "ErrorMessage", "status", "errorMessage")
+			} else {
+				unstructured.SetNestedField(machine.Object, "ErrorMessage", "status", "failureMessage")
+			}
 
 			if err := updateResource(controller.managementClient, controller.machineInformer, controller.machineResource, machine); err != nil {
 				t.Fatalf("unexpected error updating machine, got %v", err)
@@ -648,6 +655,25 @@ func TestNodeGroupDecreaseTargetSize(t *testing.T) {
 			includeFailedMachine:               true,
 			includeFailedMachineWithProviderID: true,
 		},
+		{
+			description:          "A node group with 4 replicas with one failed MAPI machine without a provider ID should decrease by 1",
+			initial:              4,
+			targetSizeIncrement:  0,
+			expected:             3,
+			delta:                -1,
+			includeFailedMachine: true,
+			failedMachineIsMAPI:  true,
+		},
+		{
+			description:                        "A node group with 4 replicas with one failed MAPI machine that has a provider ID should decrease by 1",
+			initial:                            4,
+			targetSizeIncrement:                0,
+			expected:                           3,
+			delta:                              -1,
+			includeFailedMachine:               true,
+			includeFailedMachineWithProviderID: true,
+			failedMachineIsMAPI:                true,
+		},
 	}
 
 	annotations := map[string]string{
@@ -1306,13 +1332,20 @@ func TestNodeGroupDeleteNodesSequential(t *testing.T) {
 }
 
 func TestNodeGroupWithFailedMachine(t *testing.T) {
-	test := func(t *testing.T, testConfig *testConfig) {
-		controller, stop := mustCreateTestController(t, testConfig)
+	type testCase struct {
+		description         string
+		testConfig          *testConfig
+		failedMachineIsMAPI bool
+	}
+
+	test := func(t *testing.T, tc testCase) {
+		controller, stop := mustCreateTestController(t, tc.testConfig)
 		defer stop()
 
 		// Simulate a failed machine
-		machine := testConfig.machines[3].DeepCopy()
+		machine := tc.testConfig.machines[3].DeepCopy()
 
+		machine.SetAPIVersion(fmt.Sprintf("%s/v1beta1", openshiftMAPIGroup))
 		unstructured.RemoveNestedField(machine.Object, "spec", "providerID")
 		unstructured.SetNestedField(machine.Object, "ErrorMessage", "status", "errorMessage")
 
@@ -1335,8 +1368,8 @@ func TestNodeGroupWithFailedMachine(t *testing.T) {
 			t.Fatalf("unexpected error: %v", err)
 		}
 
-		if len(nodeNames) != len(testConfig.nodes) {
-			t.Fatalf("expected len=%v, got len=%v", len(testConfig.nodes), len(nodeNames))
+		if len(nodeNames) != len(tc.testConfig.nodes) {
+			t.Fatalf("expected len=%v, got len=%v", len(tc.testConfig.nodes), len(nodeNames))
 		}
 
 		sort.SliceStable(nodeNames, func(i, j int) bool {
@@ -1360,8 +1393,8 @@ func TestNodeGroupWithFailedMachine(t *testing.T) {
 				nodeIndex = i
 			}
 
-			if nodeNames[i].Id != testConfig.nodes[nodeIndex].Spec.ProviderID {
-				t.Fatalf("expected %q, got %q", testConfig.nodes[nodeIndex].Spec.ProviderID, nodeNames[i].Id)
+			if nodeNames[i].Id != tc.testConfig.nodes[nodeIndex].Spec.ProviderID {
+				t.Fatalf("expected %q, got %q", tc.testConfig.nodes[nodeIndex].Spec.ProviderID, nodeNames[i].Id)
 			}
 		}
 	}
@@ -1370,40 +1403,48 @@ func TestNodeGroupWithFailedMachine(t *testing.T) {
 	// Going beyond 10 will break the sorting that happens in the
 	// test() function because sort.Strings() will not do natural
 	// sorting and the expected semantics in test() will fail.
+	annotations := map[string]string{
+		nodeGroupMinSizeAnnotationKey: "1",
+		nodeGroupMaxSizeAnnotationKey: "10",
+	}
 
-	t.Run("MachineSet", func(t *testing.T) {
-		test(
-			t,
-			createMachineSetTestConfig(
+	testCases := []testCase{
+		{
+			description: "one failed machine with MAPI api group",
+			testConfig: createMachineSetTestConfig(
 				RandomString(6),
 				RandomString(6),
 				RandomString(6),
 				10,
-				map[string]string{
-					nodeGroupMinSizeAnnotationKey: "1",
-					nodeGroupMaxSizeAnnotationKey: "10",
-				},
+				annotations,
 				nil,
 			),
-		)
-	})
-
-	t.Run("MachineDeployment", func(t *testing.T) {
-		test(
-			t,
-			createMachineDeploymentTestConfig(
+			failedMachineIsMAPI: true,
+		},
+		{
+			description: "one failed machine with CAPI api group",
+			testConfig: createMachineSetTestConfig(
 				RandomString(6),
 				RandomString(6),
 				RandomString(6),
 				10,
-				map[string]string{
-					nodeGroupMinSizeAnnotationKey: "1",
-					nodeGroupMaxSizeAnnotationKey: "10",
-				},
+				annotations,
 				nil,
 			),
-		)
-	})
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(fmt.Sprintf("MachineSet/%s", tc.description), func(t *testing.T) {
+			test(t, tc)
+		})
+	}
+
+	for _, tc := range testCases {
+		t.Run(fmt.Sprintf("MachineDeployment/%s", tc.description), func(t *testing.T) {
+			test(t, tc)
+		})
+	}
 }
 
 func TestNodeGroupTemplateNodeInfo(t *testing.T) {