Fixing the issue where a rapid scale up and scale down can result in a cordoned machine in the cluster. by r4mek · Pull Request #1087 · gardener/machine-controller-manager

r4mek · 2026-03-16T08:40:09Z

What this PR does / why we need it:
This PR contains a fix for the issue where a rapid scale up and scale down can result in a cordoned machine in the cluster.

The case happens when CA scales down a MCD followed quickly with scaling up the same MCD before the MCD controller notices the replica change and start reconciliation. The result is the MCD ending up with the same number of replicas that it started off with, causing the MCD controller not to have any work to do since from it's point of view, the MCD replicas have not changed.
As part of the scale down operation, CA will cordon an underutilised node. This node just remains part of the cluster as MCM makes no further attempt to remove it

Which issue(s) this PR fixes:
Fixes #1085, #1084

Special notes for your reviewer:
There is also a PR raised on gardener/autoscaler which has the changes from the autoscaler and is also required to fix the issue. PR(gardener/autoscaler#401)

Release note:

Fixing an issue where a rapid scale up and scale down can result in a cordoned machine in the cluster.

gardener-prow · 2026-03-16T08:40:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign elankath for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

r4mek · 2026-03-16T09:46:44Z

/kind bug

thiyyakat

Hi @r4mek ! Just a few small suggestions. PTAL. Thanks!

thiyyakat · 2026-03-17T10:47:17Z

pkg/util/provider/machineutils/utils.go

 	TriggerDeletionByMCM = "node.machine.sapcloud.io/trigger-deletion-by-mcm"

+	// LastReplicaChangeAnnotation contains the timestamp of last replica change in the machine deployment.
+	// This annotation is used so that MCS only deletes the machines for which it has observed change in replicas.:w


Suggested change

// This annotation is used so that MCS only deletes the machines for which it has observed change in replicas.:w

// This annotation is used so that MCS only deletes the machines for which it has observed change in replicas.

thiyyakat · 2026-03-17T10:50:17Z

pkg/controller/machineset_test.go

+			}
+		})
+
+		It("It should delete the marked machines with no change in machineSet replicas", func() {


You don't need to use "It" in the description.

Suggested change

It("It should delete the marked machines with no change in machineSet replicas", func() {

It("should delete the marked machines with no change in machineSet replicas", func() {

thiyyakat · 2026-03-17T10:50:41Z

pkg/controller/machineset_test.go

+			Expect(k8sError.IsNotFound(Err2)).Should(BeTrue())
+		})
+
+		It("It should delete the marked machines after reducing change in machineSet replicas", func() {


Same here. No need to say "It".

thiyyakat · 2026-03-17T11:20:33Z

pkg/controller/deployment_test.go

+				Expect(machine2.Annotations[machineutils.MachinePriority]).To(Equal("1"))
+				Expect(machine2.Annotations[machineutils.LastReplicaChangeAnnotation]).To(Equal(ts))
+			},
+			Entry("mark the machines named in TriggerDeletionByMCM annotation for deletion by setting their MachinePriority annotation to 1 and also set the LastReplicaChangeAnnotation on them same as that is passed with the annotation",


Suggested change

Entry("mark the machines named in TriggerDeletionByMCM annotation for deletion by setting their MachinePriority annotation to 1 and also set the LastReplicaChangeAnnotation on them same as that is passed with the annotation",

Entry("mark the machines named in TriggerDeletionByMCM annotation for deletion by setting their MachinePriority annotation to 1 and also set their LastReplicaChangeAnnotation to the same value passed with the annotation",

thiyyakat · 2026-03-17T11:24:12Z

pkg/controller/deployment.go

 	_, err := dc.controlMachineClient.MachineDeployments(mcd.Namespace).Update(ctx, mcdAdjust, metav1.UpdateOptions{})
 	if err != nil {
-		klog.Errorf("Failed to update MachineDeployment %q with #%d machine names still pending deletion, triggerDeletionAnnotValue=%q", mcd.Name, len(triggerForDeletionMachineNames), triggerDeletionAnnotValue)
+		klog.Errorf("Failed to update MachineDeployment %q with #%d machine names still pending deletion, triggerDeletionAnnotValue=%q", mcd.Name, len(triggerForDeletionMachineNamesWithTS), triggerDeletionAnnotValue)


Suggested change

klog.Errorf("Failed to update MachineDeployment %q with #%d machine names still pending deletion, triggerDeletionAnnotValue=%q", mcd.Name, len(triggerForDeletionMachineNamesWithTS), triggerDeletionAnnotValue)

klog.Errorf("failed to update MachineDeployment %q with #%d machine names still pending deletion, triggerDeletionAnnotValue=%q", mcd.Name, len(triggerForDeletionMachineNamesWithTS), triggerDeletionAnnotValue)

thiyyakat · 2026-03-17T11:26:11Z

pkg/controller/machineset.go

+	scaleDownMachines := []*v1alpha1.Machine{}
+	if machineSet.Annotations[machineutils.LastReplicaChangeAnnotation] != "" {
+		machineSetLRCA, err := time.Parse(time.RFC3339, machineSet.Annotations[machineutils.LastReplicaChangeAnnotation])
+		if err == nil {
+			for _, machine := range allMachines {
+				if machine.Annotations[machineutils.LastReplicaChangeAnnotation] == "" || machine.Annotations[machineutils.MachinePriority] != "1" {
+					continue
+				}
+				machineLRCA, err := time.Parse(time.RFC3339, machine.Annotations[machineutils.LastReplicaChangeAnnotation])
+				if err != nil {
+					continue
+				}
+				if machineLRCA.Before(machineSetLRCA) || machineLRCA.Equal(machineSetLRCA) {
+					scaleDownMachines = append(scaleDownMachines, machine)
+				}
+			}


Could you please add a comment here explaining the change?

thiyyakat · 2026-03-17T11:32:12Z

pkg/controller/machineset.go


+	scaleDownMachines := []*v1alpha1.Machine{}
+	if machineSet.Annotations[machineutils.LastReplicaChangeAnnotation] != "" {
+		machineSetLRCA, err := time.Parse(time.RFC3339, machineSet.Annotations[machineutils.LastReplicaChangeAnnotation])


The err is not handled. Maybe log the err as a warning here?

takoverflow · 2026-03-18T04:46:03Z

pkg/controller/machineset.go

+				}
+			}
+			if len(scaleDownMachines) >= 1 {
+				klog.V(3).Infof("Deleting stale machines %s", getMachineKeys(scaleDownMachines))


Please change these logs to not call them stale machines.
Similarly for the below error for the terminate call as well.

aaronfern · 2026-03-18T05:08:37Z

pkg/controller/deployment.go

-	for _, machineName := range triggerForDeletionMachineNames {
+	klog.V(3).Infof("MachineDeployment %q has #%d machine(s) marked for deletion, triggerForDeletionMachineNames=%v", mcd.Name, len(triggerForDeletionMachineNamesWithTS), triggerForDeletionMachineNamesWithTS)
+	for _, machineNameWithTS := range triggerForDeletionMachineNamesWithTS {
+		parts := strings.Split(machineNameWithTS, "~")


Add a warning is len(parts) != 2 and continue to the next if this is the case

aaronfern · 2026-03-18T05:11:08Z

pkg/controller/deployment.go

+	klog.V(3).Infof("MachineDeployment %q has #%d machine(s) marked for deletion, triggerForDeletionMachineNames=%v", mcd.Name, len(triggerForDeletionMachineNamesWithTS), triggerForDeletionMachineNamesWithTS)
+	for _, machineNameWithTS := range triggerForDeletionMachineNamesWithTS {
+		parts := strings.Split(machineNameWithTS, "~")
+		machineName, machineDeletionTS := parts[0], parts[1]


Please parse machineDeletionTS to ensure it is a valid timestamp. if it is not, log it and continue to the next

Also, if annotation is invalid, please add machine to skipTriggerForDeletionMachineNames

aaronfern · 2026-03-18T05:17:43Z

pkg/controller/deployment.go

 			mcAdjust.Annotations = make(map[string]string)
 		}
 		mcAdjust.Annotations[machineutils.MachinePriority] = "1"
+		mcAdjust.Annotations[machineutils.LastReplicaChangeAnnotation] = machineDeletionTS


Add a check to not update LastReplicaChangeAnnotation if already set
Also add a log line is TS is already set and continue

aaronfern · 2026-03-18T05:34:28Z

pkg/controller/deployment.go

@@ -643,22 +644,24 @@ func (dc *controller) updateMachineDeploymentFinalizers(ctx context.Context, mac
 }

 func (dc *controller) setMachinePriorityAnnotationAndUpdateTriggeredForDeletion(ctx context.Context, mcd *v1alpha1.MachineDeployment) error {


Add another function that is called by this one func ComputeMachineTriggerDeletionData(mcd) triggerDeletionData that takes care of computing the machineNames/timestamps and other data necessary for application from the trigger deletionAnnotation.

The struct can be as follows

type triggerDeletionData struct{ machineNamesWithTS []string skipMachineNamesWithTS []string machineMarkedDeletionTimes map[string]time.Time (consider map[string]string based on implementation) }

Also, add another method on this structure to give you annotations that you should add to the machineDeployment. This method can be called latestTriggerDeletionAnnotationValue()

aaronfern · 2026-03-18T05:36:41Z

pkg/controller/deployment.go

@@ -643,22 +644,24 @@ func (dc *controller) updateMachineDeploymentFinalizers(ctx context.Context, mac
 }

 func (dc *controller) setMachinePriorityAnnotationAndUpdateTriggeredForDeletion(ctx context.Context, mcd *v1alpha1.MachineDeployment) error {


Please rename this function to be more accurate to what it is doing

Suggested change

func (dc *controller) setMachinePriorityAnnotationAndUpdateTriggeredForDeletion(ctx context.Context, mcd *v1alpha1.MachineDeployment) error {

func (dc *controller) updateMachineAndMachineDeploymentDeletionAnnotations(ctx context.Context, mcd *v1alpha1.MachineDeployment) error {

aaronfern · 2026-03-18T05:51:46Z

pkg/controller/deployment_sync.go

 	var err error
 	if sizeNeedsUpdate || annotationsNeedUpdate {
 		isCopy.Spec.Replicas = newScale
+		isCopy.Annotations[machineutils.LastReplicaChangeAnnotation] = deployment.Annotations[machineutils.LastReplicaChangeAnnotation]


Please change the annotation name to machine.sapcloud.io/last-deployment-replica-change-time-by-scaler
This is to be used on machineDeployments and machineSets only

aaronfern · 2026-03-18T06:03:32Z

pkg/controller/deployment.go

 			mcAdjust.Annotations = make(map[string]string)
 		}
 		mcAdjust.Annotations[machineutils.MachinePriority] = "1"
+		mcAdjust.Annotations[machineutils.LastReplicaChangeAnnotation] = machineDeletionTS


Please add a new annotation to be used on machines. It can be called machine.sapcloud.io/mark-deletion-time

aaronfern · 2026-03-18T06:13:34Z

pkg/controller/machineset.go

+	scaleDownMachines := []*v1alpha1.Machine{}
+	if machineSet.Annotations[machineutils.LastReplicaChangeAnnotation] != "" {
+		machineSetLRCA, err := time.Parse(time.RFC3339, machineSet.Annotations[machineutils.LastReplicaChangeAnnotation])
+		if err == nil {
+			for _, machine := range allMachines {
+				if machine.Annotations[machineutils.LastReplicaChangeAnnotation] == "" || machine.Annotations[machineutils.MachinePriority] != "1" {
+					continue
+				}
+				machineLRCA, err := time.Parse(time.RFC3339, machine.Annotations[machineutils.LastReplicaChangeAnnotation])
+				if err != nil {
+					continue
+				}
+				if machineLRCA.Before(machineSetLRCA) || machineLRCA.Equal(machineSetLRCA) {
+					scaleDownMachines = append(scaleDownMachines, machine)
+				}
+			}
+			if len(scaleDownMachines) >= 1 {
+				klog.V(3).Infof("Deleting stale machines %s", getMachineKeys(scaleDownMachines))
+				if err := c.terminateMachines(ctx, scaleDownMachines, machineSet); err != nil {
+					klog.Errorf("failed to terminate stale machines for machineset %s: %v", machineSet.Name, err)
+				}
+			}
+		}
+	}
+


Move this to a function. Accept allMachines and machineSet.Annotations[machineutils.LastReplicaChangeAnnotation], and it should return a list of machineNames to delete

aaronfern · 2026-03-18T06:50:42Z

pkg/controller/machineset.go

 // manageReplicas checks and updates replicas for the given MachineSet.
 // Does NOT modify <filteredMachines>.
 // It will requeue the machine set in case of an error while creating/deleting machines.
 func (c *controller) manageReplicas(ctx context.Context, allMachines []*v1alpha1.Machine, machineSet *v1alpha1.MachineSet) error {


As part of manageReplicas, please make the following 2 changes

Update the sorting function in getMachinesToDelete() to now consider the timestamp set on the mark-for-deletion annotation

Do not delete machines inside the machinesWithoutUpdateSuccessfulLabelDiff > 0 block, but rather store all the machined intended to be deleted in a list and delete them later in at the end of the method after also computing stale machines

r4mek added 3 commits March 13, 2026 14:50

checkpoint

0ae8cba

adding unit tests

9e61509

fixing tests

e968e1a

r4mek requested a review from a team as a code owner March 16, 2026 08:40

gardener-prow bot added the do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Mar 16, 2026

gardener-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. labels Mar 16, 2026

r4mek added 2 commits March 16, 2026 14:16

fixing linter errors

450a176

linter fix

829bf73

r4mek mentioned this pull request Mar 16, 2026

Fixing the issue where a rapid scale up and scale down can result in a cordoned machine in the cluster gardener/autoscaler#401

Open

gardener-prow bot added kind/bug Bug and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Mar 16, 2026

nits

88529b7

thiyyakat reviewed Mar 17, 2026

View reviewed changes

takoverflow reviewed Mar 18, 2026

View reviewed changes

aaronfern reviewed Mar 18, 2026

View reviewed changes

	// This annotation is used so that MCS only deletes the machines for which it has observed change in replicas.:w
	// This annotation is used so that MCS only deletes the machines for which it has observed change in replicas.

	It("It should delete the marked machines with no change in machineSet replicas", func() {
	It("should delete the marked machines with no change in machineSet replicas", func() {

	Entry("mark the machines named in TriggerDeletionByMCM annotation for deletion by setting their MachinePriority annotation to 1 and also set the LastReplicaChangeAnnotation on them same as that is passed with the annotation",
	Entry("mark the machines named in TriggerDeletionByMCM annotation for deletion by setting their MachinePriority annotation to 1 and also set their LastReplicaChangeAnnotation to the same value passed with the annotation",

	klog.Errorf("Failed to update MachineDeployment %q with #%d machine names still pending deletion, triggerDeletionAnnotValue=%q", mcd.Name, len(triggerForDeletionMachineNamesWithTS), triggerDeletionAnnotValue)
	klog.Errorf("failed to update MachineDeployment %q with #%d machine names still pending deletion, triggerDeletionAnnotValue=%q", mcd.Name, len(triggerForDeletionMachineNamesWithTS), triggerDeletionAnnotValue)

		@@ -643,22 +644,24 @@ func (dc *controller) updateMachineDeploymentFinalizers(ctx context.Context, mac
		}

		func (dc controller) setMachinePriorityAnnotationAndUpdateTriggeredForDeletion(ctx context.Context, mcd v1alpha1.MachineDeployment) error {

	func (dc controller) setMachinePriorityAnnotationAndUpdateTriggeredForDeletion(ctx context.Context, mcd v1alpha1.MachineDeployment) error {
	func (dc controller) updateMachineAndMachineDeploymentDeletionAnnotations(ctx context.Context, mcd v1alpha1.MachineDeployment) error {

Conversation

r4mek commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gardener-prow bot commented Mar 16, 2026

Uh oh!

r4mek commented Mar 16, 2026

Uh oh!

thiyyakat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

r4mek commented Mar 16, 2026 •

edited

Loading