Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xds/resolver: Add support for cluster specifier plugins #4987

Merged
merged 7 commits into from Dec 6, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
63 changes: 45 additions & 18 deletions xds/internal/resolver/serviceconfig.go
Expand Up @@ -38,14 +38,17 @@ import (
"google.golang.org/grpc/status"
"google.golang.org/grpc/xds/internal/balancer/clustermanager"
"google.golang.org/grpc/xds/internal/balancer/ringhash"
"google.golang.org/grpc/xds/internal/clusterspecifier"
"google.golang.org/grpc/xds/internal/httpfilter"
"google.golang.org/grpc/xds/internal/httpfilter/router"
"google.golang.org/grpc/xds/internal/xdsclient/xdsresource"
)

const (
cdsName = "cds_experimental"
xdsClusterManagerName = "xds_cluster_manager_experimental"
cdsName = "cds_experimental"
xdsClusterManagerName = "xds_cluster_manager_experimental"
clusterPrefix = "cluster:"
clusterSpecifierPluginPrefix = "cluster_specifier_plugin:"
)

type serviceConfig struct {
Expand Down Expand Up @@ -86,9 +89,15 @@ func (r *xdsResolver) pruneActiveClusters() {
func serviceConfigJSON(activeClusters map[string]*clusterInfo) ([]byte, error) {
// Generate children (all entries in activeClusters).
children := make(map[string]xdsChildConfig)
for cluster := range activeClusters {
children[cluster] = xdsChildConfig{
ChildPolicy: newBalancerConfig(cdsName, cdsBalancerConfig{Cluster: cluster}),
for cluster, ci := range activeClusters {
if ci.cspCfg != nil {
children[cluster] = xdsChildConfig{
ChildPolicy: balancerConfig(ci.cspCfg),
}
} else {
children[cluster] = xdsChildConfig{
ChildPolicy: newBalancerConfig(cdsName, cdsBalancerConfig{Cluster: strings.TrimPrefix(cluster, clusterPrefix)}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we store this in the activeClusters map, too, instead of creating it here? That would give us a more unified flow through here. I.e.:

type clusterInfo struct {
	...
	cfg	balancerConfig  // either the csp config or the cds cluster config
}

And this loop becomes simply:

for cluster, ci := range activeClusters {
	children[cluster] = xdsChildConfig{ChildPolicy: ci.cfg}
}

(Or store an xdsChildConfig in clusterInfo, even.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about this for a while, chose to persist the whole child config.

}
}
}

Expand Down Expand Up @@ -158,10 +167,12 @@ func (cs *configSelector) SelectConfig(rpcInfo iresolver.RPCInfo) (*iresolver.RP
if rt == nil || rt.clusters == nil {
return nil, errNoMatchedRouteFound
}

cluster, ok := rt.clusters.Next().(*routeCluster)
if !ok {
return nil, status.Errorf(codes.Internal, "error retrieving cluster for match: %v (%T)", cluster, cluster)
}

// Add a ref to the selected cluster, as this RPC needs this cluster until
// it is committed.
ref := &cs.clusters[cluster.name].refCount
Expand Down Expand Up @@ -353,21 +364,22 @@ func (r *xdsResolver) newConfigSelector(su serviceUpdate) (*configSelector, erro

for i, rt := range su.virtualHost.Routes {
clusters := newWRR()
for cluster, wc := range rt.WeightedClusters {
if rt.ClusterSpecifierPlugin != "" {
clusterName := clusterSpecifierPluginPrefix + rt.ClusterSpecifierPlugin
clusters.Add(&routeCluster{
name: cluster,
httpFilterConfigOverride: wc.HTTPFilterConfigOverride,
}, int64(wc.Weight))

// Initialize entries in cs.clusters map, creating entries in
// r.activeClusters as necessary. Set to zero as they will be
// incremented by incRefs.
ci := r.activeClusters[cluster]
if ci == nil {
ci = &clusterInfo{refCount: 0}
r.activeClusters[cluster] = ci
name: clusterName,
}, 1)
r.initializeCluster(clusterName, cs)
r.activeClusters[clusterName].cspCfg = su.clusterSpecifierPlugins[rt.ClusterSpecifierPlugin]
} else {
for cluster, wc := range rt.WeightedClusters {
clusterName := clusterPrefix + cluster
clusters.Add(&routeCluster{
name: clusterName,
httpFilterConfigOverride: wc.HTTPFilterConfigOverride,
}, int64(wc.Weight))
r.initializeCluster(clusterName, cs)
}
cs.clusters[cluster] = ci
}
cs.routes[i].clusters = clusters

Expand Down Expand Up @@ -397,9 +409,24 @@ func (r *xdsResolver) newConfigSelector(su serviceUpdate) (*configSelector, erro
return cs, nil
}

// initializeCluster initializes entries in cs.clusters map, creating entries in
// r.activeClusters as necessary. Any created entries will be set to zero as
// they will be incremented by incRefs.
func (r *xdsResolver) initializeCluster(clusterName string, cs *configSelector) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this wants to be a method on configSelector instead. It can access the xdsResolver that way, too for adding to activeClusters. You can also pass it a balancerConfig to store in the clusterInfo as mentioned above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, great idea. These two comments make it much cleaner. Switched.

ci := r.activeClusters[clusterName]
if ci == nil {
ci = &clusterInfo{refCount: 0}
r.activeClusters[clusterName] = ci
}
cs.clusters[clusterName] = ci
}

type clusterInfo struct {
// number of references to this cluster; accessed atomically
refCount int32
// cspCfg is the configuration for this cluster if the cluster is a cluster
// specifier plugin. This will be nil otherwise.
cspCfg clusterspecifier.BalancerConfig
}

type interceptorList struct {
Expand Down
11 changes: 8 additions & 3 deletions xds/internal/resolver/watch_service.go
Expand Up @@ -25,6 +25,7 @@ import (

"google.golang.org/grpc/internal/grpclog"
"google.golang.org/grpc/internal/pretty"
"google.golang.org/grpc/xds/internal/clusterspecifier"
"google.golang.org/grpc/xds/internal/xdsclient"
"google.golang.org/grpc/xds/internal/xdsclient/xdsresource"
)
Expand All @@ -35,6 +36,9 @@ import (
type serviceUpdate struct {
// virtualHost contains routes and other configuration to route RPCs.
virtualHost *xdsresource.VirtualHost
// clusterSpecifierPlugins contain the configurations for any cluster
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: containS (the ...Plugins map is a singular)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, switched.

// specifier plugins emitted by the xdsclient.
clusterSpecifierPlugins map[string]clusterspecifier.BalancerConfig
// ldsConfig contains configuration that applies to all routes.
ldsConfig ldsConfig
}
Expand Down Expand Up @@ -120,7 +124,7 @@ func (w *serviceUpdateWatcher) handleLDSResp(update xdsresource.ListenerUpdate,
}

// Handle the inline RDS update as if it's from an RDS watch.
w.updateVirtualHostsFromRDS(*update.InlineRouteConfig)
w.applyRouteConfigUpdate(*update.InlineRouteConfig)
return
}

Expand Down Expand Up @@ -151,7 +155,7 @@ func (w *serviceUpdateWatcher) handleLDSResp(update xdsresource.ListenerUpdate,
w.rdsCancel = w.c.WatchRouteConfig(update.RouteConfigName, w.handleRDSResp)
}

func (w *serviceUpdateWatcher) updateVirtualHostsFromRDS(update xdsresource.RouteConfigUpdate) {
func (w *serviceUpdateWatcher) applyRouteConfigUpdate(update xdsresource.RouteConfigUpdate) {
matchVh := xdsresource.FindBestMatchingVirtualHost(w.serviceName, update.VirtualHosts)
if matchVh == nil {
// No matching virtual host found.
Expand All @@ -160,6 +164,7 @@ func (w *serviceUpdateWatcher) updateVirtualHostsFromRDS(update xdsresource.Rout
}

w.lastUpdate.virtualHost = matchVh
w.lastUpdate.clusterSpecifierPlugins = update.ClusterSpecifierPlugins
dfawley marked this conversation as resolved.
Show resolved Hide resolved
w.serviceCb(w.lastUpdate, nil)
}

Expand All @@ -179,7 +184,7 @@ func (w *serviceUpdateWatcher) handleRDSResp(update xdsresource.RouteConfigUpdat
w.serviceCb(serviceUpdate{}, err)
return
}
w.updateVirtualHostsFromRDS(update)
w.applyRouteConfigUpdate(update)
}

func (w *serviceUpdateWatcher) close() {
Expand Down
1 change: 0 additions & 1 deletion xds/internal/resolver/xds_resolver.go
Expand Up @@ -205,7 +205,6 @@ func (r *xdsResolver) sendNewServiceConfig(cs *configSelector) bool {
return true
}

// Produce the service config.
sc, err := serviceConfigJSON(r.activeClusters)
if err != nil {
// JSON marshal error; should never happen.
Expand Down