Merge pull request #153 from JoelSpeed/fallback-to-status-replicas-4.4

[release-4.4] BUG 1835160: Fallback to status if replicas nil in spec
openshift · Jun 25, 2020 · 2ec541e · 2ec541e
2 parents 6768c3c + 88eeac6
commit 2ec541e
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 21 deletions.
diff --git a/cluster-autoscaler/cloudprovider/openshiftmachineapi/machineapi_machinedeployment.go b/cluster-autoscaler/cloudprovider/openshiftmachineapi/machineapi_machinedeployment.go
@@ -23,6 +23,7 @@ import (
 	"github.com/openshift/cluster-api/pkg/apis/machine/v1beta1"
 	machinev1beta1 "github.com/openshift/cluster-api/pkg/client/clientset_generated/clientset/typed/machine/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/klog"
 	"k8s.io/utils/pointer"
 )
 
@@ -76,7 +77,10 @@ func (r machineDeploymentScalableResource) Nodes() ([]string, error) {
 }
 
 func (r machineDeploymentScalableResource) Replicas() int32 {
-	return pointer.Int32PtrDerefOr(r.machineDeployment.Spec.Replicas, 0)
+	if r.machineDeployment.Spec.Replicas == nil {
+		klog.Warningf("MachineDeployment %q has nil spec.replicas. This is unsupported behaviour. Falling back to status.replicas.", r.machineDeployment.Name)
+	}
+	return pointer.Int32PtrDerefOr(r.machineDeployment.Spec.Replicas, r.machineDeployment.Status.Replicas)
 }
 
 func (r machineDeploymentScalableResource) SetSize(nreplicas int32) error {

diff --git a/cluster-autoscaler/cloudprovider/openshiftmachineapi/machineapi_machineset.go b/cluster-autoscaler/cloudprovider/openshiftmachineapi/machineapi_machineset.go
@@ -23,6 +23,7 @@ import (
 	"github.com/openshift/cluster-api/pkg/apis/machine/v1beta1"
 	machinev1beta1 "github.com/openshift/cluster-api/pkg/client/clientset_generated/clientset/typed/machine/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/klog"
 	"k8s.io/utils/pointer"
 )
 
@@ -61,7 +62,10 @@ func (r machineSetScalableResource) Nodes() ([]string, error) {
 }
 
 func (r machineSetScalableResource) Replicas() int32 {
-	return pointer.Int32PtrDerefOr(r.machineSet.Spec.Replicas, 0)
+	if r.machineSet.Spec.Replicas == nil {
+		klog.Warningf("MachineSet %q has nil spec.replicas. This is unsupported behaviour. Falling back to status.replicas.", r.machineSet.Name)
+	}
+	return pointer.Int32PtrDerefOr(r.machineSet.Spec.Replicas, r.machineSet.Status.Replicas)
 }
 
 func (r machineSetScalableResource) SetSize(nreplicas int32) error {

diff --git a/cluster-autoscaler/cloudprovider/openshiftmachineapi/machineapi_nodegroup_test.go b/cluster-autoscaler/cloudprovider/openshiftmachineapi/machineapi_nodegroup_test.go
@@ -316,10 +316,11 @@ func TestNodeGroupIncreaseSizeErrors(t *testing.T) {
 
 func TestNodeGroupIncreaseSize(t *testing.T) {
 	type testCase struct {
-		description string
-		delta       int
-		initial     int32
-		expected    int32
+		description   string
+		delta         int
+		initialSpec   *int32
+		initialStatus int32
+		expected      int32
 	}
 
 	test := func(t *testing.T, tc *testCase, testConfig *testConfig) {
@@ -341,8 +342,16 @@ func TestNodeGroupIncreaseSize(t *testing.T) {
 			t.Fatalf("unexpected error: %v", err)
 		}
 
-		if currReplicas != int(tc.initial) {
-			t.Errorf("initially expected %v, got %v", tc.initial, currReplicas)
+		// If initialSpec is nil, fallback to replica count from status
+		// TODO: Remove this fallback once defaulting is implemented for MachineSet Replicas
+		if tc.initialSpec != nil {
+			if currReplicas != int(pointer.Int32PtrDerefOr(tc.initialSpec, -1)) {
+				t.Errorf("initially expected %v, got %v", tc.initialSpec, currReplicas)
+			}
+		} else {
+			if currReplicas != int(tc.initialStatus) {
+				t.Errorf("initially expected %v, got %v", tc.initialStatus, currReplicas)
+			}
 		}
 
 		if err := ng.IncreaseSize(tc.delta); err != nil {
@@ -381,29 +390,52 @@ func TestNodeGroupIncreaseSize(t *testing.T) {
 	t.Run("MachineSet", func(t *testing.T) {
 		tc := testCase{
 			description: "increase by 1",
-			initial:     3,
+			initialSpec: pointer.Int32Ptr(3),
 			expected:    4,
 			delta:       1,
 		}
-		test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations))
+		test(t, &tc, createMachineSetTestConfig(testNamespace, int(*tc.initialSpec), annotations))
 	})
 
 	t.Run("MachineDeployment", func(t *testing.T) {
 		tc := testCase{
 			description: "increase by 1",
-			initial:     3,
+			initialSpec: pointer.Int32Ptr(3),
 			expected:    4,
 			delta:       1,
 		}
-		test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(tc.initial), annotations))
+		test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(*tc.initialSpec), annotations))
+	})
+
+	t.Run("MachineSet with nil Spec.Replicas", func(t *testing.T) {
+		tc := testCase{
+			description:   "increase by 3",
+			initialSpec:   nil,
+			initialStatus: 2,
+			expected:      5,
+			delta:         3,
+		}
+		test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initialStatus), annotations))
+	})
+
+	t.Run("MachineDeployment with nil Spec.Replicas", func(t *testing.T) {
+		tc := testCase{
+			description:   "increase by 3",
+			initialSpec:   nil,
+			initialStatus: 2,
+			expected:      5,
+			delta:         3,
+		}
+		test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(tc.initialStatus), annotations))
 	})
 }
 
 func TestNodeGroupDecreaseTargetSize(t *testing.T) {
 	type testCase struct {
 		description         string
 		delta               int
-		initial             int32
+		initialSpec         *int32
+		initialStatus       int32
 		targetSizeIncrement int32
 		expected            int32
 		expectedError       bool
@@ -460,8 +492,16 @@ func TestNodeGroupDecreaseTargetSize(t *testing.T) {
 		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 initialSpec is nil, fallback to replica count from status
+		// TODO: Remove this fallback once defaulting is implemented for MachineSet Replicas
+		if tc.initialSpec != nil {
+			if currReplicas != int(*tc.initialSpec)+int(tc.targetSizeIncrement) {
+				t.Errorf("initially expected %v, got %v", tc.initialSpec, currReplicas)
+			}
+		} else {
+			if currReplicas != int(tc.initialStatus)+int(tc.targetSizeIncrement) {
+				t.Errorf("initially expected %v, got %v", tc.initialStatus, currReplicas)
+			}
 		}
 
 		if err := ng.DecreaseTargetSize(tc.delta); (err != nil) != tc.expectedError {
@@ -500,36 +540,48 @@ func TestNodeGroupDecreaseTargetSize(t *testing.T) {
 	t.Run("MachineSet", func(t *testing.T) {
 		tc := testCase{
 			description:         "Same number of existing instances and node group target size should error",
-			initial:             3,
+			initialSpec:         pointer.Int32Ptr(3),
 			targetSizeIncrement: 0,
 			expected:            3,
 			delta:               -1,
 			expectedError:       true,
 		}
-		test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations))
+		test(t, &tc, createMachineSetTestConfig(testNamespace, int(*tc.initialSpec), 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,
+			initialSpec:         pointer.Int32Ptr(3),
 			targetSizeIncrement: 1,
 			expected:            3,
 			delta:               -1,
 		}
-		test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations))
+		test(t, &tc, createMachineSetTestConfig(testNamespace, int(*tc.initialSpec), annotations))
 	})
 
 	t.Run("MachineDeployment", func(t *testing.T) {
 		tc := testCase{
 			description:         "Same number of existing instances and node group target size should error",
-			initial:             3,
+			initialSpec:         pointer.Int32Ptr(3),
 			targetSizeIncrement: 0,
 			expected:            3,
 			delta:               -1,
 			expectedError:       true,
 		}
-		test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(tc.initial), annotations))
+		test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(*tc.initialSpec), annotations))
+	})
+
+	t.Run("MachineSet with nil Spec.Replicas", func(t *testing.T) {
+		tc := testCase{
+			description:         "A node group with targe size 6 but only 4 existing instances should decrease by 2",
+			initialSpec:         nil,
+			initialStatus:       4,
+			targetSizeIncrement: 2,
+			expected:            4,
+			delta:               -2,
+		}
+		test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initialStatus), annotations))
 	})
 }