ccl/sqlproxyccl: directory proto and test server #64171

darinpp · 2021-04-24T00:33:05Z

Defines a new interface between a tenant directory client and server
Moves the tenant directory from the CC repo over
Tenant directory modified to use the new interface
Tenant directory modified to use stop.Stopper
Modified pod watcher to use streaming grpc call
Renamed package from directory to tenant, tenantID to ID etc
Renamed references to k8s, pods to server, endpoints etc
Prevents test tenant servers to start for non-existing/inactive tenants
Adds ability to shut down individual tenant servers within a test cluster
Adds a test directory server that can start/stop tenants
Adds tests of the directory running agianst the test server
Allow insecure connections from test tenant to KV server
Fixed a race in kvtenantccl

Release note: None

cockroach-teamcity · 2021-04-24T00:33:11Z

This change is

andy-kimball

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

pkg/ccl/kvccl/kvtenantccl/connector.go, line 372 at r1 (raw file):

			return nil, err
		}
		c.mu.RLock()

It'd be good to add a comment here explaining why the read lock is in fact needed.

pkg/ccl/sqlproxyccl/tenant/directory.go, line 66 at r1 (raw file):

type Directory struct {
	// ctl is the directory client instance used to make directory server calls.
	ctl DirectoryClient

NIT: I think this should be renamed to client or dir rather than using ctl like in the operator.

pkg/ccl/sqlproxyccl/tenant/directory.go, line 88 at r1 (raw file):

// NewDirectory constructs a new Directory instance that tracks SQL tenant processes
// managed by a given Directory server. The given context

NIT: spacing in comment.

pkg/ccl/sqlproxyccl/tenant/directory.go, line 257 at r1 (raw file):

// a notification and update the directory to reflect that change.
func (d *Directory) watchEndpoints(ctx context.Context, stopper *stop.Stopper) error {

NIT: extra blank line - you're triggering my OCD : )

pkg/ccl/sqlproxyccl/tenant/directory.go, line 260 at r1 (raw file):

	req := WatchEndpointsRequest{}

	var waitInit sync.WaitGroup

Maybe add a comment here explaining why this WaitGroup is needed.

pkg/ccl/sqlproxyccl/tenant/directory.go, line 281 at r1 (raw file):

					}
					log.Errorf(ctx, "err creating new watch endpoint client: %s", err)
					time.Sleep(time.Second)

Use the sleepContext helper method for this and any other sleeps, so that we cancel more quickly (and also consistently use it).

pkg/ccl/sqlproxyccl/tenant/directory.go, line 307 at r1 (raw file):

			}

			// Ensure that a directory entry exists for this tenant.

I think we decided to not create a new entry for pods that don't have a corresponding tenant in the cache yet. That would mean passing allowCreate=false below.

pkg/ccl/sqlproxyccl/tenant/directory.proto, line 15 at r1 (raw file):

import "gogoproto/gogo.proto";

message WatchEndpointsRequest {}

This file needs comments on enums/messages/fields/etc.

pkg/ccl/sqlproxyccl/tenant/directory_test.go, line 30 at r1 (raw file):

)

func CreateTenant(tc serverutils.TestClusterInterface, id ID) error {

Is there some reason that these functions need to be exported? Also, I think it's nicer when helper code like this is at the bottom of the file, so that the actual tests can be placed at the top of the file.

pkg/ccl/sqlproxyccl/tenant/entry.go, line 33 at r1 (raw file):

	TenantID ID

	// Full name of the tenant's cluster i.e. dim-dog

NIT: missing period at end of comment.

pkg/ccl/sqlproxyccl/tenant/entry.go, line 68 at r1 (raw file):

// that in the entry. After this is called once, all future calls return the
// same result (and do nothing).
func (e *tenantEntry) Initialize(ctx context.Context, ctl DirectoryClient) error {

NIT: similar to above, rename ctl to client or dir.

pkg/ccl/sqlproxyccl/tenant/entry.go, line 192 at r1 (raw file):

// endpoints available rather than blocking.
func (e *tenantEntry) ensureTenantEndpoint(
	ctx context.Context, ctl DirectoryClient, errorIfNoEndpoints bool,

NIT: rename ctl here as well (and any other places I missed).

pkg/ccl/sqlproxyccl/tenant/entry.go, line 232 at r1 (raw file):

		// Get endpoint information for the newly resumed tenant. Except in rare race
		// conditions, this is expected to immediately find an IP address, since
		// the above call started a tenant process that  already has an IP address.

NIT: double space in comment.

pkg/ccl/sqlproxyccl/tenant/entry.go, line 259 at r1 (raw file):

	ctx context.Context, ctl DirectoryClient,
) (ips []string, err error) {
	// List endpoints with a "tenant-id" label that matches this tenant's ID.

NIT: This comment is out-of-date.

pkg/ccl/sqlproxyccl/tenant/tenant_id.go, line 26 at r1 (raw file):

// For more information, see:
//   https://github.com/cockroachdb/cockroach/blob/release-20.2/pkg/roachpb/tenant.go#L19-L24
type ID uint64

We should use roachpb.TenantID instead of defining a new variant in CRDB.

pkg/ccl/sqlproxyccl/tenant/test_directory_svr.go, line 73 at r1 (raw file):

	// When both mutexes need to be held, the locking should always be
	// proc first and listen second

NIT: missing period at end of comment.

pkg/ccl/sqlproxyccl/tenant/test_directory_svr.go, line 273 at r1 (raw file):

		return nil, err
	}
	process := &Process{SQL: sql.Addr()}

What happens if the test is stopped using ctrl-C? Will the processes still be cleaned up?

pkg/testutils/serverutils/test_server_shim.go, line 225 at r1 (raw file):

	// StartTenant spawns off tenant process connecting to this TestServer with
	// context
	StartTenantWithCtx(ctx context.Context, params base.TestTenantArgs) (TestTenantInterface, error)

Why not just update StartTenant and then change all callers to pass a context? I don't see much value in having both variants.

darinpp

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball)

pkg/ccl/kvccl/kvtenantccl/connector.go, line 372 at r1 (raw file):