Merge cockroachdb#113770

113770: pkg/cli: move the default store for tenants back to using tenant-id as suffix r=andy-kimball a=jaylim-crl

Previously, in cockroachdb#110082, we updated the default tenant store to use a random
suffix, and to be precise, the PID. This can lead to a bunch of directories
being generated whenever someone runs `mt start-sql` locally. The original
motivation of using a PID was because with the `--tenant-id-flag`, we no
longer have a tenant ID during process startup, so we can't construct the
default store's name.

This commit reverts that change and moves the default tenant store back to
using tenant-id as a suffix to avoid the issues described above. Given that
the original problem only existed when the `--tenant-id-flag` is supplied, we
will explicitly require that the flag is used with the `--store` flag.

This only impacts local development, and the --tenant-id-flag is only used
by CockroachDB Cloud.

For an internal discussion, see Slack thread:
https://cockroachlabs.slack.com/archives/C02HWA24541/p1699021756659579.

Epic: none

Release note: None

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

pkg/cli/flags.go

@@ -1334,9 +1334,22 @@ func mtStartSQLFlagsInit(cmd *cobra.Command) error {
 	// Override default store for mt to use a per tenant store directory.
 	fs := cliflagcfg.FlagSetForCmd(cmd)
 	if !fs.Changed(cliflags.Store.Name) {
-		// We assume that we only need to change top level store as temp dir configs are
-		// initialized when start is executed and temp dirs inherit path from first store.
-		serverCfg.Stores.Specs[0].Path += fmt.Sprintf("-tenant-%d", os.Getpid())
+		// If the tenant-id-file flag was supplied, this means that we don't
+		// have a tenant ID during process startup, so we can't construct the
+		// default store name. In that case, explicitly require that the
+		// store is supplied.
+		if fs.Lookup(cliflags.TenantIDFile.Name).Value.String() != "" {
+			return errors.Newf(
+				"--%s must be explicitly supplied when using --%s",
+				cliflags.Store.Name,
+				cliflags.TenantIDFile.Name,
+			)
+		}
+		// We assume that we only need to change top level store as temp dir
+		// configs are initialized when start is executed and temp dirs inherit
+		// path from first store.
+		tenantID := fs.Lookup(cliflags.TenantID.Name).Value.String()
+		serverCfg.Stores.Specs[0].Path += "-tenant-" + tenantID
 	}
 
 	// In standalone SQL servers, we do not generate a ballast file,

pkg/cli/flags_test.go

@@ -1396,27 +1396,47 @@ func TestSQLPodStorageDefaults(t *testing.T) {
 
 	defer initCLIDefaults()
 
-	expectedDefaultDir, err := base.GetAbsoluteStorePath("",
-		fmt.Sprintf("cockroach-data-tenant-%d", os.Getpid()))
+	expectedDefaultDir, err := base.GetAbsoluteStorePath("", "cockroach-data-tenant-9")
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	for _, td := range []struct {
-		args      []string
-		storePath string
-	}{{[]string{"mt", "start-sql", "--tenant-id", "9"}, expectedDefaultDir},
-		{[]string{"mt", "start-sql", "--tenant-id", "9", "--store", "/tmp/data"}, "/tmp/data"},
+		args        []string
+		storePath   string
+		expectedErr string
+	}{
+		{
+			args:      []string{"mt", "start-sql", "--tenant-id", "9"},
+			storePath: expectedDefaultDir,
+		},
+		{
+			args:      []string{"mt", "start-sql", "--tenant-id", "9", "--store", "/tmp/data"},
+			storePath: "/tmp/data",
+		},
+		{
+			args:      []string{"mt", "start-sql", "--tenant-id-file", "foo", "--store", "/tmp/data"},
+			storePath: "/tmp/data",
+		},
+		{
+			args:        []string{"mt", "start-sql", "--tenant-id-file", "foo"},
+			expectedErr: "--store must be explicitly supplied when using --tenant-id-file",
+		},
 	} {
 		t.Run(strings.Join(td.args, ","), func(t *testing.T) {
 			initCLIDefaults()
 			f := mtStartSQLCmd.Flags()
 			require.NoError(t, f.Parse(td.args))
-			require.NoError(t, mtStartSQLCmd.PersistentPreRunE(mtStartSQLCmd, td.args))
-			assert.Equal(t, td.storePath, serverCfg.Stores.Specs[0].Path)
-			for _, s := range serverCfg.Stores.Specs {
-				assert.Zero(t, s.BallastSize.InBytes)
-				assert.Zero(t, s.BallastSize.Percent)
+			err := mtStartSQLCmd.PersistentPreRunE(mtStartSQLCmd, td.args)
+			if td.expectedErr == "" {
+				require.NoError(t, err)
+				assert.Equal(t, td.storePath, serverCfg.Stores.Specs[0].Path)
+				for _, s := range serverCfg.Stores.Specs {
+					assert.Zero(t, s.BallastSize.InBytes)
+					assert.Zero(t, s.BallastSize.Percent)
+				}
+			} else {
+				require.EqualError(t, err, td.expectedErr)
 			}
 		})
 	}