Ignore new Machines when calculating MachinesUpToDate condition

sbueringer · sbueringer · commit df85ff62b078 · 2024-11-18T13:32:06.000+01:00
Signed-off-by: Stefan Büringer buringerst@vmware.com
diff --git a/controlplane/kubeadm/internal/controllers/status.go b/controlplane/kubeadm/internal/controllers/status.go
@@ -358,6 +358,13 @@ func setMachinesReadyCondition(ctx context.Context, kcp *controlplanev1.KubeadmC
 }
 
 func setMachinesUpToDateCondition(ctx context.Context, kcp *controlplanev1.KubeadmControlPlane, machines collections.Machines) {
+	// Only consider Machines that have an UpToDate condition or are older than 10s.
+	// This is done to ensure the MachinesUpToDate condition doesn't flicker after a new Machine is created,
+	// because it can take a bit until the UpToDate condition is set on a new Machine.
+	machines = machines.Filter(func(machine *clusterv1.Machine) bool {
+		return v1beta2conditions.Has(machine, clusterv1.MachineUpToDateV1Beta2Condition) || time.Since(machine.CreationTimestamp.Time) > 10*time.Second
+	})
+
 	if len(machines) == 0 {
 		v1beta2conditions.Set(kcp, metav1.Condition{
 			Type:   controlplanev1.KubeadmControlPlaneMachinesUpToDateV1Beta2Condition,
diff --git a/controlplane/kubeadm/internal/controllers/status_test.go b/controlplane/kubeadm/internal/controllers/status_test.go
@@ -476,19 +476,22 @@ func Test_setMachinesReadyAndMachinesUpToDateConditions(t *testing.T) {
 					&clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: "m1"}, Status: clusterv1.MachineStatus{V1Beta2: &clusterv1.MachineV1Beta2Status{Conditions: []metav1.Condition{readyTrue, upToDateTrue}}}},
 					&clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: "m2"}, Status: clusterv1.MachineStatus{V1Beta2: &clusterv1.MachineV1Beta2Status{Conditions: []metav1.Condition{readyTrue, upToDateFalse}}}},
 					&clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: "m3"}, Status: clusterv1.MachineStatus{V1Beta2: &clusterv1.MachineV1Beta2Status{Conditions: []metav1.Condition{readyFalse, upToDateFalse}}}},
+					&clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: "m4"}, Status: clusterv1.MachineStatus{V1Beta2: &clusterv1.MachineV1Beta2Status{Conditions: []metav1.Condition{readyFalse}}}},                                                                         // Machine without UpToDate condition
+					&clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: "m5", CreationTimestamp: metav1.Time{Time: time.Now().Add(-5 * time.Second)}}, Status: clusterv1.MachineStatus{V1Beta2: &clusterv1.MachineV1Beta2Status{Conditions: []metav1.Condition{readyFalse}}}}, // New Machine without UpToDate condition (should be ignored)
 				),
 			},
 			expectMachinesReadyCondition: metav1.Condition{
 				Type:    controlplanev1.KubeadmControlPlaneMachinesReadyV1Beta2Condition,
 				Status:  metav1.ConditionFalse,
 				Reason:  controlplanev1.KubeadmControlPlaneMachinesNotReadyV1Beta2Reason,
-				Message: "* Machine m3: NotReady",
+				Message: "* Machines m3, m4, m5: NotReady",
 			},
 			expectMachinesUpToDateCondition: metav1.Condition{
-				Type:    controlplanev1.KubeadmControlPlaneMachinesUpToDateV1Beta2Condition,
-				Status:  metav1.ConditionFalse,
-				Reason:  controlplanev1.KubeadmControlPlaneMachinesNotUpToDateV1Beta2Reason,
-				Message: "* Machines m2, m3: NotUpToDate",
+				Type:   controlplanev1.KubeadmControlPlaneMachinesUpToDateV1Beta2Condition,
+				Status: metav1.ConditionFalse,
+				Reason: controlplanev1.KubeadmControlPlaneMachinesNotUpToDateV1Beta2Reason,
+				Message: "* Machines m2, m3: NotUpToDate\n" +
+					"* Machine m4: Condition UpToDate not yet reported",
 			},
 		},
 	}
diff --git a/internal/controllers/cluster/cluster_controller_status.go b/internal/controllers/cluster/cluster_controller_status.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"sort"
+	"time"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -654,6 +655,7 @@ func setMachinesReadyCondition(ctx context.Context, cluster *clusterv1.Cluster,
 
 func setMachinesUpToDateCondition(ctx context.Context, cluster *clusterv1.Cluster, machines collections.Machines, getDescendantsSucceeded bool) {
 	log := ctrl.LoggerFrom(ctx)
+
 	// If we got unexpected errors in listing the machines (this should never happen), surface them.
 	if !getDescendantsSucceeded {
 		v1beta2conditions.Set(cluster, metav1.Condition{
@@ -665,6 +667,13 @@ func setMachinesUpToDateCondition(ctx context.Context, cluster *clusterv1.Cluste
 		return
 	}
 
+	// Only consider Machines that have an UpToDate condition or are older than 10s.
+	// This is done to ensure the MachinesUpToDate condition doesn't flicker after a new Machine is created,
+	// because it can take a bit until the UpToDate condition is set on a new Machine.
+	machines = machines.Filter(func(machine *clusterv1.Machine) bool {
+		return v1beta2conditions.Has(machine, clusterv1.MachineUpToDateV1Beta2Condition) || time.Since(machine.CreationTimestamp.Time) > 10*time.Second
+	})
+
 	if len(machines) == 0 {
 		v1beta2conditions.Set(cluster, metav1.Condition{
 			Type:   clusterv1.ClusterMachinesUpToDateV1Beta2Condition,
diff --git a/internal/controllers/cluster/cluster_controller_status_test.go b/internal/controllers/cluster/cluster_controller_status_test.go
@@ -1005,10 +1005,11 @@ func TestSetMachinesUpToDateCondition(t *testing.T) {
 			},
 		},
 		{
-			name:    "One machine without up-to-date condition",
+			name:    "One machine without up-to-date condition, one new Machines without up-to-date condition",
 			cluster: fakeCluster("c"),
 			machines: []*clusterv1.Machine{
 				fakeMachine("no-condition-machine-1"),
+				fakeMachine("no-condition-machine-2-new", creationTimestamp{Time: time.Now().Add(-5 * time.Second)}), // ignored because it's new
 			},
 			getDescendantsSucceeded: true,
 			expectCondition: metav1.Condition{
@@ -2650,6 +2651,12 @@ func (r initialized) ApplyToControlPlane(cp *unstructured.Unstructured) {
 	_ = contract.ControlPlane().Initialized().Set(cp, bool(r))
 }
 
+type creationTimestamp metav1.Time
+
+func (t creationTimestamp) ApplyToMachine(m *clusterv1.Machine) {
+	m.CreationTimestamp = metav1.Time(t)
+}
+
 type nodeRef corev1.ObjectReference
 
 func (r nodeRef) ApplyToMachine(m *clusterv1.Machine) {
diff --git a/internal/controllers/machinedeployment/machinedeployment_status.go b/internal/controllers/machinedeployment/machinedeployment_status.go
@@ -320,6 +320,13 @@ func setMachinesUpToDateCondition(ctx context.Context, machineDeployment *cluste
 		return
 	}
 
+	// Only consider Machines that have an UpToDate condition or are older than 10s.
+	// This is done to ensure the MachinesUpToDate condition doesn't flicker after a new Machine is created,
+	// because it can take a bit until the UpToDate condition is set on a new Machine.
+	machines = machines.Filter(func(machine *clusterv1.Machine) bool {
+		return v1beta2conditions.Has(machine, clusterv1.MachineUpToDateV1Beta2Condition) || time.Since(machine.CreationTimestamp.Time) > 10*time.Second
+	})
+
 	if len(machines) == 0 {
 		v1beta2conditions.Set(machineDeployment, metav1.Condition{
 			Type:   clusterv1.MachineDeploymentMachinesUpToDateV1Beta2Condition,
diff --git a/internal/controllers/machinedeployment/machinedeployment_status_test.go b/internal/controllers/machinedeployment/machinedeployment_status_test.go
@@ -814,10 +814,11 @@ func Test_setMachinesUpToDateCondition(t *testing.T) {
 			},
 		},
 		{
-			name:              "One machine without up-to-date condition",
+			name:              "One machine without up-to-date condition, one new Machines without up-to-date condition",
 			machineDeployment: &clusterv1.MachineDeployment{},
 			machines: []*clusterv1.Machine{
 				fakeMachine("no-condition-machine-1"),
+				fakeMachine("no-condition-machine-2-new", withCreationTimestamp(time.Now().Add(-5*time.Second))), // ignored because it's new
 			},
 			getMachinesSucceeded: true,
 			expectCondition: metav1.Condition{
@@ -1172,6 +1173,12 @@ func fakeMachine(name string, options ...fakeMachinesOption) *clusterv1.Machine
 	return p
 }
 
+func withCreationTimestamp(t time.Time) fakeMachinesOption {
+	return func(m *clusterv1.Machine) {
+		m.CreationTimestamp = metav1.Time{Time: t}
+	}
+}
+
 func withStaleDeletion() fakeMachinesOption {
 	return func(m *clusterv1.Machine) {
 		m.DeletionTimestamp = ptr.To(metav1.Time{Time: time.Now().Add(-1 * time.Hour)})
diff --git a/internal/controllers/machineset/machineset_controller_status.go b/internal/controllers/machineset/machineset_controller_status.go
@@ -256,7 +256,7 @@ func setMachinesReadyCondition(ctx context.Context, machineSet *clusterv1.Machin
 	v1beta2conditions.Set(machineSet, *readyCondition)
 }
 
-func setMachinesUpToDateCondition(ctx context.Context, machineSet *clusterv1.MachineSet, machines []*clusterv1.Machine, getAndAdoptMachinesForMachineSetSucceeded bool) {
+func setMachinesUpToDateCondition(ctx context.Context, machineSet *clusterv1.MachineSet, machinesSlice []*clusterv1.Machine, getAndAdoptMachinesForMachineSetSucceeded bool) {
 	log := ctrl.LoggerFrom(ctx)
 	// If we got unexpected errors in listing the machines (this should never happen), surface them.
 	if !getAndAdoptMachinesForMachineSetSucceeded {
@@ -269,6 +269,13 @@ func setMachinesUpToDateCondition(ctx context.Context, machineSet *clusterv1.Mac
 		return
 	}
 
+	// Only consider Machines that have an UpToDate condition or are older than 10s.
+	// This is done to ensure the MachinesUpToDate condition doesn't flicker after a new Machine is created,
+	// because it can take a bit until the UpToDate condition is set on a new Machine.
+	machines := collections.FromMachines(machinesSlice...).Filter(func(machine *clusterv1.Machine) bool {
+		return v1beta2conditions.Has(machine, clusterv1.MachineUpToDateV1Beta2Condition) || time.Since(machine.CreationTimestamp.Time) > 10*time.Second
+	})
+
 	if len(machines) == 0 {
 		v1beta2conditions.Set(machineSet, metav1.Condition{
 			Type:   clusterv1.MachineSetMachinesUpToDateV1Beta2Condition,
@@ -279,7 +286,7 @@ func setMachinesUpToDateCondition(ctx context.Context, machineSet *clusterv1.Mac
 	}
 
 	upToDateCondition, err := v1beta2conditions.NewAggregateCondition(
-		machines, clusterv1.MachineUpToDateV1Beta2Condition,
+		machines.UnsortedList(), clusterv1.MachineUpToDateV1Beta2Condition,
 		v1beta2conditions.TargetConditionType(clusterv1.MachineSetMachinesUpToDateV1Beta2Condition),
 		// Using a custom merge strategy to override reasons applied during merge.
 		v1beta2conditions.CustomMergeStrategy{
diff --git a/internal/controllers/machineset/machineset_controller_status_test.go b/internal/controllers/machineset/machineset_controller_status_test.go
@@ -727,10 +727,11 @@ func Test_setMachinesUpToDateCondition(t *testing.T) {
 			},
 		},
 		{
-			name:       "One machine without up-to-date condition",
+			name:       "One machine without up-to-date condition, one new Machines without up-to-date condition",
 			machineSet: machineSet,
 			machines: []*clusterv1.Machine{
 				newMachine("no-condition-machine-1"),
+				fakeMachine("no-condition-machine-2-new", withCreationTimestamp(time.Now().Add(-5*time.Second))), // ignored because it's new
 			},
 			getAndAdoptMachinesForMachineSetSucceeded: true,
 			expectCondition: metav1.Condition{
@@ -1028,6 +1029,12 @@ func fakeMachine(name string, options ...fakeMachinesOption) *clusterv1.Machine
 	return p
 }
 
+func withCreationTimestamp(t time.Time) fakeMachinesOption {
+	return func(m *clusterv1.Machine) {
+		m.CreationTimestamp = metav1.Time{Time: t}
+	}
+}
+
 func withV1Beta2Condition(c metav1.Condition) fakeMachinesOption {
 	return func(m *clusterv1.Machine) {
 		if m.Status.V1Beta2 == nil {
