Improve behaviour of `Preserved` Node Condition by thiyyakat · Pull Request #1136 · gardener/machine-controller-manager

thiyyakat · 2026-08-25T09:28:52Z

What this PR does / why we need it:

Improves the behaviour of the Preserved node condition in the machine preservation flow:

Remove condition on preservation stop: Instead of setting the condition to ConditionFalse when preservation ends, the condition is now removed entirely.
Machine-code Reason field: The Reason field now follows the Kubernetes convention of using a CamelCase machine-readable token (PreservationInProgress, PreservationWithoutDrainCompleted, PreservationWithDrainCompleted, DrainFailed) so it can be used for programmatic branching. Human-readable context is moved to Message.
Early condition initialisation: The Preserved node condition is now set to PreservationInProgress before draining begins, so users can observe that MCM has acknowledged the preservation annotation and is acting on it. This also allows gardenlet health checks to exempt Failed preserved machines sooner thereby reducing alerting noise.
Message carries drain outcome and preserve-by info: The Message field now includes whether the node was auto-preserved by MCM or by the user, the drain outcome, and the expiry time.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
computeNewNodePreservedCondition is split into recomputePreservedNodeCondition (pure computation) and needsPreservedNodeConditionUpdate (dirty check), and a new RemoveConditionFromNode helper is added in pkg/util/nodeops/conditions.go.

Integration tests introduced in #1124 was run against the changes. All the tests pass.

Release note:

The `Preserved` node condition is initialised early (before drain) so operators can observe preservation progress, and it is removed (rather than set to `False`) when preservation stops.

gardener-prow · 2026-08-25T09:28:58Z

[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 ashwani2k 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

- Remove the Node Condition when preservation stops. - Modify Reason to follow convention so that it can be used for branching in the code. It is no longer in the form of a human-readable string. The Message field now has the human-readable string conveying preservation information. - Initialize the node condition before drain commences and update the node status with the condition so that the user is aware that MCM is responding to the preservation annotation. - Update tests. Signed-off-by: thiyyakat <meghana.thiyyakat@sap.com>

elankath · 2026-08-25T11:18:35Z

-	// PreservationStopped is a node condition reason to indicate that a machine/node preservation has been stopped due to annotation update or timeout
-	PreservationStopped string = "Preservation stopped."
+	// PreservationWithDrainCompleted is a node condition reason indicating the node has been drained and is fully preserved.
+	PreservationWithDrainCompleted string = "PreservationWithDrainCompleted"


Change to adjective since condition reasons indicate past tense and use 'and' PreservedAndDrained

elankath · 2026-08-25T11:27:19Z

-	// PreservedByMCM is a node condition reason for preservation of machines to indicate that the node is auto-preserved by MCM
-	PreservedByMCM string = "Preserved by MCM."
+	// PreservationInProgress is a node condition reason indicating preservation has started but is not yet complete.
+	PreservationInProgress string = "PreservationInProgress"


Why do we need this now ? It feels un-necessary with the other reasons.

What do you suggest as a Reason for the initial condition? It is set before drain starts.

I don't believe we need one since we override it anyways. Please correct me if I am wrong but we immediately call recomputePreservedNodeCondition which overwrites the same.

elankath · 2026-08-25T11:27:42Z

-	PreservedByUser string = "Preserved by user."
+	// PreservationWithoutDrainCompleted is a node condition reason indicating the node has not been drained but is fully preserved. This Reason is used
+	// when machines are preserved in Running, and the node need not be drained.
+	PreservationWithoutDrainCompleted string = "PreservationWithoutDrainCompleted"


Since node need not be drained, this should be reflected in the reason appropriately using an adjectivePreservedWithoutDrain

elankath · 2026-08-25T11:28:26Z

-	// PreservedNodeDrainUnsuccessful is a constant for the message in condition that indicates that the preserved node's drain was not successful
-	PreservedNodeDrainUnsuccessful string = "Preserved node could not be drained."
+	// DrainFailed is a node condition reason indicating the preserved node could not be drained.
+	DrainFailed string = "DrainFailed"


Why miss the Preserve prefix ? SHouldn't this be PreservedDrainFailed ?

elankath · 2026-08-25T11:29:17Z

+func RemoveConditionFromNode(ctx context.Context, c clientset.Interface, nodeName string, conditionType v1.NodeConditionType) (*v1.Node, error) {
+	firstTry := true
+	var updatedNode *v1.Node
+	err := clientretry.RetryOnConflict(Backoff, func() error {


we should avoid use of any new RetryOnConflict's introduced in the MCM codebase

Okay. Return an error in the case of conflict then? Or do you recommend doing a fetch from the API server each time?

Clarifying the point with team. I prefer just requeing similar to controller runtime but other ppl can have different opinions here.

elankath · 2026-08-25T11:35:55Z

+}
+
+// getInitializedPreservedNodeCondition returns an initialized Node Condition of Type Preserved
+func getInitializedPreservedNodeCondition(value string, preserveExpiryTime *metav1.Time) v1.NodeCondition {


This feels un-necessary as it it anyways overriden immediately in recomputePreservedNodeCondition

This will get overwritten only after drain completes, which can take a significant amount of time. The primary purpose of this PR was to add the node condition before drain starts so the gardenlet health checks exempt these nodes and don't unnecessarily mark the shoot as red, and the end user has an indication that their preservation request is being acted upon. Before this change, the user would receive an indication only once the drain finished.

elankath · 2026-08-25T11:36:23Z

-	return updatedNode, nil
+func (c *controller) addCAScaleDownDisabledAnnotationOnNode(ctx context.Context, nodeName string) error {
+	firstTry := true
+	return clientretry.RetryOnConflict(nodeops.Backoff, func() error {


We shouldn't add new RetryOnConflicts.

takoverflow

Thanks for the changes, have added some queries/suggestions PTAL

takoverflow · 2026-08-25T22:00:15Z

+	if drainErr != nil {
+		return machine, drainErr
+	}
+
+	if err != nil {
+		return machine, err
+	}


Suggested change

if drainErr != nil {

return machine, drainErr

}

if err != nil {

return machine, err

}

if drainErr != nil || err != nil {

return machine, cmp.Or(drainErr, err)

}

The two error checks can be combined, cmp.Or will go through the args and return the first non-zero/nil arg.

Oh neat. Thanks. Will make the change 👍

takoverflow · 2026-08-25T22:15:18Z

+	}
+
+	// Step 4: Update Preserved Node Condition with drain status if required
+	newCond := recomputePreservedNodeCondition(machine.Status.CurrentStatus, preserveValue, drainErr, existingNodePreservedCondition)


Just thinking out loud

Wouldn't it be beneficial to have a generic condition on the node that states that DrainSuccessful/DrainComplete or DrainInProgress as part of node drain.

Then the preservation with/without drain logic can depend on that, it will also enhance the observability of nodes in general. Rather than having drain related conditions only active for preservation, WDYT?

That's a good idea. But it should probably go into @r4mek's PR on drain and not in this. Along with those changes, we can change preservation logic to depend on the drain node condition instead.

My reservations:
It might be a stretch to correlate a standalone generic drain condition with another condition that talks about preservation, so I'm not sure it would really help is observability any more than adding the drain status to the preserved node condition.
Additionally we would also now need logic somewhere to eventually remove this drain condition, else it will get confusing when the node finally recovers.

Imo, yes, it might help simplify preservation logic, but if not done well can impact observability

Additionally we would also now need logic somewhere to eventually remove this drain condition, else it will get confusing when the node finally recovers.

I was thinking of a node condition that's only true when drain is in progress i.e. DrainInProgress with the state true only during RunDrain execution. Once drain finishes, it can either be removed or the state can transition to false.

The visibility I was referring to was also from an end-user PoV, regardless of drain being because of preservation or not. Where now they can easily look at a node to tell when it's being drained.

The other upside being that the preservation logic can also piggy-back on that.

It might be a stretch to correlate a standalone generic drain condition with another condition that talks about preservation

Isn't that what's indirectly happening right now? 😅 From #1136 (comment)

The Preserved node condition is now set to PreservationInProgress before draining begins, so users can observe that MCM has acknowledged the preservation annotation and is acting on it. This also allows gardenlet health checks to exempt Failed preserved machines sooner thereby reducing alerting noise.

You're right, depends on how much work it ends up being, if it's a lot then can be deferred for later.

I was thinking of a node condition that's only true when drain is in progress i.e. DrainInProgress with the state true only during RunDrain execution. Once drain finishes, it can either be removed or the state can transition to false.

Okay, fair. Earlier when you spoke about the drain condition having "DrainSuccessful/DrainComplete", it wasn't clear to me

It might be a stretch to correlate a standalone generic drain condition with another condition that talks about preservation

Isn't that what's indirectly happening right now? 😅 From #1136 (comment)

Um, yes and no. Whatever happens does happen within the same condition so hard to miss and iiuc is set specifically for draining

On second thought though. A node condition for drain would be nice. Seems more idiomatic. This is of course provided that after drain is completed, MCM removes it as preservation progresses.

@thiyyakat
Please correct me if I'm wrong, I did not thoroughly review the code.
Is the PreservationInProgress node condition reason only intended to be used when the node is draining? if yes then why not explicitly state that drain is in progress?

Just one thing, we cannot remove the drain condition on drain completion if we wish to use it for the preservation flow. We wouldn't know if drain has completed or if it was never attempted. So the condition would have to persist beyond drain completion which complicates things as @aaronfern pointed out.

The visibility I was referring to was also from an end-user PoV

Is this required today though? Have you felt during your DoD shifts or RCA that you would have benefited from this being surfaced? Then we should consider it.

I didn't quite get how the drain node condition will simplify preservation logic. In fact, we would need to fetch this condition and based on the condition do nearly the same branching. We may reduce the reasons associated with the preserved node condition ( by one) , but we will still need to introduce new reasons in the new node condition.

So I don't see any reason for the dependency between the conditions. If we see value in the drain node condition, it should be introduced. But I don't know if it should be for the sake of simplifying existing code. If we introduce this condition, it's not like we will change the InPlace flow or triggerDeletionFlow to rely on the node condition, right?

Isn't that what's indirectly happening right now? 😅 From #1136 (comment)

I don't understand what you mean here. How are we relying on another condition indirectly?

You're right, depends on how much work it ends up being, if it's a lot then can be deferred for later.

That depends on whether RunDrain is the only function that needs to be modified to do this. Moreover, if @r4mek is changing RunDrain, then there is no point doing it now. Maybe we can take a call based on that.

Is the PreservationInProgress node condition reason only intended to be used when the node is draining? if yes then why not explicitly state that drain is in progress?

@aaronfern

Good point. It is intended to be used even if there is no drain required. So maybe I can just set the condition reason to PreservedWithoutDrain directly, without initializing it, when drain is not required. When drain is required, I can set it to DrainInProgress before drain starts. We can do away with PreservationInProgress.

So the condition would have to persist beyond drain completion which complicates things

I'm sorry, what complications arise from persisting a DrainInProgress: false condition? Can you elaborate I'm not clear as to where the problem lies.

but we will still need to introduce new reasons in the new node condition.

What reasons are required? If its drain due to preservation, there are two conditions Preservation and DrainInProgress indicating that (with the latter not really needing any reason). If it's drain due to termination, the node only has one condition.

I don't understand what you mean here. How are we relying on another condition indirectly?

So IIUC, the entire reason for existence of PreservationInProgress as per #1136 (comment) is for the gardenlet to know that preservation is active and for user to be aware that because of node failure, drain is triggered.

Now, based on what kind of preservation annotation was added, what was the phase of the machine when preservation activated, the NodePreserved condition is being modified. This should IMO be independent of whether drain happened or not. Once preserveExpiryTime is set, the Node is essentially preserved, no matter what the condition Reason is, I would expect the condition to be added and not used as a state.

To elaborate a bit, the below checks would then be done based on the Drain condition, and not the NodePreserved condition Reason.

if existingNodePreservedCondition != nil && (existingNodePreservedCondition.Reason == v1alpha1.PreservationWithDrainCompleted || (existingNodePreservedCondition.Reason == v1alpha1.PreservationWithoutDrainCompleted && !drainRequired)) { return machine, nil } ... // Step 4: Update Preserved Node Condition with drain status if required newCond := recomputePreservedNodeCondition(machine.Status.CurrentStatus, preserveValue, drainErr, existingNodePreservedCondition) if needsPreservedNodeConditionUpdate(existingNodePreservedCondition, newCond) { _, err = nodeops.AddOrUpdateConditionsOnNode(ctx, c.targetCoreClient, nodeName, *newCond) if err != nil { klog.Errorf("error trying to update node preserved condition for node %q of machine %q : %v", nodeName, machine.Name, err) } }

Also Step 4 is not needed then as well.

Why is this condition being used as a preservation drain indicator? This muddling of concerns is something which I disliked.

That's why I proposed a separate indicator for any actor, be it MCM or gardenlet or the end-user to be aware when there's drain activity on a node.

If you still feel this is out of the scope of the Preserved node condition change, then it can be left for later whenever more Drain related changes are done. The only concern is, once a behavior is introduced there's a tendency to not change things a lot for fear of additional things breaking but it's fine.

what complications arise from persisting a DrainInProgress: false condition? Can you elaborate I'm not clear as to where the problem lies.

Say a machine has been preserved in Failed with preserve=when-failed, the node was drained successfully, then from what I understand, you wish to set node condition DrainInProgress to False. These are the extra things that need to be taken care of:

DrainInProgress being set to False implies drain has completed, not that it has never commenced. This would be a behaviour specific to this particular condition only and it will be a cognitive overhead since it cannot be inferred from the condition itself.

If say you accept the above semantics, then what would happen if the node got back to Ready, machine went to Running and the node failed again? You cannot interpret DrainInProgress at this point as an indicator for drain having completed.

I may not have fully understood your proposal, I apologise if the questions are silly 😅

What reasons are required?

Would you not want the new node condition to have any Reasons to enable branching on the state of drain? What is the use of the node condition for drain then? Just to indicate to the enduser that drain is ongoing? It also gives us a chance to branch in existing flows in a more kubernetes idiomatic way, right?

Out of curiosity, I checked if cluster-api has some indicator as well for draining and stumbled across this cluster-api/drain#observability

The drain process can be observed through the DrainingSucceeded condition on the Machine and various logs.

Regarding the questions:

DrainInProgress being set to False implies drain has completed, not that it has never commenced.

what would happen if the node got back to Ready, machine went to Running and the node failed again? You cannot interpret DrainInProgress at this point as an indicator for drain having completed.

If the node recovers, the condition can be removed as @aaronfern pointed out.

If the node never undergoes drain, the condition would not be present on it.

When the drain commences, its added with the state True and transitions to False upon completion.

As for how the preservation condition uses it, there's no need for it to be concerned about the Drain condition right? Once the PreserveExpiryTime is set, then the NodePreserved condition is also added. Now there can be two cases:

when-failed annotation: This implies that the condition is added only when the machine goes to failed state. The NodePreserved condition is added, drainPreservation is called and the Drain condition is added on the node as well.

now annotation: The NodePreserved condition is added and the Drain condition is only added when the machine actually goes to the Failed phase and the node drain is attempted.

In case of recovery post drain, the NodePreserved condition is still present since PreserveExpiryTime isn't breached but the Drain condition is removed since the node is no longer unschedulable.

This keeps the entire drain process and its progress independent of the NodePreservation conditions.

Just to indicate to the enduser that drain is ongoing?

Isn't that what the different reasons for the NodePreserved condition are achieving right now? Ideally, the condition should just be set to true once with the reason maybe stating why its preserved rather than saying PreservedAndDrained/PreservedWithoutDrain or PreservedDrainFailed.

The latter (drain) has nothing to do with Preservation.

This would be a behaviour specific to this particular condition only and it will be a cognitive overhead since it cannot be inferred from the condition itself.

Could you explain what cognitive overhead are you talking about and what do you wish to infer?

I apologise if the questions are silly

They aren't, it allows for clarification of the proposal, please don't hesitate to ask.

thiyyakat · 2026-09-01T10:59:39Z

/hold

The new node condition for drain needs to be introduced, and the code refactored.

thiyyakat requested a review from a team as a code owner August 25, 2026 09:28

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 25, 2026

thiyyakat added the kind/bug Bug label Aug 25, 2026

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

thiyyakat force-pushed the bug/node-condition-preserved branch from aa4c1ff to 86a9f5b Compare August 25, 2026 09:32

elankath reviewed Aug 25, 2026

View reviewed changes

elankath requested changes Aug 25, 2026

View reviewed changes

takoverflow reviewed Aug 25, 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 Sep 1, 2026

thiyyakat marked this pull request as draft September 4, 2026 05:11

gardener-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2026

Uh oh!

Conversation

thiyyakat commented Aug 25, 2026

Uh oh!

gardener-prow Bot commented Aug 25, 2026

Uh oh!

elankath Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

takoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

takoverflow Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

takoverflow Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

takoverflow Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

takoverflow Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiyyakat commented Sep 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

elankath Aug 25, 2026 •

edited

Loading

thiyyakat Aug 25, 2026 •

edited

Loading

takoverflow Aug 25, 2026 •

edited

Loading

takoverflow Aug 26, 2026 •

edited

Loading

thiyyakat Aug 26, 2026 •

edited

Loading

takoverflow Aug 26, 2026 •

edited

Loading

takoverflow Aug 26, 2026 •

edited

Loading