From 22c26d610b0a0cd4c7d714c25b8db1d516e3cc7f Mon Sep 17 00:00:00 2001 From: John Spray Date: Fri, 15 Mar 2024 15:23:05 +0000 Subject: [PATCH] pageserver: remove un-needed "uninit mark" (#5717) Switched the order; doing https://github.com/neondatabase/neon/pull/6139 first then can remove uninit marker after. ## Problem Previously, existence of a timeline directory was treated as evidence of the timeline's logical existence. That is no longer the case since we treat remote storage as the source of truth on each startup: we can therefore do without this mark file. The mark file had also been used as a pseudo-lock to guard against concurrent creations of the same TimelineId -- now that persistence is no longer required, this is a bit unwieldy. In #6139 the `Tenant::timelines_creating` was added to protect against concurrent creations on the same TimelineId, making the uninit mark file entirely redundant. ## Summary of changes - Code that writes & reads mark file is removed - Some nearby `pub` definitions are amended to `pub(crate)` - `test_duplicate_creation` is added to demonstrate that mutual exclusion of creations still works. --- pageserver/src/config.rs | 20 +-- pageserver/src/http/routes.rs | 6 +- pageserver/src/lib.rs | 16 +-- pageserver/src/tenant.rs | 122 +++++++----------- pageserver/src/tenant/timeline/uninit.rs | 98 ++++---------- .../fixtures/pageserver/allowed_errors.py | 1 - test_runner/fixtures/pageserver/http.py | 2 +- test_runner/regress/test_branching.py | 58 +++++++++ test_runner/regress/test_broken_timeline.py | 8 +- test_runner/regress/test_import.py | 1 - 10 files changed, 147 insertions(+), 185 deletions(-) diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs index 845b20c8db..7dac0ab352 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -41,7 +41,7 @@ use crate::tenant::{ use crate::virtual_file; use crate::{ IGNORED_TENANT_FILE_NAME, TENANT_CONFIG_NAME, TENANT_HEATMAP_BASENAME, - TENANT_LOCATION_CONFIG_NAME, TIMELINE_DELETE_MARK_SUFFIX, TIMELINE_UNINIT_MARK_SUFFIX, + TENANT_LOCATION_CONFIG_NAME, TIMELINE_DELETE_MARK_SUFFIX, }; use self::defaults::DEFAULT_CONCURRENT_TENANT_WARMUP; @@ -845,18 +845,7 @@ impl PageServerConf { .join(timeline_id.to_string()) } - pub fn timeline_uninit_mark_file_path( - &self, - tenant_shard_id: TenantShardId, - timeline_id: TimelineId, - ) -> Utf8PathBuf { - path_with_suffix_extension( - self.timeline_path(&tenant_shard_id, &timeline_id), - TIMELINE_UNINIT_MARK_SUFFIX, - ) - } - - pub fn timeline_delete_mark_file_path( + pub(crate) fn timeline_delete_mark_file_path( &self, tenant_shard_id: TenantShardId, timeline_id: TimelineId, @@ -867,7 +856,10 @@ impl PageServerConf { ) } - pub fn tenant_deleted_mark_file_path(&self, tenant_shard_id: &TenantShardId) -> Utf8PathBuf { + pub(crate) fn tenant_deleted_mark_file_path( + &self, + tenant_shard_id: &TenantShardId, + ) -> Utf8PathBuf { self.tenant_path(tenant_shard_id) .join(TENANT_DELETED_MARKER_FILE_NAME) } diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs index d6fe9f6055..7d3ede21ce 100644 --- a/pageserver/src/http/routes.rs +++ b/pageserver/src/http/routes.rs @@ -535,9 +535,9 @@ async fn timeline_create_handler( ) } Err( - tenant::CreateTimelineError::Conflict - | tenant::CreateTimelineError::AlreadyCreating, - ) => json_response(StatusCode::CONFLICT, ()), + e @ tenant::CreateTimelineError::Conflict + | e @ tenant::CreateTimelineError::AlreadyCreating, + ) => json_response(StatusCode::CONFLICT, HttpErrorBody::from_msg(e.to_string())), Err(tenant::CreateTimelineError::AncestorLsn(err)) => json_response( StatusCode::NOT_ACCEPTABLE, HttpErrorBody::from_msg(format!("{err:#}")), diff --git a/pageserver/src/lib.rs b/pageserver/src/lib.rs index 02a690d4e1..b00db02a1c 100644 --- a/pageserver/src/lib.rs +++ b/pageserver/src/lib.rs @@ -114,27 +114,27 @@ pub const METADATA_FILE_NAME: &str = "metadata"; /// Per-tenant configuration file. /// Full path: `tenants//config`. -pub const TENANT_CONFIG_NAME: &str = "config"; +pub(crate) const TENANT_CONFIG_NAME: &str = "config"; /// Per-tenant configuration file. /// Full path: `tenants//config`. -pub const TENANT_LOCATION_CONFIG_NAME: &str = "config-v1"; +pub(crate) const TENANT_LOCATION_CONFIG_NAME: &str = "config-v1"; /// Per-tenant copy of their remote heatmap, downloaded into the local /// tenant path while in secondary mode. -pub const TENANT_HEATMAP_BASENAME: &str = "heatmap-v1.json"; +pub(crate) const TENANT_HEATMAP_BASENAME: &str = "heatmap-v1.json"; /// A suffix used for various temporary files. Any temporary files found in the /// data directory at pageserver startup can be automatically removed. -pub const TEMP_FILE_SUFFIX: &str = "___temp"; +pub(crate) const TEMP_FILE_SUFFIX: &str = "___temp"; /// A marker file to mark that a timeline directory was not fully initialized. /// If a timeline directory with this marker is encountered at pageserver startup, /// the timeline directory and the marker file are both removed. /// Full path: `tenants//timelines/___uninit`. -pub const TIMELINE_UNINIT_MARK_SUFFIX: &str = "___uninit"; +pub(crate) const TIMELINE_UNINIT_MARK_SUFFIX: &str = "___uninit"; -pub const TIMELINE_DELETE_MARK_SUFFIX: &str = "___delete"; +pub(crate) const TIMELINE_DELETE_MARK_SUFFIX: &str = "___delete"; /// A marker file to prevent pageserver from loading a certain tenant on restart. /// Different from [`TIMELINE_UNINIT_MARK_SUFFIX`] due to semantics of the corresponding @@ -161,11 +161,11 @@ fn ends_with_suffix(path: &Utf8Path, suffix: &str) -> bool { // from the directory name. Instead create type "UninitMark(TimelineId)" and only parse it once // from the name. -pub fn is_uninit_mark(path: &Utf8Path) -> bool { +pub(crate) fn is_uninit_mark(path: &Utf8Path) -> bool { ends_with_suffix(path, TIMELINE_UNINIT_MARK_SUFFIX) } -pub fn is_delete_mark(path: &Utf8Path) -> bool { +pub(crate) fn is_delete_mark(path: &Utf8Path) -> bool { ends_with_suffix(path, TIMELINE_DELETE_MARK_SUFFIX) } diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index f0996328c0..ddfb47369b 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -55,8 +55,8 @@ use self::mgr::GetTenantError; use self::mgr::TenantsMap; use self::remote_timeline_client::upload::upload_index_part; use self::remote_timeline_client::RemoteTimelineClient; +use self::timeline::uninit::TimelineCreateGuard; use self::timeline::uninit::TimelineExclusionError; -use self::timeline::uninit::TimelineUninitMark; use self::timeline::uninit::UninitializedTimeline; use self::timeline::EvictionTaskTenantState; use self::timeline::TimelineResources; @@ -565,9 +565,8 @@ impl Tenant { // avoiding holding it across awaits let mut timelines_accessor = self.timelines.lock().unwrap(); match timelines_accessor.entry(timeline_id) { + // We should never try and load the same timeline twice during startup Entry::Occupied(_) => { - // The uninit mark file acts as a lock that prevents another task from - // initializing the timeline at the same time. unreachable!( "Timeline {tenant_id}/{timeline_id} already exists in the tenant map" ); @@ -1064,8 +1063,7 @@ impl Tenant { let entry_path = entry.path(); let purge = if crate::is_temporary(entry_path) - // TODO: uninit_mark isn't needed any more, since uninitialized timelines are already - // covered by the check that the timeline must exist in remote storage. + // TODO: remove uninit mark code (https://github.com/neondatabase/neon/issues/5718) || is_uninit_mark(entry_path) || crate::is_delete_mark(entry_path) { @@ -1298,11 +1296,6 @@ impl Tenant { /// Until that happens, the on-disk state is invalid (disk_consistent_lsn=Lsn(0)) /// and the timeline will fail to load at a restart. /// - /// That's why we add an uninit mark file, and wrap it together witht the Timeline - /// in-memory object into UninitializedTimeline. - /// Once the caller is done setting up the timeline, they should call - /// `UninitializedTimeline::initialize_with_lock` to remove the uninit mark. - /// /// For tests, use `DatadirModification::init_empty_test_timeline` + `commit` to setup the /// minimum amount of keys required to get a writable timeline. /// (Without it, `put` might fail due to `repartition` failing.) @@ -1318,7 +1311,9 @@ impl Tenant { "Cannot create empty timelines on inactive tenant" ); - let timeline_uninit_mark = self.create_timeline_uninit_mark(new_timeline_id)?; + // Protect against concurrent attempts to use this TimelineId + let create_guard = self.create_timeline_create_guard(new_timeline_id)?; + let new_metadata = TimelineMetadata::new( // Initialize disk_consistent LSN to 0, The caller must import some data to // make it valid, before calling finish_creation() @@ -1333,7 +1328,7 @@ impl Tenant { self.prepare_new_timeline( new_timeline_id, &new_metadata, - timeline_uninit_mark, + create_guard, initdb_lsn, None, ) @@ -1421,9 +1416,8 @@ impl Tenant { .map_err(|_| CreateTimelineError::ShuttingDown)?; // Get exclusive access to the timeline ID: this ensures that it does not already exist, - // and that no other creation attempts will be allowed in while we are working. The - // uninit_mark is a guard. - let uninit_mark = match self.create_timeline_uninit_mark(new_timeline_id) { + // and that no other creation attempts will be allowed in while we are working. + let create_guard = match self.create_timeline_create_guard(new_timeline_id) { Ok(m) => m, Err(TimelineExclusionError::AlreadyCreating) => { // Creation is in progress, we cannot create it again, and we cannot @@ -1466,6 +1460,8 @@ impl Tenant { } }; + pausable_failpoint!("timeline-creation-after-uninit"); + let loaded_timeline = match ancestor_timeline_id { Some(ancestor_timeline_id) => { let ancestor_timeline = self @@ -1513,7 +1509,7 @@ impl Tenant { &ancestor_timeline, new_timeline_id, ancestor_start_lsn, - uninit_mark, + create_guard, ctx, ) .await? @@ -1523,7 +1519,7 @@ impl Tenant { new_timeline_id, pg_version, load_existing_initdb, - uninit_mark, + create_guard, ctx, ) .await? @@ -2870,9 +2866,9 @@ impl Tenant { start_lsn: Option, ctx: &RequestContext, ) -> Result, CreateTimelineError> { - let uninit_mark = self.create_timeline_uninit_mark(dst_id).unwrap(); + let create_guard = self.create_timeline_create_guard(dst_id).unwrap(); let tl = self - .branch_timeline_impl(src_timeline, dst_id, start_lsn, uninit_mark, ctx) + .branch_timeline_impl(src_timeline, dst_id, start_lsn, create_guard, ctx) .await?; tl.set_state(TimelineState::Active); Ok(tl) @@ -2886,10 +2882,10 @@ impl Tenant { src_timeline: &Arc, dst_id: TimelineId, start_lsn: Option, - timeline_uninit_mark: TimelineUninitMark<'_>, + timeline_create_guard: TimelineCreateGuard<'_>, ctx: &RequestContext, ) -> Result, CreateTimelineError> { - self.branch_timeline_impl(src_timeline, dst_id, start_lsn, timeline_uninit_mark, ctx) + self.branch_timeline_impl(src_timeline, dst_id, start_lsn, timeline_create_guard, ctx) .await } @@ -2898,7 +2894,7 @@ impl Tenant { src_timeline: &Arc, dst_id: TimelineId, start_lsn: Option, - timeline_uninit_mark: TimelineUninitMark<'_>, + timeline_create_guard: TimelineCreateGuard<'_>, _ctx: &RequestContext, ) -> Result, CreateTimelineError> { let src_id = src_timeline.timeline_id; @@ -2982,7 +2978,7 @@ impl Tenant { .prepare_new_timeline( dst_id, &metadata, - timeline_uninit_mark, + timeline_create_guard, start_lsn + 1, Some(Arc::clone(src_timeline)), ) @@ -3014,12 +3010,12 @@ impl Tenant { load_existing_initdb: Option, ctx: &RequestContext, ) -> anyhow::Result> { - let uninit_mark = self.create_timeline_uninit_mark(timeline_id).unwrap(); + let create_guard = self.create_timeline_create_guard(timeline_id).unwrap(); self.bootstrap_timeline( timeline_id, pg_version, load_existing_initdb, - uninit_mark, + create_guard, ctx, ) .await @@ -3083,7 +3079,7 @@ impl Tenant { timeline_id: TimelineId, pg_version: u32, load_existing_initdb: Option, - timeline_uninit_mark: TimelineUninitMark<'_>, + timeline_create_guard: TimelineCreateGuard<'_>, ctx: &RequestContext, ) -> anyhow::Result> { // create a `tenant/{tenant_id}/timelines/basebackup-{timeline_id}.{TEMP_FILE_SUFFIX}/` @@ -3095,13 +3091,14 @@ impl Tenant { TEMP_FILE_SUFFIX, ); - // an uninit mark was placed before, nothing else can access this timeline files - // current initdb was not run yet, so remove whatever was left from the previous runs + // Remove whatever was left from the previous runs: safe because TimelineCreateGuard guarantees + // we won't race with other creations or existent timelines with the same path. if pgdata_path.exists() { fs::remove_dir_all(&pgdata_path).with_context(|| { format!("Failed to remove already existing initdb directory: {pgdata_path}") })?; } + // this new directory is very temporary, set to remove it immediately after bootstrap, we don't need it scopeguard::defer! { if let Err(e) = fs::remove_dir_all(&pgdata_path) { @@ -3178,7 +3175,7 @@ impl Tenant { .prepare_new_timeline( timeline_id, &new_metadata, - timeline_uninit_mark, + timeline_create_guard, pgdata_lsn, None, ) @@ -3250,13 +3247,12 @@ impl Tenant { /// /// An empty layer map is initialized, and new data and WAL can be imported starting /// at 'disk_consistent_lsn'. After any initial data has been imported, call - /// `finish_creation` to insert the Timeline into the timelines map and to remove the - /// uninit mark file. + /// `finish_creation` to insert the Timeline into the timelines map. async fn prepare_new_timeline<'a>( &'a self, new_timeline_id: TimelineId, new_metadata: &TimelineMetadata, - uninit_mark: TimelineUninitMark<'a>, + create_guard: TimelineCreateGuard<'a>, start_lsn: Lsn, ancestor: Option>, ) -> anyhow::Result { @@ -3279,9 +3275,12 @@ impl Tenant { timeline_struct.init_empty_layer_map(start_lsn); - if let Err(e) = self.create_timeline_files(&uninit_mark.timeline_path).await { + if let Err(e) = self + .create_timeline_files(&create_guard.timeline_path) + .await + { error!("Failed to create initial files for timeline {tenant_shard_id}/{new_timeline_id}, cleaning up: {e:?}"); - cleanup_timeline_directory(uninit_mark); + cleanup_timeline_directory(create_guard); return Err(e); } @@ -3292,41 +3291,31 @@ impl Tenant { Ok(UninitializedTimeline::new( self, new_timeline_id, - Some((timeline_struct, uninit_mark)), + Some((timeline_struct, create_guard)), )) } async fn create_timeline_files(&self, timeline_path: &Utf8Path) -> anyhow::Result<()> { crashsafe::create_dir(timeline_path).context("Failed to create timeline directory")?; - fail::fail_point!("after-timeline-uninit-mark-creation", |_| { - anyhow::bail!("failpoint after-timeline-uninit-mark-creation"); + fail::fail_point!("after-timeline-dir-creation", |_| { + anyhow::bail!("failpoint after-timeline-dir-creation"); }); Ok(()) } - /// Attempts to create an uninit mark file for the timeline initialization. - /// Bails, if the timeline is already loaded into the memory (i.e. initialized before), or the uninit mark file already exists. - /// - /// This way, we need to hold the timelines lock only for small amount of time during the mark check/creation per timeline init. - fn create_timeline_uninit_mark( + /// Get a guard that provides exclusive access to the timeline directory, preventing + /// concurrent attempts to create the same timeline. + fn create_timeline_create_guard( &self, timeline_id: TimelineId, - ) -> Result { + ) -> Result { let tenant_shard_id = self.tenant_shard_id; - let uninit_mark_path = self - .conf - .timeline_uninit_mark_file_path(tenant_shard_id, timeline_id); let timeline_path = self.conf.timeline_path(&tenant_shard_id, &timeline_id); - let uninit_mark = TimelineUninitMark::new( - self, - timeline_id, - uninit_mark_path.clone(), - timeline_path.clone(), - )?; + let create_guard = TimelineCreateGuard::new(self, timeline_id, timeline_path.clone())?; // At this stage, we have got exclusive access to in-memory state for this timeline ID // for creation. @@ -3342,23 +3331,7 @@ impl Tenant { ))); } - // Create the on-disk uninit mark _after_ the in-memory acquisition of the tenant ID: guarantees - // that during process runtime, colliding creations will be caught in-memory without getting - // as far as failing to write a file. - fs::OpenOptions::new() - .write(true) - .create_new(true) - .open(&uninit_mark_path) - .context("Failed to create uninit mark file") - .and_then(|_| { - crashsafe::fsync_file_and_parent(&uninit_mark_path) - .context("Failed to fsync uninit mark file") - }) - .with_context(|| { - format!("Failed to crate uninit mark for timeline {tenant_shard_id}/{timeline_id}") - })?; - - Ok(uninit_mark) + Ok(create_guard) } /// Gathers inputs from all of the timelines to produce a sizing model input. @@ -5099,15 +5072,15 @@ mod tests { } #[tokio::test] - async fn test_uninit_mark_crash() -> anyhow::Result<()> { - let name = "test_uninit_mark_crash"; + async fn test_create_guard_crash() -> anyhow::Result<()> { + let name = "test_create_guard_crash"; let harness = TenantHarness::create(name)?; { let (tenant, ctx) = harness.load().await; let tline = tenant .create_empty_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx) .await?; - // Keeps uninit mark in place + // Leave the timeline ID in [`Tenant::timelines_creating`] to exclude attempting to create it again let raw_tline = tline.raw_timeline().unwrap(); raw_tline .shutdown() @@ -5135,11 +5108,6 @@ mod tests { .timeline_path(&tenant.tenant_shard_id, &TIMELINE_ID) .exists()); - assert!(!harness - .conf - .timeline_uninit_mark_file_path(tenant.tenant_shard_id, TIMELINE_ID) - .exists()); - Ok(()) } diff --git a/pageserver/src/tenant/timeline/uninit.rs b/pageserver/src/tenant/timeline/uninit.rs index 27d6fd9c28..e1034a9fe2 100644 --- a/pageserver/src/tenant/timeline/uninit.rs +++ b/pageserver/src/tenant/timeline/uninit.rs @@ -2,8 +2,8 @@ use std::{collections::hash_map::Entry, fs, sync::Arc}; use anyhow::Context; use camino::Utf8PathBuf; -use tracing::{error, info, info_span, warn}; -use utils::{crashsafe, fs_ext, id::TimelineId, lsn::Lsn}; +use tracing::{error, info, info_span}; +use utils::{fs_ext, id::TimelineId, lsn::Lsn}; use crate::{context::RequestContext, import_datadir, tenant::Tenant}; @@ -11,22 +11,22 @@ use super::Timeline; /// A timeline with some of its files on disk, being initialized. /// This struct ensures the atomicity of the timeline init: it's either properly created and inserted into pageserver's memory, or -/// its local files are removed. In the worst case of a crash, an uninit mark file is left behind, which causes the directory -/// to be removed on next restart. +/// its local files are removed. If we crash while this class exists, then the timeline's local +/// state is cleaned up during [`Tenant::clean_up_timelines`], because the timeline's content isn't in remote storage. /// /// The caller is responsible for proper timeline data filling before the final init. #[must_use] pub struct UninitializedTimeline<'t> { pub(crate) owning_tenant: &'t Tenant, timeline_id: TimelineId, - raw_timeline: Option<(Arc, TimelineUninitMark<'t>)>, + raw_timeline: Option<(Arc, TimelineCreateGuard<'t>)>, } impl<'t> UninitializedTimeline<'t> { pub(crate) fn new( owning_tenant: &'t Tenant, timeline_id: TimelineId, - raw_timeline: Option<(Arc, TimelineUninitMark<'t>)>, + raw_timeline: Option<(Arc, TimelineCreateGuard<'t>)>, ) -> Self { Self { owning_tenant, @@ -35,8 +35,7 @@ impl<'t> UninitializedTimeline<'t> { } } - /// Finish timeline creation: insert it into the Tenant's timelines map and remove the - /// uninit mark file. + /// Finish timeline creation: insert it into the Tenant's timelines map /// /// This function launches the flush loop if not already done. /// @@ -72,16 +71,9 @@ impl<'t> UninitializedTimeline<'t> { Entry::Vacant(v) => { // after taking here should be no fallible operations, because the drop guard will not // cleanup after and would block for example the tenant deletion - let (new_timeline, uninit_mark) = + let (new_timeline, _create_guard) = self.raw_timeline.take().expect("already checked"); - // this is the mutual exclusion between different retries to create the timeline; - // this should be an assertion. - uninit_mark.remove_uninit_mark().with_context(|| { - format!( - "Failed to remove uninit mark file for timeline {tenant_shard_id}/{timeline_id}" - ) - })?; v.insert(Arc::clone(&new_timeline)); new_timeline.maybe_spawn_flush_loop(); @@ -120,8 +112,7 @@ impl<'t> UninitializedTimeline<'t> { .await .context("Failed to flush after basebackup import")?; - // All the data has been imported. Insert the Timeline into the tenant's timelines - // map and remove the uninit mark file. + // All the data has been imported. Insert the Timeline into the tenant's timelines map let tl = self.finish_creation()?; tl.activate(broker_client, None, ctx); Ok(tl) @@ -143,37 +134,35 @@ impl<'t> UninitializedTimeline<'t> { impl Drop for UninitializedTimeline<'_> { fn drop(&mut self) { - if let Some((_, uninit_mark)) = self.raw_timeline.take() { + if let Some((_, create_guard)) = self.raw_timeline.take() { let _entered = info_span!("drop_uninitialized_timeline", tenant_id = %self.owning_tenant.tenant_shard_id.tenant_id, shard_id = %self.owning_tenant.tenant_shard_id.shard_slug(), timeline_id = %self.timeline_id).entered(); error!("Timeline got dropped without initializing, cleaning its files"); - cleanup_timeline_directory(uninit_mark); + cleanup_timeline_directory(create_guard); } } } -pub(crate) fn cleanup_timeline_directory(uninit_mark: TimelineUninitMark) { - let timeline_path = &uninit_mark.timeline_path; +pub(crate) fn cleanup_timeline_directory(create_guard: TimelineCreateGuard) { + let timeline_path = &create_guard.timeline_path; match fs_ext::ignore_absent_files(|| fs::remove_dir_all(timeline_path)) { Ok(()) => { - info!("Timeline dir {timeline_path:?} removed successfully, removing the uninit mark") + info!("Timeline dir {timeline_path:?} removed successfully") } Err(e) => { error!("Failed to clean up uninitialized timeline directory {timeline_path:?}: {e:?}") } } - drop(uninit_mark); // mark handles its deletion on drop, gets retained if timeline dir exists + // Having cleaned up, we can release this TimelineId in `[Tenant::timelines_creating]` to allow other + // timeline creation attempts under this TimelineId to proceed + drop(create_guard); } -/// An uninit mark file, created along the timeline dir to ensure the timeline either gets fully initialized and loaded into pageserver's memory, -/// or gets removed eventually. -/// -/// XXX: it's important to create it near the timeline dir, not inside it to ensure timeline dir gets removed first. +/// A guard for timeline creations in process: as long as this object exists, the timeline ID +/// is kept in `[Tenant::timelines_creating]` to exclude concurrent attempts to create the same timeline. #[must_use] -pub(crate) struct TimelineUninitMark<'t> { +pub(crate) struct TimelineCreateGuard<'t> { owning_tenant: &'t Tenant, timeline_id: TimelineId, - uninit_mark_deleted: bool, - uninit_mark_path: Utf8PathBuf, pub(crate) timeline_path: Utf8PathBuf, } @@ -190,11 +179,10 @@ pub(crate) enum TimelineExclusionError { Other(#[from] anyhow::Error), } -impl<'t> TimelineUninitMark<'t> { +impl<'t> TimelineCreateGuard<'t> { pub(crate) fn new( owning_tenant: &'t Tenant, timeline_id: TimelineId, - uninit_mark_path: Utf8PathBuf, timeline_path: Utf8PathBuf, ) -> Result { // Lock order: this is the only place we take both locks. During drop() we only @@ -214,56 +202,14 @@ impl<'t> TimelineUninitMark<'t> { Ok(Self { owning_tenant, timeline_id, - uninit_mark_deleted: false, - uninit_mark_path, timeline_path, }) } } - - fn remove_uninit_mark(mut self) -> anyhow::Result<()> { - if !self.uninit_mark_deleted { - self.delete_mark_file_if_present()?; - } - - Ok(()) - } - - fn delete_mark_file_if_present(&mut self) -> anyhow::Result<()> { - let uninit_mark_file = &self.uninit_mark_path; - let uninit_mark_parent = uninit_mark_file - .parent() - .with_context(|| format!("Uninit mark file {uninit_mark_file:?} has no parent"))?; - fs_ext::ignore_absent_files(|| fs::remove_file(uninit_mark_file)).with_context(|| { - format!("Failed to remove uninit mark file at path {uninit_mark_file:?}") - })?; - crashsafe::fsync(uninit_mark_parent).context("Failed to fsync uninit mark parent")?; - self.uninit_mark_deleted = true; - - Ok(()) - } } -impl Drop for TimelineUninitMark<'_> { +impl Drop for TimelineCreateGuard<'_> { fn drop(&mut self) { - if !self.uninit_mark_deleted { - if self.timeline_path.exists() { - error!( - "Uninit mark {} is not removed, timeline {} stays uninitialized", - self.uninit_mark_path, self.timeline_path - ) - } else { - // unblock later timeline creation attempts - warn!( - "Removing intermediate uninit mark file {}", - self.uninit_mark_path - ); - if let Err(e) = self.delete_mark_file_if_present() { - error!("Failed to remove the uninit mark file: {e}") - } - } - } - self.owning_tenant .timelines_creating .lock() diff --git a/test_runner/fixtures/pageserver/allowed_errors.py b/test_runner/fixtures/pageserver/allowed_errors.py index 8ff4341cc0..839d4166c7 100755 --- a/test_runner/fixtures/pageserver/allowed_errors.py +++ b/test_runner/fixtures/pageserver/allowed_errors.py @@ -55,7 +55,6 @@ DEFAULT_PAGESERVER_ALLOWED_ERRORS = ( # FIXME: These need investigation ".*manual_gc.*is_shutdown_requested\\(\\) called in an unexpected task or thread.*", ".*tenant_list: timeline is not found in remote index while it is present in the tenants registry.*", - ".*Removing intermediate uninit mark file.*", # Tenant::delete_timeline() can cause any of the four following errors. # FIXME: we shouldn't be considering it an error: https://github.com/neondatabase/neon/issues/2946 ".*could not flush frozen layer.*queue is in state Stopped", # when schedule layer upload fails because queued got closed before compaction got killed diff --git a/test_runner/fixtures/pageserver/http.py b/test_runner/fixtures/pageserver/http.py index 6e082374d7..4e355b73a9 100644 --- a/test_runner/fixtures/pageserver/http.py +++ b/test_runner/fixtures/pageserver/http.py @@ -34,7 +34,7 @@ class TimelineCreate406(PageserverApiException): class TimelineCreate409(PageserverApiException): def __init__(self, res: requests.Response): assert res.status_code == 409 - super().__init__("", res.status_code) + super().__init__(res.json()["msg"], res.status_code) @dataclass diff --git a/test_runner/regress/test_branching.py b/test_runner/regress/test_branching.py index 9a0b91b54e..2a7a3c41ac 100644 --- a/test_runner/regress/test_branching.py +++ b/test_runner/regress/test_branching.py @@ -347,6 +347,64 @@ def test_non_uploaded_branch_is_deleted_after_restart(neon_env_builder: NeonEnvB ps_http.timeline_detail(env.initial_tenant, branch_id) +def test_duplicate_creation(neon_env_builder: NeonEnvBuilder): + env = neon_env_builder.init_configs() + env.start() + env.pageserver.tenant_create(env.initial_tenant) + + success_timeline = TimelineId.generate() + log.info(f"Creating timeline {success_timeline}") + ps_http = env.pageserver.http_client() + success_result = ps_http.timeline_create( + env.pg_version, env.initial_tenant, success_timeline, timeout=60 + ) + + ps_http.configure_failpoints(("timeline-creation-after-uninit", "pause")) + + def start_creating_timeline(): + log.info(f"Creating (expect failure) timeline {env.initial_timeline}") + with pytest.raises(RequestException): + ps_http.timeline_create( + env.pg_version, env.initial_tenant, env.initial_timeline, timeout=60 + ) + + t = threading.Thread(target=start_creating_timeline) + try: + t.start() + + wait_until_paused(env, "timeline-creation-after-uninit") + + # While timeline creation is in progress, trying to create a timeline + # again with the same ID should return 409 + with pytest.raises( + PageserverApiException, match="creation of timeline with the given ID is in progress" + ): + ps_http.timeline_create( + env.pg_version, env.initial_tenant, env.initial_timeline, timeout=60 + ) + + # Creation of a timeline already successfully created is idempotent, and is not impeded by some + # other timeline creation with a different TimelineId being stuck. + repeat_result = ps_http.timeline_create( + env.pg_version, env.initial_tenant, success_timeline, timeout=60 + ) + assert repeat_result == success_result + finally: + env.pageserver.stop(immediate=True) + t.join() + + # now without a failpoint + env.pageserver.start() + + wait_until_tenant_active(ps_http, env.initial_tenant) + + with pytest.raises(PageserverApiException, match="not found"): + ps_http.timeline_detail(env.initial_tenant, env.initial_timeline) + + # The one successfully created timeline should still be there. + assert len(ps_http.timeline_list(tenant_id=env.initial_tenant)) == 1 + + def wait_until_paused(env: NeonEnv, failpoint: str): found = False msg = f"at failpoint {failpoint}" diff --git a/test_runner/regress/test_broken_timeline.py b/test_runner/regress/test_broken_timeline.py index b046ed7f1b..804ad135ce 100644 --- a/test_runner/regress/test_broken_timeline.py +++ b/test_runner/regress/test_broken_timeline.py @@ -204,7 +204,7 @@ def test_timeline_init_break_before_checkpoint_recreate( assert timeline_id == new_timeline_id -def test_timeline_create_break_after_uninit_mark(neon_env_builder: NeonEnvBuilder): +def test_timeline_create_break_after_dir_creation(neon_env_builder: NeonEnvBuilder): env = neon_env_builder.init_start() pageserver_http = env.pageserver.http_client() @@ -214,9 +214,9 @@ def test_timeline_create_break_after_uninit_mark(neon_env_builder: NeonEnvBuilde old_tenant_timelines = env.neon_cli.list_timelines(tenant_id) initial_timeline_dirs = [d for d in timelines_dir.iterdir()] - # Introduce failpoint when creating a new timeline uninit mark, before any other files were created - pageserver_http.configure_failpoints(("after-timeline-uninit-mark-creation", "return")) - with pytest.raises(Exception, match="after-timeline-uninit-mark-creation"): + # Introduce failpoint when creating a new timeline, right after creating its directory + pageserver_http.configure_failpoints(("after-timeline-dir-creation", "return")) + with pytest.raises(Exception, match="after-timeline-dir-creation"): _ = pageserver_http.timeline_create(PgVersion.NOT_SET, tenant_id, TimelineId.generate()) # Creating the timeline didn't finish. The other timelines on tenant should still be present and work normally. diff --git a/test_runner/regress/test_import.py b/test_runner/regress/test_import.py index ec57860033..132427ba2d 100644 --- a/test_runner/regress/test_import.py +++ b/test_runner/regress/test_import.py @@ -90,7 +90,6 @@ def test_import_from_vanilla(test_output_dir, pg_bin, vanilla_pg, neon_env_build [ ".*error importing base backup .*", ".*Timeline got dropped without initializing, cleaning its files.*", - ".*Removing intermediate uninit mark file.*", ".*InternalServerError.*timeline not found.*", ".*InternalServerError.*Tenant .* not found.*", ".*InternalServerError.*Timeline .* not found.*",