From 291fcb9e4f7086237c409580b1fd3accf46d5ba3 Mon Sep 17 00:00:00 2001 From: John Spray Date: Mon, 20 May 2024 09:25:25 +0100 Subject: [PATCH] pageserver: use the heatmap upload interval to set the secondary download interval (#7793) ## Problem The heatmap upload period is configurable, but secondary mode downloads were using a fixed download period. Closes: #6200 ## Summary of changes - Use the upload period in the heatmap to adjust the download period. In practice, this will reduce the frequency of downloads from its current 60 second period to what heatmaps use, which is 5-10m depending on environment. This is an improvement rather than being optimal: we could be smarter about periods, and schedule downloads to occur around the time we expect the next upload, rather than just using the same period, but that's something we can address in future if it comes up. --- pageserver/src/tenant/secondary/downloader.rs | 84 ++++++++++--------- .../regress/test_pageserver_secondary.py | 18 ++-- 2 files changed, 58 insertions(+), 44 deletions(-) diff --git a/pageserver/src/tenant/secondary/downloader.rs b/pageserver/src/tenant/secondary/downloader.rs index 8f27220771..609e1431cf 100644 --- a/pageserver/src/tenant/secondary/downloader.rs +++ b/pageserver/src/tenant/secondary/downloader.rs @@ -62,14 +62,10 @@ use super::{ CommandRequest, DownloadCommand, }; -/// For each tenant, how long must have passed since the last download_tenant call before -/// calling it again. This is approximately the time by which local data is allowed -/// to fall behind remote data. -/// -/// TODO: this should just be a default, and the actual period should be controlled -/// via the heatmap itself -/// `` -const DOWNLOAD_FRESHEN_INTERVAL: Duration = Duration::from_millis(60000); +/// For each tenant, default period for how long must have passed since the last download_tenant call before +/// calling it again. This default is replaced with the value of [`HeatMapTenant::upload_period_ms`] after first +/// download, if the uploader populated it. +const DEFAULT_DOWNLOAD_INTERVAL: Duration = Duration::from_millis(60000); /// Range of concurrency we may use when downloading layers within a timeline. This is independent /// for each tenant we're downloading: the concurrency of _tenants_ is defined separately in @@ -152,14 +148,22 @@ pub(super) struct SecondaryDetailTimeline { pub(super) evicted_at: HashMap, } +// Aspects of a heatmap that we remember after downloading it +#[derive(Clone, Debug)] +struct DownloadSummary { + etag: Etag, + #[allow(unused)] + mtime: SystemTime, + upload_period: Duration, +} + /// This state is written by the secondary downloader, it is opaque /// to TenantManager #[derive(Debug)] pub(super) struct SecondaryDetail { pub(super) config: SecondaryLocationConfig, - last_download: Option, - last_etag: Option, + last_download: Option, next_download: Option, pub(super) timelines: HashMap, } @@ -189,7 +193,6 @@ impl SecondaryDetail { Self { config, last_download: None, - last_etag: None, next_download: None, timelines: HashMap::new(), } @@ -243,9 +246,8 @@ impl SecondaryDetail { struct PendingDownload { secondary_state: Arc, - last_download: Option, + last_download: Option, target_time: Option, - period: Option, } impl scheduler::PendingJob for PendingDownload { @@ -295,10 +297,17 @@ impl JobGenerator SchedulingResult { @@ -331,11 +340,11 @@ impl JobGenerator next_download { @@ -343,7 +352,6 @@ impl JobGenerator TenantDownloader<'a> { let tenant_shard_id = self.secondary_state.get_tenant_shard_id(); // We will use the etag from last successful download to make the download conditional on changes - let last_etag = self + let last_download = self .secondary_state .detail .lock() .unwrap() - .last_etag + .last_download .clone(); // Download the tenant's heatmap @@ -539,7 +540,7 @@ impl<'a> TenantDownloader<'a> { etag: heatmap_etag, bytes: heatmap_bytes, } = match tokio::select!( - bytes = self.download_heatmap(last_etag.as_ref()) => {bytes?}, + bytes = self.download_heatmap(last_download.as_ref().map(|d| &d.etag)) => {bytes?}, _ = self.secondary_state.cancel.cancelled() => return Ok(()) ) { HeatMapDownload::Unmodified => { @@ -599,7 +600,14 @@ impl<'a> TenantDownloader<'a> { // Only update last_etag after a full successful download: this way will not skip // the next download, even if the heatmap's actual etag is unchanged. - self.secondary_state.detail.lock().unwrap().last_etag = Some(heatmap_etag); + self.secondary_state.detail.lock().unwrap().last_download = Some(DownloadSummary { + etag: heatmap_etag, + mtime: heatmap_mtime, + upload_period: heatmap + .upload_period_ms + .map(|ms| Duration::from_millis(ms as u64)) + .unwrap_or(DEFAULT_DOWNLOAD_INTERVAL), + }); Ok(()) } diff --git a/test_runner/regress/test_pageserver_secondary.py b/test_runner/regress/test_pageserver_secondary.py index fdc09a063d..127340a1e7 100644 --- a/test_runner/regress/test_pageserver_secondary.py +++ b/test_runner/regress/test_pageserver_secondary.py @@ -575,7 +575,10 @@ def test_secondary_background_downloads(neon_env_builder: NeonEnvBuilder): tenant_timelines = {} # This mirrors a constant in `downloader.rs` - freshen_interval_secs = 60 + default_download_period_secs = 60 + + # The upload period, which will also be the download once the secondary has seen its first heatmap + upload_period_secs = 20 for _i in range(0, tenant_count): tenant_id = TenantId.generate() @@ -587,7 +590,7 @@ def test_secondary_background_downloads(neon_env_builder: NeonEnvBuilder): placement_policy='{"Attached":1}', # Run with a low heatmap period so that we can avoid having to do synthetic API calls # to trigger the upload promptly. - conf={"heatmap_period": "1s"}, + conf={"heatmap_period": f"{upload_period_secs}s"}, ) env.neon_cli.create_timeline("main2", tenant_id, timeline_b) @@ -597,7 +600,7 @@ def test_secondary_background_downloads(neon_env_builder: NeonEnvBuilder): # Wait long enough that the background downloads should happen; we expect all the inital layers # of all the initial timelines to show up on the secondary location of each tenant. - time.sleep(freshen_interval_secs * 1.5) + time.sleep(default_download_period_secs * 1.5) for tenant_id, timelines in tenant_timelines.items(): attached_to_id = env.storage_controller.locate(tenant_id)[0]["node_id"] @@ -613,8 +616,8 @@ def test_secondary_background_downloads(neon_env_builder: NeonEnvBuilder): # Delete the second timeline: this should be reflected later on the secondary env.storage_controller.pageserver_api().timeline_delete(tenant_id, timelines[1]) - # Wait long enough for the secondary locations to see the deletion - time.sleep(freshen_interval_secs * 1.5) + # Wait long enough for the secondary locations to see the deletion: 2x period plus a grace factor + time.sleep(upload_period_secs * 2.5) for tenant_id, timelines in tenant_timelines.items(): attached_to_id = env.storage_controller.locate(tenant_id)[0]["node_id"] @@ -626,6 +629,9 @@ def test_secondary_background_downloads(neon_env_builder: NeonEnvBuilder): assert ps_secondary.list_layers(tenant_id, timelines[0]) # This one was deleted + log.info( + f"Checking for secondary timeline deletion {tenant_id}/{timeline_id} on node {ps_secondary.id}" + ) assert not ps_secondary.list_layers(tenant_id, timelines[1]) t_end = time.time() @@ -640,7 +646,7 @@ def test_secondary_background_downloads(neon_env_builder: NeonEnvBuilder): download_rate = (total_heatmap_downloads / tenant_count) / (t_end - t_start) - expect_download_rate = 1.0 / freshen_interval_secs + expect_download_rate = 1.0 / upload_period_secs log.info(f"Download rate: {download_rate * 60}/min vs expected {expect_download_rate * 60}/min") assert download_rate < expect_download_rate * 2