UPSTREAM: <carry>: openshift: return no nodegroup when scaling bounds…

… == 0

Change nodeGroupForNode() to return nil when the underlying node group
has no scaling bounds. The underlying MachineSet in these cases is
either missing the following annotations:

```
annotations:
  machine.openshift.io/cluster-api-autoscaler-node-group-min-size: "1"
  machine.openshift.io/cluster-api-autoscaler-node-group-max-size: "6"
```

or they are present but the result of `max-size - min-size` is zero.

This change helps to remove a lot of spurious error messages from the
core of the autoscaler logs at runtime. Without this change we see the
following printed every time the autoscaler scans the state of the
cluster (which defaults to 10s) and can be very misleading.

```console
$ oc logs -f cluster-autoscaler-default-77f884d74b-527bq
E1204 08:26:01.996252       1 utils.go:467] Error while checking node group size worker-us-east-2a: group size not found
E1204 08:26:01.996545       1 utils.go:467] Error while checking node group size worker-us-east-2b: group size not found
E1204 08:26:01.996678       1 utils.go:467] Error while checking node group size worker-us-east-2c: group size not found
```

Those unit tests that were previously relying on nodeGroupForNode() to
always return a node group when no bounds had been specified have been
modified so that they now have a scaling bound >= 1.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1656334

cluster-autoscaler/cloudprovider/openshiftmachineapi/machineapi_controller.go

@@ -406,6 +406,12 @@ func (c *machineController) nodeGroupForNode(node *apiv1.Node) (cloudprovider.No
 			if err != nil {
 				return nil, fmt.Errorf("failed to build nodegroup for node %q: %v", node.Name, err)
 			}
+			// We don't scale from 0 so nodes must belong
+			// to a nodegroup that has a scale size of at
+			// least 1.
+			if nodegroup.MaxSize()-nodegroup.MinSize() < 1 {
+				return nil, nil
+			}
 			return nodegroup, nil
 		}
 	}
@@ -415,6 +421,12 @@ func (c *machineController) nodeGroupForNode(node *apiv1.Node) (cloudprovider.No
 		return nil, fmt.Errorf("failed to build nodegroup for node %q: %v", node.Name, err)
 	}
 
+	// We don't scale from 0 so nodes must belong to a nodegroup
+	// that has a scale size of at least 1.
+	if nodegroup.MaxSize()-nodegroup.MinSize() < 1 {
+		return nil, nil
+	}
+
 	glog.V(4).Infof("node %q is in nodegroup %q", node.Name, machineSet.Name)
 	return nodegroup, nil
 }

cluster-autoscaler/cloudprovider/openshiftmachineapi/machineapi_controller_test.go

@@ -29,6 +29,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	fakekube "k8s.io/client-go/kubernetes/fake"
 )
 
@@ -61,6 +62,50 @@ func mustCreateTestController(t *testing.T, config testControllerConfig) (*machi
 	}
 }
 
+func makeMachineOwner(i int, replicaCount int, annotations map[string]string, ownedByMachineDeployment bool) (*v1beta1.MachineSet, *v1beta1.MachineDeployment) {
+	machineSet := v1beta1.MachineSet{
+		TypeMeta: v1.TypeMeta{
+			Kind: "MachineSet",
+		},
+		ObjectMeta: v1.ObjectMeta{
+			Name:      fmt.Sprintf("machineset-%d", i),
+			Namespace: "test-namespace",
+			UID:       types.UID(fmt.Sprintf("machineset-%d", i)),
+		},
+		Spec: v1beta1.MachineSetSpec{
+			Replicas: int32ptr(int32(replicaCount)),
+		},
+	}
+
+	if !ownedByMachineDeployment {
+		machineSet.ObjectMeta.Annotations = annotations
+		return &machineSet, nil
+	}
+
+	machineDeployment := v1beta1.MachineDeployment{
+		TypeMeta: v1.TypeMeta{
+			Kind: "MachineDeployment",
+		},
+		ObjectMeta: v1.ObjectMeta{
+			Name:        fmt.Sprintf("machinedeployment-%d", i),
+			Namespace:   "test-namespace",
+			UID:         types.UID(fmt.Sprintf("machinedeployment-%d", i)),
+			Annotations: annotations,
+		},
+		Spec: v1beta1.MachineDeploymentSpec{
+			Replicas: int32ptr(int32(replicaCount)),
+		},
+	}
+
+	machineSet.OwnerReferences = append(machineSet.OwnerReferences, v1.OwnerReference{
+		Name: machineDeployment.Name,
+		Kind: machineDeployment.Kind,
+		UID:  machineDeployment.UID,
+	})
+
+	return &machineSet, &machineDeployment
+}
+
 func TestControllerFindMachineByID(t *testing.T) {
 	controller, stop := mustCreateTestController(t, testControllerConfig{})
 	defer stop()
@@ -928,3 +973,98 @@ func TestControllerNodeGroupsWithMachineSets(t *testing.T) {
 		}
 	}
 }
+
+func TestControllerNodeGroupForNodeLookup(t *testing.T) {
+	for i, tc := range []struct {
+		description    string
+		annotations    map[string]string
+		lookupSucceeds bool
+	}{{
+		description:    "lookup is nil because no annotations",
+		annotations:    map[string]string{},
+		lookupSucceeds: false,
+	}, {
+		description: "lookup is nil because scaling range == 0",
+		annotations: map[string]string{
+			nodeGroupMinSizeAnnotationKey: "1",
+			nodeGroupMaxSizeAnnotationKey: "1",
+		},
+		lookupSucceeds: false,
+	}, {
+		description: "lookup is successful because scaling range >= 1",
+		annotations: map[string]string{
+			nodeGroupMinSizeAnnotationKey: "1",
+			nodeGroupMaxSizeAnnotationKey: "2",
+		},
+		lookupSucceeds: true,
+	}} {
+		test := func(t *testing.T, i int, annotations map[string]string, useMachineDeployment bool) {
+			t.Helper()
+
+			machineSet, machineDeployment := makeMachineOwner(i, 1, annotations, useMachineDeployment)
+
+			node, machine := makeLinkedNodeAndMachine(i, v1.OwnerReference{
+				Name: machineSet.Name,
+				Kind: machineSet.Kind,
+				UID:  machineSet.UID,
+			})
+
+			machineObjects := []runtime.Object{
+				machine,
+				machineSet,
+			}
+
+			if machineDeployment != nil {
+				machineObjects = append(machineObjects, machineDeployment)
+			}
+
+			controller, stop := mustCreateTestController(t, testControllerConfig{
+				nodeObjects:    []runtime.Object{node},
+				machineObjects: machineObjects,
+			})
+			defer stop()
+
+			ng, err := controller.nodeGroupForNode(node)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+
+			if ng == nil && tc.lookupSucceeds {
+				t.Fatalf("expected nil from lookup")
+			}
+
+			if ng != nil && !tc.lookupSucceeds {
+				t.Fatalf("expected non-nil from lookup")
+			}
+
+			if tc.lookupSucceeds {
+				var expected string
+
+				if useMachineDeployment {
+					expected = path.Join(machineDeployment.Namespace, machineDeployment.Name)
+				} else {
+					expected = path.Join(machineSet.Namespace, machineSet.Name)
+				}
+
+				if actual := ng.Id(); actual != expected {
+					t.Errorf("expected %q, got %q", expected, actual)
+				}
+			}
+		}
+
+		testWithMachineDeployment := func(t *testing.T, i int, annotations map[string]string) {
+			t.Helper()
+			test(t, i, tc.annotations, true)
+		}
+
+		testWithoutMachineDeployment := func(t *testing.T, i int, annotations map[string]string) {
+			t.Helper()
+			test(t, i, tc.annotations, false)
+		}
+
+		t.Run(tc.description, func(t *testing.T) {
+			testWithMachineDeployment(t, i, tc.annotations)
+			testWithoutMachineDeployment(t, i, tc.annotations)
+		})
+	}
+}