diff --git a/pkg/apis/machine/v1alpha1/machine_types.go b/pkg/apis/machine/v1alpha1/machine_types.go index 13db2a7d3d..51405c4c00 100644 --- a/pkg/apis/machine/v1alpha1/machine_types.go +++ b/pkg/apis/machine/v1alpha1/machine_types.go @@ -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" - // 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" - // 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" - // 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" ) // CurrentStatus contains information about the current status of Machine. diff --git a/pkg/util/nodeops/conditions.go b/pkg/util/nodeops/conditions.go index f3bafad774..ffc49685c0 100644 --- a/pkg/util/nodeops/conditions.go +++ b/pkg/util/nodeops/conditions.go @@ -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 { + 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) { diff --git a/pkg/util/provider/machinecontroller/machine_test.go b/pkg/util/provider/machinecontroller/machine_test.go index 0704f5e34d..67788147f9 100644 --- a/pkg/util/provider/machinecontroller/machine_test.go +++ b/pkg/util/provider/machinecontroller/machine_test.go @@ -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, }, }), @@ -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, }, }), @@ -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, @@ -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, @@ -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, @@ -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, diff --git a/pkg/util/provider/machinecontroller/machine_util.go b/pkg/util/provider/machinecontroller/machine_util.go index d34092e33e..2b570d6bea 100644 --- a/pkg/util/provider/machinecontroller/machine_util.go +++ b/pkg/util/provider/machinecontroller/machine_util.go @@ -78,6 +78,13 @@ const ( cacheUpdateTimeout = 1 * time.Second ) +// constants used to build the Preserved node condition message/ +const ( + preservedByUser = "Preserved by user." + autoPreservedByMCM = "Auto-preserved by MCM." + preserveExpiryMessagePrefix = "Machine preserved until" +) + // ValidateMachineClass validates the machine class. func (c *controller) ValidateMachineClass(_ context.Context, classSpec *v1alpha1.ClassSpec) (*v1alpha1.MachineClass, map[string][]byte, machineutils.RetryPeriod, error) { var ( @@ -2389,30 +2396,41 @@ func (c *controller) preserveMachine(ctx context.Context, machine *v1alpha1.Mach nodeName := machine.Labels[v1alpha1.NodeLabelKey] if nodeName == "" { - // Machine has no backing node( such as in the case of self-hosted shoots), preservation is complete + // If machine has no backing node (or no targetCoreClient), such as in the case of self-hosted shoots, + // preservation is complete after setting preserveExpiryTime on the machine. klog.V(2).Infof("Machine %q without backing node is preserved successfully till %v.", machine.Name, machine.Status.CurrentStatus.PreserveExpiryTime) return machine, nil } // Machine has a backing node node, err := c.nodeLister.Get(nodeName) if err != nil { - klog.Errorf("error trying to get node %q of machine %q: %v. Retrying.", nodeName, machine.Name, err) + klog.Errorf("error trying to get node %q of machine %q: %v.", nodeName, machine.Name, err) return machine, err } - existingNodePreservedCondition := nodeops.GetCondition(node, v1alpha1.NodePreserved) + nodeClone := node.DeepCopy() + existingNodePreservedCondition := nodeops.GetCondition(nodeClone, v1alpha1.NodePreserved) 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 { + // For a Running machine, preservation is complete once preserveExpiryTime is set on the machine and + // the CA annotations are set on the node. However, for a Failed machine, preservation is complete + // only after the node is tainted and drained. When a machine in Running phase is preserved with preserve=now, and + // the machine transitions to Failed, the node needs to be drained, even though preservation was complete earlier. + if existingNodePreservedCondition != nil && + (existingNodePreservedCondition.Reason == v1alpha1.PreservationWithDrainCompleted || + (existingNodePreservedCondition.Reason == v1alpha1.PreservationWithoutDrainCompleted && !drainRequired)) { return machine, nil } + + if existingNodePreservedCondition == nil { + initialCond := getInitializedPreservedNodeCondition(preserveValue, machine.Status.CurrentStatus.PreserveExpiryTime) + if _, err = nodeops.AddOrUpdateConditionsOnNode(ctx, c.targetCoreClient, nodeName, initialCond); err != nil { + klog.Errorf("error setting initial Preserved node condition on node %q of machine %q: %v", nodeName, machine.Name, err) + return machine, err + } + existingNodePreservedCondition = &initialCond + } + // Step 2: Add annotations to prevent scale down of node by CA - updatedNode, err := c.addCAScaleDownDisabledAnnotationOnNode(ctx, node) + err = c.addCAScaleDownDisabledAnnotationOnNode(ctx, nodeName) if err != nil { return machine, err } @@ -2420,20 +2438,28 @@ func (c *controller) preserveMachine(ctx context.Context, machine *v1alpha1.Mach if drainRequired { // Step 3: If machine is in Failed Phase, drain the backing node drainErr = c.drainPreservedNode(ctx, machine) - } - newCond, needsUpdate := computeNewNodePreservedCondition(machine.Status.CurrentStatus, preserveValue, drainErr, existingNodePreservedCondition) - if needsUpdate { - // Step 4: Update NodePreserved Condition on Node, with drain status - _, err = nodeops.AddOrUpdateConditionsOnNode(ctx, c.targetCoreClient, updatedNode.Name, *newCond) if drainErr != nil { klog.Errorf("error draining preserved node %q for machine %q : %v", nodeName, machine.Name, drainErr) - return machine, drainErr } + } + + // 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) - return machine, err } } + + if drainErr != nil { + return machine, drainErr + } + + if err != nil { + return machine, err + } + klog.V(2).Infof("Machine %q and backing node %q preserved successfully till %v.", machine.Name, nodeName, machine.Status.CurrentStatus.PreserveExpiryTime) return machine, nil } @@ -2487,15 +2513,8 @@ 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, - Status: v1.ConditionFalse, - LastTransitionTime: metav1.Now(), - Reason: v1alpha1.PreservationStopped, - } - // Step 1: change node condition to reflect that preservation has stopped - updatedNode, err := nodeops.AddOrUpdateConditionsOnNode(ctx, c.targetCoreClient, node.Name, preservedConditionFalse) + // Step 1: remove Preserved Node Condition. + updatedNode, err := nodeops.RemoveConditionFromNode(ctx, c.targetCoreClient, node.Name, v1alpha1.NodePreserved) if err != nil { return nil, err } @@ -2548,56 +2567,63 @@ func (c *controller) setPreserveExpiryTimeOnMachine(ctx context.Context, machine return updatedMachine, nil } -// computeNewNodePreservedCondition returns the NodeCondition with the values set according to the preserveValue and the stage of Preservation -func computeNewNodePreservedCondition(currentStatus v1alpha1.CurrentStatus, preserveValue string, drainErr error, existingNodeCondition *v1.NodeCondition) (*v1.NodeCondition, bool) { - const preserveExpiryMessageSuffix = "Machine preserved until" - var newNodePreservedCondition *v1.NodeCondition - var needsUpdate bool +// recomputePreservedNodeCondition returns the NodeCondition with the values set according to the preserveValue and the stage of Preservation +func recomputePreservedNodeCondition(currentStatus v1alpha1.CurrentStatus, preserveValue string, drainErr error, existingNodeCondition *v1.NodeCondition) *v1.NodeCondition { + var newCond *v1.NodeCondition if existingNodeCondition == nil { - newNodePreservedCondition = &v1.NodeCondition{ - Type: v1alpha1.NodePreserved, - Status: v1.ConditionFalse, - LastTransitionTime: metav1.Now(), - } - needsUpdate = true + initialCond := getInitializedPreservedNodeCondition(preserveValue, currentStatus.PreserveExpiryTime) + newCond = &initialCond } else { - newNodePreservedCondition = existingNodeCondition.DeepCopy() - } - machinePhase := currentStatus.Phase - if machinePhase == v1alpha1.MachineFailed { - if drainErr == nil { - if !strings.Contains(newNodePreservedCondition.Message, v1alpha1.PreservedNodeDrainSuccessful) { - newNodePreservedCondition.Message = fmt.Sprintf("%s %s %v.", v1alpha1.PreservedNodeDrainSuccessful, preserveExpiryMessageSuffix, currentStatus.PreserveExpiryTime) - newNodePreservedCondition.Status = v1.ConditionTrue - needsUpdate = true - } - } else if !strings.Contains(newNodePreservedCondition.Message, v1alpha1.PreservedNodeDrainUnsuccessful) { - newNodePreservedCondition.Message = fmt.Sprintf("%s %s %v.", v1alpha1.PreservedNodeDrainUnsuccessful, preserveExpiryMessageSuffix, currentStatus.PreserveExpiryTime) - newNodePreservedCondition.Status = v1.ConditionFalse - needsUpdate = true + newCond = existingNodeCondition.DeepCopy() + } + newCond.LastTransitionTime = metav1.Now() + // there is no need to set ConditionStatus to ConditionTrue since it is handled by getInitializedNodePreservedCondition() + if currentStatus.Phase == v1alpha1.MachineFailed { + if drainErr != nil { + newCond.Reason = v1alpha1.DrainFailed + // compute message again before comparison in case drain is failing due to a different reason. + newCond.Message = buildPreservedNodeConditionMessage(fmt.Sprintf("Preserved node could not be drained: %v.", drainErr), preserveValue, currentStatus.PreserveExpiryTime) + return newCond } - } else if newNodePreservedCondition.Status != v1.ConditionTrue { - newNodePreservedCondition.Status = v1.ConditionTrue - newNodePreservedCondition.Message = fmt.Sprintf("%s %v.", preserveExpiryMessageSuffix, currentStatus.PreserveExpiryTime) - needsUpdate = true + newCond.Reason = v1alpha1.PreservationWithDrainCompleted + newCond.Message = buildPreservedNodeConditionMessage("Preserved node drained successfully.", preserveValue, currentStatus.PreserveExpiryTime) + return newCond } - if preserveValue == machineutils.PreserveMachineAnnotationValueAutoPreserved { - newNodePreservedCondition.Reason = v1alpha1.PreservedByMCM - } else { - newNodePreservedCondition.Reason = v1alpha1.PreservedByUser + newCond.Reason = v1alpha1.PreservationWithoutDrainCompleted + newCond.Message = buildPreservedNodeConditionMessage("", preserveValue, currentStatus.PreserveExpiryTime) + return newCond +} + +// needsPreservedNodeConditionUpdate returns true if newCond is not semantically equal to oldCond, +// or if either condition is nil. +// In both cases the node needs to be updated with newCond +func needsPreservedNodeConditionUpdate(oldCond, newCond *v1.NodeCondition) bool { + if oldCond == nil || newCond == nil { + return true + } + return oldCond.Status != newCond.Status || oldCond.Reason != newCond.Reason || oldCond.Message != newCond.Message +} + +// getInitializedPreservedNodeCondition returns an initialized Node Condition of Type Preserved +func getInitializedPreservedNodeCondition(value string, preserveExpiryTime *metav1.Time) v1.NodeCondition { + return v1.NodeCondition{ + Type: v1alpha1.NodePreserved, + Status: v1.ConditionTrue, // since preserveExpiryTime is the gate for checking if a machine is preserved, and this is already checked + Reason: v1alpha1.PreservationInProgress, + Message: buildPreservedNodeConditionMessage("Preservation in progress.", value, preserveExpiryTime), + LastTransitionTime: metav1.Now(), } - return newNodePreservedCondition, needsUpdate } // shouldPreservedNodeBeDrained returns true if the machine's backing node must be drained, else false func shouldPreservedNodeBeDrained(existingCondition *v1.NodeCondition, machinePhase v1alpha1.MachinePhase) bool { - if machinePhase == v1alpha1.MachineFailed { - if existingCondition == nil { - return true - } - return !strings.Contains(existingCondition.Message, v1alpha1.PreservedNodeDrainSuccessful) + if machinePhase != v1alpha1.MachineFailed { + return false } - return false + if existingCondition == nil { + return true + } + return existingCondition.Reason != v1alpha1.PreservationWithDrainCompleted } // clearMachinePreserveExpiryTime clears the PreserveExpiryTime on the machine object's Status.CurrentStatus @@ -2630,6 +2656,22 @@ func (c *controller) removePreserveAnnotationOnMachine(ctx context.Context, mach return updatedClone, nil } +func preservationCause(preserveValue string) string { + why := preservedByUser + if preserveValue == machineutils.PreserveMachineAnnotationValueAutoPreserved { + why = autoPreservedByMCM + } + return why +} + +func buildPreservedNodeConditionMessage(custom, preserveValue string, preserveExpiryTime *metav1.Time) string { + why := preservationCause(preserveValue) + if custom != "" { + return fmt.Sprintf("%s %s %s %v.", custom, why, preserveExpiryMessagePrefix, preserveExpiryTime) + } + return fmt.Sprintf("%s %s %v.", why, preserveExpiryMessagePrefix, preserveExpiryTime) +} + // drainPreservedNode attempts to drain the node backing a preserved machine func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.Machine) error { var ( @@ -2697,7 +2739,7 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M ) } else { klog.V(2).Infof( - "Drain has been triggerred for preserved machine %q with providerID %q and backing node %q with drain-timeout:%v & maxEvictRetries:%d", + "Drain has been triggered for preserved machine %q with providerID %q and backing node %q with drain-timeout:%v & maxEvictRetries:%d", machine.Name, getProviderID(machine), getNodeName(machine), @@ -2707,12 +2749,13 @@ func (c *controller) drainPreservedNode(ctx context.Context, machine *v1alpha1.M } // 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. + timeAdded := metav1.Now() err = nodeops.AddOrUpdateTaintOnNode(ctx, c.targetCoreClient, nodeName, &v1.Taint{ Key: machineutils.NodePreservedTaintKey, Effect: v1.TaintEffectNoSchedule, - TimeAdded: new(metav1.Now()), + TimeAdded: &timeAdded, }) if err != nil { klog.Errorf("tainting of backing node %q for preserved machine %q, with providerID %q, failed with error: %v", nodeName, machine.Name, getProviderID(machine), err) diff --git a/pkg/util/provider/machinecontroller/machine_util_test.go b/pkg/util/provider/machinecontroller/machine_util_test.go index 339138af91..72bed2ffcd 100644 --- a/pkg/util/provider/machinecontroller/machine_util_test.go +++ b/pkg/util/provider/machinecontroller/machine_util_test.go @@ -9,6 +9,7 @@ import ( "encoding/json" "errors" "fmt" + "math" "time" "github.com/gardener/machine-controller-manager/pkg/controller/autoscaler" @@ -25,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + k8stesting "k8s.io/client-go/testing" "k8s.io/utils/ptr" ) @@ -4098,7 +4100,7 @@ var _ = Describe("machine_util", func() { preserveNodeCondition: corev1.NodeCondition{ Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, + Reason: machinev1.PreservationWithoutDrainCompleted, }, }, }), @@ -4112,7 +4114,7 @@ var _ = Describe("machine_util", func() { preservedNodeCondition: corev1.NodeCondition{ Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, + Reason: machinev1.PreservationWithoutDrainCompleted, }, }, expect: expect{ @@ -4121,10 +4123,9 @@ var _ = Describe("machine_util", func() { isCAAnnotationPresent: true, isNodeTainted: true, preserveNodeCondition: corev1.NodeCondition{ - Type: machinev1.NodePreserved, - Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, - Message: machinev1.PreservedNodeDrainSuccessful, + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, }, }, }), @@ -4140,10 +4141,9 @@ var _ = Describe("machine_util", func() { isCAAnnotationPresent: true, isNodeTainted: true, preserveNodeCondition: corev1.NodeCondition{ - Type: machinev1.NodePreserved, - Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, - Message: machinev1.PreservedNodeDrainSuccessful, + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, }, }, }), @@ -4159,10 +4159,9 @@ var _ = Describe("machine_util", func() { isCAAnnotationPresent: true, isNodeTainted: true, preserveNodeCondition: corev1.NodeCondition{ - Type: machinev1.NodePreserved, - Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, - Message: machinev1.PreservedNodeDrainSuccessful, + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, }, }, }), @@ -4179,10 +4178,9 @@ var _ = Describe("machine_util", func() { isCAAnnotationPresent: true, isNodeTainted: true, preserveNodeCondition: corev1.NodeCondition{ - Type: machinev1.NodePreserved, - Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, - Message: machinev1.PreservedNodeDrainSuccessful, + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, }, }, }), @@ -4193,10 +4191,9 @@ var _ = Describe("machine_util", func() { preserveValue: machineutils.PreserveMachineAnnotationValueNow, isCAAnnotationPresent: true, preservedNodeCondition: corev1.NodeCondition{ - Type: machinev1.NodePreserved, - Status: corev1.ConditionFalse, - Reason: machinev1.PreservedByUser, - Message: machinev1.PreservedNodeDrainUnsuccessful, + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.DrainFailed, }, }, expect: expect{ @@ -4205,10 +4202,9 @@ var _ = Describe("machine_util", func() { isCAAnnotationPresent: true, isNodeTainted: true, preserveNodeCondition: corev1.NodeCondition{ - Type: machinev1.NodePreserved, - Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, - Message: machinev1.PreservedNodeDrainSuccessful, + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, }, }, }), @@ -4224,10 +4220,9 @@ var _ = Describe("machine_util", func() { isCAAnnotationPresent: true, isNodeTainted: true, preserveNodeCondition: corev1.NodeCondition{ - Type: machinev1.NodePreserved, - Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, - Message: machinev1.PreservedNodeDrainSuccessful, + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, }, }, }), @@ -4243,10 +4238,9 @@ var _ = Describe("machine_util", func() { isCAAnnotationPresent: true, isNodeTainted: true, preserveNodeCondition: corev1.NodeCondition{ - Type: machinev1.NodePreserved, - Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByMCM, - Message: machinev1.PreservedNodeDrainSuccessful, + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, }, }, }), @@ -4277,7 +4271,7 @@ var _ = Describe("machine_util", func() { preserveNodeCondition: corev1.NodeCondition{ Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, + Reason: machinev1.PreservationWithoutDrainCompleted, }, }, }), @@ -4294,14 +4288,130 @@ var _ = Describe("machine_util", func() { isCAAnnotationPresent: true, isNodeTainted: true, preserveNodeCondition: corev1.NodeCondition{ - Type: machinev1.NodePreserved, - Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, - Message: machinev1.PreservedNodeDrainSuccessful, + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, }, }, }), ) + It("when preserve=now, machine is Failed, and drain fails, should set NodePreserved reason to DrainFailed", func() { + stop := make(chan struct{}) + defer close(stop) + + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-1", + Annotations: map[string]string{ + autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationKey: autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationValue, + }, + }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationInProgress, + }, + }, + }, + } + machine := &machinev1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-1", + Namespace: testNamespace, + Labels: map[string]string{machinev1.NodeLabelKey: "node-1"}, + }, + Status: machinev1.MachineStatus{ + CurrentStatus: machinev1.CurrentStatus{ + Phase: machinev1.MachineFailed, + LastUpdateTime: metav1.Now(), + }, + }, + } + + c, trackers := createController(stop, testNamespace, []runtime.Object{machine}, nil, []runtime.Object{node}, nil, false) + defer trackers.Stop() + waitForCacheSync(stop, c) + + _ = trackers.TargetCore.SetFakeResourceActions( + &fakeclient.ResourceActions{ + Node: fakeclient.Actions{Update: "taint update failed"}, + }, + math.MaxInt32, + ) + + _, err := c.preserveMachine(context.TODO(), machine, machineutils.PreserveMachineAnnotationValueNow) + Expect(err).To(HaveOccurred()) + + fakeClient := c.targetCoreClient.(*fakeclient.Clientset) + var lastStatusUpdate *corev1.Node + for _, action := range fakeClient.Actions() { + if action.GetVerb() == "update" && action.GetSubresource() == "status" { + lastStatusUpdate = action.(k8stesting.UpdateAction).GetObject().(*corev1.Node) + } + } + Expect(lastStatusUpdate).NotTo(BeNil()) + cond := nodeops.GetCondition(lastStatusUpdate, machinev1.NodePreserved) + Expect(cond).NotTo(BeNil()) + Expect(cond.Status).To(Equal(corev1.ConditionTrue)) + Expect(cond.Reason).To(Equal(machinev1.DrainFailed)) + }) + It("when preserve=now, machine is Failed, and CA annotation update fails, should set NodePreserved reason to PreservationInProgress", func() { + stop := make(chan struct{}) + defer close(stop) + + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-1", + Annotations: map[string]string{}, + }, + } + machine := &machinev1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-1", + Namespace: testNamespace, + Labels: map[string]string{machinev1.NodeLabelKey: "node-1"}, + }, + Status: machinev1.MachineStatus{ + CurrentStatus: machinev1.CurrentStatus{ + Phase: machinev1.MachineFailed, + LastUpdateTime: metav1.Now(), + }, + }, + } + + c, trackers := createController(stop, testNamespace, []runtime.Object{machine}, nil, []runtime.Object{node}, nil, false) + defer trackers.Stop() + waitForCacheSync(stop, c) + + _ = trackers.TargetCore.SetFakeResourceActions( + &fakeclient.ResourceActions{ + Node: fakeclient.Actions{Update: "CA annotation update failed"}, + }, + math.MaxInt32, + ) + + _, err := c.preserveMachine(context.TODO(), machine, machineutils.PreserveMachineAnnotationValueNow) + Expect(err).To(HaveOccurred()) + + updatedMachine, getErr := c.controlMachineClient.Machines(testNamespace).Get(context.TODO(), machine.Name, metav1.GetOptions{}) + Expect(getErr).To(BeNil()) + Expect(updatedMachine.Status.CurrentStatus.PreserveExpiryTime.IsZero()).To(BeFalse()) + + fakeClient := c.targetCoreClient.(*fakeclient.Clientset) + var lastStatusUpdate *corev1.Node + for _, action := range fakeClient.Actions() { + if action.GetVerb() == "update" && action.GetSubresource() == "status" { + lastStatusUpdate = action.(k8stesting.UpdateAction).GetObject().(*corev1.Node) + } + } + Expect(lastStatusUpdate).NotTo(BeNil()) + cond := nodeops.GetCondition(lastStatusUpdate, machinev1.NodePreserved) + Expect(cond).NotTo(BeNil()) + Expect(cond.Status).To(Equal(corev1.ConditionTrue)) + Expect(cond.Reason).To(Equal(machinev1.PreservationInProgress)) + }) }) Describe("#cordonNode", func() { type setup struct { @@ -4439,7 +4549,7 @@ var _ = Describe("machine_util", func() { { Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, + Reason: machinev1.PreservationWithoutDrainCompleted, }, }, }, @@ -4477,9 +4587,7 @@ var _ = Describe("machine_util", func() { updatedNode, getErr := c.targetCoreClient.CoreV1().Nodes().Get(context.TODO(), tc.setup.nodeName, metav1.GetOptions{}) Expect(getErr).To(BeNil()) updatedNodeCondition := nodeops.GetCondition(updatedNode, machinev1.NodePreserved) - Expect(updatedNodeCondition).ToNot(BeNil()) - Expect(updatedNodeCondition.Status).To(Equal(corev1.ConditionFalse)) - Expect(updatedNodeCondition.Reason).To(Equal(machinev1.PreservationStopped)) + Expect(updatedNodeCondition).To(BeNil()) if tc.setup.removePreserveAnnotation { Expect(updatedNode.Annotations).NotTo(HaveKey(machineutils.PreserveMachineAnnotationKey)) } else { @@ -4556,7 +4664,7 @@ var _ = Describe("machine_util", func() { }), ) }) - Describe("#computeNewNodePreservedCondition", func() { + Describe("#recomputePreservedNodeCondition", func() { preserveExpiryTime := &metav1.Time{Time: time.Now().Add(2 * time.Hour)} type setup struct { currentStatus machinev1.CurrentStatus @@ -4566,15 +4674,14 @@ var _ = Describe("machine_util", func() { } type expect struct { newNodeCondition *corev1.NodeCondition - needsUpdate bool } type testCase struct { setup setup expect expect } - DescribeTable("##computeNewNodePreservedCondition behaviour scenarios", + DescribeTable("##recomputePreservedNodeCondition behaviour scenarios", func(tc *testCase) { - newNodeCondition, needsUpdate := computeNewNodePreservedCondition( + newNodeCondition := recomputePreservedNodeCondition( tc.setup.currentStatus, tc.setup.preserveValue, tc.setup.drainErr, @@ -4586,9 +4693,10 @@ var _ = Describe("machine_util", func() { Expect(newNodeCondition.Type).To(Equal(tc.expect.newNodeCondition.Type)) Expect(newNodeCondition.Status).To(Equal(tc.expect.newNodeCondition.Status)) Expect(newNodeCondition.Reason).To(Equal(tc.expect.newNodeCondition.Reason)) - Expect(newNodeCondition.Message).To(Equal(tc.expect.newNodeCondition.Message)) + if tc.expect.newNodeCondition.Message != "" { + Expect(newNodeCondition.Message).To(Equal(tc.expect.newNodeCondition.Message)) + } } - Expect(needsUpdate).To(Equal(tc.expect.needsUpdate)) }, Entry("when preserve=now, machine is Running, no existing condition", &testCase{ setup: setup{ @@ -4597,158 +4705,146 @@ var _ = Describe("machine_util", func() { LastUpdateTime: metav1.Now(), PreserveExpiryTime: preserveExpiryTime, }, - preserveValue: machineutils.PreserveMachineAnnotationValueNow, - existingNodeCondition: nil, - }, - expect: expect{ - newNodeCondition: &corev1.NodeCondition{ + preserveValue: machineutils.PreserveMachineAnnotationValueNow, + existingNodeCondition: &corev1.NodeCondition{ Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, - Message: fmt.Sprintf("Machine preserved until %v.", preserveExpiryTime), - }, - needsUpdate: true, - }, - }), - Entry("when preserve=now, machine is Failed, drain successful, no existing condition", &testCase{ - setup: setup{ - currentStatus: machinev1.CurrentStatus{ - Phase: machinev1.MachineFailed, - LastUpdateTime: metav1.Now(), - PreserveExpiryTime: preserveExpiryTime, + Reason: machinev1.PreservationInProgress, + Message: fmt.Sprintf("Preservation in progress. %s", preservedByUser), }, - preserveValue: machineutils.PreserveMachineAnnotationValueNow, - drainErr: nil, - existingNodeCondition: nil, }, expect: expect{ newNodeCondition: &corev1.NodeCondition{ Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, - Message: fmt.Sprintf("%s Machine preserved until %v.", machinev1.PreservedNodeDrainSuccessful, preserveExpiryTime), + Reason: machinev1.PreservationWithoutDrainCompleted, + Message: fmt.Sprintf("%s %s %v.", preservedByUser, preserveExpiryMessagePrefix, preserveExpiryTime), }, - needsUpdate: true, }, }), - Entry("when preserve=now, machine is Failed, drain is unsuccessful, no existing condition", &testCase{ + Entry("when preserve=now, machine is Failed, drain is successful", &testCase{ setup: setup{ currentStatus: machinev1.CurrentStatus{ Phase: machinev1.MachineFailed, LastUpdateTime: metav1.Now(), PreserveExpiryTime: preserveExpiryTime, }, - preserveValue: machineutils.PreserveMachineAnnotationValueNow, - drainErr: fmt.Errorf("test drain error"), - existingNodeCondition: nil, + preserveValue: machineutils.PreserveMachineAnnotationValueNow, + drainErr: nil, + existingNodeCondition: &corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationInProgress, + Message: fmt.Sprintf("Preservation in progress. %s", preservedByUser), + }, }, expect: expect{ newNodeCondition: &corev1.NodeCondition{ Type: machinev1.NodePreserved, - Status: corev1.ConditionFalse, - Reason: machinev1.PreservedByUser, - Message: fmt.Sprintf("%s Machine preserved until %v.", machinev1.PreservedNodeDrainUnsuccessful, preserveExpiryTime), + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, + Message: fmt.Sprintf("Preserved node drained successfully. %s %s %v.", preservedByUser, preserveExpiryMessagePrefix, preserveExpiryTime), }, - needsUpdate: true, }, }), - Entry("when machine auto-preserved by MCM, machine is Failed, drain is successful, no existing condition", &testCase{ + Entry("when preserve=now, machine is Failed, drain is unsuccessful in the first attempt", &testCase{ setup: setup{ currentStatus: machinev1.CurrentStatus{ Phase: machinev1.MachineFailed, LastUpdateTime: metav1.Now(), PreserveExpiryTime: preserveExpiryTime, }, - preserveValue: machineutils.PreserveMachineAnnotationValueAutoPreserved, - drainErr: nil, - existingNodeCondition: nil, + preserveValue: machineutils.PreserveMachineAnnotationValueNow, + drainErr: fmt.Errorf("test drain error"), + existingNodeCondition: &corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationInProgress, + Message: fmt.Sprintf("Preservation in progress. %s", preservedByUser), + }, }, expect: expect{ newNodeCondition: &corev1.NodeCondition{ Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByMCM, - Message: fmt.Sprintf("%s Machine preserved until %v.", machinev1.PreservedNodeDrainSuccessful, preserveExpiryTime), + Reason: machinev1.DrainFailed, + Message: fmt.Sprintf("Preserved node could not be drained: %v. %s %s %v.", "test drain error", preservedByUser, preserveExpiryMessagePrefix, preserveExpiryTime), }, - needsUpdate: true, }, }), - Entry("when preserve=now, machine is Failed, drain is unsuccessful, existing condition present", &testCase{ + Entry("when machine auto-preserved by MCM, machine is Failed, drain is successful in the first attempt", &testCase{ setup: setup{ currentStatus: machinev1.CurrentStatus{ Phase: machinev1.MachineFailed, LastUpdateTime: metav1.Now(), PreserveExpiryTime: preserveExpiryTime, }, - preserveValue: machineutils.PreserveMachineAnnotationValueNow, - drainErr: fmt.Errorf("test drain error"), + preserveValue: machineutils.PreserveMachineAnnotationValueAutoPreserved, + drainErr: nil, existingNodeCondition: &corev1.NodeCondition{ Type: machinev1.NodePreserved, - Status: corev1.ConditionFalse, - Reason: machinev1.PreservedByUser, - Message: "Machine preserved until " + preserveExpiryTime.String(), + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationInProgress, + Message: fmt.Sprintf("Preservation in progress. %s", autoPreservedByMCM), }, }, expect: expect{ newNodeCondition: &corev1.NodeCondition{ Type: machinev1.NodePreserved, - Status: corev1.ConditionFalse, - Reason: machinev1.PreservedByUser, - Message: fmt.Sprintf("%s Machine preserved until %v.", machinev1.PreservedNodeDrainUnsuccessful, preserveExpiryTime), + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, + Message: fmt.Sprintf("Preserved node drained successfully. %s %s %v.", autoPreservedByMCM, preserveExpiryMessagePrefix, preserveExpiryTime), }, - needsUpdate: true, }, }), - Entry("when preserve=now, machine is Failed, drain is unsuccessful for the second time, existing condition present", &testCase{ + Entry("when preserve=now was initially added on a Running machine, machine transitions to Failed later, and drain is unsuccessful on the first attempt", &testCase{ setup: setup{ currentStatus: machinev1.CurrentStatus{ Phase: machinev1.MachineFailed, LastUpdateTime: metav1.Now(), - PreserveExpiryTime: &metav1.Time{Time: time.Now().Add(2 * time.Hour)}, + PreserveExpiryTime: preserveExpiryTime, }, preserveValue: machineutils.PreserveMachineAnnotationValueNow, drainErr: fmt.Errorf("test drain error"), existingNodeCondition: &corev1.NodeCondition{ Type: machinev1.NodePreserved, - Status: corev1.ConditionFalse, - Reason: machinev1.PreservedByUser, - Message: fmt.Sprintf("%s Machine preserved until %v.", machinev1.PreservedNodeDrainUnsuccessful, preserveExpiryTime), + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithoutDrainCompleted, + Message: fmt.Sprintf("%s %s %v.", preservedByUser, preserveExpiryMessagePrefix, preserveExpiryTime), }, }, expect: expect{ newNodeCondition: &corev1.NodeCondition{ Type: machinev1.NodePreserved, - Status: corev1.ConditionFalse, - Reason: machinev1.PreservedByUser, - Message: fmt.Sprintf("%s Machine preserved until %v.", machinev1.PreservedNodeDrainUnsuccessful, preserveExpiryTime), + Status: corev1.ConditionTrue, + Reason: machinev1.DrainFailed, + Message: fmt.Sprintf("Preserved node could not be drained: %v. %s %s %v.", "test drain error", preservedByUser, preserveExpiryMessagePrefix, preserveExpiryTime), }, - needsUpdate: false, }, }), - Entry("when preserve=now, machine is Failed, drain is successful, existing condition present and status is true", &testCase{ + Entry("when preserve=now, machine is Failed, drain is unsuccessful for the second time with the same error", &testCase{ setup: setup{ currentStatus: machinev1.CurrentStatus{ Phase: machinev1.MachineFailed, LastUpdateTime: metav1.Now(), - PreserveExpiryTime: &metav1.Time{Time: time.Now().Add(2 * time.Hour)}, + PreserveExpiryTime: preserveExpiryTime, }, preserveValue: machineutils.PreserveMachineAnnotationValueNow, - drainErr: nil, + drainErr: fmt.Errorf("test drain error"), existingNodeCondition: &corev1.NodeCondition{ Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, - Message: fmt.Sprintf("%s Machine preserved until %v.", machinev1.PreservedNodeDrainSuccessful, preserveExpiryTime), + Reason: machinev1.DrainFailed, + Message: fmt.Sprintf("Preserved node could not be drained: %v. %s %s %v.", "test drain error", preservedByUser, preserveExpiryMessagePrefix, preserveExpiryTime), }, }, expect: expect{ newNodeCondition: &corev1.NodeCondition{ Type: machinev1.NodePreserved, Status: corev1.ConditionTrue, - Reason: machinev1.PreservedByUser, - Message: fmt.Sprintf("%s Machine preserved until %v.", machinev1.PreservedNodeDrainSuccessful, preserveExpiryTime), + Reason: machinev1.DrainFailed, + Message: fmt.Sprintf("Preserved node could not be drained: %v. %s %s %v.", "test drain error", preservedByUser, preserveExpiryMessagePrefix, preserveExpiryTime), }, - needsUpdate: false, }, }), ) @@ -4787,14 +4883,13 @@ var _ = Describe("machine_util", func() { shouldDrain: true, }, }), - Entry("should return true when machine is Failed and existing node condition message is PreservedNodeDrainUnsuccessful", &testCase{ + Entry("should return true when machine is Failed and existing node condition reason is DrainFailed", &testCase{ setup: setup{ machinePhase: machinev1.MachineFailed, existingCondition: &corev1.NodeCondition{ - Type: machinev1.NodePreserved, - Status: corev1.ConditionFalse, - Reason: machinev1.PreservedByUser, - Message: machinev1.PreservedNodeDrainUnsuccessful, + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.DrainFailed, }, }, expect: expect{ @@ -4923,4 +5018,114 @@ var _ = Describe("machine_util", func() { }), ) }) + Describe("#needsPreservedNodeConditionUpdate", func() { + type setup struct { + oldCond *corev1.NodeCondition + newCond *corev1.NodeCondition + } + type expect struct { + needsUpdate bool + } + type testCase struct { + setup setup + expect expect + } + + DescribeTable("##needsPreservedNodeConditionUpdate behaviour scenarios", + func(tc *testCase) { + needsUpdate := needsPreservedNodeConditionUpdate(tc.setup.oldCond, tc.setup.newCond) + Expect(needsUpdate).To(Equal(tc.expect.needsUpdate)) + }, + Entry("should return true when oldCond is nil", &testCase{ + setup: setup{ + oldCond: nil, + newCond: &corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, + }, + }, + expect: expect{needsUpdate: true}, + }), + Entry("should return true when newCond is nil", &testCase{ + setup: setup{ + oldCond: &corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, + }, + newCond: nil, + }, + expect: expect{needsUpdate: true}, + }), + Entry("should return false when both conditions are semantically equal", &testCase{ + setup: setup{ + oldCond: &corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, + Message: "some message", + }, + newCond: &corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, + Message: "some message", + }, + }, + expect: expect{needsUpdate: false}, + }), + Entry("should return true when Status differs", &testCase{ + setup: setup{ + oldCond: &corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionFalse, // this is a hypothetical case. MCM never sets ConditionFalse for this condition type + Reason: machinev1.PreservationWithDrainCompleted, + Message: "some message", + }, + newCond: &corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, + Message: "some message", + }, + }, + expect: expect{needsUpdate: true}, + }), + Entry("should return true when Reason differs", &testCase{ + setup: setup{ + oldCond: &corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationInProgress, + Message: "some message", + }, + newCond: &corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, + Message: "some message", + }, + }, + expect: expect{needsUpdate: true}, + }), + Entry("should return true when Message differs", &testCase{ + setup: setup{ + oldCond: &corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, + Message: "old message", + }, + newCond: &corev1.NodeCondition{ + Type: machinev1.NodePreserved, + Status: corev1.ConditionTrue, + Reason: machinev1.PreservationWithDrainCompleted, + Message: "new message", + }, + }, + expect: expect{needsUpdate: true}, + }), + ) + }) }) diff --git a/pkg/util/provider/machinecontroller/node.go b/pkg/util/provider/machinecontroller/node.go index 3c5b030df0..9516fd4c1c 100644 --- a/pkg/util/provider/machinecontroller/node.go +++ b/pkg/util/provider/machinecontroller/node.go @@ -15,11 +15,13 @@ import ( "github.com/gardener/machine-controller-manager/pkg/controller/autoscaler" "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" + "github.com/gardener/machine-controller-manager/pkg/util/nodeops" "github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils" corev1 "k8s.io/api/core/v1" "k8s.io/klog/v2" "k8s.io/client-go/tools/cache" + clientretry "k8s.io/client-go/util/retry" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -396,23 +398,38 @@ func (c *controller) removePreservationRelatedAnnotationsOnNode(ctx context.Cont } // addCAScaleDownDisabledAnnotationOnNode adds the cluster-autoscaler annotation to disable scale down of preserved node -func (c *controller) addCAScaleDownDisabledAnnotationOnNode(ctx context.Context, node *corev1.Node) (*corev1.Node, error) { - // Check if annotation already exists with correct value - if node.Annotations[autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationKey] == autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationValue { - return node, nil - } - // Add annotation to disable CA scale down. - // Also add annotation expressing that MCM is the one who added this annotation, so that it can be removed safely when preservation is stopped. - nodeCopy := node.DeepCopy() - if node.Annotations == nil { - nodeCopy.Annotations = make(map[string]string) - } - nodeCopy.Annotations[autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationKey] = autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationValue - nodeCopy.Annotations[autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationByMCMKey] = autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationByMCMValue - updatedNode, err := c.targetCoreClient.CoreV1().Nodes().Update(ctx, nodeCopy, metav1.UpdateOptions{}) - if err != nil { - klog.Errorf("error trying to update CA annotation on node %q: %v", node.Name, err) - return nil, err - } - return updatedNode, nil +func (c *controller) addCAScaleDownDisabledAnnotationOnNode(ctx context.Context, nodeName string) error { + firstTry := true + return clientretry.RetryOnConflict(nodeops.Backoff, func() error { + var node *corev1.Node + var err error + // First we try getting the node from the API server cache, as it's cheaper. If it fails + // we get it from etcd to be sure to have fresh data. + if firstTry { + node, err = c.targetCoreClient.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{ResourceVersion: "0"}) + firstTry = false + } else { + node, err = c.targetCoreClient.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) + } + if err != nil { + return err + } + // Check if annotation already exists with correct value + if node.Annotations[autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationKey] == autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationValue { + return nil + } + // Add annotation to disable CA scale down. + // Also add annotation expressing that MCM is the one who added this annotation, so that it can be removed safely when preservation is stopped. + nodeCopy := node.DeepCopy() + if nodeCopy.Annotations == nil { + nodeCopy.Annotations = make(map[string]string) + } + nodeCopy.Annotations[autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationKey] = autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationValue + nodeCopy.Annotations[autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationByMCMKey] = autoscaler.ClusterAutoscalerScaleDownDisabledAnnotationByMCMValue + if _, err := c.targetCoreClient.CoreV1().Nodes().Update(ctx, nodeCopy, metav1.UpdateOptions{}); err != nil { + klog.Errorf("error trying to update CA annotation on node %q: %v", nodeName, err) + return err + } + return nil + }) }