Repository: knative/serving
Pull request #9841 (2020-10-15 21:30:12)

Support `nil` target for merging
For some reason we permit `nil` percentage for targets which are
in the main route, even though the spec says `percentage` is required.

Anyway, this solves the issue when it's nil.

I'll also think of moving the helper to the `ptr` library. Probably we need those elsewhere.

/assign @tcnghia @dprotaso mattmoor

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/9841#" 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/9841#pullrequestreview-509823911" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/9841#" 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/9841?src=pr&el=h1) Report
> Merging [#9841](https://codecov.io/gh/knative/serving/pull/9841?src=pr&el=desc) into [master](https://codecov.io/gh/knative/serving/commit/1a4202af4d7417714ec419d9b0dba86fbfd25f3e?el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master    #9841      +/-   ##
==========================================
- Coverage   87.67%   87.64%   -0.04%     
==========================================
  Files         180      180              
  Lines        8593     8596       +3     
==========================================
  Hits         7534     7534              
- Misses        808      809       +1     
- Partials      251      253       +2     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/9841?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/traffic/traffic.go](https://codecov.io/gh/knative/serving/pull/9841/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy90cmFmZmljLmdv) | `92.48% <100.00%> (+0.13%)` | :arrow_up: |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/9841/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9jb25maWd1cmF0aW9uLmdv) | `86.88% <0.00%> (-2.46%)` | :arrow_down: |

------

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


Patch:
diff --git a/pkg/reconciler/route/traffic/traffic.go b/pkg/reconciler/route/traffic/traffic.go
index eb5033bb4..6425675c6 100644
--- a/pkg/reconciler/route/traffic/traffic.go
+++ b/pkg/reconciler/route/traffic/traffic.go
@@ -325,13 +325,21 @@ func (cb *configBuilder) addRevisionTarget(tt *v1.TrafficTarget) error {
 	return nil
 }
 
+// valIfNil returns `val` if `ptr==nil`, or `*ptr` otherwise.
+func valIfNil(val int64, ptr *int64) int64 {
+	if ptr == nil {
+		return val
+	}
+	return *ptr
+}
+
 // This find the exact revision+tag pair and if so, just adds the percentages.
 // This expects single digit lists, so just does an O(N) search.
 func mergeIfNecessary(rts RevisionTargets, rt RevisionTarget) RevisionTargets {
 	for i := range rts {
 		if rts[i].Tag == rt.Tag && rts[i].RevisionName == rt.RevisionName &&
 			*rt.LatestRevision == *rts[i].LatestRevision {
-			rts[i].Percent = ptr.Int64(*rts[i].Percent + *rt.Percent)
+			rts[i].Percent = ptr.Int64(valIfNil(0, rts[i].Percent) + valIfNil(0, rt.Percent))
 			return rts
 		}
 	}


Test patch:
diff --git a/pkg/reconciler/route/traffic/traffic_test.go b/pkg/reconciler/route/traffic/traffic_test.go
index 0ad298c1b..50d378a15 100644
--- a/pkg/reconciler/route/traffic/traffic_test.go
+++ b/pkg/reconciler/route/traffic/traffic_test.go
@@ -389,13 +389,39 @@ func TestBuildTrafficConfigurationTwoEntriesSameConfig(t *testing.T) {
 			niceNewRev.Name: niceNewRev,
 		},
 	}
-	if tc, err := BuildTrafficConfiguration(configLister, revLister, testRouteWithTrafficTargets(WithSpecTraffic(v1.TrafficTarget{
-		ConfigurationName: niceConfig.Name,
-		Percent:           ptr.Int64(90),
-	}, v1.TrafficTarget{
-		ConfigurationName: niceConfig.Name,
-		Percent:           ptr.Int64(10),
-	}))); err != nil {
+
+	// Test 42%/42%/16% split.
+	tc, err := BuildTrafficConfiguration(
+		configLister, revLister, testRouteWithTrafficTargets(
+			WithSpecTraffic([]v1.TrafficTarget{{
+				ConfigurationName: niceConfig.Name,
+				Percent:           ptr.Int64(42),
+			}, {
+				ConfigurationName: niceConfig.Name,
+				Percent:           ptr.Int64(16),
+			}, {
+				ConfigurationName: niceConfig.Name,
+				Percent:           ptr.Int64(42),
+			}}...)))
+	if err != nil {
+		t.Error("Unexpected error", err)
+	} else if got, want := tc, expected; !cmp.Equal(want, got, cmpOpts...) {
+		fmt.Println(spew.Sdump(got))
+		t.Error("Unexpected traffic diff (-want +got):", cmp.Diff(want, got, cmpOpts...))
+	}
+
+	// Test 100%/nil case.
+	tc, err = BuildTrafficConfiguration(
+		configLister, revLister,
+		testRouteWithTrafficTargets(WithSpecTraffic([]v1.TrafficTarget{{
+			ConfigurationName: niceConfig.Name,
+			Percent:           ptr.Int64(100),
+		}, {
+			ConfigurationName: niceConfig.Name,
+		}, {
+			ConfigurationName: niceConfig.Name, // let's throw in 2 nils! ;-)
+		}}...)))
+	if err != nil {
 		t.Error("Unexpected error", err)
 	} else if got, want := tc, expected; !cmp.Equal(want, got, cmpOpts...) {
 		fmt.Println(spew.Sdump(got))