Repository: knative/serving
Pull request #10745 (2021-02-11 00:51:29)

Deep cleaning of the time manipulation in config and revision and other fixes to the codebase

- Stop passing the clock around when we mean `now`
- This yielded quite a few places to clean up
- Various other nits and bits that I picked to fix on the way.

(Should be the last in this series of fixes, I hope)

/assign @tcnghia @whaught 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10745#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/apis/OWNERS](https://github.com/knative/serving/blob/master/pkg/apis/OWNERS)~~ [vagababov]
- ~~[pkg/reconciler/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/OWNERS)~~ [vagababov]
- ~~[pkg/testing/OWNERS](https://github.com/knative/serving/blob/master/pkg/testing/OWNERS)~~ [vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10745#pullrequestreview-588184479" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10745#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/apis/OWNERS](https://github.com/knative/serving/blob/master/pkg/apis/OWNERS)~~ [tcnghia,vagababov]
- ~~[pkg/reconciler/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/OWNERS)~~ [tcnghia,vagababov]
- ~~[pkg/testing/OWNERS](https://github.com/knative/serving/blob/master/pkg/testing/OWNERS)~~ [tcnghia,vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10745?src=pr&el=h1) Report
> Merging [#10745](https://codecov.io/gh/knative/serving/pull/10745?src=pr&el=desc) (ac19fb1) into [master](https://codecov.io/gh/knative/serving/commit/17fb0e0293ed1dd987a6c525bce3377d2ec8090b?el=desc) (17fb0e0) will **decrease** coverage by `0.03%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/serving/pull/10745/graphs/tree.svg?width=650&height=150&src=pr&token=MPpDfAj4Ui)](https://codecov.io/gh/knative/serving/pull/10745?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #10745      +/-   ##
==========================================
- Coverage   88.05%   88.02%   -0.04%     
==========================================
  Files         188      188              
  Lines        9058     9059       +1     
==========================================
- Hits         7976     7974       -2     
  Misses        830      830              
- Partials      252      255       +3     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10745?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/apis/serving/v1/revision\_helpers.go](https://codecov.io/gh/knative/serving/pull/10745/diff?src=pr&el=tree#diff-cGtnL2FwaXMvc2VydmluZy92MS9yZXZpc2lvbl9oZWxwZXJzLmdv) | `100.00% <100.00%> (ø)` | |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/10745/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9jb25maWd1cmF0aW9uLmdv) | `84.37% <100.00%> (-3.91%)` | :arrow_down: |
| [pkg/reconciler/configuration/resources/revision.go](https://codecov.io/gh/knative/serving/pull/10745/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9yZXNvdXJjZXMvcmV2aXNpb24uZ28=) | `78.57% <100.00%> (ø)` | |
| [pkg/activator/net/revision\_backends.go](https://codecov.io/gh/knative/serving/pull/10745/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9uZXQvcmV2aXNpb25fYmFja2VuZHMuZ28=) | `91.32% <0.00%> (+0.91%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/10745?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/serving/pull/10745?src=pr&el=footer). Last update [17fb0e0...ac19fb1](https://codecov.io/gh/knative/serving/pull/10745?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/apis/serving/v1/revision_helpers.go b/pkg/apis/serving/v1/revision_helpers.go
index ea83d1137..effc22445 100644
--- a/pkg/apis/serving/v1/revision_helpers.go
+++ b/pkg/apis/serving/v1/revision_helpers.go
@@ -20,7 +20,6 @@ import (
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/util/clock"
 	net "knative.dev/networking/pkg/apis/networking"
 	"knative.dev/pkg/kmeta"
 	"knative.dev/serving/pkg/apis/serving"
@@ -89,7 +88,7 @@ func (rs *RevisionSpec) GetContainer() *corev1.Container {
 
 // SetRoutingState sets the routingState label on this Revision and updates the
 // routingStateModified annotation.
-func (r *Revision) SetRoutingState(state RoutingState, clock clock.PassiveClock) {
+func (r *Revision) SetRoutingState(state RoutingState, tm time.Time) {
 	stateStr := string(state)
 	if t := r.Annotations[serving.RoutingStateModifiedAnnotationKey]; t != "" &&
 		r.Labels[serving.RoutingStateLabelKey] == stateStr {
@@ -101,8 +100,9 @@ func (r *Revision) SetRoutingState(state RoutingState, clock clock.PassiveClock)
 
 	r.Annotations = kmeta.UnionMaps(r.Annotations,
 		map[string]string{
-			serving.RoutingStateModifiedAnnotationKey: RoutingStateModifiedString(clock.Now()),
-		})
+			serving.RoutingStateModifiedAnnotationKey: RoutingStateModifiedString(tm),
+		},
+	)
 }
 
 // RoutingStateModifiedString gives a formatted now timestamp.
diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go
index 2cff7ca4e..ffad0fe52 100644
--- a/pkg/reconciler/configuration/configuration.go
+++ b/pkg/reconciler/configuration/configuration.go
@@ -279,7 +279,7 @@ func (c *Reconciler) latestCreatedRevision(ctx context.Context, config *v1.Confi
 func (c *Reconciler) createRevision(ctx context.Context, config *v1.Configuration) (*v1.Revision, error) {
 	logger := logging.FromContext(ctx)
 
-	rev := resources.MakeRevision(ctx, config, c.clock)
+	rev := resources.MakeRevision(ctx, config, c.clock.Now())
 	created, err := c.client.ServingV1().Revisions(config.Namespace).Create(ctx, rev, metav1.CreateOptions{})
 	if err != nil {
 		return nil, err
diff --git a/pkg/reconciler/configuration/resources/revision.go b/pkg/reconciler/configuration/resources/revision.go
index 49c952aa0..900ca1bbb 100644
--- a/pkg/reconciler/configuration/resources/revision.go
+++ b/pkg/reconciler/configuration/resources/revision.go
@@ -19,16 +19,16 @@ package resources
 import (
 	"context"
 	"fmt"
+	"time"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/clock"
 	"knative.dev/pkg/kmeta"
 	"knative.dev/serving/pkg/apis/serving"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 )
 
 // MakeRevision creates a revision object from configuration.
-func MakeRevision(ctx context.Context, configuration *v1.Configuration, clock clock.PassiveClock) *v1.Revision {
+func MakeRevision(ctx context.Context, configuration *v1.Configuration, tm time.Time) *v1.Revision {
 	// Start from the ObjectMeta/Spec inlined in the Configuration resources.
 	rev := &v1.Revision{
 		ObjectMeta: configuration.Spec.GetTemplate().ObjectMeta,
@@ -42,10 +42,10 @@ func MakeRevision(ctx context.Context, configuration *v1.Configuration, clock cl
 	}
 
 	// Pending tells the labeler that we have not processed this revision.
-	rev.SetRoutingState(v1.RoutingStatePending, clock)
+	rev.SetRoutingState(v1.RoutingStatePending, tm)
 
 	updateRevisionLabels(rev, configuration)
-	updateRevisionAnnotations(rev, configuration)
+	updateRevisionAnnotations(rev, configuration, tm)
 
 	// Populate OwnerReferences so that deletes cascade.
 	rev.OwnerReferences = append(rev.OwnerReferences, *kmeta.NewControllerRef(configuration))
@@ -75,7 +75,7 @@ func updateRevisionLabels(rev, config metav1.Object) {
 }
 
 // updateRevisionAnnotations sets the revision's annotation given a Configuration's updater annotation.
-func updateRevisionAnnotations(rev *v1.Revision, config metav1.Object) {
+func updateRevisionAnnotations(rev *v1.Revision, config metav1.Object, tm time.Time) {
 	annotations := rev.GetAnnotations()
 	if annotations == nil {
 		annotations = make(map[string]string, 1)
@@ -89,7 +89,7 @@ func updateRevisionAnnotations(rev *v1.Revision, config metav1.Object) {
 
 	if v, ok := cans[serving.RoutesAnnotationKey]; ok {
 		annotations[serving.RoutesAnnotationKey] = v
-		rev.SetRoutingState(v1.RoutingStateActive, clock.RealClock{})
+		rev.SetRoutingState(v1.RoutingStateActive, tm)
 	}
 
 	rev.SetAnnotations(annotations)


Test patch:
diff --git a/pkg/apis/serving/v1/revision_helpers_test.go b/pkg/apis/serving/v1/revision_helpers_test.go
index f689b52d9..e176d67c1 100644
--- a/pkg/apis/serving/v1/revision_helpers_test.go
+++ b/pkg/apis/serving/v1/revision_helpers_test.go
@@ -23,7 +23,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/clock"
 
 	net "knative.dev/networking/pkg/apis/networking"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
@@ -262,41 +261,42 @@ func TestGetContainer(t *testing.T) {
 func TestSetRoutingState(t *testing.T) {
 	rev := &Revision{}
 	empty := time.Time{}
-	clock := &clock.RealClock{}
+	now := time.Now()
 
 	// Test unset timestamp
 	if rev.GetRoutingStateModified() != empty {
-		t.Errorf("expected default value for unset modified annotation.")
+		t.Error("Expected default value for unset modified annotation.")
 	}
 
 	// Test retrieving routing state and modified
-	rev.SetRoutingState(RoutingStateActive, clock)
+	rev.SetRoutingState(RoutingStateActive, now)
 	if state := rev.GetRoutingState(); state != RoutingStateActive {
-		t.Errorf("retrieved the wrong state got: %v want: %v", state, RoutingStateActive)
+		t.Errorf("Retrieved the wrong state got: %v want: %v", state, RoutingStateActive)
 	}
+
 	modified := rev.GetRoutingStateModified()
 	if modified == empty {
-		t.Error("expected a timestamp. got", modified)
+		t.Error("Expected a non-zero timestamp")
 	}
 
 	// Test that no-op modifications don't bump timestamps.
-	rev.SetRoutingState(RoutingStateActive, clock)
+	rev.SetRoutingState(RoutingStateActive, now)
 	if rev.GetRoutingStateModified() != modified {
-		t.Errorf("modified was bumped, but no change expected.")
+		t.Error("Modified timestamp was bumped, but no change expected.")
 	}
 
 	// Test the actual modifications do bump timestamps.
-	rev.SetRoutingState(RoutingStateReserve, clock)
+	rev.SetRoutingState(RoutingStateReserve, now)
 	if state := rev.GetRoutingState(); state != RoutingStateReserve {
-		t.Errorf("retrieved the wrong state got: %v want: %v", state, RoutingStateReserve)
+		t.Errorf("Retrieved the wrong state got: %v want: %v", state, RoutingStateReserve)
 	}
 	if rev.GetRoutingStateModified() != modified {
-		t.Errorf("expected modified to be bumped.")
+		t.Error("Expected modified to be bumped.")
 	}
 
 	// Test unparsable timestamp.
 	rev.Annotations[serving.RoutingStateModifiedAnnotationKey] = "invalid"
-	if rev.GetRoutingStateModified() != empty {
-		t.Errorf("expected default value for unparsable annotation.")
+	if got, want := rev.GetRoutingStateModified(), empty; got != want {
+		t.Error("Expected default value for unparsable annotationm but got:", got)
 	}
 }
diff --git a/pkg/reconciler/configuration/configuration_test.go b/pkg/reconciler/configuration/configuration_test.go
index 96453d95f..e9ba36714 100644
--- a/pkg/reconciler/configuration/configuration_test.go
+++ b/pkg/reconciler/configuration/configuration_test.go
@@ -57,19 +57,17 @@ var revisionSpec = v1.RevisionSpec{
 	TimeoutSeconds: ptr.Int64(60),
 }
 
-var testCtx context.Context
-var testClock clock.PassiveClock
+var (
+	testCtx   context.Context
+	testClock clock.PassiveClock
+)
 
 // This is heavily based on the way the OpenShift Ingress controller tests its reconciliation method.
 func TestReconcile(t *testing.T) {
 	testClock = clock.NewFakePassiveClock(time.Now())
 	testCtx = context.Background()
-
-	test(t)
-}
-
-func test(t *testing.T) {
 	retryAttempted := false
+
 	now := testClock.Now()
 
 	table := TableTest{{
@@ -566,8 +564,8 @@ func cfg(name, namespace string, generation int64, co ...ConfigOption) *v1.Confi
 }
 
 func rev(name, namespace string, generation int64, ro ...RevisionOption) *v1.Revision {
-	r := resources.MakeRevision(testCtx, cfg(name, namespace, generation), testClock)
-	r.SetDefaults(context.Background())
+	r := resources.MakeRevision(testCtx, cfg(name, namespace, generation), testClock.Now())
+	r.SetDefaults(testCtx)
 	for _, opt := range ro {
 		opt(r)
 	}
diff --git a/pkg/reconciler/configuration/resources/revision_test.go b/pkg/reconciler/configuration/resources/revision_test.go
index 80a10a869..c84a68ce8 100644
--- a/pkg/reconciler/configuration/resources/revision_test.go
+++ b/pkg/reconciler/configuration/resources/revision_test.go
@@ -24,7 +24,6 @@ import (
 	"github.com/google/go-cmp/cmp"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/clock"
 
 	"knative.dev/pkg/ptr"
 	"knative.dev/serving/pkg/apis/serving"
@@ -34,7 +33,7 @@ import (
 var fakeCurTime = time.Unix(1e9, 20102021)
 
 func TestMakeRevisions(t *testing.T) {
-	clock := clock.NewFakePassiveClock(fakeCurTime)
+	tm := fakeCurTime
 
 	tests := []struct {
 		name          string
@@ -328,7 +327,7 @@ func TestMakeRevisions(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			got := MakeRevision(context.Background(), test.configuration, clock)
+			got := MakeRevision(context.Background(), test.configuration, tm)
 			if diff := cmp.Diff(test.want, got); diff != "" {
 				t.Error("MakeRevision (-want, +got) =", diff)
 			}
diff --git a/pkg/reconciler/gc/reconciler_test.go b/pkg/reconciler/gc/reconciler_test.go
index ab052e814..e3373c9f8 100644
--- a/pkg/reconciler/gc/reconciler_test.go
+++ b/pkg/reconciler/gc/reconciler_test.go
@@ -140,7 +140,7 @@ func cfg(name, namespace string, generation int64, co ...ConfigOption) *v1.Confi
 
 func rev(name, namespace string, generation int64, ro ...RevisionOption) *v1.Revision {
 	config := cfg(name, namespace, generation)
-	rev := resources.MakeRevision(context.Background(), config, clock.RealClock{})
+	rev := resources.MakeRevision(context.Background(), config, time.Now())
 	rev.SetDefaults(context.Background())
 
 	for _, opt := range ro {
diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go
index 8186e000b..2ab1edbd8 100644
--- a/pkg/reconciler/gc/v2/gc_test.go
+++ b/pkg/reconciler/gc/v2/gc_test.go
@@ -557,7 +557,7 @@ func cfg(name, namespace string, generation int64, co ...ConfigOption) *v1.Confi
 
 func rev(configName, namespace string, generation int64, ro ...RevisionOption) *v1.Revision {
 	config := cfg(configName, namespace, generation)
-	rev := resources.MakeRevision(context.Background(), config, clock.RealClock{})
+	rev := resources.MakeRevision(context.Background(), config, time.Now())
 	rev.SetDefaults(context.Background())
 
 	for _, opt := range ro {
diff --git a/pkg/reconciler/service/service_test.go b/pkg/reconciler/service/service_test.go
index 91efb9e47..ca7d5d866 100644
--- a/pkg/reconciler/service/service_test.go
+++ b/pkg/reconciler/service/service_test.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"testing"
+	"time"
 
 	// Install our fake informers
 	_ "knative.dev/serving/pkg/client/injection/informers/serving/v1/configuration/fake"
@@ -47,7 +48,6 @@ import (
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/util/clock"
 	clientgotesting "k8s.io/client-go/testing"
 
 	. "knative.dev/pkg/reconciler/testing"
@@ -870,5 +870,5 @@ func RouteFailed(reason, message string) RouteOption {
 
 func rev(name, namespace string, so ServiceOption, co ...ConfigOption) *v1.Revision {
 	cfg := config(name, namespace, so, co...)
-	return configresources.MakeRevision(context.Background(), cfg, clock.RealClock{})
+	return configresources.MakeRevision(context.Background(), cfg, time.Now())
 }
diff --git a/pkg/testing/v1/revision.go b/pkg/testing/v1/revision.go
index 8d38539bc..ec4b009df 100644
--- a/pkg/testing/v1/revision.go
+++ b/pkg/testing/v1/revision.go
@@ -103,7 +103,7 @@ func WithRoutingStateModified(t time.Time) RevisionOption {
 // WithRoutingState updates the annotation to the provided timestamp.
 func WithRoutingState(s v1.RoutingState, c clock.PassiveClock) RevisionOption {
 	return func(rev *v1.Revision) {
-		rev.SetRoutingState(s, c)
+		rev.SetRoutingState(s, c.Now())
 	}
 }