From 9b22259891ae84e9decf829bbd35fefafe395b7e Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Fri, 12 Jun 2020 14:43:38 -0700 Subject: [PATCH 1/4] Groom conditions time --- .../generators/reconciler_reconciler.go | 2 +- reconciler/reconcile_common.go | 27 ++++++++++++++++++- reconciler/reconcile_common_test.go | 2 +- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/codegen/cmd/injection-gen/generators/reconciler_reconciler.go b/codegen/cmd/injection-gen/generators/reconciler_reconciler.go index 647b82fd0e..8b4d0c7331 100644 --- a/codegen/cmd/injection-gen/generators/reconciler_reconciler.go +++ b/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. diff --git a/reconciler/reconcile_common.go b/reconciler/reconcile_common.go index 6a32c06f7e..f8b3037c92 100644 --- a/reconciler/reconcile_common.go +++ b/reconciler/reconcile_common.go @@ -18,7 +18,12 @@ 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" "knative.dev/pkg/logging" ) @@ -41,7 +46,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) newStatus := resource.GetStatus() mgr := resource.GetConditionSet().Manage(newStatus) @@ -55,4 +60,24 @@ 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())} + for _, cond := range resource.GetStatus().Conditions { + + if oldCond := oldResource.GetStatus().GetCondition(cond.Type); oldCond != nil { + cond.LastTransitionTime = oldCond.LastTransitionTime + if reflect.DeepEqual(&cond, &oldCond) { + return + } + } + + cond.LastTransitionTime = now + } } diff --git a/reconciler/reconcile_common_test.go b/reconciler/reconcile_common_test.go index a1858fb2fd..f12a99c112 100644 --- a/reconciler/reconcile_common_test.go +++ b/reconciler/reconcile_common_test.go @@ -91,7 +91,7 @@ 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) From a4ba0c86e9a150f502f77cdbfd38fa8e76b072fc Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Fri, 12 Jun 2020 15:43:27 -0700 Subject: [PATCH 2/4] unit test --- reconciler/reconcile_common_test.go | 30 +++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/reconciler/reconcile_common_test.go b/reconciler/reconcile_common_test.go index f12a99c112..1ad48c4b8d 100644 --- a/reconciler/reconcile_common_test.go +++ b/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" @@ -94,10 +95,35 @@ func TestPostProcessReconcileBumpsGeneration(t *testing.T) { 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() + unchangedCond := oldResource.Status.Conditions[0] + changedCond := oldResource.Status.Conditions[1] + unchangedCond.LastTransitionTime = oldNow + changedCond.Status = corev1.ConditionFalse + + new := duckv1.KRShaped(resource) + old := duckv1.KRShaped(oldResource) + PostProcessReconcile(context.Background(), new, old) + + if unchangedCond.LastTransitionTime != oldNow { + t.Errorf("Expected unchanged condition to keep old timestamp. Got=%v Want=%v", + unchangedCond.LastTransitionTime, oldNow) + } + + if changedCond.LastTransitionTime == oldNow { + t.Errorf("Expected changed condition to get a new timestamp. Got=%v", changedCond.LastTransitionTime) } } From ef3d76da0c37195dbc418c0d38f276dd32c8b2aa Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Fri, 12 Jun 2020 20:07:30 -0700 Subject: [PATCH 3/4] unit test fix --- reconciler/reconcile_common.go | 8 +++++--- reconciler/reconcile_common_test.go | 15 ++++++++++----- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/reconciler/reconcile_common.go b/reconciler/reconcile_common.go index f8b3037c92..93cff51e63 100644 --- a/reconciler/reconcile_common.go +++ b/reconciler/reconcile_common.go @@ -69,12 +69,14 @@ func PostProcessReconcile(ctx context.Context, resource, oldResource duckv1.KRSh // conditions share the same timestamp. func groomConditionsTransitionTime(resource, oldResource duckv1.KRShaped) { now := apis.VolatileTime{Inner: metav1.NewTime(time.Now())} - for _, cond := range resource.GetStatus().Conditions { + 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) { - return + if reflect.DeepEqual(cond, oldCond) { + continue } } diff --git a/reconciler/reconcile_common_test.go b/reconciler/reconcile_common_test.go index 1ad48c4b8d..cf1a71db33 100644 --- a/reconciler/reconcile_common_test.go +++ b/reconciler/reconcile_common_test.go @@ -109,21 +109,26 @@ func TestPostProcessReconcileUpdatesTransitionTimes(t *testing.T) { oldNow := apis.VolatileTime{Inner: metav1.NewTime(time.Now())} resource := makeResource() oldResource := makeResource() - unchangedCond := oldResource.Status.Conditions[0] - changedCond := oldResource.Status.Conditions[1] - unchangedCond.LastTransitionTime = oldNow - changedCond.Status = corev1.ConditionFalse + // 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", changedCond.LastTransitionTime) + t.Errorf("Expected changed condition to get a new timestamp. Got=%v Want=%v", + changedCond.LastTransitionTime, oldNow) } } From ccfdfc4d1529598b1b92d6429547ae274f68d673 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Fri, 12 Jun 2020 20:09:43 -0700 Subject: [PATCH 4/4] make readme accurate --- injection/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/injection/README.md b/injection/README.md index d30b090bc7..e23ff32f52 100644 --- a/injection/README.md +++ b/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