Skip to content

Commit

Permalink
roachprod: fix confusing start-up error
Browse files Browse the repository at this point in the history
Starting a 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
```

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.
  • Loading branch information
herkolategan committed Jun 13, 2023
1 parent 6693bef commit f0da5e8
Showing 1 changed file with 17 additions and 23 deletions.
40 changes: 17 additions & 23 deletions pkg/roachprod/install/cluster_synced.go
Original file line number Diff line number Diff line change
Expand Up @@ -1299,7 +1299,9 @@ const (
// DistributeCerts will generate and distribute certificates to all of the
// nodes.
func (c *SyncedCluster) DistributeCerts(ctx context.Context, l *logger.Logger) error {
if c.checkForCertificates(ctx, l) {
if found, err := c.checkForCertificates(ctx, l); err != nil {
return err
} else if found {
return nil
}

Expand Down Expand Up @@ -1375,11 +1377,15 @@ tar cvf %[3]s certs
func (c *SyncedCluster) DistributeTenantCerts(
ctx context.Context, l *logger.Logger, hostCluster *SyncedCluster, tenantID int,
) error {
if hostCluster.checkForTenantCertificates(ctx, l) {
if found, err := hostCluster.checkForTenantCertificates(ctx, l); err != nil {
return err
} else if found {
return nil
}

if !hostCluster.checkForCertificates(ctx, l) {
if found, err := hostCluster.checkForCertificates(ctx, l); err != nil {
return err
} else if !found {
return errors.New("host cluster missing certificate bundle")
}

Expand Down Expand Up @@ -1489,7 +1495,7 @@ func (c *SyncedCluster) getFileFromFirstNode(

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

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

func (c *SyncedCluster) fileExistsOnFirstNode(
ctx context.Context, l *logger.Logger, path string,
) bool {
var existsErr error
display := fmt.Sprintf("%s: checking %s", c.Name, path)
if err := c.Parallel(ctx, l, 1, func(ctx context.Context, i int) (*RunResultDetails, error) {
node := c.Nodes[i]
sess := c.newSession(l, node, `test -e `+path)
defer sess.Close()

out, cmdErr := sess.CombinedOutput(ctx)
res := newRunResultDetails(node, cmdErr)
res.CombinedOut = out

existsErr = res.Err
return res, nil
}, WithDisplay(display)); err != nil {
return false
}
return existsErr == nil
) (bool, error) {
l.Printf("%s: checking %s", c.Name, path)
result, err := c.runCmdOnSingleNode(ctx, l, c.Nodes[0], `$(test -e `+path+`); echo $?`, false, l.Stdout, l.Stderr)
return result.Stdout == "0", err
}

// createNodeCertArguments returns a list of strings appropriate for use as
Expand Down

0 comments on commit f0da5e8

Please sign in to comment.