Skip to content

Commit 20c9f65

Browse files
easwarseaswars
authored
xds: Store WeightedClusters as a slice instead of a map inside the Route (grpc#8632)
Reasons for this change: - The `WeightedClusters` field is never used as a map. - Weighted clusters are stored as a list in the original envoy proto as well. - In tests that require deterministic WRR behavior, we use the `testutils.NewTestWRR` to get rid of the randomness. But the output still depends on the order in which items are added to the WRR. Maps in Go are non-deterministic. RELEASE NOTES: N/A
1 parent 9ff80a7 commit 20c9f65

File tree

8 files changed

+104
-69
lines changed

8 files changed

+104
-69
lines changed

‎internal/xds/resolver/xds_resolver.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -357,14 +357,14 @@ func (r *xdsResolver) newConfigSelector() *configSelector {
357357
ci.cfg = xdsChildConfig{ChildPolicy: balancerConfig(r.currentRouteConfig.ClusterSpecifierPlugins[rt.ClusterSpecifierPlugin])}
358358
cs.clusters[clusterName] = ci
359359
} else {
360-
for cluster, wc := range rt.WeightedClusters {
361-
clusterName := clusterPrefix + cluster
360+
for _, wc := range rt.WeightedClusters {
361+
clusterName := clusterPrefix + wc.Name
362362
clusters.Add(&routeCluster{
363363
name: clusterName,
364364
httpFilterConfigOverride: wc.HTTPFilterConfigOverride,
365365
}, int64(wc.Weight))
366366
ci := r.addOrGetActiveClusterInfo(clusterName)
367-
ci.cfg = xdsChildConfig{ChildPolicy: newBalancerConfig(cdsName, cdsBalancerConfig{Cluster: cluster})}
367+
ci.cfg = xdsChildConfig{ChildPolicy: newBalancerConfig(cdsName, cdsBalancerConfig{Cluster: wc.Name})}
368368
cs.clusters[clusterName] = ci
369369
}
370370
}

‎internal/xds/xdsclient/tests/federation_watchers_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ func (s) TestFederation_RouteConfigResourceContextParamOrder(t *testing.T) {
182182
{
183183
Prefix: newStringP("/"),
184184
ActionType: xdsresource.RouteActionRoute,
185-
WeightedClusters: map[string]xdsresource.WeightedCluster{"cluster-resource": {Weight: 100}},
185+
WeightedClusters: []xdsresource.WeightedCluster{{Name: "cluster-resource", Weight: 100}},
186186
},
187187
},
188188
},

‎internal/xds/xdsclient/tests/rds_watchers_test.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ func (s) TestRDSWatch(t *testing.T) {
183183
{
184184
Prefix: newStringP("/"),
185185
ActionType: xdsresource.RouteActionRoute,
186-
WeightedClusters: map[string]xdsresource.WeightedCluster{cdsName: {Weight: 100}},
186+
WeightedClusters: []xdsresource.WeightedCluster{{Name: cdsName, Weight: 100}},
187187
},
188188
},
189189
},
@@ -206,7 +206,7 @@ func (s) TestRDSWatch(t *testing.T) {
206206
{
207207
Prefix: newStringP("/"),
208208
ActionType: xdsresource.RouteActionRoute,
209-
WeightedClusters: map[string]xdsresource.WeightedCluster{cdsNameNewStyle: {Weight: 100}},
209+
WeightedClusters: []xdsresource.WeightedCluster{{Name: cdsNameNewStyle, Weight: 100}},
210210
},
211211
},
212212
},
@@ -345,7 +345,7 @@ func (s) TestRDSWatch_TwoWatchesForSameResourceName(t *testing.T) {
345345
{
346346
Prefix: newStringP("/"),
347347
ActionType: xdsresource.RouteActionRoute,
348-
WeightedClusters: map[string]xdsresource.WeightedCluster{cdsName: {Weight: 100}},
348+
WeightedClusters: []xdsresource.WeightedCluster{{Name: cdsName, Weight: 100}},
349349
},
350350
},
351351
},
@@ -361,7 +361,7 @@ func (s) TestRDSWatch_TwoWatchesForSameResourceName(t *testing.T) {
361361
{
362362
Prefix: newStringP("/"),
363363
ActionType: xdsresource.RouteActionRoute,
364-
WeightedClusters: map[string]xdsresource.WeightedCluster{"new-cds-resource": {Weight: 100}},
364+
WeightedClusters: []xdsresource.WeightedCluster{{Name: "new-cds-resource", Weight: 100}},
365365
},
366366
},
367367
},
@@ -383,7 +383,7 @@ func (s) TestRDSWatch_TwoWatchesForSameResourceName(t *testing.T) {
383383
{
384384
Prefix: newStringP("/"),
385385
ActionType: xdsresource.RouteActionRoute,
386-
WeightedClusters: map[string]xdsresource.WeightedCluster{cdsNameNewStyle: {Weight: 100}},
386+
WeightedClusters: []xdsresource.WeightedCluster{{Name: cdsNameNewStyle, Weight: 100}},
387387
},
388388
},
389389
},
@@ -399,7 +399,7 @@ func (s) TestRDSWatch_TwoWatchesForSameResourceName(t *testing.T) {
399399
{
400400
Prefix: newStringP("/"),
401401
ActionType: xdsresource.RouteActionRoute,
402-
WeightedClusters: map[string]xdsresource.WeightedCluster{"new-cds-resource": {Weight: 100}},
402+
WeightedClusters: []xdsresource.WeightedCluster{{Name: "new-cds-resource", Weight: 100}},
403403
},
404404
},
405405
},
@@ -597,7 +597,7 @@ func (s) TestRDSWatch_ThreeWatchesForDifferentResourceNames(t *testing.T) {
597597
{
598598
Prefix: newStringP("/"),
599599
ActionType: xdsresource.RouteActionRoute,
600-
WeightedClusters: map[string]xdsresource.WeightedCluster{cdsName: {Weight: 100}},
600+
WeightedClusters: []xdsresource.WeightedCluster{{Name: cdsName, Weight: 100}},
601601
},
602602
},
603603
},
@@ -688,7 +688,7 @@ func (s) TestRDSWatch_ResourceCaching(t *testing.T) {
688688
{
689689
Prefix: newStringP("/"),
690690
ActionType: xdsresource.RouteActionRoute,
691-
WeightedClusters: map[string]xdsresource.WeightedCluster{cdsName: {Weight: 100}},
691+
WeightedClusters: []xdsresource.WeightedCluster{{Name: cdsName, Weight: 100}},
692692
},
693693
},
694694
},
@@ -819,7 +819,7 @@ func (s) TestRDSWatch_ValidResponseCancelsExpiryTimerBehavior(t *testing.T) {
819819
{
820820
Prefix: newStringP("/"),
821821
ActionType: xdsresource.RouteActionRoute,
822-
WeightedClusters: map[string]xdsresource.WeightedCluster{cdsName: {Weight: 100}},
822+
WeightedClusters: []xdsresource.WeightedCluster{{Name: cdsName, Weight: 100}},
823823
},
824824
},
825825
},
@@ -983,7 +983,7 @@ func (s) TestRDSWatch_PartialValid(t *testing.T) {
983983
{
984984
Prefix: newStringP("/"),
985985
ActionType: xdsresource.RouteActionRoute,
986-
WeightedClusters: map[string]xdsresource.WeightedCluster{cdsName: {Weight: 100}},
986+
WeightedClusters: []xdsresource.WeightedCluster{{Name: cdsName, Weight: 100}},
987987
},
988988
},
989989
},

‎internal/xds/xdsclient/tests/resource_update_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -510,7 +510,7 @@ func (s) TestHandleRouteConfigResponseFromManagementServer(t *testing.T) {
510510
{
511511
Domains: []string{"lds-target-name"},
512512
Routes: []*xdsresource.Route{{Prefix: newStringP(""),
513-
WeightedClusters: map[string]xdsresource.WeightedCluster{"cluster-name": {Weight: 1}},
513+
WeightedClusters: []xdsresource.WeightedCluster{{Name: "cluster-name", Weight: 1}},
514514
ActionType: xdsresource.RouteActionRoute}},
515515
},
516516
},
@@ -538,7 +538,7 @@ func (s) TestHandleRouteConfigResponseFromManagementServer(t *testing.T) {
538538
{
539539
Domains: []string{"lds-target-name"},
540540
Routes: []*xdsresource.Route{{Prefix: newStringP(""),
541-
WeightedClusters: map[string]xdsresource.WeightedCluster{"cluster-name": {Weight: 1}},
541+
WeightedClusters: []xdsresource.WeightedCluster{{Name: "cluster-name", Weight: 1}},
542542
ActionType: xdsresource.RouteActionRoute}},
543543
},
544544
},

‎internal/xds/xdsclient/xdsresource/type_rds.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,14 +146,16 @@ type Route struct {
146146

147147
// Only one of the following fields (WeightedClusters or
148148
// ClusterSpecifierPlugin) will be set for a route.
149-
WeightedClusters map[string]WeightedCluster
149+
WeightedClusters []WeightedCluster
150150
// ClusterSpecifierPlugin is the name of the Cluster Specifier Plugin that
151151
// this Route is linked to, if specified by xDS.
152152
ClusterSpecifierPlugin string
153153
}
154154

155155
// WeightedCluster contains settings for an xds ActionType.WeightedCluster.
156156
type WeightedCluster struct {
157+
// Name is the name of the cluster.
158+
Name string
157159
// Weight is the relative weight of the cluster. It will never be zero.
158160
Weight uint32
159161
// HTTPFilterConfigOverride contains any HTTP filter config overrides for

‎internal/xds/xdsclient/xdsresource/unmarshal_lds_test.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -565,7 +565,11 @@ func (s) TestUnmarshalListener_ClientSide(t *testing.T) {
565565
InlineRouteConfig: &RouteConfigUpdate{
566566
VirtualHosts: []*VirtualHost{{
567567
Domains: []string{v3LDSTarget},
568-
Routes: []*Route{{Prefix: newStringP("/"), WeightedClusters: map[string]WeightedCluster{clusterName: {Weight: 1}}, ActionType: RouteActionRoute}},
568+
Routes: []*Route{{
569+
Prefix: newStringP("/"),
570+
WeightedClusters: []WeightedCluster{{Name: clusterName, Weight: 1}},
571+
ActionType: RouteActionRoute,
572+
}},
569573
}}},
570574
MaxStreamDuration: time.Second,
571575
Raw: v3LisWithInlineRoute,

‎internal/xds/xdsclient/xdsresource/unmarshal_rds.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,6 @@ func routesProtoToSlice(routes []*v3routepb.Route, csps map[string]clusterspecif
300300

301301
switch r.GetAction().(type) {
302302
case *v3routepb.Route_Route:
303-
route.WeightedClusters = make(map[string]WeightedCluster)
304303
action := r.GetRoute()
305304

306305
// Hash Policies are only applicable for a Ring Hash LB.
@@ -312,7 +311,7 @@ func routesProtoToSlice(routes []*v3routepb.Route, csps map[string]clusterspecif
312311

313312
switch a := action.GetClusterSpecifier().(type) {
314313
case *v3routepb.RouteAction_Cluster:
315-
route.WeightedClusters[a.Cluster] = WeightedCluster{Weight: 1}
314+
route.WeightedClusters = append(route.WeightedClusters, WeightedCluster{Name: a.Cluster, Weight: 1})
316315
case *v3routepb.RouteAction_WeightedClusters:
317316
wcs := a.WeightedClusters
318317
var totalWeight uint64
@@ -325,13 +324,13 @@ func routesProtoToSlice(routes []*v3routepb.Route, csps map[string]clusterspecif
325324
if totalWeight > math.MaxUint32 {
326325
return nil, nil, fmt.Errorf("xds: total weight of clusters exceeds MaxUint32")
327326
}
328-
wc := WeightedCluster{Weight: w}
327+
wc := WeightedCluster{Name: c.GetName(), Weight: w}
329328
cfgs, err := processHTTPFilterOverrides(c.GetTypedPerFilterConfig())
330329
if err != nil {
331330
return nil, nil, fmt.Errorf("route %+v, action %+v: %v", r, a, err)
332331
}
333332
wc.HTTPFilterConfigOverride = cfgs
334-
route.WeightedClusters[c.GetName()] = wc
333+
route.WeightedClusters = append(route.WeightedClusters, wc)
335334
}
336335
if totalWeight == 0 {
337336
return nil, nil, fmt.Errorf("route %+v, action %+v, has no valid cluster in WeightedCluster action", r, a)

0 commit comments

Comments
 (0)