Fix race in `replicationLagModule` of `go/vt/throttle` #16078

timvaillancourt · 2024-06-06T22:45:23Z

Description

This PR addresses a race discovered in replicationLagModule of go/vt/throttle. The TL;DR is there are concurrent update/delete()s and concurrent reads/iterations of a map without locking

This race is causing vttablets with --enable-tx-throttler to crash after 40-100 minutes of running Vitess v15 w/txthrottler backports from v16-v19:

fatal error: concurrent map iteration and map write
goroutine 455 [running]:
vitess.io/vitess/go/vt/throttler.(*Throttler).MaxLag(0xc000bb4728?, 0xb94630?)
        vitess.io/vitess/go/vt/throttler/throttler.go:236 +0x9c
vitess.io/vitess/go/vt/vttablet/tabletserver/txthrottler.(*txThrottlerStateImpl).updateMaxLag(0xc0009f85a0)
        vitess.io/vitess/go/vt/vttablet/tabletserver/txthrottler/tx_throttler.go:399 +0x1b4
created by vitess.io/vitess/go/vt/vttablet/tabletserver/txthrottler.newTxThrottlerState in goroutine 99
        vitess.io/vitess/go/vt/vttablet/tabletserver/txthrottler/tx_throttler.go:321 +0x4f6

This output includes the TestThrottlerMaxLag test I added in my first commit to reproduce the race: b83f057

Related Issue(s)

Resolves #16102

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

vitess-bot · 2024-06-06T22:45:26Z

codecov · 2024-06-11T00:02:09Z

Codecov Report

Attention: Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.

Project coverage is 68.24%. Comparing base (2531cd0) to head (f53e089).
Report is 1 commits behind head on main.

Files	Patch %	Lines
go/vt/throttler/replication_lag_cache.go	91.30%	2 Missing ⚠️
go/vt/throttler/throttler.go	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16078      +/-   ##
==========================================
+ Coverage   68.22%   68.24%   +0.02%     
==========================================
  Files        1543     1543              
  Lines      197586   197611      +25     
==========================================
+ Hits       134805   134864      +59     
+ Misses      62781    62747      -34

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

timvaillancourt · 2024-06-11T00:52:18Z

go/vt/throttler/replication_lag_cache.go

@@ -76,9 +82,35 @@ func (c *replicationLagCache) add(r replicationLagRecord) {
 	entry.add(r)
 }

+// maxLag returns the maximum replication lag for the entries in cache.
+func (c *replicationLagCache) maxLag() (maxLag uint32) {


Moved this mostly-unchanged from throttler.go (.MaxLag(...)) instead of holding the mu sync.Mutex lock from a different struct

.MaxLag() now calls this func

timvaillancourt · 2024-06-11T00:53:39Z

go/vt/throttler/throttler.go

 	}
-
-	return maxLag
+	return cache.maxLag()


Moved into replicationLagCache

timvaillancourt · 2024-06-11T09:13:41Z

I think I figured out why we only see this on v15 + v16-v19 txthrottler backports:

This txthrottler regression is increasing the rate of healthcheck streams, thus .add() calls to replicationLagCache

Signed-off-by: Tim Vaillancourt <[email protected]>

timvaillancourt · 2024-06-11T09:35:40Z

Squashed commits to make this cherry-pick-able

mattlord · 2024-06-11T15:41:05Z

Squashed commits to make this cherry-pick-able

FWIW, we do squash and merge by default so you can always cherry-pick the squashed commit that is made to the upstream branch.

timvaillancourt · 2024-06-11T18:20:57Z

Squashed commits to make this cherry-pick-able

FWIW, we do squash and merge by default so you can always cherry-pick the squashed commit that is made to the upstream branch.

Right 👍. Here my goal is to cherry-pick this to slackhq pre-review/merge, so a squash makes that just 1 commit

mattlord · 2024-06-11T17:36:46Z

go/vt/throttler/replication_lag_cache.go

@@ -30,6 +31,8 @@ type replicationLagCache struct {
 	// The map key is replicationLagRecord.LegacyTabletStats.Key.
 	entries map[string]*replicationLagHistory

+	mu sync.Mutex


I wonder how important it is that these two stay in perfect sync?

slowReplicas map[string]bool ignoredSlowReplicasInARow map[string]bool

I'm wondering if we couldn't make these atomics instead:

slowReplicas atomic.Pointer[map[string]bool] ignoredSlowReplicasInARow atomic.Pointer[map[string]bool]

Do we have any idea about perf impact with the mutex? I would think that could potentially become pretty hot, but maybe not. If all we care about is eliminating the race conditions, however, then using atomics instead is an option too.

And for the iteration, we don't need to iterate over the map. We can instead iterate over a slice of the keys. That slice would be our own (a slice is really metadata) with a backing array that is using the same immutable strings that are used for the map's keys:

// MaxLag returns the max of all the last replication lag values seen across all tablets of // the provided type, excluding ignored tablets. func (t *Throttler) MaxLag(tabletType topodata.TabletType) uint32 { cache := t.maxReplicationLagModule.lagCacheByType(tabletType) var maxLag uint32 for _, key := range maps.Keys(cache.entries) {

I played around with it for a bit: https://gist.github.com/mattlord/54fcc8de7b98e23292d69258e29d26f7

The new unit test passes each time but I'm not 100% sure it's fully working as it should:

❯ go test -race -count 1 -timeout 30s -run ^TestThrottlerMaxLag$ vitess.io/vitess/go/vt/throttler ok vitess.io/vitess/go/vt/throttler 2.627s

The mutex may in fact be the way to go — certainly if the perf is adequate for you (I think Slack is the biggest user of the txthrottler today). We could probably use a RWMutex instead but that may not make a meaningful difference here either (I'm not even sure how hot this mutex may get).

Please let me know how your testing goes! If it goes well for you then that's certainly a good signal. I can come back to this quickly at any time. Thanks!

@mattlord this would work just as well with atomic.Pointers, yes, the values don't need to be perfectly in sync

I'll make this change because it at least partitions whatever locking atomic does behind the scenes 👍

Do we have any idea about perf impact with the mutex?

@mattlord we don't have these numbers. I can write a benchmark to measure the new/old code but I don't think it's possible to keep the racing code, so I've deferred this for now

mattlord · 2024-06-12T01:16:36Z

go/vt/throttler/throttler_test.go

+					throttler.MaxLag(tabletType)
+				}
+			}
+		}(&wg, ctx, throttler, tabletType)


FWIW, you don't have to pass these in as function parameters. The groutine has access to the variables from the calling goroutine's stack. Although if you want to support older go versions than is used on main (which is on 1.22.4), specifically 1.21 and older, then you can/should pass in the loop variable tabletType.

github-actions bot added this to the v21.0.0 milestone Jun 6, 2024

timvaillancourt force-pushed the throttle-lag-cache-racefix branch from e0ced54 to 1455402 Compare June 10, 2024 23:38

timvaillancourt marked this pull request as ready for review June 11, 2024 00:44

timvaillancourt requested a review from deepthi as a code owner June 11, 2024 00:44

timvaillancourt removed NeedsIssue A linked issue is missing for this Pull Request Type: Regression labels Jun 11, 2024

timvaillancourt commented Jun 11, 2024

View reviewed changes

go/vt/throttler/throttler.go

}

return maxLag

return cache.maxLag()

Copy link

Contributor Author

timvaillancourt Jun 11, 2024

Choose a reason for hiding this comment

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

Moved into replicationLagCache

timvaillancourt added the Component: Throttler label Jun 11, 2024

Fix race in replicationLagModule of go/vt/throttle

f53e089

Signed-off-by: Tim Vaillancourt <[email protected]>

timvaillancourt force-pushed the throttle-lag-cache-racefix branch from d3db9fd to f53e089 Compare June 11, 2024 09:34

mattlord self-requested a review June 11, 2024 15:41

mattlord reviewed Jun 11, 2024

View reviewed changes

mattlord reviewed Jun 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race in `replicationLagModule` of `go/vt/throttle` #16078

Fix race in `replicationLagModule` of `go/vt/throttle` #16078

timvaillancourt commented Jun 6, 2024 •

edited

Loading

vitess-bot bot commented Jun 6, 2024

codecov bot commented Jun 11, 2024 •

edited

Loading

timvaillancourt Jun 11, 2024 •

edited

Loading

timvaillancourt Jun 11, 2024

timvaillancourt commented Jun 11, 2024 •

edited

Loading

timvaillancourt commented Jun 11, 2024

mattlord commented Jun 11, 2024 •

edited

Loading

timvaillancourt commented Jun 11, 2024

mattlord Jun 11, 2024 •

edited

Loading

mattlord Jun 11, 2024 •

edited

Loading

mattlord Jun 12, 2024 •

edited

Loading

timvaillancourt Jun 12, 2024

timvaillancourt Jun 12, 2024 •

edited

Loading

mattlord Jun 12, 2024 •

edited

Loading

Fix race in replicationLagModule of go/vt/throttle #16078

Are you sure you want to change the base?

Fix race in replicationLagModule of go/vt/throttle #16078

Conversation

timvaillancourt commented Jun 6, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jun 6, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Jun 11, 2024 • edited Loading

Codecov Report

timvaillancourt Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

timvaillancourt Jun 11, 2024

Choose a reason for hiding this comment

timvaillancourt commented Jun 11, 2024 • edited Loading

timvaillancourt commented Jun 11, 2024

mattlord commented Jun 11, 2024 • edited Loading

timvaillancourt commented Jun 11, 2024

mattlord Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

mattlord Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

mattlord Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

timvaillancourt Jun 12, 2024

Choose a reason for hiding this comment

timvaillancourt Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

mattlord Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Fix race in `replicationLagModule` of `go/vt/throttle` #16078

Fix race in `replicationLagModule` of `go/vt/throttle` #16078

timvaillancourt commented Jun 6, 2024 •

edited

Loading

codecov bot commented Jun 11, 2024 •

edited

Loading

timvaillancourt Jun 11, 2024 •

edited

Loading

timvaillancourt commented Jun 11, 2024 •

edited

Loading

mattlord commented Jun 11, 2024 •

edited

Loading

mattlord Jun 11, 2024 •

edited

Loading

mattlord Jun 11, 2024 •

edited

Loading

mattlord Jun 12, 2024 •

edited

Loading

timvaillancourt Jun 12, 2024 •

edited

Loading

mattlord Jun 12, 2024 •

edited

Loading