roachtest/mixedversion: `WaitForReplication` must ensure SQL is ready #138109

srosenberg · 2024-12-31T02:10:40Z

Previously, mixedversion roachtests would occassionally fail while trying to execute SQL against a freshly started node; specifically, get-user-session would time out. The precise root cause is described in [1].

As of [2], WaitForReplication is the default used by all mixedversion roachtests. Thus, it suffices
to ensure that "SQL is ready" before
WaitForReplication is invoked.

However, a number of roachtests don't opt into
StartOpts.WaitForReplication, which makes them
susceptible to the same failure mode, e.g., [3].
Thus, we unconditionally WaitForSQLReady, for
the system tenant, on every started DB node.

[1] #137988
[2] #136607
[3] #137382

Fixes: #137988
Informs: #137332

Epic: none
Release note: None

cockroach-teamcity · 2024-12-31T02:10:50Z

This change is

srosenberg · 2024-12-31T18:24:24Z

10 runs using the SEED (COCKROACH_RANDOM_SEED=-7845636561506696193) from [1],

artifacts-137332/tpcc/mixed-headroom/n5cpu16/run_1/test.log:2024/12/31 10:02:18 test_runner.go:1205: --- PASS: tpcc/mixed-headroom/n5cpu16#1 (17286.21s)
artifacts-137332/tpcc/mixed-headroom/n5cpu16/run_3/test.log:2024/12/31 10:01:44 test_runner.go:1205: --- PASS: tpcc/mixed-headroom/n5cpu16#3 (17265.54s)
artifacts-137332/tpcc/mixed-headroom/n5cpu16/run_4/test.log:2024/12/31 10:01:57 test_runner.go:1205: --- PASS: tpcc/mixed-headroom/n5cpu16#4 (17272.15s)
artifacts-137332/tpcc/mixed-headroom/n5cpu16/run_5/test.log:2024/12/31 11:00:15 test_runner.go:1205: --- PASS: tpcc/mixed-headroom/n5cpu16#5 (17533.08s)
artifacts-137332/tpcc/mixed-headroom/n5cpu16/run_6/test.log:2024/12/31 14:50:13 test_runner.go:1205: --- PASS: tpcc/mixed-headroom/n5cpu16#6 (17271.72s)
artifacts-137332/tpcc/mixed-headroom/n5cpu16/run_7/test.log:2024/12/31 14:50:56 test_runner.go:1205: --- PASS: tpcc/mixed-headroom/n5cpu16#7 (17303.44s)
artifacts-137332/tpcc/mixed-headroom/n5cpu16/run_8/test.log:2024/12/31 14:51:06 test_runner.go:1205: --- PASS: tpcc/mixed-headroom/n5cpu16#8 (17291.17s)
artifacts-137332/tpcc/mixed-headroom/n5cpu16/run_9/test.log:2024/12/31 15:48:04 test_runner.go:1205: --- PASS: tpcc/mixed-headroom/n5cpu16#9 (17231.92s)

[1] #137332 (comment)

srosenberg · 2024-12-31T23:05:03Z

pkg/cmd/roachtest/roachtestutil/utils.go

 // WaitForReady waits until the given nodes report ready via health checks.
 // This implies that the node has completed server startup, is heartbeating its
 // liveness record, and can serve SQL clients.
+// FIXME(srosenberg): This function is a bit of a misnomer. It doesn't actually ensure that SQL is ready to serve, only


@andrewbaptist Just a heads-up since I believe this helper is used only by the tests you authored. While the Admin endpoint may be up, it doesn't guarantee that SQL is ready. The current implementation may be "ok" for these tests, but it's more pronounced in shared-process, multi-tenant configurations.

Less of an issue now after the last iteration, which unconditionally calls WaitForSQLReady, for every started db node.

srosenberg · 2024-12-31T23:07:37Z

SELECT_PROBABILITY=0.6

GCE
AWS

srosenberg · 2025-01-01T23:01:26Z

SELECT_PROBABILITY=0.6

GCE

AWS

decommission/drains failed (because sql on n4 wasn't ready), which made me reconsider the initial approach of tying WaitForSQLReady to WaitForReplication. (Even in that case, it only does it for n1.) Now, each started db node has to pass WaitForSQLReady, regardless of whether WaitForReplication is enabled. In light of that, let's try take two,

SELECT_PROBABILITY=0.8

GCE
AWS

Previously, mixedversion roachtests would occassionally fail while trying to execute SQL against a freshly started node; specifically, `get-user-session` would time out. The precise root cause is described in [1]. As of [2], `WaitForReplication` is the default used by all mixedversion roachtests. Thus, it suffices to ensure that "SQL is ready" before `WaitForReplication` is invoked. However, a number of roachtests don't opt into `StartOpts.WaitForReplication`, which makes them susceptible to the same failure mode, e.g., [3]. Thus, we unconditionally `WaitForSQLReady`, for the system tenant, on every started DB node. [1] cockroachdb#137988 [2] cockroachdb#136607 [3] cockroachdb#137382 Fixes: cockroachdb#137988 Informs: cockroachdb#137332 Epic: none Release note: None

DarrylWong

LGTM!

DarrylWong · 2025-01-02T15:33:19Z

pkg/cmd/roachtest/cluster.go

+	if !startOpts.RoachprodOpts.SkipInit {
+		// Wait for SQL to be ready on all nodes, for 'system' tenant, only.
+		for _, n := range nodes {
+			conn, err := c.ConnE(ctx, l, nodes[0], option.VirtualClusterName("system"))


nit: install.SystemInterfaceName instead of "system"

Thanks, I'll update.

DarrylWong · 2025-01-02T15:33:32Z

pkg/cmd/roachtest/cluster.go

+		// N.B. We must explicitly pass the virtual cluster name to `ConnE`, otherwise the default may turn out to be a
+		// secondary tenant, in which case we would only check the tenant's key range, not the whole system's.
+		// See "Unhidden Bug" in https://github.com/cockroachdb/cockroach/issues/137988
+		conn, err := c.ConnE(ctx, l, nodes[0], option.VirtualClusterName("system"))


nit: same as above

DarrylWong · 2025-01-02T16:04:17Z

pkg/cmd/roachtest/cluster.go

+	nodes := selectedNodesOrDefault(opts, c.CRDBNodes())
+	// N.B. If `SkipInit` is set, we don't wait for SQL since node(s) may not join the cluster in any definite time.
+	if !startOpts.RoachprodOpts.SkipInit {
+		// Wait for SQL to be ready on all nodes, for 'system' tenant, only.


Should something similar be added for the virtual cluster start counterpart: StartServiceForVirtualClusterE?

Ah, I guess it's not needed because we're mainly waiting for the privileges cache to be ready which is at the system level?

Technically, yes, we do need to wait for shared-process tenants. However, there could be any number of them, e.g.,

I241212 11:19:11.321291 53045 sql/syntheticprivilegecache/cache.go:198 â‹® [T3,Vmixed-version-tenant-mbqzz,n3] 145 warmed privileges for virtual tables in 10.007996435s

I thought it's probably best to tackle them in the mixedversion framework, since the above code already feels heavy wrt additional waiting. What do you think?

However, there could be any number of them, e.g.

But don't you just need to wait for the tenant you are starting? Which it should already know from startOpts.RoachprodOpts.VirtualClusterName?

since the above code already feels heavy wrt additional waiting

Yeah I was thinking the same thing, especially since the first SQL check is redundant if WaitForReplicationFactor > 0 since WaitForReplication is also checking if SQL is ready. No arguments from me against doing it in the mixed version framework.

srosenberg force-pushed the sr/137988 branch from 0ee60c8 to fdbbbb0 Compare December 31, 2024 23:00

srosenberg marked this pull request as ready for review December 31, 2024 23:00

srosenberg requested a review from a team as a code owner December 31, 2024 23:00

srosenberg requested review from nameisbhaskar, vidit-bhat and DarrylWong and removed request for a team December 31, 2024 23:00

srosenberg commented Dec 31, 2024

View reviewed changes

srosenberg force-pushed the sr/137988 branch 4 times, most recently from 0d880fb to d89b8e1 Compare January 1, 2025 22:55

srosenberg force-pushed the sr/137988 branch from d89b8e1 to 037a6bb Compare January 1, 2025 23:58

DarrylWong approved these changes Jan 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachtest/mixedversion: `WaitForReplication` must ensure SQL is ready #138109

roachtest/mixedversion: `WaitForReplication` must ensure SQL is ready #138109

srosenberg commented Dec 31, 2024 •

edited

Loading

cockroach-teamcity commented Dec 31, 2024

srosenberg commented Dec 31, 2024

srosenberg Dec 31, 2024

srosenberg Jan 1, 2025

srosenberg commented Dec 31, 2024

srosenberg commented Jan 1, 2025 •

edited

Loading

DarrylWong left a comment

DarrylWong Jan 2, 2025

srosenberg Jan 2, 2025

DarrylWong Jan 2, 2025

DarrylWong Jan 2, 2025

DarrylWong Jan 2, 2025 •

edited

Loading

srosenberg Jan 2, 2025

DarrylWong Jan 2, 2025 •

edited

Loading

roachtest/mixedversion: WaitForReplication must ensure SQL is ready #138109

Are you sure you want to change the base?

roachtest/mixedversion: WaitForReplication must ensure SQL is ready #138109

Conversation

srosenberg commented Dec 31, 2024 • edited Loading

cockroach-teamcity commented Dec 31, 2024

srosenberg commented Dec 31, 2024

srosenberg Dec 31, 2024

Choose a reason for hiding this comment

srosenberg Jan 1, 2025

Choose a reason for hiding this comment

srosenberg commented Dec 31, 2024

srosenberg commented Jan 1, 2025 • edited Loading

DarrylWong left a comment

Choose a reason for hiding this comment

DarrylWong Jan 2, 2025

Choose a reason for hiding this comment

srosenberg Jan 2, 2025

Choose a reason for hiding this comment

DarrylWong Jan 2, 2025

Choose a reason for hiding this comment

DarrylWong Jan 2, 2025

Choose a reason for hiding this comment

DarrylWong Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

srosenberg Jan 2, 2025

Choose a reason for hiding this comment

DarrylWong Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

roachtest/mixedversion: `WaitForReplication` must ensure SQL is ready #138109

roachtest/mixedversion: `WaitForReplication` must ensure SQL is ready #138109

srosenberg commented Dec 31, 2024 •

edited

Loading

srosenberg commented Jan 1, 2025 •

edited

Loading

DarrylWong Jan 2, 2025 •

edited

Loading

DarrylWong Jan 2, 2025 •

edited

Loading