Merge #147731

147731: roachprod: fix service registration for unregistered services r=herkolategan a=DarrylWong

As of #125393, we no longer register all virtual clusters with DNS. This was an optimization made to reduce unnecessary DNS usage when there was only one tenant using the default ports. Specifically, we no longer register shared process tenants and system tenants using the default ports. 

However, some of the existing logic still assumes that registration happens for all virtual clusters. For example, `clusterSynced.Stop` errors out if a shared process virtual cluster is not registered, even though this is now the expected behavior.

This change:

1. Establishes that the `system interface` is a `shared-process service` as it contains both a KV and SQL server. Previously, a system tenant could be registered as either depending on if custom ports were used or not.
2. Renames `DiscoverService` to `ServiceDescriptor` to differentiate it from `DiscoverServices`. `DiscoverService` is a convenience function for `DiscoverServices` that returns just a single service. Importantly, it also implements fall back logic to handle when a service is not found which is necessary now that we no longer register all services. 
3. Renames `DiscoverServices` to `discoverServices`. Since this function does not implement fall back logic needed for correctness, it should not be used directly. All existing usage of `discoverServices` is refactored to use `ServiceDescriptor` instead.
4. Adds a unit test to ensure our new fall back logic behaves as expected.

Release note: none
Epic: none
Fixes: none

Co-authored-by: DarrylWong <[email protected]>

Author: craig[bot]

pkg/roachprod/install/BUILD.bazel

@@ -71,6 +71,7 @@ go_test(
     embed = [":install"],
     deps = [
         "//pkg/roachprod/cloud",
+        "//pkg/roachprod/config",
         "//pkg/roachprod/logger",
         "//pkg/roachprod/vm",
         "//pkg/roachprod/vm/gce/testutils",
@@ -79,6 +80,7 @@ go_test(
         "//pkg/util/leaktest",
         "//pkg/util/randutil",
         "//pkg/util/retry",
+        "//pkg/util/syncutil",
         "@com_github_cockroachdb_datadriven//:datadriven",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_stretchr_testify//require",

pkg/roachprod/install/cluster_synced.go

@@ -70,6 +70,10 @@ type SyncedCluster struct {
 
 	// Nodes is used by most commands (e.g. Start, Stop, Monitor). It describes
 	// the list of nodes the operation pertains to.
+	//	$ roachprod create local -n 4
+	//	$ roachprod start local          # [1, 2, 3, 4]
+	//	$ roachprod start local:2-4      # [2, 3, 4]
+	//	$ roachprod start local:2,1,4    # [1, 2, 4]
 	Nodes Nodes
 
 	ClusterSettings
@@ -239,17 +243,6 @@ func (c *SyncedCluster) localVMDir(n Node) string {
 	return local.VMDir(c.Name, int(n))
 }
 
-// TargetNodes is the fully expanded, ordered list of nodes that any given
-// roachprod command is intending to target.
-//
-//	$ roachprod create local -n 4
-//	$ roachprod start local          # [1, 2, 3, 4]
-//	$ roachprod start local:2-4      # [2, 3, 4]
-//	$ roachprod start local:2,1,4    # [1, 2, 4]
-func (c *SyncedCluster) TargetNodes() Nodes {
-	return append(Nodes{}, c.Nodes...)
-}
-
 // GetInternalIP returns the internal IP address of the specified node.
 func (c *SyncedCluster) GetInternalIP(n Node) (string, error) {
 	if c.IsLocal() {
@@ -471,31 +464,24 @@ func (c *SyncedCluster) Stop(
 	// killProcesses indicates whether processed need to be stopped.
 	killProcesses := true
 
-	// Non system shared process virtual clusters don't get killed but are stopped via SQL.
-	// Figure out if the virtual cluster is one or not.
+	// For shared process secondary tenants, we just stop the service via SQL.
+	// Find out of this is a shared process secondary tenant.
 	if virtualClusterLabel != "" {
 		name, sqlInstance, err := VirtualClusterInfoFromLabel(virtualClusterLabel)
 		if err != nil {
 			return err
 		}
 
-		if name != SystemInterfaceName {
-			services, err := c.DiscoverServices(ctx, name, ServiceTypeSQL)
+		if !IsSystemInterface(name) {
+			isExternal, err := c.IsExternalService(ctx, name)
 			if err != nil {
 				return err
 			}
 
-			if len(services) == 0 {
-				return fmt.Errorf("no service for virtual cluster %q", virtualClusterName)
-			}
-
-			virtualClusterName = name
-			if services[0].ServiceMode == ServiceModeShared {
-				// For shared process virtual clusters, we just stop the service
-				// via SQL.
-				killProcesses = false
-			} else {
+			if isExternal {
 				virtualClusterDisplay = fmt.Sprintf(" virtual cluster %q, instance %d", virtualClusterName, sqlInstance)
+			} else {
+				killProcesses = false
 			}
 		}
 	}
@@ -2275,7 +2261,7 @@ func (c *SyncedCluster) pgurls(
 	}
 	m := make(map[Node]string, len(hosts))
 	for node, host := range hosts {
-		desc, err := c.DiscoverService(ctx, node, virtualClusterName, ServiceTypeSQL, sqlInstance)
+		desc, err := c.ServiceDescriptor(ctx, node, virtualClusterName, ServiceTypeSQL, sqlInstance)
 		if err != nil {
 			return nil, err
 		}
@@ -2310,24 +2296,19 @@ func (c *SyncedCluster) loadBalancerURL(
 	sqlInstance int,
 	auth PGAuthMode,
 ) (string, error) {
-	services, err := c.DiscoverServices(ctx, virtualClusterName, ServiceTypeSQL)
+	// Note that it's possible for our service to not be running on the entire roachprod
+	// cluster, e.g. one of the nodes is a workload node, or we have a separate process
+	// virtual cluster running on a subset of nodes. We must search for our service on
+	// the entire cluster.
+	descs, err := c.ServiceDescriptors(ctx, c.Nodes, virtualClusterName, ServiceTypeSQL, sqlInstance)
 	if err != nil {
 		return "", err
 	}
-	port := config.DefaultSQLPort
-	serviceMode := ServiceModeExternal
-	for _, service := range services {
-		if service.VirtualClusterName == virtualClusterName && service.Instance == sqlInstance {
-			serviceMode = service.ServiceMode
-			port = service.Port
-			break
-		}
-	}
-	address, err := c.FindLoadBalancer(l, port)
+	address, err := c.FindLoadBalancer(l, descs[0].Port)
 	if err != nil {
 		return "", err
 	}
-	loadBalancerURL := c.NodeURL(address.IP, address.Port, virtualClusterName, serviceMode, auth, "" /* database */)
+	loadBalancerURL := c.NodeURL(address.IP, address.Port, virtualClusterName, descs[0].ServiceMode, auth, "" /* database */)
 	return loadBalancerURL, nil
 }
 
@@ -2732,7 +2713,7 @@ func (c *SyncedCluster) Reset(l *logger.Logger) error {
 		return nil
 	}
 
-	nodes := c.TargetNodes()
+	nodes := c.Nodes
 	targetVMs := make(vm.List, len(nodes))
 	for idx, node := range nodes {
 		targetVMs[idx] = c.VMs[node-1]