server: only run tenants with service mode 'shared' #95658

knz · 2023-01-21T21:57:29Z

RFC for context: #96147

Previous PRs:

sql: improve tenant records sql: improve tenant records #95691

Prior to this patch, services for secondary tenants would be started
automatically upon first use by a client.

This commit changes this to auto-start services upfront for all
tenants with service mode SHARED. (And shut down services for tenants
with another service mode configured.)

Release note: None
Epic: CRDB-14537

blathers-crl · 2023-01-21T21:57:33Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-01-21T21:57:36Z

This change is

stevendanna

Reviewed the second commit only.

Overall, I think the polling approach will work for now. Do we need to add a test to ensure that ensures that the STOP SERVICE command does tear down the server?

stevendanna · 2023-01-26T10:10:35Z

pkg/server/server_controller.go

+				if _, err := c.createServer(ctx, tenantName, base.TestSharedProcessTenantArgs{}); err != nil {
+					log.Warningf(ctx, "unable to start service for tenant: %v", err)
+				}


Do we want any backoff for tenants that we can't start for some reason? Currently, we'll retry every second which seems a bit aggressive if we've gotten an error.

stevendanna · 2023-01-26T10:11:40Z

pkg/server/server_controller.go

+
+	rows, err := ie.QueryBuffered(ctx, "fetch-running-tenants",
+		nil, /* txn */
+		`SELECT name FROM system.tenants WHERE service_mode = $1 AND name IS NOT NULL`, mtinfopb.ServiceModeShared)


I know we prevent it on write, but do we want an extra condition here that the data_mode is also in the correct state?

stevendanna · 2023-01-26T10:17:44Z

pkg/server/server_controller.go

+		if _, ok := reqTenants[name]; !ok {
+			// Tenant currently running, but should not be running. Shut it down.
+			if err := c.stopper.RunAsyncTask(ctx, "stop-tenant-async", func(ctx context.Context) {
+				srv.server.stop(context.Background())


I assume we pass context.Background() here so that the stop process isn't canceled when we start shutting down. If not, the real reason could be worth a comment.

stevendanna · 2023-01-26T10:24:22Z

pkg/server/admin.go

+		ie := s.internalExecutor
+		rowIter, err := ie.QueryIterator(ctx, "list-tenants", nil, /* txn */
+			`SELECT name FROM system.tenants WHERE service_mode = $1 AND name IS NOT NULL`, mtinfopb.ServiceModeShared)
+		if err != nil {
+			return nil, err
+		}
+		defer func() { _ = rowIter.Close() }()
+
+		var hasNext bool
+		for hasNext, err = rowIter.Next(ctx); hasNext && err == nil; hasNext, err = rowIter.Next(ctx) {
+			row := rowIter.Cur()
+			tenantName := tree.MustBeDString(row[0])
+			tenantNames = append(tenantNames, roachpb.TenantName(tenantName))
+		}


I could see extracting this to a small function.

stevendanna · 2023-01-26T10:25:01Z

pkg/server/admin.go

+			row := rowIter.Cur()
+			tenantName := tree.MustBeDString(row[0])
+			tenantNames = append(tenantNames, roachpb.TenantName(tenantName))
+		}
 	}

 	var tenantList []*serverpb.Tenant


I suppose we could pre-allocate this since in the common case the tenantList will be the same length as tenantNames.

stevendanna · 2023-01-26T10:25:58Z

pkg/server/admin.go

+// ListTenants returns a list of tenants that are served
+// by shared-process services in this server.


I am assuming that existing callers of this API are OK with this change?

knz

Do we need to add a test to ensure that ensures that the STOP SERVICE command does tear down the server?

Absolutely! I had forgotten. Good catch. Done.

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

pkg/server/admin.go line 4226 at r1 (raw file):