Merge #149571 #149615

149571: roachtest: Skip flaky endpoint and fix retry logic for db-console/endpoints r=alyshanjahani-crl a=alyshanjahani-crl

This commit skips the rangelog endpoint on the admin server. This failure mode is described in:
#148112 (comment)

This commit also sets an initial backoff when retrying endpoints. The default of 50ms resulted in the 10 retries being exhausted too quickly. In mixed version tests nodes are taken down and brought up, and so we observed many 404 failures due to this.

Fixes: #148187
Release note: None

149615: backup: move retry loop to entire download phase r=msbutler a=kev-cao

The retry loop introduced in #148821 specifically would retry the `DownloadSpan` requests. Moving the retry loop to handle the entire download phase would also allow us to retry the cleanup step as well.

Epic: CRDB-51394

Release note: None

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

Author: 3 people

pkg/backup/restore_job.go

@@ -1793,7 +1793,7 @@ func (r *restoreResumer) doResume(ctx context.Context, execCtx interface{}) erro
 		if err := p.ExecCfg().JobRegistry.CheckPausepoint("restore.before_do_download_files"); err != nil {
 			return err
 		}
-		return r.doDownloadFiles(ctx, p)
+		return r.doDownloadFilesWithRetry(ctx, p)
 	}
 
 	if err := p.ExecCfg().JobRegistry.CheckPausepoint("restore.before_load_descriptors_from_backup"); err != nil {
@@ -2026,7 +2026,7 @@ func (r *restoreResumer) doResume(ctx context.Context, execCtx interface{}) erro
 		// TODO(msbutler): ideally doDownloadFiles would not depend on job details
 		// and is instead passed an execCfg and the download spans and anything else
 		// it needs. If that occured, we would not need to update details above.
-		if err := r.doDownloadFiles(ctx, p); err != nil {
+		if err := r.doDownloadFilesWithRetry(ctx, p); err != nil {
 			return err
 		}
 	}

pkg/backup/restore_online.go

@@ -563,28 +563,14 @@ func (r *restoreResumer) sendDownloadWorker(
 				return err
 			}
 
-			var err error
-			for r := retry.StartWithCtx(ctx, retry.Options{
-				InitialBackoff: time.Millisecond * 100,
-				MaxBackoff:     time.Second,
-				MaxRetries:     maxDownloadAttempts - 1,
-			}); r.Next(); {
-				err = func() error {
-					if testingKnobs != nil && testingKnobs.RunBeforeSendingDownloadSpan != nil {
-						if err := testingKnobs.RunBeforeSendingDownloadSpan(); err != nil {
-							return err
-						}
-					}
-					return sendDownloadSpan(ctx, execCtx, spans)
-				}()
-				if err == nil {
-					break
+			if testingKnobs != nil && testingKnobs.RunBeforeSendingDownloadSpan != nil {
+				if err := testingKnobs.RunBeforeSendingDownloadSpan(); err != nil {
+					return err
 				}
-				log.VInfof(ctx, 1, "attempt %d failed to download spans: %v", r.CurrentAttempt(), err)
 			}
 
-			if err != nil {
-				return errors.Wrapf(err, "retries exhausted for sending download spans")
+			if err := sendDownloadSpan(ctx, execCtx, spans); err != nil {
+				return err
 			}
 
 			// Wait for the completion poller to signal that it has checked our work.
@@ -818,6 +804,24 @@ func getRemainingExternalFileBytes(
 	return remaining, nil
 }
 
+func (r *restoreResumer) doDownloadFilesWithRetry(
+	ctx context.Context, execCtx sql.JobExecContext,
+) error {
+	var err error
+	for rt := retry.StartWithCtx(ctx, retry.Options{
+		InitialBackoff: time.Millisecond * 100,
+		MaxBackoff:     time.Second,
+		MaxRetries:     maxDownloadAttempts - 1,
+	}); rt.Next(); {
+		err = r.doDownloadFiles(ctx, execCtx)
+		if err == nil {
+			return nil
+		}
+		log.Warningf(ctx, "failed attempt #%d to download files: %v", rt.CurrentAttempt(), err)
+	}
+	return errors.Wrapf(err, "retries exhausted for downloading files")
+}
+
 func (r *restoreResumer) doDownloadFiles(ctx context.Context, execCtx sql.JobExecContext) error {
 	details := r.job.Details().(jobspb.RestoreDetails)
 

pkg/cmd/roachtest/tests/db-console/admin_endpoints.json

@@ -78,6 +78,7 @@
     {
       "url": "/_admin/v1/rangelog",
       "method": "GET"
+      "skip": "https://github.com/cockroachdb/cockroach/pull/148112#issuecomment-2960322577"
     },
     {
       "url": "/_admin/v1/rangelog/{range_id}",

pkg/cmd/roachtest/tests/db_console_endpoints.go

@@ -248,7 +248,7 @@ func testEndpoint(
 		return nil
 	}
 
-	return withRetries(ctx, retry.Options{MaxRetries: 10}, f)
+	return withRetries(ctx, retry.Options{InitialBackoff: time.Second, MaxRetries: 10}, f)
 }
 
 // withRetries runs the given function f with the provided retry options.