roachprod: add virtual cluster support to the monitor #111064

renatolabs · 2023-09-21T17:31:22Z

This PR makes the roachprod monitor aware of multi-tenancy. This means
that the monitor is able to identify scenarios where multiple tenants
are running on the same VM, and monitor the lifecycle of tenant
processes just like it does for non multi-tenant deployments.

Under the covers, this is achieved by leveraging the
"ROACHPROD_VIRTUAL_CLUSTER" environment variable that is
set when starting cockroach. The monitor then looks for processes
that have that environment variable set, and parses it to identify the
tenant name and instance corresponding to that cockroach process.

This change should go unnoticed by all current monitor callers: tests
that monitor cockroach processes will continue to be notified of
events just like they did before. The only thing that changes is that,
if a test uses roachprod to start and stop tenants, these events will
be available through the monitor.

This PR also adds a stop-tenant command that uses the same
ROACHPROD_TENANT environment variable to allow the caller to
stop a specific tenant process.

Epic: none

Release note: None

cockroach-teamcity · 2023-09-21T17:31:34Z

This change is

renatolabs · 2023-09-27T19:44:11Z

Open for review. A few runs so far:

SELECT_PROBABILITY=0.1, before rebasing against naming changes in roachprod: enhance/clarify terminology #111113: https://teamcity.cockroachdb.com/viewLog.html?buildId=11941103&buildTypeId=Cockroach_Nightlies_RoachtestNightlyGceBazel
SELECT_PROBABILITY=0.3, after rebasing; still running, nothing unusual so far: https://teamcity.cockroachdb.com/viewLog.html?buildId=11955819&tab=queuedBuildOverviewTab

herkolategan

Nice work! Minor nits about tenant vs. virtual cluster naming and a few other things.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @renatolabs and @smg260)

-- commits line 38 at r1:
Nit: Should this now be virtual cluster support?

-- commits line 43 at r1:
Nit: Should this now be non virtual cluster deployments?

pkg/cmd/roachprod/main.go line 561 at r1 (raw file):

		}

		// Always pick a random available port when starting tenants. We

Nit: virtual clusters.

pkg/roachprod/multitenant.go line 84 at r1 (raw file):

// StopServiceForVirtualCluster stops SQL instance processes on the virtualCluster given.
func StopServiceForVirtualCluster(
	ctx context.Context, l *logger.Logger, tenantCluster string, stopOpts StopOpts,

Nit: Do we want to update this to virtual cluster as well?

pkg/roachprod/multitenant.go line 99 at r1 (raw file):

// tenant with ID given.
//
// TODO(herko): Allow users to pass in a tenant name.

Nit: Few mentions of tenant here again. As stated in the TODO we'll probably want to be able to pass virtual cluster name in the future directly once processes can start with a virtual cluster name instead of a virtual cluster id.

pkg/roachprod/roachprod.go line 716 at r1 (raw file):

	MaxWait int

	// Options that only apply to StopTenant

Nit: StopVirtualCluster?

pkg/roachprod/install/cluster_synced.go line 403 at r1 (raw file):

// It sends a signal to all processes that have been started with ROACHPROD env
// var and optionally waits until the processes stop. If the virtualClusterLabel
// is not empty, then only tenant processes with a matching label are stopped.

Nit: sql processes.

pkg/roachprod/install/cluster_synced.go line 444 at r1 (raw file):

// for output/logging. If wait is true, the command will wait for the processes
// to exit, up to maxWait seconds.
// TODO(herko): This command does not support virtual clusters yet.

I suppose we can remove this comment now?

pkg/roachprod/install/cluster_synced.go line 647 at r1 (raw file):

	virtualClusterDesc := func(name string, instance int) string {
		if name == SystemInterfaceName {
			return "system tenant"

Nit: Wondering if we should call this system interface?

pkg/roachprod/install/cluster_synced.go line 746 at r1 (raw file):

			)
			if err := errors.CombineErrors(err, result.Err); err != nil {
				err := errors.Wrap(err, "failed to list tenants")

Nit: virtual clusters.

pkg/roachprod/install/cluster_synced.go line 869 at r1 (raw file):

{{ end }}

# make sure all tenant monitors quit when this script exits. In

Nit: sql process / virtual cluster monitors?

pkg/roachprod/install/cluster_synced.go line 939 at r1 (raw file):

						panic(fmt.Errorf("invalid output from monitor: %q", line))
					}
					// Tenant name and instance are the first fields of every

Nit: virtual cluster name.

pkg/roachprod/install/cluster_synced.go line 942 at r1 (raw file):

					// event type.
					name, instanceStr := parts[0], parts[1]
					instance, _ := strconv.Atoi(instanceStr)

Nit: Should we maybe panic like above in the rare case that this string is not an integer?

pkg/roachprod/install/cluster_synced.go line 949 at r1 (raw file):

						}})
					case runningMsg:
						pid := parts[3]

Nit: Should we add a len < 4 panic here as above?

pkg/roachprod/install/cluster_synced.go line 954 at r1 (raw file):

						}})
					case deadMsg:
						exitCode := parts[3]

Nit: Should we add a len < 4 panic here as above?

pkg/roachprod/install/cockroach.go line 602 at r1 (raw file):

}

// TenantInfoFromLabel takes as parameter a tenant label produced with

Nit: VirtualClusterInfoFromLabel ... virtual cluster label.

pkg/roachprod/install/cockroach_test.go line 27 at r1 (raw file):

	}{
		{
			name:               "empty tenant name",

Nit: empty virtual cluster name.

pkg/roachprod/install/cockroach_test.go line 32 at r1 (raw file):

		},
		{
			name:               "system tenant name",

Nit: System interface name?

pkg/roachprod/install/cockroach_test.go line 37 at r1 (raw file):

		},
		{
			name:               "simple app tenant name",

Nit: simple app virtual cluster name.

pkg/roachprod/install/cockroach_test.go line 43 at r1 (raw file):

		},
		{
			name:               "tenant name with hyphens",

Nit: virtual cluster name with hyphens.

pkg/roachprod/install/cockroach_test.go line 58 at r1 (raw file):

			require.NoError(t, err)

			expectedTenantName := tc.virtualClusterName

Nit: expectedVirtualClusterName.

renatolabs

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @herkolategan and @smg260)

-- commits line 38 at r1: