Repository: knative/serving
Pull request #10304 (2020-12-08 04:24:08)

Stop rolling when we're done with the config rollout
Ensure we reset next step time to 0, so nothing more happens to this rollout
The same with the starttime.

/assign @tcnghia 

For #9766

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10304#" 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/10304?src=pr&el=h1) Report
> Merging [#10304](https://codecov.io/gh/knative/serving/pull/10304?src=pr&el=desc) (72582e4) into [master](https://codecov.io/gh/knative/serving/commit/ed56e2f7c2556535abe41ed0e41b277692167194?el=desc) (ed56e2f) will **increase** coverage by `0.05%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10304      +/-   ##
==========================================
+ Coverage   88.19%   88.24%   +0.05%     
==========================================
  Files         184      184              
  Lines        8650     8654       +4     
==========================================
+ Hits         7629     7637       +8     
+ Misses        786      783       -3     
+ Partials      235      234       -1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10304?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/traffic/rollout.go](https://codecov.io/gh/knative/serving/pull/10304/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy9yb2xsb3V0Lmdv) | `100.00% <100.00%> (ø)` | |
| [pkg/activator/net/revision\_backends.go](https://codecov.io/gh/knative/serving/pull/10304/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9uZXQvcmV2aXNpb25fYmFja2VuZHMuZ28=) | `91.40% <0.00%> (+0.90%)` | :arrow_up: |
| [pkg/autoscaler/statforwarder/forwarder.go](https://codecov.io/gh/knative/serving/pull/10304/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/10304?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/10304?src=pr&el=footer). Last update [ed56e2f...3d3fd34](https://codecov.io/gh/knative/serving/pull/10304?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/10304#pullrequestreview-546741822" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10304#" 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":[]} -->
knative-prow-robot: @vagababov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-istio-stable-no-mesh | 3d3fd34b9bf698da6f64769e3363f4c30c9eefb1 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10304/pull-knative-serving-istio-stable-no-mesh/1336164662131036160) | `/test pull-knative-serving-istio-stable-no-mesh`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10304). [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 -->
vagababov: /test pull-knative-serving-istio-stable-no-mesh

Patch:
diff --git a/pkg/reconciler/route/traffic/rollout.go b/pkg/reconciler/route/traffic/rollout.go
index 9ded9dced..fc587484f 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -293,7 +293,13 @@ func stepRevisions(goal *ConfigurationRollout, nowTS int) {
 	// And cull the tail portion of it.
 	goal.Revisions = goal.Revisions[:writePos+1]
 	// Also set the next time.
-	goal.NextStepTime = nowTS + goal.StepDuration
+	if len(goal.Revisions) > 1 {
+		goal.NextStepTime = nowTS + goal.StepDuration
+	} else {
+		// This is the last step, we're done!
+		goal.NextStepTime = 0
+		goal.StartTime = 0
+	}
 }
 
 // stepConfig takes previous and goal configuration shapes and returns a new


Test patch:
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index 038951d75..a5124af1c 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -1090,6 +1090,7 @@ func TestStepRevisions(t *testing.T) {
 		now:  1982,
 		cfg: &ConfigurationRollout{
 			NextStepTime: 1984,
+			StepDuration: 10,
 			StepSize:     10,
 			Revisions: []RevisionRollout{{
 				Percent: 10,
@@ -1097,6 +1098,7 @@ func TestStepRevisions(t *testing.T) {
 		},
 		want: &ConfigurationRollout{
 			NextStepTime: 1984,
+			StepDuration: 10,
 			StepSize:     10,
 			Revisions: []RevisionRollout{{
 				Percent: 10,
@@ -1176,13 +1178,42 @@ func TestStepRevisions(t *testing.T) {
 		},
 		want: &ConfigurationRollout{
 			Percent:      15,
-			NextStepTime: 1988 + 77,
+			NextStepTime: 0,
 			StepDuration: 77,
 			StepSize:     10,
 			Revisions: []RevisionRollout{{
 				Percent: 15,
 			}},
 		},
+	}, {
+		name: "the very last step",
+		now:  2006,
+		cfg: &ConfigurationRollout{
+			NextStepTime: 1984,
+			Percent:      15,
+			StartTime:    1977,
+			StepDuration: 77,
+			StepSize:     8,
+			Revisions: []RevisionRollout{{
+				Percent: 5,
+			}, {
+				Percent: 1,
+			}, {
+				Percent: 1,
+			}, {
+				Percent: 8,
+			}},
+		},
+		want: &ConfigurationRollout{
+			NextStepTime: 0,
+			Percent:      15,
+			StartTime:    0,
+			StepDuration: 77,
+			StepSize:     8,
+			Revisions: []RevisionRollout{{
+				Percent: 15,
+			}},
+		},
 	}}
 
 	for _, tc := range tests {