Repository: knative/serving
Pull request #10238 (2020-12-01 21:53:53)

Move the 0% check to the build rollout, to filter things early on
There's no need to try to rollout things that will not receive any
traffic.
This is usually for tagged targets. Which only will receive traffic
directly to that tag (100%).
Also a few additional tests.

/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/10238#" 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: @vagababov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-unit-tests | d87b838066bd8d3a42bbf47e84fe42b1f72a557a | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10238/pull-knative-serving-unit-tests/1333892129071566848) | `/test pull-knative-serving-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10238). [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 -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10238?src=pr&el=h1) Report
> Merging [#10238](https://codecov.io/gh/knative/serving/pull/10238?src=pr&el=desc) (d87b838) into [master](https://codecov.io/gh/knative/serving/commit/4ee40b747748ba5747305fe2b8b1bd9d9bce06b1?el=desc) (4ee40b7) will **decrease** coverage by `0.01%`.
> The diff coverage is `50.00%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10238      +/-   ##
==========================================
- Coverage   88.14%   88.13%   -0.02%     
==========================================
  Files         184      184              
  Lines        8613     8614       +1     
==========================================
  Hits         7592     7592              
- Misses        785      786       +1     
  Partials      236      236              
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10238?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/traffic/rollout.go](https://codecov.io/gh/knative/serving/pull/10238/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy9yb2xsb3V0Lmdv) | `96.59% <0.00%> (-1.17%)` | :arrow_down: |
| [pkg/reconciler/route/traffic/traffic.go](https://codecov.io/gh/knative/serving/pull/10238/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy90cmFmZmljLmdv) | `94.21% <100.00%> (+0.06%)` | :arrow_up: |

------

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

vagababov: unrelated
/retest
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10238#pullrequestreview-542335619" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10238#" 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 | d87b838066bd8d3a42bbf47e84fe42b1f72a557a | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10238/pull-knative-serving-istio-stable-no-mesh/1333892129121898496) | `/test pull-knative-serving-istio-stable-no-mesh`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10238). [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 de61b2634..3616159af 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -117,12 +117,13 @@ func (cur *Rollout) Validate() bool {
 
 // ObserveReady traverses the configs and the ones that are in rollout
 // but have not observed step time yet, will have it set, to
-// max(1, nowTS-cfg.StartTime).
+// max(1, nowTS-cfg.StartTime). nowTS is unix timestamp in ns.
 func (cur *Rollout) ObserveReady(nowTS int) {
 	for i := range cur.Configurations {
 		c := &cur.Configurations[i]
 		if c.StepDuration == 0 && c.StartTime > 0 {
-			c.StepDuration = int(math.Max(1, math.Ceil(time.Duration(nowTS-c.StartTime).Seconds())))
+			c.StepDuration = int(math.Max(1,
+				math.Ceil(time.Duration(nowTS-c.StartTime).Seconds())))
 		}
 	}
 }
@@ -132,6 +133,7 @@ func (cur *Rollout) ObserveReady(nowTS int) {
 // At the end of the call the returned object will contain the
 // desired traffic shape.
 // Step will return cur if no previous state was available.
+// nowTS is unix timestamp in ns.
 func (cur *Rollout) Step(prev *Rollout, nowTS int) *Rollout {
 	if prev == nil || len(prev.Configurations) == 0 {
 		return cur
@@ -212,8 +214,6 @@ func (cur *Rollout) Step(prev *Rollout, nowTS int) *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:
diff --git a/pkg/reconciler/route/traffic/traffic.go b/pkg/reconciler/route/traffic/traffic.go
index c9c233cce..4ab7c19ce 100644
--- a/pkg/reconciler/route/traffic/traffic.go
+++ b/pkg/reconciler/route/traffic/traffic.go
@@ -190,6 +190,11 @@ func buildRolloutForTag(r *Rollout, tag string, rts RevisionTargets) {
 			continue
 		}
 
+		// Ignore the rollouts with 0 percent target traffic.
+		// This can happen only for the default tag.
+		if *rt.Percent == 0 {
+			continue
+		}
 		// The targets with the same revision are already joined together.
 		r.Configurations = append(r.Configurations, ConfigurationRollout{
 			ConfigurationName: rt.ConfigurationName,


Test patch:
diff --git a/pkg/reconciler/route/traffic/traffic_test.go b/pkg/reconciler/route/traffic/traffic_test.go
index 828e7122b..7dba88dcc 100644
--- a/pkg/reconciler/route/traffic/traffic_test.go
+++ b/pkg/reconciler/route/traffic/traffic_test.go
@@ -730,17 +730,34 @@ func TestBuildTrafficConfigurationCanary(t *testing.T) {
 			goodNewRev.Name: goodNewRev,
 		},
 	}
-	if tc, err := BuildTrafficConfiguration(configLister, revLister, testRouteWithTrafficTargets(WithSpecTraffic(v1.TrafficTarget{
+	tc, err := BuildTrafficConfiguration(configLister, revLister, testRouteWithTrafficTargets(WithSpecTraffic(v1.TrafficTarget{
 		RevisionName: goodOldRev.Name,
 		Percent:      ptr.Int64(90),
 	}, v1.TrafficTarget{
 		ConfigurationName: goodConfig.Name,
 		Percent:           ptr.Int64(10),
-	}))); err != nil {
+	})))
+	if err != nil {
 		t.Error("Unexpected error", err)
 	} else if got, want := tc, expected; !cmp.Equal(want, got, cmpOpts...) {
 		t.Error("Unexpected traffic diff (-want +got):", cmp.Diff(want, got, cmpOpts...))
 	}
+
+	// Ensure just the 10% is going to be rolled out.
+	gotR := tc.BuildRollout()
+	wantR := &Rollout{
+		Configurations: []ConfigurationRollout{{
+			ConfigurationName: goodConfig.Name,
+			Percent:           10,
+			Revisions: []RevisionRollout{{
+				RevisionName: goodNewRev.Name,
+				Percent:      10,
+			}},
+		}},
+	}
+	if !cmp.Equal(gotR, wantR) {
+		t.Errorf("Rollout mismatch, diff(-want,+got):\n%s", cmp.Diff(wantR, gotR))
+	}
 }
 
 // Splitting traffic between latest revision and a fixed revision which is also latest.
@@ -1483,6 +1500,105 @@ func getTestReadyConfig(name string) (*v1.Configuration, *v1.Revision, *v1.Revis
 	return config, rev1, rev2
 }
 
+func TestBuildTrafficConfigurationTag0Percent(t *testing.T) {
+	expected := &Config{
+		Targets: map[string]RevisionTargets{
+			DefaultTarget: {{
+				TrafficTarget: v1.TrafficTarget{
+					ConfigurationName: niceConfig.Name,
+					RevisionName:      niceNewRev.Name,
+					Percent:           ptr.Int64(100),
+					LatestRevision:    ptr.Bool(true),
+				},
+				Protocol: net.ProtocolH2C,
+			}, {
+				TrafficTarget: v1.TrafficTarget{
+					ConfigurationName: goodConfig.Name,
+					RevisionName:      goodNewRev.Name,
+					Percent:           ptr.Int64(0),
+					LatestRevision:    ptr.Bool(true),
+					Tag:               "robert",
+				},
+				Protocol: net.ProtocolH2C,
+			}},
+			"robert": {{
+				TrafficTarget: v1.TrafficTarget{
+					ConfigurationName: goodConfig.Name,
+					RevisionName:      goodNewRev.Name,
+					Percent:           ptr.Int64(100),
+					Tag:               "robert",
+					LatestRevision:    ptr.Bool(true),
+				},
+				Protocol: net.ProtocolH2C,
+			}},
+		},
+		revisionTargets: []RevisionTarget{{
+			TrafficTarget: v1.TrafficTarget{
+				ConfigurationName: niceConfig.Name,
+				RevisionName:      niceNewRev.Name,
+				Percent:           ptr.Int64(100),
+				LatestRevision:    ptr.Bool(true),
+			},
+			Protocol: net.ProtocolH2C,
+		}, {
+			TrafficTarget: v1.TrafficTarget{
+				ConfigurationName: goodConfig.Name,
+				RevisionName:      goodNewRev.Name,
+				Percent:           ptr.Int64(0),
+				LatestRevision:    ptr.Bool(true),
+				Tag:               "robert",
+			},
+			Protocol: net.ProtocolH2C,
+		}},
+		Configurations: map[string]*v1.Configuration{
+			niceConfig.Name: niceConfig,
+			goodConfig.Name: goodConfig,
+		},
+		Revisions: map[string]*v1.Revision{
+			niceNewRev.Name: niceNewRev,
+			goodNewRev.Name: goodNewRev,
+		},
+	}
+	tc, err := BuildTrafficConfiguration(configLister, revLister, testRouteWithTrafficTargets(WithSpecTraffic(v1.TrafficTarget{
+		ConfigurationName: niceConfig.Name,
+		Percent:           ptr.Int64(100),
+	}, v1.TrafficTarget{
+		ConfigurationName: goodConfig.Name,
+		Percent:           ptr.Int64(0),
+		Tag:               "robert",
+	})))
+	if err != nil {
+		t.Fatal("Unexpected error", err)
+	}
+	if got, want := tc, expected; !cmp.Equal(want, got, cmpOpts...) {
+		fmt.Println(spew.Sdump(got))
+		t.Fatalf("Unexpected traffic diff (-want +got):\n%s", cmp.Diff(want, got, cmpOpts...))
+	}
+
+	gotR := tc.BuildRollout()
+	wantR := &Rollout{
+		Configurations: []ConfigurationRollout{{
+			ConfigurationName: niceConfig.Name,
+			Percent:           100,
+			Revisions: []RevisionRollout{{
+				RevisionName: niceNewRev.Name,
+				Percent:      100,
+			}},
+		}, {
+			ConfigurationName: goodConfig.Name,
+			Tag:               "robert",
+			Percent:           100,
+			Revisions: []RevisionRollout{{
+				RevisionName: goodNewRev.Name,
+				Percent:      100,
+			}},
+		}},
+	}
+	if !cmp.Equal(gotR, wantR) {
+		t.Errorf("Rollout mismatch, diff(-want,+got):\n%s", cmp.Diff(wantR, gotR))
+	}
+}
+
 func TestMain(m *testing.M) {
 	setUp()
 	os.Exit(m.Run())