Decouple Cordoning from Draining to Avoid Uncordoning User-Cordoned Nodes in Preservation Flow 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
Drain when a machine transitions to Failed and is preserved
Drain when a preserved machine transitions to Failed
kubectl delete mc <machine-name> after labelling machine with force-deletion:True
Drain when machineDrainTimeout lapses (force drain)

Release note:

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

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.

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

/assign

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

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.

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

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 · 2026-07-27T11:49:08Z

/unhold

The PR has been rebased with the preservation flow changes in master. The manual tests mentioned in the PR description have been repeated with the latest changes.

gagan16k · 2026-07-28T09:11:04Z

/lgtm

Thanks for the work on preservation fixes @thiyyakat !

gardener-prow · 2026-07-28T09:11:28Z

LGTM label has been added.

Details

Git tree hash: 502c0449a9b41b7da9426dac74e423c58864bfc6

- 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.

- 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".

- 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`.

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

- Remove `func uncordonNodeIfCordoned`. It is no longer needed. - Modify preservation tests to check for taint - Remove irrelevant code in `drainPreservedNode`. - Remove `getNodePreserveAnnotationValue` since it is not used. - Correct error return statement in `cordonNode`.

…ved with preserve=now in the Running phase, transitions to Failed. This results in the node not being tainted or drained.

Co-authored-by: Gagan <gagan.katla@sap.com>

takoverflow

Just one minor nit, thanks for the changes!

All comments already addressed

Add log for successful cordoning of node.

aaronfern · 2026-08-11T10:11:39Z

Changes seem fine to me, thanks for making the changes!
/lgtm

gardener-prow · 2026-08-11T10:12:00Z

LGTM label has been added.

Details

Git tree hash: 50e773f057fe93069bd9805d6d6ed8ed0b4b6ef0

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 2 times, most recently 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 previously requested changes Jul 15, 2026

View reviewed changes

gardener-prow Bot assigned aaronfern Jul 16, 2026

aaronfern reviewed Jul 16, 2026

View reviewed changes

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

gagan16k reviewed Jul 17, 2026

View reviewed changes

Comment thread pkg/util/provider/machinecontroller/node.go

Comment thread pkg/util/provider/machinecontroller/node.go

gardener-prow Bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 17, 2026

thiyyakat force-pushed the enh/separate-cordon-drain branch from cbdbb69 to 164b58d Compare July 27, 2026 08:33

gardener-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2026

thiyyakat force-pushed the enh/separate-cordon-drain branch 2 times, most recently from a8e0897 to 117fb4b Compare July 27, 2026 08:34

thiyyakat requested review from aaronfern, gagan16k and takoverflow July 27, 2026 11:47

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

r4mek reviewed Jul 28, 2026

View reviewed changes

Comment thread pkg/util/provider/machinecontroller/machine_util.go

thiyyakat changed the title ~~De-couple cordoning and draining~~ Decouple Cordoning from Draining to Avoid Uncordoning User-Cordoned Nodes in Preservation Flow Jul 28, 2026

thiyyakat added the kind/bug Bug label Jul 28, 2026

gardener-prow Bot removed the do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Jul 28, 2026

gagan16k reviewed Jul 28, 2026

View reviewed changes

Comment thread pkg/util/provider/machinecontroller/node.go

Comment thread pkg/util/provider/machinecontroller/machine_util.go Outdated

gardener-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 28, 2026

gardener-prow Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. labels Jul 28, 2026

thiyyakat and others added 9 commits August 5, 2026 09:52

Address reviewer comments - part 3

60846d9

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

Address reviewer comments - part 4

8974502

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

Fix bug in preserveMachine that returns early when a machine preser…

8899239

…ved with preserve=now in the Running phase, transitions to Failed. This results in the node not being tainted or drained.

Fix comment for clarity.

2f63561

Co-authored-by: Gagan <gagan.katla@sap.com>

Improve logs, and revert to original pattern of returning errors.

344631d

thiyyakat force-pushed the enh/separate-cordon-drain branch from 0baf17e to 344631d Compare August 5, 2026 05:17

gardener-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2026

takoverflow reviewed Aug 6, 2026

View reviewed changes

Comment thread pkg/util/provider/machinecontroller/machine_util.go

aaronfern reviewed Aug 7, 2026

View reviewed changes

Comment thread pkg/util/provider/machinecontroller/node.go Outdated

Comment thread pkg/util/provider/machinecontroller/machine_util.go

Address reviewer comments - part 6

d9580d3

Add log for successful cordoning of node.

thiyyakat force-pushed the enh/separate-cordon-drain branch from 6f3a032 to d9580d3 Compare August 10, 2026 07:24

thiyyakat requested a review from aaronfern August 11, 2026 04:48

gardener-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 11, 2026

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aaronfern commented Jul 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

thiyyakat commented Jul 17, 2026

Uh oh!

thiyyakat commented Jul 27, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gagan16k commented Jul 28, 2026

Uh oh!

gardener-prow Bot commented Jul 28, 2026

Uh oh!

takoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aaronfern commented Aug 11, 2026

Uh oh!

gardener-prow Bot commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thiyyakat commented Jul 15, 2026 •

edited

Loading