From 4550fcfd3ca86e97d7504182ab3642c4333797d8 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Mon, 5 Dec 2022 16:55:33 +0000 Subject: [PATCH 1/3] roachtest: add `failover/system-non-liveness` This patch adds a roachtest measuring the pMax latency impact on user ranges when system range leaseholders fail (excluding the liveness range, which is tested individually in `failover/liveness`). Ideally, this should not affect user traffic at all, and the results confirm this. Epic: none Release note: None --- pkg/cmd/roachtest/tests/failover.go | 182 +++++++++++++++++++++++++++- 1 file changed, 181 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/roachtest/tests/failover.go b/pkg/cmd/roachtest/tests/failover.go index af145ebaabfc..f4cce4ef8c0c 100644 --- a/pkg/cmd/roachtest/tests/failover.go +++ b/pkg/cmd/roachtest/tests/failover.go @@ -45,6 +45,24 @@ func registerFailover(r registry.Registry) { }, }) } + + for _, failureMode := range []failureMode{ + &failureModeBlackhole{}, + &failureModeBlackholeRecv{}, + &failureModeBlackholeSend{}, + &failureModeCrash{}, + } { + failureMode := failureMode // pin loop variable + r.Add(registry.TestSpec{ + Name: fmt.Sprintf("failover/system-non-liveness/%s", failureMode), + Owner: registry.OwnerKV, + Timeout: 20 * time.Minute, + Cluster: r.MakeClusterSpec(7, spec.CPU(4)), + Run: func(ctx context.Context, t test.Test, c cluster.Cluster) { + runFailoverSystemNonLiveness(ctx, t, c, failureMode) + }, + }) + } } // runFailoverNonSystem benchmarks the maximum duration of range unavailability @@ -198,6 +216,168 @@ func runFailoverNonSystem( m.Wait() } +// runFailoverSystemNonLiveness benchmarks the maximum duration of range +// unavailability following a leaseholder failure with only system ranges, +// excluding the liveness range which is tested separately in +// runFailoverLiveness. +// +// - No user or liveness ranges located on the failed node. +// +// - SQL clients do not connect to the failed node. +// +// - The workload consists of individual point reads and writes. +// +// Since the lease unavailability is probabilistic, depending e.g. on the time +// since the last heartbeat and other variables, we run 9 failures and record +// the pMax latency to find the upper bound on unavailability. Ideally, losing +// the lease on these ranges should have no impact on the user traffic. +// +// The cluster layout is as follows: +// +// n1-n3: Workload ranges, liveness range, and SQL gateways. +// n4-n6: System ranges excluding liveness. +// n7: Workload runner. +// +// The test runs a kv50 workload with batch size 1, using 256 concurrent workers +// directed at n1-n3 with a rate of 2048 reqs/s. n4-n6 fail and recover in +// order, with 30 seconds between each operation, for 3 cycles totaling 9 +// failures. +func runFailoverSystemNonLiveness( + ctx context.Context, t test.Test, c cluster.Cluster, failureMode failureMode, +) { + require.Equal(t, 7, c.Spec().NodeCount) + require.False(t, c.IsLocal(), "test can't use local cluster") // messes with iptables + + rng, _ := randutil.NewTestRand() + + // Create cluster. + opts := option.DefaultStartOpts() + settings := install.MakeClusterSettings() + c.Put(ctx, t.Cockroach(), "./cockroach") + c.Start(ctx, t.L(), opts, settings, c.Range(1, 6)) + + if f, ok := failureMode.(*failureModeCrash); ok { + f.startOpts = opts + f.startSettings = settings + } + + conn := c.Conn(ctx, t.L(), 1) + defer conn.Close() + + // Configure cluster. This test controls the ranges manually. + t.Status("configuring cluster") + _, err := conn.ExecContext(ctx, `SET CLUSTER SETTING kv.range_split.by_load_enabled = 'false'`) + require.NoError(t, err) + + // Constrain all existing zone configs to n4-n6, except liveness which is + // constrained to n1-n3. + rows, err := conn.QueryContext(ctx, `SELECT target FROM [SHOW ALL ZONE CONFIGURATIONS]`) + require.NoError(t, err) + for rows.Next() { + var target string + require.NoError(t, rows.Scan(&target)) + _, err = conn.ExecContext(ctx, fmt.Sprintf( + `ALTER %s CONFIGURE ZONE USING num_replicas = 3, constraints = '[-node1, -node2, -node3]'`, + target)) + require.NoError(t, err) + } + require.NoError(t, rows.Err()) + + _, err = conn.ExecContext(ctx, `ALTER RANGE liveness CONFIGURE ZONE USING `+ + `num_replicas = 3, constraints = '[-node4, -node5, -node6]'`) + require.NoError(t, err) + + // Wait for upreplication. + require.NoError(t, WaitFor3XReplication(ctx, t, conn)) + + // Create the kv database, constrained to n1-n3. Despite the zone config, the + // ranges will initially be distributed across all cluster nodes. + t.Status("creating workload database") + _, err = conn.ExecContext(ctx, `CREATE DATABASE kv`) + require.NoError(t, err) + _, err = conn.ExecContext(ctx, `ALTER DATABASE kv CONFIGURE ZONE USING `+ + `num_replicas = 3, constraints = '[-node4, -node5, -node6]'`) + require.NoError(t, err) + c.Run(ctx, c.Node(7), `./cockroach workload init kv --splits 1000 {pgurl:1}`) + + // The replicate queue takes forever to move the kv ranges from n4-n6 to + // n1-n3, so we do it ourselves. Precreating the database/range and moving it + // to the correct nodes first is not sufficient, since workload will spread + // the ranges across all nodes regardless. + relocateRanges(t, ctx, conn, `database_name = 'kv' OR range_id = 2`, + []int{4, 5, 6}, []int{1, 2, 3}) + relocateRanges(t, ctx, conn, `database_name != 'kv' AND range_id != 2`, + []int{1, 2, 3}, []int{4, 5, 6}) + + // Start workload on n7, using n1-n3 as gateways. Run it for 10 minutes, since + // we take ~1 minute to fail and recover each node, and we do 3 cycles of each + // of the 3 nodes in order. + t.Status("running workload") + m := c.NewMonitor(ctx, c.Range(1, 6)) + m.Go(func(ctx context.Context) error { + c.Run(ctx, c.Node(7), `./cockroach workload run kv --read-percent 50 `+ + `--duration 600s --concurrency 256 --max-rate 2048 --timeout 30s --tolerate-errors `+ + `--histograms=`+t.PerfArtifactsDir()+`/stats.json `+ + `{pgurl:1-3}`) + return nil + }) + + // Start a worker to fail and recover n4-n6 in order. + defer failureMode.Cleanup(ctx, t, c) + + m.Go(func(ctx context.Context) error { + var raftCfg base.RaftConfig + raftCfg.SetDefaults() + + ticker := time.NewTicker(30 * time.Second) + defer ticker.Stop() + + for i := 0; i < 3; i++ { + for _, node := range []int{4, 5, 6} { + select { + case <-ticker.C: + case <-ctx.Done(): + return ctx.Err() + } + + randTimer := time.After(randutil.RandDuration(rng, raftCfg.RangeLeaseRenewalDuration())) + + // Ranges may occasionally escape their constraints. Move them + // to where they should be. + relocateRanges(t, ctx, conn, `database_name != 'kv' AND range_id != 2`, + []int{1, 2, 3}, []int{4, 5, 6}) + relocateRanges(t, ctx, conn, `database_name = 'kv' OR range_id = 2`, + []int{4, 5, 6}, []int{1, 2, 3}) + + // Randomly sleep up to the lease renewal interval, to vary the time + // between the last lease renewal and the failure. We start the timer + // before the range relocation above to run them concurrently. + select { + case <-randTimer: + case <-ctx.Done(): + } + + t.Status(fmt.Sprintf("failing n%d (%s)", node, failureMode)) + if failureMode.ExpectDeath() { + m.ExpectDeath() + } + failureMode.Fail(ctx, t, c, node) + + select { + case <-ticker.C: + case <-ctx.Done(): + return ctx.Err() + } + + t.Status(fmt.Sprintf("recovering n%d (%s)", node, failureMode)) + failureMode.Recover(ctx, t, c, node) + } + } + return nil + }) + m.Wait() +} + // failureMode fails and recovers a given node in some particular way. type failureMode interface { fmt.Stringer @@ -321,7 +501,7 @@ func relocateRanges( require.NotEmpty(t, predicate) var count int for _, source := range from { - where := fmt.Sprintf("%s AND %d = ANY(replicas)", predicate, source) + where := fmt.Sprintf("(%s) AND %d = ANY(replicas)", predicate, source) for { require.NoError(t, conn.QueryRowContext(ctx, `SELECT count(*) FROM crdb_internal.ranges WHERE `+where).Scan(&count)) From cc4728ad03d3cc13dd6fe12d806eff95258b8db1 Mon Sep 17 00:00:00 2001 From: Xin Hao Zhang Date: Mon, 5 Dec 2022 16:11:09 -0500 Subject: [PATCH 2/3] ui: prevent additional RPC fanout in insights api Previously the query used to fetch insights was triggering 2 cluster-wide RPC fanouts by doing a self-join on `crdb_internal.cluster_execution_insights`. We should buffer the insights virtual table to prevent an additional fanout to construct the table again. Epic: none Release note: None --- pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts index a85adc96b7dd..d9f4ece19457 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts @@ -665,6 +665,9 @@ const workloadInsightsQuery: InsightQuery< // Note that we don't filter by problem != 'None', so that we can get all // stmts in the problematic transaction. query: ` +WITH insightsTable as ( + SELECT * FROM crdb_internal.cluster_execution_insights +) SELECT session_id, insights.txn_id as txn_id, @@ -695,9 +698,9 @@ FROM SELECT txn_id, row_number() OVER ( PARTITION BY txn_fingerprint_id ORDER BY end_time DESC ) as rank - FROM crdb_internal.cluster_execution_insights + FROM insightsTable ) as latestTxns -JOIN crdb_internal.cluster_execution_insights AS insights +JOIN insightsTable AS insights ON latestTxns.txn_id = insights.txn_id WHERE latestTxns.rank = 1 AND app_name NOT LIKE '${INTERNAL_APP_NAME_PREFIX}%' `, From 3fd75b438e839c920b799c5b9adcd45b16755a72 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 5 Dec 2022 18:06:06 +0100 Subject: [PATCH 3/3] cli/sql: properly support special COPY input mode When in COPY mode, multi-line input is disabled and the tab key inputs raw ASCII TAB characters. This commit achieves this by redirecting the input to either the interactive editor or the bufio-based editor, depending on whether the current mode is COPY. NB: using ctrl+c while in COPY mode will not work with this patch, but that is caused by a separate issue #93053 and the behavior will be restored when that separate issue is fixed. In the meantime the user can use `\.` or ctrl+d to terminate their input. Release note: None --- pkg/cli/clisqlshell/BUILD.bazel | 1 + pkg/cli/clisqlshell/editor.go | 5 +- pkg/cli/clisqlshell/editor_bimodal.go | 83 +++++++++++++++++++++++++ pkg/cli/interactive_tests/test_copy.tcl | 62 +++++++++++------- 4 files changed, 127 insertions(+), 24 deletions(-) create mode 100644 pkg/cli/clisqlshell/editor_bimodal.go diff --git a/pkg/cli/clisqlshell/BUILD.bazel b/pkg/cli/clisqlshell/BUILD.bazel index 755b02288e21..df8c07716e23 100644 --- a/pkg/cli/clisqlshell/BUILD.bazel +++ b/pkg/cli/clisqlshell/BUILD.bazel @@ -9,6 +9,7 @@ go_library( "context.go", "doc.go", "editor.go", + "editor_bimodal.go", "editor_bubbline.go", "editor_bufio.go", "editor_editline.go", diff --git a/pkg/cli/clisqlshell/editor.go b/pkg/cli/clisqlshell/editor.go index cf1d3c62cc6d..32438398a037 100644 --- a/pkg/cli/clisqlshell/editor.go +++ b/pkg/cli/clisqlshell/editor.go @@ -46,7 +46,10 @@ func getEditor(useEditor bool, displayPrompt bool) editor { if useLibEdit { return &editlineReader{} } - return &bubblineReader{} + return &bimodalEditor{ + main: &bubblineReader{}, + copy: &bufioReader{displayPrompt: displayPrompt}, + } } var useLibEdit = envutil.EnvOrDefaultBool("COCKROACH_SQL_FORCE_LIBEDIT", false) diff --git a/pkg/cli/clisqlshell/editor_bimodal.go b/pkg/cli/clisqlshell/editor_bimodal.go new file mode 100644 index 000000000000..e408656289b4 --- /dev/null +++ b/pkg/cli/clisqlshell/editor_bimodal.go @@ -0,0 +1,83 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package clisqlshell + +import ( + "os" + + "github.com/cockroachdb/errors" +) + +// bimodalEditor redirects the input to either a "main" editor +// (outside of COPY mode) or a "copy" editor (inside COPY). +// This is because the main editor may be multi-line and +// thus inadequate for input of COPY data. +type bimodalEditor struct { + sql sqlShell + main editor + copy editor +} + +var _ editor = (*bimodalEditor)(nil) + +func (e *bimodalEditor) init( + win, wout, werr *os.File, sqlS sqlShell, maxHistEntries int, histFile string, +) (cleanupFn func(), err error) { + e.sql = sqlS + c1, err := e.main.init(win, wout, werr, sqlS, maxHistEntries, histFile) + if err != nil { + return c1, err + } + c2, err := e.copy.init(win, wout, werr, sqlS, maxHistEntries, histFile) + cleanupFn = func() { + c1() + c2() + } + return cleanupFn, err +} + +func (e *bimodalEditor) selected() editor { + if e.sql.inCopy() { + return e.copy + } + return e.main +} + +func (e *bimodalEditor) errInterrupted() error { + return e.selected().errInterrupted() +} + +func (e *bimodalEditor) getOutputStream() *os.File { + return e.selected().getOutputStream() +} + +func (e *bimodalEditor) getLine() (string, error) { + return e.selected().getLine() +} + +func (e *bimodalEditor) addHistory(line string) error { + return errors.CombineErrors( + e.copy.addHistory(line), + e.main.addHistory(line)) +} + +func (e *bimodalEditor) canPrompt() bool { + return e.main.canPrompt() +} + +func (e *bimodalEditor) setPrompt(prompt string) { + e.copy.setPrompt(prompt) + e.main.setPrompt(prompt) +} + +func (e *bimodalEditor) multilineEdit() bool { + return e.selected().multilineEdit() +} diff --git a/pkg/cli/interactive_tests/test_copy.tcl b/pkg/cli/interactive_tests/test_copy.tcl index e0eed2635a47..a9a504a6497e 100644 --- a/pkg/cli/interactive_tests/test_copy.tcl +++ b/pkg/cli/interactive_tests/test_copy.tcl @@ -4,11 +4,15 @@ source [file join [file dirname $argv0] common.tcl] start_server $argv -spawn $argv sql --no-line-editor +spawn $argv sql eexpect root@ -send "DROP TABLE IF EXISTS t;\r" -send "CREATE TABLE t (id INT PRIMARY KEY, t TEXT);\r" +send "drop table if exists t;\r" +eexpect "DROP TABLE" +eexpect root@ +send "create table t (id INT PRIMARY KEY, t TEXT);\r" +eexpect "CREATE TABLE" +eexpect root@ start_test "Check that errors are reported as appropriate." @@ -98,26 +102,30 @@ eexpect eof spawn $argv sql eexpect root@ -start_test "check CTRL+C during COPY exits the COPY mode as appropriate" - -send "COPY t FROM STDIN CSV;\r" -eexpect ">>" -send "5,cancel me\r" - -interrupt - -eexpect "ERROR: COPY canceled by user" -eexpect root@ - -send "SELECT * FROM t ORDER BY id ASC;\r" -eexpect "(6 rows)" +## The following test can be re-enabled after fixing this issue: +# https://github.com/cockroachdb/cockroach/issues/93053 + +# start_test "check CTRL+C during COPY exits the COPY mode as appropriate" +# +# send "COPY t FROM STDIN CSV;\r" +# eexpect ">>" +# send "5,cancel me\r" +# +# interrupt +# +# eexpect "ERROR: COPY canceled by user" +# eexpect root@ +# +# send "SELECT * FROM t ORDER BY id ASC;\r" +# eexpect "(6 rows)" +# eexpect root@ +# +# end_test + +send "truncate table t;\r" +eexpect "TRUNCATE" eexpect root@ -send "TRUNCATE TABLE t;\r" -eexpect root@ - -end_test - send_eof eexpect eof @@ -128,7 +136,10 @@ eexpect ":/# " start_test "Test file input invalid" -send "cat >/tmp/test_copy.sql </tmp/test_copy.sql </tmp/test_copy.sql </tmp/test_copy.sql <> /tmp/test_copy.sql\r" eexpect ":/# "