Repository: knative/serving
Pull request #10180 (2020-11-24 07:32:07)

Copy the  values from the previous rollout state to the new state.
We should clear the values when a new revision is created, but
retain the previous values from the prev (or, in future PR calculate
the new percentage values and new time for the next step).

/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/10180#" 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/10180?src=pr&el=h1) Report
> Merging [#10180](https://codecov.io/gh/knative/serving/pull/10180?src=pr&el=desc) (e382dcf) into [master](https://codecov.io/gh/knative/serving/commit/f19b585860675d05218338533749899fca517711?el=desc) (f19b585) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10180      +/-   ##
==========================================
+ Coverage   88.04%   88.06%   +0.02%     
==========================================
  Files         184      184              
  Lines        8621     8624       +3     
==========================================
+ Hits         7590     7595       +5     
+ Misses        789      787       -2     
  Partials      242      242              
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10180?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/traffic/rollout.go](https://codecov.io/gh/knative/serving/pull/10180/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy9yb2xsb3V0Lmdv) | `100.00% <100.00%> (ø)` | |
| [pkg/reconciler/autoscaling/kpa/scaler.go](https://codecov.io/gh/knative/serving/pull/10180/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvYXV0b3NjYWxpbmcva3BhL3NjYWxlci5nbw==) | `88.57% <0.00%> (-1.43%)` | :arrow_down: |
| [pkg/activator/net/revision\_backends.go](https://codecov.io/gh/knative/serving/pull/10180/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9uZXQvcmV2aXNpb25fYmFja2VuZHMuZ28=) | `91.40% <0.00%> (+0.90%)` | :arrow_up: |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/10180/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9jb25maWd1cmF0aW9uLmdv) | `88.28% <0.00%> (+1.56%)` | :arrow_up: |

------

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

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-tls | 7d34f627849ee0c7a33e1122893104682f7073de | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10180/pull-knative-serving-istio-stable-no-mesh-tls/1331138538409824256) | `/test pull-knative-serving-istio-stable-no-mesh-tls`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10180). [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 -->
tcnghia: /test pull-knative-serving-istio-stable-no-mesh-tls
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-tls | 7d34f627849ee0c7a33e1122893104682f7073de | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10180/pull-knative-serving-istio-stable-no-mesh-tls/1331184224551571456) | `/test pull-knative-serving-istio-stable-no-mesh-tls`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10180). [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-tls
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10180#pullrequestreview-537775389" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10180#" 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 tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-istio-stable-no-mesh-tls | 7d34f627849ee0c7a33e1122893104682f7073de | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10180/pull-knative-serving-istio-stable-no-mesh-tls/1331184224551571456) | `/test pull-knative-serving-istio-stable-no-mesh-tls`
pull-knative-serving-unit-tests | 752a9500154388e513f61d4691e319b82b7a444f | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10180/pull-knative-serving-unit-tests/1331297158309089280) | `/test pull-knative-serving-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10180). [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: New changes are detected. LGTM label has been removed.
tcnghia: /lgtm

Patch:
diff --git a/pkg/reconciler/route/traffic/rollout.go b/pkg/reconciler/route/traffic/rollout.go
index 2d9450c99..02eb3df9c 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -213,6 +213,9 @@ func stepConfig(goal, prev *ConfigurationRollout) *ConfigurationRollout {
 		Tag:               goal.Tag,
 		Percent:           goal.Percent,
 		Revisions:         goal.Revisions,
+
+		// If there is a new revision, then timing information should be reset.
+		// So leave them empty here and populate below, if necessary.
 	}
 
 	if len(prev.Revisions) > 0 {
@@ -222,10 +225,16 @@ func stepConfig(goal, prev *ConfigurationRollout) *ConfigurationRollout {
 	// If it matches the last revision of the previous rollout state (or there were no revisions)
 	// then no new rollout has begun for this configuration.
 	if len(prev.Revisions) == 0 || goal.Revisions[0].RevisionName == prev.Revisions[pc-1].RevisionName {
-		// TODO(vagababov): here would go the logic to compute new percentages for the rollout,
-		// i.e step function, so return value will change, depending on that.
 		if len(prev.Revisions) > 0 {
 			ret.Revisions = prev.Revisions
+
+			// TODO(vagababov): here would go the logic to compute new percentages for the rollout,
+			// i.e step function, so return value will change, depending on that.
+			// Copy the duration info from the previous when no new revision
+			// has been created.
+			ret.Deadline = prev.Deadline
+			ret.LastStepTime = prev.LastStepTime
+			ret.StepDuration = prev.StepDuration
 		}
 		return ret
 	}


Test patch:
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index 6a7e51d4e..5ae626f63 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -74,6 +74,9 @@ func TestStep(t *testing.T) {
 					RevisionName: "let-it-bleed",
 					Percent:      100,
 				}},
+				Deadline:     2006, // <- Those should be ignored.
+				LastStepTime: 2009,
+				StepDuration: 2020,
 			}},
 		},
 		prev: &Rollout{
@@ -84,6 +87,9 @@ func TestStep(t *testing.T) {
 					RevisionName: "let-it-bleed",
 					Percent:      100,
 				}},
+				Deadline:     1982, // <- Those should be copied.
+				LastStepTime: 1984,
+				StepDuration: 1988,
 			}},
 		},
 		want: &Rollout{
@@ -94,6 +100,9 @@ func TestStep(t *testing.T) {
 					RevisionName: "let-it-bleed",
 					Percent:      100,
 				}},
+				Deadline:     1982,
+				LastStepTime: 1984,
+				StepDuration: 1988,
 			}},
 		},
 	}, {
@@ -178,6 +187,9 @@ func TestStep(t *testing.T) {
 					RevisionName: "goat-head-soup",
 					Percent:      100,
 				}},
+				Deadline:     1982, // <- Those should be thrown out.
+				LastStepTime: 1984,
+				StepDuration: 1988,
 			}},
 		},
 		want: &Rollout{