Repository: knative/serving
Pull request #10202 (2020-11-25 23:22:44)

Add starttime and done.
- start time is required in the beginning to compute step time
- done is required to ignore the rollouts that are completed during
  reconciliation.

/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/10202#" 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/10202?src=pr&el=h1) Report
> Merging [#10202](https://codecov.io/gh/knative/serving/pull/10202?src=pr&el=desc) (f136778) into [master](https://codecov.io/gh/knative/serving/commit/280e3a550ce33893adb5969132ec1a76d79c7681?el=desc) (280e3a5) will **increase** coverage by `0.07%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10202      +/-   ##
==========================================
+ Coverage   88.09%   88.17%   +0.07%     
==========================================
  Files         184      184              
  Lines        8611     8615       +4     
==========================================
+ Hits         7586     7596      +10     
+ Misses        789      785       -4     
+ Partials      236      234       -2     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10202?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/traffic/rollout.go](https://codecov.io/gh/knative/serving/pull/10202/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy9yb2xsb3V0Lmdv) | `100.00% <100.00%> (ø)` | |
| [pkg/autoscaler/statforwarder/forwarder.go](https://codecov.io/gh/knative/serving/pull/10202/diff?src=pr&el=tree#diff-cGtnL2F1dG9zY2FsZXIvc3RhdGZvcndhcmRlci9mb3J3YXJkZXIuZ28=) | `82.44% <0.00%> (+1.06%)` | :arrow_up: |
| [pkg/reconciler/autoscaling/kpa/scaler.go](https://codecov.io/gh/knative/serving/pull/10202/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvYXV0b3NjYWxpbmcva3BhL3NjYWxlci5nbw==) | `90.00% <0.00%> (+1.42%)` | :arrow_up: |
| [pkg/autoscaler/scaling/multiscaler.go](https://codecov.io/gh/knative/serving/pull/10202/diff?src=pr&el=tree#diff-cGtnL2F1dG9zY2FsZXIvc2NhbGluZy9tdWx0aXNjYWxlci5nbw==) | `89.65% <0.00%> (+1.72%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/10202?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/10202?src=pr&el=footer). Last update [280e3a5...dec840f](https://codecov.io/gh/knative/serving/pull/10202?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/10202#pullrequestreview-538938429" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10202#" 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/traffic/rollout.go b/pkg/reconciler/route/traffic/rollout.go
index 02eb3df9c..1c790481e 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -60,6 +60,11 @@ type ConfigurationRollout struct {
 	// the Rollout shoud be complete.
 	Deadline int `json:"deadline,omitempty"`
 
+	// StartTime is the Unix timestamp by when (+/- reconcile precision)
+	// the Rollout has started.
+	// This is required to compute step time and deadline.
+	StartTime int `json:"starttime,omitempty"`
+
 	// LastStepTimeStamp is the Unix timestamp when the last
 	// rollout step was performed.
 	LastStepTime int `json:"lastStep,omitempty"`
@@ -80,6 +85,13 @@ type RevisionRollout struct {
 	Percent int `json:"percent"`
 }
 
+// Done returns true if there is no active rollout going on
+// for the configuration.
+func (cr *ConfigurationRollout) Done() bool {
+	// Zero or just one revision.
+	return len(cr.Revisions) < 2
+}
+
 // Validate validates current rollout for inconsistencies.
 // This is expected to be invoked after annotation deserialization.
 // If it returns false — the deserialized object should be discarded.
@@ -185,6 +197,8 @@ func (cur *Rollout) Step(prev *Rollout) *Rollout {
 // the older revisions.
 func adjustPercentage(goal int, cr *ConfigurationRollout) {
 	switch diff := goal - cr.Percent; {
+	case goal == 0:
+		cr.Revisions = nil // No traffic, no rollout.
 	case diff > 0:
 		cr.Revisions[len(cr.Revisions)-1].Percent += diff
 	case diff < 0:
@@ -235,6 +249,7 @@ func stepConfig(goal, prev *ConfigurationRollout) *ConfigurationRollout {
 			ret.Deadline = prev.Deadline
 			ret.LastStepTime = prev.LastStepTime
 			ret.StepDuration = prev.StepDuration
+			ret.StartTime = prev.StartTime
 		}
 		return ret
 	}


Test patch:
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index 5ae626f63..a76e9e774 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -25,6 +25,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 )
 
 func TestStep(t *testing.T) {
@@ -77,6 +78,7 @@ func TestStep(t *testing.T) {
 				Deadline:     2006, // <- Those should be ignored.
 				LastStepTime: 2009,
 				StepDuration: 2020,
+				StartTime:    2004,
 			}},
 		},
 		prev: &Rollout{
@@ -90,6 +92,7 @@ func TestStep(t *testing.T) {
 				Deadline:     1982, // <- Those should be copied.
 				LastStepTime: 1984,
 				StepDuration: 1988,
+				StartTime:    1955,
 			}},
 		},
 		want: &Rollout{
@@ -103,6 +106,7 @@ func TestStep(t *testing.T) {
 				Deadline:     1982,
 				LastStepTime: 1984,
 				StepDuration: 1988,
+				StartTime:    1955,
 			}},
 		},
 	}, {
@@ -673,7 +677,7 @@ func TestStep(t *testing.T) {
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			got := tc.cur.Step(tc.prev)
-			if want := tc.want; !cmp.Equal(got, want) {
+			if want := tc.want; !cmp.Equal(got, want, cmpopts.EquateEmpty()) {
 				t.Errorf("Wrong rolled rollout, diff(-want,+got):\n%s", cmp.Diff(want, got))
 			}
 			if !got.Validate() {
@@ -798,12 +802,11 @@ func TestAdjustPercentage(t *testing.T) {
 				Percent: 60,
 			}},
 		},
-		want: []RevisionRollout{},
 	}}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			adjustPercentage(tc.goal, tc.prev)
-			if got, want := tc.prev.Revisions, tc.want; !cmp.Equal(got, want) {
+			if got, want := tc.prev.Revisions, tc.want; !cmp.Equal(got, want, cmpopts.EquateEmpty()) {
 				t.Errorf("Rollout Mistmatch(-want,+got):\n%s", cmp.Diff(want, got))
 			}
 		})
@@ -884,3 +887,39 @@ func TestValidateFailures(t *testing.T) {
 	}
 
 }
+
+func TestConfigDone(t *testing.T) {
+	r := &Rollout{
+		Configurations: []ConfigurationRollout{{
+			ConfigurationName: "one",
+			Percent:           100,
+			Revisions: []RevisionRollout{{
+				RevisionName: "roy",
+				Percent:      100,
+			}},
+		}, {
+			ConfigurationName: "no",
+			Percent:           0,
+			Revisions:         []RevisionRollout{},
+		}, {
+			ConfigurationName: "many",
+			Percent:           100,
+			Revisions: []RevisionRollout{{
+				RevisionName: "black-on-blue",
+				Percent:      83,
+			}, {
+				RevisionName: "flowers",
+				Percent:      17,
+			}},
+		}},
+	}
+	if !r.Configurations[0].Done() {
+		t.Error("Single revision rollout is not `Done`")
+	}
+	if !r.Configurations[1].Done() {
+		t.Error("Zero revisions rollout is not `Done`")
+	}
+	if r.Configurations[2].Done() {
+		t.Error("Many revisions rollout is `Done`")
+	}
+}