From 44e82f93b8d16cf0a2f03eeee074a77e30d5b8f3 Mon Sep 17 00:00:00 2001 From: thiyyakat Date: Thu, 9 Jul 2026 22:57:07 +0530 Subject: [PATCH 1/8] De-couple cordoning and drain - 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. --- pkg/util/provider/drain/drain.go | 27 --- .../machinecontroller/machine_util.go | 222 +++++++++++------- .../machinecontroller/machine_util_test.go | 201 +++++++++++++++- pkg/util/provider/machineutils/utils.go | 4 + 4 files changed, 343 insertions(+), 111 deletions(-) diff --git a/pkg/util/provider/drain/drain.go b/pkg/util/provider/drain/drain.go index 2dba1a1966..2b6103f765 100644 --- a/pkg/util/provider/drain/drain.go +++ b/pkg/util/provider/drain/drain.go @@ -235,10 +235,6 @@ func (o *Options) RunDrain(ctx context.Context) error { ) }() - if err := o.RunCordonOrUncordon(drainContext, true); err != nil { - klog.Errorf("Drain Error: Cordoning of node failed with error: %v", err) - return err - } if !cache.WaitForCacheSync(drainContext.Done(), o.podSynced) { err := fmt.Errorf("timed out waiting for pod cache to sync") return err @@ -1164,29 +1160,6 @@ func SupportEviction(clientset kubernetes.Interface) (string, error) { return "", nil } -// RunCordonOrUncordon runs either Cordon or Uncordon. The desired value for -// "Unschedulable" is passed as the first arg. -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 - } - unsched := node.Spec.Unschedulable - if unsched == desired { - klog.V(3).Infof("Scheduling state for node %q is already in desired state", node.Name) - } else { - clone := node.DeepCopy() - clone.Spec.Unschedulable = desired - - _, err = o.client.CoreV1().Nodes().Update(ctx, clone, metav1.UpdateOptions{}) - if err != nil { - return err - } - } - return nil -} - func getPdbForPod(pdbLister policyv1listers.PodDisruptionBudgetLister, pod *corev1.Pod) *policyv1.PodDisruptionBudget { // GetPodPodDisruptionBudgets returns an error only if no PodDisruptionBudgets are found. // We don't return that as an error to the caller. diff --git a/pkg/util/provider/machinecontroller/machine_util.go b/pkg/util/provider/machinecontroller/machine_util.go index 770c9a5620..8497172ca2 100644 --- a/pkg/util/provider/machinecontroller/machine_util.go +++ b/pkg/util/provider/machinecontroller/machine_util.go @@ -415,22 +415,7 @@ func (c *controller) inPlaceUpdate(ctx context.Context, machine *v1alpha1.Machin func (c *controller) updateMachineStatusAndNodeCondition(ctx context.Context, machine *v1alpha1.Machine, description string, state v1alpha1.MachineState, drainError error) (machineutils.RetryPeriod, error) { if drainError != nil { - updateRetryPeriod, updateErr := c.machineStatusUpdate( - ctx, - machine, - v1alpha1.LastOperation{ - Description: description, - State: state, - Type: v1alpha1.MachineOperationDrainNode, - LastUpdateTime: metav1.Now(), - }, - // Let the clone.Status.CurrentStatus (LastUpdateTime) be as it was before. - // This helps while computing when the drain timeout to determine if force deletion is to be triggered. - // Ref - https://github.com/gardener/machine-controller-manager/blob/rel-v0.34.0/pkg/util/provider/machinecontroller/machine_util.go#L872 - machine.Status.CurrentStatus, - machine.Status.LastKnownState, - ) - + updateRetryPeriod, updateErr := c.updateMachineStatusForDrain(ctx, machine, description, state, v1alpha1.MachineOperationDrainNode) if updateErr != nil { return updateRetryPeriod, updateErr } @@ -1504,6 +1489,13 @@ func (c *controller) drainNodeForInPlace(ctx context.Context, machine *v1alpha1. ) } + if err = c.cordonNode(ctx, nodeName); err != nil { + klog.Errorf("cordoning of backing node %q for machine %q, with providerID %q, failed with error: %v", nodeName, machine.Name, getProviderID(machine), err) + description = fmt.Sprintf("Cordoning failed due to - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) + state = v1alpha1.MachineStateProcessing + return c.updateMachineStatusAndNodeCondition(ctx, machine, description, state, err) + } + buf := bytes.NewBuffer([]byte{}) errBuf := bytes.NewBuffer([]byte{}) @@ -1655,83 +1647,131 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver // update node with the machine's phase prior to termination if err = c.UpdateNodeTerminationCondition(ctx, machine); err != nil { - if forceDeleteMachine { - klog.Warningf("Failed to update node conditions: %v. However, since it's a force deletion shall continue deletion of VM.", err) - } else { + if !forceDeleteMachine { klog.Errorf("drain failed due to failure in update of node conditions: %v", err) description = fmt.Sprintf("Drain failed due to failure in update of node conditions - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) state = v1alpha1.MachineStateFailed - skipDrain = true + updateRetryPeriod, updateErr := c.updateMachineStatusForDrain(ctx, machine, description, state, v1alpha1.MachineOperationDelete) + if updateErr != nil { + return updateRetryPeriod, updateErr + } + return machineutils.ShortRetry, err } + klog.Warningf("Failed to update node conditions: %v. However, since it's a force deletion shall continue deletion of VM.", err) } - if !skipDrain { - buf := bytes.NewBuffer([]byte{}) - errBuf := bytes.NewBuffer([]byte{}) - - drainOptions := drain.NewDrainOptions( - c.targetCoreClient, - c.targetKubernetesVersion, - timeOutDuration, - maxEvictRetries, - pvDetachTimeOut, - pvReattachTimeOut, - nodeName, - -1, - forceDeletePods, - true, - true, - true, - buf, - errBuf, - c.driver, - c.pvcLister, - c.pvLister, - c.pdbLister, - c.nodeLister, - c.podLister, - c.volumeAttachmentHandler, - c.podSynced, - ) - klog.V(3).Infof("(drainNode) Invoking RunDrain, forceDeleteMachine: %t, forceDeletePods: %t, timeOutDuration: %s", forceDeletePods, forceDeleteMachine, timeOutDuration) - err = drainOptions.RunDrain(ctx) - if err == nil { - // Drain successful - klog.V(2).Infof("Drain successful for machine %q ,providerID %q, backing node %q. \nBuf:%v \nErrBuf:%v", machine.Name, getProviderID(machine), getNodeName(machine), buf, errBuf) - - if forceDeletePods { - description = fmt.Sprintf("Force Drain successful. %s", machineutils.DelVolumesAttachments) - } 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) - state = v1alpha1.MachineStateProcessing - - // Return error even when machine object is updated - } else if err != nil && forceDeleteMachine { - // Drain failed on force deletion - klog.Warningf("Drain failed for machine %q. However, since it's a force deletion shall continue deletion of VM. \nBuf:%v \nErrBuf:%v \nErr-Message:%v", machine.Name, buf, errBuf, err) - + if err = c.cordonNode(ctx, nodeName); err != nil { + if forceDeleteMachine { + // Cordoning failed on force deletion + klog.Warningf("Cordoning failed for machine %q. However, since it's a force deletion shall continue deletion of VM. \nErr-Message:%v", machine.Name, err) description = fmt.Sprintf("Drain failed due to - %s. However, since it's a force deletion shall continue deletion of VM. %s", err.Error(), machineutils.DelVolumesAttachments) state = v1alpha1.MachineStateProcessing } else { - klog.Errorf("drain failed for machine %q , providerID %q ,backing node %q. \nBuf:%v \nErrBuf:%v \nErr-Message:%v", machine.Name, getProviderID(machine), getNodeName(machine), buf, errBuf, err) - - description = fmt.Sprintf("Drain failed due to - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) + klog.Errorf("cordoning of backing node %q for machine %q, with providerID %q, failed with error: %v", nodeName, machine.Name, getProviderID(machine), err) + description = fmt.Sprintf("Cordoning failed due to - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) state = v1alpha1.MachineStateFailed } + updateRetryPeriod, updateErr := c.updateMachineStatusForDrain(ctx, machine, description, state, v1alpha1.MachineOperationDelete) + if updateErr != nil { + return updateRetryPeriod, updateErr + } + return machineutils.ShortRetry, err + } + + buf := bytes.NewBuffer([]byte{}) + errBuf := bytes.NewBuffer([]byte{}) + + drainOptions := drain.NewDrainOptions( + c.targetCoreClient, + c.targetKubernetesVersion, + timeOutDuration, + maxEvictRetries, + pvDetachTimeOut, + pvReattachTimeOut, + nodeName, + -1, + forceDeletePods, + true, + true, + true, + buf, + errBuf, + c.driver, + c.pvcLister, + c.pvLister, + c.pdbLister, + c.nodeLister, + c.podLister, + c.volumeAttachmentHandler, + c.podSynced, + ) + + klog.V(3).Infof("(drainNode) Invoking RunDrain, forceDeleteMachine: %t, forceDeletePods: %t, timeOutDuration: %s", forceDeletePods, forceDeleteMachine, timeOutDuration) + err = drainOptions.RunDrain(ctx) + if err == nil { + // Drain successful + klog.V(2).Infof("Drain successful for machine %q ,providerID %q, backing node %q. \nBuf:%v \nErrBuf:%v", machine.Name, getProviderID(machine), getNodeName(machine), buf, errBuf) + + if forceDeletePods { + description = fmt.Sprintf("Force Drain successful. %s", machineutils.DelVolumesAttachments) + } 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) + state = v1alpha1.MachineStateProcessing + + // Return error even when machine object is updated + } else if err != nil && forceDeleteMachine { + // Drain failed on force deletion + klog.Warningf("Drain failed for machine %q. However, since it's a force deletion shall continue deletion of VM. \nBuf:%v \nErrBuf:%v \nErr-Message:%v", machine.Name, buf, errBuf, err) + + description = fmt.Sprintf("Drain failed due to - %s. However, since it's a force deletion shall continue deletion of VM. %s", err.Error(), machineutils.DelVolumesAttachments) + state = v1alpha1.MachineStateProcessing + } else { + klog.Errorf("drain failed for machine %q , providerID %q ,backing node %q. \nBuf:%v \nErrBuf:%v \nErr-Message:%v", machine.Name, getProviderID(machine), getNodeName(machine), buf, errBuf, err) + + description = fmt.Sprintf("Drain failed due to - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) + state = v1alpha1.MachineStateFailed } } - updateRetryPeriod, updateErr := c.machineStatusUpdate( + updateRetryPeriod, updateErr := c.updateMachineStatusForDrain(ctx, machine, description, state, v1alpha1.MachineOperationDelete) + if updateErr != nil { + return updateRetryPeriod, updateErr + } + return machineutils.ShortRetry, fmt.Errorf("%s", description) +} + +// cordonNode sets node.Spec.Unschedulable to true, if not already set to true +func (c *controller) cordonNode(ctx context.Context, nodeName string) error { + node, err := c.targetCoreClient.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) + if err != nil { + // Deletion could be triggered when machine is just being created, no node present then + return nil + } + if node.Spec.Unschedulable { + klog.V(3).Infof("Scheduling state for node %q is already in desired state", node.Name) + return nil + } + clone := node.DeepCopy() + clone.Spec.Unschedulable = true + _, err = c.targetCoreClient.CoreV1().Nodes().Update(ctx, clone, metav1.UpdateOptions{}) + if err != nil { + return err + } + return nil +} + +func (c *controller) updateMachineStatusForDrain(ctx context.Context, machine *v1alpha1.Machine, description string, state v1alpha1.MachineState, opType v1alpha1.MachineOperationType) (machineutils.RetryPeriod, error) { + retryPeriod, err := c.machineStatusUpdate( ctx, machine, v1alpha1.LastOperation{ Description: description, State: state, - Type: v1alpha1.MachineOperationDelete, + Type: opType, LastUpdateTime: metav1.Now(), }, // Let the clone.Status.CurrentStatus (LastUpdateTime) be as it was before. @@ -1740,12 +1780,10 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver machine.Status.CurrentStatus, machine.Status.LastKnownState, ) - - if updateErr != nil { - return updateRetryPeriod, updateErr + if err != nil { + return retryPeriod, err } - - return machineutils.ShortRetry, err + return machineutils.ShortRetry, nil } // deleteNodeVolAttachments deletes VolumeAttachment(s) for a node before moving to VM deletion stage. @@ -2396,6 +2434,7 @@ func (c *controller) stopPreservationIfActive(ctx context.Context, machine *v1al if machine.Status.CurrentStatus.PreserveExpiryTime == nil { return machine, nil } + // if there is no backing node if nodeName == "" { // remove annotation from machine if needed @@ -2436,6 +2475,7 @@ func (c *controller) stopPreservationIfActive(ctx context.Context, machine *v1al klog.Errorf("error trying to get node %q of machine %q: %v. Retrying.", nodeName, machine.Name, err) return nil, err } + // prepare NodeCondition to set preservation as stopped preservedConditionFalse := v1.NodeCondition{ Type: v1alpha1.NodePreserved, @@ -2448,11 +2488,13 @@ func (c *controller) stopPreservationIfActive(ctx context.Context, machine *v1al if err != nil { return nil, err } + // Step 2: remove annotations from node updatedNode, err = c.removePreservationRelatedAnnotationsOnNode(ctx, updatedNode, removePreservationAnnotations) if err != nil { return nil, err } + // Step 3: remove annotation from machine if needed if removePreservationAnnotations { machine, err = c.removePreserveAnnotationOnMachine(ctx, machine) @@ -2460,12 +2502,14 @@ func (c *controller) stopPreservationIfActive(ctx context.Context, machine *v1al return nil, err } } - // Step 4: uncordon the node if the machine has recovered to Running. - if machine.Status.CurrentStatus.Phase == v1alpha1.MachineRunning { - if err = c.uncordonNodeIfCordoned(ctx, updatedNode); err != nil { - return nil, err - } + + // 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}) + if err != nil { + return true, err } + // Step 5: update machine status to set preserve expiry time to nil machine, err = c.clearMachinePreserveExpiryTime(ctx, machine) if err != nil { @@ -2602,13 +2646,17 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M readOnlyFileSystemCondition = condition } } - + if nodeName == "" { + klog.Warningf("(drainNode) machine %q has no node name. Skipping drain.", machine.Name) + return nil + } // verify and log node object's existence _, err = c.nodeLister.Get(nodeName) 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) + return nil } if !isConditionEmpty(nodeReadyCondition) && (nodeReadyCondition.Status != v1.ConditionTrue) && (time.Since(nodeReadyCondition.LastTransitionTime.Time) > nodeNotReadyDuration) { @@ -2648,6 +2696,14 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M ) } + // 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 + err = nodeops.AddOrUpdateTaintOnNode(ctx, c.targetCoreClient, nodeName, &v1.Taint{Key: machineutils.NodePreservedTaintKey, Effect: v1.TaintEffectNoSchedule}) + if err != nil { + klog.Errorf("cordoning of backing node %q for machine %q, with providerID %q, failed with error: %v", nodeName, machine.Name, getProviderID(machine), err) + return err + } + buf := bytes.NewBuffer([]byte{}) errBuf := bytes.NewBuffer([]byte{}) diff --git a/pkg/util/provider/machinecontroller/machine_util_test.go b/pkg/util/provider/machinecontroller/machine_util_test.go index 7e58a56b1a..76ef82e59e 100644 --- a/pkg/util/provider/machinecontroller/machine_util_test.go +++ b/pkg/util/provider/machinecontroller/machine_util_test.go @@ -3966,11 +3966,13 @@ var _ = Describe("machine_util", func() { preserveValue string isCAAnnotationPresent bool preservedNodeCondition corev1.NodeCondition + isUserCordoned bool } type expect struct { preserveNodeCondition corev1.NodeCondition isPreserveExpiryTimeSet bool isCAAnnotationPresent bool + isNodeTainted bool err error } type testCase struct { @@ -4010,6 +4012,7 @@ var _ = Describe("machine_util", func() { Labels: map[string]string{}, Annotations: map[string]string{}, }, + Spec: corev1.NodeSpec{Unschedulable: tc.setup.isUserCordoned}, Status: corev1.NodeStatus{ Conditions: []corev1.NodeCondition{}, }, @@ -4050,6 +4053,16 @@ var _ = Describe("machine_util", func() { Expect(updatedNodeCondition.Reason).To(Equal(tc.expect.preserveNodeCondition.Reason)) Expect(updatedNodeCondition.Message).To(ContainSubstring(tc.expect.preserveNodeCondition.Message)) } + Expect(updatedNode.Spec.Unschedulable).To(Equal(tc.setup.isUserCordoned), + "preserveMachine must not change Spec.Unschedulable") + hasTaint := false + for _, t := range updatedNode.Spec.Taints { + if t.Key == machineutils.NodePreservedTaintKey { + hasTaint = true + break + } + } + Expect(hasTaint).To(Equal(tc.expect.isNodeTainted)) }, Entry("when preserve=now and there is no backing node", &testCase{ setup: setup{ @@ -4089,6 +4102,26 @@ var _ = Describe("machine_util", func() { err: nil, isPreserveExpiryTimeSet: true, isCAAnnotationPresent: true, + isNodeTainted: true, + preserveNodeCondition: corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservedByUser, + Message: machinev1.PreservedNodeDrainSuccessful, + }, + }, + }), + Entry("when preserve=now, the machine has Failed, and the preservation is incomplete after step 1 - adding preserveExpiryTime", &testCase{ + setup: setup{ + machinePhase: machinev1.MachineFailed, + nodeName: "node-1", + preserveValue: machineutils.PreserveMachineAnnotationValueNow, + }, + expect: expect{ + err: nil, + isPreserveExpiryTimeSet: true, + isCAAnnotationPresent: true, + isNodeTainted: true, preserveNodeCondition: corev1.NodeCondition{ Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, @@ -4108,6 +4141,7 @@ var _ = Describe("machine_util", func() { err: nil, isPreserveExpiryTimeSet: true, isCAAnnotationPresent: true, + isNodeTainted: true, preserveNodeCondition: corev1.NodeCondition{ Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, @@ -4133,6 +4167,7 @@ var _ = Describe("machine_util", func() { err: nil, isPreserveExpiryTimeSet: true, isCAAnnotationPresent: true, + isNodeTainted: true, preserveNodeCondition: corev1.NodeCondition{ Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, @@ -4151,6 +4186,7 @@ var _ = Describe("machine_util", func() { err: nil, isPreserveExpiryTimeSet: true, isCAAnnotationPresent: true, + isNodeTainted: true, preserveNodeCondition: corev1.NodeCondition{ Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, @@ -4169,6 +4205,7 @@ var _ = Describe("machine_util", func() { err: nil, isPreserveExpiryTimeSet: true, isCAAnnotationPresent: true, + isNodeTainted: true, preserveNodeCondition: corev1.NodeCondition{ Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, @@ -4190,12 +4227,131 @@ var _ = Describe("machine_util", func() { }, }, ), + Entry("when preserve=now, the machine is Running, and the node is user-cordoned, Spec.Unschedulable must remain true", &testCase{ + setup: setup{ + machinePhase: machinev1.MachineRunning, + nodeName: "node-1", + preserveValue: machineutils.PreserveMachineAnnotationValueNow, + isUserCordoned: true, + }, + expect: expect{ + err: nil, + isPreserveExpiryTimeSet: true, + isCAAnnotationPresent: true, + preserveNodeCondition: corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservedByUser, + }, + }, + }), + Entry("when preserve=now, the machine has Failed, and the node is user-cordoned, Spec.Unschedulable must remain true", &testCase{ + setup: setup{ + machinePhase: machinev1.MachineFailed, + nodeName: "node-1", + preserveValue: machineutils.PreserveMachineAnnotationValueNow, + isUserCordoned: true, + }, + expect: expect{ + err: nil, + isPreserveExpiryTimeSet: true, + isCAAnnotationPresent: true, + isNodeTainted: true, + preserveNodeCondition: corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservedByUser, + Message: machinev1.PreservedNodeDrainSuccessful, + }, + }, + }), + ) + }) + Describe("#cordonNode", func() { + type setup struct { + nodeExists bool + alreadyCordoned bool + } + type expect struct { + err error + cordoned bool + } + type testCase struct { + setup setup + expect expect + } + DescribeTable("##cordonNode behaviour scenarios", + func(tc *testCase) { + stop := make(chan struct{}) + defer close(stop) + + var targetCoreObjects []runtime.Object + nodeName := "node-1" + + if tc.setup.nodeExists { + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: nodeName}, + Spec: corev1.NodeSpec{Unschedulable: tc.setup.alreadyCordoned}, + } + targetCoreObjects = append(targetCoreObjects, node) + } + + c, trackers := createController(stop, testNamespace, nil, nil, targetCoreObjects, nil, false) + defer trackers.Stop() + waitForCacheSync(stop, c) + + err := c.cordonNode(context.TODO(), nodeName) + if tc.expect.err != nil { + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal(tc.expect.err.Error())) + return + } + Expect(err).To(BeNil()) + + if !tc.setup.nodeExists { + return + } + updatedNode, getErr := c.targetCoreClient.CoreV1().Nodes().Get(context.TODO(), nodeName, metav1.GetOptions{}) + Expect(getErr).To(BeNil()) + Expect(updatedNode.Spec.Unschedulable).To(Equal(tc.expect.cordoned)) + }, + Entry("when the node does not exist, should succeed without error", &testCase{ + setup: setup{ + nodeExists: false, + }, + expect: expect{ + err: nil, + }, + }), + Entry("when the node exists and is not yet cordoned, should cordon it", &testCase{ + setup: setup{ + nodeExists: true, + alreadyCordoned: false, + }, + expect: expect{ + err: nil, + cordoned: true, + }, + }), + Entry("when the node exists and is already cordoned, should remain cordoned without error", &testCase{ + setup: setup{ + nodeExists: true, + alreadyCordoned: true, + }, + expect: expect{ + err: nil, + cordoned: true, + }, + }), ) }) Describe("#stopPreservationIfActive", func() { type setup struct { nodeName string removePreserveAnnotation bool + machinePhase machinev1.MachinePhase + isUserCordoned bool + isTainted bool } type expect struct { err error @@ -4212,6 +4368,10 @@ var _ = Describe("machine_util", func() { var controlMachineObjects []runtime.Object var targetCoreObjects []runtime.Object + machinePhase := tc.setup.machinePhase + if machinePhase == "" { + machinePhase = machinev1.MachineFailed + } machine := &machinev1.Machine{ ObjectMeta: metav1.ObjectMeta{ Name: "machine-1", @@ -4221,7 +4381,7 @@ var _ = Describe("machine_util", func() { Spec: machinev1.MachineSpec{}, Status: machinev1.MachineStatus{ CurrentStatus: machinev1.CurrentStatus{ - Phase: machinev1.MachineFailed, + Phase: machinePhase, LastUpdateTime: metav1.Now(), PreserveExpiryTime: &metav1.Time{Time: time.Now().Add(10 * time.Minute)}, }, @@ -4237,6 +4397,7 @@ var _ = Describe("machine_util", func() { autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationKey: autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationValue, }, }, + Spec: corev1.NodeSpec{Unschedulable: tc.setup.isUserCordoned}, Status: corev1.NodeStatus{ Conditions: []corev1.NodeCondition{ { @@ -4247,6 +4408,11 @@ var _ = Describe("machine_util", func() { }, }, } + if tc.setup.isTainted { + node.Spec.Taints = []corev1.Taint{ + {Key: machineutils.NodePreservedTaintKey, Effect: corev1.TaintEffectNoSchedule}, + } + } targetCoreObjects = append(targetCoreObjects, node) } else { @@ -4283,6 +4449,17 @@ var _ = Describe("machine_util", func() { } else { Expect(updatedNode.Annotations).To(HaveKey(machineutils.PreserveMachineAnnotationKey)) } + Expect(updatedNode.Spec.Unschedulable).To(Equal(tc.setup.isUserCordoned)) + if tc.setup.isTainted { + hasTaint := false + for _, t := range updatedNode.Spec.Taints { + if t.Key == machineutils.NodePreservedTaintKey { + hasTaint = true + break + } + } + Expect(hasTaint).To(BeFalse()) + } }, Entry("when stopping preservation on a preserved machine with backing node and preserve annotation needs to be removed", &testCase{ @@ -4319,6 +4496,28 @@ var _ = Describe("machine_util", func() { err: nil, }, }), + Entry("when machine recovers to Running and node was not user-cordoned, preservation taint must be removed and Spec.Unschedulable must remain false", &testCase{ + setup: setup{ + nodeName: "node-1", + machinePhase: machinev1.MachineRunning, + isUserCordoned: false, + isTainted: true, + }, + expect: expect{ + err: nil, + }, + }), + Entry("when machine recovers to Running and node was user-cordoned, preservation taint must be removed and Spec.Unschedulable must remain true", &testCase{ + setup: setup{ + nodeName: "node-1", + machinePhase: machinev1.MachineRunning, + isUserCordoned: true, + isTainted: true, + }, + expect: expect{ + err: nil, + }, + }), ) }) Describe("#computeNewNodePreservedCondition", func() { diff --git a/pkg/util/provider/machineutils/utils.go b/pkg/util/provider/machineutils/utils.go index 1c0f99af3d..e96ccb5364 100644 --- a/pkg/util/provider/machineutils/utils.go +++ b/pkg/util/provider/machineutils/utils.go @@ -118,6 +118,10 @@ const ( // 1) indicate to MCM that a machine must not be auto-preserved on failure // and, 2) to stop preservation of a machine that is already preserved by MCM. PreserveMachineAnnotationValueFalse = "false" + + // NodePreservedTaintKey is used to cordon a node when a Failed machine is preserved. + // This taint is added to the node before draining it, and removed when the machine is unpreserved. + NodePreservedTaintKey = "node.machine.sapcloud.io/preserved" ) // AllowedPreserveAnnotationValues contains the allowed values for the preserve annotation From ea9f84f5332b6da5ea5574a46a328c19a5d75098 Mon Sep 17 00:00:00 2001 From: thiyyakat Date: Thu, 16 Jul 2026 21:41:17 +0530 Subject: [PATCH 2/8] Address reviewer comments - 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". --- .../machinecontroller/machine_util.go | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/pkg/util/provider/machinecontroller/machine_util.go b/pkg/util/provider/machinecontroller/machine_util.go index 8497172ca2..cf4f90120b 100644 --- a/pkg/util/provider/machinecontroller/machine_util.go +++ b/pkg/util/provider/machinecontroller/machine_util.go @@ -1491,7 +1491,7 @@ func (c *controller) drainNodeForInPlace(ctx context.Context, machine *v1alpha1. if err = c.cordonNode(ctx, nodeName); err != nil { klog.Errorf("cordoning of backing node %q for machine %q, with providerID %q, failed with error: %v", nodeName, machine.Name, getProviderID(machine), err) - description = fmt.Sprintf("Cordoning failed due to - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) + description = fmt.Sprintf("Drain failed due to - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) state = v1alpha1.MachineStateProcessing return c.updateMachineStatusAndNodeCondition(ctx, machine, description, state, err) } @@ -1657,7 +1657,7 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver if updateErr != nil { return updateRetryPeriod, updateErr } - return machineutils.ShortRetry, err + return machineutils.ShortRetry, fmt.Errorf("%s", description) } klog.Warningf("Failed to update node conditions: %v. However, since it's a force deletion shall continue deletion of VM.", err) } @@ -1670,7 +1670,7 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver state = v1alpha1.MachineStateProcessing } else { klog.Errorf("cordoning of backing node %q for machine %q, with providerID %q, failed with error: %v", nodeName, machine.Name, getProviderID(machine), err) - description = fmt.Sprintf("Cordoning failed due to - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) + description = fmt.Sprintf("Drain failed due to - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) state = v1alpha1.MachineStateFailed } updateRetryPeriod, updateErr := c.updateMachineStatusForDrain(ctx, machine, description, state, v1alpha1.MachineOperationDelete) @@ -2507,7 +2507,7 @@ func (c *controller) stopPreservationIfActive(ctx context.Context, machine *v1al // 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 + return false, err } // Step 5: update machine status to set preserve expiry time to nil @@ -2639,6 +2639,11 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M nodeNotReadyDuration = 5 * time.Minute ReadonlyFilesystem v1.NodeConditionType = "ReadonlyFilesystem" ) + if nodeName == "" { + klog.Warningf("(drainNode) machine %q has no node name. Skipping drain.", machine.Name) + return nil + } + for _, condition := range machine.Status.Conditions { if condition.Type == v1.NodeReady { nodeReadyCondition = condition @@ -2646,10 +2651,7 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M readOnlyFileSystemCondition = condition } } - if nodeName == "" { - klog.Warningf("(drainNode) machine %q has no node name. Skipping drain.", machine.Name) - return nil - } + // verify and log node object's existence _, err = c.nodeLister.Get(nodeName) if err == nil { @@ -2697,10 +2699,10 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M } // 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 + // we add a taint with effect `NoSchedule`, before draining the node, instead of cordoning it. err = nodeops.AddOrUpdateTaintOnNode(ctx, c.targetCoreClient, nodeName, &v1.Taint{Key: machineutils.NodePreservedTaintKey, Effect: v1.TaintEffectNoSchedule}) if err != nil { - klog.Errorf("cordoning of backing node %q for machine %q, with providerID %q, failed with error: %v", nodeName, machine.Name, getProviderID(machine), err) + klog.Errorf("tainting of backing node %q for machine %q, with providerID %q, failed with error: %v", nodeName, machine.Name, getProviderID(machine), err) return err } From 07849d228f4ff656a91fac7e753d6456f471cf71 Mon Sep 17 00:00:00 2001 From: thiyyakat Date: Fri, 17 Jul 2026 10:05:04 +0530 Subject: [PATCH 3/8] Address reviewer comments - part 2 - 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. --- .../provider/machinecontroller/machine_util.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/pkg/util/provider/machinecontroller/machine_util.go b/pkg/util/provider/machinecontroller/machine_util.go index cf4f90120b..88eed080d6 100644 --- a/pkg/util/provider/machinecontroller/machine_util.go +++ b/pkg/util/provider/machinecontroller/machine_util.go @@ -1445,7 +1445,7 @@ func (c *controller) drainNodeForInPlace(ctx context.Context, machine *v1alpha1. if node, err = c.nodeLister.Get(nodeName); 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) + klog.Warningf("(drainNode) Node %q for machine %q doesn't exist. Skipping drain.", nodeName, machine.Name) } if !isConditionEmpty(nodeReadyCondition) && (nodeReadyCondition.Status != v1.ConditionTrue) && (time.Since(nodeReadyCondition.LastTransitionTime.Time) > nodeNotReadyDuration) { @@ -1595,7 +1595,7 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver if _, err := c.nodeLister.Get(nodeName); 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) + klog.Warningf("(drainNode) Node %q for machine %q doesn't exist. Skipping drain.", nodeName, machine.Name) } if !isConditionEmpty(nodeReadyCondition) && (nodeReadyCondition.Status != v1.ConditionTrue) && (time.Since(nodeReadyCondition.LastTransitionTime.Time) > nodeNotReadyDuration) { @@ -1657,7 +1657,7 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver if updateErr != nil { return updateRetryPeriod, updateErr } - return machineutils.ShortRetry, fmt.Errorf("%s", description) + return machineutils.ShortRetry, err } klog.Warningf("Failed to update node conditions: %v. However, since it's a force deletion shall continue deletion of VM.", err) } @@ -1665,7 +1665,7 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver if err = c.cordonNode(ctx, nodeName); err != nil { if forceDeleteMachine { // Cordoning failed on force deletion - klog.Warningf("Cordoning failed for machine %q. However, since it's a force deletion shall continue deletion of VM. \nErr-Message:%v", machine.Name, err) + klog.Warningf("Cordoning of backing node %q for machine %q, with providerID %q, failed. However, since it's a force deletion, shall continue deletion of VM. \nErr-Message:%v", nodeName, machine.Name, getProviderID(machine), err) description = fmt.Sprintf("Drain failed due to - %s. However, since it's a force deletion shall continue deletion of VM. %s", err.Error(), machineutils.DelVolumesAttachments) state = v1alpha1.MachineStateProcessing } else { @@ -1752,7 +1752,7 @@ func (c *controller) cordonNode(ctx context.Context, nodeName string) error { return nil } if node.Spec.Unschedulable { - klog.V(3).Infof("Scheduling state for node %q is already in desired state", node.Name) + klog.V(3).Infof("Node %q is already cordoned.", node.Name) return nil } clone := node.DeepCopy() @@ -2505,7 +2505,10 @@ func (c *controller) stopPreservationIfActive(ctx context.Context, machine *v1al // 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}) + err = nodeops.RemoveTaintOffNode(ctx, c.targetCoreClient, updatedNode.Name, updatedNode, &v1.Taint{ + Key: machineutils.NodePreservedTaintKey, + Effect: v1.TaintEffectNoSchedule, + }) if err != nil { return false, err } @@ -2657,7 +2660,7 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M 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) + klog.Warningf("(drainNode) Node %q for machine %q doesn't exist. Skipping drain.", nodeName, machine.Name) return nil } From fd46abba6989feb5aa65169d40723ce538029df6 Mon Sep 17 00:00:00 2001 From: thiyyakat Date: Fri, 17 Jul 2026 10:13:10 +0530 Subject: [PATCH 4/8] Address reviewer comments - part 3 - Return early from `drainNode` when nodename is empty. Remove `skipDrain`. --- .../machinecontroller/machine_util.go | 265 +++++++++--------- 1 file changed, 132 insertions(+), 133 deletions(-) diff --git a/pkg/util/provider/machinecontroller/machine_util.go b/pkg/util/provider/machinecontroller/machine_util.go index 88eed080d6..df6fe08959 100644 --- a/pkg/util/provider/machinecontroller/machine_util.go +++ b/pkg/util/provider/machinecontroller/machine_util.go @@ -1555,7 +1555,6 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver forceDeletePods bool forceDeleteMachine bool timeOutOccurred bool - skipDrain bool description string state v1alpha1.MachineState readOnlyFileSystemCondition, nodeReadyCondition v1.NodeCondition @@ -1581,160 +1580,160 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver if nodeName == "" { message := "Skipping drain as nodeName is not a valid one for machine." printLogInitError(message, &err, &description, machine, false) - skipDrain = true - } else { - for _, condition := range machine.Status.Conditions { - if condition.Type == v1.NodeReady { - nodeReadyCondition = condition - } else if condition.Type == ReadonlyFilesystem { - readOnlyFileSystemCondition = condition - } + state = v1alpha1.MachineStateProcessing + updateRetryPeriod, updateErr := c.updateMachineStatusForDrain(ctx, machine, description, state, v1alpha1.MachineOperationDelete) + if updateErr != nil { + return updateRetryPeriod, updateErr } - - // verify and log node object's existence - if _, err := c.nodeLister.Get(nodeName); 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. Skipping drain.", nodeName, machine.Name) + return machineutils.ShortRetry, fmt.Errorf("%s", description) + } + for _, condition := range machine.Status.Conditions { + if condition.Type == v1.NodeReady { + nodeReadyCondition = condition + } else if condition.Type == ReadonlyFilesystem { + readOnlyFileSystemCondition = condition } + } - if !isConditionEmpty(nodeReadyCondition) && (nodeReadyCondition.Status != v1.ConditionTrue) && (time.Since(nodeReadyCondition.LastTransitionTime.Time) > nodeNotReadyDuration) { - message := "Setting forceDeletePods & forceDeleteMachine to true for drain as machine is NotReady for over 5min" - forceDeleteMachine = true - forceDeletePods = true - printLogInitError(message, &err, &description, machine, false) - } else if !isConditionEmpty(readOnlyFileSystemCondition) && (readOnlyFileSystemCondition.Status != v1.ConditionFalse) && (time.Since(readOnlyFileSystemCondition.LastTransitionTime.Time) > nodeNotReadyDuration) { - message := "Setting forceDeletePods & forceDeleteMachine to true for drain as machine is in ReadonlyFilesystem for over 5min" - forceDeleteMachine = true - forceDeletePods = true - printLogInitError(message, &err, &description, machine, false) - } + // verify and log node object's existence + if _, err := c.nodeLister.Get(nodeName); 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. Skipping drain.", nodeName, machine.Name) } - if skipDrain { - state = v1alpha1.MachineStateProcessing - } else { - timeOutOccurred = utiltime.HasTimeOutOccurred(*machine.DeletionTimestamp, timeOutDuration) - - if forceDeleteLabelPresent || timeOutOccurred { - // To perform forceful machine drain/delete either one of the below conditions must be satified - // 1. force-deletion: "True" label must be present - // 2. Deletion operation is more than drain-timeout minutes old - // 3. Last machine drain had failed - forceDeleteMachine = true - forceDeletePods = true - timeOutDuration = 1 * time.Minute - maxEvictRetries = 1 - - klog.V(2).Infof( - "Force delete/drain has been triggerred for machine %q with providerID %q and backing node %q due to Label:%t, timeout:%t", - machine.Name, - getProviderID(machine), - getNodeName(machine), - forceDeleteLabelPresent, - timeOutOccurred, - ) - } else { - klog.V(2).Infof( - "Normal delete/drain has been triggerred for machine %q with providerID %q and backing node %q with drain-timeout:%v & maxEvictRetries:%d", - machine.Name, - getProviderID(machine), - getNodeName(machine), - timeOutDuration, - maxEvictRetries, - ) - } + if !isConditionEmpty(nodeReadyCondition) && (nodeReadyCondition.Status != v1.ConditionTrue) && (time.Since(nodeReadyCondition.LastTransitionTime.Time) > nodeNotReadyDuration) { + message := "Setting forceDeletePods & forceDeleteMachine to true for drain as machine is NotReady for over 5min" + forceDeleteMachine = true + forceDeletePods = true + printLogInitError(message, &err, &description, machine, false) + } else if !isConditionEmpty(readOnlyFileSystemCondition) && (readOnlyFileSystemCondition.Status != v1.ConditionFalse) && (time.Since(readOnlyFileSystemCondition.LastTransitionTime.Time) > nodeNotReadyDuration) { + message := "Setting forceDeletePods & forceDeleteMachine to true for drain as machine is in ReadonlyFilesystem for over 5min" + forceDeleteMachine = true + forceDeletePods = true + printLogInitError(message, &err, &description, machine, false) + } - // update node with the machine's phase prior to termination - if err = c.UpdateNodeTerminationCondition(ctx, machine); err != nil { - if !forceDeleteMachine { - klog.Errorf("drain failed due to failure in update of node conditions: %v", err) + timeOutOccurred = utiltime.HasTimeOutOccurred(*machine.DeletionTimestamp, timeOutDuration) - description = fmt.Sprintf("Drain failed due to failure in update of node conditions - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) - state = v1alpha1.MachineStateFailed + if forceDeleteLabelPresent || timeOutOccurred { + // To perform forceful machine drain/delete either one of the below conditions must be satisfied + // 1. force-deletion: "True" label must be present + // 2. Deletion operation is more than drain-timeout minutes old + // 3. Last machine drain had failed + forceDeleteMachine = true + forceDeletePods = true + timeOutDuration = 1 * time.Minute + maxEvictRetries = 1 - updateRetryPeriod, updateErr := c.updateMachineStatusForDrain(ctx, machine, description, state, v1alpha1.MachineOperationDelete) - if updateErr != nil { - return updateRetryPeriod, updateErr - } - return machineutils.ShortRetry, err - } - klog.Warningf("Failed to update node conditions: %v. However, since it's a force deletion shall continue deletion of VM.", err) - } + klog.V(2).Infof( + "Force delete/drain has been triggerred for machine %q with providerID %q and backing node %q due to Label:%t, timeout:%t", + machine.Name, + getProviderID(machine), + getNodeName(machine), + forceDeleteLabelPresent, + timeOutOccurred, + ) + } else { + klog.V(2).Infof( + "Normal delete/drain has been triggerred for machine %q with providerID %q and backing node %q with drain-timeout:%v & maxEvictRetries:%d", + machine.Name, + getProviderID(machine), + getNodeName(machine), + timeOutDuration, + maxEvictRetries, + ) + } + + // update node with the machine's phase prior to termination + if err = c.UpdateNodeTerminationCondition(ctx, machine); err != nil { + if !forceDeleteMachine { + klog.Errorf("drain failed due to failure in update of node conditions: %v", err) + + description = fmt.Sprintf("Drain failed due to failure in update of node conditions - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) + state = v1alpha1.MachineStateFailed - if err = c.cordonNode(ctx, nodeName); err != nil { - if forceDeleteMachine { - // Cordoning failed on force deletion - klog.Warningf("Cordoning of backing node %q for machine %q, with providerID %q, failed. However, since it's a force deletion, shall continue deletion of VM. \nErr-Message:%v", nodeName, machine.Name, getProviderID(machine), err) - description = fmt.Sprintf("Drain failed due to - %s. However, since it's a force deletion shall continue deletion of VM. %s", err.Error(), machineutils.DelVolumesAttachments) - state = v1alpha1.MachineStateProcessing - } else { - klog.Errorf("cordoning of backing node %q for machine %q, with providerID %q, failed with error: %v", nodeName, machine.Name, getProviderID(machine), err) - description = fmt.Sprintf("Drain failed due to - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) - state = v1alpha1.MachineStateFailed - } updateRetryPeriod, updateErr := c.updateMachineStatusForDrain(ctx, machine, description, state, v1alpha1.MachineOperationDelete) if updateErr != nil { return updateRetryPeriod, updateErr } return machineutils.ShortRetry, err } + klog.Warningf("Failed to update node conditions: %v. However, since it's a force deletion shall continue deletion of VM.", err) + } - buf := bytes.NewBuffer([]byte{}) - errBuf := bytes.NewBuffer([]byte{}) - - drainOptions := drain.NewDrainOptions( - c.targetCoreClient, - c.targetKubernetesVersion, - timeOutDuration, - maxEvictRetries, - pvDetachTimeOut, - pvReattachTimeOut, - nodeName, - -1, - forceDeletePods, - true, - true, - true, - buf, - errBuf, - c.driver, - c.pvcLister, - c.pvLister, - c.pdbLister, - c.nodeLister, - c.podLister, - c.volumeAttachmentHandler, - c.podSynced, - ) - - klog.V(3).Infof("(drainNode) Invoking RunDrain, forceDeleteMachine: %t, forceDeletePods: %t, timeOutDuration: %s", forceDeletePods, forceDeleteMachine, timeOutDuration) - err = drainOptions.RunDrain(ctx) - if err == nil { - // Drain successful - klog.V(2).Infof("Drain successful for machine %q ,providerID %q, backing node %q. \nBuf:%v \nErrBuf:%v", machine.Name, getProviderID(machine), getNodeName(machine), buf, errBuf) - - if forceDeletePods { - description = fmt.Sprintf("Force Drain successful. %s", machineutils.DelVolumesAttachments) - } 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) - state = v1alpha1.MachineStateProcessing - - // Return error even when machine object is updated - } else if err != nil && forceDeleteMachine { - // Drain failed on force deletion - klog.Warningf("Drain failed for machine %q. However, since it's a force deletion shall continue deletion of VM. \nBuf:%v \nErrBuf:%v \nErr-Message:%v", machine.Name, buf, errBuf, err) - + if err = c.cordonNode(ctx, nodeName); err != nil { + if forceDeleteMachine { + // Cordoning failed on force deletion + klog.Warningf("Cordoning of backing node %q for machine %q, with providerID %q, failed. However, since it's a force deletion, shall continue deletion of VM. \nErr-Message:%v", nodeName, machine.Name, getProviderID(machine), err) description = fmt.Sprintf("Drain failed due to - %s. However, since it's a force deletion shall continue deletion of VM. %s", err.Error(), machineutils.DelVolumesAttachments) state = v1alpha1.MachineStateProcessing } else { - klog.Errorf("drain failed for machine %q , providerID %q ,backing node %q. \nBuf:%v \nErrBuf:%v \nErr-Message:%v", machine.Name, getProviderID(machine), getNodeName(machine), buf, errBuf, err) - + klog.Errorf("cordoning of backing node %q for machine %q, with providerID %q, failed with error: %v", nodeName, machine.Name, getProviderID(machine), err) description = fmt.Sprintf("Drain failed due to - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) state = v1alpha1.MachineStateFailed } + updateRetryPeriod, updateErr := c.updateMachineStatusForDrain(ctx, machine, description, state, v1alpha1.MachineOperationDelete) + if updateErr != nil { + return updateRetryPeriod, updateErr + } + return machineutils.ShortRetry, err + } + + buf := bytes.NewBuffer([]byte{}) + errBuf := bytes.NewBuffer([]byte{}) + + drainOptions := drain.NewDrainOptions( + c.targetCoreClient, + c.targetKubernetesVersion, + timeOutDuration, + maxEvictRetries, + pvDetachTimeOut, + pvReattachTimeOut, + nodeName, + -1, + forceDeletePods, + true, + true, + true, + buf, + errBuf, + c.driver, + c.pvcLister, + c.pvLister, + c.pdbLister, + c.nodeLister, + c.podLister, + c.volumeAttachmentHandler, + c.podSynced, + ) + + klog.V(3).Infof("(drainNode) Invoking RunDrain, forceDeleteMachine: %t, forceDeletePods: %t, timeOutDuration: %s", forceDeletePods, forceDeleteMachine, timeOutDuration) + err = drainOptions.RunDrain(ctx) + if err == nil { + // Drain successful + klog.V(2).Infof("Drain successful for machine %q ,providerID %q, backing node %q. \nBuf:%v \nErrBuf:%v", machine.Name, getProviderID(machine), getNodeName(machine), buf, errBuf) + + if forceDeletePods { + description = fmt.Sprintf("Force Drain successful. %s", machineutils.DelVolumesAttachments) + } 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) + state = v1alpha1.MachineStateProcessing + + // Return error even when machine object is updated + } else if err != nil && forceDeleteMachine { + // Drain failed on force deletion + klog.Warningf("Drain failed for machine %q. However, since it's a force deletion shall continue deletion of VM. \nBuf:%v \nErrBuf:%v \nErr-Message:%v", machine.Name, buf, errBuf, err) + + description = fmt.Sprintf("Drain failed due to - %s. However, since it's a force deletion shall continue deletion of VM. %s", err.Error(), machineutils.DelVolumesAttachments) + state = v1alpha1.MachineStateProcessing + } else { + klog.Errorf("drain failed for machine %q , providerID %q ,backing node %q. \nBuf:%v \nErrBuf:%v \nErr-Message:%v", machine.Name, getProviderID(machine), getNodeName(machine), buf, errBuf, err) + + description = fmt.Sprintf("Drain failed due to - %s. Will retry in next sync. %s", err.Error(), machineutils.InitiateDrain) + state = v1alpha1.MachineStateFailed } updateRetryPeriod, updateErr := c.updateMachineStatusForDrain(ctx, machine, description, state, v1alpha1.MachineOperationDelete) From d2f925a6065f54b9e04169da7cdcf24d9ded3d5c Mon Sep 17 00:00:00 2001 From: thiyyakat Date: Fri, 17 Jul 2026 13:39:57 +0530 Subject: [PATCH 5/8] Address reviewer comments - part 4 - Remove unnecessary assignment to `err`. - Move `cordonNode()` to `node.go` - Return `err` from `cordonNode()` for errors other than NotFound errors. --- .../machinecontroller/machine_util.go | 21 ----------- pkg/util/provider/machinecontroller/node.go | 36 +++++++++++++++++++ 2 files changed, 36 insertions(+), 21 deletions(-) diff --git a/pkg/util/provider/machinecontroller/machine_util.go b/pkg/util/provider/machinecontroller/machine_util.go index df6fe08959..4dfd1b4bd2 100644 --- a/pkg/util/provider/machinecontroller/machine_util.go +++ b/pkg/util/provider/machinecontroller/machine_util.go @@ -1719,7 +1719,6 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver } 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) state = v1alpha1.MachineStateProcessing // Return error even when machine object is updated @@ -1743,26 +1742,6 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver return machineutils.ShortRetry, fmt.Errorf("%s", description) } -// cordonNode sets node.Spec.Unschedulable to true, if not already set to true -func (c *controller) cordonNode(ctx context.Context, nodeName string) error { - node, err := c.targetCoreClient.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) - if err != nil { - // Deletion could be triggered when machine is just being created, no node present then - return nil - } - if node.Spec.Unschedulable { - klog.V(3).Infof("Node %q is already cordoned.", node.Name) - return nil - } - clone := node.DeepCopy() - clone.Spec.Unschedulable = true - _, err = c.targetCoreClient.CoreV1().Nodes().Update(ctx, clone, metav1.UpdateOptions{}) - if err != nil { - return err - } - return nil -} - func (c *controller) updateMachineStatusForDrain(ctx context.Context, machine *v1alpha1.Machine, description string, state v1alpha1.MachineState, opType v1alpha1.MachineOperationType) (machineutils.RetryPeriod, error) { retryPeriod, err := c.machineStatusUpdate( ctx, diff --git a/pkg/util/provider/machinecontroller/node.go b/pkg/util/provider/machinecontroller/node.go index 174805c7c1..47217d6285 100644 --- a/pkg/util/provider/machinecontroller/node.go +++ b/pkg/util/provider/machinecontroller/node.go @@ -340,6 +340,42 @@ func addedOrRemovedEssentialTaints(oldNode, node *corev1.Node, taintKeys []strin return false } +func (c *controller) getNodePreserveAnnotationValue(nodeName string) (string, error) { + node, err := c.nodeLister.Get(nodeName) + if err != nil { + if apierrors.IsNotFound(err) { + return "", nil + } + klog.Errorf("error fetching node %q: %v", nodeName, err) + return "", err + } + return node.Annotations[machineutils.PreserveMachineAnnotationKey], nil +} + +// cordonNode sets `node.Spec.Unschedulable` to true, if not already set to true +func (c *controller) cordonNode(ctx context.Context, nodeName string) error { + node, err := c.targetCoreClient.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) + if err != nil { + // Deletion could be triggered when machine is just being created, no node present then + if apierrors.IsNotFound(err) { + klog.Warningf("Node %q could not be found.", nodeName) + return nil + } + return err + } + if node.Spec.Unschedulable { + klog.V(3).Infof("Node %q is already cordoned.", node.Name) + return nil + } + clone := node.DeepCopy() + clone.Spec.Unschedulable = true + _, err = c.targetCoreClient.CoreV1().Nodes().Update(ctx, clone, metav1.UpdateOptions{}) + if err != nil { + return err + } + return nil +} + func (c *controller) uncordonNodeIfCordoned(ctx context.Context, node *corev1.Node) error { if !node.Spec.Unschedulable { return nil From 117fb4be0cdc053060b8536b338e268099a8f0ab Mon Sep 17 00:00:00 2001 From: thiyyakat Date: Mon, 27 Jul 2026 13:44:20 +0530 Subject: [PATCH 6/8] Address review comments (5) and correct rebase - 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`. --- .../provider/machinecontroller/machine.go | 14 ++--- .../machinecontroller/machine_test.go | 60 +++++++++++-------- .../machinecontroller/machine_util.go | 33 +++++----- pkg/util/provider/machinecontroller/node.go | 32 +--------- 4 files changed, 57 insertions(+), 82 deletions(-) diff --git a/pkg/util/provider/machinecontroller/machine.go b/pkg/util/provider/machinecontroller/machine.go index 0b1a8d3080..65681aa371 100644 --- a/pkg/util/provider/machinecontroller/machine.go +++ b/pkg/util/provider/machinecontroller/machine.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "github.com/gardener/machine-controller-manager/pkg/util/nodeops" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -840,19 +841,18 @@ func (c *controller) manageMachinePreservation(ctx context.Context, machine *v1a if err != nil { return } - // For the preserve=now path (preservation still active), uncordon the node if the machine - // has recovered to Running. For all other cases, stopPreservationIfActive path handles uncordon internally. + // For the preserve=now path (preservation still active), untaint the node if the machine + // has recovered to Running. For all other cases, stopPreservationIfActive path handles untainting internally. if clone.Status.CurrentStatus.PreserveExpiryTime != nil && clone.Status.CurrentStatus.Phase == v1alpha1.MachineRunning && nodeName != "" { var node *corev1.Node node, err = c.nodeLister.Get(nodeName) if err != nil { - if apierrors.IsNotFound(err) { - err = nil - return - } return } - err = c.uncordonNodeIfCordoned(ctx, node) + err = nodeops.RemoveTaintOffNode(ctx, c.targetCoreClient, node.Name, node, &corev1.Taint{ + Key: machineutils.NodePreservedTaintKey, + Effect: corev1.TaintEffectNoSchedule, + }) if err != nil { return } diff --git a/pkg/util/provider/machinecontroller/machine_test.go b/pkg/util/provider/machinecontroller/machine_test.go index 6fb97ca0fd..0704f5e34d 100644 --- a/pkg/util/provider/machinecontroller/machine_test.go +++ b/pkg/util/provider/machinecontroller/machine_test.go @@ -10,6 +10,7 @@ import ( "math" "time" + taintutils "github.com/gardener/machine-controller-manager/pkg/util/taints" k8stesting "k8s.io/client-go/testing" . "github.com/onsi/ginkgo/v2" @@ -4105,7 +4106,7 @@ var _ = Describe("machine", func() { nodeName string machinePhase v1alpha1.MachinePhase preserveExpiryTime *metav1.Time - nodeUnschedulable bool + nodeTainted bool } type expect struct { retry machineutils.RetryPeriod @@ -4114,7 +4115,7 @@ var _ = Describe("machine", func() { machineAnnotationValue string laNodePreserveValue string err error - nodeUnschedulable *bool + nodeTainted bool } type testCase struct { setup setup @@ -4165,12 +4166,20 @@ var _ = Describe("machine", func() { Annotations: nodeAnnotations, }, Spec: corev1.NodeSpec{ - Unschedulable: tc.setup.nodeUnschedulable, + Taints: []corev1.Taint{}, }, Status: corev1.NodeStatus{ Conditions: []corev1.NodeCondition{}, }, } + if tc.setup.nodeTainted { + node.Spec.Taints = []corev1.Taint{ + { + Key: machineutils.NodePreservedTaintKey, + Effect: corev1.TaintEffectNoSchedule, + }, + } + } targetCoreObjects = append(targetCoreObjects, node) } c, trackers := createController(stop, testNamespace, controlMachineObjects, nil, targetCoreObjects, nil, false) @@ -4211,9 +4220,7 @@ var _ = Describe("machine", func() { } else { Expect(found).To(BeFalse()) } - if tc.expect.nodeUnschedulable != nil { - Expect(updatedNode.Spec.Unschedulable).To(Equal(*tc.expect.nodeUnschedulable)) - } + Expect(taintutils.TaintExists(updatedNode.Spec.Taints, &corev1.Taint{Key: machineutils.NodePreservedTaintKey, Effect: corev1.TaintEffectNoSchedule})).To(Equal(tc.expect.nodeTainted)) } Expect(updatedMachine.Annotations[machineutils.PreserveMachineAnnotationKey]).To(Equal(tc.expect.machineAnnotationValue)) Expect(updatedMachine.Annotations[machineutils.LastAppliedNodePreserveValueAnnotationKey]).To(Equal(tc.expect.laNodePreserveValue)) @@ -4271,7 +4278,8 @@ var _ = Describe("machine", func() { nodeCondition: &corev1.NodeCondition{ Type: v1alpha1.NodePreserved, Status: corev1.ConditionTrue}, - retry: machineutils.LongRetry, + retry: machineutils.LongRetry, + nodeTainted: true, }, }), Entry("when node of preserved machine is annotated with preserve value 'false', should stop preservation", testCase{ @@ -4303,6 +4311,7 @@ var _ = Describe("machine", func() { Status: corev1.ConditionTrue}, retry: machineutils.LongRetry, machineAnnotationValue: machineutils.PreserveMachineAnnotationValueAutoPreserved, + nodeTainted: true, }, }), Entry("when machine is annotated with preserve=now and preservation times out, should stop preservation, and remove annotation", testCase{ @@ -4326,6 +4335,7 @@ var _ = Describe("machine", func() { nodeName: "node-1", machinePhase: v1alpha1.MachineFailed, preserveExpiryTime: &metav1.Time{Time: metav1.Now().Add(-1 * time.Minute)}, + nodeTainted: true, }, expect: expect{ preserveExpiryTimeIsSet: false, @@ -4414,11 +4424,13 @@ var _ = Describe("machine", func() { nodeName: "node-1", machinePhase: v1alpha1.MachineRunning, preserveExpiryTime: &metav1.Time{Time: metav1.Now().Add(1 * time.Hour)}, + nodeTainted: true, }, expect: expect{ preserveExpiryTimeIsSet: false, nodeCondition: &corev1.NodeCondition{Type: v1alpha1.NodePreserved, Status: corev1.ConditionFalse}, machineAnnotationValue: "", + nodeTainted: false, retry: machineutils.LongRetry, err: nil, }, @@ -4462,8 +4474,7 @@ var _ = Describe("machine", func() { err: nil, }, }), - Entry("when preserve=when-failed machine with cordoned node recovers to Running, should stop preservation and uncordon node", func() testCase { - uncordoned := false + Entry("when preserved machine with preserve=when-failed recovers to Running, should stop preservation and untaint node", func() testCase { return testCase{ setup: setup{ machineAnnotationValue: machineutils.PreserveMachineAnnotationValueWhenFailed, @@ -4471,72 +4482,69 @@ var _ = Describe("machine", func() { nodeName: "node-1", machinePhase: v1alpha1.MachineRunning, preserveExpiryTime: &metav1.Time{Time: metav1.Now().Add(1 * time.Hour)}, - nodeUnschedulable: true, + nodeTainted: true, }, expect: expect{ preserveExpiryTimeIsSet: false, nodeCondition: &corev1.NodeCondition{Type: v1alpha1.NodePreserved, Status: corev1.ConditionFalse}, machineAnnotationValue: machineutils.PreserveMachineAnnotationValueWhenFailed, retry: machineutils.LongRetry, - nodeUnschedulable: &uncordoned, + nodeTainted: false, }, } }()), - Entry("when preserve=when-failed machine with cordoned node has expired preservation while Running, should stop preservation and uncordon node", func() testCase { - uncordoned := false + Entry("when preserved machine with preserve=when-failed has expired preservation while in Failed phase, should stop preservation and untaint node", func() testCase { return testCase{ setup: setup{ machineAnnotationValue: machineutils.PreserveMachineAnnotationValueWhenFailed, machineAnnotated: true, nodeName: "node-1", - machinePhase: v1alpha1.MachineRunning, + machinePhase: v1alpha1.MachineFailed, preserveExpiryTime: &metav1.Time{Time: metav1.Now().Add(-1 * time.Minute)}, - nodeUnschedulable: true, + nodeTainted: true, }, expect: expect{ preserveExpiryTimeIsSet: false, nodeCondition: &corev1.NodeCondition{Type: v1alpha1.NodePreserved, Status: corev1.ConditionFalse}, machineAnnotationValue: "", retry: machineutils.LongRetry, - nodeUnschedulable: &uncordoned, + nodeTainted: false, }, } }()), - Entry("when preserve=when-failed machine is Failed with active preservation and cordoned node, node should stay cordoned", func() testCase { - cordoned := true + Entry("when preserve=when-failed machine is in Failed phase with active preservation and tainted node, node should stay tainted", func() testCase { return testCase{ setup: setup{ nodeAnnotationValue: machineutils.PreserveMachineAnnotationValueWhenFailed, nodeAnnotated: true, nodeName: "node-1", machinePhase: v1alpha1.MachineFailed, - nodeUnschedulable: true, + nodeTainted: true, }, expect: expect{ laNodePreserveValue: machineutils.PreserveMachineAnnotationValueWhenFailed, preserveExpiryTimeIsSet: true, nodeCondition: &corev1.NodeCondition{Type: v1alpha1.NodePreserved, Status: corev1.ConditionTrue}, retry: machineutils.LongRetry, - nodeUnschedulable: &cordoned, + nodeTainted: true, }, } }()), - Entry("when preserve=now machine is Failed with active preservation and cordoned node, node should stay cordoned", func() testCase { - cordoned := true + Entry("when preserve=now machine is Failed with active preservation and tainted node, node should stay tainted", func() testCase { return testCase{ setup: setup{ machineAnnotationValue: machineutils.PreserveMachineAnnotationValueNow, machineAnnotated: true, nodeName: "node-1", machinePhase: v1alpha1.MachineFailed, - nodeUnschedulable: true, + nodeTainted: true, }, expect: expect{ preserveExpiryTimeIsSet: true, nodeCondition: &corev1.NodeCondition{Type: v1alpha1.NodePreserved, Status: corev1.ConditionTrue}, machineAnnotationValue: machineutils.PreserveMachineAnnotationValueNow, retry: machineutils.LongRetry, - nodeUnschedulable: &cordoned, + nodeTainted: true, }, } }()), @@ -4588,8 +4596,8 @@ var _ = Describe("machine", func() { Expect(updatedNode.Spec.Unschedulable).To(Equal(nodeUnschedulable)) } }, - Entry("node is cordoned", true, true), - Entry("node is uncordoned", false, true), + Entry("node is tainted", true, true), + Entry("node is untainted", false, true), Entry("node retrieval fails (node not found)", false, false), ) }) diff --git a/pkg/util/provider/machinecontroller/machine_util.go b/pkg/util/provider/machinecontroller/machine_util.go index 4dfd1b4bd2..a8a69ede47 100644 --- a/pkg/util/provider/machinecontroller/machine_util.go +++ b/pkg/util/provider/machinecontroller/machine_util.go @@ -2351,7 +2351,6 @@ Utility Functions for Machine Preservation // preserveMachine contains logic to start the preservation of a machine and node. func (c *controller) preserveMachine(ctx context.Context, machine *v1alpha1.Machine, preserveValue string) (*v1alpha1.Machine, error) { var err error - nodeName := machine.Labels[v1alpha1.NodeLabelKey] if machine.Status.CurrentStatus.PreserveExpiryTime == nil { klog.V(4).Infof("Starting preservation flow for machine %q.", machine.Name) // Step 1: Add preserveExpiryTime to machine status @@ -2360,8 +2359,10 @@ func (c *controller) preserveMachine(ctx context.Context, machine *v1alpha1.Mach return machine, err } } + + nodeName := machine.Labels[v1alpha1.NodeLabelKey] if nodeName == "" { - // Machine has no backing node, preservation is complete + // Machine has no backing node( such as in the case of self-hosted shoots), preservation is complete klog.V(2).Infof("Machine %q without backing node is preserved successfully till %v.", machine.Name, machine.Status.CurrentStatus.PreserveExpiryTime) return machine, nil } @@ -2372,7 +2373,7 @@ func (c *controller) preserveMachine(ctx context.Context, machine *v1alpha1.Mach return machine, err } existingNodePreservedCondition := nodeops.GetCondition(node, v1alpha1.NodePreserved) - // checks if preservation is already complete + // if the `Preserved` NodeCondition has ConditionStatus==ConditionTrue, preservation is complete if existingNodePreservedCondition != nil && existingNodePreservedCondition.Status == v1.ConditionTrue { return machine, nil } @@ -2408,12 +2409,12 @@ func (c *controller) stopPreservationIfActive(ctx context.Context, machine *v1al var err error // removal of preserveExpiryTime is the last step of stopping preservation // therefore, if preserveExpiryTime is not set, machine is not preserved - nodeName := machine.Labels[v1alpha1.NodeLabelKey] if machine.Status.CurrentStatus.PreserveExpiryTime == nil { return machine, nil } - // if there is no backing node + nodeName := machine.Labels[v1alpha1.NodeLabelKey] + // if there is no backing node (such as in the case of self-hosted shoots), only machine object should be updated if nodeName == "" { // remove annotation from machine if needed if removePreservationAnnotations { @@ -2433,8 +2434,7 @@ func (c *controller) stopPreservationIfActive(ctx context.Context, machine *v1al // Machine has a backing node node, err := c.nodeLister.Get(nodeName) if err != nil { - // if node is not found and error is simply returned, then preservation will never be stopped on machine - // therefore, this error is handled specifically + // If node is not found and error is simply returned, then preservation will never be stopped on the machine. if apierrors.IsNotFound(err) { klog.Warningf("Node %q of machine %q not found. Proceeding to stop preservation on machine.", nodeName, machine.Name) if removePreservationAnnotations { @@ -2488,7 +2488,7 @@ func (c *controller) stopPreservationIfActive(ctx context.Context, machine *v1al Effect: v1.TaintEffectNoSchedule, }) if err != nil { - return false, err + return nil, err } // Step 5: update machine status to set preserve expiry time to nil @@ -2607,7 +2607,6 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M err error forceDeletePods bool timeOutOccurred bool - description string readOnlyFileSystemCondition, nodeReadyCondition v1.NodeCondition // Initialization @@ -2643,13 +2642,11 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M } if !isConditionEmpty(nodeReadyCondition) && (nodeReadyCondition.Status != v1.ConditionTrue) && (time.Since(nodeReadyCondition.LastTransitionTime.Time) > nodeNotReadyDuration) { - message := "Setting forceDeletePods to true for drain as machine is NotReady for over 5min" + klog.V(2).Infof("Setting forceDeletePods to true for drain because node %q with backing machine %q is NotReady for over 5min", nodeName, machine.Name) forceDeletePods = true - printLogInitError(message, &err, &description, machine, true) } else if !isConditionEmpty(readOnlyFileSystemCondition) && (readOnlyFileSystemCondition.Status != v1.ConditionFalse) && (time.Since(readOnlyFileSystemCondition.LastTransitionTime.Time) > nodeNotReadyDuration) { - message := "Setting forceDeletePods to true for drain as machine is in ReadonlyFilesystem for over 5min" + klog.V(2).Infof("Setting forceDeletePods to true for drain because node %q with backing machine %q is in ReadonlyFilesystem for over 5min", nodeName, machine.Name) forceDeletePods = true - printLogInitError(message, &err, &description, machine, true) } // TODO@thiyyakat: how to calculate timeout? In the case of preserve=now, PreserveExpiryTime will not coincide with time of failure in which case pods will get force @@ -2679,8 +2676,8 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M ) } - // 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 cordoning it. + // since we do not wish to change a user's explicit cordoning of a node, for preservation, we make use of + // a taint with effect `NoSchedule` before draining the node, instead of cordoning it. err = nodeops.AddOrUpdateTaintOnNode(ctx, c.targetCoreClient, nodeName, &v1.Taint{Key: machineutils.NodePreservedTaintKey, Effect: v1.TaintEffectNoSchedule}) if err != nil { klog.Errorf("tainting of backing node %q for machine %q, with providerID %q, failed with error: %v", nodeName, machine.Name, getProviderID(machine), err) @@ -2717,13 +2714,13 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M klog.V(3).Infof("(drainNode) Invoking RunDrain, timeOutDuration: %s", timeOutDuration) err = drainOptions.RunDrain(ctx) if err != nil { - klog.Errorf("drain failed for machine %q , providerID %q ,backing node %q. \nBuf:%v \nErrBuf:%v \nErr-Message:%v", machine.Name, getProviderID(machine), getNodeName(machine), buf, errBuf, err) + klog.Errorf("drain failed for machine %q, providerID %q, backing node %q. \nBuf:%v \nErrBuf:%v \nErr-Message:%v", machine.Name, getProviderID(machine), getNodeName(machine), buf, errBuf, err) return err } if forceDeletePods { - klog.V(3).Infof("Force drain successful for machine %q , providerID %q ,backing node %q.", machine.Name, getProviderID(machine), getNodeName(machine)) + klog.V(3).Infof("Force drain successful for machine %q, providerID %q, backing node %q.", machine.Name, getProviderID(machine), getNodeName(machine)) } else { - klog.V(3).Infof("Drain successful for machine %q , providerID %q ,backing node %q.", machine.Name, getProviderID(machine), getNodeName(machine)) + klog.V(3).Infof("Drain successful for machine %q, providerID %q, backing node %q.", machine.Name, getProviderID(machine), getNodeName(machine)) } return nil } diff --git a/pkg/util/provider/machinecontroller/node.go b/pkg/util/provider/machinecontroller/node.go index 47217d6285..b5ab82e525 100644 --- a/pkg/util/provider/machinecontroller/node.go +++ b/pkg/util/provider/machinecontroller/node.go @@ -340,18 +340,6 @@ func addedOrRemovedEssentialTaints(oldNode, node *corev1.Node, taintKeys []strin return false } -func (c *controller) getNodePreserveAnnotationValue(nodeName string) (string, error) { - node, err := c.nodeLister.Get(nodeName) - if err != nil { - if apierrors.IsNotFound(err) { - return "", nil - } - klog.Errorf("error fetching node %q: %v", nodeName, err) - return "", err - } - return node.Annotations[machineutils.PreserveMachineAnnotationKey], nil -} - // cordonNode sets `node.Spec.Unschedulable` to true, if not already set to true func (c *controller) cordonNode(ctx context.Context, nodeName string) error { node, err := c.targetCoreClient.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) @@ -370,25 +358,7 @@ func (c *controller) cordonNode(ctx context.Context, nodeName string) error { clone := node.DeepCopy() clone.Spec.Unschedulable = true _, err = c.targetCoreClient.CoreV1().Nodes().Update(ctx, clone, metav1.UpdateOptions{}) - if err != nil { - return err - } - return nil -} - -func (c *controller) uncordonNodeIfCordoned(ctx context.Context, node *corev1.Node) error { - if !node.Spec.Unschedulable { - return nil - } - nodeClone := node.DeepCopy() - nodeClone.Spec.Unschedulable = false - _, err := c.targetCoreClient.CoreV1().Nodes().Update(ctx, nodeClone, metav1.UpdateOptions{}) - if err != nil { - klog.Errorf("error uncordoning node %q: %v", node.Name, err) - return err - } - klog.Infof("Successfully uncordoned node %q", node.Name) - return nil + return err } // removePreservationRelatedAnnotationsOnNode removes the cluster-autoscaler annotation that disables scale down of preserved node From 6850d21b1b37fb10c81ed36a64c31009fbba67bc Mon Sep 17 00:00:00 2001 From: thiyyakat Date: Mon, 27 Jul 2026 15:11:42 +0530 Subject: [PATCH 7/8] Fix bug in `preserveMachine` that returns early when a machine preserved with preserve=now in the Running phase, transitions to Failed. This results in the node not being tainted or drained. --- .../machinecontroller/machine_util.go | 25 ++++++--- .../machinecontroller/machine_util_test.go | 54 +++++++++++++++---- 2 files changed, 63 insertions(+), 16 deletions(-) diff --git a/pkg/util/provider/machinecontroller/machine_util.go b/pkg/util/provider/machinecontroller/machine_util.go index a8a69ede47..a4cab71142 100644 --- a/pkg/util/provider/machinecontroller/machine_util.go +++ b/pkg/util/provider/machinecontroller/machine_util.go @@ -2373,8 +2373,15 @@ func (c *controller) preserveMachine(ctx context.Context, machine *v1alpha1.Mach return machine, err } existingNodePreservedCondition := nodeops.GetCondition(node, v1alpha1.NodePreserved) - // if the `Preserved` NodeCondition has ConditionStatus==ConditionTrue, preservation is complete - if existingNodePreservedCondition != nil && existingNodePreservedCondition.Status == v1.ConditionTrue { + drainRequired := shouldPreservedNodeBeDrained(existingNodePreservedCondition, machine.Status.CurrentStatus.Phase) + // For a Running machine, preservation is complete when ConditionStatus is True. However, for a Failed machine, + // preservation is complete only once the node is drained and tainted. + // Edge-case: when a machine in Running phase is preserved with preserve=now, and + // the machine transitions to Failed. + // In such cases, even though ConditionStatus would be set to True, on transitioning to + // Failed, the preservation needs to be considered as incomplete. + if existingNodePreservedCondition != nil && existingNodePreservedCondition.Status == v1.ConditionTrue && + !drainRequired { return machine, nil } // Step 2: Add annotations to prevent scale down of node by CA @@ -2383,7 +2390,7 @@ func (c *controller) preserveMachine(ctx context.Context, machine *v1alpha1.Mach return machine, err } var drainErr error - if shouldPreservedNodeBeDrained(existingNodePreservedCondition, machine.Status.CurrentStatus.Phase) { + if drainRequired { // Step 3: If machine is in Failed Phase, drain the backing node drainErr = c.drainPreservedNode(ctx, machine) } @@ -2674,11 +2681,15 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M timeOutDuration, maxEvictRetries, ) - } - - // since we do not wish to change a user's explicit cordoning of a node, for preservation, we make use of + } // since we do not wish to change a user's explicit cordoning of a node, for preservation, we make use of // a taint with effect `NoSchedule` before draining the node, instead of cordoning it. - err = nodeops.AddOrUpdateTaintOnNode(ctx, c.targetCoreClient, nodeName, &v1.Taint{Key: machineutils.NodePreservedTaintKey, Effect: v1.TaintEffectNoSchedule}) + err = nodeops.AddOrUpdateTaintOnNode(ctx, c.targetCoreClient, + nodeName, + &v1.Taint{ + Key: machineutils.NodePreservedTaintKey, + Effect: v1.TaintEffectNoSchedule, + TimeAdded: new(metav1.Now()), + }) if err != nil { klog.Errorf("tainting of backing node %q for machine %q, with providerID %q, failed with error: %v", nodeName, machine.Name, getProviderID(machine), err) return err diff --git a/pkg/util/provider/machinecontroller/machine_util_test.go b/pkg/util/provider/machinecontroller/machine_util_test.go index 76ef82e59e..339138af91 100644 --- a/pkg/util/provider/machinecontroller/machine_util_test.go +++ b/pkg/util/provider/machinecontroller/machine_util_test.go @@ -3961,12 +3961,13 @@ var _ = Describe("machine_util", func() { }) Describe("#preserveMachine", func() { type setup struct { - machinePhase machinev1.MachinePhase - nodeName string - preserveValue string - isCAAnnotationPresent bool - preservedNodeCondition corev1.NodeCondition - isUserCordoned bool + machinePhase machinev1.MachinePhase + nodeName string + preserveValue string + isCAAnnotationPresent bool + preservedNodeCondition corev1.NodeCondition + isUserCordoned bool + isPreserveExpiryTimeSet bool } type expect struct { preserveNodeCondition corev1.NodeCondition @@ -3998,9 +3999,15 @@ var _ = Describe("machine_util", func() { Spec: machinev1.MachineSpec{}, Status: machinev1.MachineStatus{ CurrentStatus: machinev1.CurrentStatus{ - Phase: tc.setup.machinePhase, - LastUpdateTime: metav1.Now(), - PreserveExpiryTime: nil, + Phase: tc.setup.machinePhase, + LastUpdateTime: metav1.Now(), + PreserveExpiryTime: func() *metav1.Time { + if tc.setup.isPreserveExpiryTimeSet { + t := metav1.NewTime(metav1.Now().Add(96 * time.Hour)) + return &t + } + return nil + }(), }, }, } @@ -4017,6 +4024,9 @@ var _ = Describe("machine_util", func() { Conditions: []corev1.NodeCondition{}, }, } + if tc.setup.preservedNodeCondition.Type != "" { + node.Status.Conditions = append(node.Status.Conditions, tc.setup.preservedNodeCondition) + } if tc.setup.isCAAnnotationPresent { node.Annotations[autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationKey] = "true" } @@ -4092,6 +4102,32 @@ var _ = Describe("machine_util", func() { }, }, }), + Entry("when preserve=now, machine was preserved while Running (NodePreserved=True set without drain), then transitions to Failed: drain and taint must be applied", &testCase{ + setup: setup{ + machinePhase: machinev1.MachineFailed, + nodeName: "node-1", + preserveValue: machineutils.PreserveMachineAnnotationValueNow, + isCAAnnotationPresent: true, + isPreserveExpiryTimeSet: true, + preservedNodeCondition: corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservedByUser, + }, + }, + expect: expect{ + err: nil, + isPreserveExpiryTimeSet: true, + isCAAnnotationPresent: true, + isNodeTainted: true, + preserveNodeCondition: corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservedByUser, + Message: machinev1.PreservedNodeDrainSuccessful, + }, + }, + }), Entry("when preserve=now, the machine has Failed, and there is a backing node", &testCase{ setup: setup{ machinePhase: machinev1.MachineFailed, From fa150102172354882672cd71503a7a9cbeb281b2 Mon Sep 17 00:00:00 2001 From: thiyyakat Date: Tue, 28 Jul 2026 14:43:00 +0530 Subject: [PATCH 8/8] Fix comment for clarity. Co-authored-by: Gagan --- pkg/util/provider/machinecontroller/machine_util.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/util/provider/machinecontroller/machine_util.go b/pkg/util/provider/machinecontroller/machine_util.go index a4cab71142..bfb925fd68 100644 --- a/pkg/util/provider/machinecontroller/machine_util.go +++ b/pkg/util/provider/machinecontroller/machine_util.go @@ -2681,7 +2681,8 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M timeOutDuration, maxEvictRetries, ) - } // since we do not wish to change a user's explicit cordoning of a node, for preservation, we make use of + } + // since we do not wish to change a user's explicit cordoning of a node, for preservation, we make use of // a taint with effect `NoSchedule` before draining the node, instead of cordoning it. err = nodeops.AddOrUpdateTaintOnNode(ctx, c.targetCoreClient, nodeName,