Fix edge-case where scale-from-zero of MCD is mistaken for a rolling update (#956)

* Fix edge-case where scale-from-zero is mistaken for rolling update

* Address review comments

* Address review comments of PR #954

* Address review comments

* Fix unit test

Author: thiyyakat

Makefile

@@ -176,10 +176,8 @@ add-license-headers: $(GO_ADD_LICENSE)
 
 .PHONY: sast
 sast: $(GOSEC)
-	@chmod +xw hack/sast.sh
 	@./hack/sast.sh
 
 .PHONY: sast-report
 sast-report:$(GOSEC)
-	@chmod +xw hack/sast.sh
 	@./hack/sast.sh --gosec-report true

pkg/apis/machine/v1alpha1/machinedeployment_types.go

@@ -132,10 +132,9 @@ type RollingUpdateMachineDeployment struct {
 	// Value can be an absolute number (ex: 5) or a percentage of desired machines (ex: 10%).
 	// Absolute number is calculated from percentage by rounding down.
 	// This can not be 0 if MaxSurge is 0.
-	// By default, a fixed value of 1 is used.
-	// Example: when this is set to 30%, the old MC can be scaled down to 70% of desired machines
-	// immediately when the rolling update starts. Once new machines are ready, old MC
-	// can be scaled down further, followed by scaling up the new MC, ensuring
+	// Example: when this is set to 30%, the old machine set can be scaled down to 70% of desired machines
+	// immediately when the rolling update starts. Once new machines are ready, old machine set
+	// can be scaled down further, followed by scaling up the new machine set, ensuring
 	// that the total number of machines available at all times during the update is at
 	// least 70% of desired machines.
 	// +optional
@@ -146,12 +145,11 @@ type RollingUpdateMachineDeployment struct {
 	// Value can be an absolute number (ex: 5) or a percentage of desired machines (ex: 10%).
 	// This can not be 0 if MaxUnavailable is 0.
 	// Absolute number is calculated from percentage by rounding up.
-	// By default, a value of 1 is used.
-	// Example: when this is set to 30%, the new MC can be scaled up immediately when
-	// the rolling update starts, such that the total number of old and new machines do not exceed
+	// Example: when this is set to 30%, the new machine set can be scaled up immediately when
+	// the rolling update starts, such that the total number of old and new machines does not exceed
 	// 130% of desired machines. Once old machines have been killed,
-	// new MC can be scaled up further, ensuring that total number of machines running
-	// at any time during the update is atmost 130% of desired machines.
+	// new machine set can be scaled up further, ensuring that total number of machines running
+	// at any time during the update is utmost 130% of desired machines.
 	// +optional
 	MaxSurge *intstr.IntOrString `json:"maxSurge,omitempty"`
 }

pkg/apis/machine/validation/machinedeployment.go

@@ -36,10 +36,10 @@ func canConvertIntOrStringToInt32(val *intstr.IntOrString, replicas int) bool {
 
 func validateUpdateStrategy(spec *machine.MachineDeploymentSpec, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
-	if spec.Strategy.Type != "RollingUpdate" && spec.Strategy.Type != "Recreate" {
+	if spec.Strategy.Type != machine.RollingUpdateMachineDeploymentStrategyType && spec.Strategy.Type != machine.RecreateMachineDeploymentStrategyType {
 		allErrs = append(allErrs, field.Required(fldPath.Child("strategy.type"), "Type can either be RollingUpdate or Recreate"))
 	}
-	if spec.Strategy.Type == "RollingUpdate" {
+	if spec.Strategy.Type == machine.RollingUpdateMachineDeploymentStrategyType {
 		if spec.Strategy.RollingUpdate == nil {
 			allErrs = append(allErrs, field.Required(fldPath.Child("strategy.rollingUpdate"), "RollingUpdate parameter cannot be nil for rolling update strategy"))
 		} else {

pkg/controller/controller_utils_test.go

@@ -552,4 +552,61 @@ var _ = Describe("#controllerUtils", func() {
 			}),
 		)
 	})
+	Describe("##FilterActiveMachineSets", func() {
+		testMachineSet := &machinev1.MachineSet{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "MachineSet-test",
+				Namespace: testNamespace,
+				Labels: map[string]string{
+					"test-label": "test-label",
+				},
+				Annotations: map[string]string{
+					"deployment.kubernetes.io/revision": "1",
+				},
+				UID: "1234567",
+				OwnerReferences: []metav1.OwnerReference{
+					{
+						Kind:       "MachineDeployment",
+						Name:       "MachineDeployment-test",
+						UID:        "1234567",
+						Controller: nil,
+					},
+				},
+				Generation: 5,
+			},
+			TypeMeta: metav1.TypeMeta{
+				Kind:       "MachineSet",
+				APIVersion: "machine.sapcloud.io/v1alpha1",
+			},
+			Spec: machinev1.MachineSetSpec{
+				Replicas: 0,
+				Template: machinev1.MachineTemplateSpec{
+					ObjectMeta: metav1.ObjectMeta{
+						Labels: map[string]string{
+							"test-label": "test-label",
+						},
+					},
+				},
+				Selector: &metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"test-label": "test-label",
+					},
+				},
+			},
+		}
+		It("should return an empty list when machine sets have 0 replicas", func() {
+			testMachineSet.Spec.Replicas = 0
+			testMachineSets := []*machinev1.MachineSet{
+				testMachineSet,
+			}
+			Expect(FilterActiveMachineSets(testMachineSets)).To(HaveLen(0))
+		})
+		It("should return expected machine sets when replicas are positive", func() {
+			testMachineSet.Spec.Replicas = 1
+			testMachineSets := []*machinev1.MachineSet{
+				testMachineSet,
+			}
+			Expect(FilterActiveMachineSets(testMachineSets)).To(HaveLen(1))
+		})
+	})
 })

pkg/controller/deployment_sync.go

@@ -666,6 +666,7 @@ func calculateDeploymentStatus(allISs []*v1alpha1.MachineSet, newIS *v1alpha1.Ma
 
 // isScalingEvent checks whether the provided deployment has been updated with a scaling event
 // by looking at the desired-replicas annotation in the active machine sets of the deployment, and returns if there was scale-out or not.
+// However, when there are no active machine sets, but the replica count in the machine deployment's spec > 0, it is recognized as a scale-out event.
 //
 // rsList should come from getReplicaSetsForDeployment(d).
 // machineMap should come from getmachineMapForDeployment(d, rsList).
@@ -674,8 +675,16 @@ func (dc *controller) isScalingEvent(ctx context.Context, d *v1alpha1.MachineDep
 	if err != nil {
 		return false, err
 	}
+	if newIS == nil {
+		return false, nil
+	}
 	allISs := append(oldISs, newIS)
-	for _, is := range FilterActiveMachineSets(allISs) {
+	activeMachineSets := FilterActiveMachineSets(allISs)
+	//if this is a scale from zero scenario, return true
+	if len(activeMachineSets) == 0 && d.Spec.Replicas > 0 {
+		return true, nil
+	}
+	for _, is := range activeMachineSets {
 		desired, ok := GetDesiredReplicasAnnotation(is)
 		if !ok {
 			continue

pkg/test/integration/common/framework.go

@@ -8,6 +8,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"github.com/gardener/machine-controller-manager/pkg/controller"
 	"io"
 	"log"
 	"os"
@@ -953,27 +954,74 @@ func (c *IntegrationTestFramework) ControllerTests() {
 	// Testcase #02 | machine deployment
 	ginkgo.Describe("machine deployment resource", func() {
 		var initialNodes int16 // initialization should be part of creation test logic
-		ginkgo.Context("creation with replicas=3", func() {
-			ginkgo.It("should not lead to errors and add 3 more nodes to target cluster", func() {
+		ginkgo.Context("creation with replicas=0, scale up with replicas=1", func() {
+			ginkgo.It("should not lead to errors and add 1 more node to target cluster", func() {
+				var machineDeployment *v1alpha1.MachineDeployment
 				//probe initialnodes before continuing
 				initialNodes = c.TargetCluster.GetNumberOfNodes()
 
 				ginkgo.By("Checking for errors")
-				gomega.Expect(c.ControlCluster.CreateMachineDeployment(controlClusterNamespace)).To(gomega.BeNil())
+				gomega.Expect(c.ControlCluster.CreateMachineDeployment(controlClusterNamespace, 0)).To(gomega.BeNil())
 
-				ginkgo.By("Waiting until number of ready nodes are 3 more than initial")
+				ginkgo.By("Waiting for Machine Set to be created")
+				gomega.Eventually(func() int { return c.getNumberOfMachineSets(ctx, controlClusterNamespace) }, c.timeout, c.pollingInterval).Should(gomega.BeNumerically("==", 1))
+
+				ginkgo.By("Updating machineDeployment replicas to 1")
+				retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+					machineDeployment, _ = c.ControlCluster.McmClient.
+						MachineV1alpha1().
+						MachineDeployments(controlClusterNamespace).
+						Get(ctx, "test-machine-deployment", metav1.GetOptions{})
+					machineDeployment.Spec.Replicas = 1
+					_, updateErr := c.ControlCluster.McmClient.
+						MachineV1alpha1().
+						MachineDeployments(controlClusterNamespace).
+						Update(ctx, machineDeployment, metav1.UpdateOptions{})
+					return updateErr
+				})
+				gomega.Expect(retryErr).NotTo(gomega.HaveOccurred())
+
+				//check machineDeploymentStatus to make sure correct condition is reflected
+				ginkgo.By("Checking if machineDeployment's status has been updated with correct conditions")
+				gomega.Eventually(func() bool {
+					var err error
+					machineDeployment, err = c.ControlCluster.McmClient.
+						MachineV1alpha1().
+						MachineDeployments(controlClusterNamespace).
+						Get(ctx, "test-machine-deployment", metav1.GetOptions{})
+					gomega.Expect(err).NotTo(gomega.HaveOccurred())
+					if len(machineDeployment.Status.Conditions) != 2 {
+						return false
+					}
+					isMCDAvailable := false
+					hasMCDProgressed := false
+					for _, condition := range machineDeployment.Status.Conditions {
+						if condition.Type == v1alpha1.MachineDeploymentAvailable && condition.Status == v1alpha1.ConditionTrue && condition.Reason == controller.MinimumReplicasAvailable {
+							isMCDAvailable = true
+						}
+						if condition.Type == v1alpha1.MachineDeploymentProgressing && condition.Status == v1alpha1.ConditionTrue && condition.Reason == controller.NewISAvailableReason {
+							hasMCDProgressed = true
+						}
+					}
+					return hasMCDProgressed && isMCDAvailable && machineDeployment.Status.AvailableReplicas == 1
+				}, c.timeout, c.pollingInterval).Should(gomega.BeTrue())
+
+				//check number of ready nodes
+				ginkgo.By("Checking number of ready nodes==1")
 				gomega.Eventually(
 					c.TargetCluster.GetNumberOfNodes,
-					c.timeout, c.pollingInterval).
-					Should(gomega.BeNumerically("==", initialNodes+3))
+					c.timeout,
+					c.pollingInterval).
+					Should(gomega.BeNumerically("==", initialNodes+1))
 				gomega.Eventually(
 					c.TargetCluster.GetNumberOfReadyNodes,
-					c.timeout, c.pollingInterval).
-					Should(gomega.BeNumerically("==", initialNodes+3))
+					c.timeout,
+					c.pollingInterval).
+					Should(gomega.BeNumerically("==", initialNodes+1))
 			})
 		})
 		ginkgo.Context("scale-up with replicas=6", func() {
-			ginkgo.It("should not lead to errors and add futher 3 nodes to target cluster", func() {
+			ginkgo.It("should not lead to errors and add further 5 nodes to target cluster", func() {
 				retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
 					machineDployment, _ := c.ControlCluster.McmClient.
 						MachineV1alpha1().
@@ -1407,6 +1455,18 @@ func (c *IntegrationTestFramework) Cleanup() {
 
 }
 
+func (c *IntegrationTestFramework) getNumberOfMachineSets(ctx context.Context, namespace string) int {
+	machineSets, err := c.ControlCluster.McmClient.MachineV1alpha1().MachineSets(namespace).List(ctx, metav1.ListOptions{})
+	gomega.Expect(err).NotTo(gomega.HaveOccurred())
+	count := 0
+	for _, machineSet := range machineSets.Items {
+		if machineSet.OwnerReferences[0].Name == "test-machine-deployment" {
+			count++
+		}
+	}
+	return count
+}
+
 func checkMcmRepoAvailable() {
 	ginkgo.By("Checking Machine-Controller-Manager repo is available at: " + mcmRepoPath)
 	_, err := os.Stat(mcmRepoPath)

pkg/test/integration/common/helpers/machine_resources.go

@@ -40,7 +40,7 @@ func (c *Cluster) CreateMachine(namespace string) error {
 }
 
 // CreateMachineDeployment creates a test-machine-deployment with 3 replicas and returns error if it occurs
-func (c *Cluster) CreateMachineDeployment(namespace string) error {
+func (c *Cluster) CreateMachineDeployment(namespace string, replicas int32) error {
 	labels := map[string]string{"test-label": "test-label"}
 	_, err := c.McmClient.
 		MachineV1alpha1().
@@ -53,7 +53,7 @@ func (c *Cluster) CreateMachineDeployment(namespace string) error {
 					Namespace: namespace,
 				},
 				Spec: v1alpha1.MachineDeploymentSpec{
-					Replicas:        3,
+					Replicas:        replicas,
 					MinReadySeconds: 500,
 					Strategy: v1alpha1.MachineDeploymentStrategy{
 						Type: v1alpha1.RollingUpdateMachineDeploymentStrategyType,