De-couple cordoning and draining by thiyyakat · Pull Request #1119 · gardener/machine-controller-manager

thiyyakat · 2026-07-15T11:44:35Z

What this PR does / why we need it:

This PR removes the cordoning activity from RunDrain allowing us to control the two activities separately. This also enables the cordoning during machine preservation's drain to be done without altering spec.Unschedulable, thereby ensuring a user's manual cordoning/uncordoning is always honoured.

The changes in the PR are:

Remove function RunCordonOrUncordon
Add a new function cordonNode() and call it in the callers of RunDrain().
For preservation of failed machines, use custom taint to cordon.
Add helper function updateMachineStatusForDrain to update machine status during drain across all drain-xxx functions.

Which issue(s) this PR fixes:
Fixes #1114

Special notes for your reviewer:

IT was run with provider-AWS

The changes were tested by running mcm and mcm-provider-aws locally, targeting a shoot cluster.

Scenarios tested:

Drain during rolling update
Drain during inplace update
Drain during scale-down

Release note:

De-couple cordoning and drain in the various operations that involve draining such as updates and machine preservation.

- Add a new function `cordonNode()` and call it in the callers of `RunDrain()``. - For preservation of failed machines, use custom taint to cordon. - Remove function `RunCordonOrUncordon` - Add helper function `updateMachineStatusForDrain` to update machine status during drain across all drain-xxx functions.

takoverflow

Thanks for the changes, would it be possible as part of this PR to combine the three separate drain methods? Since it's getting harder to keep track of which one does what specifically.

takoverflow · 2026-07-15T21:25:21Z

Since you've gotten rid of this skipDrain check elsewhere by returning early, why not do it here as well, this would allow for removal of another if-else at L1612. WDYT?

Addressed in 6f42253. The function returns early now when the node name is empty.

takoverflow · 2026-07-15T21:32:14Z

+		return nil
+	}
+	if node.Spec.Unschedulable {
+		klog.V(3).Infof("Scheduling state for node %q is already in desired state", node.Name)


Suggested change

klog.V(3).Infof("Scheduling state for node %q is already in desired state", node.Name)

klog.V(3).Infof("Node %q is already marked as Unschedulable", node.Name)

The log stating desired state without context doesn't really inform what the desired state actually is.

I've tried not to deviate from original behaviour. Will make this change since two of you feel this should be done 👍

Shall I make it "Node %q is already cordoned"? To maintain consistency with the remaining logs and comments?

Sure, that's better

Addressed in 96309f2.

takoverflow · 2026-07-15T21:36:34Z

+
+	// Step 4: remove preservation-related taint regardless of machine phase.
+	// If machine is in Running, workload can get scheduled onto it.
+	err = nodeops.RemoveTaintOffNode(ctx, c.targetCoreClient, updatedNode.Name, updatedNode, &v1.Taint{Key: machineutils.NodePreservedTaintKey, Effect: v1.TaintEffectNoSchedule})


Suggested change

err = nodeops.RemoveTaintOffNode(ctx, c.targetCoreClient, updatedNode.Name, updatedNode, &v1.Taint{Key: machineutils.NodePreservedTaintKey, Effect: v1.TaintEffectNoSchedule})

err = nodeops.RemoveTaintOffNode(ctx, c.targetCoreClient, updatedNode.Name, updatedNode, &v1.Taint{

Key: machineutils.NodePreservedTaintKey,

Effect: v1.TaintEffectNoSchedule,

})

What happens if c.targetCoreClient is nil?

takoverflow · 2026-07-15T21:44:24Z

+	// If machine is in Running, workload can get scheduled onto it.
+	err = nodeops.RemoveTaintOffNode(ctx, c.targetCoreClient, updatedNode.Name, updatedNode, &v1.Taint{Key: machineutils.NodePreservedTaintKey, Effect: v1.TaintEffectNoSchedule})
+	if err != nil {
+		return true, err


Why is this returning true?

Should not be returning true. Will make the change.

Changed return value to false. Addressed in b036aac.

takoverflow · 2026-07-15T21:47:04Z

 		}
 	}
-
+	if nodeName == "" {


This can be moved above the conditions check, since that is useless if the nodeName is ""

Addressed in b036aac.

takoverflow · 2026-07-15T21:47:58Z

 	if err == nil {
 		klog.V(3).Infof("(drainNode) For node %q, machine %q, nodeReadyCondition: %s, readOnlyFileSystemCondition: %s", nodeName, machine.Name, nodeReadyCondition, readOnlyFileSystemCondition)
 	} else if apierrors.IsNotFound(err) {
 		klog.Warningf("(drainNode) Node %q for machine %q doesn't exist, so drain will finish instantly", nodeName, machine.Name)


Suggested change

klog.Warningf("(drainNode) Node %q for machine %q doesn't exist, so drain will finish instantly", nodeName, machine.Name)

klog.Warningf("(drainNode) Node %q for machine %q doesn't exist. Skipping drain.", nodeName, machine.Name)

Addressed in 96309f2. I've made the changes across all drain functions.

takoverflow · 2026-07-15T21:49:39Z

 	}

+	// since we do not wish to accidentally uncordon a user-cordoned node after preservation stops,
+	// we add a taint with effect NoSchedule, before draining the node, instead of setting Spec.Unschedulable = true


Suggested change

// we add a taint with effect NoSchedule, before draining the node, instead of setting Spec.Unschedulable = true

// we add a taint with effect 'NoSchedule' before draining the node, instead of cordoning it.

Changed the comment. Addressed in b036aac.

aaronfern · 2026-07-16T10:20:56Z

/assign

aaronfern · 2026-07-16T11:21:29Z

+			} else { // regular drain already waits for vol detach and attach for another node.
+				description = fmt.Sprintf("Drain successful. %s", machineutils.InitiateVMDeletion)
+			}
+			err = fmt.Errorf("%s", description)


I do not see this err value being used anywhere, and it seems odd (or rather wrong) that a "Drain successful" message is added to an error

This is the original behaviour, if I am not wrong:

machine-controller-manager/pkg/util/provider/machinecontroller/machine_util.go

Line 1709 in 59134c3

err = fmt.Errorf("%s", description)

This is aligned with the other functions called in triggerDeletionFlow :

machine-controller-manager/pkg/util/provider/machinecontroller/machine_util.go

Line 1856 in 59134c3

err = fmt.Errorf("Machine deletion in process. %s", description)

machine-controller-manager/pkg/util/provider/machinecontroller/machine_util.go

Line 1944 in 59134c3

err = fmt.Errorf("machine deletion in process: %s", description)

In the original behaviour this was needed because err was returned. Since the final return uses fmt.Errorf("%s", description) now, err ends up not being used

You're right. The assignment is useless. Will make the change 👍

Removed the assignment in commit: cbdbb69

aaronfern · 2026-07-16T11:24:16Z

+	if err != nil {
+		// Deletion could be triggered when machine is just being created, no node present then
+		return nil
+	}


Use apierrors.IsNotFound(err) to filter out cases where node is not found. For all other errors, return the error

This is the behaviour of the earlier cordoning function:

machine-controller-manager/pkg/util/provider/drain/drain.go

Lines 1169 to 1174 in 59134c3

func (o *Options) RunCordonOrUncordon(ctx context.Context, desired bool) error {

node, err := o.client.CoreV1().Nodes().Get(ctx, o.nodeName, metav1.GetOptions{})

if err != nil {

// Deletion could be triggered when machine is just being created, no node present then

return nil

}

I can change this if you like.

Yes, please, I would like this change to be added.
Any other error should be caught and returned

Have changed cordonNode() (now moved to node.go) to return all errors except for NotFound errors. Addressed in : cbdbb69

aaronfern · 2026-07-16T11:37:14Z

would it be possible as part of this PR to combine the three separate drain methods? Since it's getting harder to keep track of which one does what specifically.

This is needed, but I think it should not be done in this PR. The scope and complexity will increase considerably. Not to mention additional time to properly test it.
I can open an issue for it, so that it can be picked up separately, and I can pick this up
wdyt? @takoverflow

- Replace "Cordoning" in descriptions with "Drain" on failure of `cordonNode()`. - Correct return value from `stopPreservationIfActive()`. - Move warning and early return due to empty node above condition check. - Change comment as per reviewer's suggestion. - Change error message on preservation-related taint failure to say "tainting" instead of "cordoning".

takoverflow · 2026-07-17T03:55:53Z

This is needed, but I think it should not be done in this PR.

Alright, fair enough. That can be tackled later.

- Improve readability of `RemoveTaintOffNode` function call in `stopPreservationIfActive`. - Correct warning on `NotFound` errors in drain functions. - Change log in `cordonNode` to improve clarity. - Add node details to warning when cordoning fails for the force-deletion case.

- Return early from `drainNode` when nodename is empty. Remove `skipDrain`.

gardener-prow · 2026-07-17T05:14:21Z

[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 ask for approval from aaronfern. 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

- Remove unnecessary assignment to `err`. - Move `cordonNode()` to `node.go` - Return `err` from `cordonNode()` for errors other than NotFound errors.

gagan16k · 2026-07-17T10:02:05Z

 }

+// cordonNode sets `node.Spec.Unschedulable` to true, if not already set to true
+func (c *controller) cordonNode(ctx context.Context, nodeName string) error {


The function below this one, uncordonNodeIfCordoned sets the value for spec.Unschedulable = false.
This could remove a manually set value, as manageMachinePreservation() calls this function when a machine goes from Failed back into Running.

Thanks. Will make the change. I think I'll put this PR on hold until #1118 gets merged. After that will take all this into consideration.

gagan16k · 2026-07-17T10:26:49Z

+	if err != nil {
+		return err
+	}
+	return nil


Suggested change

if err != nil {

return err

}

return nil

return err

Nit

Ha ha. Thanks! Good catch!

thiyyakat · 2026-07-17T13:08:57Z

/hold

Putting this PR on hold until #1118 gets merged. This PR will require some preservation related cordoning-uncordoning changes, and requires the changes in #1118 to be included first.

thiyyakat requested a review from a team as a code owner July 15, 2026 11:44

gardener-prow Bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 15, 2026

thiyyakat force-pushed the enh/separate-cordon-drain branch from 0a04d6a to 5299319 Compare July 15, 2026 11:52

thiyyakat force-pushed the enh/separate-cordon-drain branch from 5299319 to 49ad362 Compare July 15, 2026 12:17

thiyyakat changed the title ~~De-couple Cordoning and Draining~~ De-couple cordoning and draining Jul 15, 2026

takoverflow requested changes Jul 15, 2026

View reviewed changes

gardener-prow Bot assigned aaronfern Jul 16, 2026

aaronfern reviewed Jul 16, 2026

View reviewed changes

thiyyakat added 2 commits July 17, 2026 10:05

Address reviewer comments - part 3

6f42253

- Return early from `drainNode` when nodename is empty. Remove `skipDrain`.

gardener-prow Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 17, 2026

Address reviewer comments - part 4

cbdbb69

- Remove unnecessary assignment to `err`. - Move `cordonNode()` to `node.go` - Return `err` from `cordonNode()` for errors other than NotFound errors.

gagan16k reviewed Jul 17, 2026

View reviewed changes

gardener-prow Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2026

	klog.V(3).Infof("Scheduling state for node %q is already in desired state", node.Name)
	klog.V(3).Infof("Node %q is already marked as Unschedulable", node.Name)

	klog.Warningf("(drainNode) Node %q for machine %q doesn't exist, so drain will finish instantly", nodeName, machine.Name)
	klog.Warningf("(drainNode) Node %q for machine %q doesn't exist. Skipping drain.", nodeName, machine.Name)

	// we add a taint with effect NoSchedule, before draining the node, instead of setting Spec.Unschedulable = true
	// we add a taint with effect 'NoSchedule' before draining the node, instead of cordoning it.

	func (o *Options) RunCordonOrUncordon(ctx context.Context, desired bool) error {
	node, err := o.client.CoreV1().Nodes().Get(ctx, o.nodeName, metav1.GetOptions{})
	if err != nil {
	// Deletion could be triggered when machine is just being created, no node present then
	return nil
	}

Uh oh!

Conversation

thiyyakat commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

takoverflow 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!

Choose a reason for hiding this comment

Uh oh!

aaronfern commented Jul 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

aaronfern commented Jul 16, 2026

Uh oh!

takoverflow commented Jul 17, 2026

Uh oh!

gardener-prow Bot commented Jul 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiyyakat commented Jul 15, 2026 •

edited

Loading