Repository: knative/serving
Pull request #10059 (2020-11-06 23:48:10)

Fix some bugs and raise coverage for the rollout code
- cover the cases when prev had no revisions (should not happen, but
might, if there's a bug, so defensive coding)
- add test for this
- cover 0% cases uniformly
- add tests for the 0% cases.

For #9766

/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/10059#" 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: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10059#pullrequestreview-525560310" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10059#" 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":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10059?src=pr&el=h1) Report
> Merging [#10059](https://codecov.io/gh/knative/serving/pull/10059?src=pr&el=desc) (980131f) into [master](https://codecov.io/gh/knative/serving/commit/861357753161f70a68615fcf1c40e485a8cdedec?el=desc) (8613577) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

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

```diff
@@           Coverage Diff           @@
##           master   #10059   +/-   ##
=======================================
  Coverage   87.96%   87.96%           
=======================================
  Files         185      185           
  Lines        8683     8684    +1     
=======================================
+ Hits         7638     7639    +1     
- Misses        795      796    +1     
+ Partials      250      249    -1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10059?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/traffic/rollout.go](https://codecov.io/gh/knative/serving/pull/10059/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy9yb2xsb3V0Lmdv) | `100.00% <100.00%> (ø)` | |
| [pkg/activator/net/revision\_backends.go](https://codecov.io/gh/knative/serving/pull/10059/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9uZXQvcmV2aXNpb25fYmFja2VuZHMuZ28=) | `90.49% <0.00%> (-0.91%)` | :arrow_down: |
| [pkg/autoscaler/statforwarder/forwarder.go](https://codecov.io/gh/knative/serving/pull/10059/diff?src=pr&el=tree#diff-cGtnL2F1dG9zY2FsZXIvc3RhdGZvcndhcmRlci9mb3J3YXJkZXIuZ28=) | `82.25% <0.00%> (+1.07%)` | :arrow_up: |

------

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


Patch:
diff --git a/pkg/reconciler/route/traffic/rollout.go b/pkg/reconciler/route/traffic/rollout.go
index 744464490..61ffb5256 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -21,7 +21,9 @@ limitations under the License.
 
 package traffic
 
-import "sort"
+import (
+	"sort"
+)
 
 // Rollout encapsulates the current rollout state of the system.
 // Since the route might reference more than one configuration.
@@ -112,7 +114,8 @@ func (cur *Rollout) Step(prev *Rollout) *Rollout {
 				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).
+				// receives no traffic via default tag). So just skip it from the rollout
+				// altogether.
 				if ccfgs[i].Percent != 0 {
 					ret = append(ret, *stepConfig(ccfgs[i], pcfgs[j]))
 				}
@@ -120,7 +123,10 @@ func (cur *Rollout) Step(prev *Rollout) *Rollout {
 				j++
 			case ccfgs[i].ConfigurationName < pcfgs[j].ConfigurationName:
 				// A new config, has been added. No action for rollout though.
-				ret = append(ret, *ccfgs[i])
+				// Keep it if it will receive traffic.
+				if ccfgs[i].Percent != 0 {
+					ret = append(ret, *ccfgs[i])
+				}
 				i++
 			default: // cur > prev.
 				// A config has been removed during this update.
@@ -147,7 +153,9 @@ func stepConfig(goal, prev *ConfigurationRollout) *ConfigurationRollout {
 		Percent:           goal.Percent,
 	}
 	// goal will always have just one revision in the list – the current desired revision.
-	if goal.Revisions[0].RevisionName == prev.Revisions[pc-1].RevisionName {
+	// If it matches the last revision of the previous rollout state (or there were no revisions)
+	// then no new rollout hast 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.
 		// TODO(vagababov): percentage might change, so this should trigger recompute of existing


Test patch:
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index 929a304d0..6e4a0b5f5 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -27,7 +27,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 )
 
-func TestRoll(t *testing.T) {
+func TestStep(t *testing.T) {
 	tests := []struct {
 		name            string
 		prev, cur, want *Rollout
@@ -36,6 +36,34 @@ func TestRoll(t *testing.T) {
 		cur:  &Rollout{},
 		prev: nil,
 		want: &Rollout{},
+	}, {
+		name: "prev is empty",
+		cur: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "mick",
+				Percent:           100,
+				Revisions: []RevisionRollout{{
+					RevisionName: "let-it-bleed",
+					Percent:      100,
+				}},
+			}},
+		},
+		prev: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "mick",
+				Percent:           100,
+			}},
+		},
+		want: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "mick",
+				Percent:           100,
+				Revisions: []RevisionRollout{{
+					RevisionName: "let-it-bleed",
+					Percent:      100,
+				}},
+			}},
+		},
 	}, {
 		name: "simplest, same",
 		cur: &Rollout{
@@ -68,6 +96,68 @@ func TestRoll(t *testing.T) {
 				}},
 			}},
 		},
+	}, {
+		name: "when new revision becomes 0%",
+		cur: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "charlie",
+				Percent:           0,
+				Revisions: []RevisionRollout{{
+					RevisionName: "aftermath",
+					Percent:      0,
+				}},
+			}},
+		},
+		prev: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "charlie",
+				Percent:           100,
+				Revisions: []RevisionRollout{{
+					RevisionName: "your-satanic-majesties-request",
+					Percent:      100,
+				}},
+			}},
+		},
+		want: &Rollout{},
+	}, {
+		name: "when new config is added but it's 0%",
+		cur: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "charlie",
+				Percent:           0,
+				Revisions: []RevisionRollout{{
+					RevisionName: "aftermath",
+					Percent:      0,
+				}},
+			}, {
+				ConfigurationName: "mick",
+				Percent:           100,
+				Revisions: []RevisionRollout{{
+					RevisionName: "between-the-buttons",
+					Percent:      100,
+				}},
+			}},
+		},
+		prev: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "mick",
+				Percent:           100,
+				Revisions: []RevisionRollout{{
+					RevisionName: "between-the-buttons",
+					Percent:      100,
+				}},
+			}},
+		},
+		want: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "mick",
+				Percent:           100,
+				Revisions: []RevisionRollout{{
+					RevisionName: "between-the-buttons",
+					Percent:      100,
+				}},
+			}},
+		},
 	}, {
 		name: "simplest, roll",
 		cur: &Rollout{