From 21e229d470252808d6fc17b7f18fa11439bbf07e Mon Sep 17 00:00:00 2001 From: Renato Costa Date: Sun, 26 May 2024 20:00:44 +0800 Subject: [PATCH] roachprod: fix check for errors in `find-ports` When finding ports, we only check for the `err` return value from `runCmdOnSingleNode`. However, that function only returns an error if there is a string expansion error. For command errors, the caller needs to check `res.Err`, which was not happening before. Informs: #124715 --- pkg/roachprod/install/cluster_synced.go | 8 +++++++- pkg/roachprod/install/services.go | 4 ++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/pkg/roachprod/install/cluster_synced.go b/pkg/roachprod/install/cluster_synced.go index 9d99080f992..651272af28b 100644 --- a/pkg/roachprod/install/cluster_synced.go +++ b/pkg/roachprod/install/cluster_synced.go @@ -1146,14 +1146,20 @@ func defaultCmdOpts(debugName string) RunCmdOptions { // - specifying the stdin, stdout, and stderr streams // - specifying the remote session options // - whether the command should be run with the ROACHPROD env variable (true for all user commands) +// +// NOTE: do *not* return a `nil` `*RunResultDetails` in this function: +// we want to support callers being able to use +// `errors.CombineErrors(err, res.Err)` when they don't care about the +// origin of the error. func (c *SyncedCluster) runCmdOnSingleNode( ctx context.Context, l *logger.Logger, node Node, cmd string, opts RunCmdOptions, ) (*RunResultDetails, error) { + var noResult RunResultDetails // Argument template expansion is node specific (e.g. for {store-dir}). e := expander{node: node} expandedCmd, err := e.expand(ctx, l, c, cmd) if err != nil { - return nil, errors.WithDetailf(err, "error expanding command: %s", cmd) + return &noResult, errors.WithDetailf(err, "error expanding command: %s", cmd) } nodeCmd := expandedCmd diff --git a/pkg/roachprod/install/services.go b/pkg/roachprod/install/services.go index daaf56724e6..e0889a90e1f 100644 --- a/pkg/roachprod/install/services.go +++ b/pkg/roachprod/install/services.go @@ -425,8 +425,8 @@ func (c *SyncedCluster) FindOpenPorts( } res, err := c.runCmdOnSingleNode(ctx, l, node, buf.String(), defaultCmdOpts("find-ports")) - if err != nil { - return nil, transientFailure(errors.Wrapf(err, "output:\n%s", res.CombinedOut)) + if findPortsErr := errors.CombineErrors(err, res.Err); findPortsErr != nil { + return nil, transientFailure(errors.Wrapf(findPortsErr, "output:\n%s", res.CombinedOut)) } ports, err = stringToIntegers(strings.TrimSpace(res.CombinedOut)) if err != nil {