Track preserved failed machines in MCS and MCD status

docs/documents/apis.md

@@ -1566,6 +1566,20 @@ int32
 </tr>
 <tr>
 <td>
+<code>preservedFailedReplicas</code>
+</td>
+<td>
+<em>
+int32
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>PreservedFailedReplicas is the number of preserved machines in Failed phase targeted by this MachineDeployment</p>
+</td>
+</tr>
+<tr>
+<td>
 <code>unavailableReplicas</code>
 </td>
 <td>
@@ -1577,7 +1591,8 @@ int32
 <em>(Optional)</em>
 <p>Total number of unavailable machines targeted by this MachineDeployment. This is the total number of
 machines that are still required for the MachineDeployment to have 100% available capacity. They may
-either be machines that are running but not yet available or machines that still have not been created.</p>
+either be machines that are running but not yet available, machines that still have not been created, or
+machines that are preserved in Failed phase.</p>
 </td>
 </tr>
 <tr>
@@ -2026,6 +2041,19 @@ int32
 </tr>
 <tr>
 <td>
+<code>preservedFailedReplicas</code>
+</td>
+<td>
+<em>
+int32
+</em>
+</td>
+<td>
+<p>PreservedFailedReplicas is the number of preserved replicas in Failed phase for this replica set</p>
+</td>
+</tr>
+<tr>
+<td>
 <code>observedGeneration</code>
 </td>
 <td>

docs/usage/machine-preservation.md

@@ -81,6 +81,9 @@ spec:
 > If `AutoPreserveFailedMachineMax` is decreased, preservation is stopped for older auto-preserved machines(earlier creationTimestamp) until the number of preserved machines is within the new limit.
 > If the number of failed machines exceeds the `AutoPreserveFailedMachineMax` limit at any given time, machines with more recent creationTimestamp are auto-preserved first.
 
+> Note: ⚠️Preserved failed nodes still count toward the total number of nodes unable to renew their leases. If this total — across both preserved and non-preserved nodes — exceeds the threshold configured (defaults to 60%) for
+> dependency-watchdog, then the components configured as dependent resources in dependency-watchdog (by default: kube-controller-manager, machine-controller-manager, and cluster-autoscaler),
+> will be scaled-down. Since a higher AutoPreserveFailedMachineMax value increases the number of preserved failed nodes, it also raises the risk of breaching this threshold. Please exercise caution when setting this value.
 ### Preservation annotations
 
 annotation key: `node.machine.sapcloud.io/preserve`
@@ -148,6 +151,8 @@ In all the cases, when the machine moves to `Running` during preservation, the b
 - Scale-down preference: Preserved machines are the last to be scaled down.
 - Preservation status is visible via Node Conditions and Machine Status fields.
 - machinePreserveTimeout changes do not affect existing preserved machines. Operators may edit PreserveExpiryTime directly if required to extend preservation.
+- If a machine or its backing node is annotated for preservation when the machine is in the Failed state, MCM may not be able to act on it before the machine is Terminated. Therefore, it is recommended to annotate the machine/node for preservation when the machine enters the Unknown phase, or as soon as the node is observed to be NotReady, to increase the chances of preservation taking effect before termination.
+- Similarly, if an underutilized and unneeded machine is annotated for preservation, but CA initiates scale-down before MCM can add the CA scale-down-disabled annotation as a part of its preservation logic, the machine may be scaled down before preservation takes effect. 
 
 
-> NOTE: To prevent confusion and unintended behaviour, it is recommended to use preservation by annotating the node object if it exists and can be accessed.
+> NOTE: To prevent confusion and unintended behaviour, it is recommended to use preservation by annotating the node object, if it exists and can be accessed, rather than the machine object.

kubernetes/crds/machine.sapcloud.io_machineclasses.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.16.1
+    controller-gen.kubebuilder.io/version: v0.17.3
   name: machineclasses.machine.sapcloud.io
 spec:
   group: machine.sapcloud.io

kubernetes/crds/machine.sapcloud.io_machinedeployments.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.16.1
+    controller-gen.kubebuilder.io/version: v0.17.3
   name: machinedeployments.machine.sapcloud.io
 spec:
   group: machine.sapcloud.io
@@ -530,6 +530,11 @@ spec:
                 description: The generation observed by the MachineDeployment controller.
                 format: int64
                 type: integer
+              preservedFailedReplicas:
+                description: PreservedFailedReplicas is the number of preserved machines
+                  in Failed phase targeted by this MachineDeployment
+                format: int32
+                type: integer
               readyReplicas:
                 description: Total number of ready machines targeted by this MachineDeployment.
                 format: int32
@@ -543,7 +548,8 @@ spec:
                 description: |-
                   Total number of unavailable machines targeted by this MachineDeployment. This is the total number of
                   machines that are still required for the MachineDeployment to have 100% available capacity. They may
-                  either be machines that are running but not yet available or machines that still have not been created.
+                  either be machines that are running but not yet available, machines that still have not been created, or
+                  machines that are preserved in Failed phase.
                 format: int32
                 type: integer
               updatedReplicas:

kubernetes/crds/machine.sapcloud.io_machines.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.16.1
+    controller-gen.kubebuilder.io/version: v0.17.3
   name: machines.machine.sapcloud.io
 spec:
   group: machine.sapcloud.io

kubernetes/crds/machine.sapcloud.io_machinesets.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.16.1
+    controller-gen.kubebuilder.io/version: v0.17.3
   name: machinesets.machine.sapcloud.io
 spec:
   group: machine.sapcloud.io
@@ -428,6 +428,11 @@ spec:
                   by the controller.
                 format: int64
                 type: integer
+              preservedFailedReplicas:
+                description: PreservedFailedReplicas is the number of preserved replicas
+                  in Failed phase for this replica set
+                format: int32
+                type: integer
               readyReplicas:
                 description: The number of ready replicas for this replica set.
                 format: int32

pkg/apis/machine/types.go

@@ -405,6 +405,9 @@ type MachineSetStatus struct {
 	// The number of available replicas (ready for at least minReadySeconds) for this replica set.
 	AvailableReplicas int32
 
+	// PreservedFailedReplicas is the number of preserved replicas in Failed phase for this replica set
+	PreservedFailedReplicas int32
+
 	// ObservedGeneration is the most recent generation observed by the controller.
 	ObservedGeneration int64
 
@@ -633,9 +636,13 @@ type MachineDeploymentStatus struct {
 	// Total number of available machines (ready for at least minReadySeconds) targeted by this MachineDeployment.
 	AvailableReplicas int32
 
+	// PreservedFailedReplicas is the number of preserved machines in Failed phase targeted by this MachineDeployment
+	PreservedFailedReplicas int32
+
 	// Total number of unavailable machines targeted by this MachineDeployment. This is the total number of
 	// machines that are still required for the MachineDeployment to have 100% available capacity. They may
-	// either be machines that are running but not yet available or machines that still have not been created.
+	// either be machines that are running but not yet available, machines that still have not been created, or
+	// machines that are preserved in Failed phase.
 	UnavailableReplicas int32
 
 	// Represents the latest available observations of a MachineDeployment's current state.

pkg/apis/machine/v1alpha1/machinedeployment_types.go

@@ -213,9 +213,14 @@ type MachineDeploymentStatus struct {
 	// +optional
 	AvailableReplicas int32 `json:"availableReplicas,omitempty"`
 
+	// PreservedFailedReplicas is the number of preserved machines in Failed phase targeted by this MachineDeployment
+	// +optional
+	PreservedFailedReplicas int32 `json:"preservedFailedReplicas,omitempty"`
+
 	// Total number of unavailable machines targeted by this MachineDeployment. This is the total number of
 	// machines that are still required for the MachineDeployment to have 100% available capacity. They may
-	// either be machines that are running but not yet available or machines that still have not been created.
+	// either be machines that are running but not yet available, machines that still have not been created, or
+	// machines that are preserved in Failed phase.
 	// +optional
 	UnavailableReplicas int32 `json:"unavailableReplicas,omitempty"`
 

pkg/apis/machine/v1alpha1/machineset_types.go

@@ -124,6 +124,9 @@ type MachineSetStatus struct {
 	// +optional
 	AvailableReplicas int32 `json:"availableReplicas,omitempty"`
 
+	// PreservedFailedReplicas is the number of preserved replicas in Failed phase for this replica set
+	PreservedFailedReplicas int32 `json:"preservedFailedReplicas,omitempty"`
+
 	// ObservedGeneration is the most recent generation observed by the controller.
 	// +optional
 	ObservedGeneration int64 `json:"observedGeneration,omitempty"`

pkg/controller/controller_utils.go

@@ -745,6 +745,24 @@ func (s ActiveMachines) Less(i, j int) bool {
 	if isPreservedI != isPreservedJ {
 		return !isPreservedI // if s[i] preserved, it is "greater" and should not be deleted first, therefore, "less" is false, and vice versa
 	}
+	if isPreservedI { // both machines are preserved, in which case deprioritize explicitly preserved machines over auto-preserved
+		isAutoPreserved := func(m *v1alpha1.Machine) bool {
+			return m.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValuePreservedByMCM
+		}
+		isAutoPreservedI := isAutoPreserved(s[i])
+		isAutoPreservedJ := isAutoPreserved(s[j])
+		if isAutoPreservedI != isAutoPreservedJ {
+			return isAutoPreservedI // if s[i] is auto-preserved, it is "less", and should be deleted first
+		}
+		// if both are auto-preserved or both are explicitly preserved, we decide based on whichever machine
+		// has an earlier preserve expiry time
+		diff := s[j].Status.CurrentStatus.PreserveExpiryTime.Time.Sub(s[i].Status.CurrentStatus.PreserveExpiryTime.Time)
+		if diff > 0 {
+			return true // if s[i] expires earlier, it is "less" and should be deleted first
+		} else if diff < 0 {
+			return false
+		}
+	}
 
 	// Default priority for machine objects
 	machineIPriority := 3

pkg/controller/deployment_machineset_util.go

@@ -25,9 +25,10 @@ package controller
 import (
 	"context"
 	"fmt"
-	"github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils"
 	"reflect"
 
+	"github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils"
+
 	"k8s.io/klog/v2"
 
 	"github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1"
@@ -50,7 +51,8 @@ func updateMachineSetStatus(ctx context.Context, machineClient machineapi.Machin
 		is.Generation == is.Status.ObservedGeneration &&
 		reflect.DeepEqual(is.Status.Conditions, newStatus.Conditions) &&
 		reflect.DeepEqual(is.Status.FailedMachines, newStatus.FailedMachines) &&
-		is.Status.AutoPreserveFailedMachineCount == newStatus.AutoPreserveFailedMachineCount {
+		is.Status.AutoPreserveFailedMachineCount == newStatus.AutoPreserveFailedMachineCount &&
+		is.Status.PreservedFailedReplicas == newStatus.PreservedFailedReplicas {
 		return is, nil
 	}
 
@@ -68,8 +70,9 @@ func updateMachineSetStatus(ctx context.Context, machineClient machineapi.Machin
 			fmt.Sprintf("fullyLabeledReplicas %d->%d, ", is.Status.FullyLabeledReplicas, newStatus.FullyLabeledReplicas)+
 			fmt.Sprintf("readyReplicas %d->%d, ", is.Status.ReadyReplicas, newStatus.ReadyReplicas)+
 			fmt.Sprintf("availableReplicas %d->%d, ", is.Status.AvailableReplicas, newStatus.AvailableReplicas)+
-			fmt.Sprintf("sequence No: %v->%v,", is.Status.ObservedGeneration, newStatus.ObservedGeneration)+
-			fmt.Sprintf("autoPreserveFailedMachineCount %v->%v", is.Status.AutoPreserveFailedMachineCount, newStatus.AutoPreserveFailedMachineCount))
+			fmt.Sprintf("observedGeneration: %v->%v, ", is.Status.ObservedGeneration, newStatus.ObservedGeneration)+
+			fmt.Sprintf("autoPreserveFailedMachineCount %v->%v, ", is.Status.AutoPreserveFailedMachineCount, newStatus.AutoPreserveFailedMachineCount)+
+			fmt.Sprintf("preservedFailedReplicas %v->%v", is.Status.PreservedFailedReplicas, newStatus.PreservedFailedReplicas))
 
 		is.Status = newStatus
 		updatedIS, updateErr = c.UpdateStatus(ctx, is, metav1.UpdateOptions{})
@@ -102,6 +105,7 @@ func calculateMachineSetStatus(is *v1alpha1.MachineSet, filteredMachines []*v1al
 	fullyLabeledReplicasCount := 0
 	readyReplicasCount := 0
 	availableReplicasCount := 0
+	preservedFailedReplicasCount := 0
 	autoPreserveFailedMachineCount := 0
 
 	failedMachines := []v1alpha1.MachineSummary{}
@@ -118,7 +122,11 @@ func calculateMachineSetStatus(is *v1alpha1.MachineSet, filteredMachines []*v1al
 				readyReplicasCount++
 			}
 		}
-		if machine.Status.LastOperation.State == v1alpha1.MachineStateFailed {
+
+		// exclude failed preserved machines from failedMachines to prevent MCD from being marked as unavailable
+		if isMachinePreservedAndFailed(machine) {
+			preservedFailedReplicasCount++
+		} else if machine.Status.LastOperation.State == v1alpha1.MachineStateFailed {
 			machineSummary.Name = machine.Name
 			machineSummary.ProviderID = machine.Spec.ProviderID
 			machineSummary.LastOperation = machine.Status.LastOperation
@@ -129,6 +137,7 @@ func calculateMachineSetStatus(is *v1alpha1.MachineSet, filteredMachines []*v1al
 			failedMachines = append(failedMachines, machineSummary)
 		}
 		// Count number of failed machines annotated with PreserveMachineAnnotationValuePreservedByMCM
+		// Cannot combine with above if block in case auto-preservation is not complete yet
 		if machine.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValuePreservedByMCM {
 			autoPreserveFailedMachineCount++
 		}
@@ -162,6 +171,7 @@ func calculateMachineSetStatus(is *v1alpha1.MachineSet, filteredMachines []*v1al
 	newStatus.AvailableReplicas = int32(availableReplicasCount)       // #nosec  G115 (CWE-190) -- number of machines will not exceed MaxInt32
 	newStatus.LastOperation.LastUpdateTime = metav1.Now()
 	newStatus.AutoPreserveFailedMachineCount = int32(autoPreserveFailedMachineCount) // #nosec  G115 (CWE-190) -- number of machines will not exceed MaxInt32
+	newStatus.PreservedFailedReplicas = int32(preservedFailedReplicasCount)          // #nosec  G115 (CWE-190) -- number of machines will not exceed MaxInt32
 	return newStatus
 }
 
@@ -224,6 +234,10 @@ func isMachineAvailable(machine *v1alpha1.Machine) bool {
 	return false
 }
 
+func isMachinePreservedAndFailed(machine *v1alpha1.Machine) bool {
+	return machine.Status.CurrentStatus.Phase == v1alpha1.MachineFailed && !machine.Status.CurrentStatus.PreserveExpiryTime.IsZero()
+}
+
 func isMachineReady(machine *v1alpha1.Machine) bool {
 	// TODO add more conditions
 	return machine.Status.CurrentStatus.Phase == v1alpha1.MachineRunning

pkg/controller/deployment_sync.go

@@ -625,19 +625,21 @@ func (dc *controller) syncMachineDeploymentStatus(ctx context.Context, allISs []
 func calculateDeploymentStatus(allISs []*v1alpha1.MachineSet, newIS *v1alpha1.MachineSet, deployment *v1alpha1.MachineDeployment) v1alpha1.MachineDeploymentStatus {
 	availableReplicas := GetAvailableReplicaCountForMachineSets(allISs)
 	totalReplicas := GetReplicaCountForMachineSets(allISs)
+	totalPreservedFailedReplicas := GetPreservedFailedReplicaCountForMachineSets(allISs)
 	// If unavailableReplicas is negative, then that means the Deployment has more available replicas running than
 	// desired, e.g. whenever it scales down. In such a case we should simply default unavailableReplicas to zero.
 	unavailableReplicas := max(totalReplicas-availableReplicas, 0)
 
 	status := v1alpha1.MachineDeploymentStatus{
 		// TODO: Ensure that if we start retrying status updates, we won't pick up a new Generation value.
-		ObservedGeneration:  deployment.Generation,
-		Replicas:            GetActualReplicaCountForMachineSets(allISs),
-		UpdatedReplicas:     GetActualReplicaCountForMachineSets([]*v1alpha1.MachineSet{newIS}),
-		ReadyReplicas:       GetReadyReplicaCountForMachineSets(allISs),
-		AvailableReplicas:   availableReplicas,
-		UnavailableReplicas: unavailableReplicas,
-		CollisionCount:      deployment.Status.CollisionCount,
+		ObservedGeneration:      deployment.Generation,
+		Replicas:                GetActualReplicaCountForMachineSets(allISs),
+		UpdatedReplicas:         GetActualReplicaCountForMachineSets([]*v1alpha1.MachineSet{newIS}),
+		ReadyReplicas:           GetReadyReplicaCountForMachineSets(allISs),
+		AvailableReplicas:       availableReplicas,
+		PreservedFailedReplicas: totalPreservedFailedReplicas,
+		UnavailableReplicas:     unavailableReplicas,
+		CollisionCount:          deployment.Status.CollisionCount,
 	}
 	status.FailedMachines = []*v1alpha1.MachineSummary{}
 
@@ -654,7 +656,8 @@ func calculateDeploymentStatus(allISs []*v1alpha1.MachineSet, newIS *v1alpha1.Ma
 	// Copy conditions one by one so we won't mutate the original object.
 	status.Conditions = append(status.Conditions, deployment.Status.Conditions...)
 
-	if availableReplicas >= (deployment.Spec.Replicas)-MaxUnavailable(*deployment) {
+	minRequired := max((deployment.Spec.Replicas)-MaxUnavailable(*deployment)-totalPreservedFailedReplicas, 0)
+	if availableReplicas >= minRequired {
 		minAvailability := NewMachineDeploymentCondition(v1alpha1.MachineDeploymentAvailable, v1alpha1.ConditionTrue, MinimumReplicasAvailable, "Deployment has minimum availability.")
 		SetMachineDeploymentCondition(&status, *minAvailability)
 	} else {