server: add ./cockroach mt start-sql #49908
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 8 of 8 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @tbg)
pkg/cli/flags.go, line 726 at r1 (raw file):
// NB: serverInsecure populates baseCfg.{Insecure,SSLCertsDir} in this the following method // (which is a PreRun for this command): _ = extraServerFlagInit
I think that's missing ...() at the end
pkg/cli/flags.go, line 732 at r1 (raw file):
VarFlag(f, addrSetter{&serverSQLAddr, &serverSQLPort}, cliflags.ListenSQLAddr) StringSlice(f, &serverCfg.SQLConfig.TenantKVAddrs, cliflags.KVAddrs, []string{"127.0.0.1:26257"})
the default value should be assigned to TenantKVAddrs in context.go
pkg/cmd/roachtest/multitenant.go, line 41 at r1 (raw file):
"--tenant-id", "123", "--kv-addrs", strings.Join(kvAddrs, ","), "--sql-addr", "0.0.0.0:36257",
I don't understand "0.0.0.0" here - why do you need to listen on all interfaces?
tbg
force-pushed
the
sqlcli
branch
2 times, most recently
from
0b4e6dc to
98134e5
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/cli/flags.go, line 726 at r1 (raw file):
Previously, knz (kena) wrote…
I think that's missing
...()at the end
No, I didn't want to call it - this is just an anchor to make it easier to navigate to the method (and it prevents rot should the method get renamed).
pkg/cli/flags.go, line 732 at r1 (raw file):
Previously, knz (kena) wrote…
the default value should be assigned to TenantKVAddrs in context.go
Done.
pkg/cmd/roachtest/multitenant.go, line 41 at r1 (raw file):
Previously, knz (kena) wrote…
I don't understand "0.0.0.0" here - why do you need to listen on all interfaces?
I don't, but I want to make sure I listen on the external one in case this is running in roachtest. What do you want to see here? I can make it 127.0.0.1 if local if that's the concern. (Done).
There was a problem hiding this comment.
I would recommend ensuring that the output of cockroach mt help and the --help outputs for the sub-commands highlight that the feature is experimental (our code word for "we don't support this externally")
LGTM otherwise
Reviewed 4 of 4 files at r2.
Reviewable status:
pkg/cmd/roachtest/multitenant.go, line 41 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
I don't, but I want to make sure I listen on the external one in case this is running in roachtest. What do you want to see here? I can make it
127.0.0.1if local if that's the concern. (Done).
Ok I simply did not understand.
In that case, I'd suggest simplifying to "--sql-addr", ":36257" without an address prefix, in which case it should default to all-interfaces already.
There was a problem hiding this comment.
I would recommend ensuring that the output of cockroach mt help and the --help outputs for the sub-commands highlight that the feature is experimental (our code word for "we don't support this externally")
Done (the mt command is also hidden).
Reviewable status:
pkg/cmd/roachtest/multitenant.go, line 41 at r1 (raw file):
Previously, knz (kena) wrote…
Ok I simply did not understand.
In that case, I'd suggest simplifying to"--sql-addr", ":36257"without an address prefix, in which case it should default to all-interfaces already.
Are you opposed to the current code? I think it's kind of nice to not listen everywhere on --local runs.
No objection. |
There was a problem hiding this comment.
Nothing major in the review. This is exciting to see!
Reviewed 3 of 8 files at r1, 4 of 4 files at r2, 2 of 2 files at r3.
Reviewable status:
pkg/cli/cli_mt.go, line 1 at r3 (raw file):
// Copyright 2020 The Cockroach Authors.
nit: why prefix the filename with cli_? We don't seem to do that anywhere else.
pkg/cli/cli_mt.go, line 16 at r3 (raw file):
func init() { cockroachCmd.AddCommand(mtCmd)
I'm assuming you deliberately did not place this in the list of all other commands in cli.go?
pkg/cli/flags.go, line 741 at r3 (raw file):
return errors.Wrap(err, "invalid tenant ID") } *w.tenID = roachpb.MakeTenantID(tenID)
This is going to blow up with a tenID of 0. Are you ok with that?
pkg/cli/cliflags/flags_mt.go, line 23 at r3 (raw file):
KVAddrs = FlagInfo{ Name: "kv-addrs", Shorthand: "",
nit: why specify this?
pkg/cmd/roachtest/acceptance.go, line 45 at r3 (raw file):
{name: "gossip/restart-node-one", fn: runGossipRestartNodeOne}, {name: "gossip/locality-address", fn: runCheckLocalityIPAddress}, {name: "multitenant", fn: runAcceptanceMultitenant},
We need a minVersion here, right?
pkg/server/testserver.go, line 629 at r3 (raw file):
} // StartTenant starts a stand-alone SQL server against a dedicated KV backend.
What does "dedicated" mean here? The KV backend can and is still running its own SQL server, right?
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
There was a problem hiding this comment.
TFTR! Addressed all of the comments.
bors r=knz,nvanbenschoten
Reviewable status:
pkg/cli/flags.go, line 741 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
This is going to blow up with a tenID of 0. Are you ok with that?
I'm basically OK with it but I added a check to make things look nice.
pkg/cli/cli_mt.go, line 16 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I'm assuming you deliberately did not place this in the list of all other commands in
cli.go?
Correct.
Build succeeded |
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
mtcommand:mtis an abbreviation formulti-tenantwhich was considered too long; justtenantwasconsidered 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:
This roughly matches what the newly introduced
acceptance/multitenantroachtest does as well.
cc @nvanbenschoten @asubiotto
Release note: None