roachtest: set pausepoint before backups in OR recovery roachtest

The OR recovery roachtests were encountering intermittent failures where
the download job was found in a succeeded state when the test was
waiting for it to pause. It was determined this was occurring whenever a
full cluster backup/restore was performed. Because the pausepoint was
set _after_ the backups were taken, this meant that the link phase of OR
in a full cluster restore would wipe the pausepoints before the download
phase. This patch moves the pausepoint setting so that it is set before
the backups and therefore will be preserved after a full cluster OR.

Fixes: #147557

Release note: None

Author: kev-cao

pkg/cmd/roachtest/tests/backup_restore_roundtrip.go

@@ -478,6 +478,20 @@ func testOnlineRestoreRecovery(ctx context.Context, t test.Test, c cluster.Clust
 			Execute: allNodes,
 		}
 
+		// We set this pausepoint so that the download job is paused before it
+		// begins downloading external files. This allows us to guarantee that when
+		// we delete an SST file, it won't have already been downloaded by the
+		// download phase and therefore not trigger a failure.
+		// We also must set this BEFORE taking backups. In the event of a full
+		// cluster backup, the link phase of OR will reset the pausepoints back to
+		// the point of the backup, which means this pausepoint will be wiped if it
+		// is set after the backups.
+		if _, err := dbConn.ExecContext(
+			ctx, "SET CLUSTER SETTING jobs.debug.pausepoints = 'restore.before_download'",
+		); err != nil {
+			return errors.Wrap(err, "failed to set pausepoint for online restore")
+		}
+
 		t.L().Printf("starting backup")
 		collection, err := d.createBackupCollection(
 			ctx, t.L(), t, testRNG, bspec, bspec, "online-restore-recovery-backup",
@@ -509,16 +523,6 @@ func testOnlineRestoreRecovery(ctx context.Context, t test.Test, c cluster.Clust
 			}
 		}
 
-		// We set this pausepoint so that the download job is paused before it
-		// begins downloading external files. This allows us to guarantee that when
-		// we delete an SST file, it won't have already been downloaded by the
-		// download phase and therefore not trigger a failure.
-		if _, err := dbConn.ExecContext(
-			ctx, "SET CLUSTER SETTING jobs.debug.pausepoints = 'restore.before_download'",
-		); err != nil {
-			return errors.Wrap(err, "failed to set pausepoint for online restore")
-		}
-
 		t.L().Printf("performing online restore of backup")
 		if _, _, err := d.runRestore(
 			ctx, t.L(), testRNG, collection, false /* checkFiles */, true, /* internalSystemJobs */
@@ -541,7 +545,6 @@ func testOnlineRestoreRecovery(ctx context.Context, t test.Test, c cluster.Clust
 			return errors.Wrap(err, "failed to remove pausepoint for online restore")
 		}
 
-		t.L().Printf("deleting sst from backup")
 		if err := d.deleteRandomSST(ctx, t.L(), dbConn, collection); err != nil {
 			return err
 		}
@@ -555,10 +558,7 @@ func testOnlineRestoreRecovery(ctx context.Context, t test.Test, c cluster.Clust
 				err, "waiting for download job %v to reach resumed state", downloadJobID,
 			)
 		}
-		if err := errors.Wrapf(
-			WaitForFailed(ctx, dbConn, jobspb.JobID(downloadJobID), jobStatusWait),
-			"waiting for download job %v to reach failed state", downloadJobID,
-		); err != nil {
+		if err := WaitForFailed(ctx, dbConn, jobspb.JobID(downloadJobID), jobStatusWait); err != nil {
 			return errors.Wrapf(
 				err, "waiting for download job %v to reach failed state", downloadJobID,
 			)

pkg/cmd/roachtest/tests/jobs_util.go

@@ -198,7 +198,7 @@ func WaitForState(
 				return nil
 			}
 			if timeutil.Since(startTime) > maxWait {
-				return errors.Newf("job %d did not reach state %s after %s", jobID, state, maxWait)
+				return errors.Newf("job %d still in state %s after %s", jobID, state, maxWait)
 			}
 			ticker.Reset(5 * time.Second)
 		case <-ctx.Done():