Repository: knative/serving
Pull request #9916 (2020-10-22 20:54:10)

Sort both configs and tags within rollouts
Revisions should not be sorted, since they are sorted by 'age'.

/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/9916#" 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/9916#pullrequestreview-515113021" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/9916#" 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 e529681d7..88906cf1b 100644
--- a/pkg/reconciler/route/traffic/traffic.go
+++ b/pkg/reconciler/route/traffic/traffic.go
@@ -182,6 +182,10 @@ func (cfg *Config) BuildRollout() *Rollout {
 // from run to run, since input to the process is map iterator.
 func sortRollout(r *Rollout) {
 	sort.Slice(r.Configurations, func(i, j int) bool {
+		// Sort by tag and within tag sort by config name.
+		if r.Configurations[i].Tag == r.Configurations[j].Tag {
+			return r.Configurations[i].ConfigurationName < r.Configurations[j].ConfigurationName
+		}
 		return r.Configurations[i].Tag < r.Configurations[j].Tag
 	})
 }


Test patch:
diff --git a/pkg/reconciler/route/traffic/traffic_test.go b/pkg/reconciler/route/traffic/traffic_test.go
index 463ee9c4a..d2656f55a 100644
--- a/pkg/reconciler/route/traffic/traffic_test.go
+++ b/pkg/reconciler/route/traffic/traffic_test.go
@@ -443,6 +443,166 @@ func TestBuildTrafficConfigurationTwoEntriesSameConfig(t *testing.T) {
 	}
 }
 
+func TestBuildTrafficConfigThreeConfigs(t *testing.T) {
+	// This tests the rollout building and sorting mostly.
+	expected := &Config{
+		Targets: map[string]RevisionTargets{
+			DefaultTarget: {{
+				TrafficTarget: v1.TrafficTarget{
+					ConfigurationName: niceConfig.Name,
+					RevisionName:      niceNewRev.Name,
+					Percent:           ptr.Int64(30),
+					LatestRevision:    ptr.Bool(true),
+				},
+				Active:   true,
+				Protocol: net.ProtocolH2C,
+			}, {
+				TrafficTarget: v1.TrafficTarget{
+					ConfigurationName: goodConfig.Name,
+					RevisionName:      goodNewRev.Name,
+					Percent:           ptr.Int64(35),
+					LatestRevision:    ptr.Bool(true),
+					Tag:               "robert",
+				},
+				Active:   true,
+				Protocol: net.ProtocolH2C,
+			}, {
+				TrafficTarget: v1.TrafficTarget{
+					ConfigurationName: inactiveConfig.Name,
+					RevisionName:      inactiveRev.Name,
+					Percent:           ptr.Int64(35),
+					LatestRevision:    ptr.Bool(true),
+					Tag:               "jackson",
+				},
+				Active:   false,
+				Protocol: net.ProtocolHTTP1,
+			}},
+			"robert": {{
+				TrafficTarget: v1.TrafficTarget{
+					ConfigurationName: goodConfig.Name,
+					RevisionName:      goodNewRev.Name,
+					Percent:           ptr.Int64(100),
+					Tag:               "robert",
+					LatestRevision:    ptr.Bool(true),
+				},
+				Active:   true,
+				Protocol: net.ProtocolH2C,
+			}},
+			"jackson": {{
+				TrafficTarget: v1.TrafficTarget{
+					ConfigurationName: inactiveConfig.Name,
+					RevisionName:      inactiveRev.Name,
+					Percent:           ptr.Int64(100),
+					Tag:               "jackson",
+					LatestRevision:    ptr.Bool(true),
+				},
+				Active:   false,
+				Protocol: net.ProtocolHTTP1,
+			}},
+		},
+		revisionTargets: []RevisionTarget{{
+			TrafficTarget: v1.TrafficTarget{
+				ConfigurationName: niceConfig.Name,
+				RevisionName:      niceNewRev.Name,
+				Percent:           ptr.Int64(30),
+				LatestRevision:    ptr.Bool(true),
+			},
+			Active:   true,
+			Protocol: net.ProtocolH2C,
+		}, {
+			TrafficTarget: v1.TrafficTarget{
+				ConfigurationName: goodConfig.Name,
+				RevisionName:      goodNewRev.Name,
+				Percent:           ptr.Int64(35),
+				LatestRevision:    ptr.Bool(true),
+				Tag:               "robert",
+			},
+			Active:   true,
+			Protocol: net.ProtocolH2C,
+		}, {
+			TrafficTarget: v1.TrafficTarget{
+				ConfigurationName: inactiveConfig.Name,
+				RevisionName:      inactiveRev.Name,
+				Percent:           ptr.Int64(35),
+				LatestRevision:    ptr.Bool(true),
+				Tag:               "jackson",
+			},
+			Active:   false,
+			Protocol: net.ProtocolHTTP1,
+		}},
+		Configurations: map[string]*v1.Configuration{
+			niceConfig.Name:     niceConfig,
+			goodConfig.Name:     goodConfig,
+			inactiveConfig.Name: inactiveConfig,
+		},
+		Revisions: map[string]*v1.Revision{
+			niceNewRev.Name:  niceNewRev,
+			goodNewRev.Name:  goodNewRev,
+			inactiveRev.Name: inactiveRev,
+		},
+	}
+	tc, err := BuildTrafficConfiguration(configLister, revLister, testRouteWithTrafficTargets(WithSpecTraffic([]v1.TrafficTarget{{
+		ConfigurationName: niceConfig.Name,
+		Percent:           ptr.Int64(30),
+	}, {
+		ConfigurationName: goodConfig.Name,
+		Percent:           ptr.Int64(35),
+		Tag:               "robert",
+	}, {
+		ConfigurationName: inactiveConfig.Name,
+		Percent:           ptr.Int64(35),
+		Tag:               "jackson",
+	}}...)))
+	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{
+		// Default tag configs are sorted as well.
+		Configurations: []ConfigurationRollout{{
+			ConfigurationName: goodConfig.Name,
+			Revisions: []RevisionRollout{{
+				RevisionName: goodNewRev.Name,
+				Percent:      35,
+			}},
+		}, {
+			ConfigurationName: inactiveConfig.Name,
+			Revisions: []RevisionRollout{{
+				RevisionName: inactiveRev.Name,
+				Percent:      35,
+			}},
+		}, {
+			ConfigurationName: niceConfig.Name,
+			Revisions: []RevisionRollout{{
+				RevisionName: niceNewRev.Name,
+				Percent:      30,
+			}},
+		}, {
+			// Note sorting flipped these two.
+			ConfigurationName: inactiveConfig.Name,
+			Tag:               "jackson",
+			Revisions: []RevisionRollout{{
+				RevisionName: inactiveRev.Name,
+				Percent:      100,
+			}},
+		}, {
+			ConfigurationName: goodConfig.Name,
+			Tag:               "robert",
+			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 TestBuildTrafficConfigurationTwoEntriesSameConfigDifferentTags(t *testing.T) {
 	expected := &Config{
 		Targets: map[string]RevisionTargets{