Merge #147922

147922: roachtest: set pausepoint before backups in OR recovery roachtest r=msbutler a=kev-cao

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

Co-authored-by: Kevin Cao <[email protected]>

Author: craig[bot]

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():