From c9cbdf0bf7e5c8680061dbdd82110195b03f1cf3 Mon Sep 17 00:00:00 2001 From: John Spray Date: Wed, 18 Oct 2023 14:16:54 +0100 Subject: [PATCH] pageserver: retry forever for remote timeline listing There is no point giving up: if S3 is unavailable, we should keep trying until it becomes available. --- libs/remote_storage/src/lib.rs | 4 ++ pageserver/src/tenant.rs | 39 ++++++++++++++----- .../tenant/remote_timeline_client/download.rs | 24 +++++++++++- 3 files changed, 57 insertions(+), 10 deletions(-) diff --git a/libs/remote_storage/src/lib.rs b/libs/remote_storage/src/lib.rs index e7ff6dda21..bef5157d61 100644 --- a/libs/remote_storage/src/lib.rs +++ b/libs/remote_storage/src/lib.rs @@ -240,6 +240,9 @@ pub enum DownloadError { BadInput(anyhow::Error), /// The file was not found in the remote storage. NotFound, + /// A cancellation token aborted the download, typically during + /// tenant detach or process shutdown. + Cancelled, /// The file was found in the remote storage, but the download failed. Other(anyhow::Error), } @@ -250,6 +253,7 @@ impl std::fmt::Display for DownloadError { DownloadError::BadInput(e) => { write!(f, "Failed to download a remote file due to user input: {e}") } + DownloadError::Cancelled => write!(f, "Cancelled, shutting down"), DownloadError::NotFound => write!(f, "No file found for the remote object id given"), DownloadError::Other(e) => write!(f, "Failed to download a remote file: {e:?}"), } diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 2caa60aa0b..368e7eb413 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -661,7 +661,7 @@ impl Tenant { let preload = match &remote_storage { Some(remote_storage) => Some( match tenant_clone - .preload(remote_storage) + .preload(remote_storage, task_mgr::shutdown_token()) .instrument( tracing::info_span!(parent: None, "attach_preload", tenant_id=%tenant_id), ) @@ -754,12 +754,17 @@ impl Tenant { pub(crate) async fn preload( self: &Arc, remote_storage: &GenericRemoteStorage, + cancel: CancellationToken, ) -> anyhow::Result { // Get list of remote timelines // download index files for every tenant timeline info!("listing remote timelines"); - let (remote_timeline_ids, other_keys) = - remote_timeline_client::list_remote_timelines(remote_storage, self.tenant_id).await?; + let (remote_timeline_ids, other_keys) = remote_timeline_client::list_remote_timelines( + remote_storage, + self.tenant_id, + cancel.clone(), + ) + .await?; info!("found {} timelines", remote_timeline_ids.len()); let deleting = other_keys.contains(TENANT_DELETED_MARKER_FILE_NAME); @@ -770,7 +775,7 @@ impl Tenant { Ok(TenantPreload { deleting, timelines: self - .load_timeline_metadata(remote_timeline_ids, remote_storage) + .load_timeline_metadata(remote_timeline_ids, remote_storage, cancel) .await?, }) } @@ -1199,6 +1204,7 @@ impl Tenant { self: &Arc, timeline_ids: HashSet, remote_storage: &GenericRemoteStorage, + cancel: CancellationToken, ) -> anyhow::Result> { let mut part_downloads = JoinSet::new(); for timeline_id in timeline_ids { @@ -1232,10 +1238,25 @@ impl Tenant { } let mut timeline_preloads: HashMap = HashMap::new(); - while let Some(result) = part_downloads.join_next().await { - let preload_result = result.context("join preload task")?; - let preload = preload_result?; - timeline_preloads.insert(preload.timeline_id, preload); + + loop { + tokio::select!( + next = part_downloads.join_next() => { + match next { + Some(result) => { + let preload_result = result.context("join preload task")?; + let preload = preload_result?; + timeline_preloads.insert(preload.timeline_id, preload); + }, + None => { + break; + } + } + }, + _ = cancel.cancelled() => { + anyhow::bail!("Cancelled while waiting for remote index download") + } + ) } Ok(timeline_preloads) @@ -3605,7 +3626,7 @@ pub(crate) mod harness { } LoadMode::Remote => { let preload = tenant - .preload(&self.remote_storage) + .preload(&self.remote_storage, CancellationToken::new()) .instrument(info_span!("try_load_preload", tenant_id=%self.tenant_id)) .await?; tenant diff --git a/pageserver/src/tenant/remote_timeline_client/download.rs b/pageserver/src/tenant/remote_timeline_client/download.rs index 473547d5fa..46b664ac97 100644 --- a/pageserver/src/tenant/remote_timeline_client/download.rs +++ b/pageserver/src/tenant/remote_timeline_client/download.rs @@ -170,6 +170,7 @@ pub fn is_temp_download_file(path: &Utf8Path) -> bool { pub async fn list_remote_timelines( storage: &GenericRemoteStorage, tenant_id: TenantId, + cancel: CancellationToken, ) -> anyhow::Result<(HashSet, HashSet)> { let remote_path = remote_timelines_path(&tenant_id); @@ -177,9 +178,10 @@ pub async fn list_remote_timelines( anyhow::bail!("storage-sync-list-remote-timelines"); }); - let listing = download_retry( + let listing = download_retry_forever( || storage.list(Some(&remote_path), ListingMode::WithDelimiter), &format!("list timelines for {tenant_id}"), + cancel, ) .await?; @@ -373,3 +375,23 @@ where ) .await } + +async fn download_retry_forever( + op: O, + description: &str, + cancel: CancellationToken, +) -> Result +where + O: FnMut() -> F, + F: Future>, +{ + backoff::retry( + op, + |e| matches!(e, DownloadError::BadInput(_) | DownloadError::NotFound), + FAILED_DOWNLOAD_WARN_THRESHOLD, + u32::MAX, + description, + backoff::Cancel::new(cancel, || DownloadError::Cancelled), + ) + .await +}