Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Groom conditions LastTransitionTime in postprocess #1403

Merged
merged 6 commits into from
Jun 18, 2020
Merged

Groom conditions LastTransitionTime in postprocess #1403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b22259
Groom conditions time
whaught Jun 12, 2020
a4ba0c8
unit test
whaught Jun 12, 2020
ef3d76d
unit test fix
whaught Jun 13, 2020
ccfdfc4
make readme accurate
whaught Jun 13, 2020
c67d97e
Merge remote-tracking branch 'upstream/master' into groom-conditions
whaught Jun 15, 2020
227f0f1
Merge remote-tracking branch 'upstream/master' into groom-conditions
whaught Jun 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion codegen/cmd/injection-gen/generators/reconciler_reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion injection/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ reconciler.PreProcessReconcile(ctx, resource)

reconcileEvent = r.reconciler.ReconcileKind(ctx, resource)

reconciler.PostProcessReconcile(ctx, resource)
reconciler.PostProcessReconcile(ctx, resource, oldResource)
```

#### Stubs
Expand Down
28 changes: 27 additions & 1 deletion reconciler/reconcile_common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 maybe I'm misunderstanding, but iiuc the entire point of VolatileTime is to avoid what this is doing 😅

Copy link
Contributor Author

@whaught whaught Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The VolatileTime strategy has a few weaknesses I've outlined in #1394

  • Requires use of k8s specific comparison libraries
  • The snapshot of now() is taken multiple times during reconciliation, but all written back at the same time. This can make it hard to tell which conditions are changed in a single reconciliation step (although this is optional, we could update them as we go).
  • It fails to keep the LastTransitionTime stable if a single reconciliation sets conditions more than once (eg. flip-flopping but winds up in the same place).

This obviates the VolatileTime and condition manager strategy so that implementers don't really need to worry about how they are modifying the status struct - it just works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me comment on the issue, but it may be worth discussing more synchronous Weds (put on the agenda for API WG?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea let's just discuss in WG - that'll be easiest

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
}
}
37 changes: 34 additions & 3 deletions reconciler/reconcile_common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package reconciler
import (
"context"
"testing"
"time"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -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)
}
}