Skip to content

Improve behaviour of Preserved Node Condition #1136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
thiyyakat wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Improve behaviour of Preserved Node Condition #1136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 11 additions & 12 deletions pkg/apis/machine/v1alpha1/machine_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,24 +244,23 @@ const (
UpdateFailed string = "UpdateFailed"
)

// Constants used by the preservation flow.
const (
// NodePreserved is a node condition type for preservation of machines to allow end-user to know that a node is preserved
// NodePreserved is a node condition type that surfaces preservation information to the end-users.
NodePreserved corev1.NodeConditionType = "Preserved"

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

@elankath elankath Aug 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thiyyakat thiyyakat Aug 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@elankath elankath Aug 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// PreservedByUser is a node condition reason to indicate that a machine/node has been preserved due to explicit annotation by user
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"

@elankath elankath Aug 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

@elankath elankath Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// PreservedNodeDrainSuccessful is a constant for the message in condition that indicates that the preserved node's drain is successful
PreservedNodeDrainSuccessful string = "Preserved node drained successfully."

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

@elankath elankath Aug 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

// CurrentStatus contains information about the current status of Machine.
Expand Down
31 changes: 31 additions & 0 deletions pkg/util/nodeops/conditions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,37 @@ func AddOrUpdateConditionsOnNode(ctx context.Context, c clientset.Interface, nod
return updatedNode, err
}

// RemoveConditionFromNode removes the condition with the given type from the node's status.
// If the condition is not present, it is a no-op.
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 {

@elankath elankath Aug 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thiyyakat thiyyakat Aug 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@elankath elankath Aug 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var err error
var oldNode *v1.Node
if firstTry {
oldNode, err = c.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{ResourceVersion: "0"})
firstTry = false
} else {
oldNode, err = c.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{})
}
if err != nil {
return err
}
newNode := oldNode.DeepCopy()
conditions := make([]v1.NodeCondition, 0, len(newNode.Status.Conditions))
for _, cond := range newNode.Status.Conditions {
if cond.Type != conditionType {
conditions = append(conditions, cond)
}
}
newNode.Status.Conditions = conditions
updatedNode, err = UpdateNodeConditions(ctx, c, nodeName, oldNode, newNode)
return err
})
return updatedNode, err
}

// UpdateNodeConditions is for updating the node conditions from oldNode to the newNode
// using the node's UpdateStatus() method
func UpdateNodeConditions(ctx context.Context, c clientset.Interface, nodeName string, oldNode *v1.Node, newNode *v1.Node) (*v1.Node, error) {
Expand Down
12 changes: 6 additions & 6 deletions pkg/util/provider/machinecontroller/machine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4324,7 +4324,7 @@ var _ = Describe("machine", func() {
},
expect: expect{
preserveExpiryTimeIsSet: false,
nodeCondition: &corev1.NodeCondition{Type: v1alpha1.NodePreserved, Status: corev1.ConditionFalse},
nodeCondition: nil,
retry: machineutils.LongRetry,
},
}),
Expand All @@ -4339,7 +4339,7 @@ var _ = Describe("machine", func() {
},
expect: expect{
preserveExpiryTimeIsSet: false,
nodeCondition: &corev1.NodeCondition{Type: v1alpha1.NodePreserved, Status: corev1.ConditionFalse},
nodeCondition: nil,
retry: machineutils.LongRetry,
},
}),
Expand Down Expand Up @@ -4428,7 +4428,7 @@ var _ = Describe("machine", func() {
},
expect: expect{
preserveExpiryTimeIsSet: false,
nodeCondition: &corev1.NodeCondition{Type: v1alpha1.NodePreserved, Status: corev1.ConditionFalse},
nodeCondition: nil,
machineAnnotationValue: "",
nodeTainted: false,
retry: machineutils.LongRetry,
Expand Down Expand Up @@ -4467,7 +4467,7 @@ var _ = Describe("machine", func() {
},
expect: expect{
preserveExpiryTimeIsSet: false,
nodeCondition: &corev1.NodeCondition{Type: v1alpha1.NodePreserved, Status: corev1.ConditionFalse},
nodeCondition: nil,
machineAnnotationValue: "",
laNodePreserveValue: "",
retry: machineutils.LongRetry,
Expand All @@ -4486,7 +4486,7 @@ var _ = Describe("machine", func() {
},
expect: expect{
preserveExpiryTimeIsSet: false,
nodeCondition: &corev1.NodeCondition{Type: v1alpha1.NodePreserved, Status: corev1.ConditionFalse},
nodeCondition: nil,
machineAnnotationValue: machineutils.PreserveMachineAnnotationValueWhenFailed,
retry: machineutils.LongRetry,
nodeTainted: false,
Expand All @@ -4505,7 +4505,7 @@ var _ = Describe("machine", func() {
},
expect: expect{
preserveExpiryTimeIsSet: false,
nodeCondition: &corev1.NodeCondition{Type: v1alpha1.NodePreserved, Status: corev1.ConditionFalse},
nodeCondition: nil,
machineAnnotationValue: "",
retry: machineutils.LongRetry,
nodeTainted: false,
Expand Down
Loading
Loading