Only update machine condition LastTransitionTime when the status changes

Signed-off-by: Lukas Frank <lukas.frank@sap.com>

Author: lukasfrank

poollet/machinepoollet/controllers/machine_controller.go

@@ -15,6 +15,7 @@ import (
 	"google.golang.org/grpc/status"
 
 	"github.com/ironcore-dev/controller-utils/clientutils"
+	"github.com/ironcore-dev/controller-utils/conditionutils"
 	commonv1alpha1 "github.com/ironcore-dev/ironcore/api/common/v1alpha1"
 	computev1alpha1 "github.com/ironcore-dev/ironcore/api/compute/v1alpha1"
 	ipamv1alpha1 "github.com/ironcore-dev/ironcore/api/ipam/v1alpha1"
@@ -502,43 +503,42 @@ func (r *MachineReconciler) updateMachineStatus(ctx context.Context, machine *co
 	machine.Status.ObservedGeneration = generation
 	machine.Status.Volumes = volumeStatuses
 	machine.Status.NetworkInterfaces = nicStatuses
-	machine.Status.Conditions = r.computeMachineConditions(state, volumeStatuses, nicStatuses, now)
+	if err := ComputeMachineConditions(&machine.Status.Conditions, state, volumeStatuses, nicStatuses); err != nil {
+		return fmt.Errorf("error computing machine conditions: %w", err)
+	}
 
 	if err := r.Status().Patch(ctx, machine, client.MergeFrom(base)); err != nil {
 		return fmt.Errorf("error patching status: %w", err)
 	}
 	return nil
 }
 
-// computeMachineConditions computes the conditions for the machine based on its current state.
-func (r *MachineReconciler) computeMachineConditions(
+func ComputeMachineConditions(
+	conditions *[]computev1alpha1.MachineCondition,
 	state computev1alpha1.MachineState,
 	volumeStatuses []computev1alpha1.VolumeStatus,
 	nicStatuses []computev1alpha1.NetworkInterfaceStatus,
-	now metav1.Time,
-) []computev1alpha1.MachineCondition {
-	var conditions []computev1alpha1.MachineCondition
-
-	conditions = append(conditions, r.computeMachineReadyCondition(state, now))
-
+) error {
+	desired := []computev1alpha1.MachineCondition{computeMachineReadyCondition(state)}
 	if len(volumeStatuses) > 0 {
-		if c := r.computeVolumesReadyCondition(volumeStatuses, now); c.Type != "" {
-			conditions = append(conditions, c)
-		}
+		desired = append(desired, computeVolumesReadyCondition(volumeStatuses))
 	}
-
 	if len(nicStatuses) > 0 {
-		if c := r.computeNetworkInterfacesReadyCondition(nicStatuses, now); c.Type != "" {
-			conditions = append(conditions, c)
-		}
+		desired = append(desired, computeNetworkInterfacesReadyCondition(nicStatuses))
 	}
 
-	return conditions
+	for _, cond := range desired {
+		if err := conditionutils.UpdateSlice(conditions, string(cond.Type),
+			conditionutils.UpdateFromCondition{Condition: cond},
+		); err != nil {
+			return fmt.Errorf("error updating %s condition: %w", cond.Type, err)
+		}
+	}
+	return nil
 }
 
-func (r *MachineReconciler) computeMachineReadyCondition(state computev1alpha1.MachineState, now metav1.Time) computev1alpha1.MachineCondition {
+func computeMachineReadyCondition(state computev1alpha1.MachineState) computev1alpha1.MachineCondition {
 	status, reason, message := corev1.ConditionFalse, "NotReady", "Machine is not ready"
-
 	switch state {
 	case computev1alpha1.MachineStateRunning:
 		status, reason, message = corev1.ConditionTrue, "Running", "Machine is running"
@@ -547,23 +547,11 @@ func (r *MachineReconciler) computeMachineReadyCondition(state computev1alpha1.M
 	case computev1alpha1.MachineStateTerminating, computev1alpha1.MachineStateTerminated:
 		status, reason, message = corev1.ConditionFalse, "Terminating", "Machine is terminating or terminated"
 	}
-
-	return computev1alpha1.MachineCondition{
-		Type:               "Ready",
-		Status:             status,
-		Reason:             reason,
-		Message:            message,
-		LastTransitionTime: now,
-	}
+	return computev1alpha1.MachineCondition{Type: "Ready", Status: status, Reason: reason, Message: message}
 }
 
-func (r *MachineReconciler) computeVolumesReadyCondition(volumeStatuses []computev1alpha1.VolumeStatus, now metav1.Time) computev1alpha1.MachineCondition {
-	if len(volumeStatuses) == 0 {
-		return computev1alpha1.MachineCondition{}
-	}
-
+func computeVolumesReadyCondition(volumeStatuses []computev1alpha1.VolumeStatus) computev1alpha1.MachineCondition {
 	status, reason, message := corev1.ConditionTrue, "VolumesReady", "All volumes are ready"
-
 	for _, vs := range volumeStatuses {
 		if vs.State != computev1alpha1.VolumeStateAttached {
 			status = corev1.ConditionFalse
@@ -572,23 +560,11 @@ func (r *MachineReconciler) computeVolumesReadyCondition(volumeStatuses []comput
 			break
 		}
 	}
-
-	return computev1alpha1.MachineCondition{
-		Type:               computev1alpha1.MachineConditionType("VolumesReady"),
-		Status:             status,
-		Reason:             reason,
-		Message:            message,
-		LastTransitionTime: now,
-	}
+	return computev1alpha1.MachineCondition{Type: "VolumesReady", Status: status, Reason: reason, Message: message}
 }
 
-func (r *MachineReconciler) computeNetworkInterfacesReadyCondition(nicStatuses []computev1alpha1.NetworkInterfaceStatus, now metav1.Time) computev1alpha1.MachineCondition {
-	if len(nicStatuses) == 0 {
-		return computev1alpha1.MachineCondition{}
-	}
-
+func computeNetworkInterfacesReadyCondition(nicStatuses []computev1alpha1.NetworkInterfaceStatus) computev1alpha1.MachineCondition {
 	status, reason, message := corev1.ConditionTrue, "NetworkInterfacesReady", "All network interfaces are ready"
-
 	for _, nicStatus := range nicStatuses {
 		if nicStatus.State != computev1alpha1.NetworkInterfaceStateAttached {
 			status = corev1.ConditionFalse
@@ -597,14 +573,7 @@ func (r *MachineReconciler) computeNetworkInterfacesReadyCondition(nicStatuses [
 			break
 		}
 	}
-
-	return computev1alpha1.MachineCondition{
-		Type:               computev1alpha1.MachineConditionType("NetworkInterfacesReady"),
-		Status:             status,
-		Reason:             reason,
-		Message:            message,
-		LastTransitionTime: now,
-	}
+	return computev1alpha1.MachineCondition{Type: "NetworkInterfacesReady", Status: status, Reason: reason, Message: message}
 }
 
 func (r *MachineReconciler) prepareIRIPower(power computev1alpha1.Power) (iri.Power, error) {

poollet/machinepoollet/controllers/machine_controller_test.go

@@ -6,6 +6,7 @@ package controllers_test
 import (
 	"encoding/json"
 	"fmt"
+	"time"
 
 	. "github.com/afritzler/protoequal"
 	commonv1alpha1 "github.com/ironcore-dev/ironcore/api/common/v1alpha1"
@@ -18,6 +19,7 @@ import (
 	testingmachine "github.com/ironcore-dev/ironcore/iri/testing/machine"
 	poolletutils "github.com/ironcore-dev/ironcore/poollet/common/utils"
 	machinepoolletv1alpha1 "github.com/ironcore-dev/ironcore/poollet/machinepoollet/api/v1alpha1"
+	"github.com/ironcore-dev/ironcore/poollet/machinepoollet/controllers"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	. "github.com/onsi/gomega/gstruct"
@@ -1147,3 +1149,62 @@ func mustMarshalJSON(v interface{}) string {
 	}
 	return string(data)
 }
+
+var _ = Describe("ComputeMachineConditions", func() {
+	findCondition := func(conditions []computev1alpha1.MachineCondition, typ computev1alpha1.MachineConditionType) *computev1alpha1.MachineCondition {
+		for i := range conditions {
+			if conditions[i].Type == typ {
+				return &conditions[i]
+			}
+		}
+		return nil
+	}
+
+	It("preserves a condition's LastTransitionTime when its status does not change", func() {
+		old := metav1.NewTime(time.Now().Add(-time.Hour))
+		conditions := []computev1alpha1.MachineCondition{
+			{Type: "Ready", Status: corev1.ConditionTrue, Reason: "Running", Message: "Machine is running", LastTransitionTime: old},
+		}
+
+		Expect(controllers.ComputeMachineConditions(&conditions, computev1alpha1.MachineStateRunning, nil, nil)).To(Succeed())
+
+		ready := findCondition(conditions, "Ready")
+		Expect(ready).NotTo(BeNil())
+		Expect(ready.Status).To(Equal(corev1.ConditionTrue))
+		Expect(ready.LastTransitionTime.Time).To(BeTemporally("==", old.Time))
+	})
+
+	It("bumps a condition's LastTransitionTime when its status changes", func() {
+		old := metav1.NewTime(time.Now().Add(-time.Hour))
+		conditions := []computev1alpha1.MachineCondition{
+			{Type: "Ready", Status: corev1.ConditionTrue, Reason: "Running", Message: "Machine is running", LastTransitionTime: old},
+		}
+
+		Expect(controllers.ComputeMachineConditions(&conditions, computev1alpha1.MachineStateTerminating, nil, nil)).To(Succeed())
+
+		ready := findCondition(conditions, "Ready")
+		Expect(ready).NotTo(BeNil())
+		Expect(ready.Status).To(Equal(corev1.ConditionFalse))
+		Expect(ready.Reason).To(Equal("Terminating"))
+		Expect(ready.LastTransitionTime.Time).To(BeTemporally(">", old.Time))
+	})
+
+	It("adds volume and network interface conditions and preserves their timestamps on an unchanged recompute", func() {
+		var conditions []computev1alpha1.MachineCondition
+		volumeStatuses := []computev1alpha1.VolumeStatus{{Name: "root", State: computev1alpha1.VolumeStateAttached}}
+		nicStatuses := []computev1alpha1.NetworkInterfaceStatus{{Name: "nic", State: computev1alpha1.NetworkInterfaceStateAttached}}
+
+		Expect(controllers.ComputeMachineConditions(&conditions, computev1alpha1.MachineStateRunning, volumeStatuses, nicStatuses)).To(Succeed())
+
+		for _, typ := range []computev1alpha1.MachineConditionType{"Ready", "VolumesReady", "NetworkInterfacesReady"} {
+			cond := findCondition(conditions, typ)
+			Expect(cond).NotTo(BeNil(), "condition %s should be present", typ)
+			Expect(cond.Status).To(Equal(corev1.ConditionTrue))
+			Expect(cond.LastTransitionTime.IsZero()).To(BeFalse())
+		}
+
+		volumesBefore := findCondition(conditions, "VolumesReady").LastTransitionTime
+		Expect(controllers.ComputeMachineConditions(&conditions, computev1alpha1.MachineStateRunning, volumeStatuses, nicStatuses)).To(Succeed())
+		Expect(findCondition(conditions, "VolumesReady").LastTransitionTime.Time).To(BeTemporally("==", volumesBefore.Time))
+	})
+})