Let Nodes() return the list of all machines

The autoscaler expects provider implementations nodeGroups to implement the Nodes() function to return the number of instances belonging to the group regardless of they have become a kubernetes node or not.
This information is then used for instance to realise about unregistered nodes https://github.com/kubernetes/autoscaler/blob/bf3a9fb52e3214dff0bea5ef2b97f17ad00a7702/cluster-autoscaler/clusterstate/clusterstate.go#L307-L311

Author: enxebre

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go

@@ -336,25 +336,22 @@ func (c *machineController) machineSetProviderIDs(machineSet *MachineSet) ([]str
 		return nil, fmt.Errorf("error listing machines: %v", err)
 	}
 
-	var nodes []string
-
+	var providerIDs []string
 	for _, machine := range machines {
+		if machine.Spec.ProviderID == nil || *machine.Spec.ProviderID == "" {
+			klog.Warningf("Machine %q has no providerID", machine.Name)
+		}
+
 		if machine.Spec.ProviderID != nil && *machine.Spec.ProviderID != "" {
-			// Prefer machine<=>node mapping using ProviderID
-			node, err := c.findNodeByProviderID(*machine.Spec.ProviderID)
-			if err != nil {
-				return nil, err
-			}
-			if node != nil {
-				nodes = append(nodes, node.Spec.ProviderID)
-				continue
-			}
+			providerIDs = append(providerIDs, *machine.Spec.ProviderID)
+			continue
 		}
 
 		if machine.Status.NodeRef == nil {
 			klog.V(4).Infof("Status.NodeRef of machine %q is currently nil", machine.Name)
 			continue
 		}
+
 		if machine.Status.NodeRef.Kind != "Node" {
 			klog.Errorf("Status.NodeRef of machine %q does not reference a node (rather %q)", machine.Name, machine.Status.NodeRef.Kind)
 			continue
@@ -366,13 +363,12 @@ func (c *machineController) machineSetProviderIDs(machineSet *MachineSet) ([]str
 		}
 
 		if node != nil {
-			nodes = append(nodes, node.Spec.ProviderID)
+			providerIDs = append(providerIDs, node.Spec.ProviderID)
 		}
 	}
 
-	klog.V(4).Infof("nodegroup %s has nodes %v", machineSet.Name, nodes)
-
-	return nodes, nil
+	klog.V(4).Infof("nodegroup %s has nodes %v", machineSet.Name, providerIDs)
+	return providerIDs, nil
 }
 
 func (c *machineController) filterAllMachineSets(f machineSetFilterFunc) error {

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_machinedeployment.go

@@ -61,11 +61,11 @@ func (r machineDeploymentScalableResource) Nodes() ([]string, error) {
 
 	if err := r.controller.filterAllMachineSets(func(machineSet *MachineSet) error {
 		if machineSetIsOwnedByMachineDeployment(machineSet, r.machineDeployment) {
-			names, err := r.controller.machineSetNodeNames(machineSet)
+			providerIDs, err := r.controller.machineSetProviderIDs(machineSet)
 			if err != nil {
 				return err
 			}
-			result = append(result, names...)
+			result = append(result, providerIDs...)
 		}
 		return nil
 	}); err != nil {

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_machineset.go

@@ -57,7 +57,7 @@ func (r machineSetScalableResource) Namespace() string {
 }
 
 func (r machineSetScalableResource) Nodes() ([]string, error) {
-	return r.controller.machineSetNodeNames(r.machineSet)
+	return r.controller.machineSetProviderIDs(r.machineSet)
 }
 
 func (r machineSetScalableResource) Replicas() int32 {

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go

@@ -179,6 +179,7 @@ func (ng *nodegroup) Debug() string {
 }
 
 // Nodes returns a list of all nodes that belong to this node group.
+// This includes instances that might have not become a kubernetes node yet.
 func (ng *nodegroup) Nodes() ([]cloudprovider.Instance, error) {
 	nodes, err := ng.scalableResource.Nodes()
 	if err != nil {

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go

@@ -401,10 +401,12 @@ func TestNodeGroupIncreaseSize(t *testing.T) {
 
 func TestNodeGroupDecreaseTargetSize(t *testing.T) {
 	type testCase struct {
-		description string
-		delta       int
-		initial     int32
-		expected    int32
+		description         string
+		delta               int
+		initial             int32
+		targetSizeIncrement int32
+		expected            int32
+		expectedError       bool
 	}
 
 	test := func(t *testing.T, tc *testCase, testConfig *testConfig) {
@@ -421,17 +423,49 @@ func TestNodeGroupDecreaseTargetSize(t *testing.T) {
 		}
 
 		ng := nodegroups[0]
-		currReplicas, err := ng.TargetSize()
-			if err := controller.machineSetInformer.Informer().GetStore().Add(newUnstructuredFromMachineSet(testConfig.machineSet)); err != nil {
-		}
+		// DecreaseTargetSize should only decrease the size when the current target size of the nodeGroup
+		// is bigger than the number existing instances for that group. We force such a scenario with targetSizeIncrement.
+		switch v := (ng.scalableResource).(type) {
+		case *machineSetScalableResource:
+			testConfig.machineSet.Spec.Replicas = int32ptr(*testConfig.machineSet.Spec.Replicas + tc.targetSizeIncrement)
+			ms, err := ng.machineapiClient.MachineSets(ng.Namespace()).Update(testConfig.machineSet)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
 
-		if err := controller.nodeInformer.GetStore().Delete(testConfig.nodes[0]); err != nil {
-			if err := controller.machineDeploymentInformer.Informer().GetStore().Add(newUnstructuredFromMachineDeployment(testConfig.machineDeployment)); err != nil {
+			if err := controller.machineSetInformer.Informer().GetStore().Add(ms); err != nil {
+				t.Fatalf("failed to add new machine: %v", err)
+			}
+		case *machineDeploymentScalableResource:
+			testConfig.machineDeployment.Spec.Replicas = int32ptr(*testConfig.machineDeployment.Spec.Replicas + tc.targetSizeIncrement)
+			md, err := ng.machineapiClient.MachineDeployments(ng.Namespace()).Update(testConfig.machineDeployment)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if err := controller.machineDeploymentInformer.Informer().GetStore().Add(md); err != nil {
+				t.Fatalf("failed to add new machine: %v", err)
+			}
+		default:
+			t.Errorf("unexpected type: %T", v)
 		}
+		// A nodegroup is immutable; get a fresh copy after adding targetSizeIncrement.
+		nodegroups, err = controller.nodeGroups()
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		ng = nodegroups[0]
 
-		if err := ng.DecreaseTargetSize(tc.delta); err != nil {
+		currReplicas, err := ng.TargetSize()
+		if err != nil {
 			t.Fatalf("unexpected error: %v", err)
 		}
+		if currReplicas != int(tc.initial)+int(tc.targetSizeIncrement) {
+			t.Errorf("initially expected %v, got %v", tc.initial, currReplicas)
+		}
+
+		if err := ng.DecreaseTargetSize(tc.delta); (err != nil) != tc.expectedError {
+			t.Fatalf("expected error: %v, got: %v", tc.expectedError, err)
+		}
 
 		switch v := (ng.scalableResource).(type) {
 		case *machineSetScalableResource:
@@ -464,20 +498,35 @@ func TestNodeGroupDecreaseTargetSize(t *testing.T) {
 
 	t.Run("MachineSet", func(t *testing.T) {
 		tc := testCase{
-			description: "decrease by 1",
-			initial:     3,
-			expected:    2,
-			delta:       -1,
+			description:         "Same number of existing instances and node group target size should error",
+			initial:             3,
+			targetSizeIncrement: 0,
+			expected:            3,
+			delta:               -1,
+			expectedError:       true,
+		}
+		test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations))
+	})
+
+	t.Run("MachineSet", func(t *testing.T) {
+		tc := testCase{
+			description:         "A node group with targe size 4 but only 3 existing instances should decrease by 1",
+			initial:             3,
+			targetSizeIncrement: 1,
+			expected:            3,
+			delta:               -1,
 		}
 		test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations))
 	})
 
 	t.Run("MachineDeployment", func(t *testing.T) {
 		tc := testCase{
-			description: "decrease by 1",
-			initial:     3,
-			expected:    2,
-			delta:       -1,
+			description:         "Same number of existing instances and node group target size should error",
+			initial:             3,
+			targetSizeIncrement: 0,
+			expected:            3,
+			delta:               -1,
+			expectedError:       true,
 		}
 		test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(tc.initial), annotations))
 	})

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_scalableresource.go

@@ -34,7 +34,7 @@ type scalableResource interface {
 	// Namespace returns the namespace the resource is in
 	Namespace() string
 
-	// Nodes returns a list of all nodes that belong to this
+	// Nodes returns a list of all machines that already have or should become nodes that belong to this
 	// resource
 	Nodes() ([]string, error)