Repository: knative/serving
Pull request #10229 (2020-11-30 21:57:40)

Stamp the start time when the rollout starts
We need to stamp the rollout with when it started, so when the ingress becomes ready,
we can set the rollout step time.

/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/10229#" 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":[]} -->
knative-prow-robot: @vagababov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-unit-tests | d6853997fb75a5cdbd0984362f927c0cfc9a1f38 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10229/pull-knative-serving-unit-tests/1333530689630375936) | `/test pull-knative-serving-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10229). [Your PR dashboard](https://gubernator.knative.dev/pr/vagababov).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-prow-robot: @vagababov: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-unit-tests | d6853997fb75a5cdbd0984362f927c0cfc9a1f38 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10229/pull-knative-serving-unit-tests/1333530689630375936) | `/test pull-knative-serving-unit-tests`
pull-knative-serving-build-tests | d6853997fb75a5cdbd0984362f927c0cfc9a1f38 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10229/pull-knative-serving-build-tests/1333530689605210112) | `/test pull-knative-serving-build-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10229). [Your PR dashboard](https://gubernator.knative.dev/pr/vagababov).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-prow-robot: @vagababov: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-unit-tests | d6853997fb75a5cdbd0984362f927c0cfc9a1f38 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10229/pull-knative-serving-unit-tests/1333530689630375936) | `/test pull-knative-serving-unit-tests`
pull-knative-serving-build-tests | d6853997fb75a5cdbd0984362f927c0cfc9a1f38 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10229/pull-knative-serving-build-tests/1333530689605210112) | `/test pull-knative-serving-build-tests`
pull-knative-serving-istio-stable-no-mesh-tls | d6853997fb75a5cdbd0984362f927c0cfc9a1f38 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10229/pull-knative-serving-istio-stable-no-mesh-tls/1333530689710067712) | `/test pull-knative-serving-istio-stable-no-mesh-tls`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10229). [Your PR dashboard](https://gubernator.knative.dev/pr/vagababov).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-prow-robot: @vagababov: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-unit-tests | d6853997fb75a5cdbd0984362f927c0cfc9a1f38 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10229/pull-knative-serving-unit-tests/1333530689630375936) | `/test pull-knative-serving-unit-tests`
pull-knative-serving-build-tests | d6853997fb75a5cdbd0984362f927c0cfc9a1f38 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10229/pull-knative-serving-build-tests/1333530689605210112) | `/test pull-knative-serving-build-tests`
pull-knative-serving-istio-stable-no-mesh-tls | d6853997fb75a5cdbd0984362f927c0cfc9a1f38 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10229/pull-knative-serving-istio-stable-no-mesh-tls/1333530689710067712) | `/test pull-knative-serving-istio-stable-no-mesh-tls`
pull-knative-serving-istio-stable-no-mesh | d6853997fb75a5cdbd0984362f927c0cfc9a1f38 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10229/pull-knative-serving-istio-stable-no-mesh/1333530689672318976) | `/test pull-knative-serving-istio-stable-no-mesh`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10229). [Your PR dashboard](https://gubernator.knative.dev/pr/vagababov).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10229?src=pr&el=h1) Report
> Merging [#10229](https://codecov.io/gh/knative/serving/pull/10229?src=pr&el=desc) (c691537) into [master](https://codecov.io/gh/knative/serving/commit/326822dbaf304dc28dd7b0ce007e0d11986ce9b0?el=desc) (326822d) will **increase** coverage by `0.01%`.
> The diff coverage is `37.50%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10229      +/-   ##
==========================================
+ Coverage   88.10%   88.11%   +0.01%     
==========================================
  Files         184      184              
  Lines        8606     8607       +1     
==========================================
+ Hits         7582     7584       +2     
  Misses        787      787              
+ Partials      237      236       -1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10229?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [cmd/activator/main.go](https://codecov.io/gh/knative/serving/pull/10229/diff?src=pr&el=tree#diff-Y21kL2FjdGl2YXRvci9tYWluLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/activator/net/throttler.go](https://codecov.io/gh/knative/serving/pull/10229/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9uZXQvdGhyb3R0bGVyLmdv) | `88.29% <0.00%> (ø)` | |
| [pkg/reconciler/route/reconcile\_resources.go](https://codecov.io/gh/knative/serving/pull/10229/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcmVjb25jaWxlX3Jlc291cmNlcy5nbw==) | `86.00% <100.00%> (ø)` | |
| [pkg/reconciler/route/traffic/rollout.go](https://codecov.io/gh/knative/serving/pull/10229/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy9yb2xsb3V0Lmdv) | `100.00% <100.00%> (ø)` | |
| [pkg/reconciler/revision/reconcile\_resources.go](https://codecov.io/gh/knative/serving/pull/10229/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcmV2aXNpb24vcmVjb25jaWxlX3Jlc291cmNlcy5nbw==) | `80.72% <0.00%> (-2.41%)` | :arrow_down: |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/10229/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9jb25maWd1cmF0aW9uLmdv) | `86.71% <0.00%> (+2.34%)` | :arrow_up: |

------

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

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10229#pullrequestreview-541364399" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10229#" 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)~~ [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":[]} -->

Patch:
diff --git a/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go
index 059158e6d..f2fd17e1e 100644
--- a/pkg/reconciler/route/reconcile_resources.go
+++ b/pkg/reconciler/route/reconcile_resources.go
@@ -75,7 +75,7 @@ func (c *Reconciler) reconcileIngress(
 			ingress.Annotations[networking.RolloutAnnotationKey])
 
 		// And recompute the rollout state.
-		effectiveRO := curRO.Step(prevRO)
+		effectiveRO := curRO.Step(prevRO, c.clock)
 		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 1c790481e..94e7e36f8 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -23,6 +23,8 @@ package traffic
 
 import (
 	"sort"
+
+	"knative.dev/pkg/system"
 )
 
 // Rollout encapsulates the current rollout state of the system.
@@ -118,7 +120,7 @@ func (cur *Rollout) Validate() bool {
 // 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) *Rollout {
+func (cur *Rollout) Step(prev *Rollout, clk system.Clock) *Rollout {
 	if prev == nil || len(prev.Configurations) == 0 {
 		return cur
 	}
@@ -161,11 +163,12 @@ func (cur *Rollout) Step(prev *Rollout) *Rollout {
 				// altogether.
 				switch p := ccfgs[i].Percent; {
 				case p > 1:
-					ret = append(ret, *stepConfig(ccfgs[i], pcfgs[j]))
+					ret = append(ret, *stepConfig(ccfgs[i], pcfgs[j], clk))
 				case p == 1:
 					// Skip all the work if it's a common A/B scenario where the test config
 					// receives just 1% of traffic.
 					ret = append(ret, *ccfgs[i])
+					// default p == 0 => just ignore for rollout.
 				}
 				i++
 				j++
@@ -220,7 +223,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) *ConfigurationRollout {
+func stepConfig(goal, prev *ConfigurationRollout, clk system.Clock) *ConfigurationRollout {
 	pc := len(prev.Revisions)
 	ret := &ConfigurationRollout{
 		ConfigurationName: goal.ConfigurationName,
@@ -254,6 +257,9 @@ func stepConfig(goal, prev *ConfigurationRollout) *ConfigurationRollout {
 		return ret
 	}
 
+	// Otherwise we start a rollout, which means we need to stamp the starttime.
+	ret.StartTime = int(clk.Now().Unix())
+
 	// Go backwards and find first revision with traffic assignment > 0.
 	// Reduce it by one, so we can give that 1% to the new revision.
 	// By design we drain newest revision first.


Test patch:
diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go
index 0afc597e1..7c2eec26d 100644
--- a/pkg/reconciler/route/table_test.go
+++ b/pkg/reconciler/route/table_test.go
@@ -717,7 +717,7 @@ func TestReconcile(t *testing.T) {
 					RevisionName: "config-00001", Percent: 99,
 				}, {
 					RevisionName: "config-00002", Percent: 1,
-				}})),
+				}}, fakeCurTime)),
 		}},
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: Route("default", "new-latest-ready", WithConfigTarget("config"),
@@ -921,7 +921,7 @@ func TestReconcile(t *testing.T) {
 					RevisionName: "config-00001", Percent: 99,
 				}, {
 					RevisionName: "config-00002", Percent: 1,
-				}})),
+				}}, fakeCurTime)),
 		}},
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: Route("default", "update-ci-failure", WithConfigTarget("config"),
@@ -2603,11 +2603,12 @@ func url(s string) *apis.URL {
 	return url
 }
 
-func simpleRollout(cfg string, revs []traffic.RevisionRollout) IngressOption {
+func simpleRollout(cfg string, revs []traffic.RevisionRollout, now time.Time) IngressOption {
 	return func(i *netv1alpha1.Ingress) {
 		r := &traffic.Rollout{
 			Configurations: []traffic.ConfigurationRollout{{
 				ConfigurationName: cfg,
+				StartTime:         int(now.Unix()),
 				Percent:           100,
 				Revisions:         revs,
 			}},
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index 447e97aad..6a6e12fef 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -24,12 +24,18 @@ package traffic
 import (
 	"encoding/json"
 	"testing"
+	"time"
 
 	"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,
+	}
 	tests := []struct {
 		name            string
 		prev, cur, want *Rollout
@@ -201,6 +207,7 @@ func TestStep(t *testing.T) {
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "mick",
 				Percent:           100,
+				StartTime:         int(now.Unix()),
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
 					Percent:      99,
@@ -239,6 +246,7 @@ func TestStep(t *testing.T) {
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "mick",
 				Percent:           33,
+				StartTime:         int(now.Unix()),
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
 					Percent:      2,
@@ -279,6 +287,7 @@ func TestStep(t *testing.T) {
 		want: &Rollout{
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "mick",
+				StartTime:         int(now.Unix()),
 				Percent:           75,
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
@@ -318,6 +327,7 @@ func TestStep(t *testing.T) {
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "brian",
 				Percent:           70,
+				StartTime:         int(now.Unix()),
 				Revisions: []RevisionRollout{{
 					RevisionName: "exile-on-main-st",
 					Percent:      69,
@@ -343,6 +353,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.
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
 					Percent:      95,
@@ -356,6 +367,7 @@ func TestStep(t *testing.T) {
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "mick",
 				Percent:           100,
+				StartTime:         int(now.Unix()),
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
 					Percent:      95,
@@ -384,6 +396,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.
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
 					Percent:      99,
@@ -396,6 +409,7 @@ func TestStep(t *testing.T) {
 		want: &Rollout{
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "mick",
+				StartTime:         int(now.Unix()),
 				Percent:           100,
 				Revisions: []RevisionRollout{{
 					RevisionName: "goat-head-soup",
@@ -565,6 +579,7 @@ func TestStep(t *testing.T) {
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "keith",
 				Percent:           99,
+				StartTime:         int(now.Unix()),
 				Revisions: []RevisionRollout{{ // <-- note this one actually rolls.
 					RevisionName: "can't-get-no-satisfaction",
 					Percent:      98,
@@ -677,7 +692,7 @@ func TestStep(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			got := tc.cur.Step(tc.prev)
+			got := tc.cur.Step(tc.prev, clk)
 			if want := tc.want; !cmp.Equal(got, want, cmpopts.EquateEmpty()) {
 				t.Errorf("Wrong rolled rollout, diff(-want,+got):\n%s", cmp.Diff(want, got))
 			}