Skip to content

Commit b6597b3

Browse files
eshitachandwanieshitachandwani
authored
xds/clusterimpl: use xdsConfig for updates and remove redundant fields from LB config (#8945)
This is part of [gRFC A74](https://github.com/grpc/proposal/blob/master/A74-xds-config-tears.md) changes. This PR changes clusterimpl balancer to use updates from XDSConfig directly and as a result remove fields from LBConfig that can be used from XDSConfig. Changed tests and configbuilder for cds balancer accordingly. RELEASE NOTES: None
1 parent 1d4fa8a commit b6597b3

File tree

9 files changed

+173
-266
lines changed

9 files changed

+173
-266
lines changed

‎internal/xds/balancer/cdsbalancer/aggregate_cluster_test.go‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ func (s) TestAggregateClusterSuccess_LeafNode(t *testing.T) {
8888
wantFirstChildCfg: &priority.LBConfig{
8989
Children: map[string]*priority.Child{
9090
"priority-0-0": {
91-
Config: createPriorityConfig(clusterName, serviceName),
91+
Config: createPriorityConfig(clusterName),
9292
IgnoreReresolutionRequests: true,
9393
},
9494
},
@@ -97,7 +97,7 @@ func (s) TestAggregateClusterSuccess_LeafNode(t *testing.T) {
9797
wantSecondChildCfg: &priority.LBConfig{
9898
Children: map[string]*priority.Child{
9999
"priority-1-0": {
100-
Config: createPriorityConfig(clusterName, serviceName+"-new"),
100+
Config: createPriorityConfig(clusterName),
101101
IgnoreReresolutionRequests: true,
102102
},
103103
},
@@ -109,11 +109,11 @@ func (s) TestAggregateClusterSuccess_LeafNode(t *testing.T) {
109109
firstClusterResource: makeLogicalDNSClusterResource(clusterName, "dns_host", uint32(port)),
110110
secondClusterResource: makeLogicalDNSClusterResource(clusterName, "dns_host_new", uint32(port)),
111111
wantFirstChildCfg: &priority.LBConfig{
112-
Children: map[string]*priority.Child{"priority-0": {Config: createPriorityConfig(clusterName, "")}},
112+
Children: map[string]*priority.Child{"priority-0": {Config: createPriorityConfig(clusterName)}},
113113
Priorities: []string{"priority-0"},
114114
},
115115
wantSecondChildCfg: &priority.LBConfig{
116-
Children: map[string]*priority.Child{"priority-1": {Config: createPriorityConfig(clusterName, "")}},
116+
Children: map[string]*priority.Child{"priority-1": {Config: createPriorityConfig(clusterName)}},
117117
Priorities: []string{"priority-1"},
118118
},
119119
},
@@ -208,10 +208,10 @@ func (s) TestAggregateClusterSuccess_ThenUpdateChildClusters(t *testing.T) {
208208
wantChildCfg := &priority.LBConfig{
209209
Children: map[string]*priority.Child{
210210
"priority-0-0": {
211-
Config: createPriorityConfig(edsClusterName, serviceName),
211+
Config: createPriorityConfig(edsClusterName),
212212
IgnoreReresolutionRequests: true,
213213
},
214-
"priority-1": {Config: createPriorityConfig(dnsClusterName, "")},
214+
"priority-1": {Config: createPriorityConfig(dnsClusterName)},
215215
},
216216
Priorities: []string{"priority-0-0", "priority-1"},
217217
}
@@ -239,10 +239,10 @@ func (s) TestAggregateClusterSuccess_ThenUpdateChildClusters(t *testing.T) {
239239
wantChildCfg = &priority.LBConfig{
240240
Children: map[string]*priority.Child{
241241
"priority-0-0": {
242-
Config: createPriorityConfig(edsClusterName, serviceName),
242+
Config: createPriorityConfig(edsClusterName),
243243
IgnoreReresolutionRequests: true,
244244
},
245-
"priority-2": {Config: createPriorityConfig(dnsClusterNameNew, "")},
245+
"priority-2": {Config: createPriorityConfig(dnsClusterNameNew)},
246246
},
247247
Priorities: []string{"priority-0-0", "priority-2"},
248248
}
@@ -285,10 +285,10 @@ func (s) TestAggregateClusterSuccess_ThenChangeRootToEDS(t *testing.T) {
285285
wantChildCfg := &priority.LBConfig{
286286
Children: map[string]*priority.Child{
287287
"priority-0-0": {
288-
Config: createPriorityConfig(edsClusterName, serviceName),
288+
Config: createPriorityConfig(edsClusterName),
289289
IgnoreReresolutionRequests: true,
290290
},
291-
"priority-1": {Config: createPriorityConfig(dnsClusterName, "")},
291+
"priority-1": {Config: createPriorityConfig(dnsClusterName)},
292292
},
293293
Priorities: []string{"priority-0-0", "priority-1"},
294294
}
@@ -314,7 +314,7 @@ func (s) TestAggregateClusterSuccess_ThenChangeRootToEDS(t *testing.T) {
314314
wantChildCfg = &priority.LBConfig{
315315
Children: map[string]*priority.Child{
316316
"priority-0-0": {
317-
Config: createPriorityConfig(clusterName, serviceName),
317+
Config: createPriorityConfig(clusterName),
318318
IgnoreReresolutionRequests: true,
319319
},
320320
},
@@ -349,7 +349,7 @@ func (s) TestAggregatedClusterSuccess_SwitchBetweenLeafAndAggregate(t *testing.T
349349
wantChildCfg := &priority.LBConfig{
350350
Children: map[string]*priority.Child{
351351
"priority-0-0": {
352-
Config: createPriorityConfig(clusterName, serviceName),
352+
Config: createPriorityConfig(clusterName),
353353
IgnoreReresolutionRequests: true,
354354
},
355355
},
@@ -378,10 +378,10 @@ func (s) TestAggregatedClusterSuccess_SwitchBetweenLeafAndAggregate(t *testing.T
378378
wantChildCfg = &priority.LBConfig{
379379
Children: map[string]*priority.Child{
380380
"priority-0-0": {
381-
Config: createPriorityConfig(edsClusterName, serviceName),
381+
Config: createPriorityConfig(edsClusterName),
382382
IgnoreReresolutionRequests: true,
383383
},
384-
"priority-1": {Config: createPriorityConfig(dnsClusterName, "")},
384+
"priority-1": {Config: createPriorityConfig(dnsClusterName)},
385385
},
386386
Priorities: []string{"priority-0-0", "priority-1"},
387387
}
@@ -403,7 +403,7 @@ func (s) TestAggregatedClusterSuccess_SwitchBetweenLeafAndAggregate(t *testing.T
403403
wantChildCfg = &priority.LBConfig{
404404
Children: map[string]*priority.Child{
405405
"priority-0-0": {
406-
Config: createPriorityConfig(clusterName, serviceName),
406+
Config: createPriorityConfig(clusterName),
407407
IgnoreReresolutionRequests: true,
408408
},
409409
},
@@ -561,7 +561,7 @@ func (s) TestAggregatedClusterSuccess_DiamondDependency(t *testing.T) {
561561
wantChildCfg := &priority.LBConfig{
562562
Children: map[string]*priority.Child{
563563
"priority-0-0": {
564-
Config: createPriorityConfig(clusterNameD, serviceName),
564+
Config: createPriorityConfig(clusterNameD),
565565
IgnoreReresolutionRequests: true,
566566
},
567567
},
@@ -629,11 +629,11 @@ func (s) TestAggregatedClusterSuccess_IgnoreDups(t *testing.T) {
629629
wantChildCfg := &priority.LBConfig{
630630
Children: map[string]*priority.Child{
631631
"priority-0-0": {
632-
Config: createPriorityConfig(clusterNameC, edsClusterName),
632+
Config: createPriorityConfig(clusterNameC),
633633
IgnoreReresolutionRequests: true,
634634
},
635635
"priority-1-0": {
636-
Config: createPriorityConfig(clusterNameD, serviceName),
636+
Config: createPriorityConfig(clusterNameD),
637637
IgnoreReresolutionRequests: true,
638638
},
639639
},
@@ -717,7 +717,7 @@ func (s) TestAggregatedCluster_NodeChildOfItself(t *testing.T) {
717717
wantChildCfg := &priority.LBConfig{
718718
Children: map[string]*priority.Child{
719719
"priority-0-0": {
720-
Config: createPriorityConfig(clusterNameB, serviceName),
720+
Config: createPriorityConfig(clusterNameB),
721721
IgnoreReresolutionRequests: true,
722722
},
723723
},
@@ -826,7 +826,7 @@ func (s) TestAggregatedCluster_CycleWithLeafNode(t *testing.T) {
826826
wantChildCfg := &priority.LBConfig{
827827
Children: map[string]*priority.Child{
828828
"priority-0-0": {
829-
Config: createPriorityConfig(clusterNameC, serviceName),
829+
Config: createPriorityConfig(clusterNameC),
830830
IgnoreReresolutionRequests: true,
831831
},
832832
},

‎internal/xds/balancer/cdsbalancer/cdsbalancer_test.go‎

Lines changed: 6 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ import (
4141
"google.golang.org/grpc/internal/stubserver"
4242
"google.golang.org/grpc/internal/testutils"
4343
"google.golang.org/grpc/internal/testutils/xds/e2e"
44-
xdsinternal "google.golang.org/grpc/internal/xds"
4544
"google.golang.org/grpc/internal/xds/balancer/clusterimpl"
4645
"google.golang.org/grpc/internal/xds/balancer/outlierdetection"
4746
"google.golang.org/grpc/internal/xds/balancer/priority"
@@ -248,8 +247,7 @@ func verifyRPCError(gotErr error, wantCode codes.Code, wantErr, wantNodeID strin
248247
}
249248

250249
// createPriorityConfig creates priority config for both EDS and DNS cluster.
251-
// For DNS clusters, edsServiceName passed should be empty.
252-
func createPriorityConfig(cluster, edsServiceName string) *iserviceconfig.BalancerConfig {
250+
func createPriorityConfig(cluster string) *iserviceconfig.BalancerConfig {
253251
return &iserviceconfig.BalancerConfig{
254252
Name: outlierdetection.Name,
255253
Config: &outlierdetection.LBConfig{
@@ -260,9 +258,7 @@ func createPriorityConfig(cluster, edsServiceName string) *iserviceconfig.Balanc
260258
ChildPolicy: &iserviceconfig.BalancerConfig{
261259
Name: clusterimpl.Name,
262260
Config: &clusterimpl.LBConfig{
263-
Cluster: cluster,
264-
EDSServiceName: edsServiceName,
265-
TelemetryLabels: xdsinternal.UnknownCSMLabels,
261+
Cluster: cluster,
266262
ChildPolicy: &iserviceconfig.BalancerConfig{
267263
Name: wrrlocality.Name,
268264
Config: &wrrlocality.LBConfig{ChildPolicy: &iserviceconfig.BalancerConfig{Name: roundrobin.Name}},
@@ -410,10 +406,7 @@ func (s) TestClusterUpdate_Success(t *testing.T) {
410406
ChildPolicy: &iserviceconfig.BalancerConfig{
411407
Name: clusterimpl.Name,
412408
Config: &clusterimpl.LBConfig{
413-
Cluster: clusterName,
414-
EDSServiceName: serviceName,
415-
TelemetryLabels: xdsinternal.UnknownCSMLabels,
416-
MaxConcurrentRequests: newUint32(512),
409+
Cluster: clusterName,
417410
ChildPolicy: &iserviceconfig.BalancerConfig{
418411
Name: wrrlocality.Name,
419412
Config: &wrrlocality.LBConfig{ChildPolicy: &iserviceconfig.BalancerConfig{Name: roundrobin.Name}},
@@ -458,9 +451,7 @@ func (s) TestClusterUpdate_Success(t *testing.T) {
458451
ChildPolicy: &iserviceconfig.BalancerConfig{
459452
Name: clusterimpl.Name,
460453
Config: &clusterimpl.LBConfig{
461-
Cluster: clusterName,
462-
EDSServiceName: serviceName,
463-
TelemetryLabels: xdsinternal.UnknownCSMLabels,
454+
Cluster: clusterName,
464455
ChildPolicy: &iserviceconfig.BalancerConfig{
465456
Name: ringhash.Name,
466457
Config: &iringhash.LBConfig{
@@ -509,9 +500,7 @@ func (s) TestClusterUpdate_Success(t *testing.T) {
509500
ChildPolicy: &iserviceconfig.BalancerConfig{
510501
Name: clusterimpl.Name,
511502
Config: &clusterimpl.LBConfig{
512-
Cluster: clusterName,
513-
EDSServiceName: serviceName,
514-
TelemetryLabels: xdsinternal.UnknownCSMLabels,
503+
Cluster: clusterName,
515504
ChildPolicy: &iserviceconfig.BalancerConfig{
516505
Name: ringhash.Name,
517506
Config: &iringhash.LBConfig{
@@ -579,9 +568,7 @@ func (s) TestClusterUpdate_Success(t *testing.T) {
579568
ChildPolicy: &iserviceconfig.BalancerConfig{
580569
Name: clusterimpl.Name,
581570
Config: &clusterimpl.LBConfig{
582-
Cluster: clusterName,
583-
EDSServiceName: serviceName,
584-
TelemetryLabels: xdsinternal.UnknownCSMLabels,
571+
Cluster: clusterName,
585572
ChildPolicy: &iserviceconfig.BalancerConfig{
586573
Name: ringhash.Name,
587574
Config: &iringhash.LBConfig{
@@ -625,71 +612,6 @@ func (s) TestClusterUpdate_Success(t *testing.T) {
625612
}
626613
}
627614

628-
// Tests a single success scenario where the cds LB policy receives a cluster
629-
// resource from the management server with LRS enabled. Verifies that the load
630-
// balancing configuration pushed to the child is as expected.
631-
func (s) TestClusterUpdate_SuccessWithLRS(t *testing.T) {
632-
lbCfgCh, _, _, _ := registerWrappedPriorityPolicy(t)
633-
mgmtServer, nodeID, _ := setupWithManagementServer(t, nil, nil)
634-
635-
clusterResource := e2e.ClusterResourceWithOptions(e2e.ClusterOptions{
636-
ClusterName: clusterName,
637-
ServiceName: serviceName,
638-
EnableLRS: true,
639-
})
640-
lrsServerCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: fmt.Sprintf("passthrough:///%s", mgmtServer.Address), ServerFeatures: []string{"trusted_xds_server"}})
641-
if err != nil {
642-
t.Fatalf("Failed to create LRS server config for testing: %v", err)
643-
}
644-
645-
wantChildCfg := &priority.LBConfig{
646-
Children: map[string]*priority.Child{
647-
"priority-0-0": {
648-
Config: &iserviceconfig.BalancerConfig{
649-
Name: outlierdetection.Name,
650-
Config: &outlierdetection.LBConfig{
651-
Interval: iserviceconfig.Duration(10 * time.Second), // default interval
652-
BaseEjectionTime: iserviceconfig.Duration(30 * time.Second),
653-
MaxEjectionTime: iserviceconfig.Duration(300 * time.Second),
654-
MaxEjectionPercent: 10,
655-
ChildPolicy: &iserviceconfig.BalancerConfig{
656-
Name: clusterimpl.Name,
657-
Config: &clusterimpl.LBConfig{
658-
Cluster: clusterName,
659-
EDSServiceName: serviceName,
660-
TelemetryLabels: xdsinternal.UnknownCSMLabels,
661-
LoadReportingServer: lrsServerCfg,
662-
ChildPolicy: &iserviceconfig.BalancerConfig{
663-
Name: wrrlocality.Name,
664-
Config: &wrrlocality.LBConfig{ChildPolicy: &iserviceconfig.BalancerConfig{Name: roundrobin.Name}},
665-
},
666-
},
667-
},
668-
},
669-
},
670-
IgnoreReresolutionRequests: true,
671-
},
672-
},
673-
Priorities: []string{"priority-0-0"},
674-
}
675-
676-
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
677-
defer cancel()
678-
if err := mgmtServer.Update(ctx, e2e.UpdateOptions{
679-
NodeID: nodeID,
680-
Listeners: []*v3listenerpb.Listener{e2e.DefaultClientListener(target, routeName)},
681-
Routes: []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig(routeName, target, clusterResource.Name)},
682-
Clusters: []*v3clusterpb.Cluster{clusterResource},
683-
Endpoints: []*v3endpointpb.ClusterLoadAssignment{e2e.DefaultEndpoint(serviceName, host, []uint32{port})},
684-
}); err != nil {
685-
t.Fatal(err)
686-
}
687-
688-
if err := compareLoadBalancingConfig(ctx, lbCfgCh, wantChildCfg); err != nil {
689-
t.Fatal(err)
690-
}
691-
}
692-
693615
// Tests scenarios for a bad cluster update received from the management server.
694616
//
695617
// - when a bad cluster resource update is received without any previous good

‎internal/xds/balancer/cdsbalancer/configbuilder.go‎

Lines changed: 6 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -162,11 +162,8 @@ func buildClusterImplConfigForDNS(g *nameGenerator, config *xdsresource.ClusterC
162162
pName := fmt.Sprintf("priority-%v", g.prefix)
163163
clusterUpdate := config.Cluster
164164
lbconfig := &clusterimpl.LBConfig{
165-
Cluster: clusterUpdate.ClusterName,
166-
TelemetryLabels: clusterUpdate.TelemetryLabels,
167-
ChildPolicy: xdsLBPolicy,
168-
MaxConcurrentRequests: clusterUpdate.MaxRequests,
169-
LoadReportingServer: clusterUpdate.LRSServerConfig,
165+
Cluster: clusterUpdate.ClusterName,
166+
ChildPolicy: xdsLBPolicy,
170167
}
171168
endpoints := config.EndpointConfig.DNSEndpoints.Endpoints
172169
if len(endpoints) == 0 {
@@ -203,13 +200,6 @@ func buildClusterImplConfigForDNS(g *nameGenerator, config *xdsresource.ClusterC
203200
// - p0 addresses' hierarchy attributes are set to p0
204201
func buildClusterImplConfigForEDS(g *nameGenerator, config *xdsresource.ClusterConfig, xdsLBPolicy *internalserviceconfig.BalancerConfig) ([]string, map[string]*clusterimpl.LBConfig, []resolver.Endpoint, error) {
205202
edsUpdate := config.EndpointConfig.EDSUpdate
206-
drops := make([]clusterimpl.DropConfig, 0, len(edsUpdate.Drops))
207-
for _, d := range edsUpdate.Drops {
208-
drops = append(drops, clusterimpl.DropConfig{
209-
Category: d.Category,
210-
RequestsPerMillion: d.Numerator * million / d.Denominator,
211-
})
212-
}
213203

214204
// Localities of length 0 is triggered by an NACK or resource-not-found
215205
// error before update, or an empty localities list in an update. In either
@@ -227,7 +217,7 @@ func buildClusterImplConfigForEDS(g *nameGenerator, config *xdsresource.ClusterC
227217
var retEndpoints []resolver.Endpoint
228218
for i, pName := range retNames {
229219
priorityLocalities := priorities[i]
230-
cfg, endpoints, err := priorityLocalitiesToClusterImpl(priorityLocalities, pName, *config.Cluster, drops, xdsLBPolicy)
220+
cfg, endpoints, err := priorityLocalitiesToClusterImpl(priorityLocalities, pName, *config.Cluster, xdsLBPolicy)
231221
if err != nil {
232222
return nil, nil, nil, err
233223
}
@@ -265,7 +255,7 @@ func groupLocalitiesByPriority(localities []xdsresource.Locality) [][]xdsresourc
265255
// priority), and generates a cluster impl policy config, and a list of
266256
// addresses with their path hierarchy set to [priority-name, locality-name], so
267257
// priority and the xDS LB Policy know which child policy each address is for.
268-
func priorityLocalitiesToClusterImpl(localities []xdsresource.Locality, priorityName string, clusterUpdate xdsresource.ClusterUpdate, drops []clusterimpl.DropConfig, xdsLBPolicy *internalserviceconfig.BalancerConfig) (*clusterimpl.LBConfig, []resolver.Endpoint, error) {
258+
func priorityLocalitiesToClusterImpl(localities []xdsresource.Locality, priorityName string, clusterUpdate xdsresource.ClusterUpdate, xdsLBPolicy *internalserviceconfig.BalancerConfig) (*clusterimpl.LBConfig, []resolver.Endpoint, error) {
269259
var retEndpoints []resolver.Endpoint
270260

271261
// Compute the sum of locality weights to normalize locality weights. The
@@ -327,13 +317,8 @@ func priorityLocalitiesToClusterImpl(localities []xdsresource.Locality, priority
327317
}
328318
}
329319
return &clusterimpl.LBConfig{
330-
Cluster: clusterUpdate.ClusterName,
331-
EDSServiceName: clusterUpdate.EDSServiceName,
332-
LoadReportingServer: clusterUpdate.LRSServerConfig,
333-
MaxConcurrentRequests: clusterUpdate.MaxRequests,
334-
TelemetryLabels: clusterUpdate.TelemetryLabels,
335-
DropCategories: drops,
336-
ChildPolicy: xdsLBPolicy,
320+
Cluster: clusterUpdate.ClusterName,
321+
ChildPolicy: xdsLBPolicy,
337322
}, retEndpoints, nil
338323
}
339324

0 commit comments

Comments
 (0)