Repository: knative/serving
Pull request #10231 (2020-12-01 01:55:30)

Observe start time and compute step time
Also refactor to accept time in nanonsecs directly, than pass in time.
This simplifies testing as well

/assign @tcnghia 


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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10231#" 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/reconciler/route/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/route/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":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10231?src=pr&el=h1) Report
> Merging [#10231](https://codecov.io/gh/knative/serving/pull/10231?src=pr&el=desc) (04cd971) into [master](https://codecov.io/gh/knative/serving/commit/091384b58648acf8d4c12fc7a044e3f4607eb01b?el=desc) (091384b) will **decrease** coverage by `0.00%`.
> The diff coverage is `77.77%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10231      +/-   ##
==========================================
- Coverage   88.16%   88.15%   -0.01%     
==========================================
  Files         184      184              
  Lines        8607     8613       +6     
==========================================
+ Hits         7588     7593       +5     
  Misses        784      784              
- Partials      235      236       +1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10231?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/traffic/rollout.go](https://codecov.io/gh/knative/serving/pull/10231/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy9yb2xsb3V0Lmdv) | `97.75% <60.00%> (-2.25%)` | :arrow_down: |
| [pkg/reconciler/route/reconcile\_resources.go](https://codecov.io/gh/knative/serving/pull/10231/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcmVjb25jaWxlX3Jlc291cmNlcy5nbw==) | `86.40% <100.00%> (+0.40%)` | :arrow_up: |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/10231/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9jb25maWd1cmF0aW9uLmdv) | `85.93% <0.00%> (-0.79%)` | :arrow_down: |
| [pkg/autoscaler/statforwarder/forwarder.go](https://codecov.io/gh/knative/serving/pull/10231/diff?src=pr&el=tree#diff-cGtnL2F1dG9zY2FsZXIvc3RhdGZvcndhcmRlci9mb3J3YXJkZXIuZ28=) | `82.44% <0.00%> (+1.06%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/10231?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/10231?src=pr&el=footer). Last update [091384b...04cd971](https://codecov.io/gh/knative/serving/pull/10231?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/go.mod b/go.mod
index b519f4026..19c17750b 100644
--- a/go.mod
+++ b/go.mod
@@ -37,5 +37,5 @@ require (
 	knative.dev/caching v0.0.0-20201125150135-91fb5833c3c6
 	knative.dev/hack v0.0.0-20201125230335-c46a6498e9ed
 	knative.dev/networking v0.0.0-20201125133435-4b21f11ccfa7
-	knative.dev/pkg v0.0.0-20201125095035-9bf616d2f46a
+	knative.dev/pkg v0.0.0-20201130192436-e5346d90e980
 )
diff --git a/go.sum b/go.sum
index 530275691..914cb879e 100644
--- a/go.sum
+++ b/go.sum
@@ -1224,6 +1224,8 @@ knative.dev/networking v0.0.0-20201125133435-4b21f11ccfa7 h1:O40i4ZCYfpaw7m1yUwt
 knative.dev/networking v0.0.0-20201125133435-4b21f11ccfa7/go.mod h1:EpImbpW3gu85Mg39GT7gUJTVYLtFYWDsZQCZlKFmGUM=
 knative.dev/pkg v0.0.0-20201125095035-9bf616d2f46a h1:pdJpLaq50mLKrPWYSQgTH2p64Dk7Fq/xID6l0F69cVY=
 knative.dev/pkg v0.0.0-20201125095035-9bf616d2f46a/go.mod h1:wXZqP8MXCxb51yNFlecA13BwG7Hk370SWDFWV4dx4ug=
+knative.dev/pkg v0.0.0-20201130192436-e5346d90e980 h1:44zakwrFjbivCM/YR+ni70e9u1ELtwA7VF1HSmUcj+g=
+knative.dev/pkg v0.0.0-20201130192436-e5346d90e980/go.mod h1:wXZqP8MXCxb51yNFlecA13BwG7Hk370SWDFWV4dx4ug=
 modernc.org/cc v1.0.0/go.mod h1:1Sk4//wdnYJiUIxnW8ddKpaOJCF37yAdqYnkxUpaYxw=
 modernc.org/golex v1.0.0/go.mod h1:b/QX9oBD/LhixY6NDh+IdGv17hgB+51fET1i2kPSmvk=
 modernc.org/mathutil v1.0.0/go.mod h1:wU0vUrJsVWBZ4P6e7xtFJEhFSNsfRLJ8H458uRjg03k=
diff --git a/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go
index f2fd17e1e..14a91c791 100644
--- a/pkg/reconciler/route/reconcile_resources.go
+++ b/pkg/reconciler/route/reconcile_resources.go
@@ -75,7 +75,14 @@ func (c *Reconciler) reconcileIngress(
 			ingress.Annotations[networking.RolloutAnnotationKey])
 
 		// And recompute the rollout state.
-		effectiveRO := curRO.Step(prevRO, c.clock)
+		effectiveRO := curRO.Step(prevRO, int(c.clock.Now().UnixNano()))
+
+		// Now check if the ingress status changed from not ready to ready.
+		rtView := r.Status.GetCondition(v1.RouteConditionIngressReady)
+		if ingress.IsReady() && !rtView.IsTrue() {
+			effectiveRO.ObserveReady(int(c.clock.Now().UnixNano()))
+		}
+
 		desired, err := resources.MakeIngressWithRollout(ctx, r, tc, effectiveRO,
 			tls, ingressClass, acmeChallenges...)
 		if err != nil {
diff --git a/pkg/reconciler/route/traffic/rollout.go b/pkg/reconciler/route/traffic/rollout.go
index 94e7e36f8..de61b2634 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -22,9 +22,9 @@ limitations under the License.
 package traffic
 
 import (
+	"math"
 	"sort"
-
-	"knative.dev/pkg/system"
+	"time"
 )
 
 // Rollout encapsulates the current rollout state of the system.
@@ -115,12 +115,24 @@ func (cur *Rollout) Validate() bool {
 	return true
 }
 
+// ObserveReady traverses the configs and the ones that are in rollout
+// but have not observed step time yet, will have it set, to
+// max(1, nowTS-cfg.StartTime).
+func (cur *Rollout) ObserveReady(nowTS int) {
+	for i := range cur.Configurations {
+		c := &cur.Configurations[i]
+		if c.StepDuration == 0 && c.StartTime > 0 {
+			c.StepDuration = int(math.Max(1, math.Ceil(time.Duration(nowTS-c.StartTime).Seconds())))
+		}
+	}
+}
+
 // Step merges this rollout object with the previous state and
 // returns a new Rollout object representing the merged state.
 // At the end of the call the returned object will contain the
 // desired traffic shape.
 // Step will return cur if no previous state was available.
-func (cur *Rollout) Step(prev *Rollout, clk system.Clock) *Rollout {
+func (cur *Rollout) Step(prev *Rollout, nowTS int) *Rollout {
 	if prev == nil || len(prev.Configurations) == 0 {
 		return cur
 	}
@@ -163,7 +175,7 @@ func (cur *Rollout) Step(prev *Rollout, clk system.Clock) *Rollout {
 				// altogether.
 				switch p := ccfgs[i].Percent; {
 				case p > 1:
-					ret = append(ret, *stepConfig(ccfgs[i], pcfgs[j], clk))
+					ret = append(ret, *stepConfig(ccfgs[i], pcfgs[j], nowTS))
 				case p == 1:
 					// Skip all the work if it's a common A/B scenario where the test config
 					// receives just 1% of traffic.
@@ -223,7 +235,7 @@ func adjustPercentage(goal int, cr *ConfigurationRollout) {
 
 // stepConfig takes previous and goal configuration shapes and returns a new
 // config rollout, after computing the percetage allocations.
-func stepConfig(goal, prev *ConfigurationRollout, clk system.Clock) *ConfigurationRollout {
+func stepConfig(goal, prev *ConfigurationRollout, nowTS int) *ConfigurationRollout {
 	pc := len(prev.Revisions)
 	ret := &ConfigurationRollout{
 		ConfigurationName: goal.ConfigurationName,
@@ -258,7 +270,7 @@ func stepConfig(goal, prev *ConfigurationRollout, clk system.Clock) *Configurati
 	}
 
 	// Otherwise we start a rollout, which means we need to stamp the starttime.
-	ret.StartTime = int(clk.Now().Unix())
+	ret.StartTime = nowTS
 
 	// Go backwards and find first revision with traffic assignment > 0.
 	// Reduce it by one, so we can give that 1% to the new revision.
diff --git a/vendor/modules.txt b/vendor/modules.txt
index 02c85e5d5..be5672bde 100644
--- a/vendor/modules.txt
+++ b/vendor/modules.txt
@@ -1009,7 +1009,7 @@ knative.dev/networking/test/conformance/ingress
 knative.dev/networking/test/defaultsystem
 knative.dev/networking/test/test_images/grpc-ping/proto
 knative.dev/networking/test/types
-# knative.dev/pkg v0.0.0-20201125095035-9bf616d2f46a
+# knative.dev/pkg v0.0.0-20201130192436-e5346d90e980
 ## explicit
 knative.dev/pkg/apiextensions/storageversion
 knative.dev/pkg/apiextensions/storageversion/cmd/migrate


Test patch:
diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go
index 7c2eec26d..3f2bcd5f1 100644
--- a/pkg/reconciler/route/table_test.go
+++ b/pkg/reconciler/route/table_test.go
@@ -2608,7 +2608,7 @@ func simpleRollout(cfg string, revs []traffic.RevisionRollout, now time.Time) In
 		r := &traffic.Rollout{
 			Configurations: []traffic.ConfigurationRollout{{
 				ConfigurationName: cfg,
-				StartTime:         int(now.Unix()),
+				StartTime:         int(now.UnixNano()),
 				Percent:           100,
 				Revisions:         revs,
 			}},
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index 6a6e12fef..6ee8baaec 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -28,14 +28,10 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
-	ptest "knative.dev/pkg/reconciler/testing"
 )
 
 func TestStep(t *testing.T) {
-	now := time.Now()
-	clk := ptest.FakeClock{
-		Time: now,
-	}
+	now := int(time.Now().UnixNano())
 	tests := []struct {
 		name            string
 		prev, cur, want *Rollout
@@ -207,7 +203,7 @@ func TestStep(t *testing.T) {
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "mick",
 				Percent:           100,
-				StartTime:         int(now.Unix()),
+				StartTime:         now,
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
 					Percent:      99,
@@ -246,7 +242,7 @@ func TestStep(t *testing.T) {
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "mick",
 				Percent:           33,
-				StartTime:         int(now.Unix()),
+				StartTime:         now,
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
 					Percent:      2,
@@ -287,7 +283,7 @@ func TestStep(t *testing.T) {
 		want: &Rollout{
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "mick",
-				StartTime:         int(now.Unix()),
+				StartTime:         now,
 				Percent:           75,
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
@@ -327,7 +323,7 @@ func TestStep(t *testing.T) {
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "brian",
 				Percent:           70,
-				StartTime:         int(now.Unix()),
+				StartTime:         now,
 				Revisions: []RevisionRollout{{
 					RevisionName: "exile-on-main-st",
 					Percent:      69,
@@ -353,7 +349,7 @@ func TestStep(t *testing.T) {
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "mick",
 				Percent:           100,
-				StartTime:         int(now.Unix()) - 1982, // A rollout in progress, this would be set.
+				StartTime:         now - 1982, // A rollout in progress, this would be set.
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
 					Percent:      95,
@@ -367,7 +363,7 @@ func TestStep(t *testing.T) {
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "mick",
 				Percent:           100,
-				StartTime:         int(now.Unix()),
+				StartTime:         now,
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
 					Percent:      95,
@@ -396,7 +392,7 @@ func TestStep(t *testing.T) {
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "mick",
 				Percent:           100,
-				StartTime:         int(now.Unix()) - 1984, // A rollout in progress, this would be set.
+				StartTime:         now - 1984, // A rollout in progress, this would be set.
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
 					Percent:      99,
@@ -409,7 +405,7 @@ func TestStep(t *testing.T) {
 		want: &Rollout{
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "mick",
-				StartTime:         int(now.Unix()),
+				StartTime:         now,
 				Percent:           100,
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
@@ -579,7 +575,7 @@ func TestStep(t *testing.T) {
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "keith",
 				Percent:           99,
-				StartTime:         int(now.Unix()),
+				StartTime:         now,
 				Revisions: []RevisionRollout{{ // <-- note this one actually rolls.
 					RevisionName: "can't-get-no-satisfaction",
 					Percent:      98,
@@ -692,7 +688,7 @@ func TestStep(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			got := tc.cur.Step(tc.prev, clk)
+			got := tc.cur.Step(tc.prev, now)
 			if want := tc.want; !cmp.Equal(got, want, cmpopts.EquateEmpty()) {
 				t.Errorf("Wrong rolled rollout, diff(-want,+got):\n%s", cmp.Diff(want, got))
 			}
@@ -703,6 +699,40 @@ func TestStep(t *testing.T) {
 	}
 }
 
+func TestObserveReady(t *testing.T) {
+	const (
+		now       = 200620092020 + 1982
+		oldenDays = 198219841988
+	)
+	ro := Rollout{
+		Configurations: []ConfigurationRollout{{
+			ConfigurationName: "has-step",
+			StepDuration:      11,
+		}, {
+			ConfigurationName: "no-step-no-begin",
+		}, {
+			ConfigurationName: "step-begin < 1s",
+			StartTime:         200620092020,
+		}, {
+			ConfigurationName: "step-begin > 1s",
+			StartTime:         oldenDays,
+		}},
+	}
+
+	want := ro
+	want.Configurations[2].StepDuration = 1
+	want.Configurations[3].StepDuration = 3 // 2.4 rounded up.
+
+	// This works in place.
+	ro.ObserveReady(now)
+
+	if !cmp.Equal(ro, want) {
+		t.Errorf("ObserveReady generated mismatched config: diff(-want,+got):\n%s",
+			cmp.Diff(want, ro))
+	}
+
+}
+
 func TestAdjustPercentage(t *testing.T) {
 	tests := []struct {
 		name string
diff --git a/vendor/knative.dev/pkg/test/mako/sidecar.go b/vendor/knative.dev/pkg/test/mako/sidecar.go
index ad1531f34..ff890f624 100644
--- a/vendor/knative.dev/pkg/test/mako/sidecar.go
+++ b/vendor/knative.dev/pkg/test/mako/sidecar.go
@@ -93,7 +93,7 @@ func SetupHelper(ctx context.Context, benchmarkKey *string, benchmarkName *strin
 	// Get the commit of the benchmarks
 	commitID, err := changeset.Get()
 	if err != nil {
-		return nil, err
+		log.Println("Cannot find commit ID")
 	}
 
 	// Setup a deployment informer, so that we can use the lister to track