Automated cherry pick of #803: edge cases dealt with, unit tests added (

#804)

* edge cases dealt with, unit tests added

* removed printf statements

* minor fix

* error logging corrected

* added warning log

* removed unused package import

* fixed failing unit tests

---------

Co-authored-by: Madhav Bhargava <madhav.bhargava@sap.com>

pkg/controller/controller_suite_test.go

@@ -47,7 +47,7 @@ import (
 )
 
 func TestMachineControllerManagerSuite(t *testing.T) {
-	//for filtering out warning logs. Reflector short watch warning logs won't print now
+	// for filtering out warning logs. Reflector short watch warning logs won't print now
 	klog.SetOutput(io.Discard)
 	flags := &flag.FlagSet{}
 	klog.InitFlags(flags)
@@ -66,12 +66,20 @@ func newMachineDeployment(
 	specTemplate *v1alpha1.MachineTemplateSpec,
 	replicas int32,
 	minReadySeconds int32,
+	maxSurge int,
+	maxUnavailable int,
 	statusTemplate *v1alpha1.MachineDeploymentStatus,
 	owner *metav1.OwnerReference,
 	annotations map[string]string,
 	labels map[string]string,
 ) *v1alpha1.MachineDeployment {
-	return newMachineDeployments(1, specTemplate, replicas, minReadySeconds, statusTemplate, owner, annotations, labels)[0]
+	md := newMachineDeployments(1, specTemplate, replicas, minReadySeconds, statusTemplate, owner, annotations, labels)[0]
+	intStrMaxSurge := intstr.FromInt(maxSurge)
+	intStrMaxUnavailable := intstr.FromInt(maxUnavailable)
+	md.Spec.Strategy.RollingUpdate.MaxSurge = &intStrMaxSurge
+	md.Spec.Strategy.RollingUpdate.MaxUnavailable = &intStrMaxUnavailable
+
+	return md
 }
 
 func newMachineDeployments(
@@ -105,6 +113,7 @@ func newMachineDeployments(
 					MatchLabels: deepCopy(specTemplate.ObjectMeta.Labels),
 				},
 				Strategy: v1alpha1.MachineDeploymentStrategy{
+					Type: v1alpha1.RollingUpdateMachineDeploymentStrategyType,
 					RollingUpdate: &v1alpha1.RollingUpdateMachineDeployment{
 						MaxSurge:       &intStr1,
 						MaxUnavailable: &intStr1,
@@ -181,14 +190,17 @@ func newMachineSetsFromMachineDeployment(
 
 func newMachineSet(
 	specTemplate *v1alpha1.MachineTemplateSpec,
+	name string,
 	replicas int32,
 	minReadySeconds int32,
 	statusTemplate *v1alpha1.MachineSetStatus,
 	owner *metav1.OwnerReference,
 	annotations map[string]string,
 	labels map[string]string,
 ) *v1alpha1.MachineSet {
-	return newMachineSets(1, specTemplate, replicas, minReadySeconds, statusTemplate, owner, annotations, labels)[0]
+	ms := newMachineSets(1, specTemplate, replicas, minReadySeconds, statusTemplate, owner, annotations, labels)[0]
+	ms.Name = name
+	return ms
 }
 
 func newMachineSets(

pkg/controller/deployment_sync.go

@@ -452,7 +452,7 @@ func (dc *controller) scale(ctx context.Context, deployment *v1alpha1.MachineDep
 		nameToSize := make(map[string]int32)
 		deploymentReplicasAdded := int32(0)
 		switch {
-		case deploymentReplicasToAdd > 0:
+		case deploymentReplicasToAdd >= 0:
 			scalingOperation = "up"
 			nameToSize = dc.scaleNewMachineSet(newIS, allISs, deploymentReplicasToAdd, deployment)
 			deploymentReplicasAdded = deploymentReplicasToAdd
@@ -478,6 +478,7 @@ func (dc *controller) scale(ctx context.Context, deployment *v1alpha1.MachineDep
 			// TODO: Use transactions when we have them.
 			if _, _, err := dc.scaleMachineSet(ctx, is, nameToSize[is.Name], deployment, scalingOperation); err != nil {
 				// Return as soon as we fail, the deployment is requeued
+				klog.Warningf("updating machineSet %s failed while scaling. This could lead to desired replicas annotation not being updated. err: %v", is.Name, err)
 				return err
 			}
 		}
@@ -677,6 +678,7 @@ func (dc *controller) isScalingEvent(ctx context.Context, d *v1alpha1.MachineDep
 			continue
 		}
 		if desired != (d.Spec.Replicas) {
+			klog.V(2).Infof("Desired replicas annotation value: %d on machineSet %s, Spec Desired Replicas value: %d on corresponding machineDeployment, so scaling has happened.", desired, is.Name, d.Spec.Replicas)
 			return true, nil
 		}
 	}