diff --git a/safekeeper/src/timelines_global_map.rs b/safekeeper/src/timelines_global_map.rs index af33bcbd20..e3f7d88f7c 100644 --- a/safekeeper/src/timelines_global_map.rs +++ b/safekeeper/src/timelines_global_map.rs @@ -44,6 +44,7 @@ struct GlobalTimelinesState { // on-demand timeline creation from recreating deleted timelines. This is only soft-enforced, as // this map is dropped on restart. tombstones: HashMap, + tenant_tombstones: HashMap, conf: Arc, broker_active_set: Arc, @@ -81,10 +82,25 @@ impl GlobalTimelinesState { } } + fn has_tombstone(&self, ttid: &TenantTimelineId) -> bool { + self.tombstones.contains_key(ttid) || self.tenant_tombstones.contains_key(&ttid.tenant_id) + } + + /// Removes all blocking tombstones for the given timeline ID. + /// Returns `true` if there have been actual changes. + fn remove_tombstone(&mut self, ttid: &TenantTimelineId) -> bool { + self.tombstones.remove(ttid).is_some() + || self.tenant_tombstones.remove(&ttid.tenant_id).is_some() + } + fn delete(&mut self, ttid: TenantTimelineId) { self.timelines.remove(&ttid); self.tombstones.insert(ttid, Instant::now()); } + + fn add_tenant_tombstone(&mut self, tenant_id: TenantId) { + self.tenant_tombstones.insert(tenant_id, Instant::now()); + } } /// A struct used to manage access to the global timelines map. @@ -99,6 +115,7 @@ impl GlobalTimelines { state: Mutex::new(GlobalTimelinesState { timelines: HashMap::new(), tombstones: HashMap::new(), + tenant_tombstones: HashMap::new(), conf, broker_active_set: Arc::new(TimelinesSet::default()), global_rate_limiter: RateLimiter::new(1, 1), @@ -245,7 +262,7 @@ impl GlobalTimelines { return Ok(timeline); } - if state.tombstones.contains_key(&ttid) { + if state.has_tombstone(&ttid) { anyhow::bail!("Timeline {ttid} is deleted, refusing to recreate"); } @@ -295,13 +312,14 @@ impl GlobalTimelines { _ => {} } if check_tombstone { - if state.tombstones.contains_key(&ttid) { + if state.has_tombstone(&ttid) { anyhow::bail!("timeline {ttid} is deleted, refusing to recreate"); } } else { // We may be have been asked to load a timeline that was previously deleted (e.g. from `pull_timeline.rs`). We trust // that the human doing this manual intervention knows what they are doing, and remove its tombstone. - if state.tombstones.remove(&ttid).is_some() { + // It's also possible that we enter this when the tenant has been deleted, even if the timeline itself has never existed. + if state.remove_tombstone(&ttid) { warn!("un-deleted timeline {ttid}"); } } @@ -482,6 +500,7 @@ impl GlobalTimelines { let tli_res = { let state = self.state.lock().unwrap(); + // Do NOT check tenant tombstones here: those were set earlier if state.tombstones.contains_key(ttid) { // Presence of a tombstone guarantees that a previous deletion has completed and there is no work to do. info!("Timeline {ttid} was already deleted"); @@ -557,6 +576,10 @@ impl GlobalTimelines { action: DeleteOrExclude, ) -> Result> { info!("deleting all timelines for tenant {}", tenant_id); + + // Adding a tombstone before getting the timelines to prevent new timeline additions + self.state.lock().unwrap().add_tenant_tombstone(*tenant_id); + let to_delete = self.get_all_for_tenant(*tenant_id); let mut err = None; @@ -600,6 +623,9 @@ impl GlobalTimelines { state .tombstones .retain(|_, v| now.duration_since(*v) < *tombstone_ttl); + state + .tenant_tombstones + .retain(|_, v| now.duration_since(*v) < *tombstone_ttl); } } diff --git a/test_runner/regress/test_storage_controller.py b/test_runner/regress/test_storage_controller.py index d07fb38c5a..346ef0951d 100644 --- a/test_runner/regress/test_storage_controller.py +++ b/test_runner/regress/test_storage_controller.py @@ -4192,10 +4192,10 @@ def test_storcon_create_delete_sk_down( # ensure the safekeeper deleted the timeline def timeline_deleted_on_active_sks(): env.safekeepers[0].assert_log_contains( - f"deleting timeline {tenant_id}/{child_timeline_id} from disk" + f"((deleting timeline|Timeline) {tenant_id}/{child_timeline_id} (from disk|was already deleted)|DELETE.*tenant/{tenant_id} .*status: 200 OK)" ) env.safekeepers[2].assert_log_contains( - f"deleting timeline {tenant_id}/{child_timeline_id} from disk" + f"((deleting timeline|Timeline) {tenant_id}/{child_timeline_id} (from disk|was already deleted)|DELETE.*tenant/{tenant_id} .*status: 200 OK)" ) wait_until(timeline_deleted_on_active_sks) @@ -4210,7 +4210,7 @@ def test_storcon_create_delete_sk_down( # ensure that there is log msgs for the third safekeeper too def timeline_deleted_on_sk(): env.safekeepers[1].assert_log_contains( - f"deleting timeline {tenant_id}/{child_timeline_id} from disk" + f"((deleting timeline|Timeline) {tenant_id}/{child_timeline_id} (from disk|was already deleted)|DELETE.*tenant/{tenant_id} .*status: 200 OK)" ) wait_until(timeline_deleted_on_sk)