Skip to content

Commit

Permalink
roachtest: always use active node in backup schedule injection
Browse files Browse the repository at this point in the history
Previously, the schedule backup cmd injection on Start() would always run on
the first node on the cluster, but if that node were not available, the cmd
would fail. This patch ensures the injection runs on a node that was just
started.

Fixes #97558

Release note: None
  • Loading branch information
msbutler committed Feb 24, 2023
1 parent f33f404 commit c5ef384
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 5 deletions.
4 changes: 3 additions & 1 deletion pkg/cmd/roachtest/tests/multitenant_upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,9 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
t.Status("upgrading system tenant binary")
c.Stop(ctx, t.L(), option.DefaultStopOpts(), kvNodes)
settings.Binary = currentBinary
c.Start(ctx, t.L(), option.DefaultStartOpts(), settings, kvNodes)
// TODO (msbutler): investigate why the scheduled backup command fails due to a `Is the Server
// running?` error.
c.Start(ctx, t.L(), option.DefaultStartOptsNoBackups(), settings, kvNodes)
time.Sleep(time.Second)

t.Status("checking the pre-upgrade sql server still works after the system tenant binary upgrade")
Expand Down
19 changes: 15 additions & 4 deletions pkg/roachprod/install/cockroach.go
Original file line number Diff line number Diff line change
Expand Up @@ -826,15 +826,26 @@ func (c *SyncedCluster) createFixedBackupSchedule(
createScheduleCmd := fmt.Sprintf(`CREATE SCHEDULE IF NOT EXISTS test_only_backup FOR BACKUP INTO '%s' %s`,
collectionPath, scheduleArgs)

node := Node(1)
node := c.Nodes[0]
binary := cockroachNodeBinary(c, node)
url := c.NodeURL("localhost", c.NodePort(node), "" /* tenantName */)
fullCmd := fmt.Sprintf(`COCKROACH_CONNECT_TIMEOUT=0 %s sql --url %s -e %q`,
binary, url, createScheduleCmd)
// Instead of using `c.ExecSQL()`, use the more flexible c.Run(), which allows us to
// Instead of using `c.ExecSQL()`, use the more flexible c.newSession(), which allows us to
// 1) prefix the schedule backup cmd with COCKROACH_CONNECT_TIMEOUT=0.
// 2) run the command against the first node on the cluster.
return c.Run(ctx, l, l.Stdout, l.Stderr, Nodes{node}, "init scheduled backup", fullCmd)
// 2) run the command against the first node in the cluster target.
sess := c.newSession(l, node, fullCmd, withDebugName("init-backup-schedule"))
defer sess.Close()

out, err := sess.CombinedOutput(ctx)
if err != nil {
return errors.Wrapf(err, "~ %s\n%s", fullCmd, out)
}

if out := strings.TrimSpace(string(out)); out != "" {
l.Printf(out)
}
return nil
}

// getEnvVars returns all COCKROACH_* environment variables, in the form
Expand Down

0 comments on commit c5ef384

Please sign in to comment.