multitenant: add multitenant/shared-process/basic roachtest #95999

msbutler · 2023-01-26T15:08:11Z

This patch introduces a simple roachtest that runs in a shared-process tenant.
This test imports a 500 tpcc workload (about 30 GB of replicated data), and
runs the workload for 10 minutes. The test is run on a 4 node, 4vcpu cluster
with local ssds.

A future patch could complicate the test by running schema changes or other
bulk operations.

Fixes #95990

Release note: None

cockroach-teamcity · 2023-01-26T15:08:23Z

This change is

msbutler · 2023-01-26T15:48:04Z

Test seems to run just fine!

msbutler · 2023-01-26T21:04:24Z

(unrelated flake in CI. opened seperate PR to skip that test)

knz

(Don't forget to update the PR title and description from the new commit message/title.)

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @msbutler, @smg260, and @stevendanna)

pkg/cmd/roachtest/tests/multitenant_shared_process.go line 36 at r1 (raw file):

		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
			var (
				appTenantID    = 2

CREATE TENANT will not guarantee you a fixed appTenantID. If you need the tenant ID, you need to look it up after CREATE TENANT.

(IMHO we should extend update_tenant_resource_limits to accept a tenant name as first argument instead. Feel free to do so; or file an issue to do so and add a TODO around here.)

pkg/cmd/roachtest/tests/multitenant_shared_process.go line 50 at r1 (raw file):

			sysSQL := sqlutils.MakeSQLRunner(sysConn)

			sysSQL.Exec(t, fmt.Sprintf(`CREATE TENANT %s`, appTenantName))

After #95658 merges, you will want to do ALTER TENANT %1 START SERVICE SHARED here.
And after that you will need something like "SucceedsSoon" to wait until the service has started. Otherwise, the workload init below has a small chance to fail if it tries to connect before the tenant service is ready.

pkg/cmd/roachtest/tests/multitenant_shared_process.go line 50 at r1 (raw file):

			sysSQL := sqlutils.MakeSQLRunner(sysConn)

			sysSQL.Exec(t, fmt.Sprintf(`CREATE TENANT %s`, appTenantName))

also,
sysSQL.Exec(t, CREATE TENANT $1, appTenantName). No need for Sprintf.

msbutler

@knz I've added an additional commit that I believe addresses all your feedback. I'll plan to squash it with the second commit in this PR, if you approve. Thanks!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @knz, @smg260, and @stevendanna)

pkg/cmd/roachtest/tests/multitenant_shared_process.go line 36 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

CREATE TENANT will not guarantee you a fixed appTenantID. If you need the tenant ID, you need to look it up after CREATE TENANT.

(IMHO we should extend update_tenant_resource_limits to accept a tenant name as first argument instead. Feel free to do so; or file an issue to do so and add a TODO around here.)

added a todo and opened the following issue #96176

pkg/cmd/roachtest/tests/multitenant_shared_process.go line 50 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

After #95658 merges, you will want to do ALTER TENANT %1 START SERVICE SHARED here.
And after that you will need something like "SucceedsSoon" to wait until the service has started. Otherwise, the workload init below has a small chance to fail if it tries to connect before the tenant service is ready.

done

pkg/cmd/roachtest/tests/multitenant_shared_process.go line 50 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

also,
sysSQL.Exec(t, CREATE TENANT $1, appTenantName). No need for Sprintf.

done

knz

LGTM modulo a few nits and checking the retry behavior.

Reviewed 1 of 5 files at r3, 1 of 4 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @msbutler, @smg260, and @stevendanna)

-- commits line 17 at r5:
commit title: funciton -> function

pkg/cmd/roachtest/tests/multitenant_shared_process.go line 46 at r5 (raw file):

			// In order to observe the app tenant's db console,
			//create a secure cluster and add Admin roles to the system and app tenant.

nit: space after //

pkg/cmd/roachtest/tests/multitenant_utils.go line 350 at r5 (raw file):

	sysSQL.Exec(t, "ALTER TENANT $1 START SERVICE SHARED", tenantName)

	// Ensure the tenant server is ready to accept queires

nit: queries and period after the end.

pkg/cmd/roachtest/tests/multitenant_utils.go line 353 at r5 (raw file):

	tenantConn := c.Conn(ctx, t.L(), nodes.RandNode()[0], option.TenantName(tenantName))
	tenantSQL := sqlutils.MakeSQLRunner(tenantConn)
	tenantSQL.ExecSucceedsSoon(t, `SELECT 1`)

Could you stress this? I'm surprised this works -- I'd have expected that the call to .Conn would be the one to fail.
Maybe you need to use .ConnE instead and retry based on that error.

pkg/sql/sem/builtins/builtins.go line 6433 at r5 (raw file):

	// Used to configure the tenant token bucket. See UpdateTenantResourceLimits.
	//
	// TODO(multitenantTeam): use tenantName instead of tenantID

nit: period at end of sentence.
Also you can include a link to the new issue in the comment.

msbutler

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @smg260, and @stevendanna)

pkg/cmd/roachtest/tests/multitenant_utils.go line 353 at r5 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Could you stress this? I'm surprised this works -- I'd have expected that the call to .Conn would be the one to fail.
Maybe you need to use .ConnE instead and retry based on that error.

huh, I chose to retry the SELECT query instead of .Conn cmd bc I was under the impression that the connection to the in-process tenant is lazily created during the first query, but maybe I misinterpreted @stevendanna? FWIW the c.Conn call will panic if .ConnE throws an error, and I don't think we've seen c.Conn fail in the c2c roachtests that use in-process tenants.

msbutler

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @smg260, and @stevendanna)

pkg/cmd/roachtest/tests/multitenant_utils.go line 353 at r5 (raw file):

Previously, msbutler (Michael Butler) wrote…

huh, I chose to retry the SELECT query instead of .Conn cmd bc I was under the impression that the connection to the in-process tenant is lazily created during the first query, but maybe I misinterpreted @stevendanna? FWIW the c.Conn call will panic if .ConnE throws an error, and I don't think we've seen c.Conn fail in the c2c roachtests that use in-process tenants.

could you explain why you're surprised?

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @msbutler, @smg260, and @stevendanna)

pkg/cmd/roachtest/tests/multitenant_utils.go line 353 at r5 (raw file):

Previously, msbutler (Michael Butler) wrote…

could you explain why you're surprised?

Under the hood .Conn / .ConnE call gosql.Open. It is driver-specific whether gosql.Open directly opens the SQL session, or whether the SQL session will only be opened on first use. The API doesn't specify either way. So you're just being lucky here.
You're then passing that conn object through MakeSQLRunner. There's no API guarantee in MakeSQLRunner either that it doesn't "do" something with the connection before handing you a *SQLRunner object back.

msbutler

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @knz, @smg260, and @stevendanna)

-- commits line 17 at r5:

Previously, knz (Raphael 'kena' Poss) wrote…

commit title: funciton -> function

Done.

pkg/cmd/roachtest/tests/multitenant_shared_process.go line 46 at r5 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: space after //

Done

pkg/cmd/roachtest/tests/multitenant_utils.go line 350 at r5 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: queries and period after the end.

Done

pkg/cmd/roachtest/tests/multitenant_utils.go line 353 at r5 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Under the hood .Conn / .ConnE call gosql.Open. It is driver-specific whether gosql.Open directly opens the SQL session, or whether the SQL session will only be opened on first use. The API doesn't specify either way. So you're just being lucky here.

You're then passing that conn object through MakeSQLRunner. There's no API guarantee in MakeSQLRunner either that it doesn't "do" something with the connection before handing you a *SQLRunner object back.

I see. I've modified my PR in the last commit to hedge against the vague API. Let me know if this strategy makes sense!

pkg/sql/sem/builtins/builtins.go line 6433 at r5 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: period at end of sentence.
Also you can include a link to the new issue in the comment.

Done

knz

Reviewed 2 of 3 files at r6, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @msbutler, @smg260, and @stevendanna)

pkg/cmd/roachtest/tests/multitenant_utils.go line 351 at r8 (raw file):

	// Opening a SQL session to a newly created in-process tenant may require a
	// few retires. Unfortunately, the c.ConnE and MakeSQLRunner APIs do not make

nit: few retries

pkg/cmd/roachtest/tests/multitenant_utils.go line 364 at r8 (raw file):

		return nil
	})
	tenantSQL.ExecSucceedsSoon(t, `SELECT 1`)

You can remove this ExecSucceedsSoon and add a call to tenantConn.Ping() inside the SucceedsSoon above instead.

msbutler

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @knz, @smg260, and @stevendanna)

pkg/cmd/roachtest/tests/multitenant_utils.go line 351 at r8 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: few retries

Done

pkg/cmd/roachtest/tests/multitenant_utils.go line 364 at r8 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

You can remove this ExecSucceedsSoon and add a call to tenantConn.Ping() inside the SucceedsSoon above instead.

Done

This patch introduces a simple roachtest that runs in a shared-process tenant. This test imports a 500 tpcc workload (about 30 GB of replicated data), and runs the workload for 10 minutes. The test is run on a 4 node, 4vcpu cluster with local ssds. A future patch could complicate the test by running schema changes or other bulk operations. Fixes cockroachdb#95990 Release note: None

Currently, a user can only observe the db console for the app tenant in a secure cluster, after logging in with a provided role. This small patch adds the createTenantAdminRole utility function which allows the roachtest writer to create an admin role for the provided tenant, with the user name "secure" and password "roach". Once the roachtest writer has called this function for each tenant in their cluster, they can log into the db console and toggle between tenant db consoles. This patch also adds the createInMemoryTenant() function which makes it easy for a roachtest writer to create an in-memory tenant. Epic: none Release note: None

msbutler · 2023-02-01T19:41:48Z

TFTR!!

bors r=knz

95622: backupccl,storage: add logs around manifest handling and ExportRequest pagination r=stevendanna a=adityamaru backupccl: add logging to backup manifest handling Release note: None storage: log the ExportRequest pagination reason Release note: None Epic: None 95865: cmd/roachtest: adapt disk-stall detection roachtest r=nicktrav,erikgrinaker a=jbowens Move the existing disk-stall/* roachtests under disk-stall/fuse/* (for the FUSE filesystem approach to stalling) and skip them for now. Currently, they're not capable of stalling the disk longer 50us (see #95886), which makes them unreliable at exercising stalls. Add two new roachtests, disk-stall/dmsetup and disk-stall/cgroup that use dmsetup and cgroup bandwidth restrctions respectively to reliably induce a write stall for an indefinite duration. Informs #94373. Epic: None Release note: None 95999: multitenant: add multitenant/shared-process/basic roachtest r=stevendanna a=msbutler This patch introduces a simple roachtest that runs in a shared-process tenant. This test imports a 500 tpcc workload (about 30 GB of replicated data), and runs the workload for 10 minutes. The test is run on a 4 node, 4vcpu cluster with local ssds. A future patch could complicate the test by running schema changes or other bulk operations. Fixes #95990 Release note: None 96115: schemachanger: Implement `DROP CONSTRAINT` in declarative schema changer r=Xiang-Gu a=Xiang-Gu This PR implements `ALTER TABLE t DROP CONSTRAINT cons_name` in declarative schema changer. Supported constraints include Checks, FK, and UniqueWithoutIndex. Dropping PK or Unique constraints will fall back to legacy schema changer, which in turn spits out an "not supported yet" error. Epic: None 96202: opt: inverted-index accelerate filters of the form j->0 @> '{"b": "c"} r=Shivs11 a=Shivs11 Previously, the optimizer did not plan inverted index scans for filters having an integer as the index for the fetch value in a filter alongside the "contains" or the "contained by" operator. To address this, we now build JSON arrays from fetch value expressions with integer indexes. From these JSON arrays, inverted spans are built for constraining scans over inverted indexes. With these changes chains of both integer and string fetch value operators are now supported alongside the "contains" and the "contained by" operators. (e.g., j->0 `@>` '{"b": "c"}' and j->0 <@ '{"b": "c"}'). Epic: [CRDB-3301](https://cockroachlabs.atlassian.net/browse/CRDB-3301) Fixes: #94667 Release note (performance improvement): The optimizer now plans inverted index scans for queries that filter by JSON fetch value operators (->) with integer indices alongside the "contains" or the "contained by" operators, e.g, json_col->0 `@>` '{"b": "c"}' or json_col->0 <@ '{"b": "c"}' 96235: sem/tree: add support for producing vectorized data from strings r=cucaroach a=cucaroach tree.ValueHandler exposes raw machine type hooks that are used by vec_handler to build coldata.Vec's. Epic: CRDB-18892 Informs: #91831 Release note: None 96328: udf: allow strict UDF with no arguments r=DrewKimball a=DrewKimball This patch fixes the case when a strict UDF (returns null on null input) has no arguments. Previously, attempting to call such a function would result in `ERROR: reflect: call of reflect.Value.Pointer on zero Value`. Fixes #96326 Release note: None 96366: release: skip nil GitHub events r=celiala a=rail Previously, we referenced `*event.Event`, but in some cases the event objects are `nil`. This PR skips the nil GitHub event objects. Epic: none Release note: None Co-authored-by: adityamaru <[email protected]> Co-authored-by: Jackson Owens <[email protected]> Co-authored-by: Michael Butler <[email protected]> Co-authored-by: Xiang Gu <[email protected]> Co-authored-by: Shivam Saraf <[email protected]> Co-authored-by: Tommy Reilly <[email protected]> Co-authored-by: Drew Kimball <[email protected]> Co-authored-by: Rail Aliiev <[email protected]>

craig · 2023-02-01T21:00:02Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-02-02T00:31:39Z

Build succeeded:

Bazel Essential CI (Cockroach)

msbutler added the T-multitenant Issues owned by the multi-tenant virtual team label Jan 26, 2023

msbutler requested a review from a team January 26, 2023 15:08

msbutler requested a review from a team as a code owner January 26, 2023 15:08

msbutler requested review from herkolategan and smg260 and removed request for a team January 26, 2023 15:08

msbutler self-assigned this Jan 26, 2023

msbutler requested a review from knz January 26, 2023 15:08

msbutler force-pushed the butler-mt-ua branch 2 times, most recently from 4d871df to 8d03006 Compare January 26, 2023 20:04

adityamaru mentioned this pull request Jan 27, 2023

roachtest: c2c/tpcc/warehouses=500/duration=10/cutover=5 failed #95672

Closed

msbutler changed the title ~~multitenant: add unified/basic roachtest~~ multitenant: add multitenant/shared-process/basic roachtest Jan 27, 2023

msbutler requested a review from stevendanna January 27, 2023 18:37

knz reviewed Jan 28, 2023

View reviewed changes

msbutler force-pushed the butler-mt-ua branch from 8d03006 to 9c87704 Compare January 31, 2023 15:25

msbutler requested a review from a team January 31, 2023 15:25

msbutler commented Jan 31, 2023

View reviewed changes

knz reviewed Jan 31, 2023

View reviewed changes

msbutler commented Jan 31, 2023

View reviewed changes

knz reviewed Jan 31, 2023

View reviewed changes

msbutler force-pushed the butler-mt-ua branch from 9c87704 to c582697 Compare January 31, 2023 23:07

msbutler commented Jan 31, 2023

View reviewed changes

msbutler force-pushed the butler-mt-ua branch from c582697 to 9270e65 Compare January 31, 2023 23:11

knz approved these changes Jan 31, 2023

View reviewed changes

msbutler force-pushed the butler-mt-ua branch from 9270e65 to d579128 Compare February 1, 2023 13:04

msbutler commented Feb 1, 2023

View reviewed changes

msbutler force-pushed the butler-mt-ua branch from d579128 to ad1922a Compare February 1, 2023 14:09

msbutler added 2 commits February 1, 2023 13:04

msbutler force-pushed the butler-mt-ua branch from ad1922a to 33a01d2 Compare February 1, 2023 18:04

craig bot merged commit c77104e into cockroachdb:master Feb 2, 2023

msbutler deleted the butler-mt-ua branch February 22, 2023 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multitenant: add multitenant/shared-process/basic roachtest #95999

multitenant: add multitenant/shared-process/basic roachtest #95999

msbutler commented Jan 26, 2023 •

edited

Loading

cockroach-teamcity commented Jan 26, 2023

msbutler commented Jan 26, 2023

msbutler commented Jan 26, 2023

knz left a comment

msbutler left a comment

knz left a comment

msbutler left a comment

msbutler left a comment

knz left a comment

msbutler left a comment

knz left a comment

msbutler left a comment

msbutler commented Feb 1, 2023 •

edited

Loading

craig bot commented Feb 1, 2023

craig bot commented Feb 2, 2023

multitenant: add multitenant/shared-process/basic roachtest #95999

multitenant: add multitenant/shared-process/basic roachtest #95999

Conversation

msbutler commented Jan 26, 2023 • edited Loading

cockroach-teamcity commented Jan 26, 2023

msbutler commented Jan 26, 2023

msbutler commented Jan 26, 2023

knz left a comment

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

msbutler commented Feb 1, 2023 • edited Loading

craig bot commented Feb 1, 2023

craig bot commented Feb 2, 2023

msbutler commented Jan 26, 2023 •

edited

Loading

msbutler commented Feb 1, 2023 •

edited

Loading