[GEP-31] Controller changes to Support Manual InPlaceUpdate strategy (#987)

* Manual Inplace

Co-Authored-By: Shafeeque E S <shafeeque.e.s@sap.com>
Co-Authored-By: Sonu Kumar Singh <28653770+acumino@users.noreply.github.com>

* Minor Nit

Co-Authored-By: Shafeeque E S <shafeeque.e.s@sap.com>
Co-Authored-By: Sonu Kumar Singh <28653770+acumino@users.noreply.github.com>

* Address Review

* Address Review

* Get MCD for machines without passing context

* Add predicate to reconcile MCD when machine update is successful

* Enhance few logs

* Change is to machineSet

---------

Co-authored-by: Shafeeque E S <shafeeque.e.s@sap.com>
Co-authored-by: Sonu Kumar Singh <28653770+acumino@users.noreply.github.com>
Co-authored-by: Sonu Kumar Singh <sksgkpvks@gmail.com>

Author: 3 people

pkg/controller/controller.go

@@ -119,6 +119,7 @@ func NewController(
 
 	// MachineDeployment Controller Informers
 	_, _ = machineInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+		UpdateFunc: controller.updateMachineToMachineDeployment,
 		DeleteFunc: controller.deleteMachineDeployment,
 	})
 

pkg/controller/deployment.go

@@ -222,6 +222,32 @@ func (dc *controller) deleteMachineSetToDeployment(obj interface{}) {
 	dc.enqueueMachineDeployment(d)
 }
 
+// updateMachineToMachineDeployment will enqueue the machine deployment if the machine InPlaceUpdate node condition changes to UpdateSuccessful.
+func (dc *controller) updateMachineToMachineDeployment(old, cur any) {
+	oldMachine, ok := old.(*v1alpha1.Machine)
+	if !ok {
+		return
+	}
+
+	curMachine, ok := cur.(*v1alpha1.Machine)
+	if !ok {
+		return
+	}
+
+	oldMachineCondition := getMachineCondition(oldMachine, v1alpha1.NodeInPlaceUpdate)
+	currMachineCondition := getMachineCondition(curMachine, v1alpha1.NodeInPlaceUpdate)
+
+	oldMachineConditionReasonUpdateSuccessful := oldMachineCondition != nil && oldMachineCondition.Reason == v1alpha1.UpdateSuccessful
+	currMachineConditionReasonUpdateSuccessful := currMachineCondition != nil && currMachineCondition.Reason == v1alpha1.UpdateSuccessful
+
+	if !oldMachineConditionReasonUpdateSuccessful && currMachineConditionReasonUpdateSuccessful {
+		d := dc.getMachineDeploymentForMachine(curMachine)
+		if d != nil {
+			dc.enqueueMachineDeployment(d)
+		}
+	}
+}
+
 // deleteMachine will enqueue a Recreate Deployment once all of its Machines have stopped running.
 func (dc *controller) deleteMachineToMachineDeployment(ctx context.Context, obj interface{}) {
 	machine, ok := obj.(*v1alpha1.Machine)
@@ -243,7 +269,7 @@ func (dc *controller) deleteMachineToMachineDeployment(ctx context.Context, obj
 		}
 	}
 	klog.V(4).Infof("Machine %s deleted.", machine.Name)
-	if d := dc.getMachineDeploymentForMachine(ctx, machine); d != nil && d.Spec.Strategy.Type == v1alpha1.RecreateMachineDeploymentStrategyType {
+	if d := dc.getMachineDeploymentForMachine(machine); d != nil && d.Spec.Strategy.Type == v1alpha1.RecreateMachineDeploymentStrategyType {
 		// Sync if this Deployment now has no more Machines.
 		machineSets, err := ListMachineSets(d, IsListFromClient(ctx, dc.controlMachineClient))
 		if err != nil {
@@ -295,7 +321,7 @@ func (dc *controller) enqueueMachineDeploymentAfter(deployment *v1alpha1.Machine
 }
 
 // getDeploymentForMachine returns the deployment managing the given Machine.
-func (dc *controller) getMachineDeploymentForMachine(ctx context.Context, machine *v1alpha1.Machine) *v1alpha1.MachineDeployment {
+func (dc *controller) getMachineDeploymentForMachine(machine *v1alpha1.Machine) *v1alpha1.MachineDeployment {
 	// Find the owning machine set
 	var is *v1alpha1.MachineSet
 	var err error
@@ -308,7 +334,8 @@ func (dc *controller) getMachineDeploymentForMachine(ctx context.Context, machin
 		// Not a Machine owned by a machine set.
 		return nil
 	}
-	is, err = dc.controlMachineClient.MachineSets(machine.Namespace).Get(ctx, controllerRef.Name, metav1.GetOptions{})
+
+	is, err = dc.machineSetLister.MachineSets(machine.Namespace).Get(controllerRef.Name)
 	if err != nil || is.UID != controllerRef.UID {
 		klog.V(4).Infof("Cannot get machineset %q for machine %q: %v", controllerRef.Name, machine.Name, err)
 		return nil
@@ -546,12 +573,9 @@ func (dc *controller) reconcileClusterMachineDeployment(key string) error {
 	case v1alpha1.RollingUpdateMachineDeploymentStrategyType:
 		return dc.rolloutRolling(ctx, d, machineSets, machineMap)
 	case v1alpha1.InPlaceUpdateMachineDeploymentStrategyType:
-		if d.Spec.Strategy.InPlaceUpdate.OrchestrationType == v1alpha1.OrchestrationTypeAuto {
-			return dc.rolloutAutoInPlace(ctx, d, machineSets, machineMap)
-		}
-
-		// TODO(ary1992): Implement Manual InPlace strategy
+		return dc.rolloutInPlace(ctx, d, machineSets, machineMap)
 	}
+
 	return fmt.Errorf("unexpected deployment strategy type: %s", d.Spec.Strategy.Type)
 }
 

pkg/controller/deployment_inplace.go

@@ -25,15 +25,15 @@ import (
 	"k8s.io/utils/integer"
 )
 
-// rolloutAutoInPlace implements the logic for rolling  a machine set without replacing it.
-func (dc *controller) rolloutAutoInPlace(ctx context.Context, d *v1alpha1.MachineDeployment, isList []*v1alpha1.MachineSet, machineMap map[types.UID]*v1alpha1.MachineList) error {
+// rolloutInPlace implements the logic for rolling a machine set without replacing its machines.
+func (dc *controller) rolloutInPlace(ctx context.Context, d *v1alpha1.MachineDeployment, machineSetList []*v1alpha1.MachineSet, machineMap map[types.UID]*v1alpha1.MachineList) error {
 	clusterAutoscalerScaleDownAnnotations := make(map[string]string)
 	clusterAutoscalerScaleDownAnnotations[autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationKey] = autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationValue
 
 	// We do this to avoid accidentally deleting the user provided annotations.
 	clusterAutoscalerScaleDownAnnotations[autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationByMCMKey] = autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationByMCMValue
 
-	newMachineSet, oldMachineSets, err := dc.getAllMachineSetsAndSyncRevision(ctx, d, isList, machineMap, true)
+	newMachineSet, oldMachineSets, err := dc.getAllMachineSetsAndSyncRevision(ctx, d, machineSetList, machineMap, true)
 	if err != nil {
 		return err
 	}
@@ -95,7 +95,7 @@ func (dc *controller) rolloutAutoInPlace(ctx context.Context, d *v1alpha1.Machin
 		return dc.syncRolloutStatus(ctx, allMachineSets, newMachineSet, d)
 	}
 
-	// prepare old ISs for update
+	// prepare old machineSets for update
 	workDone, err := dc.reconcileOldMachineSetsInPlace(ctx, allMachineSets, FilterActiveMachineSets(oldMachineSets), newMachineSet, d)
 	if err != nil {
 		return err
@@ -131,13 +131,14 @@ func (dc *controller) syncMachineSets(ctx context.Context, oldMachineSets []*v1a
 	}
 
 	machinesWithUpdateSuccessfulLabel := filterMachinesWithUpdateSuccessfulLabel(newMachines)
-	klog.V(3).Infof("Found %d machine(s) with label %q=%q in new machine set", len(machinesWithUpdateSuccessfulLabel), v1alpha1.LabelKeyNodeUpdateResult, v1alpha1.LabelValueNodeUpdateSuccessful)
+	klog.V(3).Infof("Found %d machine(s) with label %q=%q in new machine set %q", len(machinesWithUpdateSuccessfulLabel), v1alpha1.LabelKeyNodeUpdateResult, v1alpha1.LabelValueNodeUpdateSuccessful, newMachineSet.Name)
 
 	if len(newMachines) > int(newMachineSet.Spec.Replicas) && len(machinesWithUpdateSuccessfulLabel) > 0 {
 		// scale up the new machine set to the number of machines with the update successful label.
 		// This is to ensure that the machines with moved to the new machine set when ownership is transferred is accounted.
 		scaleUpBy := min(len(machinesWithUpdateSuccessfulLabel), len(newMachines)-int(newMachineSet.Spec.Replicas))
-		_, _, err := dc.scaleMachineSetAndRecordEvent(ctx, newMachineSet, newMachineSet.Spec.Replicas+int32(scaleUpBy), deployment) // #nosec G115 (CWE-190) -- value already validated
+		klog.V(3).Infof("scale up the new machine set %s by %d to %d replicas", newMachineSet.Name, scaleUpBy, newMachineSet.Spec.Replicas+int32(scaleUpBy)) // #nosec G115 (CWE-190) -- value already validated
+		_, _, err := dc.scaleMachineSetAndRecordEvent(ctx, newMachineSet, newMachineSet.Spec.Replicas+int32(scaleUpBy), deployment)                          // #nosec G115 (CWE-190) -- value already validated
 		if err != nil {
 			return err
 		}
@@ -247,7 +248,7 @@ func (dc *controller) reconcileNewMachineSetInPlace(ctx context.Context, oldMach
 		return false, nil
 	}
 
-	klog.V(3).Infof("scale up the new machine set %s by %d", newMachineSet.Name, addedNewReplicasCount)
+	klog.V(3).Infof("scale up the new machine set %s by %d to %d replicas", newMachineSet.Name, addedNewReplicasCount, newMachineSet.Spec.Replicas+addedNewReplicasCount)
 	scaled, _, err := dc.scaleMachineSetAndRecordEvent(ctx, newMachineSet, newMachineSet.Spec.Replicas+addedNewReplicasCount, deployment)
 	return scaled, err
 }
@@ -273,33 +274,45 @@ func (dc *controller) reconcileOldMachineSetsInPlace(ctx context.Context, allMac
 		return true, nil
 	}
 
+	// Manual inplace update
+	if deployment.Spec.Strategy.InPlaceUpdate.OrchestrationType == v1alpha1.OrchestrationTypeManual {
+		return false, nil
+	}
+
 	allMachinesCount := GetReplicaCountForMachineSets(allMachineSets)
 	klog.V(3).Infof("New machine set %s has %d available machines.", newMachineSet.Name, newMachineSet.Status.AvailableReplicas)
 	maxUnavailable := MaxUnavailable(*deployment)
 
 	minAvailable := deployment.Spec.Replicas - maxUnavailable
-	newISUnavailableMachineCount := newMachineSet.Spec.Replicas - newMachineSet.Status.AvailableReplicas
-	oldISsMachinesUndergoingUpdate, err := dc.getMachinesUndergoingUpdate(oldMachineSets)
+	newMachineSetUnavailableMachineCount := newMachineSet.Spec.Replicas - newMachineSet.Status.AvailableReplicas
+	oldMachineSetsMachinesUndergoingUpdate, err := dc.getMachinesUndergoingUpdate(oldMachineSets)
 	if err != nil {
 		return false, err
 	}
 
-	klog.V(3).Infof("allMachinesCount:%d,  minAvailable:%d,  newISUnavailableMachineCount:%d,  oldISsMachineInUpdateProcess:%d", allMachinesCount, minAvailable, newISUnavailableMachineCount, oldISsMachinesUndergoingUpdate)
+	// Machines from old machine sets which are undergoing update will eventually move to new machine set.
+	// So once the current new machine set replcas + old machine set replicas undergoing update reaches the desired replicas,
+	// we can stop selecting machines from old machine sets for update.
+	if oldMachineSetsMachinesUndergoingUpdate+newMachineSet.Spec.Replicas >= deployment.Spec.Replicas {
+		return false, nil
+	}
+
+	klog.V(3).Infof("allMachinesCount:%d,  minAvailable:%d,  newMachineSetUnavailableMachineCount:%d,  oldISsMachineInUpdateProcess:%d", allMachinesCount, minAvailable, newMachineSetUnavailableMachineCount, oldMachineSetsMachinesUndergoingUpdate)
 
 	// maxUpdatePossible is calculated as the total number of machines (allMachinesCount)
 	// minus the minimum number of machines that must remain available (minAvailable),
-	// minus the number of machines in the new instance set that are currently unavailable (newISUnavailableMachineCount),
-	// minus the number of machines in the old instance sets that are undergoing updates (oldISsMachinesUndergoingUpdate).
+	// minus the number of machines in the new instance set that are currently unavailable (newMachineSetUnavailableMachineCount),
+	// minus the number of machines in the old instance sets that are undergoing updates (oldMachineSetsMachinesUndergoingUpdate).
 	// here unavailable machines of old machine sets are not considered as first we want to check if we can select machines for update from old machine sets
 	// after fulfilling all the constraints.
-	maxUpdatePossible := allMachinesCount - minAvailable - newISUnavailableMachineCount - oldISsMachinesUndergoingUpdate
+	maxUpdatePossible := allMachinesCount - minAvailable - newMachineSetUnavailableMachineCount - oldMachineSetsMachinesUndergoingUpdate
 	if maxUpdatePossible <= 0 {
 		klog.V(3).Infof("no machines can be selected for update from old machine sets")
 		return false, nil
 	}
 
 	// prepare machines from old machine sets for update, need to check maxUnavailable to ensure we can select machines for update.
-	numOfMachinesSelectedForUpdate, err := dc.selectNumOfMachineForUpdate(ctx, allMachineSets, oldMachineSets, newMachineSet, deployment, oldISsMachinesUndergoingUpdate)
+	numOfMachinesSelectedForUpdate, err := dc.selectNumOfMachineForUpdate(ctx, allMachineSets, oldMachineSets, newMachineSet, deployment, oldMachineSetsMachinesUndergoingUpdate)
 	if err != nil {
 		return false, err
 	}
@@ -354,7 +367,7 @@ func (dc *controller) transferMachinesFromOldToNewMachineSet(ctx context.Context
 			// update the owner reference of the machine to the new machine set and update the labels
 			addControllerPatch := fmt.Sprintf(
 				`{"metadata":{"ownerReferences":[{"apiVersion":"machine.sapcloud.io/v1alpha1","kind":"%s","name":"%s","uid":"%s","controller":true,"blockOwnerDeletion":true}],"labels":%s,"uid":"%s"}}`,
-				v1alpha1.SchemeGroupVersion.WithKind("MachineSet"),
+				v1alpha1.SchemeGroupVersion.WithKind("MachineSet").Kind,
 				newMachineSet.GetName(), newMachineSet.GetUID(), string(labelsJSONBytes), oldMachine.UID)
 
 			err = dc.machineControl.PatchMachine(ctx, oldMachine.Namespace, oldMachine.Name, []byte(addControllerPatch))
@@ -379,7 +392,7 @@ func (dc *controller) transferMachinesFromOldToNewMachineSet(ctx context.Context
 			continue
 		}
 
-		klog.V(3).Infof("%d machine(s) transferred to new machine set. scaling down machine set %s", transferredMachineCount, oldMachineSet.Name)
+		klog.V(3).Infof("%d machine(s) transferred to new machine set. scaling down machine set %s to %d replicas", transferredMachineCount, oldMachineSet.Name, oldMachineSet.Spec.Replicas-transferredMachineCount)
 		_, _, err = dc.scaleMachineSetAndRecordEvent(ctx, oldMachineSet, oldMachineSet.Spec.Replicas-transferredMachineCount, deployment)
 		if err != nil {
 			klog.Errorf("scale down failed %s", err)
@@ -390,14 +403,14 @@ func (dc *controller) transferMachinesFromOldToNewMachineSet(ctx context.Context
 	return addedNewReplicasCount, nil
 }
 
-func (dc *controller) selectNumOfMachineForUpdate(ctx context.Context, allMachineSets []*v1alpha1.MachineSet, oldMachineSets []*v1alpha1.MachineSet, newMachineSet *v1alpha1.MachineSet, deployment *v1alpha1.MachineDeployment, oldISsMachinesUndergoingUpdate int32) (int32, error) {
+func (dc *controller) selectNumOfMachineForUpdate(ctx context.Context, allMachineSets []*v1alpha1.MachineSet, oldMachineSets []*v1alpha1.MachineSet, newMachineSet *v1alpha1.MachineSet, deployment *v1alpha1.MachineDeployment, oldMachineSetsMachinesUndergoingUpdate int32) (int32, error) {
 	maxUnavailable := MaxUnavailable(*deployment)
 
 	// Check if we can pick machines from old ISes for updating to new IS.
 	minAvailable := deployment.Spec.Replicas - maxUnavailable
 
 	// Find the number of available machines.
-	availableMachineCount := GetAvailableReplicaCountForMachineSets(allMachineSets) - oldISsMachinesUndergoingUpdate
+	availableMachineCount := GetAvailableReplicaCountForMachineSets(allMachineSets) - oldMachineSetsMachinesUndergoingUpdate
 	if availableMachineCount <= minAvailable {
 		// Cannot pick for updating.
 		return 0, nil