sql: retry more distributed errors as local #108336

yuzefovich · 2023-08-07T22:29:02Z

This PR contains a couple of commits that increase the allow-list of errors that are retried locally. In particular, it allows us to hide some issues we have around using DistSQL and shutting down SQL pods.

Fixes: #106537.
Fixes: #108152.
Fixes: #108271.

Release note: None

cockroach-teamcity · 2023-08-07T22:29:12Z

This change is

yuzefovich

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson, @knz, and @mgartner)

pkg/sql/distsql_running.go line 107 at r1 (raw file):

}

func isDialErr(err error) bool {

~~I briefly looked into using errors.Is or errors.As here but wasn't sure what's the canonical way that would definitely work. Probably Raphael has a suggestion here.~~ Reimplemented with errors.HasType.

yuzefovich

I added another commit to include more errors in the allow-list for retrying. Done pushing for now, PTAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson, @knz, and @mgartner)

mgartner

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jeffswenson, @knz, and @yuzefovich)

pkg/ccl/serverccl/server_sql_test.go line 441 at r2 (raw file):

	}()

	listener, err := net.Listen("tcp", rpcAddr)

Why is this listener required?

jeffswenson

LGTM

jeffswenson · 2023-08-08T15:18:41Z

pkg/sql/distsql_running.go

@@ -100,6 +101,22 @@ type runnerResult struct {
 err error
 }

+type runnerDialErr struct {


I like the approach of tagging errors that occur during the dial flow. This is a robust way to handle issues that occur during start up, which is the most common case we have seen for Serverless.

yuzefovich

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jeffswenson and @knz)

pkg/ccl/serverccl/server_sql_test.go line 441 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Why is this listener required?

I'm not sure, I just copied this test from Jeff #106538. @jeffswenson can you share some context about this listener?

jeffswenson · 2023-08-08T16:01:26Z

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jeffswenson and @knz)

pkg/ccl/serverccl/server_sql_test.go line 441 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…
I'm not sure, I just copied this test from Jeff #106538. @jeffswenson can you share some context about this listener?

The test is covering the following scenario:

A sql server starts up and is assigned port a
The sql server shuts down and releases port a
Something else starts up and claims port a. In the test that is the listener. This is important because the listener causes connections to a to hang instead of responding with a RESET packet.
A different server with stale instance information schedules a distsql flow and attempts to dial port a.

yuzefovich

Usage of the string matching for errors in IsDistSQLRetryable is not great, but it's not a new problem and we do have #82847 as a tracking issue to improve the situation.

TFTRs!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffswenson, @knz, and @mgartner)

pkg/ccl/serverccl/server_sql_test.go line 441 at r2 (raw file):

Previously, JeffSwenson (Jeff Swenson) wrote…

The test is covering the following scenario:

A sql server starts up and is assigned port a

The sql server shuts down and releases port a

Something else starts up and claims port a. In the test that is the listener. This is important because the listener causes connections to a to hang instead of responding with a RESET packet.

A different server with stale instance information schedules a distsql flow and attempts to dial port a.

Thanks for this context. I incorporated it as a comment on the test.

craig · 2023-08-08T23:12:28Z

Build failed:

Bazel Essential CI (Cockroach)

yuzefovich · 2023-08-08T23:16:28Z

Hm, I'm confused

[23:12:22][Run unit tests] ERROR: /go/src/github.com/cockroachdb/cockroach/pkg/jobs/BUILD.bazel:88:8: GoCompilePkg pkg/jobs/autoconfig.recompile1376.a failed: (Exit 1): builder failed: error executing command (from target //pkg/jobs:jobs_test) bazel-out/k8-opt-exec-2B5CBBC6/bin/external/go_sdk/builder_reset/builder compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -tags bazel,gss,bazel,gss -src pkg/server/autoconfig/auto_config.go ... (remaining 62 arguments skipped)
[23:12:22][Run unit tests] 
[23:12:22][Run unit tests] Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
[23:12:22][Run unit tests] compilepkg: panic: runtime error: invalid memory address or nil pointer dereference
[23:12:22][Run unit tests] [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x5e89bc]

Let's try one more time.

bors r+

craig · 2023-08-08T23:51:35Z

Build failed:

Bazel Essential CI (Cockroach)

yuzefovich · 2023-08-08T23:58:38Z

Known failure #108340.

bors r+

craig · 2023-08-09T01:12:24Z

Build failed:

Bazel Essential CI (Cockroach)

yuzefovich · 2023-08-09T04:07:52Z

The latest failure is at least legitimate:

=== RUN   TestStartTenantWithStaleInstance
    test_log_scope.go:167: test logs captured to: /artifacts/tmp/_tmp/c101b7a464a1afc1f5af0cd85792187e/logTestStartTenantWithStaleInstance3465982568
    test_log_scope.go:81: use -show-logs to present logs inline
    server_sql_test.go:450: 
        	Error Trace:	github.com/cockroachdb/cockroach/pkg/ccl/serverccl/server_sql_test.go:450
        	Error:      	Received unexpected error:
        	            	listen tcp 127.0.0.1:45845: bind: address already in use
        	Test:       	TestStartTenantWithStaleInstance

I'll take a look tomorrow, but I did stress this new test on the gceworker with no failures.

mgartner · 2023-08-09T18:31:38Z

pkg/ccl/serverccl/server_sql_test.go line 441 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Thanks for this context. I incorporated it as a comment on the test.

Thanks for the context. The only part I'm missing is how the "different server" is dialing rpcAddr - I don't see how that's forced.

yuzefovich

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffswenson, @knz, and @mgartner)

pkg/ccl/serverccl/server_sql_test.go line 450 at r7 (raw file):

	listener, err := net.Listen("tcp", rpcAddr)
	require.NoError(t, err)

For the failure that I got under stress on CI on this line:

=== RUN   TestStartTenantWithStaleInstance
    test_log_scope.go:167: test logs captured to: /artifacts/tmp/_tmp/c101b7a464a1afc1f5af0cd85792187e/logTestStartTenantWithStaleInstance3465982568
    test_log_scope.go:81: use -show-logs to present logs inline
    server_sql_test.go:450: 
        	Error Trace:	github.com/cockroachdb/cockroach/pkg/ccl/serverccl/server_sql_test.go:450
        	Error:      	Received unexpected error:
        	            	listen tcp 127.0.0.1:45845: bind: address already in use
        	Test:       	TestStartTenantWithStaleInstance

it seems like the stopped tenant hasn't released the socket, and to me it seems like a benign error with the test setup. I'm inclined to introduce SucceedsSoon until net.Listen doesn't return an error. @jeffswenson WDYT?

This commit marks the error that DistSQL runners produce when dialing remote nodes in a special way that is now always retried-as-local. In particular, this allows us to fix two problematic scenarios that could occur when using secondary tenants: - when attempting to start a pod with stale instance information - the port is in use by an RPC server for the same tenant, but with a new instance id. This commit includes the test from Jeff that exposed the gap in the retry-as-local mechanism. Release note: None

This commit moves the "drain succeeded" logging message to be at the very end of the drain process. Also, it removes now stale comment. Release note: None

This commit includes all errors that contain `rpc error` substring to be retried-as-local. In particular, this allows us to avoid problems with DistSQL using no-longer-live SQL pod after that pod is shutdown. (This usage of the downed pod is currently expected given that the cache of live instances isn't updated when the pod is shutdown.) Release note: None

yuzefovich

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffswenson, @knz, and @mgartner)

pkg/ccl/serverccl/server_sql_test.go line 441 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Thanks for the context. The only part I'm missing is how the "different server" is dialing rpcAddr - I don't see how that's forced.

I think I see this. In step 4. the "different server" is the new being-started-right-now SQL pod (which itself will listen on a different port), and when starting up, it sees that there is another SQL pod for the same tenant in sql_instances system table (see #106537 (comment)). Thus, it attempts to dial that SQL pod to perform the startup migration, which fails because we have listener not responding to that dial attempt.

jeffswenson · 2023-08-10T15:33:21Z

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffswenson, @knz, and @mgartner)

pkg/ccl/serverccl/server_sql_test.go line 450 at r7 (raw file):
	listener, err := net.Listen("tcp", rpcAddr)
	require.NoError(t, err)
For the failure that I got under stress on CI on this line:
=== RUN   TestStartTenantWithStaleInstance
    test_log_scope.go:167: test logs captured to: /artifacts/tmp/_tmp/c101b7a464a1afc1f5af0cd85792187e/logTestStartTenantWithStaleInstance3465982568
    test_log_scope.go:81: use -show-logs to present logs inline
    server_sql_test.go:450: 
        	Error Trace:	github.com/cockroachdb/cockroach/pkg/ccl/serverccl/server_sql_test.go:450
        	Error:      	Received unexpected error:
        	            	listen tcp 127.0.0.1:45845: bind: address already in use
        	Test:       	TestStartTenantWithStaleInstance
it seems like the stopped tenant hasn't released the socket, and to me it seems like a benign error with the test setup. I'm inclined to introduce SucceedsSoon until net.Listen doesn't return an error. @jeffswenson WDYT?

Adding a SucceedsSoon LGTM. Another option is we could start the listener, then inject a synthetic sql_instance row. But I like the current implementation of the test because it is very generic and has minimal coupling to the values in the sql_instance table.

yuzefovich

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffswenson, @knz, and @mgartner)

pkg/ccl/serverccl/server_sql_test.go line 450 at r7 (raw file):

Previously, JeffSwenson (Jeff Swenson) wrote…

Adding a SucceedsSoon LGTM. Another option is we could start the listener, then inject a synthetic sql_instance row. But I like the current implementation of the test because it is very generic and has minimal coupling to the values in the sql_instance table.

Thanks, let's keep SucceedsSoon then.

craig · 2023-08-10T16:25:57Z

Build succeeded:

Bazel Essential CI (Cockroach)

yuzefovich requested review from knz and jeffswenson August 7, 2023 22:29

yuzefovich requested review from a team as code owners August 7, 2023 22:29

yuzefovich requested a review from mgartner August 7, 2023 22:29

yuzefovich commented Aug 7, 2023

View reviewed changes

yuzefovich force-pushed the sql-retryable branch from 8c2c676 to 3c82a67 Compare August 8, 2023 00:06

yuzefovich mentioned this pull request Aug 8, 2023

server: TestServerShutdownReleasesSession failed #108152

Closed

yuzefovich force-pushed the sql-retryable branch 3 times, most recently from b605ba8 to dbaeb23 Compare August 8, 2023 02:06

yuzefovich requested review from a team as code owners August 8, 2023 03:07

yuzefovich requested review from rhu713 and removed request for a team August 8, 2023 03:07

yuzefovich changed the title ~~sql: retry all DistSQL runner dial errors~~ sql: retry more distributed errors as local Aug 8, 2023

yuzefovich removed request for a team and rhu713 August 8, 2023 03:09

yuzefovich commented Aug 8, 2023

View reviewed changes

mgartner approved these changes Aug 8, 2023

View reviewed changes

jeffswenson approved these changes Aug 8, 2023

View reviewed changes

jeffswenson reviewed Aug 8, 2023

View reviewed changes

yuzefovich commented Aug 8, 2023

View reviewed changes

yuzefovich force-pushed the sql-retryable branch from 0028b76 to 50a9308 Compare August 8, 2023 22:53

yuzefovich commented Aug 8, 2023

View reviewed changes

yuzefovich commented Aug 10, 2023

View reviewed changes

yuzefovich force-pushed the sql-retryable branch from 50a9308 to a8d6fd4 Compare August 10, 2023 02:21

yuzefovich added 3 commits August 9, 2023 19:23

server: minor cleanup of logging around draining

847722e

This commit moves the "drain succeeded" logging message to be at the very end of the drain process. Also, it removes now stale comment. Release note: None

yuzefovich force-pushed the sql-retryable branch from a8d6fd4 to 5c09eb1 Compare August 10, 2023 02:23

yuzefovich commented Aug 10, 2023

View reviewed changes

craig bot merged commit 1f8fa96 into cockroachdb:master Aug 10, 2023
2 checks passed

yuzefovich deleted the sql-retryable branch August 10, 2023 17:26

yuzefovich mentioned this pull request Aug 16, 2023

pkg/ccl/kvccl/kvtenantccl/upgradeccl/upgradeccl_test: TestTenantUpgrade failed #108086

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: retry more distributed errors as local #108336

sql: retry more distributed errors as local #108336

yuzefovich commented Aug 7, 2023 •

edited

Loading

cockroach-teamcity commented Aug 7, 2023

yuzefovich left a comment •

edited

Loading

yuzefovich left a comment

mgartner left a comment

jeffswenson left a comment

jeffswenson Aug 8, 2023

yuzefovich left a comment

jeffswenson commented Aug 8, 2023

yuzefovich left a comment

craig bot commented Aug 8, 2023

yuzefovich commented Aug 8, 2023

craig bot commented Aug 8, 2023

yuzefovich commented Aug 8, 2023

craig bot commented Aug 9, 2023

yuzefovich commented Aug 9, 2023

mgartner commented Aug 9, 2023

yuzefovich left a comment

yuzefovich left a comment

jeffswenson commented Aug 10, 2023

yuzefovich left a comment

craig bot commented Aug 10, 2023

sql: retry more distributed errors as local #108336

sql: retry more distributed errors as local #108336

Conversation

yuzefovich commented Aug 7, 2023 • edited Loading

cockroach-teamcity commented Aug 7, 2023

yuzefovich left a comment • edited Loading

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

mgartner left a comment

Choose a reason for hiding this comment

jeffswenson left a comment

Choose a reason for hiding this comment

jeffswenson Aug 8, 2023

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

jeffswenson commented Aug 8, 2023

yuzefovich left a comment

Choose a reason for hiding this comment

craig bot commented Aug 8, 2023

yuzefovich commented Aug 8, 2023

craig bot commented Aug 8, 2023

yuzefovich commented Aug 8, 2023

craig bot commented Aug 9, 2023

yuzefovich commented Aug 9, 2023

mgartner commented Aug 9, 2023

yuzefovich left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

jeffswenson commented Aug 10, 2023

yuzefovich left a comment

Choose a reason for hiding this comment

craig bot commented Aug 10, 2023

yuzefovich commented Aug 7, 2023 •

edited

Loading

yuzefovich left a comment •

edited

Loading