server: use "read-only" Gossip for tenants #49693

tbg · 2020-05-29T10:59:08Z

We were previously using the Gossip instance of the TestServer against
which the tenant was initialized.

This commit trims the dependency further by initializing its own Gossip
instance which is never written to (i.e. AddInfo is never called) and
which does not accept incoming connections.

As a reminder, the remaining problematic uses of Gossip as of this
commit are:

making a nodeDialer (for DistSender), tracked in:
kv: DistSender for SQL tenants #47909
access to the system config:
- (schemaChangeGCResumer).Resume, tracked:
  schema: lose gossiped system config dep in schemaChangeGCResumer #49691
- database.Cache, tracked:
  database: lose gossiped system config dep in Cache #49692
(physicalplan).spanResolver (for replica oracle).
This is likely not a blocker as we can use a "dumber" oracle in this case;
the oracle is used for distsql physical planning of which tenants will
do none. Tracked in: replicaoracle: avoid gossip and node descriptor #48432

cc @ajwerner

Release note: None

cockroach-teamcity · 2020-05-29T10:59:18Z

This change is

asubiotto

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 4 of 4 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, 3 of 3 files at r6.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @tbg)

pkg/server/config.go, line 292 at r2 (raw file):

		},
	}
	kvCfg.SetDefaults()

nit: I find kvCfg.RaftConfig.SetDefaults easier to read, up to you

pkg/server/testserver.go, line 636 at r5 (raw file):

		}
	}
	ctx := context.Background()

nit: pass this context in?

pkg/server/testserver.go, line 580 at r6 (raw file):

			recorder:     dummyRecorder,
			isMeta1Leaseholder: func(timestamp hlc.Timestamp) (bool, error) {
				return false, errors.New("isMeta1Leaseholder is not available to tenants")

s/to tenants/to non-system tenants?

nvanbenschoten

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 4 of 4 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, 2 of 3 files at r6.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @tbg)

pkg/server/config.go, line 145 at r1 (raw file):

	baseCfg := BaseConfig{
		AmbientCtx:        log.AmbientContext{Tracer: st.Tracer},
		Config:            new(base.Config),

nit: any reason to embed a pointer?

Release note: None

tbg

TFTRs!

Comments addressed.

bors r=asubiotto,nvanbenschoten

Dismissed @asubiotto from a discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @asubiotto and @nvanbenschoten)

craig · 2020-06-02T13:43:58Z

Build failed

GitHub CI (Cockroach)

Release note: None

We were previously using the Gossip instance of the TestServer against which the tenant was initialized. This commit trims the dependency further by initializing its own Gossip instance which is never written to (i.e. `AddInfo` is never called) and which does not accept incoming connections. As a reminder, the remaining problematic uses of Gossip as of this commit are: - making a `nodeDialer` (for `DistSender`), tracked in: cockroachdb#47909 - access to the system config: - `(schemaChangeGCResumer).Resume`, tracked: cockroachdb#49691 - `database.Cache`, tracked: cockroachdb#49692 - `(physicalplan).spanResolver` (for replica oracle). This is likely not a blocker as we can use a "dumber" oracle in this case; the oracle is used for distsql physical planning of which tenants will do none. Tracked in: cockroachdb#48432 Release note: None

The goal is to turn `testSQLServerArgs` into something that can be re-used to introduce a cli command for starting a SQL tenant. This commit takes a step in that direction by lifting some testing-specific inputs up. Release note: None

Release note: None

tbg · 2020-06-02T13:59:50Z

Embedding base.Config by value broke the build pretty badly. I'll just remove that commit for now, we're likely now taking a copy where we previously changed the original. Not worth holding this PR up for but it's on my TODO list.

bors r=asubiotto,nvanbenschoten

craig · 2020-06-02T16:02:13Z

Build succeeded

GitHub CI (Cockroach)

This adds a CLI command to start a SQL tenant in a standalone process. The tenant currently communicates with the KV layer "as a node"; this will only change with cockroachdb#47898. As is, the tenant has full access to the KV layer and so a few things may work that shouldn't as a result of that. Additionally, the tenant runs a Gossip client as a temporary stopgap until we have removed the remaining dependencies on it (cockroachdb#49693 has the details on what those are). Apart from that, though, this is the real thing and can be used to start setting up end-to-end testing of ORMs as well as the deploy- ments. A word on the choice of the `mt` command: `mt` is an abbreviation for `multi-tenant` which was considered too long; just `tenant` was considered misleading - there will be multiple additional subcommands housing the other services required for running the whole infrastructure including certs generation, the KV auth broker server, and the SQL proxy. Should a lively bikeshed around naming break out we can rename the commands later when consensus has been reached. For those curious to try it out the following will be handy: ```bash rm -rf ~/.cockroach-certs cockroach-data && killall -9 cockroach && \ export COCKROACH_CA_KEY=$HOME/.cockroach-certs/ca.key && \ ./cockroach cert create-ca && \ ./cockroach cert create-client root && \ ./cockroach cert create-node 127.0.0.1 && \ ./cockroach start --host 127.0.0.1 --background && \ ./cockroach sql --host 127.0.0.1 -e 'select crdb_internal.create_tenant(123);' ./cockroach mt start-sql --tenant-id 123 --kv-addrs 127.0.0.1:26257 --sql-addr :5432 & sleep 1 && \ ./cockroach sql --host 127.0.0.1 --port 5432 ``` This roughly matches what the newly introduced `acceptance/multitenant` roachtest does as well. Release note: None

@nvanbenschoten

49908: server: add ./cockroach mt start-sql r=knz,nvanbenschoten a=tbg This adds a CLI command to start a SQL tenant in a standalone process. The tenant currently communicates with the KV layer "as a node"; this will only change with #47898. As is, the tenant has full access to the KV layer and so a few things may work that shouldn't as a result of that. Additionally, the tenant runs a Gossip client as a temporary stopgap until we have removed the remaining dependencies on it (#49693 has the details on what those are). Apart from that, though, this is the real thing and can be used to start setting up end-to-end testing of ORMs as well as the deploy- ments. A word on the choice of the `mt` command: `mt` is an abbreviation for `multi-tenant` which was considered too long; just `tenant` was considered misleading - there will be multiple additional subcommands housing the other services required for running the whole infrastructure including certs generation, the KV auth broker server, and the SQL proxy. Should a lively bikeshed around naming break out we can rename the commands later when consensus has been reached. For those curious to try it out the following will be handy: ```bash rm -rf ~/.cockroach-certs cockroach-data && killall -9 cockroach && \ export COCKROACH_CA_KEY=$HOME/.cockroach-certs/ca.key && \ ./cockroach cert create-ca && \ ./cockroach cert create-client root && \ ./cockroach cert create-node 127.0.0.1 && \ ./cockroach start --host 127.0.0.1 --background && \ ./cockroach sql --host 127.0.0.1 -e 'select crdb_internal.create_tenant(123);' ./cockroach mt start-sql --tenant-id 123 --kv-addrs 127.0.0.1:26257 --sql-addr :5432 & sleep 1 && \ ./cockroach sql --host 127.0.0.1 --port 5432 ``` This roughly matches what the newly introduced `acceptance/multitenant` roachtest does as well. cc @nvanbenschoten @asubiotto Release note: None Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>

tbg requested review from nvanbenschoten and asubiotto May 29, 2020 10:59

tbg force-pushed the tenant-gossip-hack branch 2 times, most recently from 3d71bab to 1a27edc Compare May 29, 2020 14:12

tbg marked this pull request as ready for review May 29, 2020 14:12

tbg added the A-multitenancy Related to multi-tenancy label May 29, 2020

asubiotto approved these changes Jun 1, 2020

View reviewed changes

nvanbenschoten approved these changes Jun 1, 2020

View reviewed changes

server: add Make{SQL,Base}Config

151f128

Release note: None

tbg force-pushed the tenant-gossip-hack branch from 1a27edc to 2825c9d Compare June 2, 2020 11:19

tbg requested a review from a team as a code owner June 2, 2020 11:19

tbg requested review from asubiotto and nvanbenschoten June 2, 2020 11:19

tbg commented Jun 2, 2020

View reviewed changes

tbg added 7 commits June 2, 2020 15:58

server: introduce KVConfig

7c5360f

Release note: None

server: fold tenantID into SQLConfig

b424bbd

Release note: None

server: makeTestConfig -> makeTest{SQL,Base,KV}Config

06de45a

Release note: None

server: make cli-ready startTenant method()

72cd586

The goal is to turn `testSQLServerArgs` into something that can be re-used to introduce a cli command for starting a SQL tenant. This commit takes a step in that direction by lifting some testing-specific inputs up. Release note: None

testserver: massage mock error messages

76eed9c

Release note: None

server: pass ctx to startTenant

d94b356

Release note: None

tbg force-pushed the tenant-gossip-hack branch from 2825c9d to d94b356 Compare June 2, 2020 13:58

craig bot merged commit 3b2245f into cockroachdb:master Jun 2, 2020

tbg deleted the tenant-gossip-hack branch June 4, 2020 07:35

tbg mentioned this pull request Jun 5, 2020

server: add ./cockroach mt start-sql #49908

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: use "read-only" Gossip for tenants #49693

server: use "read-only" Gossip for tenants #49693

tbg commented May 29, 2020

cockroach-teamcity commented May 29, 2020

asubiotto left a comment

nvanbenschoten left a comment

tbg left a comment

craig bot commented Jun 2, 2020

tbg commented Jun 2, 2020

craig bot commented Jun 2, 2020

server: use "read-only" Gossip for tenants #49693

server: use "read-only" Gossip for tenants #49693

Conversation

tbg commented May 29, 2020

cockroach-teamcity commented May 29, 2020

asubiotto left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

tbg left a comment

Choose a reason for hiding this comment

craig bot commented Jun 2, 2020

Build failed

tbg commented Jun 2, 2020

craig bot commented Jun 2, 2020

Build succeeded