Repository: knative/serving
Pull request #9996 (2020-10-30 20:33:12)

Raise coverage and fix a bug
This edgecase wasn't covered after refactoring
and well enough, it had a bug 😳😳😳

/assign @tcnghia mattmoor

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

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

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

```diff
@@            Coverage Diff             @@
##           master    #9996      +/-   ##
==========================================
+ Coverage   87.84%   87.87%   +0.02%     
==========================================
  Files         184      184              
  Lines        8680     8681       +1     
==========================================
+ Hits         7625     7628       +3     
+ Misses        802      801       -1     
+ Partials      253      252       -1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/9996?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/traffic/rollout.go](https://codecov.io/gh/knative/serving/pull/9996/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy9yb2xsb3V0Lmdv) | `100.00% <100.00%> (+4.08%)` | :arrow_up: |
| [pkg/activator/net/revision\_backends.go](https://codecov.io/gh/knative/serving/pull/9996/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9uZXQvcmV2aXNpb25fYmFja2VuZHMuZ28=) | `90.49% <0.00%> (-0.91%)` | :arrow_down: |
| [pkg/reconciler/autoscaling/kpa/scaler.go](https://codecov.io/gh/knative/serving/pull/9996/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvYXV0b3NjYWxpbmcva3BhL3NjYWxlci5nbw==) | `90.00% <0.00%> (+1.42%)` | :arrow_up: |

------

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

tcnghia: /lgtm

Patch:
diff --git a/pkg/reconciler/route/traffic/rollout.go b/pkg/reconciler/route/traffic/rollout.go
index 460c392fe..ac123ec10 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -111,12 +111,13 @@ func (cur *Rollout) Step(prev *Rollout) *Rollout {
 		}
 		// This is basically an intersect algorithm,
 		// It relies on the fact that inputs are sorted.
-		for i, j := 0, 0; i < len(ccfgs) && j < len(pcfgs); {
+		for i, j := 0, 0; i < len(ccfgs); {
 			switch {
 			case j >= len(pcfgs):
 				// Those are the new configs that were added during this reconciliation.
 				// So we just copy them to the result.
 				ret = append(ret, *ccfgs[i])
+				i++
 			case ccfgs[i].ConfigurationName == pcfgs[j].ConfigurationName:
 				// Config might have 0% traffic assigned, if it is a tag only route (i.e.
 				// receives no traffic via default tag).


Test patch:
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index d9bd0281f..6faed9879 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -270,7 +270,7 @@ func TestRoll(t *testing.T) {
 			}},
 		},
 	}, {
-		name: "new config, no roll",
+		name: "new config, no roll, newer smaller",
 		cur: &Rollout{
 			Configurations: []ConfigurationRollout{{
 				ConfigurationName: "keith",
@@ -315,6 +315,52 @@ func TestRoll(t *testing.T) {
 				}},
 			}},
 		},
+	}, {
+		name: "new config, no roll, newer larger",
+		cur: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "keith",
+				Percent:           50,
+				Revisions: []RevisionRollout{{
+					RevisionName: "black-on-blue",
+					Percent:      50,
+				}},
+			}, {
+				ConfigurationName: "mick",
+				Percent:           50,
+				Revisions: []RevisionRollout{{
+					RevisionName: "it's-only-rock-n-roll",
+					Percent:      50,
+				}},
+			}},
+		},
+		prev: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "keith",
+				Percent:           100,
+				Revisions: []RevisionRollout{{
+					RevisionName: "black-on-blue",
+					Percent:      100,
+				}},
+			}},
+		},
+		want: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "keith",
+				Percent:           50,
+				Revisions: []RevisionRollout{{
+					RevisionName: "black-on-blue",
+					Percent:      50,
+				}},
+			}, {
+				ConfigurationName: "mick",
+				Percent:           50,
+				Revisions: []RevisionRollout{{
+					RevisionName: "it's-only-rock-n-roll",
+					Percent:      50,
+				}},
+			}},
+		},
 	}}
 
 	for _, tc := range tests {