Skip to content

Commit c8266c9

Browse files
xdsresolver: use the refcounted utility for cluster refcounting
Replace the hand-rolled atomic.Int32 in clusterInfo with grpcsync.RefCounted. The "decrement, and if the count hit zero run the cleanup" logic was duplicated at three sites; it now lives in a single onZero callback registered when the entry is created. Entries remove themselves from activeClusters/activePlugins when their last reference is released, so pruneActiveClustersAndPlugins is no longer needed. Removal is scheduled on the serializer, since the last reference is usually released by an RPC completing on an arbitrary goroutine, and is guarded by an identity check so a pending removal cannot evict a newer entry that has since taken the same key. References for a config selector are now taken as each cluster is recorded rather than in a batch after all routes are built, so a config selector that fails partway through releases exactly what it acquired. RELEASE NOTES: none
1 parent ec03539 commit c8266c9

3 files changed

Lines changed: 237 additions & 170 deletions

File tree

internal/xds/resolver/serviceconfig.go

Lines changed: 32 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -75,14 +75,14 @@ type xdsClusterManagerConfig struct {
7575
// serviceConfigJSON produces a service config in JSON format that contains LB
7676
// policy config for the "xds_cluster_manager" LB policy, with entries in the
7777
// children map for all active clusters.
78-
func serviceConfigJSON(activeClusters map[string]*clusterInfo, activePlugins map[string]*clusterInfo) []byte {
78+
func serviceConfigJSON(activeClusters, activePlugins map[string]*grpcsync.RefCounted[*clusterInfo]) []byte {
7979
// Generate children (all entries in activeClusters).
8080
children := make(map[string]xdsChildConfig)
8181
for cluster, ci := range activeClusters {
82-
children[cluster] = ci.cfg
82+
children[cluster] = ci.Value().cfg
8383
}
8484
for plugin, ci := range activePlugins {
85-
children[plugin] = ci.cfg
85+
children[plugin] = ci.Value().cfg
8686
}
8787

8888
sc := serviceConfig{
@@ -159,8 +159,8 @@ type configSelector struct {
159159
// Configuration received from the xDS management server.
160160
virtualHost virtualHost
161161
routes []route
162-
clusters map[string]*clusterInfo
163-
plugins map[string]*clusterInfo
162+
clusters map[string]*grpcsync.RefCounted[*clusterInfo]
163+
plugins map[string]*grpcsync.RefCounted[*clusterInfo]
164164
httpFilterConfig []xdsresource.HTTPFilter
165165
xdsConfig *xdsresource.XDSConfig
166166
}
@@ -220,46 +220,30 @@ func (cs *configSelector) SelectConfig(rpcInfo iresolver.RPCInfo) (*iresolver.RP
220220
Context: lbCtx,
221221
Interceptor: cluster.interceptor,
222222
}
223+
info, ok := cs.clusters[cluster.name]
224+
if !ok {
225+
info, ok = cs.plugins[cluster.name]
226+
if !ok {
227+
// This should be unreachable because all route clusters are
228+
// normalized into cs.clusters or cs.plugins during config selector
229+
// creation.
230+
panic(fmt.Sprintf("matched cluster %q not found in ConfigSelector", cluster.name))
231+
}
232+
}
233+
223234
// Add a ref to the selected cluster to keep the interceptors alive until RPC
224235
// is committed.
225236
rc.Increment()
226-
if info, ok := cs.clusters[cluster.name]; ok {
227-
// Add a ref to the selected cluster, as this RPC needs this
228-
// cluster until it is committed.
229-
info.refCount.Add(1)
230-
config.OnCommitted = sync.OnceFunc(func() {
231-
if v := info.refCount.Add(-1); v == 0 {
232-
// We call unsubscribe rather than sendNewServiceConfig to
233-
// prevent redundant updates. If the reference count in the
234-
// dependency manager drops to zero, it will automatically
235-
// trigger a service config update with this cluster
236-
// removed. Calling unsubscribe allows the dependency
237-
// manager to handle the update flow once and for all.
238-
info.unsubscribe()
239-
}
240-
// Decrement the refcount of the route cluster and close the interceptor
241-
// if refcount goes to zero.
242-
rc.Decrement()
243-
})
244-
} else if info, ok := cs.plugins[cluster.name]; ok {
245-
// Add a ref to the selected plugin, as this RPC needs this
246-
// plugin until it is committed.
247-
info.refCount.Add(1)
248-
config.OnCommitted = sync.OnceFunc(func() {
249-
if v := info.refCount.Add(-1); v == 0 {
250-
// This entry will be removed from activePlugins when
251-
// producing a new service config update.
252-
cs.sendNewServiceConfig()
253-
}
254-
// Decrement the refcount of the route cluster and close the interceptor
255-
// if refcount goes to zero.
256-
rc.Decrement()
257-
})
258-
} else {
259-
// This should be unreachable because all route clusters are normalized
260-
// into cs.clusters or cs.plugins during config selector creation.
261-
panic(fmt.Sprintf("matched cluster %q not found in ConfigSelector", cluster.name))
262-
}
237+
// Add a ref to the selected cluster or plugin, as this RPC needs it until it
238+
// is committed. Releasing the last reference unsubscribes from the cluster
239+
// or pushes a new service config for a plugin.
240+
info.Increment()
241+
config.OnCommitted = sync.OnceFunc(func() {
242+
info.Decrement()
243+
// Decrement the refcount of the route cluster and close the interceptor
244+
// if refcount goes to zero.
245+
rc.Decrement()
246+
})
263247

264248
if rt.maxStreamDuration != 0 {
265249
config.MethodConfig.Timeout = &rt.maxStreamDuration
@@ -364,18 +348,14 @@ func (cs *configSelector) stop() {
364348
}
365349
}
366350

367-
// If any reference counts drop to zero, a service config update is required
368-
// to remove the clusters. Since the old config selector is stopped
369-
// after a new one is active, we must trigger a subsequent update to delete
370-
// the now-unused clusters.
351+
// Release this config selector's reference on each cluster and plugin. If
352+
// any reference count drops to zero, the cleanup registered when the entry
353+
// was created removes it from the resolver's active maps and triggers the
354+
// service config update needed to drop it from the channel's config.
371355
for _, ci := range cs.clusters {
372-
if v := ci.refCount.Add(-1); v == 0 {
373-
ci.unsubscribe()
374-
}
356+
ci.Decrement()
375357
}
376358
for _, ci := range cs.plugins {
377-
if v := ci.refCount.Add(-1); v == 0 {
378-
cs.sendNewServiceConfig()
379-
}
359+
ci.Decrement()
380360
}
381361
}

internal/xds/resolver/serviceconfig_test.go

Lines changed: 102 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,17 @@ package resolver
2020

2121
import (
2222
"context"
23+
"errors"
2324
"regexp"
2425
"testing"
2526
"time"
2627

2728
xxhash "github.com/cespare/xxhash/v2"
28-
"github.com/google/go-cmp/cmp"
29+
"google.golang.org/grpc/internal/grpcsync"
2930
"google.golang.org/grpc/internal/grpctest"
3031
"google.golang.org/grpc/internal/grpcutil"
3132
iresolver "google.golang.org/grpc/internal/resolver"
33+
"google.golang.org/grpc/internal/testutils"
3234
_ "google.golang.org/grpc/internal/xds/balancer/cdsbalancer" // To parse LB config
3335
"google.golang.org/grpc/internal/xds/xdsclient/xdsresource"
3436
"google.golang.org/grpc/metadata"
@@ -44,50 +46,113 @@ func Test(t *testing.T) {
4446
grpctest.RunSubTests(t, s{})
4547
}
4648

47-
func (s) TestPruneActiveClusters(t *testing.T) {
48-
newClusterInfo := func(ref int32, unsubscribe func()) *clusterInfo {
49-
ci := &clusterInfo{unsubscribe: unsubscribe}
50-
ci.refCount.Store(ref)
51-
return ci
52-
}
49+
// newResolverForActiveEntryTests returns a resolver with just enough state to
50+
// exercise acquireActiveClusterInfo and the cleanup it registers. The current
51+
// config selector is an erroring one so that pushing a new service config does
52+
// not require an xDS config to be present.
53+
func newResolverForActiveEntryTests(t *testing.T) *xdsResolver {
54+
t.Helper()
55+
56+
ctx, cancel := context.WithCancel(context.Background())
57+
t.Cleanup(cancel)
5358
r := &xdsResolver{
54-
activeClusters: map[string]*clusterInfo{
55-
"zero": newClusterInfo(0, func() {}),
56-
"one": newClusterInfo(1, func() {}),
57-
"two": newClusterInfo(2, func() {}),
58-
"anotherzero": newClusterInfo(0, func() {}),
59-
},
60-
activePlugins: map[string]*clusterInfo{
61-
"zero": newClusterInfo(0, nil),
62-
"one": newClusterInfo(1, nil),
63-
"two": newClusterInfo(2, nil),
64-
"anotherzero": newClusterInfo(0, nil),
65-
},
59+
cc: &testutils.ResolverClientConn{Logger: t},
60+
activeClusters: make(map[string]*grpcsync.RefCounted[*clusterInfo]),
61+
activePlugins: make(map[string]*grpcsync.RefCounted[*clusterInfo]),
62+
serializer: grpcsync.NewCallbackSerializer(ctx),
63+
serializerCancel: cancel,
64+
curConfigSelector: newErroringConfigSelector(errors.New("test"), ""),
6665
}
67-
wantActiveClusters := map[string]int32{
68-
"one": 1,
69-
"two": 2,
66+
r.logger = prefixLogger(r)
67+
return r
68+
}
69+
70+
// runOnSerializer runs f in the context of a serializer callback, which is the
71+
// only place the resolver's active cluster and plugin maps may be touched, and
72+
// blocks until it has run. Any callback queued by an earlier call to this
73+
// helper, including a removal queued when a reference count reached zero, is
74+
// guaranteed to have run by the time f is invoked.
75+
func runOnSerializer(ctx context.Context, t *testing.T, r *xdsResolver, f func()) {
76+
t.Helper()
77+
78+
done := make(chan struct{})
79+
r.serializer.TrySchedule(func(context.Context) {
80+
defer close(done)
81+
f()
82+
})
83+
select {
84+
case <-done:
85+
case <-ctx.Done():
86+
t.Fatal("Timeout waiting for serializer callback to run")
7087
}
71-
wantActivePlugins := map[string]int32{
72-
"one": 1,
73-
"two": 2,
88+
}
89+
90+
// TestActivePluginRefCounting verifies that repeated acquisitions of a cluster
91+
// specifier plugin share a single entry, and that the entry is removed from
92+
// activePlugins only once the last reference is released.
93+
func (s) TestActivePluginRefCounting(t *testing.T) {
94+
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
95+
defer cancel()
96+
97+
r := newResolverForActiveEntryTests(t)
98+
const key = "cluster_specifier_plugin:test-plugin"
99+
100+
var first, second *grpcsync.RefCounted[*clusterInfo]
101+
runOnSerializer(ctx, t, r, func() {
102+
first = r.acquireActiveClusterInfo(key, "")
103+
second = r.acquireActiveClusterInfo(key, "")
104+
})
105+
if first != second {
106+
t.Fatalf("acquireActiveClusterInfo(%q) returned a new entry; want the existing one to be reused", key)
74107
}
75-
r.pruneActiveClustersAndPlugins()
76108

77-
getRefCounts := func(m map[string]*clusterInfo) map[string]int32 {
78-
res := make(map[string]int32)
79-
for k, v := range m {
80-
res[k] = v.refCount.Load()
109+
// Two references are outstanding, so releasing one must keep the entry.
110+
runOnSerializer(ctx, t, r, func() { first.Decrement() })
111+
runOnSerializer(ctx, t, r, func() {
112+
if _, ok := r.activePlugins[key]; !ok {
113+
t.Errorf("activePlugins[%q] was removed while a reference is still held", key)
81114
}
82-
return res
83-
}
115+
})
84116

85-
if d := cmp.Diff(getRefCounts(r.activeClusters), wantActiveClusters); d != "" {
86-
t.Fatalf("r.activeClusters refCounts mismatch (-got +want):\n%s", d)
87-
}
88-
if d := cmp.Diff(getRefCounts(r.activePlugins), wantActivePlugins); d != "" {
89-
t.Fatalf("r.activePlugins refCounts mismatch (-got +want):\n%s", d)
117+
// Releasing the last reference must remove the entry.
118+
runOnSerializer(ctx, t, r, func() { second.Decrement() })
119+
runOnSerializer(ctx, t, r, func() {
120+
if _, ok := r.activePlugins[key]; ok {
121+
t.Errorf("activePlugins[%q] still present after the last reference was released", key)
122+
}
123+
})
124+
}
125+
126+
// TestActivePluginNotRevivedAfterRelease verifies that an entry whose reference
127+
// count has already dropped to zero is replaced by a fresh entry rather than
128+
// resurrected, and that the pending removal of the dead entry does not delete
129+
// its replacement.
130+
func (s) TestActivePluginNotRevivedAfterRelease(t *testing.T) {
131+
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
132+
defer cancel()
133+
134+
r := newResolverForActiveEntryTests(t)
135+
const key = "cluster_specifier_plugin:test-plugin"
136+
137+
var dead, live *grpcsync.RefCounted[*clusterInfo]
138+
runOnSerializer(ctx, t, r, func() {
139+
// Release the only reference. The entry is now dead, but its removal is
140+
// queued behind this callback and so has not run yet. Acquiring again
141+
// must therefore hand back a fresh entry rather than revive this one.
142+
dead = r.acquireActiveClusterInfo(key, "")
143+
dead.Decrement()
144+
live = r.acquireActiveClusterInfo(key, "")
145+
})
146+
if live == dead {
147+
t.Fatal("acquireActiveClusterInfo() returned an entry whose refcount had already reached zero; want a new entry")
90148
}
149+
150+
// The dead entry's queued removal must not evict the replacement.
151+
runOnSerializer(ctx, t, r, func() {
152+
if got := r.activePlugins[key]; got != live {
153+
t.Errorf("activePlugins[%q] = %p, want the newly created entry %p", key, got, live)
154+
}
155+
})
91156
}
92157

93158
func (s) TestGenerateRequestHash(t *testing.T) {

0 commit comments

Comments
 (0)