diff --git a/internal/xds/balancer/balancer.go b/internal/xds/balancer/balancer.go index af3f999a1204..c61fd9dc0b59 100644 --- a/internal/xds/balancer/balancer.go +++ b/internal/xds/balancer/balancer.go @@ -25,7 +25,6 @@ import ( _ "google.golang.org/grpc/internal/xds/balancer/cdsbalancer" // Register the CDS balancer _ "google.golang.org/grpc/internal/xds/balancer/clusterimpl" // Register the xds_cluster_impl balancer _ "google.golang.org/grpc/internal/xds/balancer/clustermanager" // Register the xds_cluster_manager balancer - _ "google.golang.org/grpc/internal/xds/balancer/clusterresolver" // Register the xds_cluster_resolver balancer _ "google.golang.org/grpc/internal/xds/balancer/outlierdetection" // Register the outlier_detection balancer _ "google.golang.org/grpc/internal/xds/balancer/priority" // Register the priority balancer ) diff --git a/internal/xds/balancer/cdsbalancer/aggregate_cluster_test.go b/internal/xds/balancer/cdsbalancer/aggregate_cluster_test.go index 7e277f3e5a8c..19a9222d7e46 100644 --- a/internal/xds/balancer/cdsbalancer/aggregate_cluster_test.go +++ b/internal/xds/balancer/cdsbalancer/aggregate_cluster_test.go @@ -18,8 +18,6 @@ package cdsbalancer import ( "context" - "encoding/json" - "fmt" "strings" "testing" "time" @@ -31,8 +29,7 @@ import ( "google.golang.org/grpc/internal/stubserver" "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/internal/testutils/xds/e2e" - xdsinternal "google.golang.org/grpc/internal/xds" - "google.golang.org/grpc/internal/xds/balancer/clusterresolver" + "google.golang.org/grpc/internal/xds/balancer/priority" "google.golang.org/grpc/internal/xds/xdsclient/xdsresource/version" "google.golang.org/grpc/serviceconfig" "google.golang.org/grpc/status" @@ -45,6 +42,7 @@ import ( testgrpc "google.golang.org/grpc/interop/grpc_testing" testpb "google.golang.org/grpc/interop/grpc_testing" + _ "google.golang.org/grpc/balancer/pickfirst" _ "google.golang.org/grpc/internal/xds/httpfilter/router" // Register the router filter. _ "google.golang.org/grpc/internal/xds/resolver" // Register the xds resolver ) @@ -70,11 +68,11 @@ func makeLogicalDNSClusterResource(name, dnsHost string, dnsPort uint32) *v3clus }) } -// Tests the case where the cluster resource requested by the cds LB policy is a -// leaf cluster. The management server sends two updates for the same leaf -// cluster resource. The test verifies that the load balancing configuration -// pushed to the cluster_resolver LB policy contains the expected discovery -// mechanism corresponding to the leaf cluster, on both occasions. +// Tests the case where the cluster resource requested is a leaf cluster. The +// management server sends two updates for the same leaf cluster resource. The +// test verifies that the load balancing configuration pushed to the priority LB +// policy contains the expected discovery mechanism corresponding to the leaf +// cluster, on both occasions. func (s) TestAggregateClusterSuccess_LeafNode(t *testing.T) { tests := []struct { name string @@ -87,57 +85,43 @@ func (s) TestAggregateClusterSuccess_LeafNode(t *testing.T) { name: "eds", firstClusterResource: e2e.DefaultCluster(clusterName, serviceName, e2e.SecurityLevelNone), secondClusterResource: e2e.DefaultCluster(clusterName, serviceName+"-new", e2e.SecurityLevelNone), - wantFirstChildCfg: &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + wantFirstChildCfg: &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: createPriorityConfig(clusterName, serviceName), + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-0-0"}, }, - wantSecondChildCfg: &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName + "-new", - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + wantSecondChildCfg: &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-1-0": { + Config: createPriorityConfig(clusterName, serviceName+"-new"), + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-1-0"}, }, }, { name: "dns", firstClusterResource: makeLogicalDNSClusterResource(clusterName, "dns_host", uint32(port)), secondClusterResource: makeLogicalDNSClusterResource(clusterName, "dns_host_new", uint32(port)), - wantFirstChildCfg: &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterName, - Type: clusterresolver.DiscoveryMechanismTypeLogicalDNS, - DNSHostname: fmt.Sprintf("dns_host:%d", port), - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + wantFirstChildCfg: &priority.LBConfig{ + Children: map[string]*priority.Child{"priority-0": {Config: createPriorityConfig(clusterName, "")}}, + Priorities: []string{"priority-0"}, }, - wantSecondChildCfg: &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterName, - Type: clusterresolver.DiscoveryMechanismTypeLogicalDNS, - DNSHostname: fmt.Sprintf("dns_host_new:%d", port), - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + wantSecondChildCfg: &priority.LBConfig{ + Children: map[string]*priority.Child{"priority-1": {Config: createPriorityConfig(clusterName, "")}}, + Priorities: []string{"priority-1"}, }, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - lbCfgCh, _, _, _ := registerWrappedClusterResolverPolicy(t) + lbCfgCh, _, _, _ := registerWrappedPriorityPolicy(t) mgmtServer, nodeID, _ := setupWithManagementServer(t, nil, nil) // Push the first cluster resource through the management server and @@ -173,18 +157,17 @@ func (s) TestAggregateClusterSuccess_LeafNode(t *testing.T) { } } -// Tests the case where the cluster resource requested by the cds LB policy is -// an aggregate cluster root pointing to two child clusters, one of type EDS and -// the other of type LogicalDNS. The test verifies that load balancing -// configuration is pushed to the cluster_resolver LB policy only when all child -// clusters are resolved and it also verifies that the pushed configuration -// contains the expected discovery mechanisms. The test then updates the -// aggregate cluster to point to two child clusters, the same leaf cluster of -// type EDS and a different leaf cluster of type LogicalDNS and verifies that -// the load balancing configuration pushed to the cluster_resolver LB policy -// contains the expected discovery mechanisms. +// Tests the case where the cluster resource requested is an aggregate cluster +// root pointing to two child clusters, one of type EDS and the other of type +// LogicalDNS. The test verifies that load balancing configuration is pushed to +// the priority LB policy only when all child clusters are resolved and it also +// verifies that the pushed configuration contains the expected priority config. +// The test then updates the aggregate cluster to point to two child clusters, +// the same leaf cluster of type EDS and a different leaf cluster of type +// LogicalDNS and verifies that the load balancing configuration pushed to the +// priority LB policy contains the expected config. func (s) TestAggregateClusterSuccess_ThenUpdateChildClusters(t *testing.T) { - lbCfgCh, _, _, _ := registerWrappedClusterResolverPolicy(t) + lbCfgCh, _, _, _ := registerWrappedPriorityPolicy(t) mgmtServer, nodeID, _ := setupWithManagementServer(t, nil, nil) // Configure the management server with the aggregate cluster resource @@ -222,24 +205,15 @@ func (s) TestAggregateClusterSuccess_ThenUpdateChildClusters(t *testing.T) { t.Fatal(err) } - wantChildCfg := &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{ - { - Cluster: edsClusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }, - { - Cluster: dnsClusterName, - Type: clusterresolver.DiscoveryMechanismTypeLogicalDNS, - DNSHostname: fmt.Sprintf("%s:%d", dnsHostName, dnsPort), - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, + wantChildCfg := &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: createPriorityConfig(edsClusterName, serviceName), + IgnoreReresolutionRequests: true, }, + "priority-1": {Config: createPriorityConfig(dnsClusterName, "")}, }, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + Priorities: []string{"priority-0-0", "priority-1"}, } if err := compareLoadBalancingConfig(ctx, lbCfgCh, wantChildCfg); err != nil { t.Fatal(err) @@ -262,40 +236,31 @@ func (s) TestAggregateClusterSuccess_ThenUpdateChildClusters(t *testing.T) { if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) } - wantChildCfg = &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{ - { - Cluster: edsClusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }, - { - Cluster: dnsClusterNameNew, - Type: clusterresolver.DiscoveryMechanismTypeLogicalDNS, - DNSHostname: fmt.Sprintf("%s:%d", dnsHostNameNew, dnsPort), - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, + wantChildCfg = &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: createPriorityConfig(edsClusterName, serviceName), + IgnoreReresolutionRequests: true, }, + "priority-2": {Config: createPriorityConfig(dnsClusterNameNew, "")}, }, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + Priorities: []string{"priority-0-0", "priority-2"}, } if err := compareLoadBalancingConfig(ctx, lbCfgCh, wantChildCfg); err != nil { t.Fatal(err) } } -// Tests the case where the cluster resource requested by the cds LB policy is -// an aggregate cluster root pointing to two child clusters, one of type EDS and -// the other of type LogicalDNS. The test verifies that the load balancing -// configuration pushed to the cluster_resolver LB policy contains the discovery -// mechanisms for both child clusters. The test then updates the root cluster -// resource requested by the cds LB policy to a leaf cluster of type EDS and -// verifies the load balancing configuration pushed to the cluster_resolver LB -// policy contains a single discovery mechanism. +// Tests the case where the cluster resource requested is an aggregate cluster +// root pointing to two child clusters, one of type EDS and the other of type +// LogicalDNS. The test verifies that the load balancing configuration pushed to +// the priority LB policy contains the discovery mechanisms for both child +// clusters. The test then updates the root cluster resource requested by the +// cds LB policy to a leaf cluster of type EDS and verifies the load balancing +// configuration pushed to the priority LB policy contains a single discovery +// mechanism. func (s) TestAggregateClusterSuccess_ThenChangeRootToEDS(t *testing.T) { - lbCfgCh, _, _, _ := registerWrappedClusterResolverPolicy(t) + lbCfgCh, _, _, _ := registerWrappedPriorityPolicy(t) mgmtServer, nodeID, _ := setupWithManagementServer(t, nil, nil) // Configure the management server with the aggregate cluster resource @@ -317,24 +282,15 @@ func (s) TestAggregateClusterSuccess_ThenChangeRootToEDS(t *testing.T) { t.Fatal(err) } - wantChildCfg := &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{ - { - Cluster: edsClusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }, - { - Cluster: dnsClusterName, - Type: clusterresolver.DiscoveryMechanismTypeLogicalDNS, - DNSHostname: fmt.Sprintf("%s:%d", dnsHostName, dnsPort), - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, + wantChildCfg := &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: createPriorityConfig(edsClusterName, serviceName), + IgnoreReresolutionRequests: true, }, + "priority-1": {Config: createPriorityConfig(dnsClusterName, "")}, }, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + Priorities: []string{"priority-0-0", "priority-1"}, } if err := compareLoadBalancingConfig(ctx, lbCfgCh, wantChildCfg); err != nil { t.Fatal(err) @@ -353,15 +309,16 @@ func (s) TestAggregateClusterSuccess_ThenChangeRootToEDS(t *testing.T) { if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) } - wantChildCfg = &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + // Since the service name of the EDS cluster remains same, same priority name + // is used. + wantChildCfg = &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: createPriorityConfig(clusterName, serviceName), + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-0-0"}, } if err := compareLoadBalancingConfig(ctx, lbCfgCh, wantChildCfg); err != nil { t.Fatal(err) @@ -371,10 +328,9 @@ func (s) TestAggregateClusterSuccess_ThenChangeRootToEDS(t *testing.T) { // Tests the case where a requested cluster resource switches between being a // leaf and an aggregate cluster pointing to an EDS and LogicalDNS child // cluster. In each of these cases, the test verifies that the load balancing -// configuration pushed to the cluster_resolver LB policy contains the expected -// discovery mechanisms. +// configuration pushed to the priority LB policy contains the expected config. func (s) TestAggregatedClusterSuccess_SwitchBetweenLeafAndAggregate(t *testing.T) { - lbCfgCh, _, _, _ := registerWrappedClusterResolverPolicy(t) + lbCfgCh, _, _, _ := registerWrappedPriorityPolicy(t) mgmtServer, nodeID, _ := setupWithManagementServer(t, nil, nil) // Start off with the requested cluster being a leaf EDS cluster. @@ -390,15 +346,14 @@ func (s) TestAggregatedClusterSuccess_SwitchBetweenLeafAndAggregate(t *testing.T if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) } - wantChildCfg := &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + wantChildCfg := &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: createPriorityConfig(clusterName, serviceName), + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-0-0"}, } if err := compareLoadBalancingConfig(ctx, lbCfgCh, wantChildCfg); err != nil { t.Fatal(err) @@ -420,24 +375,15 @@ func (s) TestAggregatedClusterSuccess_SwitchBetweenLeafAndAggregate(t *testing.T if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) } - wantChildCfg = &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{ - { - Cluster: edsClusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }, - { - Cluster: dnsClusterName, - Type: clusterresolver.DiscoveryMechanismTypeLogicalDNS, - DNSHostname: fmt.Sprintf("%s:%d", dnsHostName, dnsPort), - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, + wantChildCfg = &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: createPriorityConfig(edsClusterName, serviceName), + IgnoreReresolutionRequests: true, }, + "priority-1": {Config: createPriorityConfig(dnsClusterName, "")}, }, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + Priorities: []string{"priority-0-0", "priority-1"}, } if err := compareLoadBalancingConfig(ctx, lbCfgCh, wantChildCfg); err != nil { t.Fatal(err) @@ -454,15 +400,14 @@ func (s) TestAggregatedClusterSuccess_SwitchBetweenLeafAndAggregate(t *testing.T if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) } - wantChildCfg = &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + wantChildCfg = &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: createPriorityConfig(clusterName, serviceName), + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-0-0"}, } if err := compareLoadBalancingConfig(ctx, lbCfgCh, wantChildCfg); err != nil { t.Fatal(err) @@ -562,11 +507,11 @@ func (s) TestAggregatedClusterFailure_ExceedsMaxStackDepth(t *testing.T) { } // Tests a diamond shaped aggregate cluster (A->[B,C]; B->D; C->D). Verifies -// that the load balancing configuration pushed to the cluster_resolver LB +// that the load balancing configuration pushed to the priority LB // policy specifies cluster D only once. Also verifies that configuration is // pushed only after all child clusters are resolved. func (s) TestAggregatedClusterSuccess_DiamondDependency(t *testing.T) { - lbCfgCh, _, _, _ := registerWrappedClusterResolverPolicy(t) + lbCfgCh, _, _, _ := registerWrappedPriorityPolicy(t) mgmtServer, nodeID, _ := setupWithManagementServer(t, nil, nil) // Configure the management server with an aggregate cluster resource having @@ -613,15 +558,14 @@ func (s) TestAggregatedClusterSuccess_DiamondDependency(t *testing.T) { t.Fatal(err) } - wantChildCfg := &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterNameD, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + wantChildCfg := &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: createPriorityConfig(clusterNameD, serviceName), + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-0-0"}, } if err := compareLoadBalancingConfig(ctx, lbCfgCh, wantChildCfg); err != nil { t.Fatal(err) @@ -630,12 +574,11 @@ func (s) TestAggregatedClusterSuccess_DiamondDependency(t *testing.T) { // Tests the case where the aggregate cluster graph contains duplicates (A->[B, // C]; B->[C, D]). Verifies that the load balancing configuration pushed to the -// cluster_resolver LB policy does not contain duplicates, and that the -// discovery mechanism corresponding to cluster C is of higher priority than the -// discovery mechanism for cluster D. Also verifies that the configuration is -// pushed only after all child clusters are resolved. +// priority LB policy does not contain duplicates, and that the priority +// corresponding to cluster C is higher than that for cluster D. Also verifies +// that the configuration is pushed only after all child clusters are resolved. func (s) TestAggregatedClusterSuccess_IgnoreDups(t *testing.T) { - lbCfgCh, _, _, _ := registerWrappedClusterResolverPolicy(t) + lbCfgCh, _, _, _ := registerWrappedPriorityPolicy(t) mgmtServer, nodeID, _ := setupWithManagementServer(t, nil, nil) // Configure the management server with an aggregate cluster resource that @@ -678,29 +621,23 @@ func (s) TestAggregatedClusterSuccess_IgnoreDups(t *testing.T) { // thereby completing the cluster graph. This should result in configuration // being pushed down to the child policy. resources.Clusters = append(resources.Clusters, e2e.DefaultCluster(clusterNameC, edsClusterName, e2e.SecurityLevelNone)) - resources.Endpoints = append(resources.Endpoints, e2e.DefaultEndpoint(edsClusterName, "eshita_eds", []uint32{1234})) + resources.Endpoints = append(resources.Endpoints, e2e.DefaultEndpoint(edsClusterName, "localhost", []uint32{1234})) if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) } - wantChildCfg := &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{ - { - Cluster: clusterNameC, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: edsClusterName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, + wantChildCfg := &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: createPriorityConfig(clusterNameC, edsClusterName), + IgnoreReresolutionRequests: true, }, - { - Cluster: clusterNameD, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, + "priority-1-0": { + Config: createPriorityConfig(clusterNameD, serviceName), + IgnoreReresolutionRequests: true, }, }, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + Priorities: []string{"priority-0-0", "priority-1-0"}, } if err := compareLoadBalancingConfig(ctx, lbCfgCh, wantChildCfg); err != nil { t.Fatal(err) @@ -716,7 +653,7 @@ func (s) TestAggregatedClusterSuccess_IgnoreDups(t *testing.T) { // where B is a leaf EDS cluster. Verifies that configuration is pushed to the // child policy and that an RPC can be successfully made. func (s) TestAggregatedCluster_NodeChildOfItself(t *testing.T) { - lbCfgCh, _, _, _ := registerWrappedClusterResolverPolicy(t) + lbCfgCh, _, _, _ := registerWrappedPriorityPolicy(t) mgmtServer, nodeID, cc := setupWithManagementServer(t, nil, nil) const ( @@ -777,15 +714,14 @@ func (s) TestAggregatedCluster_NodeChildOfItself(t *testing.T) { } // Verify the configuration pushed to the child policy. - wantChildCfg := &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterNameB, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + wantChildCfg := &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: createPriorityConfig(clusterNameB, serviceName), + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-0-0"}, } if err := compareLoadBalancingConfig(ctx, lbCfgCh, wantChildCfg); err != nil { t.Fatal(err) @@ -804,7 +740,7 @@ func (s) TestAggregatedCluster_NodeChildOfItself(t *testing.T) { // are expected to fail with code UNAVAILABLE and an error message specifying // that the aggregate cluster graph has no leaf clusters. func (s) TestAggregatedCluster_CycleWithNoLeafNode(t *testing.T) { - lbCfgCh, _, _, _ := registerWrappedClusterResolverPolicy(t) + lbCfgCh, _, _, _ := registerWrappedPriorityPolicy(t) mgmtServer, nodeID, cc := setupWithManagementServer(t, nil, nil) const ( @@ -854,7 +790,7 @@ func (s) TestAggregatedCluster_CycleWithNoLeafNode(t *testing.T) { // there is a leaf cluster in this graph , configuration should be pushed to the // child policy and RPCs should get routed to that leaf cluster. func (s) TestAggregatedCluster_CycleWithLeafNode(t *testing.T) { - lbCfgCh, _, _, _ := registerWrappedClusterResolverPolicy(t) + lbCfgCh, _, _, _ := registerWrappedPriorityPolicy(t) mgmtServer, nodeID, cc := setupWithManagementServer(t, nil, nil) // Start a test service backend. @@ -887,15 +823,14 @@ func (s) TestAggregatedCluster_CycleWithLeafNode(t *testing.T) { } // Verify the configuration pushed to the child policy. - wantChildCfg := &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterNameC, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + wantChildCfg := &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: createPriorityConfig(clusterNameC, serviceName), + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-0-0"}, } if err := compareLoadBalancingConfig(ctx, lbCfgCh, wantChildCfg); err != nil { t.Fatal(err) diff --git a/internal/xds/balancer/cdsbalancer/cdsbalancer.go b/internal/xds/balancer/cdsbalancer/cdsbalancer.go index c00a81b98269..a73d5ff46c5c 100644 --- a/internal/xds/balancer/cdsbalancer/cdsbalancer.go +++ b/internal/xds/balancer/cdsbalancer/cdsbalancer.go @@ -25,6 +25,7 @@ import ( "sync/atomic" "unsafe" + "google.golang.org/grpc/attributes" "google.golang.org/grpc/balancer" "google.golang.org/grpc/balancer/base" "google.golang.org/grpc/connectivity" @@ -33,34 +34,29 @@ import ( "google.golang.org/grpc/internal/balancer/nop" xdsinternal "google.golang.org/grpc/internal/credentials/xds" "google.golang.org/grpc/internal/grpclog" - "google.golang.org/grpc/internal/grpcsync" "google.golang.org/grpc/internal/pretty" - "google.golang.org/grpc/internal/xds/balancer/clusterresolver" + internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" + "google.golang.org/grpc/internal/xds/balancer/outlierdetection" + "google.golang.org/grpc/internal/xds/balancer/priority" "google.golang.org/grpc/internal/xds/xdsclient" "google.golang.org/grpc/internal/xds/xdsclient/xdsresource" + "google.golang.org/grpc/internal/xds/xdsdepmgr" "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" ) -const ( - cdsName = "cds_experimental" - aggregateClusterMaxDepth = 16 -) +const cdsName = "cds_experimental" var ( - errBalancerClosed = fmt.Errorf("cds_experimental LB policy is closed") - errExceedsMaxDepth = fmt.Errorf("aggregate cluster graph exceeds max depth (%d)", aggregateClusterMaxDepth) - - // newChildBalancer is a helper function to build a new cluster_resolver - // balancer and will be overridden in unittests. + // newChildBalancer is a helper function to build a new priority balancer + // and will be overridden in unittests. newChildBalancer = func(cc balancer.ClientConn, opts balancer.BuildOptions) (balancer.Balancer, error) { - builder := balancer.Get(clusterresolver.Name) + builder := balancer.Get(priority.Name) if builder == nil { - return nil, fmt.Errorf("xds: no balancer builder with name %v", clusterresolver.Name) + return nil, fmt.Errorf("xds: no balancer builder with name %v", priority.Name) } - // We directly pass the parent clientConn to the underlying - // cluster_resolver balancer because the cdsBalancer does not deal with - // subConns. + // We directly pass the parent clientConn to the underlying priority + // balancer because the cdsBalancer does not deal with subConns. return builder.Build(cc, opts), nil } buildProvider = buildProviderFunc @@ -81,30 +77,28 @@ type bb struct{} // Build creates a new CDS balancer with the ClientConn. func (bb) Build(cc balancer.ClientConn, opts balancer.BuildOptions) balancer.Balancer { - builder := balancer.Get(clusterresolver.Name) + builder := balancer.Get(priority.Name) if builder == nil { - // Shouldn't happen, registered through imported Cluster Resolver, + // Shouldn't happen, registered through imported Priority builder. Still, // defensive programming. - logger.Errorf("%q LB policy is needed but not registered", clusterresolver.Name) - return nop.NewBalancer(cc, fmt.Errorf("%q LB policy is needed but not registered", clusterresolver.Name)) + logger.Errorf("%q LB policy is needed but not registered", priority.Name) + return nop.NewBalancer(cc, fmt.Errorf("%q LB policy is needed but not registered", priority.Name)) } parser, ok := builder.(balancer.ConfigParser) if !ok { - // Shouldn't happen, imported Cluster Resolver builder has this method. - logger.Errorf("%q LB policy does not implement a config parser", clusterresolver.Name) - return nop.NewBalancer(cc, fmt.Errorf("%q LB policy does not implement a config parser", clusterresolver.Name)) + // Shouldn't happen, imported Priority builder has this method. + logger.Errorf("%q LB policy does not implement a config parser", priority.Name) + return nop.NewBalancer(cc, fmt.Errorf("%q LB policy does not implement a config parser", priority.Name)) } - ctx, cancel := context.WithCancel(context.Background()) hi := xdsinternal.NewHandshakeInfo(nil, nil, nil, false) xdsHIPtr := unsafe.Pointer(hi) b := &cdsBalancer{ bOpts: opts, childConfigParser: parser, - serializer: grpcsync.NewCallbackSerializer(ctx), - serializerCancel: cancel, xdsHIPtr: &xdsHIPtr, - watchers: make(map[string]*watcherState), + clusterConfigs: make(map[string]*xdsresource.ClusterResult), + priorityConfigs: make(map[string]*priorityConfig), } b.logger = prefixLogger(b) b.ccw = &ccWrapper{ @@ -168,16 +162,26 @@ type cdsBalancer struct { xdsHIPtr *unsafe.Pointer // Accessed atomically. - // The serializer and its cancel func are initialized at build time, and the - // rest of the fields here are only accessed from serializer callbacks (or - // from balancer.Balancer methods, which themselves are guaranteed to be - // mutually exclusive) and hence do not need to be guarded by a mutex. - serializer *grpcsync.CallbackSerializer // Serializes updates from gRPC and xDS client. - serializerCancel context.CancelFunc // Stops the above serializer. - childLB balancer.Balancer // Child policy, built upon resolution of the cluster graph. - xdsClient xdsclient.XDSClient // xDS client to watch Cluster resources. - watchers map[string]*watcherState // Set of watchers and associated state, keyed by cluster name. - lbCfg *lbConfig // Current load balancing configuration. + // All fields below are accessed only from methods implementing the + // balancer.Balancer interface. Since gRPC guarantees that these methods are + // never invoked concurrently, no additional synchronization is required to + // protect access to these fields. + xdsClient xdsclient.XDSClient + childLB balancer.Balancer // Child policy, built upon resolution of the cluster graph. + clusterConfigs map[string]*xdsresource.ClusterResult // Cluster name to the last received result for that cluster. + priorityConfigs map[string]*priorityConfig // Hostname to priority config for that leaf cluster. + lbCfg *lbConfig // Current load balancing configuration. + priorities []*priorityConfig // List of priorities in the order. + unsubscribe func() // For dynamic cluster unsubscription. + isSubscribed bool // True if a dynamic cluster has been subscribed to. + clusterSubscriber xdsdepmgr.ClusterSubscriber // To subscribe to dynamic cluster resource. + xdsLBPolicy internalserviceconfig.BalancerConfig // Stores the locality and endpoint picking policy. + attributes *attributes.Attributes // Attributes from resolver state. + serviceConfig *serviceconfig.ParseResult + // Each new leaf cluster needs a child name generator to reuse child policy + // names. But to make sure the names across leaf clusters doesn't conflict, + // we need a seq ID. This ID is incremented for each new cluster. + childNameGeneratorSeqID uint64 // The certificate providers are cached here to that they can be closed when // a new provider is to be created. @@ -189,8 +193,6 @@ type cdsBalancer struct { // management server, creates appropriate certificate provider plugins, and // updates the HandshakeInfo which is added as an address attribute in // NewSubConn() calls. -// -// Only executed in the context of a serializer callback. func (b *cdsBalancer) handleSecurityConfig(config *xdsresource.SecurityConfig) error { // If xdsCredentials are not in use, i.e, the user did not want to get // security configuration from an xDS server, we should not be acting on the @@ -273,24 +275,11 @@ func buildProviderFunc(configs map[string]*certprovider.BuildableConfig, instanc return provider, nil } -// A convenience method to create a watcher for cluster `name`. It also -// registers the watch with the xDS client, and adds the newly created watcher -// to the list of watchers maintained by the LB policy. -func (b *cdsBalancer) createAndAddWatcherForCluster(name string) { - w := &clusterWatcher{ - name: name, - parent: b, - } - ws := &watcherState{ - watcher: w, - cancelWatch: xdsresource.WatchCluster(b.xdsClient, name, w), - } - b.watchers[name] = ws -} - -// UpdateClientConnState receives the serviceConfig (which contains the -// clusterName to watch for in CDS) and the xdsClient object from the -// xdsResolver. +// UpdateClientConnState receives the serviceConfig, xdsConfig, +// ClusterSubscriber and the xdsClient object from the xdsResolver. If an error +// is encountered, the parent (clustermanager) sets the corresponding cluster’s +// picker to transient_failure. Otherwise, the received configuration is +// processed and forwarded to the appropriate child policy. func (b *cdsBalancer) UpdateClientConnState(state balancer.ClientConnState) error { if b.xdsClient == nil { c := xdsclient.FromResolverState(state.ResolverState) @@ -302,6 +291,17 @@ func (b *cdsBalancer) UpdateClientConnState(state balancer.ClientConnState) erro } b.logger.Infof("Received balancer config update: %s", pretty.ToJSON(state.BalancerConfig)) + xdsConfig := xdsresource.XDSConfigFromResolverState(state.ResolverState) + if xdsConfig == nil { + b.logger.Warningf("Received balancer config with no xDS config") + return balancer.ErrBadResolverState + } + b.clusterConfigs = xdsConfig.Clusters + b.clusterSubscriber = xdsdepmgr.XDSClusterSubscriberFromResolverState(state.ResolverState) + if b.clusterSubscriber == nil { + b.logger.Warningf("Received balancer config with no cluster subscriber") + return balancer.ErrBadResolverState + } // The errors checked here should ideally never happen because the // ServiceConfig in this case is prepared by the xdsResolver and is not // something that is received on the wire. @@ -315,74 +315,220 @@ func (b *cdsBalancer) UpdateClientConnState(state balancer.ClientConnState) erro return balancer.ErrBadResolverState } - // Do nothing and return early if configuration has not changed. - if b.lbCfg != nil && b.lbCfg.ClusterName == lbCfg.ClusterName { + b.lbCfg = lbCfg + b.serviceConfig = state.ResolverState.ServiceConfig + b.attributes = state.ResolverState.Attributes + return b.handleXDSConfigUpdate() +} + +// handleXDSConfigUpdate processes the XDSConfig update from the xDS resolver. +func (b *cdsBalancer) handleXDSConfigUpdate() error { + clusterName := b.lbCfg.ClusterName + + // If the cluster is dynamic and we dont have a subscription yet, create + // one. + if b.lbCfg.IsDynamic && !b.isSubscribed { + b.unsubscribe = b.clusterSubscriber.SubscribeToCluster(clusterName) + b.isSubscribed = true return nil } - b.lbCfg = lbCfg - // Handle the update in a blocking fashion. - errCh := make(chan error, 1) - callback := func(context.Context) { - // A config update with a changed top-level cluster name means that none - // of our old watchers make any sense any more. - b.closeAllWatchers() - - // Create a new watcher for the top-level cluster. Upon resolution, it - // could end up creating more watchers if turns out to be an aggregate - // cluster. - b.createAndAddWatcherForCluster(lbCfg.ClusterName) - errCh <- nil - } - onFailure := func() { - // The call to Schedule returns false *only* if the serializer has been - // closed, which happens only when we receive an update after close. - errCh <- errBalancerClosed - } - b.serializer.ScheduleOr(callback, onFailure) - return <-errCh + clusterUpdate, ok := b.clusterConfigs[clusterName] + if !ok { + // If the cluster is missing from the config, check if it is dynamic. + // For dynamic clusters, the xDS config may be updated before the + // corresponding cluster resource is received. This should never occur + // for static clusters. + if b.lbCfg.IsDynamic { + return nil + } + return b.annotateErrorWithNodeID(fmt.Errorf("did not find the cluster %q in XDSConfig", clusterName)) + } + // If the cluster resource has an error, return the error. + if clusterUpdate.Err != nil { + return clusterUpdate.Err + } + + if err := b.handleSecurityConfig(clusterUpdate.Config.Cluster.SecurityCfg); err != nil { + // If the security config is invalid, for example, if the provider + // instance is not found in the bootstrap config, we need to put the + // channel in transient failure. + return b.annotateErrorWithNodeID(fmt.Errorf("received Cluster resource that contains invalid security config: %v", err)) + + } + return b.handleClusterUpdate() } -// ResolverError handles errors reported by the xdsResolver. -func (b *cdsBalancer) ResolverError(err error) { - b.serializer.TrySchedule(func(context.Context) { - // Missing Listener or RouteConfiguration on the management server - // results in a 'resource not found' error from the xDS resolver. In - // these cases, we should stap watching all of the current clusters - // being watched. - if xdsresource.ErrType(err) == xdsresource.ErrorTypeResourceNotFound { - b.closeAllWatchers() - b.closeChildPolicyAndReportTF(err) - return +// handleClusterUpdate handles a good XDSConfig update from the xDS resolver. +// Builds the child policy config and pushes it down. +func (b *cdsBalancer) handleClusterUpdate() error { + clusterName := b.lbCfg.ClusterName + clusterConfig := b.clusterConfigs[clusterName].Config + + var newPriorities []*priorityConfig + switch clusterConfig.Cluster.ClusterType { + case xdsresource.ClusterTypeEDS, xdsresource.ClusterTypeLogicalDNS: + p := b.updatePriorityConfig(clusterName, &clusterConfig) + newPriorities = append(newPriorities, p) + case xdsresource.ClusterTypeAggregate: + for _, leaf := range clusterConfig.AggregateConfig.LeafClusters { + leafCluster := b.clusterConfigs[leaf] + // Update priority config for leaf clusters. + p := b.updatePriorityConfig(leaf, &leafCluster.Config) + newPriorities = append(newPriorities, p) } - var root string - if b.lbCfg != nil { - root = b.lbCfg.ClusterName + } + b.priorities = newPriorities + + if err := b.updateOutlierDetection(); err != nil { + return b.annotateErrorWithNodeID(fmt.Errorf("failed to correctly update Outlier Detection config %v", err)) + } + + // The LB policy is configured by the root cluster. + if err := json.Unmarshal(clusterConfig.Cluster.LBPolicy, &b.xdsLBPolicy); err != nil { + return b.annotateErrorWithNodeID(fmt.Errorf("error unmarshalling xDS LB Policy: %v", err)) + } + if err := b.updateChildConfig(); err != nil { + return b.annotateErrorWithNodeID(err) + } + return nil +} + +// updateChildConfig builds child policy configuration using endpoint addresses +// returned from the XDSConfig and child policy configuration. +// +// A child policy is created if one doesn't already exist. The newly built +// configuration is then pushed to the child policy. +func (b *cdsBalancer) updateChildConfig() error { + if b.childLB == nil { + childLB, err := newChildBalancer(b.ccw, b.bOpts) + if err != nil { + return fmt.Errorf("failed to create child policy of type %s: %v", priority.Name, err) } - b.onClusterError(root, err) - }) + b.childLB = childLB + } + + childCfgBytes, endpoints, err := buildPriorityConfigJSON(b.priorities, &b.xdsLBPolicy) + if err != nil { + return fmt.Errorf("failed to build child policy config: %v", err) + } + childCfg, err := b.childConfigParser.ParseConfig(childCfgBytes) + if err != nil { + return fmt.Errorf("failed to parse child policy config. This should never happen because the config was generated: %v", err) + } + if b.logger.V(2) { + b.logger.Infof("Built child policy config: %s", pretty.ToJSON(childCfg)) + } + + for i := range endpoints { + for j := range endpoints[i].Addresses { + addr := endpoints[i].Addresses[j] + addr.BalancerAttributes = endpoints[i].Attributes + // BalancerAttributes need to be present in endpoint addresses. This + // temporary workaround is required to make load reporting work + // with the old pickfirst policy which creates SubConns with multiple + // addresses. Since the addresses can be from different localities, + // an Address.BalancerAttribute is used to identify the locality of the + // address used by the transport. This workaround can be removed once + // the old pickfirst is removed. + // See https://github.com/grpc/grpc-go/issues/7339 + endpoints[i].Addresses[j] = addr + } + } + if err := b.childLB.UpdateClientConnState(balancer.ClientConnState{ + ResolverState: resolver.State{ + Endpoints: endpoints, + ServiceConfig: b.serviceConfig, + Attributes: b.attributes, + }, + BalancerConfig: childCfg, + }); err != nil { + return fmt.Errorf("failed to push config to child policy: %v", err) + } + return nil } -// UpdateSubConnState handles subConn updates from gRPC. -func (b *cdsBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.SubConnState) { - b.logger.Errorf("UpdateSubConnState(%v, %+v) called unexpectedly", sc, state) +// updatePriorityConfig updates the priority configuration for the specified EDS +// or DNS cluster, creating it if it does not already exist. +func (b *cdsBalancer) updatePriorityConfig(clusterName string, clusterConfig *xdsresource.ClusterConfig) *priorityConfig { + name := hostName(clusterName, *clusterConfig.Cluster) + pc, ok := b.priorityConfigs[name] + if !ok { + pc = &priorityConfig{ + childNameGen: newNameGenerator(b.childNameGeneratorSeqID), + } + b.priorityConfigs[name] = pc + // Increment the seq ID for the next new cluster. This is done to make + // sure that the child policy names generated for different clusters + // don't conflict with each other. + b.childNameGeneratorSeqID++ + } + pc.clusterConfig = clusterConfig + return pc } -// Closes all registered cluster watchers and removes them from the internal map. -// -// Only executed in the context of a serializer callback. -func (b *cdsBalancer) closeAllWatchers() { - for name, state := range b.watchers { - state.cancelWatch() - delete(b.watchers, name) +// updateOutlierDetection updates Outlier Detection config for all priorities. +func (b *cdsBalancer) updateOutlierDetection() error { + odBuilder := balancer.Get(outlierdetection.Name) + if odBuilder == nil { + // Shouldn't happen, registered through imported Outlier Detection, + // defensive programming. + return fmt.Errorf("%q LB policy is needed but not registered", outlierdetection.Name) + } + + odParser, ok := odBuilder.(balancer.ConfigParser) + if !ok { + // Shouldn't happen, imported Outlier Detection builder has this method. + return fmt.Errorf("%q LB policy does not implement a config parser", outlierdetection.Name) } + + for _, p := range b.priorities { + // Update Outlier Detection Config. + odJSON := p.clusterConfig.Cluster.OutlierDetection + if odJSON == nil { + odJSON = json.RawMessage(`{}`) + } + + lbCfg, err := odParser.ParseConfig(odJSON) + if err != nil { + return fmt.Errorf("error parsing Outlier Detection config %v: %v", odJSON, err) + } + + odCfg, ok := lbCfg.(*outlierdetection.LBConfig) + if !ok { + // Shouldn't happen, Parser built at build time with Outlier + // Detection builder pulled from gRPC LB Registry. + return fmt.Errorf("config parser for Outlier Detection returned config with unexpected type %T: %v", lbCfg, lbCfg) + } + p.outlierDetection = *odCfg + } + return nil +} + +// ResolverError handles errors reported by the xdsResolver. +func (b *cdsBalancer) ResolverError(err error) { + // Missing Listener or RouteConfiguration on the management server + // results in a 'resource not found' error from the xDS resolver. In + // these cases, we should report transient failure. + if xdsresource.ErrType(err) == xdsresource.ErrorTypeResourceNotFound { + b.closeChildPolicyAndReportTF(err) + return + } + var root string + if b.lbCfg != nil { + root = b.lbCfg.ClusterName + } + b.onClusterError(root, err) +} + +// UpdateSubConnState handles subConn updates from gRPC. +func (b *cdsBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.SubConnState) { + b.logger.Errorf("UpdateSubConnState(%v, %+v) called unexpectedly", sc, state) } // closeChildPolicyAndReportTF closes the child policy, if it exists, and // updates the connectivity state of the channel to TransientFailure with an // error picker. -// -// Only executed in the context of a serializer callback. func (b *cdsBalancer) closeChildPolicyAndReportTF(err error) { if b.childLB != nil { b.childLB.Close() @@ -394,40 +540,35 @@ func (b *cdsBalancer) closeChildPolicyAndReportTF(err error) { }) } -// Close cancels the CDS watch, closes the child policy and closes the -// cdsBalancer. +// Close closes the child policy, unsubscribes to the dynamic cluster, and +// closes the cdsBalancer. func (b *cdsBalancer) Close() { - b.serializer.TrySchedule(func(context.Context) { - b.closeAllWatchers() - - if b.childLB != nil { - b.childLB.Close() - b.childLB = nil - } - if b.cachedRoot != nil { - b.cachedRoot.Close() - } - if b.cachedIdentity != nil { - b.cachedIdentity.Close() - } - b.logger.Infof("Shutdown") - }) - b.serializerCancel() - <-b.serializer.Done() + if b.childLB != nil { + b.childLB.Close() + b.childLB = nil + } + if b.cachedRoot != nil { + b.cachedRoot.Close() + } + if b.cachedIdentity != nil { + b.cachedIdentity.Close() + } + if b.unsubscribe != nil { + b.unsubscribe() + } + b.logger.Infof("Shutdown") } func (b *cdsBalancer) ExitIdle() { - b.serializer.TrySchedule(func(context.Context) { - if b.childLB == nil { - b.logger.Warningf("Received ExitIdle with no child policy") - return - } - // This implementation assumes the child balancer supports - // ExitIdle (but still checks for the interface's existence to - // avoid a panic if not). If the child does not, no subconns - // will be connected. - b.childLB.ExitIdle() - }) + if b.childLB == nil { + b.logger.Warningf("Received ExitIdle with no child policy") + return + } + // This implementation assumes the child balancer supports + // ExitIdle (but still checks for the interface's existence to + // avoid a panic if not). If the child does not, no subconns + // will be connected. + b.childLB.ExitIdle() } // Node ID needs to be manually added to errors generated in the following @@ -445,105 +586,8 @@ func (b *cdsBalancer) annotateErrorWithNodeID(err error) error { return fmt.Errorf("[xDS node id: %v]: %w", nodeID, err) } -// Handles a good Cluster update from the xDS client. Kicks off the discovery -// mechanism generation process from the top-level cluster and if the cluster -// graph is resolved, generates child policy config and pushes it down. -// -// Only executed in the context of a serializer callback. -func (b *cdsBalancer) onClusterUpdate(name string, update *xdsresource.ClusterUpdate) { - state := b.watchers[name] - if state == nil { - // We are currently not watching this cluster anymore. Return early. - return - } - - b.logger.Infof("Received Cluster resource: %s", pretty.ToJSON(update)) - - // Update the watchers map with the update for the cluster. - state.lastUpdate = update - - // For an aggregate cluster, always use the security configuration on the - // root cluster. - if name == b.lbCfg.ClusterName { - // Process the security config from the received update before building the - // child policy or forwarding the update to it. We do this because the child - // policy may try to create a new subConn inline. Processing the security - // configuration here and setting up the handshakeInfo will make sure that - // such attempts are handled properly. - if err := b.handleSecurityConfig(update.SecurityCfg); err != nil { - // If the security config is invalid, for example, if the provider - // instance is not found in the bootstrap config, we need to put the - // channel in transient failure. - b.onClusterError(name, b.annotateErrorWithNodeID(fmt.Errorf("received Cluster resource contains invalid security config: %v", err))) - return - } - } - - clustersSeen := make(map[string]bool) - dms, ok, err := b.generateDMsForCluster(b.lbCfg.ClusterName, 0, nil, clustersSeen) - if err != nil { - b.onClusterError(b.lbCfg.ClusterName, b.annotateErrorWithNodeID(fmt.Errorf("failed to generate discovery mechanisms: %v", err))) - return - } - if ok { - if len(dms) == 0 { - b.onClusterError(b.lbCfg.ClusterName, b.annotateErrorWithNodeID(fmt.Errorf("aggregate cluster graph has no leaf clusters"))) - return - } - // Child policy is built the first time we resolve the cluster graph. - if b.childLB == nil { - childLB, err := newChildBalancer(b.ccw, b.bOpts) - if err != nil { - b.logger.Errorf("Failed to create child policy of type %s: %v", clusterresolver.Name, err) - return - } - b.childLB = childLB - b.logger.Infof("Created child policy %p of type %s", b.childLB, clusterresolver.Name) - } - - // Prepare the child policy configuration, convert it to JSON, have it - // parsed by the child policy to convert it into service config and push - // an update to it. - childCfg := &clusterresolver.LBConfig{ - DiscoveryMechanisms: dms, - // The LB policy is configured by the root cluster. - XDSLBPolicy: b.watchers[b.lbCfg.ClusterName].lastUpdate.LBPolicy, - } - cfgJSON, err := json.Marshal(childCfg) - if err != nil { - // Shouldn't happen, since we just prepared struct. - b.logger.Errorf("cds_balancer: error marshalling prepared config: %v", childCfg) - return - } - - var sc serviceconfig.LoadBalancingConfig - if sc, err = b.childConfigParser.ParseConfig(cfgJSON); err != nil { - b.logger.Errorf("cds_balancer: cluster_resolver config generated %v is invalid: %v", string(cfgJSON), err) - return - } - - ccState := balancer.ClientConnState{ - ResolverState: xdsclient.SetClient(resolver.State{}, b.xdsClient), - BalancerConfig: sc, - } - if err := b.childLB.UpdateClientConnState(ccState); err != nil { - b.logger.Errorf("Encountered error when sending config {%+v} to child policy: %v", ccState, err) - } - } - // We no longer need the clusters that we did not see in this iteration of - // generateDMsForCluster(). - for cluster, state := range b.watchers { - if !clustersSeen[cluster] { - state.cancelWatch() - delete(b.watchers, cluster) - } - } -} - -// Handles an ambient error Cluster update from the xDS client to not stop -// using the previously seen resource. -// -// Only executed in the context of a serializer callback. +// onClusterAmbientError handles an ambient error, if a childLB already has a +// good update, it should continue using that. func (b *cdsBalancer) onClusterAmbientError(name string, err error) { b.logger.Warningf("Cluster resource %q received ambient error update: %v", name, err) @@ -554,116 +598,14 @@ func (b *cdsBalancer) onClusterAmbientError(name string, err error) { } } -// Handles an error Cluster update from the xDS client to stop using the -// previously seen resource. Propagates the error down to the child policy -// if one exists, and puts the channel in TRANSIENT_FAILURE. -// -// Only executed in the context of a serializer callback. +// onClusterResourceError handles errors to stop using the previously seen +// resource. Propagates the error down to the child policy if one exists, and +// puts the channel in TRANSIENT_FAILURE. func (b *cdsBalancer) onClusterResourceError(name string, err error) { b.logger.Warningf("CDS watch for resource %q reported resource error", name) b.closeChildPolicyAndReportTF(err) } -// Generates discovery mechanisms for the cluster graph rooted at `name`. This -// method is called recursively if `name` corresponds to an aggregate cluster, -// with the base case for recursion being a leaf cluster. If a new cluster is -// encountered when traversing the graph, a watcher is created for it. -// -// Inputs: -// - name: name of the cluster to start from -// - depth: recursion depth of the current cluster, starting from root -// - dms: prioritized list of current discovery mechanisms -// - clustersSeen: cluster names seen so far in the graph traversal -// -// Outputs: -// - new prioritized list of discovery mechanisms -// - boolean indicating if traversal of the aggregate cluster graph is -// complete. If false, the above list of discovery mechanisms is ignored. -// - error indicating if any error was encountered as part of the graph -// traversal. If error is non-nil, the other return values are ignored. -// -// Only executed in the context of a serializer callback. -func (b *cdsBalancer) generateDMsForCluster(name string, depth int, dms []clusterresolver.DiscoveryMechanism, clustersSeen map[string]bool) ([]clusterresolver.DiscoveryMechanism, bool, error) { - if depth >= aggregateClusterMaxDepth { - return dms, false, errExceedsMaxDepth - } - - if clustersSeen[name] { - // Discovery mechanism already seen through a different branch. - return dms, true, nil - } - clustersSeen[name] = true - - state, ok := b.watchers[name] - if !ok { - // If we have not seen this cluster so far, create a watcher for it, add - // it to the map, start the watch and return. - b.createAndAddWatcherForCluster(name) - - // And since we just created the watcher, we know that we haven't - // resolved the cluster graph yet. - return dms, false, nil - } - - // A watcher exists, but no update has been received yet. - if state.lastUpdate == nil { - return dms, false, nil - } - - var dm clusterresolver.DiscoveryMechanism - cluster := state.lastUpdate - switch cluster.ClusterType { - case xdsresource.ClusterTypeAggregate: - // This boolean is used to track if any of the clusters in the graph is - // not yet completely resolved or returns errors, thereby allowing us to - // traverse as much of the graph as possible (and start the associated - // watches where required) to ensure that clustersSeen contains all - // clusters in the graph that we can traverse to. - missingCluster := false - var err error - for _, child := range cluster.PrioritizedClusterNames { - var ok bool - dms, ok, err = b.generateDMsForCluster(child, depth+1, dms, clustersSeen) - if err != nil || !ok { - missingCluster = true - } - } - return dms, !missingCluster, err - case xdsresource.ClusterTypeEDS: - dm = clusterresolver.DiscoveryMechanism{ - Type: clusterresolver.DiscoveryMechanismTypeEDS, - Cluster: cluster.ClusterName, - EDSServiceName: cluster.EDSServiceName, - MaxConcurrentRequests: cluster.MaxRequests, - LoadReportingServer: cluster.LRSServerConfig, - } - case xdsresource.ClusterTypeLogicalDNS: - dm = clusterresolver.DiscoveryMechanism{ - Type: clusterresolver.DiscoveryMechanismTypeLogicalDNS, - Cluster: cluster.ClusterName, - DNSHostname: cluster.DNSHostName, - MaxConcurrentRequests: cluster.MaxRequests, - LoadReportingServer: cluster.LRSServerConfig, - } - } - odJSON := cluster.OutlierDetection - // "In the cds LB policy, if the outlier_detection field is not set in - // the Cluster resource, a "no-op" outlier_detection config will be - // generated in the corresponding DiscoveryMechanism config, with all - // fields unset." - A50 - if odJSON == nil { - // This will pick up top level defaults in Cluster Resolver - // ParseConfig, but sre and fpe will be nil still so still a - // "no-op" config. - odJSON = json.RawMessage(`{}`) - } - dm.OutlierDetection = odJSON - - dm.TelemetryLabels = cluster.TelemetryLabels - - return append(dms, dm), true, nil -} - func (b *cdsBalancer) onClusterError(name string, err error) { if b.childLB != nil { b.onClusterAmbientError(name, err) diff --git a/internal/xds/balancer/cdsbalancer/cdsbalancer_security_test.go b/internal/xds/balancer/cdsbalancer/cdsbalancer_security_test.go index 5ee152208eba..8e0f24236b54 100644 --- a/internal/xds/balancer/cdsbalancer/cdsbalancer_security_test.go +++ b/internal/xds/balancer/cdsbalancer/cdsbalancer_security_test.go @@ -23,7 +23,6 @@ import ( "encoding/json" "fmt" "os" - "strings" "testing" "github.com/google/uuid" @@ -552,14 +551,8 @@ func (s) TestSecurityConfigUpdate_GoodToFallback(t *testing.T) { // bootstrap file contents. Verifies that the connection between the client and // the server is secure. Subsequently, the cds LB policy receives a cluster // resource that is NACKed by the xDS client. Test verifies that the cds LB -// policy continues to use the previous good configuration, but the error from -// the xDS client is propagated to the child policy. +// policy continues to use the previous good configuration. func (s) TestSecurityConfigUpdate_GoodToBad(t *testing.T) { - // Register a wrapped clusterresolver LB policy (child policy of the cds LB - // policy) for the duration of this test that makes the resolver error - // pushed to it available to the test. - _, resolverErrCh, _, _ := registerWrappedClusterResolverPolicy(t) - // Spin up an xDS management server. mgmtServer := e2e.StartManagementServer(t, e2e.ManagementServerOptions{}) @@ -615,16 +608,6 @@ func (s) TestSecurityConfigUpdate_GoodToBad(t *testing.T) { t.Fatal(err) } - const wantNACKErr = "instance name \"unknown-certificate-provider-instance\" missing in bootstrap configuration" - select { - case err := <-resolverErrCh: - if !strings.Contains(err.Error(), wantNACKErr) { - t.Fatalf("Child policy got resolver error: %v, want err: %v", err, wantNACKErr) - } - case <-ctx.Done(): - t.Fatal("Timeout when waiting for resolver error to be pushed to the child policy") - } - // Verify that a successful RPC can be made over a secure connection. if _, err := client.EmptyCall(ctx, &testpb.Empty{}, grpc.WaitForReady(true)); err != nil { t.Fatalf("EmptyCall() failed: %v", err) diff --git a/internal/xds/balancer/cdsbalancer/cdsbalancer_test.go b/internal/xds/balancer/cdsbalancer/cdsbalancer_test.go index 32d74dd56902..174ab8c1b597 100644 --- a/internal/xds/balancer/cdsbalancer/cdsbalancer_test.go +++ b/internal/xds/balancer/cdsbalancer/cdsbalancer_test.go @@ -36,14 +36,18 @@ import ( "google.golang.org/grpc/internal" "google.golang.org/grpc/internal/balancer/stub" "google.golang.org/grpc/internal/grpctest" + iringhash "google.golang.org/grpc/internal/ringhash" + iserviceconfig "google.golang.org/grpc/internal/serviceconfig" "google.golang.org/grpc/internal/stubserver" "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/internal/testutils/xds/e2e" xdsinternal "google.golang.org/grpc/internal/xds" - "google.golang.org/grpc/internal/xds/balancer/clusterresolver" + "google.golang.org/grpc/internal/xds/balancer/clusterimpl" + "google.golang.org/grpc/internal/xds/balancer/outlierdetection" + "google.golang.org/grpc/internal/xds/balancer/priority" + "google.golang.org/grpc/internal/xds/balancer/wrrlocality" "google.golang.org/grpc/internal/xds/bootstrap" "google.golang.org/grpc/internal/xds/xdsclient" - "google.golang.org/grpc/internal/xds/xdsclient/xdsresource" "google.golang.org/grpc/internal/xds/xdsclient/xdsresource/version" "google.golang.org/grpc/resolver" "google.golang.org/grpc/resolver/manual" @@ -61,7 +65,8 @@ import ( testgrpc "google.golang.org/grpc/interop/grpc_testing" testpb "google.golang.org/grpc/interop/grpc_testing" - _ "google.golang.org/grpc/balancer/ringhash" // Register the ring_hash LB policy + "google.golang.org/grpc/balancer/ringhash" + "google.golang.org/grpc/balancer/roundrobin" _ "google.golang.org/grpc/internal/xds/httpfilter/router" // Register the router filter. _ "google.golang.org/grpc/internal/xds/resolver" // Register the xds resolver ) @@ -107,7 +112,7 @@ func waitForResourceNames(ctx context.Context, resourceNamesCh chan []string, wa return nil } -// Registers a wrapped cluster_resolver LB policy (child policy of the cds LB +// Registers a wrapped priority LB policy (child policy of the cds LB // policy) for the duration of this test that retains all the functionality of // the former, but makes certain events available for inspection by the test. // @@ -116,21 +121,21 @@ func waitForResourceNames(ctx context.Context, resourceNamesCh chan []string, wa // - a channel to read received resolver error // - a channel that is closed when ExitIdle() is called // - a channel that is closed when the balancer is closed -func registerWrappedClusterResolverPolicy(t *testing.T) (chan serviceconfig.LoadBalancingConfig, chan error, chan struct{}, chan struct{}) { - clusterresolverBuilder := balancer.Get(clusterresolver.Name) - internal.BalancerUnregister(clusterresolverBuilder.Name()) +func registerWrappedPriorityPolicy(t *testing.T) (chan serviceconfig.LoadBalancingConfig, chan error, chan struct{}, chan struct{}) { + priorityBuilder := balancer.Get(priority.Name) + internal.BalancerUnregister(priorityBuilder.Name()) lbCfgCh := make(chan serviceconfig.LoadBalancingConfig, 1) resolverErrCh := make(chan error, 1) exitIdleCh := make(chan struct{}) closeCh := make(chan struct{}) - stub.Register(clusterresolver.Name, stub.BalancerFuncs{ + stub.Register(priority.Name, stub.BalancerFuncs{ Init: func(bd *stub.BalancerData) { - bd.ChildBalancer = clusterresolverBuilder.Build(bd.ClientConn, bd.BuildOptions) + bd.ChildBalancer = priorityBuilder.Build(bd.ClientConn, bd.BuildOptions) }, ParseConfig: func(lbCfg json.RawMessage) (serviceconfig.LoadBalancingConfig, error) { - return clusterresolverBuilder.(balancer.ConfigParser).ParseConfig(lbCfg) + return priorityBuilder.(balancer.ConfigParser).ParseConfig(lbCfg) }, UpdateClientConnState: func(bd *stub.BalancerData, ccs balancer.ClientConnState) error { select { @@ -155,73 +160,11 @@ func registerWrappedClusterResolverPolicy(t *testing.T) (chan serviceconfig.Load close(closeCh) }, }) - t.Cleanup(func() { balancer.Register(clusterresolverBuilder) }) + t.Cleanup(func() { balancer.Register(priorityBuilder) }) return lbCfgCh, resolverErrCh, exitIdleCh, closeCh } -// Performs the following setup required for tests: -// - Spins up an xDS management server and the provided onStreamRequest -// function is set to be called for every incoming request on the ADS stream. -// - Creates an xDS client talking to this management server -// - Creates a manual resolver that configures the cds LB policy as the -// top-level policy, and pushes an initial configuration to it -// - Creates a gRPC channel with the above manual resolver -// -// Returns the following: -// - the xDS management server -// - the nodeID expected by the management server -// - the grpc channel to the test backend service -// - the manual resolver configured on the channel -// - the xDS client used the grpc channel -func setupWithManagementServerAndManualResolver(t *testing.T, lis net.Listener, onStreamRequest func(int64, *v3discoverypb.DiscoveryRequest) error) (*e2e.ManagementServer, string, *grpc.ClientConn, *manual.Resolver, xdsclient.XDSClient) { - t.Helper() - mgmtServer := e2e.StartManagementServer(t, e2e.ManagementServerOptions{ - Listener: lis, - OnStreamRequest: onStreamRequest, - // Required for aggregate clusters as all resources cannot be requested - // at once. - AllowResourceSubset: true, - }) - - // Create bootstrap configuration pointing to the above management server. - nodeID := uuid.New().String() - bc := e2e.DefaultBootstrapContents(t, nodeID, mgmtServer.Address) - - config, err := bootstrap.NewConfigFromContents(bc) - if err != nil { - t.Fatalf("Failed to parse bootstrap contents: %s, %v", string(bc), err) - } - pool := xdsclient.NewPool(config) - xdsC, xdsClose, err := pool.NewClientForTesting(xdsclient.OptionsForTesting{ - Name: t.Name(), - }) - if err != nil { - t.Fatalf("Failed to create xDS client: %v", err) - } - t.Cleanup(xdsClose) - - r := manual.NewBuilderWithScheme("whatever") - jsonSC := fmt.Sprintf(`{ - "loadBalancingConfig":[{ - "cds_experimental":{ - "cluster": "%s" - } - }] - }`, clusterName) - scpr := internal.ParseServiceConfig.(func(string) *serviceconfig.ParseResult)(jsonSC) - r.InitialState(xdsclient.SetClient(resolver.State{ServiceConfig: scpr}, xdsC)) - - cc, err := grpc.NewClient(r.Scheme()+":///test.service", grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithResolvers(r)) - if err != nil { - t.Fatalf("grpc.NewClient(\"%s:///test.service\") = %v", r.Scheme(), err) - } - cc.Connect() - t.Cleanup(func() { cc.Close() }) - - return mgmtServer, nodeID, cc, r, xdsC -} - // Performs the following setup required for tests: // - Spins up an xDS management server and the provided onStreamRequest // function is set to be called for every incoming request on the ADS stream. @@ -304,72 +247,29 @@ func verifyRPCError(gotErr error, wantCode codes.Code, wantErr, wantNodeID strin return nil } -// Tests the functionality that handles LB policy configuration. Verifies that -// the appropriate xDS resource is requested corresponding to the provided LB -// policy configuration. Also verifies that when the LB policy receives the same -// configuration again, it does not send out a new request, and when the -// configuration changes, it stops requesting the old cluster resource and -// starts requesting the new one. -func (s) TestConfigurationUpdate_Success(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() - cdsResourceRequestedCh := make(chan []string, 1) - onStreamReq := func(_ int64, req *v3discoverypb.DiscoveryRequest) error { - if req.GetTypeUrl() == version.V3ClusterURL { - if len(req.GetResourceNames()) > 0 { - select { - case cdsResourceRequestedCh <- req.GetResourceNames(): - case <-ctx.Done(): - } - } - } - return nil - } - _, _, _, r, xdsClient := setupWithManagementServerAndManualResolver(t, nil, onStreamReq) - - // Verify that the specified cluster resource is requested. - wantNames := []string{clusterName} - if err := waitForResourceNames(ctx, cdsResourceRequestedCh, wantNames); err != nil { - t.Fatal(err) - } - - // Push the same configuration again. - jsonSC := fmt.Sprintf(`{ - "loadBalancingConfig":[{ - "cds_experimental":{ - "cluster": "%s" - } - }] - }`, clusterName) - scpr := internal.ParseServiceConfig.(func(string) *serviceconfig.ParseResult)(jsonSC) - r.UpdateState(xdsclient.SetClient(resolver.State{ServiceConfig: scpr}, xdsClient)) - - // Verify that a new CDS request is not sent. - sCtx, sCancel := context.WithTimeout(ctx, defaultTestShortTimeout) - defer sCancel() - select { - case <-sCtx.Done(): - case gotNames := <-cdsResourceRequestedCh: - t.Fatalf("CDS resources %v requested when none expected", gotNames) - } - - // Push an updated configuration with a different cluster name. - newClusterName := clusterName + "-new" - jsonSC = fmt.Sprintf(`{ - "loadBalancingConfig":[{ - "cds_experimental":{ - "cluster": "%s" - } - }] - }`, newClusterName) - scpr = internal.ParseServiceConfig.(func(string) *serviceconfig.ParseResult)(jsonSC) - r.UpdateState(xdsclient.SetClient(resolver.State{ServiceConfig: scpr}, xdsClient)) - - // Verify that the new cluster name is requested and the old one is no - // longer requested. - wantNames = []string{newClusterName} - if err := waitForResourceNames(ctx, cdsResourceRequestedCh, wantNames); err != nil { - t.Fatal(err) +// createPriorityConfig creates priority config for both EDS and DNS cluster. +// For DNS clusters, edsServiceName passed should be empty. +func createPriorityConfig(cluster, edsServiceName string) *iserviceconfig.BalancerConfig { + return &iserviceconfig.BalancerConfig{ + Name: outlierdetection.Name, + Config: &outlierdetection.LBConfig{ + Interval: iserviceconfig.Duration(10 * time.Second), // default interval + BaseEjectionTime: iserviceconfig.Duration(30 * time.Second), + MaxEjectionTime: iserviceconfig.Duration(300 * time.Second), + MaxEjectionPercent: 10, + ChildPolicy: &iserviceconfig.BalancerConfig{ + Name: clusterimpl.Name, + Config: &clusterimpl.LBConfig{ + Cluster: cluster, + EDSServiceName: edsServiceName, + TelemetryLabels: xdsinternal.UnknownCSMLabels, + ChildPolicy: &iserviceconfig.BalancerConfig{ + Name: wrrlocality.Name, + Config: &wrrlocality.LBConfig{ChildPolicy: &iserviceconfig.BalancerConfig{Name: roundrobin.Name}}, + }, + }, + }, + }, } } @@ -497,16 +397,35 @@ func (s) TestClusterUpdate_Success(t *testing.T) { } return c }(), - wantChildCfg: &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - MaxConcurrentRequests: newUint32(512), - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + wantChildCfg: &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: &iserviceconfig.BalancerConfig{ + Name: outlierdetection.Name, + Config: &outlierdetection.LBConfig{ + Interval: iserviceconfig.Duration(10 * time.Second), // default interval + BaseEjectionTime: iserviceconfig.Duration(30 * time.Second), + MaxEjectionTime: iserviceconfig.Duration(300 * time.Second), + MaxEjectionPercent: 10, + ChildPolicy: &iserviceconfig.BalancerConfig{ + Name: clusterimpl.Name, + Config: &clusterimpl.LBConfig{ + Cluster: clusterName, + EDSServiceName: serviceName, + TelemetryLabels: xdsinternal.UnknownCSMLabels, + MaxConcurrentRequests: newUint32(512), + ChildPolicy: &iserviceconfig.BalancerConfig{ + Name: wrrlocality.Name, + Config: &wrrlocality.LBConfig{ChildPolicy: &iserviceconfig.BalancerConfig{Name: roundrobin.Name}}, + }, + }, + }, + }, + }, + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-0-0"}, }, }, { @@ -526,15 +445,37 @@ func (s) TestClusterUpdate_Success(t *testing.T) { } return c }(), - wantChildCfg: &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"ring_hash_experimental": {"minRingSize":100, "maxRingSize":1000}}]`), + wantChildCfg: &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: &iserviceconfig.BalancerConfig{ + Name: outlierdetection.Name, + Config: &outlierdetection.LBConfig{ + Interval: iserviceconfig.Duration(10 * time.Second), // default interval + BaseEjectionTime: iserviceconfig.Duration(30 * time.Second), + MaxEjectionTime: iserviceconfig.Duration(300 * time.Second), + MaxEjectionPercent: 10, + ChildPolicy: &iserviceconfig.BalancerConfig{ + Name: clusterimpl.Name, + Config: &clusterimpl.LBConfig{ + Cluster: clusterName, + EDSServiceName: serviceName, + TelemetryLabels: xdsinternal.UnknownCSMLabels, + ChildPolicy: &iserviceconfig.BalancerConfig{ + Name: ringhash.Name, + Config: &iringhash.LBConfig{ + MinRingSize: 100, + MaxRingSize: 1000, + }, + }, + }, + }, + }, + }, + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-0-0"}, }, }, { @@ -549,15 +490,43 @@ func (s) TestClusterUpdate_Success(t *testing.T) { c.OutlierDetection = &v3clusterpb.OutlierDetection{} return c }(), - wantChildCfg: &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{"successRateEjection":{}}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"ring_hash_experimental": {"minRingSize":1024, "maxRingSize":8388608}}]`), + wantChildCfg: &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: &iserviceconfig.BalancerConfig{ + Name: outlierdetection.Name, + Config: &outlierdetection.LBConfig{ + Interval: iserviceconfig.Duration(10 * time.Second), // default interval + BaseEjectionTime: iserviceconfig.Duration(30 * time.Second), + MaxEjectionTime: iserviceconfig.Duration(300 * time.Second), + MaxEjectionPercent: 10, + SuccessRateEjection: &outlierdetection.SuccessRateEjection{ + StdevFactor: 1900, + EnforcementPercentage: 100, + MinimumHosts: 5, + RequestVolume: 100, + }, + ChildPolicy: &iserviceconfig.BalancerConfig{ + Name: clusterimpl.Name, + Config: &clusterimpl.LBConfig{ + Cluster: clusterName, + EDSServiceName: serviceName, + TelemetryLabels: xdsinternal.UnknownCSMLabels, + ChildPolicy: &iserviceconfig.BalancerConfig{ + Name: ringhash.Name, + Config: &iringhash.LBConfig{ + MinRingSize: 1024, // default sizes + MaxRingSize: 4096, + }, + }, + }, + }, + }, + }, + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-0-0"}, }, }, { @@ -585,39 +554,56 @@ func (s) TestClusterUpdate_Success(t *testing.T) { } return c }(), - wantChildCfg: &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - OutlierDetection: json.RawMessage(`{ - "interval": "10s", - "baseEjectionTime": "30s", - "maxEjectionTime": "300s", - "maxEjectionPercent": 10, - "successRateEjection": { - "stdevFactor": 1900, - "enforcementPercentage": 100, - "minimumHosts": 5, - "requestVolume": 100 + wantChildCfg: &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: &iserviceconfig.BalancerConfig{ + Name: outlierdetection.Name, + Config: &outlierdetection.LBConfig{ + Interval: iserviceconfig.Duration(10 * time.Second), // default interval + BaseEjectionTime: iserviceconfig.Duration(30 * time.Second), + MaxEjectionTime: iserviceconfig.Duration(300 * time.Second), + MaxEjectionPercent: 10, + SuccessRateEjection: &outlierdetection.SuccessRateEjection{ + StdevFactor: 1900, + EnforcementPercentage: 100, + MinimumHosts: 5, + RequestVolume: 100, + }, + FailurePercentageEjection: &outlierdetection.FailurePercentageEjection{ + Threshold: 85, + EnforcementPercentage: 5, + MinimumHosts: 5, + RequestVolume: 50, + }, + ChildPolicy: &iserviceconfig.BalancerConfig{ + Name: clusterimpl.Name, + Config: &clusterimpl.LBConfig{ + Cluster: clusterName, + EDSServiceName: serviceName, + TelemetryLabels: xdsinternal.UnknownCSMLabels, + ChildPolicy: &iserviceconfig.BalancerConfig{ + Name: ringhash.Name, + Config: &iringhash.LBConfig{ + MinRingSize: 1024, // default sizes + MaxRingSize: 4096, + }, + }, + }, + }, + }, }, - "failurePercentageEjection": { - "threshold": 85, - "enforcementPercentage": 5, - "minimumHosts": 5, - "requestVolume": 50 - } - }`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"ring_hash_experimental": {"minRingSize":1024, "maxRingSize":8388608}}]`), + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-0-0"}, }, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - lbCfgCh, _, _, _ := registerWrappedClusterResolverPolicy(t) + lbCfgCh, _, _, _ := registerWrappedPriorityPolicy(t) mgmtServer, nodeID, _ := setupWithManagementServer(t, nil, nil) ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) @@ -643,7 +629,7 @@ func (s) TestClusterUpdate_Success(t *testing.T) { // resource from the management server with LRS enabled. Verifies that the load // balancing configuration pushed to the child is as expected. func (s) TestClusterUpdate_SuccessWithLRS(t *testing.T) { - lbCfgCh, _, _, _ := registerWrappedClusterResolverPolicy(t) + lbCfgCh, _, _, _ := registerWrappedPriorityPolicy(t) mgmtServer, nodeID, _ := setupWithManagementServer(t, nil, nil) clusterResource := e2e.ClusterResourceWithOptions(e2e.ClusterOptions{ @@ -656,16 +642,35 @@ func (s) TestClusterUpdate_SuccessWithLRS(t *testing.T) { t.Fatalf("Failed to create LRS server config for testing: %v", err) } - wantChildCfg := &clusterresolver.LBConfig{ - DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - LoadReportingServer: lrsServerCfg, - OutlierDetection: json.RawMessage(`{}`), - TelemetryLabels: xdsinternal.UnknownCSMLabels, - }}, - XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), + wantChildCfg := &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: &iserviceconfig.BalancerConfig{ + Name: outlierdetection.Name, + Config: &outlierdetection.LBConfig{ + Interval: iserviceconfig.Duration(10 * time.Second), // default interval + BaseEjectionTime: iserviceconfig.Duration(30 * time.Second), + MaxEjectionTime: iserviceconfig.Duration(300 * time.Second), + MaxEjectionPercent: 10, + ChildPolicy: &iserviceconfig.BalancerConfig{ + Name: clusterimpl.Name, + Config: &clusterimpl.LBConfig{ + Cluster: clusterName, + EDSServiceName: serviceName, + TelemetryLabels: xdsinternal.UnknownCSMLabels, + LoadReportingServer: lrsServerCfg, + ChildPolicy: &iserviceconfig.BalancerConfig{ + Name: wrrlocality.Name, + Config: &wrrlocality.LBConfig{ChildPolicy: &iserviceconfig.BalancerConfig{Name: roundrobin.Name}}, + }, + }, + }, + }, + }, + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-0-0"}, } ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) @@ -694,7 +699,6 @@ func (s) TestClusterUpdate_SuccessWithLRS(t *testing.T) { // update from the management server, the cds LB policy is expected to // continue using the previous good update. func (s) TestClusterUpdate_Failure(t *testing.T) { - _, resolverErrCh, _, _ := registerWrappedClusterResolverPolicy(t) ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) defer cancel() cdsResourceCanceledCh := make(chan struct{}, 1) @@ -787,16 +791,6 @@ func (s) TestClusterUpdate_Failure(t *testing.T) { if _, err := client.EmptyCall(ctx, &testpb.Empty{}, grpc.WaitForReady(true)); err != nil { t.Fatalf("EmptyCall() failed: %v", err) } - - // Verify that the resolver error is pushed to the child policy. - select { - case err := <-resolverErrCh: - if !strings.Contains(err.Error(), wantClusterNACKErr) { - t.Fatalf("Error pushed to child policy is %v, want %v", err, wantClusterNACKErr) - } - case <-ctx.Done(): - t.Fatal("Timeout when waiting for resolver error to be pushed to the child policy") - } } // Tests the following scenarios for resolver errors: @@ -805,114 +799,74 @@ func (s) TestClusterUpdate_Failure(t *testing.T) { // TRANSIENT_FAILURE. // - when a resolver error is received (one that is not a resource-not-found // error), with a previous good update from the management server, the cds LB -// policy is expected to push the error down the child policy, but is expected -// to continue to use the previously received good configuration. +// policy is expected to continue to use the previously received good +// configuration. // - when a resolver error is received (one that is a resource-not-found // error, which is usually the case when the LDS resource is removed), // with a previous good update from the management server, the cds LB policy // is expected to push the error down the child policy and put the channel in // TRANSIENT_FAILURE. It is also expected to cancel the CDS watch. func (s) TestResolverError(t *testing.T) { - _, resolverErrCh, _, childPolicyCloseCh := registerWrappedClusterResolverPolicy(t) - lis := testutils.NewListenerWrapper(t, nil) - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() - cdsResourceCanceledCh := make(chan struct{}, 1) - cdsResourceRequestedCh := make(chan []string, 1) - onStreamReq := func(_ int64, req *v3discoverypb.DiscoveryRequest) error { - if req.GetTypeUrl() == version.V3ClusterURL { - switch len(req.GetResourceNames()) { - case 0: - select { - case cdsResourceCanceledCh <- struct{}{}: - case <-ctx.Done(): - } - default: - select { - case cdsResourceRequestedCh <- req.GetResourceNames(): - case <-ctx.Done(): - } - } - } - return nil - } - mgmtServer, nodeID, cc, r, _ := setupWithManagementServerAndManualResolver(t, lis, onStreamReq) + registerWrappedPriorityPolicy(t) + mgmtServer, nodeID, cc := setupWithManagementServer(t, nil, nil) - // Grab the wrapped connection from the listener wrapper. This will be used - // to verify the connection is closed. - val, err := lis.NewConnCh.Receive(ctx) - if err != nil { - t.Fatalf("Failed to receive new connection from wrapped listener: %v", err) - } - conn := val.(*testutils.ConnWrapper) + // Start a test service backend. + server := stubserver.StartTestService(t, nil) + t.Cleanup(server.Stop) - // Verify that the specified cluster resource is requested. - wantNames := []string{clusterName} - if err := waitForResourceNames(ctx, cdsResourceRequestedCh, wantNames); err != nil { + // Host and port for the backend. + host := "localhost" + port := testutils.ParsePort(t, server.Address) + + // Push a resolver error (Bad Listener) on startup. Since there are no + // active clusters, xdsResolver should call ReportError. + resources := e2e.DefaultClientResources(e2e.ResourceParams{ + DialTarget: target, + NodeID: nodeID, + Host: host, + Port: port, + }) + resources.Listeners[0].ApiListener.ApiListener = nil + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) } - // Push a resolver error that is not a resource-not-found error. Here, we - // assume that errors from the xDS client or from the xDS resolver contain - // the xDS node ID. - resolverErr := fmt.Errorf("[xds node id: %s]: resolver-error-not-a-resource-not-found-error", nodeID) - r.CC().ReportError(resolverErr) - + // Verify that the channel enters TRANSIENT_FAILURE. testutils.AwaitState(ctx, t, cc, connectivity.TransientFailure) - // Drain the resolver error channel. - select { - case <-resolverErrCh: - default: - } - - // Ensure that the resolver error is propagated to the RPC caller. + // Verify that RPCs fail. client := testgrpc.NewTestServiceClient(cc) - _, err = client.EmptyCall(ctx, &testpb.Empty{}) - if err := verifyRPCError(err, codes.Unavailable, resolverErr.Error(), nodeID); err != nil { - t.Fatal(err) + if _, err := client.EmptyCall(ctx, &testpb.Empty{}); err == nil { + t.Fatalf("EmptyCall() succeeded, want failure due to bad listener") } - // Also verify that the watch for the cluster resource is not cancelled. - sCtx, sCancel := context.WithTimeout(ctx, defaultTestShortTimeout) - defer sCancel() - select { - case <-sCtx.Done(): - case <-cdsResourceCanceledCh: - t.Fatal("Watch for cluster resource is cancelled when not expected to") - } - - // Start a test service backend. - server := stubserver.StartTestService(t, nil) - t.Cleanup(server.Stop) - - // Configure good cluster and endpoints resources in the management server. - resources := e2e.UpdateOptions{ - NodeID: nodeID, - Clusters: []*v3clusterpb.Cluster{e2e.DefaultCluster(clusterName, serviceName, e2e.SecurityLevelNone)}, - Endpoints: []*v3endpointpb.ClusterLoadAssignment{e2e.DefaultEndpoint(serviceName, host, []uint32{testutils.ParsePort(t, server.Address)})}, - SkipValidation: true, - } + // Configure good resources. + resources = e2e.DefaultClientResources(e2e.ResourceParams{ + DialTarget: target, + NodeID: nodeID, + Host: host, + Port: port, + }) if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) } + // Verify that the channel enters READY state. + testutils.AwaitState(ctx, t, cc, connectivity.Ready) + // Verify that a successful RPC can be made. if _, err := client.EmptyCall(ctx, &testpb.Empty{}, grpc.WaitForReady(true)); err != nil { t.Fatalf("EmptyCall() failed: %v", err) } - // Again push a resolver error that is not a resource-not-found error. - r.CC().ReportError(resolverErr) - - // And again verify that the watch for the cluster resource is not - // cancelled. - sCtx, sCancel = context.WithTimeout(ctx, defaultTestShortTimeout) - defer sCancel() - select { - case <-sCtx.Done(): - case <-cdsResourceCanceledCh: - t.Fatal("Watch for cluster resource is cancelled when not expected to") + // Push a Bad Listener again (NACK). xdsResolver has active clusters, so it + // should NOT call ReportError, but keep old config. This should be treated + // as an ambient error. + resources.Listeners[0].ApiListener.ApiListener = nil + if err := mgmtServer.Update(ctx, resources); err != nil { + t.Fatal(err) } // Verify that a successful RPC can be made, using the previously received @@ -921,50 +875,19 @@ func (s) TestResolverError(t *testing.T) { t.Fatalf("EmptyCall() failed: %v", err) } - // Verify that the resolver error is pushed to the child policy. - select { - case err := <-resolverErrCh: - if err != resolverErr { - t.Fatalf("Error pushed to child policy is %v, want %v", err, resolverErr) - } - case <-ctx.Done(): - t.Fatal("Timeout when waiting for resolver error to be pushed to the child policy") - } - - // Push a resource-not-found-error this time around. Our xDS resolver does - // not send this error though. When an LDS or RDS resource is missing, the - // xDS resolver instead sends an erroring config selector which returns an - // error at RPC time with the xDS node ID, for new RPCs. Once ongoing RPCs - // complete, the xDS resolver will send an empty service config with no - // addresses, which will result in pick_first being configured on the - // channel. And pick_first will put the channel in TRANSIENT_FAILURE since - // it would have received an update with no addresses. - resolverErr = fmt.Errorf("[xds node id: %s]: %w", nodeID, xdsresource.NewError(xdsresource.ErrorTypeResourceNotFound, "xds resource not found error")) - r.CC().ReportError(resolverErr) - - // Wait for the CDS resource to be not requested anymore, or the connection - // to the management server to be closed (which happens as part of the last - // resource watch being canceled). - select { - case <-ctx.Done(): - t.Fatal("Timeout when waiting for CDS resource to be not requested") - case <-cdsResourceCanceledCh: - case <-conn.CloseCh.C: - } - - // Verify that the resolver error is pushed to the child policy. - select { - case <-childPolicyCloseCh: - case <-ctx.Done(): - t.Fatal("Timeout when waiting for child policy to be closed") + // Remove the Listener (Resource Not Found). + resources.Listeners = nil + resources.SkipValidation = true + if err := mgmtServer.Update(ctx, resources); err != nil { + t.Fatal(err) } + // Verify that RPCs eventually fail. testutils.AwaitState(ctx, t, cc, connectivity.TransientFailure) // Ensure that the resolver error is propagated to the RPC caller. - _, err = client.EmptyCall(ctx, &testpb.Empty{}) - if err := verifyRPCError(err, codes.Unavailable, resolverErr.Error(), nodeID); err != nil { - t.Fatal(err) + if _, err := client.EmptyCall(ctx, &testpb.Empty{}); err == nil { + t.Fatal("EmptyCall() succeeded, want failure due to removed listener") } } @@ -974,7 +897,7 @@ func (s) TestResolverError(t *testing.T) { // error down the child policy and put the channel in TRANSIENT_FAILURE. It is // also expected to cancel the CDS watch. func (s) TestResourceNotFoundResolverError(t *testing.T) { - _, _, _, childPolicyCloseCh := registerWrappedClusterResolverPolicy(t) + _, _, _, childPolicyCloseCh := registerWrappedPriorityPolicy(t) ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() cdsResourceCanceledCh := make(chan struct{}, 1) @@ -1017,13 +940,6 @@ func (s) TestResourceNotFoundResolverError(t *testing.T) { resources.SkipValidation = true mgmtServer.Update(ctx, resources) - // Wait for the CDS resource to be not requested anymore. - select { - case <-ctx.Done(): - t.Fatal("Timeout when waiting for CDS resource to be not requested") - case <-cdsResourceCanceledCh: - } - // Verify that the resolver error is pushed to the child policy. select { case <-childPolicyCloseCh: @@ -1133,8 +1049,8 @@ func (s) TestClusterUpdate_ResourceNotFound(t *testing.T) { // Tests that closing the cds LB policy results in the the child policy being // closed. -func TestClose(t *testing.T) { - _, _, _, childPolicyCloseCh := registerWrappedClusterResolverPolicy(t) +func (s) TestClose(t *testing.T) { + _, _, _, childPolicyCloseCh := registerWrappedPriorityPolicy(t) mgmtServer, nodeID, cc := setupWithManagementServer(t, nil, nil) // Start a test service backend. @@ -1174,7 +1090,7 @@ func TestClose(t *testing.T) { // Tests that calling ExitIdle on the cds LB policy results in the call being // propagated to the child policy. func (s) TestExitIdle(t *testing.T) { - _, _, exitIdleCh, _ := registerWrappedClusterResolverPolicy(t) + _, _, exitIdleCh, _ := registerWrappedPriorityPolicy(t) mgmtServer, nodeID, cc := setupWithManagementServer(t, nil, nil) // Start a test service backend. diff --git a/internal/xds/balancer/cdsbalancer/cluster_watcher.go b/internal/xds/balancer/cdsbalancer/cluster_watcher.go deleted file mode 100644 index 355923964c36..000000000000 --- a/internal/xds/balancer/cdsbalancer/cluster_watcher.go +++ /dev/null @@ -1,56 +0,0 @@ -/* - * Copyright 2023 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package cdsbalancer - -import ( - "context" - - "google.golang.org/grpc/internal/xds/xdsclient/xdsresource" -) - -// clusterWatcher implements the xdsresource.ClusterWatcher interface, and is -// passed to the xDS client as part of the WatchResource() API. -// -// It watches a single cluster and handles callbacks from the xDS client by -// scheduling them on the parent LB policy's serializer. -type clusterWatcher struct { - name string - parent *cdsBalancer -} - -func (cw *clusterWatcher) ResourceChanged(u *xdsresource.ClusterUpdate, onDone func()) { - handleUpdate := func(context.Context) { cw.parent.onClusterUpdate(cw.name, u); onDone() } - - cw.parent.serializer.ScheduleOr(handleUpdate, onDone) -} - -func (cw *clusterWatcher) ResourceError(err error, onDone func()) { - handleResourceError := func(context.Context) { cw.parent.onClusterResourceError(cw.name, err); onDone() } - cw.parent.serializer.ScheduleOr(handleResourceError, onDone) -} - -func (cw *clusterWatcher) AmbientError(err error, onDone func()) { - handleError := func(context.Context) { cw.parent.onClusterAmbientError(cw.name, err); onDone() } - cw.parent.serializer.ScheduleOr(handleError, onDone) -} - -// watcherState groups the state associated with a clusterWatcher. -type watcherState struct { - watcher *clusterWatcher // The underlying watcher. - cancelWatch func() // Cancel func to cancel the watch. - lastUpdate *xdsresource.ClusterUpdate // Most recent update received for this cluster. -} diff --git a/internal/xds/balancer/clusterresolver/configbuilder.go b/internal/xds/balancer/cdsbalancer/configbuilder.go similarity index 79% rename from internal/xds/balancer/clusterresolver/configbuilder.go rename to internal/xds/balancer/cdsbalancer/configbuilder.go index a917fe8f55c0..3089eb1351fc 100644 --- a/internal/xds/balancer/clusterresolver/configbuilder.go +++ b/internal/xds/balancer/cdsbalancer/configbuilder.go @@ -16,7 +16,7 @@ * */ -package clusterresolver +package cdsbalancer import ( "encoding/json" @@ -40,24 +40,41 @@ import ( const million = 1000000 -// priorityConfig is config for one priority. For example, if there's an EDS and a -// DNS, the priority list will be [priorityConfig{EDS}, priorityConfig{DNS}]. +// priorityConfig is config for one priority. For example, if there's an EDS and +// a DNS, the priority list will be [priorityConfig{EDS}, priorityConfig{DNS}]. // -// Each priorityConfig corresponds to one discovery mechanism from the LBConfig -// generated by the CDS balancer. The CDS balancer resolves the cluster name to -// an ordered list of discovery mechanisms (if the top cluster is an aggregated -// cluster), one for each underlying cluster. +// Each priorityConfig corresponds to one leaf cluster retrieved from XDSConfig +// for the top-level cluster. type priorityConfig struct { - mechanism DiscoveryMechanism - // edsResp is set only if type is EDS. - edsResp xdsresource.EndpointsUpdate - // endpoints is set only if type is DNS. - endpoints []resolver.Endpoint - // Each discovery mechanism has a name generator so that the child policies - // can reuse names between updates (EDS updates for example). + // clusterConfig has the cluster update as well as EDS or DNS endpoints + // depending on the leaf cluster type. + clusterConfig *xdsresource.ClusterConfig + // outlierDetection is the Outlier Detection LB configuration for this + // priority. + outlierDetection outlierdetection.LBConfig + // Each leaf cluster has a name generator so that the child policies can + // reuse names between updates (EDS updates for example). childNameGen *nameGenerator } +// hostName returns the name of the host for the given cluster. +// +// For EDS, it's the EDSServiceName (or ClusterName if empty). +// For DNS, it's the DNSHostName. +func hostName(clusterName string, update xdsresource.ClusterUpdate) string { + switch update.ClusterType { + case xdsresource.ClusterTypeEDS: + if update.EDSServiceName != "" { + return update.EDSServiceName + } + return clusterName + case xdsresource.ClusterTypeLogicalDNS: + return update.DNSHostName + default: + return "" + } +} + // buildPriorityConfigJSON builds balancer config for the passed in // priorities. // @@ -74,7 +91,7 @@ type priorityConfig struct { // ┌──────▼─────┐ ┌─────▼──────┐ // │xDSLBPolicy │ │xDSLBPolicy │ (Locality and Endpoint picking layer) // └────────────┘ └────────────┘ -func buildPriorityConfigJSON(priorities []priorityConfig, xdsLBPolicy *internalserviceconfig.BalancerConfig) ([]byte, []resolver.Endpoint, error) { +func buildPriorityConfigJSON(priorities []*priorityConfig, xdsLBPolicy *internalserviceconfig.BalancerConfig) ([]byte, []resolver.Endpoint, error) { pc, endpoints, err := buildPriorityConfig(priorities, xdsLBPolicy) if err != nil { return nil, nil, fmt.Errorf("failed to build priority config: %v", err) @@ -86,21 +103,22 @@ func buildPriorityConfigJSON(priorities []priorityConfig, xdsLBPolicy *internals return ret, endpoints, nil } -func buildPriorityConfig(priorities []priorityConfig, xdsLBPolicy *internalserviceconfig.BalancerConfig) (*priority.LBConfig, []resolver.Endpoint, error) { +func buildPriorityConfig(priorities []*priorityConfig, xdsLBPolicy *internalserviceconfig.BalancerConfig) (*priority.LBConfig, []resolver.Endpoint, error) { var ( retConfig = &priority.LBConfig{Children: make(map[string]*priority.Child)} retEndpoints []resolver.Endpoint ) for _, p := range priorities { - switch p.mechanism.Type { - case DiscoveryMechanismTypeEDS: - names, configs, endpoints, err := buildClusterImplConfigForEDS(p.childNameGen, p.edsResp, p.mechanism, xdsLBPolicy) + clusterUpdate := p.clusterConfig.Cluster + switch clusterUpdate.ClusterType { + case xdsresource.ClusterTypeEDS: + names, configs, endpoints, err := buildClusterImplConfigForEDS(p.childNameGen, p.clusterConfig, xdsLBPolicy) if err != nil { return nil, nil, err } retConfig.Priorities = append(retConfig.Priorities, names...) retEndpoints = append(retEndpoints, endpoints...) - odCfgs := convertClusterImplMapToOutlierDetection(configs, p.mechanism.outlierDetection) + odCfgs := convertClusterImplMapToOutlierDetection(configs, p.outlierDetection) for n, c := range odCfgs { retConfig.Children[n] = &priority.Child{ Config: &internalserviceconfig.BalancerConfig{Name: outlierdetection.Name, Config: c}, @@ -109,11 +127,11 @@ func buildPriorityConfig(priorities []priorityConfig, xdsLBPolicy *internalservi } } continue - case DiscoveryMechanismTypeLogicalDNS: - name, config, endpoints := buildClusterImplConfigForDNS(p.childNameGen, p.endpoints, p.mechanism, xdsLBPolicy) + case xdsresource.ClusterTypeLogicalDNS: + name, config, endpoints := buildClusterImplConfigForDNS(p.childNameGen, p.clusterConfig, xdsLBPolicy) retConfig.Priorities = append(retConfig.Priorities, name) retEndpoints = append(retEndpoints, endpoints...) - odCfg := makeClusterImplOutlierDetectionChild(config, p.mechanism.outlierDetection) + odCfg := makeClusterImplOutlierDetectionChild(config, p.outlierDetection) retConfig.Children[name] = &priority.Child{ Config: &internalserviceconfig.BalancerConfig{Name: outlierdetection.Name, Config: odCfg}, // Not ignore re-resolution from DNS children, they will trigger @@ -140,15 +158,17 @@ func makeClusterImplOutlierDetectionChild(ciCfg *clusterimpl.LBConfig, odCfg out return &odCfgRet } -func buildClusterImplConfigForDNS(g *nameGenerator, endpoints []resolver.Endpoint, mechanism DiscoveryMechanism, xdsLBPolicy *internalserviceconfig.BalancerConfig) (string, *clusterimpl.LBConfig, []resolver.Endpoint) { +func buildClusterImplConfigForDNS(g *nameGenerator, config *xdsresource.ClusterConfig, xdsLBPolicy *internalserviceconfig.BalancerConfig) (string, *clusterimpl.LBConfig, []resolver.Endpoint) { pName := fmt.Sprintf("priority-%v", g.prefix) + clusterUpdate := config.Cluster lbconfig := &clusterimpl.LBConfig{ - Cluster: mechanism.Cluster, - TelemetryLabels: mechanism.TelemetryLabels, + Cluster: clusterUpdate.ClusterName, + TelemetryLabels: clusterUpdate.TelemetryLabels, ChildPolicy: xdsLBPolicy, - MaxConcurrentRequests: mechanism.MaxConcurrentRequests, - LoadReportingServer: mechanism.LoadReportingServer, + MaxConcurrentRequests: clusterUpdate.MaxRequests, + LoadReportingServer: clusterUpdate.LRSServerConfig, } + endpoints := config.EndpointConfig.DNSEndpoints.Endpoints if len(endpoints) == 0 { return pName, lbconfig, nil } @@ -164,7 +184,7 @@ func buildClusterImplConfigForDNS(g *nameGenerator, endpoints []resolver.Endpoin // LB policies that rely on locality information (like weighted_target) // continue to work. localityStr := xdsinternal.LocalityString(clients.Locality{}) - retEndpoint = xdsresource.SetHostname(hierarchy.SetInEndpoint(retEndpoint, []string{pName, localityStr}), mechanism.DNSHostname) + retEndpoint = xdsresource.SetHostname(hierarchy.SetInEndpoint(retEndpoint, []string{pName, localityStr}), clusterUpdate.DNSHostName) // Set the locality weight to 1. This is required because the child policy // like weighted_target which relies on locality weights to distribute // traffic. These policies may drop traffic if the weight is 0. @@ -181,9 +201,10 @@ func buildClusterImplConfigForDNS(g *nameGenerator, endpoints []resolver.Endpoin // - map{"p0":p0_config, "p1":p1_config} // - [p0_address_0, p0_address_1, p1_address_0, p1_address_1] // - p0 addresses' hierarchy attributes are set to p0 -func buildClusterImplConfigForEDS(g *nameGenerator, edsResp xdsresource.EndpointsUpdate, mechanism DiscoveryMechanism, xdsLBPolicy *internalserviceconfig.BalancerConfig) ([]string, map[string]*clusterimpl.LBConfig, []resolver.Endpoint, error) { - drops := make([]clusterimpl.DropConfig, 0, len(edsResp.Drops)) - for _, d := range edsResp.Drops { +func buildClusterImplConfigForEDS(g *nameGenerator, config *xdsresource.ClusterConfig, xdsLBPolicy *internalserviceconfig.BalancerConfig) ([]string, map[string]*clusterimpl.LBConfig, []resolver.Endpoint, error) { + edsUpdate := config.EndpointConfig.EDSUpdate + drops := make([]clusterimpl.DropConfig, 0, len(edsUpdate.Drops)) + for _, d := range edsUpdate.Drops { drops = append(drops, clusterimpl.DropConfig{ Category: d.Category, RequestsPerMillion: d.Numerator * million / d.Denominator, @@ -198,15 +219,15 @@ func buildClusterImplConfigForEDS(g *nameGenerator, edsResp xdsresource.Endpoint // should report a result that is a single priority with no endpoints." - // A37 priorities := [][]xdsresource.Locality{{}} - if len(edsResp.Localities) != 0 { - priorities = groupLocalitiesByPriority(edsResp.Localities) + if len(edsUpdate.Localities) != 0 { + priorities = groupLocalitiesByPriority(edsUpdate.Localities) } retNames := g.generate(priorities) retConfigs := make(map[string]*clusterimpl.LBConfig, len(retNames)) var retEndpoints []resolver.Endpoint for i, pName := range retNames { priorityLocalities := priorities[i] - cfg, endpoints, err := priorityLocalitiesToClusterImpl(priorityLocalities, pName, mechanism, drops, xdsLBPolicy) + cfg, endpoints, err := priorityLocalitiesToClusterImpl(priorityLocalities, pName, *config.Cluster, drops, xdsLBPolicy) if err != nil { return nil, nil, nil, err } @@ -244,7 +265,7 @@ func groupLocalitiesByPriority(localities []xdsresource.Locality) [][]xdsresourc // priority), and generates a cluster impl policy config, and a list of // addresses with their path hierarchy set to [priority-name, locality-name], so // priority and the xDS LB Policy know which child policy each address is for. -func priorityLocalitiesToClusterImpl(localities []xdsresource.Locality, priorityName string, mechanism DiscoveryMechanism, drops []clusterimpl.DropConfig, xdsLBPolicy *internalserviceconfig.BalancerConfig) (*clusterimpl.LBConfig, []resolver.Endpoint, error) { +func priorityLocalitiesToClusterImpl(localities []xdsresource.Locality, priorityName string, clusterUpdate xdsresource.ClusterUpdate, drops []clusterimpl.DropConfig, xdsLBPolicy *internalserviceconfig.BalancerConfig) (*clusterimpl.LBConfig, []resolver.Endpoint, error) { var retEndpoints []resolver.Endpoint // Compute the sum of locality weights to normalize locality weights. The @@ -306,11 +327,11 @@ func priorityLocalitiesToClusterImpl(localities []xdsresource.Locality, priority } } return &clusterimpl.LBConfig{ - Cluster: mechanism.Cluster, - EDSServiceName: mechanism.EDSServiceName, - LoadReportingServer: mechanism.LoadReportingServer, - MaxConcurrentRequests: mechanism.MaxConcurrentRequests, - TelemetryLabels: mechanism.TelemetryLabels, + Cluster: clusterUpdate.ClusterName, + EDSServiceName: clusterUpdate.EDSServiceName, + LoadReportingServer: clusterUpdate.LRSServerConfig, + MaxConcurrentRequests: clusterUpdate.MaxRequests, + TelemetryLabels: clusterUpdate.TelemetryLabels, DropCategories: drops, ChildPolicy: xdsLBPolicy, }, retEndpoints, nil diff --git a/internal/xds/balancer/clusterresolver/configbuilder_childname.go b/internal/xds/balancer/cdsbalancer/configbuilder_childname.go similarity index 99% rename from internal/xds/balancer/clusterresolver/configbuilder_childname.go rename to internal/xds/balancer/cdsbalancer/configbuilder_childname.go index 296ed740e401..8f118d4f68fa 100644 --- a/internal/xds/balancer/clusterresolver/configbuilder_childname.go +++ b/internal/xds/balancer/cdsbalancer/configbuilder_childname.go @@ -15,7 +15,7 @@ * limitations under the License. */ -package clusterresolver +package cdsbalancer import ( "fmt" diff --git a/internal/xds/balancer/clusterresolver/configbuilder_childname_test.go b/internal/xds/balancer/cdsbalancer/configbuilder_childname_test.go similarity index 97% rename from internal/xds/balancer/clusterresolver/configbuilder_childname_test.go rename to internal/xds/balancer/cdsbalancer/configbuilder_childname_test.go index c5abcd8fe929..e534d54ee18c 100644 --- a/internal/xds/balancer/clusterresolver/configbuilder_childname_test.go +++ b/internal/xds/balancer/cdsbalancer/configbuilder_childname_test.go @@ -15,7 +15,7 @@ * limitations under the License. */ -package clusterresolver +package cdsbalancer import ( "testing" @@ -25,7 +25,7 @@ import ( "google.golang.org/grpc/internal/xds/xdsclient/xdsresource" ) -func Test_nameGenerator_generate(t *testing.T) { +func (s) Test_nameGenerator_generate(t *testing.T) { tests := []struct { name string prefix uint64 diff --git a/internal/xds/balancer/clusterresolver/configbuilder_test.go b/internal/xds/balancer/cdsbalancer/configbuilder_test.go similarity index 84% rename from internal/xds/balancer/clusterresolver/configbuilder_test.go rename to internal/xds/balancer/cdsbalancer/configbuilder_test.go index 3e36ee2c2ad4..c91fb8a3f1e0 100644 --- a/internal/xds/balancer/clusterresolver/configbuilder_test.go +++ b/internal/xds/balancer/cdsbalancer/configbuilder_test.go @@ -16,7 +16,7 @@ * */ -package clusterresolver +package cdsbalancer import ( "bytes" @@ -50,6 +50,8 @@ import ( ) const ( + testClusterName = "test-cluster-name" + testClusterName2 = "google_cfe_some-name" testMaxRequests = 314 testEDSServiceName = "service-name-from-parent" testDropCategory = "test-drops" @@ -152,37 +154,43 @@ func (s) TestBuildPriorityConfigJSON(t *testing.T) { t.Fatalf("Failed to create LRS server config for testing: %v", err) } - gotConfig, _, err := buildPriorityConfigJSON([]priorityConfig{ + gotConfig, _, err := buildPriorityConfigJSON([]*priorityConfig{ { - mechanism: DiscoveryMechanism{ - Cluster: testClusterName, - LoadReportingServer: testLRSServerConfig, - MaxConcurrentRequests: newUint32(testMaxRequests), - Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSServiceName, - }, - edsResp: xdsresource.EndpointsUpdate{ - Drops: []xdsresource.OverloadDropConfig{ - { - Category: testDropCategory, - Numerator: testDropOverMillion, - Denominator: million, - }, + clusterConfig: &xdsresource.ClusterConfig{ + Cluster: &xdsresource.ClusterUpdate{ + ClusterName: testClusterName, + ClusterType: xdsresource.ClusterTypeEDS, + EDSServiceName: testEDSServiceName, + MaxRequests: newUint32(testMaxRequests), + LRSServerConfig: testLRSServerConfig, }, - Localities: []xdsresource.Locality{ - testLocalitiesP0[0], - testLocalitiesP0[1], - testLocalitiesP1[0], - testLocalitiesP1[1], + EndpointConfig: &xdsresource.EndpointConfig{ + EDSUpdate: &xdsresource.EndpointsUpdate{ + Drops: []xdsresource.OverloadDropConfig{{ + Category: testDropCategory, + Numerator: testDropOverMillion, + Denominator: million, + }}, + Localities: []xdsresource.Locality{ + testLocalitiesP0[0], + testLocalitiesP0[1], + testLocalitiesP1[0], + testLocalitiesP1[1], + }, + }, }, }, childNameGen: newNameGenerator(0), }, { - mechanism: DiscoveryMechanism{ - Type: DiscoveryMechanismTypeLogicalDNS, + clusterConfig: &xdsresource.ClusterConfig{ + Cluster: &xdsresource.ClusterUpdate{ + ClusterType: xdsresource.ClusterTypeLogicalDNS, + }, + EndpointConfig: &xdsresource.EndpointConfig{ + DNSEndpoints: &xdsresource.DNSUpdate{Endpoints: testResolverEndpoints[4]}, + }, }, - endpoints: testResolverEndpoints[4], childNameGen: newNameGenerator(1), }, }, nil) @@ -207,38 +215,48 @@ func (s) TestBuildPriorityConfigJSON(t *testing.T) { // balancer per priority should be an Outlier Detection balancer, with a Cluster // Impl Balancer as a child. func (s) TestBuildPriorityConfig(t *testing.T) { - gotConfig, _, _ := buildPriorityConfig([]priorityConfig{ + gotConfig, _, _ := buildPriorityConfig([]*priorityConfig{ { // EDS - OD config should be the top level for both of the EDS // priorities balancer This EDS priority will have multiple sub // priorities. The Outlier Detection configuration specified in the // Discovery Mechanism should be the top level for each sub // priorities balancer. - mechanism: DiscoveryMechanism{ - Cluster: testClusterName, - Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSServiceName, - outlierDetection: noopODCfg, - }, - edsResp: xdsresource.EndpointsUpdate{ - Localities: []xdsresource.Locality{ - testLocalitiesP0[0], - testLocalitiesP0[1], - testLocalitiesP1[0], - testLocalitiesP1[1], + clusterConfig: &xdsresource.ClusterConfig{ + Cluster: &xdsresource.ClusterUpdate{ + ClusterName: testClusterName, + ClusterType: xdsresource.ClusterTypeEDS, + EDSServiceName: testEDSServiceName, + }, + EndpointConfig: &xdsresource.EndpointConfig{ + EDSUpdate: &xdsresource.EndpointsUpdate{ + Localities: []xdsresource.Locality{ + testLocalitiesP0[0], + testLocalitiesP0[1], + testLocalitiesP1[0], + testLocalitiesP1[1], + }, + }, }, }, - childNameGen: newNameGenerator(0), + outlierDetection: noopODCfg, + childNameGen: newNameGenerator(0), }, { // This OD config should wrap the Logical DNS priorities balancer. - mechanism: DiscoveryMechanism{ - Cluster: testClusterName2, - Type: DiscoveryMechanismTypeLogicalDNS, - outlierDetection: noopODCfg, + clusterConfig: &xdsresource.ClusterConfig{ + Cluster: &xdsresource.ClusterUpdate{ + ClusterName: testClusterName2, + ClusterType: xdsresource.ClusterTypeLogicalDNS, + }, + EndpointConfig: &xdsresource.EndpointConfig{ + DNSEndpoints: &xdsresource.DNSUpdate{ + Endpoints: testResolverEndpoints[4], + }, + }, }, - endpoints: testResolverEndpoints[4], - childNameGen: newNameGenerator(1), + outlierDetection: noopODCfg, + childNameGen: newNameGenerator(1), }, }, nil) @@ -363,7 +381,15 @@ func (s) TestBuildClusterImplConfigForDNS(t *testing.T) { }, } { t.Run(tt.name, func(t *testing.T) { - gotName, gotConfig, gotEndpoints := buildClusterImplConfigForDNS(newNameGenerator(3), tt.endpoints, DiscoveryMechanism{Cluster: testClusterName2, Type: DiscoveryMechanismTypeLogicalDNS}, tt.xdsLBPolicy) + gotName, gotConfig, gotEndpoints := buildClusterImplConfigForDNS(newNameGenerator(3), + &xdsresource.ClusterConfig{ + Cluster: &xdsresource.ClusterUpdate{ + ClusterName: testClusterName2, + ClusterType: xdsresource.ClusterTypeLogicalDNS, + }, + EndpointConfig: &xdsresource.EndpointConfig{DNSEndpoints: &xdsresource.DNSUpdate{Endpoints: tt.endpoints}}, + }, + tt.xdsLBPolicy) const wantName = "priority-3" if diff := cmp.Diff(wantName, gotName); diff != "" { t.Errorf("buildClusterImplConfigForDNS() diff (-want +got) %v", diff) @@ -385,7 +411,7 @@ func (s) TestBuildClusterImplConfigForDNS(t *testing.T) { } } -func TestBuildClusterImplConfigForEDS_PickFirstWeightedShuffling_Disabled(t *testing.T) { +func (s) TestBuildClusterImplConfigForEDS_PickFirstWeightedShuffling_Disabled(t *testing.T) { testutils.SetEnvConfig(t, &envconfig.PickFirstWeightedShuffling, false) testLRSServerConfig, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{ @@ -398,45 +424,50 @@ func TestBuildClusterImplConfigForEDS_PickFirstWeightedShuffling_Disabled(t *tes gotNames, gotConfigs, gotEndpoints, _ := buildClusterImplConfigForEDS( newNameGenerator(2), - xdsresource.EndpointsUpdate{ - Drops: []xdsresource.OverloadDropConfig{ - { - Category: testDropCategory, - Numerator: testDropOverMillion, - Denominator: million, - }, + &xdsresource.ClusterConfig{ + Cluster: &xdsresource.ClusterUpdate{ + ClusterName: testClusterName, + ClusterType: xdsresource.ClusterTypeEDS, + EDSServiceName: testEDSServiceName, + LRSServerConfig: testLRSServerConfig, + MaxRequests: newUint32(testMaxRequests), }, - Localities: []xdsresource.Locality{ - { - Endpoints: testEndpoints[3], - ID: testLocalityIDs[3], - Weight: 80, - Priority: 1, - }, { - Endpoints: testEndpoints[1], - ID: testLocalityIDs[1], - Weight: 80, - Priority: 0, - }, { - Endpoints: testEndpoints[2], - ID: testLocalityIDs[2], - Weight: 20, - Priority: 1, - }, { - Endpoints: testEndpoints[0], - ID: testLocalityIDs[0], - Weight: 20, - Priority: 0, + EndpointConfig: &xdsresource.EndpointConfig{ + EDSUpdate: &xdsresource.EndpointsUpdate{ + Drops: []xdsresource.OverloadDropConfig{{ + Category: testDropCategory, + Numerator: testDropOverMillion, + Denominator: million, + }}, + Localities: []xdsresource.Locality{ + { + Endpoints: testEndpoints[3], + ID: testLocalityIDs[3], + Weight: 80, + Priority: 1, + }, + { + Endpoints: testEndpoints[1], + ID: testLocalityIDs[1], + Weight: 80, + Priority: 0, + }, + { + Endpoints: testEndpoints[2], + ID: testLocalityIDs[2], + Weight: 20, + Priority: 1, + }, + { + Endpoints: testEndpoints[0], + ID: testLocalityIDs[0], + Weight: 20, + Priority: 0, + }, + }, }, }, }, - DiscoveryMechanism{ - Cluster: testClusterName, - MaxConcurrentRequests: newUint32(testMaxRequests), - LoadReportingServer: testLRSServerConfig, - Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSServiceName, - }, nil, ) @@ -450,24 +481,20 @@ func TestBuildClusterImplConfigForEDS_PickFirstWeightedShuffling_Disabled(t *tes EDSServiceName: testEDSServiceName, LoadReportingServer: testLRSServerConfig, MaxConcurrentRequests: newUint32(testMaxRequests), - DropCategories: []clusterimpl.DropConfig{ - { - Category: testDropCategory, - RequestsPerMillion: testDropOverMillion, - }, - }, + DropCategories: []clusterimpl.DropConfig{{ + Category: testDropCategory, + RequestsPerMillion: testDropOverMillion, + }}, }, "priority-2-1": { Cluster: testClusterName, EDSServiceName: testEDSServiceName, LoadReportingServer: testLRSServerConfig, MaxConcurrentRequests: newUint32(testMaxRequests), - DropCategories: []clusterimpl.DropConfig{ - { - Category: testDropCategory, - RequestsPerMillion: testDropOverMillion, - }, - }, + DropCategories: []clusterimpl.DropConfig{{ + Category: testDropCategory, + RequestsPerMillion: testDropOverMillion, + }}, }, } // Endpoint weight is the product of locality weight and endpoint weight. @@ -493,7 +520,7 @@ func TestBuildClusterImplConfigForEDS_PickFirstWeightedShuffling_Disabled(t *tes } } -func TestBuildClusterImplConfigForEDS_PickFirstWeightedShuffling_Enabled(t *testing.T) { +func (s) TestBuildClusterImplConfigForEDS_PickFirstWeightedShuffling_Enabled(t *testing.T) { testutils.SetEnvConfig(t, &envconfig.PickFirstWeightedShuffling, true) testLRSServerConfig, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{ @@ -506,45 +533,50 @@ func TestBuildClusterImplConfigForEDS_PickFirstWeightedShuffling_Enabled(t *test gotNames, gotConfigs, gotEndpoints, _ := buildClusterImplConfigForEDS( newNameGenerator(2), - xdsresource.EndpointsUpdate{ - Drops: []xdsresource.OverloadDropConfig{ - { - Category: testDropCategory, - Numerator: testDropOverMillion, - Denominator: million, - }, + &xdsresource.ClusterConfig{ + Cluster: &xdsresource.ClusterUpdate{ + ClusterName: testClusterName, + ClusterType: xdsresource.ClusterTypeEDS, + EDSServiceName: testEDSServiceName, + LRSServerConfig: testLRSServerConfig, + MaxRequests: newUint32(testMaxRequests), }, - Localities: []xdsresource.Locality{ - { - Endpoints: testEndpoints[3], - ID: testLocalityIDs[3], - Weight: 80, - Priority: 1, - }, { - Endpoints: testEndpoints[1], - ID: testLocalityIDs[1], - Weight: 80, - Priority: 0, - }, { - Endpoints: testEndpoints[2], - ID: testLocalityIDs[2], - Weight: 20, - Priority: 1, - }, { - Endpoints: testEndpoints[0], - ID: testLocalityIDs[0], - Weight: 20, - Priority: 0, + EndpointConfig: &xdsresource.EndpointConfig{ + EDSUpdate: &xdsresource.EndpointsUpdate{ + Drops: []xdsresource.OverloadDropConfig{{ + Category: testDropCategory, + Numerator: testDropOverMillion, + Denominator: million, + }}, + Localities: []xdsresource.Locality{ + { + Endpoints: testEndpoints[3], + ID: testLocalityIDs[3], + Weight: 80, + Priority: 1, + }, + { + Endpoints: testEndpoints[1], + ID: testLocalityIDs[1], + Weight: 80, + Priority: 0, + }, + { + Endpoints: testEndpoints[2], + ID: testLocalityIDs[2], + Weight: 20, + Priority: 1, + }, + { + Endpoints: testEndpoints[0], + ID: testLocalityIDs[0], + Weight: 20, + Priority: 0, + }, + }, }, }, }, - DiscoveryMechanism{ - Cluster: testClusterName, - MaxConcurrentRequests: newUint32(testMaxRequests), - LoadReportingServer: testLRSServerConfig, - Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSServiceName, - }, nil, ) @@ -558,24 +590,20 @@ func TestBuildClusterImplConfigForEDS_PickFirstWeightedShuffling_Enabled(t *test EDSServiceName: testEDSServiceName, LoadReportingServer: testLRSServerConfig, MaxConcurrentRequests: newUint32(testMaxRequests), - DropCategories: []clusterimpl.DropConfig{ - { - Category: testDropCategory, - RequestsPerMillion: testDropOverMillion, - }, - }, + DropCategories: []clusterimpl.DropConfig{{ + Category: testDropCategory, + RequestsPerMillion: testDropOverMillion, + }}, }, "priority-2-1": { Cluster: testClusterName, EDSServiceName: testEDSServiceName, LoadReportingServer: testLRSServerConfig, MaxConcurrentRequests: newUint32(testMaxRequests), - DropCategories: []clusterimpl.DropConfig{ - { - Category: testDropCategory, - RequestsPerMillion: testDropOverMillion, - }, - }, + DropCategories: []clusterimpl.DropConfig{{ + Category: testDropCategory, + RequestsPerMillion: testDropOverMillion, + }}, }, } // Endpoints weights are the product of normalized locality weight and @@ -617,7 +645,7 @@ func TestBuildClusterImplConfigForEDS_PickFirstWeightedShuffling_Enabled(t *test t.Errorf("buildClusterImplConfigForEDS() diff (-got +want) %v", diff) } } -func TestGroupLocalitiesByPriority(t *testing.T) { +func (s) TestGroupLocalitiesByPriority(t *testing.T) { tests := []struct { name string localities []xdsresource.Locality @@ -679,13 +707,13 @@ func TestGroupLocalitiesByPriority(t *testing.T) { } } -func TestPriorityLocalitiesToClusterImpl_PickFirstWeightedShuffling_Disabled(t *testing.T) { +func (s) TestPriorityLocalitiesToClusterImpl_PickFirstWeightedShuffling_Disabled(t *testing.T) { testutils.SetEnvConfig(t, &envconfig.PickFirstWeightedShuffling, false) tests := []struct { name string localities []xdsresource.Locality priorityName string - mechanism DiscoveryMechanism + clusterUpdate xdsresource.ClusterUpdate childPolicy *iserviceconfig.BalancerConfig wantConfig *clusterimpl.LBConfig wantEndpoints []resolver.Endpoint @@ -729,15 +757,15 @@ func TestPriorityLocalitiesToClusterImpl_PickFirstWeightedShuffling_Disabled(t * }, priorityName: "test-priority", childPolicy: &iserviceconfig.BalancerConfig{Name: roundrobin.Name}, - mechanism: DiscoveryMechanism{ - Cluster: testClusterName, - Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSService, + clusterUpdate: xdsresource.ClusterUpdate{ + ClusterName: testClusterName, + ClusterType: xdsresource.ClusterTypeEDS, + EDSServiceName: testEDSServiceName, }, // lrsServer is nil, so LRS policy will not be used. wantConfig: &clusterimpl.LBConfig{ Cluster: testClusterName, - EDSServiceName: testEDSService, + EDSServiceName: testEDSServiceName, ChildPolicy: &iserviceconfig.BalancerConfig{Name: roundrobin.Name}, }, // Endpoint weight is the product of locality weight and endpoint weight. @@ -804,7 +832,7 @@ func TestPriorityLocalitiesToClusterImpl_PickFirstWeightedShuffling_Disabled(t * } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotConfig, gotEndpoints, err := priorityLocalitiesToClusterImpl(tt.localities, tt.priorityName, tt.mechanism, nil, tt.childPolicy) + gotConfig, gotEndpoints, err := priorityLocalitiesToClusterImpl(tt.localities, tt.priorityName, tt.clusterUpdate, nil, tt.childPolicy) if (err != nil) != tt.wantErr { t.Fatalf("priorityLocalitiesToClusterImpl() error = %v, wantErr %v", err, tt.wantErr) } @@ -818,13 +846,13 @@ func TestPriorityLocalitiesToClusterImpl_PickFirstWeightedShuffling_Disabled(t * } } -func TestPriorityLocalitiesToClusterImpl_PickFirstWeightedShuffling_Enabled(t *testing.T) { +func (s) TestPriorityLocalitiesToClusterImpl_PickFirstWeightedShuffling_Enabled(t *testing.T) { testutils.SetEnvConfig(t, &envconfig.PickFirstWeightedShuffling, true) tests := []struct { name string localities []xdsresource.Locality priorityName string - mechanism DiscoveryMechanism + clusterUpdate xdsresource.ClusterUpdate childPolicy *iserviceconfig.BalancerConfig wantConfig *clusterimpl.LBConfig wantEndpoints []resolver.Endpoint @@ -868,15 +896,15 @@ func TestPriorityLocalitiesToClusterImpl_PickFirstWeightedShuffling_Enabled(t *t }, priorityName: "test-priority", childPolicy: &iserviceconfig.BalancerConfig{Name: roundrobin.Name}, - mechanism: DiscoveryMechanism{ - Cluster: testClusterName, - Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSService, + clusterUpdate: xdsresource.ClusterUpdate{ + ClusterName: testClusterName, + ClusterType: xdsresource.ClusterTypeEDS, + EDSServiceName: testEDSServiceName, }, // lrsServer is nil, so LRS policy will not be used. wantConfig: &clusterimpl.LBConfig{ Cluster: testClusterName, - EDSServiceName: testEDSService, + EDSServiceName: testEDSServiceName, ChildPolicy: &iserviceconfig.BalancerConfig{Name: roundrobin.Name}, }, // Endpoints weights are the product of normalized locality weight and @@ -979,7 +1007,7 @@ func TestPriorityLocalitiesToClusterImpl_PickFirstWeightedShuffling_Enabled(t *t } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotConfig, gotEndpoints, err := priorityLocalitiesToClusterImpl(tt.localities, tt.priorityName, tt.mechanism, nil, tt.childPolicy) + gotConfig, gotEndpoints, err := priorityLocalitiesToClusterImpl(tt.localities, tt.priorityName, tt.clusterUpdate, nil, tt.childPolicy) if (err != nil) != tt.wantErr { t.Fatalf("priorityLocalitiesToClusterImpl() error = %v, wantErr %v", err, tt.wantErr) } diff --git a/internal/xds/balancer/cdsbalancer/e2e_test/aggregate_cluster_test.go b/internal/xds/balancer/cdsbalancer/e2e_test/aggregate_cluster_test.go index 461d3076f930..76de30a41eb1 100644 --- a/internal/xds/balancer/cdsbalancer/e2e_test/aggregate_cluster_test.go +++ b/internal/xds/balancer/cdsbalancer/e2e_test/aggregate_cluster_test.go @@ -20,7 +20,7 @@ import ( "context" "fmt" "net" - "sort" + "slices" "strconv" "strings" "testing" @@ -33,6 +33,7 @@ import ( "google.golang.org/grpc/connectivity" "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/internal" + "google.golang.org/grpc/internal/grpcsync" "google.golang.org/grpc/internal/stubserver" "google.golang.org/grpc/internal/testutils/pickfirst" "google.golang.org/grpc/internal/testutils/xds/e2e" @@ -107,9 +108,13 @@ func (s) TestAggregateCluster_WithTwoEDSClusters(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() - // Start an xDS management server that pushes the EDS resource names onto a - // channel when requested. - edsResourceNameCh := make(chan []string, 1) + const clusterName1 = clusterName + "-cluster-1" + const clusterName2 = clusterName + "-cluster-2" + + // gotBothEDSRequests is fired when the management server receives EDS + // requests for both clusterName1 and clusterName2. This is used to block + // the test until both EDS requests have been received. + gotBothEDSRequests := grpcsync.NewEvent() managementServer := e2e.StartManagementServer(t, e2e.ManagementServerOptions{ OnStreamRequest: func(_ int64, req *v3discoverypb.DiscoveryRequest) error { if req.GetTypeUrl() != version.V3EndpointsURL { @@ -121,9 +126,14 @@ func (s) TestAggregateCluster_WithTwoEDSClusters(t *testing.T) { // resources. return nil } - select { - case edsResourceNameCh <- req.GetResourceNames(): - case <-ctx.Done(): + + // Check if we have a request for both EDS resources. If so, fire + // the event to unblock the test. + names := req.GetResourceNames() + sortedNames := slices.Clone(names) + slices.Sort(sortedNames) + if cmp.Equal(sortedNames, []string{clusterName1, clusterName2}) { + gotBothEDSRequests.Fire() } return nil }, @@ -144,8 +154,6 @@ func (s) TestAggregateCluster_WithTwoEDSClusters(t *testing.T) { // Configure an aggregate cluster, two EDS clusters and only one endpoints // resource (corresponding to the first EDS cluster) in the management // server. - const clusterName1 = clusterName + "-cluster-1" - const clusterName2 = clusterName + "-cluster-2" resources := e2e.UpdateOptions{ NodeID: nodeID, Listeners: []*v3listenerpb.Listener{e2e.DefaultClientListener(serviceName, routeName)}, @@ -167,23 +175,9 @@ func (s) TestAggregateCluster_WithTwoEDSClusters(t *testing.T) { defer cleanup() // Wait for both EDS resources to be requested. - func() { - for ; ctx.Err() == nil; <-time.After(defaultTestShortTimeout) { - select { - case names := <-edsResourceNameCh: - // Copy and sort the sortedNames to avoid racing with an - // OnStreamRequest call. - sortedNames := make([]string, len(names)) - copy(sortedNames, names) - sort.Strings(sortedNames) - if cmp.Equal(sortedNames, []string{clusterName1, clusterName2}) { - return - } - default: - } - } - }() - if ctx.Err() != nil { + select { + case <-gotBothEDSRequests.Done(): + case <-ctx.Done(): t.Fatalf("Timeout when waiting for all EDS resources %v to be requested", []string{clusterName1, clusterName2}) } diff --git a/internal/xds/balancer/cdsbalancer/e2e_test/dns_impl_test.go b/internal/xds/balancer/cdsbalancer/e2e_test/dns_impl_test.go index 03209f22bf59..902317ea5d6c 100644 --- a/internal/xds/balancer/cdsbalancer/e2e_test/dns_impl_test.go +++ b/internal/xds/balancer/cdsbalancer/e2e_test/dns_impl_test.go @@ -37,17 +37,17 @@ import ( testgrpc "google.golang.org/grpc/interop/grpc_testing" testpb "google.golang.org/grpc/interop/grpc_testing" - _ "google.golang.org/grpc/internal/xds/balancer/clusterresolver" + _ "google.golang.org/grpc/internal/xds/balancer/priority" // Register priority LB policy. ) -// TestLogicalDNS_MultipleEndpoints tests the cluster_resolver LB policy -// using a LOGICAL_DNS discovery mechanism. +// TestLogicalDNS_MultipleEndpoints tests the priority LB policy using a +// LOGICAL_DNS discovery mechanism. // // The test verifies that multiple addresses returned by the DNS resolver are -// grouped into a single endpoint (as per gRFC A61). Because the round_robin -// LB policy sees only one endpoint, it should not rotate traffic between the -// addresses. Instead, the single endpoint is picked, and connects to the -// first address. +// grouped into a single endpoint (as per gRFC A61). Because the round_robin LB +// policy sees only one endpoint, it should not rotate traffic between the +// addresses. Instead, the single endpoint is picked, and connects to the first +// address. func (s) TestLogicalDNS_MultipleEndpoints(t *testing.T) { // Spin up a management server to receive xDS resources from. managementServer := e2e.StartManagementServer(t, e2e.ManagementServerOptions{}) diff --git a/internal/xds/balancer/cdsbalancer/e2e_test/eds_impl_test.go b/internal/xds/balancer/cdsbalancer/e2e_test/eds_impl_test.go index 973915105e64..4e65a4d4eb92 100644 --- a/internal/xds/balancer/cdsbalancer/e2e_test/eds_impl_test.go +++ b/internal/xds/balancer/cdsbalancer/e2e_test/eds_impl_test.go @@ -56,7 +56,6 @@ import ( testgrpc "google.golang.org/grpc/interop/grpc_testing" testpb "google.golang.org/grpc/interop/grpc_testing" - _ "google.golang.org/grpc/internal/xds/balancer/clusterresolver" // Register the "cluster_resolver_experimental" LB policy. "google.golang.org/grpc/internal/xds/balancer/priority" ) diff --git a/internal/xds/balancer/clusterimpl/tests/balancer_test.go b/internal/xds/balancer/clusterimpl/tests/balancer_test.go index 2fcae94951d5..49efd006c586 100644 --- a/internal/xds/balancer/clusterimpl/tests/balancer_test.go +++ b/internal/xds/balancer/clusterimpl/tests/balancer_test.go @@ -955,22 +955,25 @@ func (s) TestReResolutionAfterTransientFailure(t *testing.T) { Clusters: []*v3clusterpb.Cluster{cluster, ldnsCluster}, Endpoints: nil, } + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() // Replace DNS resolver with a wrapped resolver to capture ResolveNow calls. - resolveNowCh := make(chan struct{}, 1) + resolveNowCh := make(chan struct{}) dnsR := manual.NewBuilderWithScheme("dns") dnsResolverBuilder := resolver.Get("dns") resolver.Register(dnsR) defer resolver.Register(dnsResolverBuilder) dnsR.ResolveNowCallback = func(resolver.ResolveNowOptions) { - close(resolveNowCh) + select { + case resolveNowCh <- struct{}{}: + case <-ctx.Done(): + } } dnsR.UpdateState(resolver.State{ Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: fmt.Sprintf("%s:%d", host, port)}}}}, }) - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() if err := mgmtServer.Update(ctx, updateOpts); err != nil { t.Fatalf("Failed to update xDS resources: %v", err) } diff --git a/internal/xds/balancer/clustermanager/clustermanager.go b/internal/xds/balancer/clustermanager/clustermanager.go index 05e10e579dd1..c5300a62fc36 100644 --- a/internal/xds/balancer/clustermanager/clustermanager.go +++ b/internal/xds/balancer/clustermanager/clustermanager.go @@ -141,7 +141,7 @@ func (b *bal) updateChildren(s balancer.ClientConnState, newConfig *lbConfig) er }, BalancerConfig: lbCfg, }); err != nil { - retErr = fmt.Errorf("failed to push new configuration %v to child %q", childCfg.ChildPolicy.Config, childName) + retErr = fmt.Errorf("failed to push new configuration %v to child %q: %v", childCfg.ChildPolicy.Config, childName, err) b.setErrorPickerForChild(childName, retErr) } diff --git a/internal/xds/balancer/clustermanager/e2e_test/clustermanager_test.go b/internal/xds/balancer/clustermanager/e2e_test/clustermanager_test.go index 379783f91d1f..a55c9c58eaa5 100644 --- a/internal/xds/balancer/clustermanager/e2e_test/clustermanager_test.go +++ b/internal/xds/balancer/clustermanager/e2e_test/clustermanager_test.go @@ -94,7 +94,7 @@ func makeUnaryCallRPCAndVerifyPeer(ctx context.Context, client testgrpc.TestServ func (s) TestConfigUpdate_ChildPolicyChange(t *testing.T) { // Spin up an xDS management server. - mgmtServer := e2e.StartManagementServer(t, e2e.ManagementServerOptions{}) + mgmtServer := e2e.StartManagementServer(t, e2e.ManagementServerOptions{AllowResourceSubset: true}) // Create bootstrap configuration pointing to the above management server. nodeID := uuid.New().String() @@ -250,7 +250,7 @@ func (s) TestConfigUpdate_ChildPolicyChange(t *testing.T) { server3 := stubserver.StartTestService(t, nil) defer server3.Stop() endpoints3 := e2e.DefaultEndpoint(endpointsName3, "localhost", []uint32{testutils.ParsePort(t, server3.Address)}) - resources.Clusters[1] = cluster2 + resources.Clusters = append(resources.Clusters, cluster2) resources.Endpoints = append(resources.Endpoints, endpoints3) if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) diff --git a/internal/xds/balancer/clusterresolver/clusterresolver.go b/internal/xds/balancer/clusterresolver/clusterresolver.go deleted file mode 100644 index 199070468239..000000000000 --- a/internal/xds/balancer/clusterresolver/clusterresolver.go +++ /dev/null @@ -1,410 +0,0 @@ -/* - * - * Copyright 2019 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -// Package clusterresolver contains the implementation of the -// cluster_resolver_experimental LB policy which resolves endpoint addresses -// using a list of one or more discovery mechanisms. -package clusterresolver - -import ( - "encoding/json" - "errors" - "fmt" - - "google.golang.org/grpc/attributes" - "google.golang.org/grpc/balancer" - "google.golang.org/grpc/balancer/base" - "google.golang.org/grpc/connectivity" - "google.golang.org/grpc/internal/balancer/nop" - "google.golang.org/grpc/internal/buffer" - "google.golang.org/grpc/internal/grpclog" - "google.golang.org/grpc/internal/grpcsync" - "google.golang.org/grpc/internal/pretty" - "google.golang.org/grpc/internal/xds/balancer/outlierdetection" - "google.golang.org/grpc/internal/xds/balancer/priority" - "google.golang.org/grpc/internal/xds/xdsclient" - "google.golang.org/grpc/internal/xds/xdsclient/xdsresource" - "google.golang.org/grpc/resolver" - "google.golang.org/grpc/serviceconfig" -) - -// Name is the name of the cluster_resolver balancer. -const Name = "cluster_resolver_experimental" - -var ( - errBalancerClosed = errors.New("cdsBalancer is closed") - newChildBalancer = func(bb balancer.Builder, cc balancer.ClientConn, o balancer.BuildOptions) balancer.Balancer { - return bb.Build(cc, o) - } -) - -func init() { - balancer.Register(bb{}) -} - -type bb struct{} - -// Build helps implement the balancer.Builder interface. -func (bb) Build(cc balancer.ClientConn, opts balancer.BuildOptions) balancer.Balancer { - priorityBuilder := balancer.Get(priority.Name) - if priorityBuilder == nil { - logger.Errorf("%q LB policy is needed but not registered", priority.Name) - return nop.NewBalancer(cc, fmt.Errorf("%q LB policy is needed but not registered", priority.Name)) - } - priorityConfigParser, ok := priorityBuilder.(balancer.ConfigParser) - if !ok { - logger.Errorf("%q LB policy does not implement a config parser", priority.Name) - return nop.NewBalancer(cc, fmt.Errorf("%q LB policy does not implement a config parser", priority.Name)) - } - - b := &clusterResolverBalancer{ - bOpts: opts, - updateCh: buffer.NewUnbounded(), - closed: grpcsync.NewEvent(), - done: grpcsync.NewEvent(), - - priorityBuilder: priorityBuilder, - priorityConfigParser: priorityConfigParser, - } - b.logger = prefixLogger(b) - b.logger.Infof("Created") - - b.resourceWatcher = newResourceResolver(b, b.logger) - b.cc = &ccWrapper{ - ClientConn: cc, - b: b, - resourceWatcher: b.resourceWatcher, - } - - go b.run() - return b -} - -func (bb) Name() string { - return Name -} - -func (bb) ParseConfig(j json.RawMessage) (serviceconfig.LoadBalancingConfig, error) { - odBuilder := balancer.Get(outlierdetection.Name) - if odBuilder == nil { - // Shouldn't happen, registered through imported Outlier Detection, - // defensive programming. - return nil, fmt.Errorf("%q LB policy is needed but not registered", outlierdetection.Name) - } - odParser, ok := odBuilder.(balancer.ConfigParser) - if !ok { - // Shouldn't happen, imported Outlier Detection builder has this method. - return nil, fmt.Errorf("%q LB policy does not implement a config parser", outlierdetection.Name) - } - - var cfg *LBConfig - if err := json.Unmarshal(j, &cfg); err != nil { - return nil, fmt.Errorf("unable to unmarshal balancer config %s into cluster-resolver config, error: %v", string(j), err) - } - - for i, dm := range cfg.DiscoveryMechanisms { - lbCfg, err := odParser.ParseConfig(dm.OutlierDetection) - if err != nil { - return nil, fmt.Errorf("error parsing Outlier Detection config %v: %v", dm.OutlierDetection, err) - } - odCfg, ok := lbCfg.(*outlierdetection.LBConfig) - if !ok { - // Shouldn't happen, Parser built at build time with Outlier Detection - // builder pulled from gRPC LB Registry. - return nil, fmt.Errorf("odParser returned config with unexpected type %T: %v", lbCfg, lbCfg) - } - cfg.DiscoveryMechanisms[i].outlierDetection = *odCfg - } - if err := json.Unmarshal(cfg.XDSLBPolicy, &cfg.xdsLBPolicy); err != nil { - // This will never occur, valid configuration is emitted from the xDS - // Client. Validity is already checked in the xDS Client, however, this - // double validation is present because Unmarshalling and Validating are - // coupled into one json.Unmarshal operation. We will switch this in - // the future to two separate operations. - return nil, fmt.Errorf("error unmarshalling xDS LB Policy: %v", err) - } - return cfg, nil -} - -// ccUpdate wraps a clientConn update received from gRPC. -type ccUpdate struct { - state balancer.ClientConnState - err error -} - -type exitIdle struct{} - -// clusterResolverBalancer resolves endpoint addresses using a list of one or -// more discovery mechanisms. -type clusterResolverBalancer struct { - cc balancer.ClientConn - bOpts balancer.BuildOptions - updateCh *buffer.Unbounded // Channel for updates from gRPC. - resourceWatcher *resourceResolver - logger *grpclog.PrefixLogger - closed *grpcsync.Event - done *grpcsync.Event - - priorityBuilder balancer.Builder - priorityConfigParser balancer.ConfigParser - - config *LBConfig - configRaw *serviceconfig.ParseResult - xdsClient xdsclient.XDSClient // xDS client to watch EDS resource. - attrsWithClient *attributes.Attributes // Attributes with xdsClient attached to be passed to the child policies. - - child balancer.Balancer - priorities []priorityConfig - watchUpdateReceived bool -} - -// handleClientConnUpdate handles a ClientConnUpdate received from gRPC. -// -// A good update results in creation of endpoint resolvers for the configured -// discovery mechanisms. An update with an error results in cancellation of any -// existing endpoint resolution and propagation of the same to the child policy. -func (b *clusterResolverBalancer) handleClientConnUpdate(update *ccUpdate) { - if err := update.err; err != nil { - b.handleErrorFromUpdate(err, true) - return - } - - if b.logger.V(2) { - b.logger.Infof("Received new balancer config: %v", pretty.ToJSON(update.state.BalancerConfig)) - } - - cfg, _ := update.state.BalancerConfig.(*LBConfig) - if cfg == nil { - b.logger.Warningf("Ignoring unsupported balancer configuration of type: %T", update.state.BalancerConfig) - return - } - - b.config = cfg - b.configRaw = update.state.ResolverState.ServiceConfig - b.resourceWatcher.updateMechanisms(cfg.DiscoveryMechanisms) - - // The child policy is created only after all configured discovery - // mechanisms have been successfully returned endpoints. If that is not the - // case, we return early. - if !b.watchUpdateReceived { - return - } - b.updateChildConfig() -} - -// handleResourceUpdate handles a resource update or error from the resource -// resolver by propagating the same to the child LB policy. -func (b *clusterResolverBalancer) handleResourceUpdate(update *resourceUpdate) { - b.watchUpdateReceived = true - b.priorities = update.priorities - - // An update from the resource resolver contains resolved endpoint addresses - // for all configured discovery mechanisms ordered by priority. This is used - // to generate configuration for the priority LB policy. - b.updateChildConfig() - - if update.onDone != nil { - update.onDone() - } -} - -// updateChildConfig builds child policy configuration using endpoint addresses -// returned by the resource resolver and child policy configuration provided by -// parent LB policy. -// -// A child policy is created if one doesn't already exist. The newly built -// configuration is then pushed to the child policy. -func (b *clusterResolverBalancer) updateChildConfig() { - if b.child == nil { - b.child = newChildBalancer(b.priorityBuilder, b.cc, b.bOpts) - } - - childCfgBytes, endpoints, err := buildPriorityConfigJSON(b.priorities, &b.config.xdsLBPolicy) - if err != nil { - b.logger.Warningf("Failed to build child policy config: %v", err) - return - } - childCfg, err := b.priorityConfigParser.ParseConfig(childCfgBytes) - if err != nil { - b.logger.Warningf("Failed to parse child policy config. This should never happen because the config was generated: %v", err) - return - } - if b.logger.V(2) { - b.logger.Infof("Built child policy config: %s", pretty.ToJSON(childCfg)) - } - - for i := range endpoints { - for j := range endpoints[i].Addresses { - addr := endpoints[i].Addresses[j] - addr.BalancerAttributes = endpoints[i].Attributes - // BalancerAttributes need to be present in endpoint addresses. This - // temporary workaround is required to make load reporting work - // with the old pickfirst policy which creates SubConns with multiple - // addresses. Since the addresses can be from different localities, - // an Address.BalancerAttribute is used to identify the locality of the - // address used by the transport. This workaround can be removed once - // the old pickfirst is removed. - // See https://github.com/grpc/grpc-go/issues/7339 - endpoints[i].Addresses[j] = addr - } - } - if err := b.child.UpdateClientConnState(balancer.ClientConnState{ - ResolverState: resolver.State{ - Endpoints: endpoints, - ServiceConfig: b.configRaw, - Attributes: b.attrsWithClient, - }, - BalancerConfig: childCfg, - }); err != nil { - b.logger.Warningf("Failed to push config to child policy: %v", err) - } -} - -// handleErrorFromUpdate handles errors from the parent LB policy and endpoint -// resolvers. fromParent is true if error is from the parent LB policy. In both -// cases, the error is propagated to the child policy, if one exists. -func (b *clusterResolverBalancer) handleErrorFromUpdate(err error, fromParent bool) { - b.logger.Warningf("Received error: %v", err) - - // A resource-not-found error from the parent LB policy means that the LDS - // or CDS resource was removed. This should result in endpoint resolvers - // being stopped here. - // - // A resource-not-found error from the EDS endpoint resolver means that the - // EDS resource was removed. No action needs to be taken for this, and we - // should continue watching the same EDS resource. - if fromParent && xdsresource.ErrType(err) == xdsresource.ErrorTypeResourceNotFound { - b.resourceWatcher.stop(false) - } - - if b.child != nil { - b.child.ResolverError(err) - return - } - b.cc.UpdateState(balancer.State{ - ConnectivityState: connectivity.TransientFailure, - Picker: base.NewErrPicker(err), - }) -} - -// run is a long-running goroutine that handles updates from gRPC and endpoint -// resolvers. The methods handling the individual updates simply push them onto -// a channel which is read and acted upon from here. -func (b *clusterResolverBalancer) run() { - for { - select { - case u, ok := <-b.updateCh.Get(): - if !ok { - return - } - b.updateCh.Load() - switch update := u.(type) { - case *ccUpdate: - b.handleClientConnUpdate(update) - case exitIdle: - if b.child == nil { - // This is not necessarily an error. The EDS/DNS watch may - // not have returned a list of endpoints yet, so the child - // may not be built. - if b.logger.V(2) { - b.logger.Infof("xds: received ExitIdle with no child balancer") - } - break - } - b.child.ExitIdle() - } - case u := <-b.resourceWatcher.updateChannel: - b.handleResourceUpdate(u) - - // Close results in stopping the endpoint resolvers and closing the - // underlying child policy and is the only way to exit this goroutine. - case <-b.closed.Done(): - b.resourceWatcher.stop(true) - - if b.child != nil { - b.child.Close() - b.child = nil - } - b.updateCh.Close() - // This is the *ONLY* point of return from this function. - b.logger.Infof("Shutdown") - b.done.Fire() - return - } - } -} - -// Following are methods to implement the balancer interface. - -func (b *clusterResolverBalancer) UpdateClientConnState(state balancer.ClientConnState) error { - if b.closed.HasFired() { - b.logger.Warningf("Received update from gRPC {%+v} after close", state) - return errBalancerClosed - } - - if b.xdsClient == nil { - c := xdsclient.FromResolverState(state.ResolverState) - if c == nil { - return balancer.ErrBadResolverState - } - b.xdsClient = c - b.attrsWithClient = state.ResolverState.Attributes - } - - b.updateCh.Put(&ccUpdate{state: state}) - return nil -} - -// ResolverError handles errors reported by the xdsResolver. -func (b *clusterResolverBalancer) ResolverError(err error) { - if b.closed.HasFired() { - b.logger.Warningf("Received resolver error {%v} after close", err) - return - } - b.updateCh.Put(&ccUpdate{err: err}) -} - -// UpdateSubConnState handles subConn updates from gRPC. -func (b *clusterResolverBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.SubConnState) { - b.logger.Errorf("UpdateSubConnState(%v, %+v) called unexpectedly", sc, state) -} - -// Close closes the cdsBalancer and the underlying child balancer. -func (b *clusterResolverBalancer) Close() { - b.closed.Fire() - <-b.done.Done() -} - -func (b *clusterResolverBalancer) ExitIdle() { - b.updateCh.Put(exitIdle{}) -} - -// ccWrapper overrides ResolveNow(), so that re-resolution from the child -// policies will trigger the DNS resolver in cluster_resolver balancer. It -// also intercepts NewSubConn calls in case children don't set the -// StateListener, to allow redirection to happen via this cluster_resolver -// balancer. -type ccWrapper struct { - balancer.ClientConn - b *clusterResolverBalancer - resourceWatcher *resourceResolver -} - -func (c *ccWrapper) ResolveNow(resolver.ResolveNowOptions) { - c.resourceWatcher.resolveNow() -} diff --git a/internal/xds/balancer/clusterresolver/clusterresolver_test.go b/internal/xds/balancer/clusterresolver/clusterresolver_test.go deleted file mode 100644 index bdf6e60b35c6..000000000000 --- a/internal/xds/balancer/clusterresolver/clusterresolver_test.go +++ /dev/null @@ -1,43 +0,0 @@ -/* - * - * Copyright 2019 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -package clusterresolver - -import ( - "testing" - "time" - - "google.golang.org/grpc/internal/grpctest" -) - -const ( - defaultTestTimeout = 5 * time.Second - defaultTestShortTimeout = 10 * time.Millisecond - testEDSService = "test-eds-service-name" - testClusterName = "test-cluster-name" - testClusterName2 = "google_cfe_some-name" - testBalancerNameFooBar = "foo.bar" -) - -type s struct { - grpctest.Tester -} - -func Test(t *testing.T) { - grpctest.RunSubTests(t, s{}) -} diff --git a/internal/xds/balancer/clusterresolver/config.go b/internal/xds/balancer/clusterresolver/config.go deleted file mode 100644 index f3b4c6bf6146..000000000000 --- a/internal/xds/balancer/clusterresolver/config.go +++ /dev/null @@ -1,160 +0,0 @@ -/* - * - * Copyright 2021 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package clusterresolver - -import ( - "bytes" - "encoding/json" - "fmt" - - internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" - "google.golang.org/grpc/internal/xds/balancer/outlierdetection" - "google.golang.org/grpc/internal/xds/bootstrap" - "google.golang.org/grpc/serviceconfig" -) - -// DiscoveryMechanismType is the type of discovery mechanism. -type DiscoveryMechanismType int - -const ( - // DiscoveryMechanismTypeEDS is eds. - DiscoveryMechanismTypeEDS DiscoveryMechanismType = iota // `json:"EDS"` - // DiscoveryMechanismTypeLogicalDNS is DNS. - DiscoveryMechanismTypeLogicalDNS // `json:"LOGICAL_DNS"` -) - -// MarshalJSON marshals a DiscoveryMechanismType to a quoted json string. -// -// This is necessary to handle enum (as strings) from JSON. -// -// Note that this needs to be defined on the type not pointer, otherwise the -// variables of this type will marshal to int not string. -func (t DiscoveryMechanismType) MarshalJSON() ([]byte, error) { - buffer := bytes.NewBufferString(`"`) - switch t { - case DiscoveryMechanismTypeEDS: - buffer.WriteString("EDS") - case DiscoveryMechanismTypeLogicalDNS: - buffer.WriteString("LOGICAL_DNS") - } - buffer.WriteString(`"`) - return buffer.Bytes(), nil -} - -// UnmarshalJSON unmarshals a quoted json string to the DiscoveryMechanismType. -func (t *DiscoveryMechanismType) UnmarshalJSON(b []byte) error { - var s string - err := json.Unmarshal(b, &s) - if err != nil { - return err - } - switch s { - case "EDS": - *t = DiscoveryMechanismTypeEDS - case "LOGICAL_DNS": - *t = DiscoveryMechanismTypeLogicalDNS - default: - return fmt.Errorf("unable to unmarshal string %q to type DiscoveryMechanismType", s) - } - return nil -} - -// DiscoveryMechanism is the discovery mechanism, can be either EDS or DNS. -// -// For DNS, the ClientConn target will be used for name resolution. -// -// For EDS, if EDSServiceName is not empty, it will be used for watching. If -// EDSServiceName is empty, Cluster will be used. -type DiscoveryMechanism struct { - // Cluster is the cluster name. - Cluster string `json:"cluster,omitempty"` - // LoadReportingServer is the LRS server to send load reports to. If not - // present, load reporting will be disabled. - LoadReportingServer *bootstrap.ServerConfig `json:"lrsLoadReportingServer,omitempty"` - // MaxConcurrentRequests is the maximum number of outstanding requests can - // be made to the upstream cluster. Default is 1024. - MaxConcurrentRequests *uint32 `json:"maxConcurrentRequests,omitempty"` - // Type is the discovery mechanism type. - Type DiscoveryMechanismType `json:"type,omitempty"` - // EDSServiceName is the EDS service name, as returned in CDS. May be unset - // if not specified in CDS. For type EDS only. - // - // This is used for EDS watch if set. If unset, Cluster is used for EDS - // watch. - EDSServiceName string `json:"edsServiceName,omitempty"` - // DNSHostname is the DNS name to resolve in "host:port" form. For type - // LOGICAL_DNS only. - DNSHostname string `json:"dnsHostname,omitempty"` - // OutlierDetection is the Outlier Detection LB configuration for this - // priority. - OutlierDetection json.RawMessage `json:"outlierDetection,omitempty"` - // TelemetryLabels are the telemetry labels associated with this cluster. - TelemetryLabels map[string]string `json:"telemetryLabels,omitempty"` - outlierDetection outlierdetection.LBConfig -} - -// Equal returns whether the DiscoveryMechanism is the same with the parameter. -func (dm DiscoveryMechanism) Equal(b DiscoveryMechanism) bool { - od := &dm.outlierDetection - switch { - case dm.Cluster != b.Cluster: - return false - case !equalUint32P(dm.MaxConcurrentRequests, b.MaxConcurrentRequests): - return false - case dm.Type != b.Type: - return false - case dm.EDSServiceName != b.EDSServiceName: - return false - case dm.DNSHostname != b.DNSHostname: - return false - case !od.EqualIgnoringChildPolicy(&b.outlierDetection): - return false - } - - if dm.LoadReportingServer == nil && b.LoadReportingServer == nil { - return true - } - if (dm.LoadReportingServer != nil) != (b.LoadReportingServer != nil) { - return false - } - return dm.LoadReportingServer.String() == b.LoadReportingServer.String() -} - -func equalUint32P(a, b *uint32) bool { - if a == nil && b == nil { - return true - } - if a == nil || b == nil { - return false - } - return *a == *b -} - -// LBConfig is the config for cluster resolver balancer. -type LBConfig struct { - serviceconfig.LoadBalancingConfig `json:"-"` - // DiscoveryMechanisms is an ordered list of discovery mechanisms. - // - // Must have at least one element. Results from each discovery mechanism are - // concatenated together in successive priorities. - DiscoveryMechanisms []DiscoveryMechanism `json:"discoveryMechanisms,omitempty"` - - // XDSLBPolicy specifies the policy for locality picking and endpoint picking. - XDSLBPolicy json.RawMessage `json:"xdsLbPolicy,omitempty"` - xdsLBPolicy internalserviceconfig.BalancerConfig -} diff --git a/internal/xds/balancer/clusterresolver/config_test.go b/internal/xds/balancer/clusterresolver/config_test.go deleted file mode 100644 index 964359aaf368..000000000000 --- a/internal/xds/balancer/clusterresolver/config_test.go +++ /dev/null @@ -1,371 +0,0 @@ -/* - * - * Copyright 2021 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -package clusterresolver - -import ( - "encoding/json" - "testing" - "time" - - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - "google.golang.org/grpc/balancer" - "google.golang.org/grpc/balancer/ringhash" - "google.golang.org/grpc/balancer/roundrobin" - iringhash "google.golang.org/grpc/internal/ringhash" - iserviceconfig "google.golang.org/grpc/internal/serviceconfig" - "google.golang.org/grpc/internal/xds/balancer/outlierdetection" - "google.golang.org/grpc/internal/xds/bootstrap" -) - -func TestDiscoveryMechanismTypeMarshalJSON(t *testing.T) { - tests := []struct { - name string - typ DiscoveryMechanismType - want string - }{ - { - name: "eds", - typ: DiscoveryMechanismTypeEDS, - want: `"EDS"`, - }, - { - name: "dns", - typ: DiscoveryMechanismTypeLogicalDNS, - want: `"LOGICAL_DNS"`, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got, err := json.Marshal(tt.typ); err != nil || string(got) != tt.want { - t.Fatalf("DiscoveryMechanismTypeEDS.MarshalJSON() = (%v, %v), want (%s, nil)", string(got), err, tt.want) - } - }) - } -} -func TestDiscoveryMechanismTypeUnmarshalJSON(t *testing.T) { - tests := []struct { - name string - js string - want DiscoveryMechanismType - wantErr bool - }{ - { - name: "eds", - js: `"EDS"`, - want: DiscoveryMechanismTypeEDS, - }, - { - name: "dns", - js: `"LOGICAL_DNS"`, - want: DiscoveryMechanismTypeLogicalDNS, - }, - { - name: "error", - js: `"1234"`, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var got DiscoveryMechanismType - err := json.Unmarshal([]byte(tt.js), &got) - if (err != nil) != tt.wantErr { - t.Fatalf("DiscoveryMechanismTypeEDS.UnmarshalJSON() error = %v, wantErr %v", err, tt.wantErr) - } - if diff := cmp.Diff(got, tt.want); diff != "" { - t.Fatalf("DiscoveryMechanismTypeEDS.UnmarshalJSON() got unexpected output, diff (-got +want): %v", diff) - } - }) - } -} - -const ( - testJSONConfig1 = `{ - "discoveryMechanisms": [{ - "cluster": "test-cluster-name", - "lrsLoadReportingServer": { - "server_uri": "trafficdirector.googleapis.com:443", - "channel_creds": [ { "type": "google_default" } ] - }, - "maxConcurrentRequests": 314, - "type": "EDS", - "edsServiceName": "test-eds-service-name", - "outlierDetection": {} - }], - "xdsLbPolicy":[{"round_robin":{}}] -}` - testJSONConfig2 = `{ - "discoveryMechanisms": [{ - "cluster": "test-cluster-name", - "lrsLoadReportingServer": { - "server_uri": "trafficdirector.googleapis.com:443", - "channel_creds": [ { "type": "google_default" } ] - }, - "maxConcurrentRequests": 314, - "type": "EDS", - "edsServiceName": "test-eds-service-name", - "outlierDetection": {} - },{ - "type": "LOGICAL_DNS", - "outlierDetection": {} - }], - "xdsLbPolicy":[{"round_robin":{}}] -}` - testJSONConfig3 = `{ - "discoveryMechanisms": [{ - "cluster": "test-cluster-name", - "lrsLoadReportingServer": { - "server_uri": "trafficdirector.googleapis.com:443", - "channel_creds": [ { "type": "google_default" } ] - }, - "maxConcurrentRequests": 314, - "type": "EDS", - "edsServiceName": "test-eds-service-name", - "outlierDetection": {} - }], - "xdsLbPolicy":[{"round_robin":{}}] -}` - testJSONConfig4 = `{ - "discoveryMechanisms": [{ - "cluster": "test-cluster-name", - "lrsLoadReportingServer": { - "server_uri": "trafficdirector.googleapis.com:443", - "channel_creds": [ { "type": "google_default" } ] - }, - "maxConcurrentRequests": 314, - "type": "EDS", - "edsServiceName": "test-eds-service-name", - "outlierDetection": {} - }], - "xdsLbPolicy":[{"ring_hash_experimental":{}}] -}` - testJSONConfig5 = `{ - "discoveryMechanisms": [{ - "cluster": "test-cluster-name", - "lrsLoadReportingServer": { - "server_uri": "trafficdirector.googleapis.com:443", - "channel_creds": [ { "type": "google_default" } ] - }, - "maxConcurrentRequests": 314, - "type": "EDS", - "edsServiceName": "test-eds-service-name", - "outlierDetection": {} - }], - "xdsLbPolicy":[{"round_robin":{}}] -}` -) - -func TestParseConfig(t *testing.T) { - testLRSServerConfig, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{ - URI: "trafficdirector.googleapis.com:443", - ChannelCreds: []bootstrap.ChannelCreds{{Type: "google_default"}}, - }) - if err != nil { - t.Fatalf("Failed to create LRS server config for testing: %v", err) - } - tests := []struct { - name string - js string - want *LBConfig - wantErr bool - }{ - { - name: "empty json", - js: "", - want: nil, - wantErr: true, - }, - { - name: "OK with one discovery mechanism", - js: testJSONConfig1, - want: &LBConfig{ - DiscoveryMechanisms: []DiscoveryMechanism{ - { - Cluster: testClusterName, - LoadReportingServer: testLRSServerConfig, - MaxConcurrentRequests: newUint32(testMaxRequests), - Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSService, - outlierDetection: outlierdetection.LBConfig{ - Interval: iserviceconfig.Duration(10 * time.Second), // default interval - BaseEjectionTime: iserviceconfig.Duration(30 * time.Second), - MaxEjectionTime: iserviceconfig.Duration(300 * time.Second), - MaxEjectionPercent: 10, - // sre and fpe are both nil - }, - }, - }, - xdsLBPolicy: iserviceconfig.BalancerConfig{ // do we want to make this not pointer - Name: roundrobin.Name, - Config: nil, - }, - }, - wantErr: false, - }, - { - name: "OK with multiple discovery mechanisms", - js: testJSONConfig2, - want: &LBConfig{ - DiscoveryMechanisms: []DiscoveryMechanism{ - { - Cluster: testClusterName, - LoadReportingServer: testLRSServerConfig, - MaxConcurrentRequests: newUint32(testMaxRequests), - Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSService, - outlierDetection: outlierdetection.LBConfig{ - Interval: iserviceconfig.Duration(10 * time.Second), // default interval - BaseEjectionTime: iserviceconfig.Duration(30 * time.Second), - MaxEjectionTime: iserviceconfig.Duration(300 * time.Second), - MaxEjectionPercent: 10, - // sre and fpe are both nil - }, - }, - { - Type: DiscoveryMechanismTypeLogicalDNS, - outlierDetection: outlierdetection.LBConfig{ - Interval: iserviceconfig.Duration(10 * time.Second), // default interval - BaseEjectionTime: iserviceconfig.Duration(30 * time.Second), - MaxEjectionTime: iserviceconfig.Duration(300 * time.Second), - MaxEjectionPercent: 10, - // sre and fpe are both nil - }, - }, - }, - xdsLBPolicy: iserviceconfig.BalancerConfig{ - Name: roundrobin.Name, - Config: nil, - }, - }, - wantErr: false, - }, - { - name: "OK with picking policy round_robin", - js: testJSONConfig3, - want: &LBConfig{ - DiscoveryMechanisms: []DiscoveryMechanism{ - { - Cluster: testClusterName, - LoadReportingServer: testLRSServerConfig, - MaxConcurrentRequests: newUint32(testMaxRequests), - Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSService, - outlierDetection: outlierdetection.LBConfig{ - Interval: iserviceconfig.Duration(10 * time.Second), // default interval - BaseEjectionTime: iserviceconfig.Duration(30 * time.Second), - MaxEjectionTime: iserviceconfig.Duration(300 * time.Second), - MaxEjectionPercent: 10, - // sre and fpe are both nil - }, - }, - }, - xdsLBPolicy: iserviceconfig.BalancerConfig{ - Name: roundrobin.Name, - Config: nil, - }, - }, - wantErr: false, - }, - { - name: "OK with picking policy ring_hash", - js: testJSONConfig4, - want: &LBConfig{ - DiscoveryMechanisms: []DiscoveryMechanism{ - { - Cluster: testClusterName, - LoadReportingServer: testLRSServerConfig, - MaxConcurrentRequests: newUint32(testMaxRequests), - Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSService, - outlierDetection: outlierdetection.LBConfig{ - Interval: iserviceconfig.Duration(10 * time.Second), // default interval - BaseEjectionTime: iserviceconfig.Duration(30 * time.Second), - MaxEjectionTime: iserviceconfig.Duration(300 * time.Second), - MaxEjectionPercent: 10, - // sre and fpe are both nil - }, - }, - }, - xdsLBPolicy: iserviceconfig.BalancerConfig{ - Name: ringhash.Name, - Config: &iringhash.LBConfig{ - // Ringhash LB config with default min and max. - MinRingSize: 1024, - MaxRingSize: 4096, - }, - }, - }, - wantErr: false, - }, - { - name: "noop-outlier-detection", - js: testJSONConfig5, - want: &LBConfig{ - DiscoveryMechanisms: []DiscoveryMechanism{ - { - Cluster: testClusterName, - LoadReportingServer: testLRSServerConfig, - MaxConcurrentRequests: newUint32(testMaxRequests), - Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSService, - outlierDetection: outlierdetection.LBConfig{ - Interval: iserviceconfig.Duration(10 * time.Second), // default interval - BaseEjectionTime: iserviceconfig.Duration(30 * time.Second), - MaxEjectionTime: iserviceconfig.Duration(300 * time.Second), - MaxEjectionPercent: 10, - // sre and fpe are both nil - }, - }, - }, - xdsLBPolicy: iserviceconfig.BalancerConfig{ - Name: roundrobin.Name, - Config: nil, - }, - }, - wantErr: false, - }, - } - for _, tt := range tests { - b := balancer.Get(Name) - if b == nil { - t.Fatalf("LB policy %q not registered", Name) - } - cfgParser, ok := b.(balancer.ConfigParser) - if !ok { - t.Fatalf("LB policy %q does not support config parsing", Name) - } - t.Run(tt.name, func(t *testing.T) { - got, err := cfgParser.ParseConfig([]byte(tt.js)) - if (err != nil) != tt.wantErr { - t.Fatalf("parseConfig() error = %v, wantErr %v", err, tt.wantErr) - } - if tt.wantErr { - return - } - if diff := cmp.Diff(got, tt.want, cmp.AllowUnexported(LBConfig{}), cmpopts.IgnoreFields(LBConfig{}, "XDSLBPolicy")); diff != "" { - t.Errorf("parseConfig() got unexpected output, diff (-got +want): %v", diff) - } - }) - } -} - -func newUint32(i uint32) *uint32 { - return &i -} diff --git a/internal/xds/balancer/clusterresolver/logging.go b/internal/xds/balancer/clusterresolver/logging.go deleted file mode 100644 index 728f1f709c28..000000000000 --- a/internal/xds/balancer/clusterresolver/logging.go +++ /dev/null @@ -1,34 +0,0 @@ -/* - * - * Copyright 2020 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -package clusterresolver - -import ( - "fmt" - - "google.golang.org/grpc/grpclog" - internalgrpclog "google.golang.org/grpc/internal/grpclog" -) - -const prefix = "[xds-cluster-resolver-lb %p] " - -var logger = grpclog.Component("xds") - -func prefixLogger(p *clusterResolverBalancer) *internalgrpclog.PrefixLogger { - return internalgrpclog.NewPrefixLogger(logger, fmt.Sprintf(prefix, p)) -} diff --git a/internal/xds/balancer/clusterresolver/resource_resolver.go b/internal/xds/balancer/clusterresolver/resource_resolver.go deleted file mode 100644 index 90e950c73332..000000000000 --- a/internal/xds/balancer/clusterresolver/resource_resolver.go +++ /dev/null @@ -1,322 +0,0 @@ -/* - * - * Copyright 2021 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -package clusterresolver - -import ( - "context" - "sync" - - "google.golang.org/grpc/internal/grpclog" - "google.golang.org/grpc/internal/grpcsync" - "google.golang.org/grpc/internal/xds/xdsclient/xdsresource" - "google.golang.org/grpc/resolver" -) - -// resourceUpdate is a combined update from all the resources, in the order of -// priority. For example, it can be {EDS, EDS, DNS}. -type resourceUpdate struct { - // A discovery mechanism would return an empty update when it runs into - // errors, and this would result in the priority LB policy reporting - // TRANSIENT_FAILURE (if there was a single discovery mechanism), or would - // fallback to the next highest priority that is available. - priorities []priorityConfig - // To be invoked once the update is completely processed, or is dropped in - // favor of a newer update. - onDone func() -} - -// topLevelResolver is used by concrete endpointsResolver implementations for -// reporting updates and errors. The `resourceResolver` type implements this -// interface and takes appropriate actions upon receipt of updates and errors -// from underlying concrete resolvers. -type topLevelResolver interface { - // onUpdate is called when a new update is received from the underlying - // endpointsResolver implementation. The onDone callback is to be invoked - // once the update is completely processed, or is dropped in favor of a - // newer update. - onUpdate(onDone func()) -} - -// endpointsResolver wraps the functionality to resolve a given resource name to -// a set of endpoints. The mechanism used by concrete implementations depend on -// the supported discovery mechanism type. -type endpointsResolver interface { - // lastUpdate returns endpoint results from the most recent resolution. - // - // The type of the first return result is dependent on the resolver - // implementation. - // - // The second return result indicates whether the resolver was able to - // successfully resolve the resource name to endpoints. If set to false, the - // first return result is invalid and must not be used. - lastUpdate() (any, bool) - - // resolverNow triggers re-resolution of the resource. - resolveNow() - - // stop stops resolution of the resource. Implementations must not invoke - // any methods on the topLevelResolver interface once `stop()` returns. - stop() -} - -// discoveryMechanismKey is {type+resource_name}, it's used as the map key, so -// that the same resource resolver can be reused (e.g. when there are two -// mechanisms, both for the same EDS resource, but has different circuit -// breaking config). -type discoveryMechanismKey struct { - typ DiscoveryMechanismType - name string -} - -// discoveryMechanismAndResolver is needed to keep the resolver and the -// discovery mechanism together, because resolvers can be shared. And we need -// the mechanism for fields like circuit breaking, LRS etc when generating the -// balancer config. -type discoveryMechanismAndResolver struct { - dm DiscoveryMechanism - r endpointsResolver - - childNameGen *nameGenerator -} - -type resourceResolver struct { - parent *clusterResolverBalancer - logger *grpclog.PrefixLogger - updateChannel chan *resourceUpdate - serializer *grpcsync.CallbackSerializer - serializerCancel context.CancelFunc - - // mu protects the slice and map, and content of the resolvers in the slice. - mu sync.Mutex - mechanisms []DiscoveryMechanism - children []discoveryMechanismAndResolver - // childrenMap's value only needs the resolver implementation (type - // discoveryMechanism) and the childNameGen. The other two fields are not - // used. - // - // TODO(cleanup): maybe we can make a new type with just the necessary - // fields, and use it here instead. - childrenMap map[discoveryMechanismKey]discoveryMechanismAndResolver - // Each new discovery mechanism needs a child name generator to reuse child - // policy names. But to make sure the names across discover mechanism - // doesn't conflict, we need a seq ID. This ID is incremented for each new - // discover mechanism. - childNameGeneratorSeqID uint64 -} - -func newResourceResolver(parent *clusterResolverBalancer, logger *grpclog.PrefixLogger) *resourceResolver { - rr := &resourceResolver{ - parent: parent, - logger: logger, - updateChannel: make(chan *resourceUpdate, 1), - childrenMap: make(map[discoveryMechanismKey]discoveryMechanismAndResolver), - } - ctx, cancel := context.WithCancel(context.Background()) - rr.serializer = grpcsync.NewCallbackSerializer(ctx) - rr.serializerCancel = cancel - return rr -} - -func equalDiscoveryMechanisms(a, b []DiscoveryMechanism) bool { - if len(a) != len(b) { - return false - } - for i, aa := range a { - bb := b[i] - if !aa.Equal(bb) { - return false - } - } - return true -} - -func discoveryMechanismToKey(dm DiscoveryMechanism) discoveryMechanismKey { - switch dm.Type { - case DiscoveryMechanismTypeEDS: - nameToWatch := dm.EDSServiceName - if nameToWatch == "" { - nameToWatch = dm.Cluster - } - return discoveryMechanismKey{typ: dm.Type, name: nameToWatch} - case DiscoveryMechanismTypeLogicalDNS: - return discoveryMechanismKey{typ: dm.Type, name: dm.DNSHostname} - default: - return discoveryMechanismKey{} - } -} - -func (rr *resourceResolver) updateMechanisms(mechanisms []DiscoveryMechanism) { - rr.mu.Lock() - defer rr.mu.Unlock() - if equalDiscoveryMechanisms(rr.mechanisms, mechanisms) { - return - } - rr.mechanisms = mechanisms - rr.children = make([]discoveryMechanismAndResolver, len(mechanisms)) - newDMs := make(map[discoveryMechanismKey]bool) - - // Start one watch for each new discover mechanism {type+resource_name}. - for i, dm := range mechanisms { - dmKey := discoveryMechanismToKey(dm) - newDMs[dmKey] = true - dmAndResolver, ok := rr.childrenMap[dmKey] - if ok { - // If this is not new, keep the fields (especially childNameGen), - // and only update the DiscoveryMechanism. - // - // Note that the same dmKey doesn't mean the same - // DiscoveryMechanism. There are fields (e.g. - // MaxConcurrentRequests) in DiscoveryMechanism that are not copied - // to dmKey, we need to keep those updated. - dmAndResolver.dm = dm - rr.children[i] = dmAndResolver - continue - } - - // Create resolver for a newly seen resource. - var resolver endpointsResolver - switch dm.Type { - case DiscoveryMechanismTypeEDS: - resolver = newEDSResolver(dmKey.name, rr.parent.xdsClient, rr, rr.logger) - case DiscoveryMechanismTypeLogicalDNS: - resolver = newDNSResolver(dmKey.name, rr, rr.logger) - } - dmAndResolver = discoveryMechanismAndResolver{ - dm: dm, - r: resolver, - childNameGen: newNameGenerator(rr.childNameGeneratorSeqID), - } - rr.childrenMap[dmKey] = dmAndResolver - rr.children[i] = dmAndResolver - rr.childNameGeneratorSeqID++ - } - - // Stop the resources that were removed. - for dm, r := range rr.childrenMap { - if !newDMs[dm] { - delete(rr.childrenMap, dm) - go r.r.stop() - } - } - // Regenerate even if there's no change in discovery mechanism, in case - // priority order changed. - rr.generateLocked(func() {}) -} - -// resolveNow is typically called to trigger re-resolve of DNS. The EDS -// resolveNow() is a noop. -func (rr *resourceResolver) resolveNow() { - rr.mu.Lock() - defer rr.mu.Unlock() - for _, r := range rr.childrenMap { - r.r.resolveNow() - } -} - -func (rr *resourceResolver) stop(closing bool) { - rr.mu.Lock() - - // Save the previous childrenMap to stop the children outside the mutex, - // and reinitialize the map. We only need to reinitialize to allow for the - // policy to be reused if the resource comes back. In practice, this does - // not happen as the parent LB policy will also be closed, causing this to - // be removed entirely, but a future use case might want to reuse the - // policy instead. - cm := rr.childrenMap - rr.childrenMap = make(map[discoveryMechanismKey]discoveryMechanismAndResolver) - rr.mechanisms = nil - rr.children = nil - - rr.mu.Unlock() - - for _, r := range cm { - r.r.stop() - } - - if closing { - rr.serializerCancel() - <-rr.serializer.Done() - } - - // stop() is called when the LB policy is closed or when the underlying - // cluster resource is removed by the management server. In the latter case, - // an empty config update needs to be pushed to the child policy to ensure - // that a picker that fails RPCs is sent up to the channel. - // - // Resource resolver implementations are expected to not send any updates - // after they are stopped. Therefore, we don't have to worry about another - // write to this channel happening at the same time as this one. - select { - case ru := <-rr.updateChannel: - if ru.onDone != nil { - ru.onDone() - } - default: - } - rr.updateChannel <- &resourceUpdate{} -} - -// generateLocked collects updates from all resolvers. It pushes the combined -// result on the update channel if all child resolvers have received at least -// one update. Otherwise it returns early. -// -// The onDone callback is invoked inline if not all child resolvers have -// received at least one update. If all child resolvers have received at least -// one update, onDone is invoked when the combined update is processed by the -// clusterresolver LB policy. -// -// Caller must hold rr.mu. -func (rr *resourceResolver) generateLocked(onDone func()) { - var ret []priorityConfig - for _, rDM := range rr.children { - u, ok := rDM.r.lastUpdate() - if !ok { - // Don't send updates to parent until all resolvers have update to - // send. - onDone() - return - } - switch uu := u.(type) { - case xdsresource.EndpointsUpdate: - ret = append(ret, priorityConfig{mechanism: rDM.dm, edsResp: uu, childNameGen: rDM.childNameGen}) - case []resolver.Endpoint: - ret = append(ret, priorityConfig{mechanism: rDM.dm, endpoints: uu, childNameGen: rDM.childNameGen}) - } - } - select { - // A previously unprocessed update is dropped in favor of the new one, and - // the former's onDone callback is invoked to unblock the xDS client's - // receive path. - case ru := <-rr.updateChannel: - if ru.onDone != nil { - ru.onDone() - } - default: - } - rr.updateChannel <- &resourceUpdate{priorities: ret, onDone: onDone} -} - -func (rr *resourceResolver) onUpdate(onDone func()) { - handleUpdate := func(context.Context) { - rr.mu.Lock() - rr.generateLocked(onDone) - rr.mu.Unlock() - } - rr.serializer.ScheduleOr(handleUpdate, func() { onDone() }) -} diff --git a/internal/xds/balancer/clusterresolver/resource_resolver_dns.go b/internal/xds/balancer/clusterresolver/resource_resolver_dns.go deleted file mode 100644 index ea292e712d19..000000000000 --- a/internal/xds/balancer/clusterresolver/resource_resolver_dns.go +++ /dev/null @@ -1,172 +0,0 @@ -/* - * - * Copyright 2021 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -package clusterresolver - -import ( - "fmt" - "net/url" - "sync" - - "google.golang.org/grpc/internal/grpclog" - "google.golang.org/grpc/internal/pretty" - "google.golang.org/grpc/resolver" - "google.golang.org/grpc/serviceconfig" -) - -var ( - newDNS = func(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { - // The dns resolver is registered by the grpc package. So, this call to - // resolver.Get() is never expected to return nil. - return resolver.Get("dns").Build(target, cc, opts) - } -) - -// dnsDiscoveryMechanism watches updates for the given DNS hostname. -// -// It implements resolver.ClientConn interface to work with the DNS resolver. -type dnsDiscoveryMechanism struct { - target string - topLevelResolver topLevelResolver - dnsR resolver.Resolver - logger *grpclog.PrefixLogger - - mu sync.Mutex - endpoints []resolver.Endpoint - updateReceived bool -} - -// newDNSResolver creates an endpoints resolver which uses a DNS resolver under -// the hood. -// -// An error in parsing the provided target string or an error in creating a DNS -// resolver means that we will never be able to resolve the provided target -// strings to endpoints. The topLevelResolver propagates address updates to the -// clusterresolver LB policy **only** after it receives updates from all its -// child resolvers. Therefore, an error here means that the topLevelResolver -// will never send address updates to the clusterresolver LB policy. -// -// Calling the onError() callback will ensure that this error is -// propagated to the child policy which eventually move the channel to -// transient failure. -// -// The `dnsR` field is unset if we run into errors in this function. Therefore, a -// nil check is required wherever we access that field. -func newDNSResolver(target string, topLevelResolver topLevelResolver, logger *grpclog.PrefixLogger) *dnsDiscoveryMechanism { - ret := &dnsDiscoveryMechanism{ - target: target, - topLevelResolver: topLevelResolver, - logger: logger, - } - u, err := url.Parse("dns:///" + target) - if err != nil { - if ret.logger.V(2) { - ret.logger.Infof("Failed to parse dns hostname %q in clusterresolver LB policy", target) - } - ret.updateReceived = true - ret.topLevelResolver.onUpdate(func() {}) - return ret - } - - r, err := newDNS(resolver.Target{URL: *u}, ret, resolver.BuildOptions{}) - if err != nil { - if ret.logger.V(2) { - ret.logger.Infof("Failed to build DNS resolver for target %q: %v", target, err) - } - ret.updateReceived = true - ret.topLevelResolver.onUpdate(func() {}) - return ret - } - ret.dnsR = r - return ret -} - -func (dr *dnsDiscoveryMechanism) lastUpdate() (any, bool) { - dr.mu.Lock() - defer dr.mu.Unlock() - - if !dr.updateReceived { - return nil, false - } - return dr.endpoints, true -} - -func (dr *dnsDiscoveryMechanism) resolveNow() { - if dr.dnsR != nil { - dr.dnsR.ResolveNow(resolver.ResolveNowOptions{}) - } -} - -// The definition of stop() mentions that implementations must not invoke any -// methods on the topLevelResolver once the call to `stop()` returns. The -// underlying dns resolver does not send any updates to the resolver.ClientConn -// interface passed to it (implemented by dnsDiscoveryMechanism in this case) -// after its `Close()` returns. Therefore, we can guarantee that no methods of -// the topLevelResolver are invoked after we return from this method. -func (dr *dnsDiscoveryMechanism) stop() { - if dr.dnsR != nil { - dr.dnsR.Close() - } -} - -// dnsDiscoveryMechanism needs to implement resolver.ClientConn interface to receive -// updates from the real DNS resolver. - -func (dr *dnsDiscoveryMechanism) UpdateState(state resolver.State) error { - if dr.logger.V(2) { - dr.logger.Infof("DNS discovery mechanism for resource %q reported an update: %s", dr.target, pretty.ToJSON(state)) - } - - dr.mu.Lock() - dr.endpoints = state.Endpoints - dr.updateReceived = true - dr.mu.Unlock() - - dr.topLevelResolver.onUpdate(func() {}) - return nil -} - -func (dr *dnsDiscoveryMechanism) ReportError(err error) { - if dr.logger.V(2) { - dr.logger.Infof("DNS discovery mechanism for resource %q reported error: %v", dr.target, err) - } - - dr.mu.Lock() - // If a previous good update was received, suppress the error and continue - // using the previous update. If RPCs were succeeding prior to this, they - // will continue to do so. Also suppress errors if we previously received an - // error, since there will be no downstream effects of propagating this - // error. - if dr.updateReceived { - dr.mu.Unlock() - return - } - dr.endpoints = nil - dr.updateReceived = true - dr.mu.Unlock() - - dr.topLevelResolver.onUpdate(func() {}) -} - -func (dr *dnsDiscoveryMechanism) NewAddress([]resolver.Address) { - dr.logger.Errorf("NewAddress called unexpectedly.") -} - -func (dr *dnsDiscoveryMechanism) ParseServiceConfig(string) *serviceconfig.ParseResult { - return &serviceconfig.ParseResult{Err: fmt.Errorf("service config not supported")} -} diff --git a/internal/xds/balancer/clusterresolver/resource_resolver_eds.go b/internal/xds/balancer/clusterresolver/resource_resolver_eds.go deleted file mode 100644 index 6dcdb898e5fb..000000000000 --- a/internal/xds/balancer/clusterresolver/resource_resolver_eds.go +++ /dev/null @@ -1,124 +0,0 @@ -/* - * - * Copyright 2023 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -package clusterresolver - -import ( - "sync" - - "google.golang.org/grpc/internal/grpclog" - "google.golang.org/grpc/internal/grpcsync" - "google.golang.org/grpc/internal/xds/xdsclient/xdsresource" -) - -type edsDiscoveryMechanism struct { - nameToWatch string - cancelWatch func() - topLevelResolver topLevelResolver - stopped *grpcsync.Event - logger *grpclog.PrefixLogger - - mu sync.Mutex - update *xdsresource.EndpointsUpdate // Nil indicates no update received so far. -} - -func (er *edsDiscoveryMechanism) lastUpdate() (any, bool) { - er.mu.Lock() - defer er.mu.Unlock() - - if er.update == nil { - return nil, false - } - return *er.update, true -} - -func (er *edsDiscoveryMechanism) resolveNow() { -} - -// The definition of stop() mentions that implementations must not invoke any -// methods on the topLevelResolver once the call to `stop()` returns. -func (er *edsDiscoveryMechanism) stop() { - // Canceling a watch with the xDS client can race with an xDS response - // received around the same time, and can result in the watch callback being - // invoked after the watch is canceled. Callers need to handle this race, - // and we fire the stopped event here to ensure that a watch callback - // invocation around the same time becomes a no-op. - er.stopped.Fire() - er.cancelWatch() -} - -// newEDSResolver returns an implementation of the endpointsResolver interface -// that uses EDS to resolve the given name to endpoints. -func newEDSResolver(nameToWatch string, producer xdsresource.Producer, topLevelResolver topLevelResolver, logger *grpclog.PrefixLogger) *edsDiscoveryMechanism { - ret := &edsDiscoveryMechanism{ - nameToWatch: nameToWatch, - topLevelResolver: topLevelResolver, - logger: logger, - stopped: grpcsync.NewEvent(), - } - ret.cancelWatch = xdsresource.WatchEndpoints(producer, nameToWatch, ret) - return ret -} - -// ResourceChanged is invoked to report an update for the resource being watched. -func (er *edsDiscoveryMechanism) ResourceChanged(update *xdsresource.EndpointsUpdate, onDone func()) { - if er.stopped.HasFired() { - onDone() - return - } - - er.mu.Lock() - er.update = update - er.mu.Unlock() - - er.topLevelResolver.onUpdate(onDone) -} - -func (er *edsDiscoveryMechanism) ResourceError(err error, onDone func()) { - if er.stopped.HasFired() { - onDone() - return - } - - if er.logger.V(2) { - er.logger.Infof("EDS discovery mechanism for resource %q reported resource error: %v", er.nameToWatch, err) - } - - // Report an empty update that would result in no priority child being - // created for this discovery mechanism. This would result in the priority - // LB policy reporting TRANSIENT_FAILURE (as there would be no priorities or - // localities) if this was the only discovery mechanism, or would result in - // the priority LB policy using a lower priority discovery mechanism when - // that becomes available. - er.mu.Lock() - er.update = &xdsresource.EndpointsUpdate{} - er.mu.Unlock() - - er.topLevelResolver.onUpdate(onDone) -} - -func (er *edsDiscoveryMechanism) AmbientError(err error, onDone func()) { - if er.stopped.HasFired() { - onDone() - return - } - - if er.logger.V(2) { - er.logger.Infof("EDS discovery mechanism for resource %q reported ambient error: %v", er.nameToWatch, err) - } -} diff --git a/internal/xds/resolver/helpers_test.go b/internal/xds/resolver/helpers_test.go index 1b6f711264de..f168b663ca5c 100644 --- a/internal/xds/resolver/helpers_test.go +++ b/internal/xds/resolver/helpers_test.go @@ -278,7 +278,7 @@ func waitForResourceNames(ctx context.Context, t *testing.T, namesCh chan []stri select { case <-ctx.Done(): case gotNames := <-namesCh: - if cmp.Equal(gotNames, wantNames, cmpopts.EquateEmpty()) { + if cmp.Equal(gotNames, wantNames, cmpopts.EquateEmpty(), cmpopts.SortSlices(func(a, b string) bool { return a < b })) { return } t.Logf("Received resource names %v, want %v", gotNames, wantNames) diff --git a/internal/xds/resolver/serviceconfig.go b/internal/xds/resolver/serviceconfig.go index 40a423f1f1e2..2846412470fb 100644 --- a/internal/xds/resolver/serviceconfig.go +++ b/internal/xds/resolver/serviceconfig.go @@ -75,12 +75,15 @@ type xdsClusterManagerConfig struct { // serviceConfigJSON produces a service config in JSON format that contains LB // policy config for the "xds_cluster_manager" LB policy, with entries in the // children map for all active clusters. -func serviceConfigJSON(activeClusters map[string]*clusterInfo) []byte { +func serviceConfigJSON(activeClusters map[string]*clusterInfo, activePlugins map[string]*clusterInfo) []byte { // Generate children (all entries in activeClusters). children := make(map[string]xdsChildConfig) for cluster, ci := range activeClusters { children[cluster] = ci.cfg } + for plugin, ci := range activePlugins { + children[plugin] = ci.cfg + } sc := serviceConfig{ LoadBalancingConfig: newBalancerConfig( @@ -156,6 +159,7 @@ type configSelector struct { virtualHost virtualHost routes []route clusters map[string]*clusterInfo + plugins map[string]*clusterInfo httpFilterConfig []xdsresource.HTTPFilter } @@ -194,7 +198,13 @@ func (cs *configSelector) SelectConfig(rpcInfo iresolver.RPCInfo) (*iresolver.RP // Add a ref to the selected cluster, as this RPC needs this cluster until // it is committed. - ref := &cs.clusters[cluster.name].refCount + var ref *int32 + if info, ok := cs.clusters[cluster.name]; ok { + ref = &info.refCount + } + if info, ok := cs.plugins[cluster.name]; ok { + ref = &info.refCount + } atomic.AddInt32(ref, 1) lbCtx := clustermanager.SetPickedCluster(rpcInfo.Context, cluster.name) @@ -209,10 +219,25 @@ func (cs *configSelector) SelectConfig(rpcInfo iresolver.RPCInfo) (*iresolver.RP OnCommitted: func() { // When the RPC is committed, the cluster is no longer required. // Decrease its ref. - if v := atomic.AddInt32(ref, -1); v == 0 { - // This entry will be removed from activeClusters when - // producing the service config for the empty update. - cs.sendNewServiceConfig() + if info, ok := cs.clusters[cluster.name]; ok { + ref := &info.refCount + if v := atomic.AddInt32(ref, -1); v == 0 { + // We call unsubscribe rather than sendNewServiceConfig to + // prevent redundant updates. If the reference count in the + // dependency manager drops to zero, it will automatically + // trigger a service config update with this cluster + // removed. Calling unsubscribe allows the dependency + // manager to handle the update flow once and for all. + info.unsubscribe() + } + } + if info, ok := cs.plugins[cluster.name]; ok { + ref := &info.refCount + if v := atomic.AddInt32(ref, -1); v == 0 { + // This entry will be removed from activePlugins when + // producing a new service config update. + cs.sendNewServiceConfig() + } } }, Interceptor: cluster.interceptor, @@ -311,21 +336,19 @@ func (cs *configSelector) stop() { if cs == nil { return } - // If any refs drop to zero, we'll need a service config update to delete - // the cluster. - needUpdate := false - // Loops over cs.clusters, but these are pointers to entries in - // activeClusters. + // If any reference counts drop to zero, a service config update is required + // to remove the clusters. Since the old config selector is stopped + // after a new one is active, we must trigger a subsequent update to delete + // the now-unused clusters. for _, ci := range cs.clusters { if v := atomic.AddInt32(&ci.refCount, -1); v == 0 { - needUpdate = true + ci.unsubscribe() } } - // We stop the old config selector immediately after sending a new config - // selector; we need another update to delete clusters from the config (if - // we don't have another update pending already). - if needUpdate { - cs.sendNewServiceConfig() + for _, ci := range cs.plugins { + if v := atomic.AddInt32(&ci.refCount, -1); v == 0 { + cs.sendNewServiceConfig() + } } } diff --git a/internal/xds/resolver/serviceconfig_test.go b/internal/xds/resolver/serviceconfig_test.go index f05a7d9144c0..ebf0d9615052 100644 --- a/internal/xds/resolver/serviceconfig_test.go +++ b/internal/xds/resolver/serviceconfig_test.go @@ -26,6 +26,7 @@ import ( xxhash "github.com/cespare/xxhash/v2" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/internal/grpcutil" iresolver "google.golang.org/grpc/internal/resolver" @@ -45,19 +46,34 @@ func Test(t *testing.T) { } func (s) TestPruneActiveClusters(t *testing.T) { - r := &xdsResolver{activeClusters: map[string]*clusterInfo{ - "zero": {refCount: 0}, - "one": {refCount: 1}, - "two": {refCount: 2}, - "anotherzero": {refCount: 0}, - }} - want := map[string]*clusterInfo{ + r := &xdsResolver{ + activeClusters: map[string]*clusterInfo{ + "zero": {refCount: 0, unsubscribe: func() {}}, + "one": {refCount: 1, unsubscribe: func() {}}, + "two": {refCount: 2, unsubscribe: func() {}}, + "anotherzero": {refCount: 0, unsubscribe: func() {}}, + }, + activePlugins: map[string]*clusterInfo{ + "zero": {refCount: 0}, + "one": {refCount: 1}, + "two": {refCount: 2}, + "anotherzero": {refCount: 0}, + }, + } + wantActiveClusters := map[string]*clusterInfo{ "one": {refCount: 1}, "two": {refCount: 2}, } - r.pruneActiveClusters() - if d := cmp.Diff(r.activeClusters, want, cmp.AllowUnexported(clusterInfo{})); d != "" { - t.Fatalf("r.activeClusters = %v; want %v\nDiffs: %v", r.activeClusters, want, d) + wantActivePlugins := map[string]*clusterInfo{ + "one": {refCount: 1}, + "two": {refCount: 2}, + } + r.pruneActiveClustersAndPlugins() + if d := cmp.Diff(r.activeClusters, wantActiveClusters, cmp.AllowUnexported(clusterInfo{}), cmpopts.IgnoreFields(clusterInfo{}, "unsubscribe")); d != "" { + t.Errorf("r.activeClusters = %v; want %v\nDiffs: %v", r.activeClusters, wantActiveClusters, d) + } + if d := cmp.Diff(r.activePlugins, wantActivePlugins, cmp.AllowUnexported(clusterInfo{})); d != "" { + t.Fatalf("r.activePlugins = %v; want %v\nDiffs: %v", r.activePlugins, wantActivePlugins, d) } } diff --git a/internal/xds/resolver/xds_resolver.go b/internal/xds/resolver/xds_resolver.go index d484957a7c0f..918ce623efb0 100644 --- a/internal/xds/resolver/xds_resolver.go +++ b/internal/xds/resolver/xds_resolver.go @@ -134,6 +134,7 @@ func (b *xdsResolverBuilder) Build(target resolver.Target, cc resolver.ClientCon xdsClient: client, xdsClientClose: xdsClientClose, activeClusters: make(map[string]*clusterInfo), + activePlugins: make(map[string]*clusterInfo), channelID: rand.Uint64(), ldsResourceName: ldsResourceName, @@ -239,8 +240,20 @@ type xdsResolver struct { // callbacks. xdsConfig *xdsresource.XDSConfig // activeClusters is a map from cluster name to information about the - // cluster that includes a ref count and load balancing configuration. - activeClusters map[string]*clusterInfo + // weighted cluster that includes a reference count and load balancing + // configuration. These counts are used only by the resolver. The current + // configSelector holds one reference, and each ongoing RPC holds an + // additional reference. When the count hits zero, the resolver removes the + // cluster from this map and calls unsubscribe. This signals the dependency + // manager to stop the xDS watch once its own reference count reaches zero. + activeClusters map[string]*clusterInfo + // activePlugins is a map from cluster specifier plugin name to information + // about the cluster specifier plugin that includes a ref count and load + // balancing configuration. These counts are used only by the resolver. The + // current configSelector holds one reference, and each ongoing RPC holds an + // additional reference. When the count hits zero, the resolver removes the + // plugin name from this map. + activePlugins map[string]*clusterInfo curConfigSelector stoppableConfigSelector } @@ -315,11 +328,9 @@ func (r *xdsResolver) sendNewServiceConfig(cs stoppableConfigSelector) bool { // Delete entries from r.activeClusters with zero references; // otherwise serviceConfigJSON will generate a config including // them. - r.pruneActiveClusters() + r.pruneActiveClustersAndPlugins() - errCS, ok := cs.(*erroringConfigSelector) - if ok && len(r.activeClusters) == 0 { - // There are no clusters and we are sending a failing configSelector. + if errCS, ok := cs.(*erroringConfigSelector); ok { // Send an empty config, which picks pick-first, with no address, and // puts the ClientConn into transient failure. // @@ -334,7 +345,7 @@ func (r *xdsResolver) sendNewServiceConfig(cs stoppableConfigSelector) bool { return true } - sc := serviceConfigJSON(r.activeClusters) + sc := serviceConfigJSON(r.activeClusters, r.activePlugins) if r.logger.V(2) { r.logger.Infof("For Listener resource %q and RouteConfiguration resource %q, generated service config: %+v", r.ldsResourceName, r.xdsConfig.Listener.APIListener.RouteConfigName, sc) } @@ -373,6 +384,7 @@ func (r *xdsResolver) newConfigSelector() (*configSelector, error) { }, routes: make([]route, len(r.xdsConfig.VirtualHost.Routes)), clusters: make(map[string]*clusterInfo), + plugins: make(map[string]*clusterInfo), httpFilterConfig: r.xdsConfig.Listener.APIListener.HTTPFilters, } @@ -381,9 +393,9 @@ func (r *xdsResolver) newConfigSelector() (*configSelector, error) { if rt.ClusterSpecifierPlugin != "" { clusterName := clusterSpecifierPluginPrefix + rt.ClusterSpecifierPlugin clusters.Add(&routeCluster{name: clusterName}, 1) - ci := r.addOrGetActiveClusterInfo(clusterName) + ci := r.addOrGetActiveClusterInfo(clusterName, "") ci.cfg = xdsChildConfig{ChildPolicy: balancerConfig(r.xdsConfig.RouteConfig.ClusterSpecifierPlugins[rt.ClusterSpecifierPlugin])} - cs.clusters[clusterName] = ci + cs.plugins[clusterName] = ci } else { for _, wc := range rt.WeightedClusters { clusterName := clusterPrefix + wc.Name @@ -395,7 +407,7 @@ func (r *xdsResolver) newConfigSelector() (*configSelector, error) { name: clusterName, interceptor: interceptor, }, int64(wc.Weight)) - ci := r.addOrGetActiveClusterInfo(clusterName) + ci := r.addOrGetActiveClusterInfo(clusterName, wc.Name) ci.cfg = xdsChildConfig{ChildPolicy: newBalancerConfig(cdsName, cdsBalancerConfig{Cluster: wc.Name})} cs.clusters[clusterName] = ci } @@ -415,34 +427,63 @@ func (r *xdsResolver) newConfigSelector() (*configSelector, error) { cs.routes[i].autoHostRewrite = rt.AutoHostRewrite } - // Account for this config selector's clusters. Do this after no further - // errors may occur. Note: cs.clusters are pointers to entries in + // Account for this config selector's clusters. Do this after no further + // errors may occur. Note: cs.clusters are pointers to entries in // activeClusters. for _, ci := range cs.clusters { atomic.AddInt32(&ci.refCount, 1) } + for _, ci := range cs.plugins { + atomic.AddInt32(&ci.refCount, 1) + } return cs, nil } -// pruneActiveClusters deletes entries in r.activeClusters with zero -// references. -func (r *xdsResolver) pruneActiveClusters() { +// pruneActiveClustersAndPlugins removes entries from activeClusters and +// activePlugins that have a reference count of zero. For clusters, it also +// invokes the unsubscribe function to signal the dependency manager to stop the +// xDS watch. Because cluster specifier plugins do not have their own watches, +// they are simply removed from the map without an unsubscribe call. +// +// Only executed in the context of a serializer callback. +func (r *xdsResolver) pruneActiveClustersAndPlugins() { for cluster, ci := range r.activeClusters { if atomic.LoadInt32(&ci.refCount) == 0 { + ci.unsubscribe() delete(r.activeClusters, cluster) } } + for cluster, ci := range r.activePlugins { + if atomic.LoadInt32(&ci.refCount) == 0 { + delete(r.activePlugins, cluster) + } + } } -func (r *xdsResolver) addOrGetActiveClusterInfo(name string) *clusterInfo { - ci := r.activeClusters[name] - if ci != nil { +// addOrGetActiveClusterInfo returns the clusterInfo for the provided key, +// creating it if it does not exist. It accepts the following parameters: +// - key: Formatted as "cluster:" or "cluster_specifier_plugin:", +// this is the lookup key for the activeClusters or activePlugins maps. +// - name: The actual xDS resource name used to initiate a CDS watch. +// If empty (e.g., for plugins), no resource watch is triggered. +// +// This function manages entry creation and xDS subscriptions but does not +// increment the reference count of the returned clusterInfo. +func (r *xdsResolver) addOrGetActiveClusterInfo(key string, name string) *clusterInfo { + if name == "" { + ci, ok := r.activePlugins[key] + if !ok { + ci = &clusterInfo{} + r.activePlugins[key] = ci + } return ci } - - ci = &clusterInfo{refCount: 0} - r.activeClusters[name] = ci + ci, ok := r.activeClusters[key] + if !ok { + ci = &clusterInfo{unsubscribe: r.dm.SubscribeToCluster(name)} + r.activeClusters[key] = ci + } return ci } @@ -452,6 +493,13 @@ type clusterInfo struct { // cfg is the child configuration for this cluster, containing either the // csp config or the cds cluster config. cfg xdsChildConfig + // unsubscribe is the function to call to unsubscribe from this cluster's + // CDS resource. It is populated only for clusters in activeClusters and not + // for cluster specifier plugins. When invoked, it decrements the reference + // count in the dependency manager; once that count reaches zero, the + // underlying CDS watch is terminated. Plugins do not have associated + // watches and therefore do not require an unsubscribe function. + unsubscribe func() } // Contains common functionality to be executed when resources of either type diff --git a/internal/xds/resolver/xds_resolver_test.go b/internal/xds/resolver/xds_resolver_test.go index 27a66e3c0d1a..a00412ad665b 100644 --- a/internal/xds/resolver/xds_resolver_test.go +++ b/internal/xds/resolver/xds_resolver_test.go @@ -648,8 +648,7 @@ func (s) TestResolverRequestHash(t *testing.T) { // Tests the case where resources are removed from the management server, // causing it to send an empty update to the xDS client, which returns a -// resource-not-found error to the xDS resolver. The test verifies that an -// ongoing RPC is handled to completion when this happens. +// resource-not-found error to the xDS resolver. func (s) TestResolverRemovedWithRPCs(t *testing.T) { // Spin up an xDS management server for the test. ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) @@ -677,28 +676,17 @@ func (s) TestResolverRemovedWithRPCs(t *testing.T) { t.Fatalf("cs.SelectConfig(): %v", err) } - // Delete the resources on the management server. This should result in a - // resource-not-found error from the xDS client. - if err := mgmtServer.Update(ctx, e2e.UpdateOptions{NodeID: nodeID}); err != nil { - t.Fatal(err) - } - - // The RPC started earlier is still in progress. So, the xDS resolver will - // not produce an empty service config at this point. Instead it will retain - // the cluster to which the RPC is ongoing in the service config, but will - // return an erroring config selector which will fail new RPCs. - cs = verifyUpdateFromResolver(ctx, t, stateCh, wantServiceConfig(resources.Clusters[0].Name)) - _, err = cs.SelectConfig(iresolver.RPCInfo{Context: ctx, Method: "/service/method"}) - if err := verifyResolverError(err, codes.Unavailable, "has been removed", nodeID); err != nil { + // Delete the listener resource on the management server. This should result + // in a resource-not-found error from the xDS client. + oldListeners := resources.Listeners + resources.Listeners = nil + resources.SkipValidation = true + if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) } - // "Finish the RPC"; this could cause a panic if the resolver doesn't - // handle it correctly. - res.OnCommitted() - - // Now that the RPC is committed, the xDS resolver is expected to send an - // update with an empty service config. + // Even though the RPC started earlier is still in progress, the xDS resolver will + // produce an empty service config. var state resolver.State select { case <-ctx.Done(): @@ -713,7 +701,12 @@ func (s) TestResolverRemovedWithRPCs(t *testing.T) { } } + // "Finish the RPC"; this could cause a panic if the resolver doesn't + // handle it correctly. + res.OnCommitted() + // Add the resources back. + resources.Listeners = oldListeners mgmtServer.Update(ctx, resources) // The resolver should send a service config with the cluster name. @@ -762,23 +755,15 @@ func (s) TestResolverRemovedResource(t *testing.T) { // handle it correctly. res.OnCommitted() - // Delete the resources on the management server, resulting in a + // Delete the listener resource on the management server, resulting in a // resource-not-found error from the xDS client. - if err := mgmtServer.Update(ctx, e2e.UpdateOptions{NodeID: nodeID}); err != nil { - t.Fatal(err) - } - - // The channel should receive the existing service config with the original - // cluster but with an erroring config selector. - cs = verifyUpdateFromResolver(ctx, t, stateCh, wantServiceConfig(resources.Clusters[0].Name)) - - // "Make another RPC" by invoking the config selector. - _, err = cs.SelectConfig(iresolver.RPCInfo{Context: ctx, Method: "/service/method"}) - if err := verifyResolverError(err, codes.Unavailable, "has been removed", nodeID); err != nil { + resources.Listeners = nil + resources.SkipValidation = true + if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) } - // In the meantime, an empty ServiceConfig update should have been sent. + // An empty ServiceConfig update should have been sent. var state resolver.State select { case <-ctx.Done(): @@ -994,8 +979,10 @@ func (s) TestResolverDelayedOnCommitted(t *testing.T) { newClusterName := "new-" + defaultTestClusterName newEndpointName := "new-" + defaultTestEndpointName resources.Routes = []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig(resources.Routes[0].Name, defaultTestServiceName, newClusterName)} - resources.Clusters = []*v3clusterpb.Cluster{e2e.DefaultCluster(newClusterName, newEndpointName, e2e.SecurityLevelNone)} - resources.Endpoints = []*v3endpointpb.ClusterLoadAssignment{e2e.DefaultEndpoint(newEndpointName, defaultTestHostname, defaultTestPort)} + // Appending the new cluster and endpoint resources to avoid getting + // resource removed errors. + resources.Clusters = append(resources.Clusters, e2e.DefaultCluster(newClusterName, newEndpointName, e2e.SecurityLevelNone)) + resources.Endpoints = append(resources.Endpoints, e2e.DefaultEndpoint(newEndpointName, defaultTestHostname, defaultTestPort)) if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) } @@ -1373,7 +1360,8 @@ func (s) TestResolver_AutoHostRewrite(t *testing.T) { }}, }}, }}, - SkipValidation: true, + Clusters: []*v3clusterpb.Cluster{e2e.DefaultCluster(defaultTestClusterName, defaultTestEndpointName, e2e.SecurityLevelNone)}, + Endpoints: []*v3endpointpb.ClusterLoadAssignment{e2e.DefaultEndpoint(defaultTestEndpointName, "localhost", []uint32{8080})}, } if err := mgmtServer.Update(ctx, resources); err != nil { @@ -1423,8 +1411,6 @@ func (s) TestResolver_AutoHostRewrite(t *testing.T) { // a cluster watch open when there are active RPCs using that cluster, even if // the cluster is no longer referenced by the current route configuration. func (s) TestResolverKeepWatchOpen_ActiveRPCs(t *testing.T) { - t.Skip("Will be enabled when all the watchers have shifted to dependency manager") - clusterA := "cluster-A" clusterB := "cluster-B" diff --git a/internal/xds/xdsdepmgr/xds_dependency_manager.go b/internal/xds/xdsdepmgr/xds_dependency_manager.go index 9dfe66884d45..7ce0218440b2 100644 --- a/internal/xds/xdsdepmgr/xds_dependency_manager.go +++ b/internal/xds/xdsdepmgr/xds_dependency_manager.go @@ -38,10 +38,6 @@ const prefix = "[xdsdepmgr %p] " var logger = grpclog.Component("xds") -// EnableClusterAndEndpointsWatch is a flag used to control whether the CDS/EDS -// watchers in the dependency manager should be used. -var EnableClusterAndEndpointsWatch = false - func prefixLogger(p *DependencyManager) *internalgrpclog.PrefixLogger { return internalgrpclog.NewPrefixLogger(logger, fmt.Sprintf(prefix, p)) } @@ -88,10 +84,6 @@ func (x *xdsResourceState[T, U]) setLastError(err error) { x.lastUpdate = nil } -func (x *xdsResourceState[T, U]) updateLastError(err error) { - x.lastErr = err -} - type dnsExtras struct { dnsR resolver.Resolver } @@ -232,11 +224,6 @@ func (m *DependencyManager) maybeSendUpdateLocked() { Clusters: make(map[string]*xdsresource.ClusterResult), } - if !EnableClusterAndEndpointsWatch { - m.watcher.Update(config) - return - } - edsResourcesSeen := make(map[string]bool) dnsResourcesSeen := make(map[string]bool) clusterResourcesSeen := make(map[string]bool) @@ -463,49 +450,46 @@ func (m *DependencyManager) applyRouteConfigUpdateLocked(update *xdsresource.Rou m.routeConfigWatcher.setLastUpdate(update) m.routeConfigWatcher.extras.virtualHost = matchVH - if EnableClusterAndEndpointsWatch { - // Get the clusters to be watched from the routes in the virtual host. - // If the ClusterSpecifierPlugin field is set, we ignore it for now as the - // clusters will be determined dynamically for it. - newClusters := make(map[string]bool) - - for _, rt := range matchVH.Routes { - for _, cluster := range rt.WeightedClusters { - newClusters[cluster.Name] = true - } + // Get the clusters to be watched from the routes in the virtual host. + // If the ClusterSpecifierPlugin field is set, we ignore it for now as the + // clusters will be determined dynamically for it. + newClusters := make(map[string]bool) + for _, rt := range matchVH.Routes { + for _, cluster := range rt.WeightedClusters { + newClusters[cluster.Name] = true } + } - // Add subscriptions for all new clusters. - for cluster := range newClusters { - // If the cluster already has a reference, increase its static - // reference. - if sub, ok := m.clusterSubscriptions[cluster]; ok { - sub.staticRefCount++ - continue - } - // If cluster is not present in subscriptions, add it with static - // ref count as 1. - m.clusterSubscriptions[cluster] = &clusterRef{ - staticRefCount: 1, - } + // Add subscriptions for all new clusters. + for cluster := range newClusters { + // If the cluster already has a reference, increase its static + // reference. + if sub, ok := m.clusterSubscriptions[cluster]; ok { + sub.staticRefCount++ + continue } + // If cluster is not present in subscriptions, add it with static + // ref count as 1. + m.clusterSubscriptions[cluster] = &clusterRef{ + staticRefCount: 1, + } + } - // Unsubscribe to clusters from last route config. - for cluster := range m.clustersFromLastRouteConfig { - clusterRef, ok := m.clusterSubscriptions[cluster] - if !ok { - // Should not reach here as the cluster was present in last - // route config so should be present in current cluster - // subscriptions. - continue - } - clusterRef.staticRefCount-- - if clusterRef.staticRefCount == 0 && clusterRef.dynamicRefCount == 0 { - delete(m.clusterSubscriptions, cluster) - } + // Unsubscribe to clusters from last route config. + for cluster := range m.clustersFromLastRouteConfig { + clusterRef, ok := m.clusterSubscriptions[cluster] + if !ok { + // Should not reach here as the cluster was present in last + // route config so should be present in current cluster + // subscriptions. + continue + } + clusterRef.staticRefCount-- + if clusterRef.staticRefCount == 0 && clusterRef.dynamicRefCount == 0 { + delete(m.clusterSubscriptions, cluster) } - m.clustersFromLastRouteConfig = newClusters } + m.clustersFromLastRouteConfig = newClusters // maybeSendUpdate is called to update the configuration with the new route, // start watching the newly added clusters and stop watching clusters that @@ -709,8 +693,8 @@ func (m *DependencyManager) onClusterResourceError(resourceName string, err erro m.maybeSendUpdateLocked() } -// Records the error in the state. The last successful update is retained -// because it should continue to be used as an amnbient error is received. +// Ambient errors from cluster resource are logged and the last successful +// update is retained because it should continue to be used. func (m *DependencyManager) onClusterAmbientError(resourceName string, err error, onDone func()) { m.mu.Lock() defer m.mu.Unlock() @@ -768,24 +752,26 @@ func (m *DependencyManager) onEndpointResourceError(resourceName string, err err return } m.logger.Warningf("Received resource error for Endpoint resource %q: %v", resourceName, m.annotateErrorWithNodeID(err)) - m.endpointWatchers[resourceName].setLastError(err) + // Send an empty EndpointsUpdate instead of nil to avoid nil-check handling + // in the CDS balancer. The priority balancer will handle the case of having + // no endpoints and transition the channel to Transient Failure if needed. + m.endpointWatchers[resourceName].lastUpdate = &xdsresource.EndpointsUpdate{} + m.endpointWatchers[resourceName].lastErr = err + m.endpointWatchers[resourceName].updateReceived = true m.maybeSendUpdateLocked() } -// Records the ambient error without clearing the last successful update, as the -// endpoints should continue to be used. +// Logs the ambient error and does not update the state, as the last successful +// update for endpoints should continue to be used. func (m *DependencyManager) onEndpointAmbientError(resourceName string, err error, onDone func()) { m.mu.Lock() defer m.mu.Unlock() - defer onDone() if m.stopped || m.endpointWatchers[resourceName] == nil { return } m.logger.Warningf("Endpoint resource ambient error %q: %v", resourceName, m.annotateErrorWithNodeID(err)) - m.endpointWatchers[resourceName].updateLastError(err) - m.maybeSendUpdateLocked() } // Converts the DNS resolver state to an internal update, handling address-only @@ -808,10 +794,10 @@ func (m *DependencyManager) onDNSUpdate(resourceName string, update *resolver.St // Records a DNS resolver error. It clears the last update only if no successful // update has been received yet, then triggers a dependency update. // -// If a previous good update was received, the error is recorded but the -// previous update is retained for continued use. Errors are suppressed if a -// resource error was already received, as further propagation would have no -// downstream effect. +// If a previous good update was received, the error is logged and the previous +// update is retained for continued use. Errors are suppressed if a resource +// error was already received, as further propagation would have no downstream +// effect. func (m *DependencyManager) onDNSError(resourceName string, err error) { m.mu.Lock() defer m.mu.Unlock() @@ -824,11 +810,15 @@ func (m *DependencyManager) onDNSError(resourceName string, err error) { m.logger.Warningf("%v", err) state := m.dnsResolvers[resourceName] if state.updateReceived { - state.updateLastError(err) return } - state.setLastError(err) + // Send an empty DNSUpdate instead of nil to avoid nil-check handling in the + // CDS balancer. The priority balancer will handle the case of having no + // endpoints and transition the channel to Transient Failure if needed. + state.lastUpdate = &xdsresource.DNSUpdate{} + state.lastErr = err + state.updateReceived = true m.maybeSendUpdateLocked() } diff --git a/internal/xds/xdsdepmgr/xds_dependency_manager_test.go b/internal/xds/xdsdepmgr/xds_dependency_manager_test.go index d051bce844f6..af0984961721 100644 --- a/internal/xds/xdsdepmgr/xds_dependency_manager_test.go +++ b/internal/xds/xdsdepmgr/xds_dependency_manager_test.go @@ -59,13 +59,12 @@ type s struct { } func Test(t *testing.T) { - xdsdepmgr.EnableClusterAndEndpointsWatch = true grpctest.RunSubTests(t, s{}) } const ( defaultTestTimeout = 10 * time.Second - defaultTestShortTimeout = 100 * time.Microsecond + defaultTestShortTimeout = 10 * time.Millisecond defaultTestServiceName = "service-name" defaultTestRouteConfigName = "route-config-name" @@ -1283,6 +1282,7 @@ func (s) TestAggregateClusterChildError(t *testing.T) { EDSServiceName: defaultTestEDSServiceName, }, EndpointConfig: &xdsresource.EndpointConfig{ + EDSUpdate: &xdsresource.EndpointsUpdate{}, ResolutionNote: fmt.Errorf("[xDS node id: %v]: %v", nodeID, fmt.Errorf("EDS response contains an endpoint with zero weight: endpoint:{address:{socket_address:{address:%q port_value:%v}}} load_balancing_weight:{}", "localhost", 8080)), }, }, @@ -1466,17 +1466,22 @@ func (s) TestAggregateClusterMaxDepth(t *testing.T) { } } -// Tests the scenario where the Endpoint watcher receives an ambient error. Tests -// verifies that the error is stored in resolution note and the update remains -// too. +// Tests the case where the dependency manager receives a endpoint resource +// ambient error. A valid endpoint resource is sent first, then an invalid +// one and then the valid resource again. The valid resource is sent again +// to make sure that the ambient error reaches the dependency manager since +// there is no other way to wait for it. func (s) TestEndpointAmbientError(t *testing.T) { - nodeID, mgmtServer, xdsClient := setupManagementServerAndClient(t, true) + // Expect a warning log for the ambient error. + grpctest.ExpectWarning("Endpoint resource ambient error") + + nodeID, mgmtServer, xdsClient := setupManagementServerAndClient(t, false) watcher := newTestWatcher() + defer watcher.close() ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() - resources := e2e.DefaultClientResources(e2e.ResourceParams{ NodeID: nodeID, DialTarget: defaultTestServiceName, @@ -1484,6 +1489,7 @@ func (s) TestEndpointAmbientError(t *testing.T) { Port: 8080, SecLevel: e2e.SecurityLevelNone, }) + if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) } @@ -1491,27 +1497,38 @@ func (s) TestEndpointAmbientError(t *testing.T) { dm := xdsdepmgr.New(defaultTestServiceName, defaultTestServiceName, xdsClient, watcher) defer dm.Close() - // Defer closing the watcher to prevent a potential hang. The management - // server may send repeated errors, triggering updates that hold the - // dependency manager's mutex. This defer is defined last so it executes - // first (before dm.Close()). If we don't stop the watcher, dm.Close() will - // deadlock waiting for the mutex currently held by the blocking Update - // call. - defer watcher.close() - - wantXdsConfig := makeXDSConfig(resources.Routes[0].Name, resources.Clusters[0].Name, resources.Endpoints[0].ClusterName, "localhost:8080") - + wantXdsConfig := makeXDSConfig(resources.Routes[0].Name, resources.Clusters[0].Name, resources.Clusters[0].EdsClusterConfig.ServiceName, "localhost:8080") if err := verifyXDSConfig(ctx, watcher.updateCh, watcher.errorCh, wantXdsConfig); err != nil { t.Fatal(err) } - // Send an ambient error for the endpoint resource by setting the weight to - // 0. - resources.Endpoints[0].Endpoints[0].LbEndpoints[0].LoadBalancingWeight = &wrapperspb.UInt32Value{Value: 0} + // Configure a endpoint resource that is expected to be NACKed because it + // does not contain the `Locality` field. Since a valid one is already + // cached, this should result in an ambient error. + resources.Endpoints[0].Endpoints[0].Locality = nil if err := mgmtServer.Update(ctx, resources); err != nil { t.Fatal(err) } - wantXdsConfig.Clusters[resources.Clusters[0].Name].Config.EndpointConfig.ResolutionNote = fmt.Errorf("[xDS node id: %v]: %v", nodeID, fmt.Errorf("EDS response contains an endpoint with zero weight: endpoint:{address:{socket_address:{address:%q port_value:%v}}} load_balancing_weight:{}", "localhost", 8080)) + + select { + case <-time.After(defaultTestShortTimeout): + case update := <-watcher.updateCh: + t.Fatalf("received unexpected update from dependency manager: %v", update) + } + + // Send valid resources again to guarantee we get the cluster ambient error + // before the test ends. + resources = e2e.DefaultClientResources(e2e.ResourceParams{ + NodeID: nodeID, + DialTarget: defaultTestServiceName, + Host: "localhost", + Port: 8080, + SecLevel: e2e.SecurityLevelNone, + }) + if err := mgmtServer.Update(ctx, resources); err != nil { + t.Fatal(err) + } + if err := verifyXDSConfig(ctx, watcher.updateCh, watcher.errorCh, wantXdsConfig); err != nil { t.Fatal(err) }