Repository: knative/serving
Pull request #9746 (2020-10-08 22:51:48)

Merge equal targets together
If the input has multiple inputs of the same kind, e.g.:
config=x; percent=5
config=x; percent=10
config=x; percent=30; tag=whatevs
config=y; percent=55

then the ingress will be
for route main URL:
revisionx => 45%
revisiony => 55%
for route tag url whatevs
revisionx => 100%

but the status of the route will be
revisionx; percent=5
revisionx; percent=10
revisionx; percent=30; tag=whatevs; url=whateves...
revisiony; percent=55
url: route...

This change merges the top ones together, so that the output is more
reasonable and more matching of what we'd do to ingress
revisionx: percent=15
revisionx; percent=30, tag...
revisiony; percent=55

When there are no tags this might have collapsed into nice 2 element
enty.

This should also simplify the work I am doing for gradual rollout, though there we'd be mostly working with ingress.

/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/9746#" 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/9746#pullrequestreview-505240981" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/9746#" 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":[]} -->

Patch:
diff --git a/pkg/reconciler/route/traffic/traffic.go b/pkg/reconciler/route/traffic/traffic.go
index 8413ab18c..59adaac6a 100644
--- a/pkg/reconciler/route/traffic/traffic.go
+++ b/pkg/reconciler/route/traffic/traffic.go
@@ -291,11 +291,25 @@ func (cb *configBuilder) addRevisionTarget(tt *v1.TrafficTarget) error {
 	return nil
 }
 
+// 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)
+			return rts
+		}
+	}
+	return append(rts, rt)
+}
+
 func (cb *configBuilder) addFlattenedTarget(target RevisionTarget) {
 	name := target.TrafficTarget.Tag
-	cb.revisionTargets = append(cb.revisionTargets, target)
+	cb.revisionTargets = mergeIfNecessary(cb.revisionTargets, target)
 	cb.targets[DefaultTarget] = append(cb.targets[DefaultTarget], target)
 	if name != "" {
+		// This should always have just a single entry at most.
 		cb.targets[name] = append(cb.targets[name], target)
 	}
 }


Test patch:
diff --git a/pkg/reconciler/route/traffic/traffic_test.go b/pkg/reconciler/route/traffic/traffic_test.go
index c1c482fb0..c2cecf4ff 100644
--- a/pkg/reconciler/route/traffic/traffic_test.go
+++ b/pkg/reconciler/route/traffic/traffic_test.go
@@ -19,10 +19,12 @@ package traffic
 import (
 	"context"
 	"errors"
+	"fmt"
 	"os"
 	"testing"
 	"time"
 
+	"github.com/davecgh/go-spew/spew"
 	"github.com/google/go-cmp/cmp"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -324,6 +326,51 @@ func TestBuildTrafficConfigurationTwoConfigs(t *testing.T) {
 	}
 }
 
+func TestBuildTrafficConfigurationTwoEntriesSameConfig(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),
+				},
+				Active:   true,
+				Protocol: net.ProtocolH2C,
+			}},
+		},
+		revisionTargets: []RevisionTarget{{
+			TrafficTarget: v1.TrafficTarget{
+				ConfigurationName: niceConfig.Name,
+				RevisionName:      niceNewRev.Name,
+				Percent:           ptr.Int64(100),
+				LatestRevision:    ptr.Bool(true),
+			},
+			Active:   true,
+			Protocol: net.ProtocolH2C,
+		}},
+		Configurations: map[string]*v1.Configuration{
+			niceConfig.Name: niceConfig,
+		},
+		Revisions: map[string]*v1.Revision{
+			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 {
+		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...))
+	}
+}
+
 // Splitting traffic between a fixed revision and the latest revision (canary).
 func TestBuildTrafficConfigurationCanary(t *testing.T) {
 	expected := &Config{