kvtenant: move kv tenant connector out of ccl by herkolategan · Pull Request #98203 · cockroachdb/cockroach

herkolategan · 2023-03-08T12:23:49Z

This change moves the kv tenant connector out of kvtenantccl to enable wider
test coverage of multi-tenant outside ccl. There are now two versions of the
kv tenant connector factory, one that provides a connector that only connects
clients to a loopback address, and another that will allow both loopback and
remote addresses. If the loopback version is used in the wrong context an error
will be returned explaining license requirements.

To assist with testing, utilities have been added to instantiate the required kv
connector factory. Tests requiring a kv connector that can connect to remote
nodes can use a testing only factory that is available outside ccl. Existing
tests that fell into this category, and depended on ccl, have been updated to
use the appropriate utility and connector.

This change does not yet remove the license check for maybe starting a tenant
when a test server is started, since this will most likely break various
existing tests and will require a larger change to fix, it will be done
separately.

Additionally, there are a few other places where TenantKVAddrs is populated
with a loopback address, and we can rather favour the new TenantLoopbackAddr
to better let the connector factory know that it's only making a loopback
connection and choose the correct logic accordingly. This will be implemented as
part of the test changes mentioned above.

Resolves: #98226
Epic: CRDB-16091

cockroach-teamcity · 2023-03-08T12:23:58Z

This change is

knz

Excellent, thanks. I have only a few comments. Let's rebase this and have a look at CI before this can merge.

Reviewed 25 of 41 files at r1, 2 of 7 files at r2, 1 of 1 files at r4, 4 of 11 files at r5, 15 of 15 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan)

pkg/kv/kvclient/kvtenant/connector.go line 139 at r6 (raw file):

// connector mediates the communication of cluster-wide state to sandboxed
// SQL-only tenant processes through a restricted interface.

through this comment and the other comments below replace "process" by "server".

pkg/kv/kvclient/kvtenant/connector.go line 143 at r6 (raw file):

// A connector is instantiated inside a tenant's SQL process and is seeded with
// a set of one or more network addresses that reference existing KV nodes in
// the host cluster (or a load-balancer which fans out to some/all KV nodes). On

s/host/storage, here and below

pkg/kv/kvclient/kvtenant/connector.go line 148 at r6 (raw file):

// communication.
//
// The connector communicates with the host cluster through the roachpb.Internal

s/host/storage

pkg/kv/kvclient/kvtenant/connector.go line 191 at r6 (raw file):

// connector is capable of providing information on each of the KV nodes in the
// cluster in the form of NodeDescriptors. This obviates the need for SQL-only

replace "SQL-only tenant processes" to "tenant SQL servers", here and below.

pkg/kv/kvclient/kvtenant/connector_factory.go line 36 at r6 (raw file):

}

type KVAddressConfig struct {

I think this needs a docstring.

This change moves the kv tenant connector out of `kvtenantccl` to enable wider test coverage of multi-tenant outside `ccl`. There are now two versions of the kv tenant connector factory, one that provides a connector that only connects clients to a loopback address, and another that will allow both loopback and remote addresses. If the loopback version is used in the wrong context an error will be returned explaining license requirements. To assist with testing, utilities have been added to instantiate the required kv connector factory. Tests requiring a kv connector that can connect to remote nodes can use a testing only factory that is available outside `ccl`. Existing tests that fell into this category, and depended on `ccl`, have been updated to use the appropriate utility and connector. This change does not yet remove the license check for maybe starting a tenant when a test server is started, since this will most likely break various existing tests and will require a larger change to fix, it will be done separately. Additionally, there are a few other places where `TenantKVAddrs` is populated with a loopback address, and we can rather favour the new `TenantLoopbackAddr` to better let the connector factory know that it's only making a loopback connection and choose the correct logic accordingly. This will be implemented as part of the test changes mentioned above. Resolves: cockroachdb#98226 Epic: CRDB-16091

herkolategan

@knz Thanks for the excellent review! Addressing the comments and doing a rebase. During the rebase I saw that there are two new ccl packages that added tests under kvtenantccl. I have decided to leave those there, unless instructed otherwise.

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

pkg/kv/kvclient/kvtenant/connector.go line 139 at r6 (raw file):