From 7e4dded2c8b02697d03cf22e6fcc8e537c8f79a7 Mon Sep 17 00:00:00 2001 From: Yuchen Liang Date: Tue, 17 Sep 2024 09:53:28 -0400 Subject: [PATCH 01/16] pageserver: wait for lsn lease duration after transition into AttachedSingle Signed-off-by: Yuchen Liang --- pageserver/src/tenant/gc_block.rs | 23 +++++++++++++++++++++-- pageserver/src/tenant/mgr.rs | 13 +++++++++++++ pageserver/src/tenant/tasks.rs | 17 +---------------- 3 files changed, 35 insertions(+), 18 deletions(-) diff --git a/pageserver/src/tenant/gc_block.rs b/pageserver/src/tenant/gc_block.rs index 8b41ba174669..e9867f25dbf4 100644 --- a/pageserver/src/tenant/gc_block.rs +++ b/pageserver/src/tenant/gc_block.rs @@ -1,8 +1,9 @@ -use std::collections::HashMap; +use std::{collections::HashMap, time::Duration}; +use tokio_util::sync::CancellationToken; use utils::id::TimelineId; -use super::remote_timeline_client::index::GcBlockingReason; +use super::{remote_timeline_client::index::GcBlockingReason, tasks::Cancelled}; type Storage = HashMap>; @@ -42,6 +43,24 @@ impl GcBlock { } } + /// Blocks GC until `duration` has elapsed. + /// + /// We do this as the leases mapping are not persisted to disk. By delaying GC by default + /// length, we gurantees that all the leases we granted before will expire when we run GC for + /// the first time after restart / transition from AttachedMulti to AttachedSingle. + pub(super) async fn block_for( + &self, + duration: Duration, + cancel: &CancellationToken, + ) -> Result<(), Cancelled> { + // hold this lock so gc_iteration cannot proceed. + let _g = self.blocking.lock().await; + match tokio::time::timeout(duration, cancel.cancelled()).await { + Ok(_) => Err(Cancelled), + Err(_) => Ok(()), + } + } + pub(crate) fn summary(&self) -> Option { let g = self.reasons.lock().unwrap(); diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs index 2104f415319e..646e50a29f5d 100644 --- a/pageserver/src/tenant/mgr.rs +++ b/pageserver/src/tenant/mgr.rs @@ -949,6 +949,19 @@ impl TenantManager { (LocationMode::Attached(attach_conf), Some(TenantSlot::Attached(tenant))) => { match attach_conf.generation.cmp(&tenant.generation) { Ordering::Equal => { + if attach_conf.attach_mode == AttachmentMode::Single { + // We need to wait for LSN lease duration after transitioning into `AttachedSingle` + // before doing any gc so that the client has time to renew the lease. + let tenant = tenant.clone(); + tokio::spawn(async move { + let lsn_lease_length = tenant.get_lsn_lease_length(); + let _ = tenant + .gc_block + .block_for(lsn_lease_length, &tenant.cancel) + .await; + }); + } + // A transition from Attached to Attached in the same generation, we may // take our fast path and just provide the updated configuration // to the tenant. diff --git a/pageserver/src/tenant/tasks.rs b/pageserver/src/tenant/tasks.rs index 478e9bb4f074..58b678d99d7c 100644 --- a/pageserver/src/tenant/tasks.rs +++ b/pageserver/src/tenant/tasks.rs @@ -363,7 +363,7 @@ async fn gc_loop(tenant: Arc, cancel: CancellationToken) { first = false; let delays = async { - delay_by_lease_length(tenant.get_lsn_lease_length(), &cancel).await?; + tenant.gc_block.block_for(tenant.get_lsn_lease_length(), &cancel).await?; random_init_delay(period, &cancel).await?; Ok::<_, Cancelled>(()) }; @@ -545,21 +545,6 @@ pub(crate) async fn random_init_delay( } } -/// Delays GC by defaul lease length at restart. -/// -/// We do this as the leases mapping are not persisted to disk. By delaying GC by default -/// length, we gurantees that all the leases we granted before the restart will expire -/// when we run GC for the first time after the restart. -pub(crate) async fn delay_by_lease_length( - length: Duration, - cancel: &CancellationToken, -) -> Result<(), Cancelled> { - match tokio::time::timeout(length, cancel.cancelled()).await { - Ok(_) => Err(Cancelled), - Err(_) => Ok(()), - } -} - struct Iteration { started_at: Instant, period: Duration, From 7743144ad3683105b14e11d0bc5406cb91151bf9 Mon Sep 17 00:00:00 2001 From: Yuchen Liang Date: Tue, 17 Sep 2024 10:25:14 -0400 Subject: [PATCH 02/16] update comments Signed-off-by: Yuchen Liang --- pageserver/src/tenant/gc_block.rs | 2 +- pageserver/src/tenant/mgr.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pageserver/src/tenant/gc_block.rs b/pageserver/src/tenant/gc_block.rs index e9867f25dbf4..e04383859a5e 100644 --- a/pageserver/src/tenant/gc_block.rs +++ b/pageserver/src/tenant/gc_block.rs @@ -46,7 +46,7 @@ impl GcBlock { /// Blocks GC until `duration` has elapsed. /// /// We do this as the leases mapping are not persisted to disk. By delaying GC by default - /// length, we gurantees that all the leases we granted before will expire when we run GC for + /// length, we guarantee that all the leases we granted before will expire when we run GC for /// the first time after restart / transition from AttachedMulti to AttachedSingle. pub(super) async fn block_for( &self, diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs index 646e50a29f5d..d75f0f171330 100644 --- a/pageserver/src/tenant/mgr.rs +++ b/pageserver/src/tenant/mgr.rs @@ -951,7 +951,7 @@ impl TenantManager { Ordering::Equal => { if attach_conf.attach_mode == AttachmentMode::Single { // We need to wait for LSN lease duration after transitioning into `AttachedSingle` - // before doing any gc so that the client has time to renew the lease. + // before doing any gc so that all previously granted lease will expire. let tenant = tenant.clone(); tokio::spawn(async move { let lsn_lease_length = tenant.get_lsn_lease_length(); From 9c16c74348f4acea2ede6f212dbecf5929e7bfe2 Mon Sep 17 00:00:00 2001 From: Yuchen Liang Date: Tue, 17 Sep 2024 12:28:08 -0400 Subject: [PATCH 03/16] refactor GCBlock reason storage Signed-off-by: Yuchen Liang --- pageserver/src/tenant/gc_block.rs | 36 +++++++++++++++++++------------ 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/pageserver/src/tenant/gc_block.rs b/pageserver/src/tenant/gc_block.rs index e04383859a5e..2aa615e52339 100644 --- a/pageserver/src/tenant/gc_block.rs +++ b/pageserver/src/tenant/gc_block.rs @@ -5,7 +5,13 @@ use utils::id::TimelineId; use super::{remote_timeline_client::index::GcBlockingReason, tasks::Cancelled}; -type Storage = HashMap>; +type TimelinesBlocked = HashMap>; + +#[derive(Default)] +struct Storage { + pub timelines_blocked: TimelinesBlocked, + pub tenant_blocked: bool, +} #[derive(Default)] pub(crate) struct GcBlock { @@ -83,7 +89,7 @@ impl GcBlock { ) -> anyhow::Result { let (added, uploaded) = { let mut g = self.reasons.lock().unwrap(); - let set = g.entry(timeline.timeline_id).or_default(); + let set = g.timelines_blocked.entry(timeline.timeline_id).or_default(); let added = set.insert(reason); // LOCK ORDER: intentionally hold the lock, see self.reasons. @@ -114,7 +120,7 @@ impl GcBlock { let (remaining_blocks, uploaded) = { let mut g = self.reasons.lock().unwrap(); - match g.entry(timeline.timeline_id) { + match g.timelines_blocked.entry(timeline.timeline_id) { Entry::Occupied(mut oe) => { let set = oe.get_mut(); set.remove(reason); @@ -128,7 +134,7 @@ impl GcBlock { } } - let remaining_blocks = g.len(); + let remaining_blocks = g.timelines_blocked.len(); // LOCK ORDER: intentionally hold the lock while scheduling; see self.reasons let uploaded = timeline @@ -153,11 +159,11 @@ impl GcBlock { pub(crate) fn before_delete(&self, timeline: &super::Timeline) { let unblocked = { let mut g = self.reasons.lock().unwrap(); - if g.is_empty() { + if g.timelines_blocked.is_empty() { return; } - g.remove(&timeline.timeline_id); + g.timelines_blocked.remove(&timeline.timeline_id); BlockingReasons::clean_and_summarize(g).is_none() }; @@ -168,10 +174,11 @@ impl GcBlock { } /// Initialize with the non-deleted timelines of this tenant. - pub(crate) fn set_scanned(&self, scanned: Storage) { + pub(crate) fn set_scanned(&self, scanned: TimelinesBlocked) { let mut g = self.reasons.lock().unwrap(); - assert!(g.is_empty()); - g.extend(scanned.into_iter().filter(|(_, v)| !v.is_empty())); + assert!(g.timelines_blocked.is_empty()); + g.timelines_blocked + .extend(scanned.into_iter().filter(|(_, v)| !v.is_empty())); if let Some(reasons) = BlockingReasons::clean_and_summarize(g) { tracing::info!(summary=?reasons, "initialized with gc blocked"); @@ -202,13 +209,13 @@ impl std::fmt::Display for BlockingReasons { impl BlockingReasons { fn clean_and_summarize(mut g: std::sync::MutexGuard<'_, Storage>) -> Option { let mut reasons = enumset::EnumSet::empty(); - g.retain(|_key, value| { + g.timelines_blocked.retain(|_key, value| { reasons = reasons.union(*value); !value.is_empty() }); - if !g.is_empty() { + if !g.timelines_blocked.is_empty() { Some(BlockingReasons { - timelines: g.len(), + timelines: g.timelines_blocked.len(), reasons, }) } else { @@ -217,14 +224,15 @@ impl BlockingReasons { } fn summarize(g: &std::sync::MutexGuard<'_, Storage>) -> Option { - if g.is_empty() { + if g.timelines_blocked.is_empty() { None } else { let reasons = g + .timelines_blocked .values() .fold(enumset::EnumSet::empty(), |acc, next| acc.union(*next)); Some(BlockingReasons { - timelines: g.len(), + timelines: g.timelines_blocked.len(), reasons, }) } From af5e9b5a9744fbb88b97f9a30d63913e0a3ff44e Mon Sep 17 00:00:00 2001 From: Yuchen Liang Date: Tue, 17 Sep 2024 13:07:30 -0400 Subject: [PATCH 04/16] use reasons to block gc Signed-off-by: Yuchen Liang --- pageserver/src/tenant/gc_block.rs | 38 +++++++++++++++++++------------ pageserver/src/tenant/tasks.rs | 19 ++++++++++++---- 2 files changed, 37 insertions(+), 20 deletions(-) diff --git a/pageserver/src/tenant/gc_block.rs b/pageserver/src/tenant/gc_block.rs index 2aa615e52339..cf73ad3cb328 100644 --- a/pageserver/src/tenant/gc_block.rs +++ b/pageserver/src/tenant/gc_block.rs @@ -1,9 +1,13 @@ use std::{collections::HashMap, time::Duration}; +use anyhow::bail; use tokio_util::sync::CancellationToken; use utils::id::TimelineId; -use super::{remote_timeline_client::index::GcBlockingReason, tasks::Cancelled}; +use super::{ + remote_timeline_client::index::GcBlockingReason, + tasks::{self, Cancelled}, +}; type TimelinesBlocked = HashMap>; @@ -54,16 +58,17 @@ impl GcBlock { /// We do this as the leases mapping are not persisted to disk. By delaying GC by default /// length, we guarantee that all the leases we granted before will expire when we run GC for /// the first time after restart / transition from AttachedMulti to AttachedSingle. - pub(super) async fn block_for( - &self, - duration: Duration, - cancel: &CancellationToken, - ) -> Result<(), Cancelled> { - // hold this lock so gc_iteration cannot proceed. - let _g = self.blocking.lock().await; - match tokio::time::timeout(duration, cancel.cancelled()).await { - Ok(_) => Err(Cancelled), - Err(_) => Ok(()), + pub(super) async fn block_for(&self, duration: Duration, cancel: &CancellationToken) { + { + let g = self.reasons.lock().unwrap(); + g.tenant_blocked = true; + } + + let _ = tasks::delay_by_duration(duration, cancel).await; + + { + let g = self.reasons.lock().unwrap(); + g.tenant_blocked = false; } } @@ -192,6 +197,7 @@ pub(super) struct Guard<'a> { #[derive(Debug)] pub(crate) struct BlockingReasons { + tenant_blocked: bool, timelines: usize, reasons: enumset::EnumSet, } @@ -200,8 +206,8 @@ impl std::fmt::Display for BlockingReasons { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!( f, - "{} timelines block for {:?}", - self.timelines, self.reasons + "tenant blocked: {}; {} timelines block for {:?}", + self.tenant_blocked, self.timelines, self.reasons ) } } @@ -213,8 +219,9 @@ impl BlockingReasons { reasons = reasons.union(*value); !value.is_empty() }); - if !g.timelines_blocked.is_empty() { + if !g.timelines_blocked.is_empty() || g.tenant_blocked { Some(BlockingReasons { + tenant_blocked: g.tenant_blocked, timelines: g.timelines_blocked.len(), reasons, }) @@ -224,7 +231,7 @@ impl BlockingReasons { } fn summarize(g: &std::sync::MutexGuard<'_, Storage>) -> Option { - if g.timelines_blocked.is_empty() { + if g.timelines_blocked.is_empty() || !g.tenant_blocked { None } else { let reasons = g @@ -232,6 +239,7 @@ impl BlockingReasons { .values() .fold(enumset::EnumSet::empty(), |acc, next| acc.union(*next)); Some(BlockingReasons { + tenant_blocked: g.tenant_blocked, timelines: g.timelines_blocked.len(), reasons, }) diff --git a/pageserver/src/tenant/tasks.rs b/pageserver/src/tenant/tasks.rs index 58b678d99d7c..fc21fb7af14a 100644 --- a/pageserver/src/tenant/tasks.rs +++ b/pageserver/src/tenant/tasks.rs @@ -363,7 +363,8 @@ async fn gc_loop(tenant: Arc, cancel: CancellationToken) { first = false; let delays = async { - tenant.gc_block.block_for(tenant.get_lsn_lease_length(), &cancel).await?; + let lsn_lease_length = tenant.get_lsn_lease_length(); + delay_by_duration(lsn_lease_length, &tenant.cancel).await?; random_init_delay(period, &cancel).await?; Ok::<_, Cancelled>(()) }; @@ -522,6 +523,17 @@ async fn wait_for_active_tenant(tenant: &Arc) -> ControlFlow<()> { #[error("cancelled")] pub(crate) struct Cancelled; +/// Waits for `duration` time unless cancelled. +pub(crate) async fn delay_by_duration( + duration: Duration, + cancel: &CancellationToken, +) -> Result<(), Cancelled> { + match tokio::time::timeout(duration, cancel.cancelled()).await { + Ok(_) => Err(Cancelled), + Err(_) => Ok(()), + } +} + /// Provide a random delay for background task initialization. /// /// This delay prevents a thundering herd of background tasks and will likely keep them running on @@ -539,10 +551,7 @@ pub(crate) async fn random_init_delay( rng.gen_range(Duration::ZERO..=period) }; - match tokio::time::timeout(d, cancel.cancelled()).await { - Ok(_) => Err(Cancelled), - Err(_) => Ok(()), - } + delay_by_duration(d, cancel).await } struct Iteration { From 8bcaa47a311c418984e5d84f9ddc376065b602bc Mon Sep 17 00:00:00 2001 From: Yuchen Liang Date: Tue, 17 Sep 2024 13:10:00 -0400 Subject: [PATCH 05/16] fix comments Signed-off-by: Yuchen Liang --- pageserver/src/tenant/gc_block.rs | 4 ++-- pageserver/src/tenant/mgr.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pageserver/src/tenant/gc_block.rs b/pageserver/src/tenant/gc_block.rs index cf73ad3cb328..f670f109791c 100644 --- a/pageserver/src/tenant/gc_block.rs +++ b/pageserver/src/tenant/gc_block.rs @@ -56,8 +56,8 @@ impl GcBlock { /// Blocks GC until `duration` has elapsed. /// /// We do this as the leases mapping are not persisted to disk. By delaying GC by default - /// length, we guarantee that all the leases we granted before will expire when we run GC for - /// the first time after restart / transition from AttachedMulti to AttachedSingle. + /// length, we guarantee that all the leases we granted before will have a chance to renew + /// when we run GC for the first time after restart / transition from AttachedMulti to AttachedSingle. pub(super) async fn block_for(&self, duration: Duration, cancel: &CancellationToken) { { let g = self.reasons.lock().unwrap(); diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs index d75f0f171330..cb1b8093b1f1 100644 --- a/pageserver/src/tenant/mgr.rs +++ b/pageserver/src/tenant/mgr.rs @@ -951,11 +951,11 @@ impl TenantManager { Ordering::Equal => { if attach_conf.attach_mode == AttachmentMode::Single { // We need to wait for LSN lease duration after transitioning into `AttachedSingle` - // before doing any gc so that all previously granted lease will expire. + // before doing any gc so that all previously granted lease will have a chance to renew. let tenant = tenant.clone(); tokio::spawn(async move { let lsn_lease_length = tenant.get_lsn_lease_length(); - let _ = tenant + tenant .gc_block .block_for(lsn_lease_length, &tenant.cancel) .await; From 3b972d61075200f237efccfdbf6ca7b3f5b56c7b Mon Sep 17 00:00:00 2001 From: Yuchen Liang Date: Tue, 17 Sep 2024 13:13:14 -0400 Subject: [PATCH 06/16] fix build Signed-off-by: Yuchen Liang --- pageserver/src/tenant/gc_block.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/pageserver/src/tenant/gc_block.rs b/pageserver/src/tenant/gc_block.rs index f670f109791c..853dd2620d32 100644 --- a/pageserver/src/tenant/gc_block.rs +++ b/pageserver/src/tenant/gc_block.rs @@ -1,14 +1,9 @@ use std::{collections::HashMap, time::Duration}; -use anyhow::bail; +use super::{remote_timeline_client::index::GcBlockingReason, tasks}; use tokio_util::sync::CancellationToken; use utils::id::TimelineId; -use super::{ - remote_timeline_client::index::GcBlockingReason, - tasks::{self, Cancelled}, -}; - type TimelinesBlocked = HashMap>; #[derive(Default)] @@ -60,14 +55,14 @@ impl GcBlock { /// when we run GC for the first time after restart / transition from AttachedMulti to AttachedSingle. pub(super) async fn block_for(&self, duration: Duration, cancel: &CancellationToken) { { - let g = self.reasons.lock().unwrap(); + let mut g = self.reasons.lock().unwrap(); g.tenant_blocked = true; } let _ = tasks::delay_by_duration(duration, cancel).await; { - let g = self.reasons.lock().unwrap(); + let mut g = self.reasons.lock().unwrap(); g.tenant_blocked = false; } } From 526d8a094115afe5531622a43e02b734c3b1fc40 Mon Sep 17 00:00:00 2001 From: Yuchen Liang Date: Tue, 17 Sep 2024 16:32:21 -0400 Subject: [PATCH 07/16] drop pub; rename tenant_blocked field to be more informative Signed-off-by: Yuchen Liang --- pageserver/src/tenant/gc_block.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pageserver/src/tenant/gc_block.rs b/pageserver/src/tenant/gc_block.rs index 853dd2620d32..57c56e0ef999 100644 --- a/pageserver/src/tenant/gc_block.rs +++ b/pageserver/src/tenant/gc_block.rs @@ -8,8 +8,8 @@ type TimelinesBlocked = HashMap>; #[derive(Default)] struct Storage { - pub timelines_blocked: TimelinesBlocked, - pub tenant_blocked: bool, + timelines_blocked: TimelinesBlocked, + tenant_post_attached_single_wait: bool, } #[derive(Default)] @@ -56,14 +56,14 @@ impl GcBlock { pub(super) async fn block_for(&self, duration: Duration, cancel: &CancellationToken) { { let mut g = self.reasons.lock().unwrap(); - g.tenant_blocked = true; + g.tenant_post_attached_single_wait = true; } let _ = tasks::delay_by_duration(duration, cancel).await; { let mut g = self.reasons.lock().unwrap(); - g.tenant_blocked = false; + g.tenant_post_attached_single_wait = false; } } @@ -214,9 +214,9 @@ impl BlockingReasons { reasons = reasons.union(*value); !value.is_empty() }); - if !g.timelines_blocked.is_empty() || g.tenant_blocked { + if !g.timelines_blocked.is_empty() || g.tenant_post_attached_single_wait { Some(BlockingReasons { - tenant_blocked: g.tenant_blocked, + tenant_blocked: g.tenant_post_attached_single_wait, timelines: g.timelines_blocked.len(), reasons, }) @@ -226,7 +226,7 @@ impl BlockingReasons { } fn summarize(g: &std::sync::MutexGuard<'_, Storage>) -> Option { - if g.timelines_blocked.is_empty() || !g.tenant_blocked { + if g.timelines_blocked.is_empty() || !g.tenant_post_attached_single_wait { None } else { let reasons = g @@ -234,7 +234,7 @@ impl BlockingReasons { .values() .fold(enumset::EnumSet::empty(), |acc, next| acc.union(*next)); Some(BlockingReasons { - tenant_blocked: g.tenant_blocked, + tenant_blocked: g.tenant_post_attached_single_wait, timelines: g.timelines_blocked.len(), reasons, }) From b900db1eaf71367d2b767b5b75b99a717944c474 Mon Sep 17 00:00:00 2001 From: Yuchen Liang Date: Tue, 17 Sep 2024 16:49:51 -0400 Subject: [PATCH 08/16] fix test_gc_blocking_by_timeline and test_sharding_split_compaction Signed-off-by: Yuchen Liang --- pageserver/src/tenant/gc_block.rs | 12 ++++++------ test_runner/regress/test_sharding.py | 1 + test_runner/regress/test_timeline_gc_blocking.py | 5 ++++- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/pageserver/src/tenant/gc_block.rs b/pageserver/src/tenant/gc_block.rs index 57c56e0ef999..6e1bea8854aa 100644 --- a/pageserver/src/tenant/gc_block.rs +++ b/pageserver/src/tenant/gc_block.rs @@ -192,7 +192,7 @@ pub(super) struct Guard<'a> { #[derive(Debug)] pub(crate) struct BlockingReasons { - tenant_blocked: bool, + tenant_post_attached_single_wait: bool, timelines: usize, reasons: enumset::EnumSet, } @@ -201,8 +201,8 @@ impl std::fmt::Display for BlockingReasons { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!( f, - "tenant blocked: {}; {} timelines block for {:?}", - self.tenant_blocked, self.timelines, self.reasons + "tenant_post_attached_single_wait: {}; {} timelines block for {:?}", + self.tenant_post_attached_single_wait, self.timelines, self.reasons ) } } @@ -216,7 +216,7 @@ impl BlockingReasons { }); if !g.timelines_blocked.is_empty() || g.tenant_post_attached_single_wait { Some(BlockingReasons { - tenant_blocked: g.tenant_post_attached_single_wait, + tenant_post_attached_single_wait: g.tenant_post_attached_single_wait, timelines: g.timelines_blocked.len(), reasons, }) @@ -226,7 +226,7 @@ impl BlockingReasons { } fn summarize(g: &std::sync::MutexGuard<'_, Storage>) -> Option { - if g.timelines_blocked.is_empty() || !g.tenant_post_attached_single_wait { + if g.timelines_blocked.is_empty() && !g.tenant_post_attached_single_wait { None } else { let reasons = g @@ -234,7 +234,7 @@ impl BlockingReasons { .values() .fold(enumset::EnumSet::empty(), |acc, next| acc.union(*next)); Some(BlockingReasons { - tenant_blocked: g.tenant_post_attached_single_wait, + tenant_post_attached_single_wait: g.tenant_post_attached_single_wait, timelines: g.timelines_blocked.len(), reasons, }) diff --git a/test_runner/regress/test_sharding.py b/test_runner/regress/test_sharding.py index 4a84dca399a3..1eb33b2d39ca 100644 --- a/test_runner/regress/test_sharding.py +++ b/test_runner/regress/test_sharding.py @@ -200,6 +200,7 @@ def test_sharding_split_compaction(neon_env_builder: NeonEnvBuilder, failpoint: # Disable automatic creation of image layers, as we will create them explicitly when we want them "image_creation_threshold": 9999, "image_layer_creation_check_threshold": 0, + "lsn_lease_length": "0s", } neon_env_builder.storage_controller_config = { diff --git a/test_runner/regress/test_timeline_gc_blocking.py b/test_runner/regress/test_timeline_gc_blocking.py index ddfe9b911fd8..d0b121e8e271 100644 --- a/test_runner/regress/test_timeline_gc_blocking.py +++ b/test_runner/regress/test_timeline_gc_blocking.py @@ -45,7 +45,10 @@ def test_gc_blocking_by_timeline(neon_env_builder: NeonEnvBuilder, sharded: bool tenant_after = http.tenant_status(env.initial_tenant) assert tenant_before != tenant_after gc_blocking = tenant_after["gc_blocking"] - assert gc_blocking == "BlockingReasons { timelines: 1, reasons: EnumSet(Manual) }" + assert ( + gc_blocking + == "BlockingReasons { tenant_post_attached_single_wait: false, timelines: 1, reasons: EnumSet(Manual) }" + ) wait_for_another_gc_round() pss.assert_log_contains(gc_skipped_line) From 7d8bbddbb990ad34452889b3dc1df09c03f084f3 Mon Sep 17 00:00:00 2001 From: Yuchen Liang Date: Wed, 18 Sep 2024 11:08:40 -0400 Subject: [PATCH 09/16] use blocked until deadline mechanism Signed-off-by: Yuchen Liang --- pageserver/src/tenant/gc_block.rs | 56 +++++++++++-------- pageserver/src/tenant/mgr.rs | 13 +---- pageserver/src/tenant/tasks.rs | 17 +++--- .../regress/test_timeline_gc_blocking.py | 2 +- 4 files changed, 46 insertions(+), 42 deletions(-) diff --git a/pageserver/src/tenant/gc_block.rs b/pageserver/src/tenant/gc_block.rs index 6e1bea8854aa..9dba536f3703 100644 --- a/pageserver/src/tenant/gc_block.rs +++ b/pageserver/src/tenant/gc_block.rs @@ -1,7 +1,7 @@ use std::{collections::HashMap, time::Duration}; -use super::{remote_timeline_client::index::GcBlockingReason, tasks}; -use tokio_util::sync::CancellationToken; +use super::remote_timeline_client::index::GcBlockingReason; +use tokio::time::Instant; use utils::id::TimelineId; type TimelinesBlocked = HashMap>; @@ -9,7 +9,17 @@ type TimelinesBlocked = HashMap>; #[derive(Default)] struct Storage { timelines_blocked: TimelinesBlocked, - tenant_post_attached_single_wait: bool, + /// The deadline before which we are blocked from GC so that + /// leases have a chance to be renewed. + lsn_lease_deadline: Option, +} + +impl Storage { + fn is_blocked_by_lsn_lease_deadline(&self) -> bool { + self.lsn_lease_deadline + .map(|d| Instant::now() < d) + .unwrap_or(false) + } } #[derive(Default)] @@ -48,23 +58,21 @@ impl GcBlock { } } - /// Blocks GC until `duration` has elapsed. + /// Sets a deadline before which we cannot proceed to GC due to lsn lease. /// - /// We do this as the leases mapping are not persisted to disk. By delaying GC by default + /// We do this as the leases mapping are not persisted to disk. By delaying GC by lease /// length, we guarantee that all the leases we granted before will have a chance to renew /// when we run GC for the first time after restart / transition from AttachedMulti to AttachedSingle. - pub(super) async fn block_for(&self, duration: Duration, cancel: &CancellationToken) { - { - let mut g = self.reasons.lock().unwrap(); - g.tenant_post_attached_single_wait = true; - } - - let _ = tasks::delay_by_duration(duration, cancel).await; + pub(super) fn set_lsn_lease_deadline(&self, lsn_lease_length: Duration) { + let deadline = Instant::now() + lsn_lease_length; + let mut g = self.reasons.lock().unwrap(); + g.lsn_lease_deadline = Some(deadline); + } - { - let mut g = self.reasons.lock().unwrap(); - g.tenant_post_attached_single_wait = false; - } + /// Gets the deadline before which we cannot proceed to GC due to lsn lease. + pub(super) fn get_lsn_lease_deadline(&self) -> Instant { + let g = self.reasons.lock().unwrap(); + g.lsn_lease_deadline.unwrap_or(Instant::now()) } pub(crate) fn summary(&self) -> Option { @@ -192,7 +200,7 @@ pub(super) struct Guard<'a> { #[derive(Debug)] pub(crate) struct BlockingReasons { - tenant_post_attached_single_wait: bool, + tenant_blocked_by_lsn_lease_deadline: bool, timelines: usize, reasons: enumset::EnumSet, } @@ -201,8 +209,8 @@ impl std::fmt::Display for BlockingReasons { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!( f, - "tenant_post_attached_single_wait: {}; {} timelines block for {:?}", - self.tenant_post_attached_single_wait, self.timelines, self.reasons + "tenant_blocked_by_lsn_lease_deadline: {}, {} timelines block for {:?}", + self.tenant_blocked_by_lsn_lease_deadline, self.timelines, self.reasons ) } } @@ -214,9 +222,10 @@ impl BlockingReasons { reasons = reasons.union(*value); !value.is_empty() }); - if !g.timelines_blocked.is_empty() || g.tenant_post_attached_single_wait { + let blocked_by_lsn_lease_deadline = g.is_blocked_by_lsn_lease_deadline(); + if !g.timelines_blocked.is_empty() || blocked_by_lsn_lease_deadline { Some(BlockingReasons { - tenant_post_attached_single_wait: g.tenant_post_attached_single_wait, + tenant_blocked_by_lsn_lease_deadline: blocked_by_lsn_lease_deadline, timelines: g.timelines_blocked.len(), reasons, }) @@ -226,7 +235,8 @@ impl BlockingReasons { } fn summarize(g: &std::sync::MutexGuard<'_, Storage>) -> Option { - if g.timelines_blocked.is_empty() && !g.tenant_post_attached_single_wait { + let blocked_by_lsn_lease_deadline = g.is_blocked_by_lsn_lease_deadline(); + if g.timelines_blocked.is_empty() && !blocked_by_lsn_lease_deadline { None } else { let reasons = g @@ -234,7 +244,7 @@ impl BlockingReasons { .values() .fold(enumset::EnumSet::empty(), |acc, next| acc.union(*next)); Some(BlockingReasons { - tenant_post_attached_single_wait: g.tenant_post_attached_single_wait, + tenant_blocked_by_lsn_lease_deadline: blocked_by_lsn_lease_deadline, timelines: g.timelines_blocked.len(), reasons, }) diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs index cb1b8093b1f1..1e7c1e10a5e7 100644 --- a/pageserver/src/tenant/mgr.rs +++ b/pageserver/src/tenant/mgr.rs @@ -950,16 +950,9 @@ impl TenantManager { match attach_conf.generation.cmp(&tenant.generation) { Ordering::Equal => { if attach_conf.attach_mode == AttachmentMode::Single { - // We need to wait for LSN lease duration after transitioning into `AttachedSingle` - // before doing any gc so that all previously granted lease will have a chance to renew. - let tenant = tenant.clone(); - tokio::spawn(async move { - let lsn_lease_length = tenant.get_lsn_lease_length(); - tenant - .gc_block - .block_for(lsn_lease_length, &tenant.cancel) - .await; - }); + tenant + .gc_block + .set_lsn_lease_deadline(tenant.get_lsn_lease_length()); } // A transition from Attached to Attached in the same generation, we may diff --git a/pageserver/src/tenant/tasks.rs b/pageserver/src/tenant/tasks.rs index fc21fb7af14a..d3f641ba4163 100644 --- a/pageserver/src/tenant/tasks.rs +++ b/pageserver/src/tenant/tasks.rs @@ -346,6 +346,7 @@ async fn gc_loop(tenant: Arc, cancel: CancellationToken) { RequestContext::todo_child(TaskKind::GarbageCollector, DownloadBehavior::Download); let mut first = true; + tenant.gc_block.set_lsn_lease_deadline(tenant.get_lsn_lease_length()); loop { tokio::select! { _ = cancel.cancelled() => { @@ -363,8 +364,8 @@ async fn gc_loop(tenant: Arc, cancel: CancellationToken) { first = false; let delays = async { - let lsn_lease_length = tenant.get_lsn_lease_length(); - delay_by_duration(lsn_lease_length, &tenant.cancel).await?; + let deadline = tenant.gc_block.get_lsn_lease_deadline(); + delay_until(deadline, &tenant.cancel).await?; random_init_delay(period, &cancel).await?; Ok::<_, Cancelled>(()) }; @@ -423,7 +424,8 @@ async fn gc_loop(tenant: Arc, cancel: CancellationToken) { } }; - if tokio::time::timeout(sleep_duration, cancel.cancelled()) + let deadline = tenant.gc_block.get_lsn_lease_deadline().max(tokio::time::Instant::now() + sleep_duration); + if tokio::time::timeout_at(deadline, cancel.cancelled()) .await .is_ok() { @@ -524,11 +526,11 @@ async fn wait_for_active_tenant(tenant: &Arc) -> ControlFlow<()> { pub(crate) struct Cancelled; /// Waits for `duration` time unless cancelled. -pub(crate) async fn delay_by_duration( - duration: Duration, +async fn delay_until( + deadline: tokio::time::Instant, cancel: &CancellationToken, ) -> Result<(), Cancelled> { - match tokio::time::timeout(duration, cancel.cancelled()).await { + match tokio::time::timeout_at(deadline, cancel.cancelled()).await { Ok(_) => Err(Cancelled), Err(_) => Ok(()), } @@ -550,8 +552,7 @@ pub(crate) async fn random_init_delay( let mut rng = rand::thread_rng(); rng.gen_range(Duration::ZERO..=period) }; - - delay_by_duration(d, cancel).await + delay_until(tokio::time::Instant::now() + d, cancel).await } struct Iteration { diff --git a/test_runner/regress/test_timeline_gc_blocking.py b/test_runner/regress/test_timeline_gc_blocking.py index d0b121e8e271..765c72cf2a2f 100644 --- a/test_runner/regress/test_timeline_gc_blocking.py +++ b/test_runner/regress/test_timeline_gc_blocking.py @@ -47,7 +47,7 @@ def test_gc_blocking_by_timeline(neon_env_builder: NeonEnvBuilder, sharded: bool gc_blocking = tenant_after["gc_blocking"] assert ( gc_blocking - == "BlockingReasons { tenant_post_attached_single_wait: false, timelines: 1, reasons: EnumSet(Manual) }" + == "BlockingReasons { tenant_blocked_by_lsn_lease_deadline: false, timelines: 1, reasons: EnumSet(Manual) }" ) wait_for_another_gc_round() From 9ad70331f3c17dca0a6ba222a78ca6c44c525e1d Mon Sep 17 00:00:00 2001 From: Yuchen Liang Date: Wed, 18 Sep 2024 11:34:04 -0400 Subject: [PATCH 10/16] skip current round of gc if deadline not reached Signed-off-by: Yuchen Liang --- pageserver/src/tenant/gc_block.rs | 6 ------ pageserver/src/tenant/tasks.rs | 5 +---- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/pageserver/src/tenant/gc_block.rs b/pageserver/src/tenant/gc_block.rs index 9dba536f3703..59519c23de9d 100644 --- a/pageserver/src/tenant/gc_block.rs +++ b/pageserver/src/tenant/gc_block.rs @@ -69,12 +69,6 @@ impl GcBlock { g.lsn_lease_deadline = Some(deadline); } - /// Gets the deadline before which we cannot proceed to GC due to lsn lease. - pub(super) fn get_lsn_lease_deadline(&self) -> Instant { - let g = self.reasons.lock().unwrap(); - g.lsn_lease_deadline.unwrap_or(Instant::now()) - } - pub(crate) fn summary(&self) -> Option { let g = self.reasons.lock().unwrap(); diff --git a/pageserver/src/tenant/tasks.rs b/pageserver/src/tenant/tasks.rs index d3f641ba4163..d50846266077 100644 --- a/pageserver/src/tenant/tasks.rs +++ b/pageserver/src/tenant/tasks.rs @@ -364,8 +364,6 @@ async fn gc_loop(tenant: Arc, cancel: CancellationToken) { first = false; let delays = async { - let deadline = tenant.gc_block.get_lsn_lease_deadline(); - delay_until(deadline, &tenant.cancel).await?; random_init_delay(period, &cancel).await?; Ok::<_, Cancelled>(()) }; @@ -424,8 +422,7 @@ async fn gc_loop(tenant: Arc, cancel: CancellationToken) { } }; - let deadline = tenant.gc_block.get_lsn_lease_deadline().max(tokio::time::Instant::now() + sleep_duration); - if tokio::time::timeout_at(deadline, cancel.cancelled()) + if tokio::time::timeout(sleep_duration, cancel.cancelled()) .await .is_ok() { From 78a1493f44c2566f8240c06aa89b000d9d2ae4c8 Mon Sep 17 00:00:00 2001 From: Yuchen Liang Date: Wed, 18 Sep 2024 18:16:27 -0400 Subject: [PATCH 11/16] set lsn_lease_length=0s in tests Signed-off-by: Yuchen Liang --- pageserver/src/tenant/tasks.rs | 16 ++++------------ test_runner/regress/test_branch_behind.py | 4 +++- test_runner/regress/test_branching.py | 2 +- test_runner/regress/test_compaction.py | 1 + test_runner/regress/test_hot_standby.py | 2 +- test_runner/regress/test_layer_eviction.py | 1 + .../regress/test_pageserver_generations.py | 1 + test_runner/regress/test_remote_storage.py | 1 + test_runner/regress/test_storage_controller.py | 2 +- test_runner/regress/test_storage_scrubber.py | 1 + test_runner/regress/test_tenant_detach.py | 2 +- 11 files changed, 16 insertions(+), 17 deletions(-) diff --git a/pageserver/src/tenant/tasks.rs b/pageserver/src/tenant/tasks.rs index d50846266077..57f0123d8fa3 100644 --- a/pageserver/src/tenant/tasks.rs +++ b/pageserver/src/tenant/tasks.rs @@ -522,17 +522,6 @@ async fn wait_for_active_tenant(tenant: &Arc) -> ControlFlow<()> { #[error("cancelled")] pub(crate) struct Cancelled; -/// Waits for `duration` time unless cancelled. -async fn delay_until( - deadline: tokio::time::Instant, - cancel: &CancellationToken, -) -> Result<(), Cancelled> { - match tokio::time::timeout_at(deadline, cancel.cancelled()).await { - Ok(_) => Err(Cancelled), - Err(_) => Ok(()), - } -} - /// Provide a random delay for background task initialization. /// /// This delay prevents a thundering herd of background tasks and will likely keep them running on @@ -549,7 +538,10 @@ pub(crate) async fn random_init_delay( let mut rng = rand::thread_rng(); rng.gen_range(Duration::ZERO..=period) }; - delay_until(tokio::time::Instant::now() + d, cancel).await + match tokio::time::timeout(d, cancel.cancelled()).await { + Ok(_) => Err(Cancelled), + Err(_) => Ok(()), + } } struct Iteration { diff --git a/test_runner/regress/test_branch_behind.py b/test_runner/regress/test_branch_behind.py index 0a5336f5a246..2bf7041cf14b 100644 --- a/test_runner/regress/test_branch_behind.py +++ b/test_runner/regress/test_branch_behind.py @@ -11,7 +11,9 @@ # def test_branch_behind(neon_env_builder: NeonEnvBuilder): # Disable pitr, because here we want to test branch creation after GC - env = neon_env_builder.init_start(initial_tenant_conf={"pitr_interval": "0 sec"}) + env = neon_env_builder.init_start( + initial_tenant_conf={"pitr_interval": "0 sec", "lsn_lease_length": "0s"} + ) error_regexes = [ ".*invalid branch start lsn.*", diff --git a/test_runner/regress/test_branching.py b/test_runner/regress/test_branching.py index fc7470763942..0a98647b5610 100644 --- a/test_runner/regress/test_branching.py +++ b/test_runner/regress/test_branching.py @@ -419,7 +419,7 @@ def start_creating_timeline(): def test_branching_while_stuck_find_gc_cutoffs(neon_env_builder: NeonEnvBuilder): - env = neon_env_builder.init_start() + env = neon_env_builder.init_start(initial_tenant_conf={"lsn_lease_length": "0s"}) client = env.pageserver.http_client() diff --git a/test_runner/regress/test_compaction.py b/test_runner/regress/test_compaction.py index be787e064262..cb34551b53fc 100644 --- a/test_runner/regress/test_compaction.py +++ b/test_runner/regress/test_compaction.py @@ -240,6 +240,7 @@ def test_uploads_and_deletions( "image_creation_threshold": "1", "image_layer_creation_check_threshold": "0", "compaction_algorithm": json.dumps({"kind": compaction_algorithm.value}), + "lsn_lease_length": "0s", } env = neon_env_builder.init_start(initial_tenant_conf=tenant_conf) diff --git a/test_runner/regress/test_hot_standby.py b/test_runner/regress/test_hot_standby.py index ae63136abb35..ca175455d561 100644 --- a/test_runner/regress/test_hot_standby.py +++ b/test_runner/regress/test_hot_standby.py @@ -222,7 +222,7 @@ def pgbench_accounts_initialized(ep): # Without hs feedback enabled we'd see 'User query might have needed to see row # versions that must be removed.' errors. def test_hot_standby_feedback(neon_env_builder: NeonEnvBuilder, pg_bin: PgBin): - env = neon_env_builder.init_start() + env = neon_env_builder.init_start(initial_tenant_conf={"lsn_lease_length": "0s"}) agressive_vacuum_conf = [ "log_autovacuum_min_duration = 0", "autovacuum_naptime = 10s", diff --git a/test_runner/regress/test_layer_eviction.py b/test_runner/regress/test_layer_eviction.py index 193149ea0388..97093ea535c2 100644 --- a/test_runner/regress/test_layer_eviction.py +++ b/test_runner/regress/test_layer_eviction.py @@ -173,6 +173,7 @@ def test_gc_of_remote_layers(neon_env_builder: NeonEnvBuilder): # "image_creation_threshold": set at runtime "compaction_target_size": f"{128 * (1024**2)}", # make it so that we only have 1 partition => image coverage for delta layers => enables gc of delta layers "image_layer_creation_check_threshold": "0", # always check if a new image layer can be created + "lsn_lease_length": "0s", } def tenant_update_config(changes): diff --git a/test_runner/regress/test_pageserver_generations.py b/test_runner/regress/test_pageserver_generations.py index ebf58d2bd191..899b4974a6ac 100644 --- a/test_runner/regress/test_pageserver_generations.py +++ b/test_runner/regress/test_pageserver_generations.py @@ -53,6 +53,7 @@ # create image layers eagerly, so that GC can remove some layers "image_creation_threshold": "1", "image_layer_creation_check_threshold": "0", + "lsn_lease_length": "0s", } diff --git a/test_runner/regress/test_remote_storage.py b/test_runner/regress/test_remote_storage.py index 2e5260ca781a..201740de6391 100644 --- a/test_runner/regress/test_remote_storage.py +++ b/test_runner/regress/test_remote_storage.py @@ -244,6 +244,7 @@ def test_remote_storage_upload_queue_retries( # create image layers eagerly, so that GC can remove some layers "image_creation_threshold": "1", "image_layer_creation_check_threshold": "0", + "lsn_lease_length": "0s", } ) diff --git a/test_runner/regress/test_storage_controller.py b/test_runner/regress/test_storage_controller.py index eea05d75484a..32ef2ed908b0 100644 --- a/test_runner/regress/test_storage_controller.py +++ b/test_runner/regress/test_storage_controller.py @@ -485,7 +485,7 @@ def handler(request: Request): httpserver.expect_request("/notify", method="PUT").respond_with_handler(handler) # Start running - env = neon_env_builder.init_start() + env = neon_env_builder.init_start(initial_tenant_conf={"lsn_lease_length": "0s"}) # Initial notification from tenant creation assert len(notifications) == 1 diff --git a/test_runner/regress/test_storage_scrubber.py b/test_runner/regress/test_storage_scrubber.py index 848e214c5e46..b6c19f03f6ab 100644 --- a/test_runner/regress/test_storage_scrubber.py +++ b/test_runner/regress/test_storage_scrubber.py @@ -204,6 +204,7 @@ def test_scrubber_physical_gc_ancestors( # No PITR, so that as soon as child shards generate an image layer, it covers ancestor deltas # and makes them GC'able "pitr_interval": "0s", + "lsn_lease_length": "0s", }, ) diff --git a/test_runner/regress/test_tenant_detach.py b/test_runner/regress/test_tenant_detach.py index b165588636c7..165cf29df8bd 100644 --- a/test_runner/regress/test_tenant_detach.py +++ b/test_runner/regress/test_tenant_detach.py @@ -272,7 +272,7 @@ def test_tenant_detach_smoke(neon_env_builder: NeonEnvBuilder): env.pageserver.allowed_errors.extend(PERMIT_PAGE_SERVICE_ERRORS) # create new nenant - tenant_id, timeline_id = env.neon_cli.create_tenant() + tenant_id, timeline_id = env.neon_cli.create_tenant(conf={"lsn_lease_length": "0s"}) # assert tenant exists on disk assert env.pageserver.tenant_dir(tenant_id).exists() From 7ef2421c3f1099a2c067230713db26d698c58db5 Mon Sep 17 00:00:00 2001 From: Yuchen Liang Date: Wed, 18 Sep 2024 22:27:11 -0400 Subject: [PATCH 12/16] add lsn_lease_length=0s to more tests Signed-off-by: Yuchen Liang --- test_runner/regress/test_branch_and_gc.py | 1 + test_runner/regress/test_remote_storage.py | 1 + 2 files changed, 2 insertions(+) diff --git a/test_runner/regress/test_branch_and_gc.py b/test_runner/regress/test_branch_and_gc.py index f2e3855c123e..d7c4cf059a4e 100644 --- a/test_runner/regress/test_branch_and_gc.py +++ b/test_runner/regress/test_branch_and_gc.py @@ -142,6 +142,7 @@ def test_branch_creation_before_gc(neon_simple_env: NeonEnv): "image_creation_threshold": "1", # set PITR interval to be small, so we can do GC "pitr_interval": "0 s", + "lsn_lease_length": "0s", } ) diff --git a/test_runner/regress/test_remote_storage.py b/test_runner/regress/test_remote_storage.py index 201740de6391..0a57fc960563 100644 --- a/test_runner/regress/test_remote_storage.py +++ b/test_runner/regress/test_remote_storage.py @@ -392,6 +392,7 @@ def test_remote_timeline_client_calls_started_metric( # disable background compaction and GC. We invoke it manually when we want it to happen. "gc_period": "0s", "compaction_period": "0s", + "lsn_lease_length": "0s", } ) From a76e1f71c58b2cacd433c1a256ea5ea1d2340820 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Thu, 19 Sep 2024 09:18:03 +0000 Subject: [PATCH 13/16] test: remove extra tenant --- test_runner/regress/test_tenant_detach.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test_runner/regress/test_tenant_detach.py b/test_runner/regress/test_tenant_detach.py index 165cf29df8bd..e7c6d5a4c382 100644 --- a/test_runner/regress/test_tenant_detach.py +++ b/test_runner/regress/test_tenant_detach.py @@ -266,13 +266,13 @@ async def reattach_while_busy( def test_tenant_detach_smoke(neon_env_builder: NeonEnvBuilder): - env = neon_env_builder.init_start() + env = neon_env_builder.init_start(initial_tenant_conf={"lsn_lease_length": "0s"}) pageserver_http = env.pageserver.http_client() env.pageserver.allowed_errors.extend(PERMIT_PAGE_SERVICE_ERRORS) # create new nenant - tenant_id, timeline_id = env.neon_cli.create_tenant(conf={"lsn_lease_length": "0s"}) + tenant_id, timeline_id = env.initial_tenant, env.initial_timeline # assert tenant exists on disk assert env.pageserver.tenant_dir(tenant_id).exists() From 03873f538191050d3f6a43476d076aaef2eb073c Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Thu, 19 Sep 2024 10:39:39 +0000 Subject: [PATCH 14/16] add a comment on what is the blocking --- pageserver/src/tenant/gc_block.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pageserver/src/tenant/gc_block.rs b/pageserver/src/tenant/gc_block.rs index 59519c23de9d..be14442d17c0 100644 --- a/pageserver/src/tenant/gc_block.rs +++ b/pageserver/src/tenant/gc_block.rs @@ -29,6 +29,12 @@ pub(crate) struct GcBlock { /// LOCK ORDER: this is held locked while scheduling the next index_part update. This is done /// to keep the this field up to date with RemoteTimelineClient `upload_queue.dirty`. reasons: std::sync::Mutex, + + /// GC background task or manually run `Tenant::gc_iteration` holds a lock on this. + /// + /// Do not add any more features taking and forbidding taking this lock. It should be + /// `tokio::sync::Notify`, but that is rarely used. On the other side, [`GcBlock::insert`] + /// synchronizes with gc tasks by locking and unlocking this mutex. blocking: tokio::sync::Mutex<()>, } From 9ad42bf6f26dfc0b90df6866039e0f8324c1ad33 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Thu, 19 Sep 2024 10:42:04 +0000 Subject: [PATCH 15/16] doc --- pageserver/src/tenant/gc_block.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pageserver/src/tenant/gc_block.rs b/pageserver/src/tenant/gc_block.rs index be14442d17c0..b7c92b183ca0 100644 --- a/pageserver/src/tenant/gc_block.rs +++ b/pageserver/src/tenant/gc_block.rs @@ -34,7 +34,7 @@ pub(crate) struct GcBlock { /// /// Do not add any more features taking and forbidding taking this lock. It should be /// `tokio::sync::Notify`, but that is rarely used. On the other side, [`GcBlock::insert`] - /// synchronizes with gc tasks by locking and unlocking this mutex. + /// synchronizes with gc attempts by locking and unlocking this mutex. blocking: tokio::sync::Mutex<()>, } From 67652546b40b53c9450a506aa26f18c4363e0631 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Thu, 19 Sep 2024 10:53:37 +0000 Subject: [PATCH 16/16] doc: more stuff --- pageserver/src/tenant/gc_block.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pageserver/src/tenant/gc_block.rs b/pageserver/src/tenant/gc_block.rs index b7c92b183ca0..1271d25b7659 100644 --- a/pageserver/src/tenant/gc_block.rs +++ b/pageserver/src/tenant/gc_block.rs @@ -22,6 +22,8 @@ impl Storage { } } +/// GcBlock provides persistent (per-timeline) gc blocking and facilitates transient time based gc +/// blocking. #[derive(Default)] pub(crate) struct GcBlock { /// The timelines which have current reasons to block gc. @@ -75,6 +77,9 @@ impl GcBlock { g.lsn_lease_deadline = Some(deadline); } + /// Describe the current gc blocking reasons. + /// + /// TODO: make this json serializable. pub(crate) fn summary(&self) -> Option { let g = self.reasons.lock().unwrap();