Skip to content

Commit 3572499

Browse files
craig[bot]herkolategan
craig[bot]
and
herkolategan
committed
104772: roachtest: remove unused create tenant options r=srosenberg,stevendanna a=herkolategan The `otherTenantIDs` field on `createTenantOptions` is no longer used. Complimentary to this change: #97585 Epic: [CRDB-23559](https://cockroachlabs.atlassian.net/browse/CRDB-23559) 104777: roachprod: fix confusing start-up error r=srosenberg a=herkolategan Starting a secure cluster locally does a check for certificates. In the event the certificates are not found, which is a valid case, an error is printed. The start command works correctly, but the error can cause confusion: ```bash ./bin/roachprod start local --secure 12:47:56 cluster_synced.go:2475: 0: COMMAND_PROBLEM: exit status 1 (1) COMMAND_PROBLEM Wraps: (2) exit status 1 Error types: (1) errors.Cmd (2) *exec.ExitError: local: initializing certs 1/1 / local: distributing certs 2/2 12:47:59 cockroach.go:184: local: starting nodes ``` This change modifies the command to do a check without causing an error, and also propagates any real errors that could occur while doing the check. Epic: none Co-authored-by: Herko Lategan <herko@cockroachlabs.com>
3 parents d43dd82 + 06cd54a + f0da5e8 commit 3572499

4 files changed

Lines changed: 23 additions & 47 deletions

File tree

‎pkg/cmd/roachtest/tests/admission_control_multitenant_fairness.go‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -203,11 +203,6 @@ func runMultiTenantFairness(
203203

204204
// Create the tenants.
205205
t.L().Printf("initializing %d tenants (<%s)", numTenants, 5*time.Minute)
206-
tenantIDs := make([]int, 0, numTenants)
207-
for i := 0; i < numTenants; i++ {
208-
tenantIDs = append(tenantIDs, tenantID(i))
209-
}
210-
211206
tenants := make([]*tenantNode, numTenants)
212207
for i := 0; i < numTenants; i++ {
213208
if !t.SkipInit() {
@@ -216,8 +211,7 @@ func runMultiTenantFairness(
216211
}
217212

218213
tenant := createTenantNode(ctx, t, c,
219-
crdbNode, tenantID(i), tenantNodeID(i), tenantHTTPPort(i), tenantSQLPort(i),
220-
createTenantOtherTenantIDs(tenantIDs))
214+
crdbNode, tenantID(i), tenantNodeID(i), tenantHTTPPort(i), tenantSQLPort(i))
221215
defer tenant.stop(ctx, t, c)
222216

223217
tenants[i] = tenant

‎pkg/cmd/roachtest/tests/multitenant_upgrade.go‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,6 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
8080

8181
settings := install.MakeClusterSettings(install.BinaryOption(predecessorBinary), install.SecureOption(true))
8282
c.Start(ctx, t.L(), option.DefaultStartOpts(), settings, kvNodes)
83-
tenantStartOpt := createTenantOtherTenantIDs([]int{11, 12, 13, 14})
8483

8584
const tenant11aHTTPPort, tenant11aSQLPort = 8011, 20011
8685
const tenant11bHTTPPort, tenant11bSQLPort = 8016, 20016
@@ -98,13 +97,13 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
9897
// Create two instances of tenant 11 so that we can test with two pods
9998
// running during migration.
10099
const tenantNode = 2
101-
tenant11a := createTenantNode(ctx, t, c, kvNodes, tenant11ID, tenantNode, tenant11aHTTPPort, tenant11aSQLPort, tenantStartOpt)
100+
tenant11a := createTenantNode(ctx, t, c, kvNodes, tenant11ID, tenantNode, tenant11aHTTPPort, tenant11aSQLPort)
102101
tenant11a.start(ctx, t, c, predecessorBinary)
103102
defer tenant11a.stop(ctx, t, c)
104103

105104
// Since the certs are created with the createTenantNode call above, we
106105
// call the "no certs" version of create tenant here.
107-
tenant11b := createTenantNodeNoCerts(ctx, t, c, kvNodes, tenant11ID, tenantNode, tenant11bHTTPPort, tenant11bSQLPort, tenantStartOpt)
106+
tenant11b := createTenantNodeNoCerts(ctx, t, c, kvNodes, tenant11ID, tenantNode, tenant11bHTTPPort, tenant11bSQLPort)
108107
tenant11b.start(ctx, t, c, predecessorBinary)
109108
defer tenant11b.stop(ctx, t, c)
110109

@@ -159,7 +158,7 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
159158
withResults([][]string{{"1", "bar"}}))
160159

161160
t.Status("starting tenant 12 server with older binary")
162-
tenant12 := createTenantNode(ctx, t, c, kvNodes, tenant12ID, tenantNode, tenant12HTTPPort, tenant12SQLPort, tenantStartOpt)
161+
tenant12 := createTenantNode(ctx, t, c, kvNodes, tenant12ID, tenantNode, tenant12HTTPPort, tenant12SQLPort)
163162
tenant12.start(ctx, t, c, predecessorBinary)
164163
defer tenant12.stop(ctx, t, c)
165164

@@ -181,7 +180,7 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
181180
runner.Exec(t, `SELECT crdb_internal.create_tenant($1::INT)`, tenant13ID)
182181

183182
t.Status("starting tenant 13 server with new binary")
184-
tenant13 := createTenantNode(ctx, t, c, kvNodes, tenant13ID, tenantNode, tenant13HTTPPort, tenant13SQLPort, tenantStartOpt)
183+
tenant13 := createTenantNode(ctx, t, c, kvNodes, tenant13ID, tenantNode, tenant13HTTPPort, tenant13SQLPort)
185184
tenant13.start(ctx, t, c, currentBinary)
186185
defer tenant13.stop(ctx, t, c)
187186

@@ -332,7 +331,7 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
332331
runner.Exec(t, `SELECT crdb_internal.create_tenant($1::INT)`, tenant14ID)
333332

334333
t.Status("verifying that the tenant 14 server works and has the proper version")
335-
tenant14 := createTenantNode(ctx, t, c, kvNodes, tenant14ID, tenantNode, tenant14HTTPPort, tenant14SQLPort, tenantStartOpt)
334+
tenant14 := createTenantNode(ctx, t, c, kvNodes, tenant14ID, tenantNode, tenant14HTTPPort, tenant14SQLPort)
336335
tenant14.start(ctx, t, c, currentBinary)
337336
defer tenant14.stop(ctx, t, c)
338337
verifySQL(t, tenant14.pgURL,

‎pkg/cmd/roachtest/tests/multitenant_utils.go‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -53,22 +53,11 @@ type tenantNode struct {
5353
}
5454

5555
type createTenantOptions struct {
56-
// TODO(ssd): This is a hack to work around the currently tangled state of
57-
// cluster management between roachtest and roachprod. createTenantNode
58-
// recreates client certs. Only one copy of the client certs are cached
59-
// locally, so if we want a client to work against multiple tenants in a
60-
// single test, we need to create the certs with all tenants.
61-
otherTenantIDs []int
62-
6356
// Set this to expand the scope of the nodes added to the tenant certs.
6457
certNodes option.NodeListOption
6558
}
6659
type createTenantOpt func(*createTenantOptions)
6760

68-
func createTenantOtherTenantIDs(ids []int) createTenantOpt {
69-
return func(c *createTenantOptions) { c.otherTenantIDs = ids }
70-
}
71-
7261
func createTenantCertNodes(nodes option.NodeListOption) createTenantOpt {
7362
return func(c *createTenantOptions) { c.certNodes = nodes }
7463
}

‎pkg/roachprod/install/cluster_synced.go‎

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1299,7 +1299,9 @@ const (
12991299
// DistributeCerts will generate and distribute certificates to all of the
13001300
// nodes.
13011301
func (c *SyncedCluster) DistributeCerts(ctx context.Context, l *logger.Logger) error {
1302-
if c.checkForCertificates(ctx, l) {
1302+
if found, err := c.checkForCertificates(ctx, l); err != nil {
1303+
return err
1304+
} else if found {
13031305
return nil
13041306
}
13051307

@@ -1375,11 +1377,15 @@ tar cvf %[3]s certs
13751377
func (c *SyncedCluster) DistributeTenantCerts(
13761378
ctx context.Context, l *logger.Logger, hostCluster *SyncedCluster, tenantID int,
13771379
) error {
1378-
if hostCluster.checkForTenantCertificates(ctx, l) {
1380+
if found, err := hostCluster.checkForTenantCertificates(ctx, l); err != nil {
1381+
return err
1382+
} else if found {
13791383
return nil
13801384
}
13811385

1382-
if !hostCluster.checkForCertificates(ctx, l) {
1386+
if found, err := hostCluster.checkForCertificates(ctx, l); err != nil {
1387+
return err
1388+
} else if !found {
13831389
return errors.New("host cluster missing certificate bundle")
13841390
}
13851391

@@ -1489,7 +1495,7 @@ func (c *SyncedCluster) getFileFromFirstNode(
14891495

14901496
// checkForCertificates checks if the cluster already has a certs bundle created
14911497
// on the first node.
1492-
func (c *SyncedCluster) checkForCertificates(ctx context.Context, l *logger.Logger) bool {
1498+
func (c *SyncedCluster) checkForCertificates(ctx context.Context, l *logger.Logger) (bool, error) {
14931499
dir := ""
14941500
if c.IsLocal() {
14951501
dir = c.localVMDir(1)
@@ -1499,7 +1505,9 @@ func (c *SyncedCluster) checkForCertificates(ctx context.Context, l *logger.Logg
14991505

15001506
// checkForTenantCertificates checks if the cluster already has a tenant-certs bundle created
15011507
// on the first node.
1502-
func (c *SyncedCluster) checkForTenantCertificates(ctx context.Context, l *logger.Logger) bool {
1508+
func (c *SyncedCluster) checkForTenantCertificates(
1509+
ctx context.Context, l *logger.Logger,
1510+
) (bool, error) {
15031511
dir := ""
15041512
if c.IsLocal() {
15051513
dir = c.localVMDir(1)
@@ -1509,24 +1517,10 @@ func (c *SyncedCluster) checkForTenantCertificates(ctx context.Context, l *logge
15091517

15101518
func (c *SyncedCluster) fileExistsOnFirstNode(
15111519
ctx context.Context, l *logger.Logger, path string,
1512-
) bool {
1513-
var existsErr error
1514-
display := fmt.Sprintf("%s: checking %s", c.Name, path)
1515-
if err := c.Parallel(ctx, l, 1, func(ctx context.Context, i int) (*RunResultDetails, error) {
1516-
node := c.Nodes[i]
1517-
sess := c.newSession(l, node, `test -e `+path)
1518-
defer sess.Close()
1519-
1520-
out, cmdErr := sess.CombinedOutput(ctx)
1521-
res := newRunResultDetails(node, cmdErr)
1522-
res.CombinedOut = out
1523-
1524-
existsErr = res.Err
1525-
return res, nil
1526-
}, WithDisplay(display)); err != nil {
1527-
return false
1528-
}
1529-
return existsErr == nil
1520+
) (bool, error) {
1521+
l.Printf("%s: checking %s", c.Name, path)
1522+
result, err := c.runCmdOnSingleNode(ctx, l, c.Nodes[0], `$(test -e `+path+`); echo $?`, false, l.Stdout, l.Stderr)
1523+
return result.Stdout == "0", err
15301524
}
15311525

15321526
// createNodeCertArguments returns a list of strings appropriate for use as

0 commit comments

Comments
 (0)