Groom conditions LastTransitionTime in postprocess (#1403)

* Groom conditions time

* unit test

* unit test fix

* make readme accurate

codegen/cmd/injection-gen/generators/reconciler_reconciler.go

@@ -320,7 +320,7 @@ func (r *reconcilerImpl) Reconcile(ctx {{.contextContext|raw}}, key string) erro
 		reconcileEvent = r.reconciler.ReconcileKind(ctx, resource)
 
 		{{if .isKRShaped}}
-		reconciler.PostProcessReconcile(ctx, resource)
+		reconciler.PostProcessReconcile(ctx, resource, original)
 		{{end}}
 	} else if fin, ok := r.reconciler.(Finalizer); ok {
 		// Append the target method to the logger.

injection/README.md

@@ -471,7 +471,7 @@ reconciler.PreProcessReconcile(ctx, resource)
 
 reconcileEvent = r.reconciler.ReconcileKind(ctx, resource)
 
-reconciler.PostProcessReconcile(ctx, resource)
+reconciler.PostProcessReconcile(ctx, resource, oldResource)
 ```
 
 #### Stubs

reconciler/reconcile_common.go

@@ -18,6 +18,10 @@ package reconciler
 
 import (
 	"context"
+	"reflect"
+	"time"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"knative.dev/pkg/apis"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
@@ -50,7 +54,7 @@ func PreProcessReconcile(ctx context.Context, resource duckv1.KRShaped) {
 }
 
 // PostProcessReconcile contains logic to apply after reconciliation of a resource.
-func PostProcessReconcile(ctx context.Context, resource duckv1.KRShaped) {
+func PostProcessReconcile(ctx context.Context, resource, oldResource duckv1.KRShaped) {
 	logger := logging.FromContext(ctx)
 	status := resource.GetStatus()
 	mgr := resource.GetConditionSet().Manage(status)
@@ -64,4 +68,26 @@ func PostProcessReconcile(ctx context.Context, resource duckv1.KRShaped) {
 	} else if rc.Reason == failedGenerationBump {
 		logger.Warn("A reconciler observed a new generation without updating the resource status")
 	}
+
+	groomConditionsTransitionTime(resource, oldResource)
+}
+
+// groomConditionsTransitionTime ensures that the LastTransitionTime only advances for resources
+// where the condition has changed during reconciliation. This also ensures that all advanced
+// conditions share the same timestamp.
+func groomConditionsTransitionTime(resource, oldResource duckv1.KRShaped) {
+	now := apis.VolatileTime{Inner: metav1.NewTime(time.Now())}
+	sts := resource.GetStatus()
+	for i := range sts.Conditions {
+		cond := &sts.Conditions[i]
+
+		if oldCond := oldResource.GetStatus().GetCondition(cond.Type); oldCond != nil {
+			cond.LastTransitionTime = oldCond.LastTransitionTime
+			if reflect.DeepEqual(cond, oldCond) {
+				continue
+			}
+		}
+
+		cond.LastTransitionTime = now
+	}
 }

reconciler/reconcile_common_test.go

@@ -19,6 +19,7 @@ package reconciler
 import (
 	"context"
 	"testing"
+	"time"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -99,13 +100,43 @@ func TestPostProcessReconcileBumpsGeneration(t *testing.T) {
 	resource := makeResource()
 
 	krShape := duckv1.KRShaped(resource)
-	PostProcessReconcile(context.Background(), krShape)
+	PostProcessReconcile(context.Background(), krShape, krShape)
 
 	if resource.Status.ObservedGeneration != resource.Generation {
-		t.Errorf("Expected observed generation bump got=%d want=%d", resource.Status.ObservedGeneration, resource.Generation)
+		t.Errorf("Expected observed generation bump got=%d want=%d",
+			resource.Status.ObservedGeneration, resource.Generation)
 	}
 
 	if krShape.GetStatus().ObservedGeneration != krShape.GetGeneration() {
-		t.Errorf("Expected observed generation bump got=%d want=%d", resource.Status.ObservedGeneration, resource.Generation)
+		t.Errorf("Expected observed generation bump got=%d want=%d",
+			resource.Status.ObservedGeneration, resource.Generation)
+	}
+}
+
+func TestPostProcessReconcileUpdatesTransitionTimes(t *testing.T) {
+	oldNow := apis.VolatileTime{Inner: metav1.NewTime(time.Now())}
+	resource := makeResource()
+	oldResource := makeResource()
+	// initialize old conditions with oldNow
+	oldResource.Status.Conditions[0].LastTransitionTime = oldNow
+	oldResource.Status.Conditions[1].LastTransitionTime = oldNow
+	// change the second condition, but keep the old timestamp.
+	resource.Status.Conditions[1].LastTransitionTime = oldNow
+	resource.Status.Conditions[1].Status = corev1.ConditionFalse
+
+	new := duckv1.KRShaped(resource)
+	old := duckv1.KRShaped(oldResource)
+	PostProcessReconcile(context.Background(), new, old)
+
+	unchangedCond := resource.Status.Conditions[0]
+	if unchangedCond.LastTransitionTime != oldNow {
+		t.Errorf("Expected unchanged condition to keep old timestamp. Got=%v Want=%v",
+			unchangedCond.LastTransitionTime, oldNow)
+	}
+
+	changedCond := resource.Status.Conditions[1]
+	if changedCond.LastTransitionTime == oldNow {
+		t.Errorf("Expected changed condition to get a new timestamp. Got=%v Want=%v",
+			changedCond.LastTransitionTime, oldNow)
 	}
 }