Merge pull request #42 from frobware/fix-bz-1656334

UPSTREAM: <carry>: openshift: return no nodegroup when scaling size is zero

Author: openshift-merge-robot

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()
@@ -591,178 +636,6 @@ func TestControllerNodeGroupForNodeWithMissingMachineOwner(t *testing.T) {
 	}
 }
 
-func TestControllerNodeGroupForNodeSuccessFromMachineSet(t *testing.T) {
-	node := &apiv1.Node{
-		TypeMeta: v1.TypeMeta{
-			Kind: "Node",
-		},
-		ObjectMeta: v1.ObjectMeta{
-			Name: "node",
-			Annotations: map[string]string{
-				machineAnnotationKey: "test-namespace/machine",
-			},
-		},
-		Spec: apiv1.NodeSpec{
-			ProviderID: "provider-id",
-		},
-	}
-
-	machineSet := &v1beta1.MachineSet{
-		TypeMeta: v1.TypeMeta{
-			Kind: "MachineSet",
-		},
-		ObjectMeta: v1.ObjectMeta{
-			Name:      "machineset",
-			Namespace: "test-namespace",
-			UID:       uuid1,
-		},
-	}
-
-	machine := &v1beta1.Machine{
-		TypeMeta: v1.TypeMeta{
-			Kind: "Machine",
-		},
-		ObjectMeta: v1.ObjectMeta{
-			Name:      "machine",
-			Namespace: "test-namespace",
-			OwnerReferences: []v1.OwnerReference{{
-				Name: machineSet.Name,
-				Kind: machineSet.Kind,
-				UID:  machineSet.UID,
-			}},
-		},
-	}
-
-	controller, stop := mustCreateTestController(t, testControllerConfig{
-		nodeObjects: []runtime.Object{
-			node,
-		},
-		machineObjects: []runtime.Object{
-			machine,
-			machineSet,
-		},
-	})
-	defer stop()
-
-	ng, err := controller.nodeGroupForNode(&apiv1.Node{
-		TypeMeta: v1.TypeMeta{
-			Kind: "Node",
-		},
-		ObjectMeta: v1.ObjectMeta{
-			Name: "node",
-		},
-		Spec: apiv1.NodeSpec{
-			ProviderID: "provider-id",
-		},
-	})
-
-	if err != nil {
-		t.Fatal("expected no error")
-	}
-
-	if ng == nil {
-		t.Fatal("expected a nodegroup")
-	}
-
-	expected := path.Join(machineSet.Namespace, machineSet.Name)
-	if actual := ng.Id(); actual != expected {
-		t.Errorf("expected %q, got %q", expected, actual)
-	}
-}
-
-func TestControllerNodeGroupForNodeSuccessFromMachineDeployment(t *testing.T) {
-	node := &apiv1.Node{
-		TypeMeta: v1.TypeMeta{
-			Kind: "Node",
-		},
-		ObjectMeta: v1.ObjectMeta{
-			Name: "node",
-			Annotations: map[string]string{
-				machineAnnotationKey: "test-namespace/machine",
-			},
-		},
-		Spec: apiv1.NodeSpec{
-			ProviderID: "provider-id",
-		},
-	}
-
-	machineDeployment := &v1beta1.MachineDeployment{
-		TypeMeta: v1.TypeMeta{
-			Kind: "MachineDeployment",
-		},
-		ObjectMeta: v1.ObjectMeta{
-			Name:      "machinedeployment",
-			Namespace: "test-namespace",
-		},
-	}
-
-	machineSet := &v1beta1.MachineSet{
-		TypeMeta: v1.TypeMeta{
-			Kind: "MachineSet",
-		},
-		ObjectMeta: v1.ObjectMeta{
-			Name:      "machineset",
-			Namespace: "test-namespace",
-			OwnerReferences: []v1.OwnerReference{{
-				Kind: "MachineDeployment",
-				Name: machineDeployment.Name,
-			}},
-		},
-	}
-
-	machine := &v1beta1.Machine{
-		TypeMeta: v1.TypeMeta{
-			Kind: "Machine",
-		},
-		ObjectMeta: v1.ObjectMeta{
-			Name:      "machine",
-			Namespace: "test-namespace",
-			OwnerReferences: []v1.OwnerReference{{
-				Name: machineSet.Name,
-				Kind: machineSet.Kind,
-				UID:  machineSet.UID,
-			}},
-		},
-	}
-
-	controller, stop := mustCreateTestController(t, testControllerConfig{
-		nodeObjects: []runtime.Object{
-			node,
-		},
-		machineObjects: []runtime.Object{
-			machine,
-			machineSet,
-			machineDeployment,
-		},
-	})
-	defer stop()
-
-	ng, err := controller.nodeGroupForNode(&apiv1.Node{
-		TypeMeta: v1.TypeMeta{
-			Kind: "Node",
-		},
-		ObjectMeta: v1.ObjectMeta{
-			Name: "node",
-		},
-		Spec: apiv1.NodeSpec{
-			ProviderID: "provider-id",
-		},
-	})
-
-	if err != nil {
-		t.Fatal("expected no error")
-	}
-
-	if ng == nil {
-		t.Fatal("expected a nodegroup")
-	}
-
-	expected := path.Join(machineDeployment.Namespace, machineDeployment.Name)
-	if actual := ng.Id(); actual != expected {
-		t.Errorf("expected %q, got %q", expected, actual)
-	}
-}
-
 func TestControllerNodeGroupsWithMachineDeployments(t *testing.T) {
 	machineDeploymentTemplate := &v1beta1.MachineDeployment{
 		TypeMeta: v1.TypeMeta{
@@ -928,3 +801,88 @@ 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)
+				}
+			}
+		}
+
+		t.Run(tc.description, func(t *testing.T) {
+			test(t, i, tc.annotations, true) // with MachineDeployment
+			test(t, i, tc.annotations, false)
+		})
+	}
+}