From b782b11b33b505ae16667fc0139b621c1885ff3a Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Fri, 25 Oct 2024 12:04:27 +0200 Subject: [PATCH] refactor(timeline creation): represent bootstrap vs branch using enum (#9366) # Problem Timeline creation can either be bootstrap or branch. The distinction is made based on whether the `ancestor_*` fields are present or not. In the PGDATA import code (https://github.com/neondatabase/neon/pull/9218), I add a third variant to timeline creation. # Solution The above pushed me to refactor the code in Pageserver to distinguish the different creation requests through enum variants. There is no externally observable effect from this change. On the implementation level, a notable change is that the acquisition of the `TimelineCreationGuard` happens later than before. This is necessary so that we have everything in place to construct the `CreateTimelineIdempotency`. Notably, this moves the acquisition of the creation guard _after_ the acquisition of the `gc_cs` lock in the case of branching. This might appear as if we're at risk of holding `gc_cs` longer than before this PR, but, even before this PR, we were holding `gc_cs` until after the `wait_completion()` that makes the timeline creation durable in S3 returns. I don't see any deadlock risk with reversing the lock acquisition order. As a drive-by change, I found that the `create_timeline()` function in `neon_local` is unused, so I removed it. # Refs * platform context: https://github.com/neondatabase/neon/pull/9218 * product context: https://github.com/neondatabase/cloud/issues/17507 * next PR stacked atop this one: https://github.com/neondatabase/neon/pull/9501 --- control_plane/src/bin/neon_local.rs | 25 +- control_plane/src/pageserver.rs | 22 -- libs/pageserver_api/src/models.rs | 31 +- pageserver/src/http/routes.rs | 42 ++- pageserver/src/tenant.rs | 371 +++++++++++++++++------- storage_controller/src/service.rs | 8 +- test_runner/fixtures/pageserver/http.py | 11 +- 7 files changed, 334 insertions(+), 176 deletions(-) diff --git a/control_plane/src/bin/neon_local.rs b/control_plane/src/bin/neon_local.rs index 624936620d..48438adf43 100644 --- a/control_plane/src/bin/neon_local.rs +++ b/control_plane/src/bin/neon_local.rs @@ -1073,10 +1073,10 @@ async fn handle_tenant(subcmd: &TenantCmd, env: &mut local_env::LocalEnv) -> any tenant_id, TimelineCreateRequest { new_timeline_id, - ancestor_timeline_id: None, - ancestor_start_lsn: None, - existing_initdb_timeline_id: None, - pg_version: Some(args.pg_version), + mode: pageserver_api::models::TimelineCreateRequestMode::Bootstrap { + existing_initdb_timeline_id: None, + pg_version: Some(args.pg_version), + }, }, ) .await?; @@ -1133,10 +1133,10 @@ async fn handle_timeline(cmd: &TimelineCmd, env: &mut local_env::LocalEnv) -> Re let storage_controller = StorageController::from_env(env); let create_req = TimelineCreateRequest { new_timeline_id, - ancestor_timeline_id: None, - existing_initdb_timeline_id: None, - ancestor_start_lsn: None, - pg_version: Some(args.pg_version), + mode: pageserver_api::models::TimelineCreateRequestMode::Bootstrap { + existing_initdb_timeline_id: None, + pg_version: Some(args.pg_version), + }, }; let timeline_info = storage_controller .tenant_timeline_create(tenant_id, create_req) @@ -1189,10 +1189,11 @@ async fn handle_timeline(cmd: &TimelineCmd, env: &mut local_env::LocalEnv) -> Re let storage_controller = StorageController::from_env(env); let create_req = TimelineCreateRequest { new_timeline_id, - ancestor_timeline_id: Some(ancestor_timeline_id), - existing_initdb_timeline_id: None, - ancestor_start_lsn: start_lsn, - pg_version: None, + mode: pageserver_api::models::TimelineCreateRequestMode::Branch { + ancestor_timeline_id, + ancestor_start_lsn: start_lsn, + pg_version: None, + }, }; let timeline_info = storage_controller .tenant_timeline_create(tenant_id, create_req) diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index cae9416af6..5b5828c6ed 100644 --- a/control_plane/src/pageserver.rs +++ b/control_plane/src/pageserver.rs @@ -529,28 +529,6 @@ impl PageServerNode { Ok(self.http_client.list_timelines(*tenant_shard_id).await?) } - pub async fn timeline_create( - &self, - tenant_shard_id: TenantShardId, - new_timeline_id: TimelineId, - ancestor_start_lsn: Option, - ancestor_timeline_id: Option, - pg_version: Option, - existing_initdb_timeline_id: Option, - ) -> anyhow::Result { - let req = models::TimelineCreateRequest { - new_timeline_id, - ancestor_start_lsn, - ancestor_timeline_id, - pg_version, - existing_initdb_timeline_id, - }; - Ok(self - .http_client - .timeline_create(tenant_shard_id, &req) - .await?) - } - /// Import a basebackup prepared using either: /// a) `pg_basebackup -F tar`, or /// b) The `fullbackup` pageserver endpoint diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index d0ee4b64d1..8684927554 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -211,13 +211,30 @@ pub enum TimelineState { #[derive(Serialize, Deserialize, Clone)] pub struct TimelineCreateRequest { pub new_timeline_id: TimelineId, - #[serde(default)] - pub ancestor_timeline_id: Option, - #[serde(default)] - pub existing_initdb_timeline_id: Option, - #[serde(default)] - pub ancestor_start_lsn: Option, - pub pg_version: Option, + #[serde(flatten)] + pub mode: TimelineCreateRequestMode, +} + +#[derive(Serialize, Deserialize, Clone)] +#[serde(untagged)] +pub enum TimelineCreateRequestMode { + Branch { + ancestor_timeline_id: TimelineId, + #[serde(default)] + ancestor_start_lsn: Option, + // TODO: cplane sets this, but, the branching code always + // inherits the ancestor's pg_version. Earlier code wasn't + // using a flattened enum, so, it was an accepted field, and + // we continue to accept it by having it here. + pg_version: Option, + }, + // NB: Bootstrap is all-optional, and thus the serde(untagged) will cause serde to stop at Bootstrap. + // (serde picks the first matching enum variant, in declaration order). + Bootstrap { + #[serde(default)] + existing_initdb_timeline_id: Option, + pg_version: Option, + }, } #[derive(Serialize, Deserialize, Clone)] diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs index 2490bf5f20..bc03df9ad2 100644 --- a/pageserver/src/http/routes.rs +++ b/pageserver/src/http/routes.rs @@ -38,6 +38,7 @@ use pageserver_api::models::TenantShardSplitRequest; use pageserver_api::models::TenantShardSplitResponse; use pageserver_api::models::TenantSorting; use pageserver_api::models::TimelineArchivalConfigRequest; +use pageserver_api::models::TimelineCreateRequestMode; use pageserver_api::models::TimelinesInfoAndOffloaded; use pageserver_api::models::TopTenantShardItem; use pageserver_api::models::TopTenantShardsRequest; @@ -85,6 +86,7 @@ use crate::tenant::timeline::Timeline; use crate::tenant::GetTimelineError; use crate::tenant::OffloadedTimeline; use crate::tenant::{LogicalSizeCalculationCause, PageReconstructError}; +use crate::DEFAULT_PG_VERSION; use crate::{disk_usage_eviction_task, tenant}; use pageserver_api::models::{ StatusResponse, TenantConfigRequest, TenantInfo, TimelineCreateRequest, TimelineGcRequest, @@ -547,6 +549,26 @@ async fn timeline_create_handler( check_permission(&request, Some(tenant_shard_id.tenant_id))?; let new_timeline_id = request_data.new_timeline_id; + // fill in the default pg_version if not provided & convert request into domain model + let params: tenant::CreateTimelineParams = match request_data.mode { + TimelineCreateRequestMode::Bootstrap { + existing_initdb_timeline_id, + pg_version, + } => tenant::CreateTimelineParams::Bootstrap(tenant::CreateTimelineParamsBootstrap { + new_timeline_id, + existing_initdb_timeline_id, + pg_version: pg_version.unwrap_or(DEFAULT_PG_VERSION), + }), + TimelineCreateRequestMode::Branch { + ancestor_timeline_id, + ancestor_start_lsn, + pg_version: _, + } => tenant::CreateTimelineParams::Branch(tenant::CreateTimelineParamsBranch { + new_timeline_id, + ancestor_timeline_id, + ancestor_start_lsn, + }), + }; let ctx = RequestContext::new(TaskKind::MgmtRequest, DownloadBehavior::Error); @@ -559,22 +581,12 @@ async fn timeline_create_handler( tenant.wait_to_become_active(ACTIVE_TENANT_TIMEOUT).await?; - if let Some(ancestor_id) = request_data.ancestor_timeline_id.as_ref() { - tracing::info!(%ancestor_id, "starting to branch"); - } else { - tracing::info!("bootstrapping"); - } + // earlier versions of the code had pg_version and ancestor_lsn in the span + // => continue to provide that information, but, through a log message that doesn't require us to destructure + tracing::info!(?params, "creating timeline"); match tenant - .create_timeline( - new_timeline_id, - request_data.ancestor_timeline_id, - request_data.ancestor_start_lsn, - request_data.pg_version.unwrap_or(crate::DEFAULT_PG_VERSION), - request_data.existing_initdb_timeline_id, - state.broker_client.clone(), - &ctx, - ) + .create_timeline(params, state.broker_client.clone(), &ctx) .await { Ok(new_timeline) => { @@ -625,8 +637,6 @@ async fn timeline_create_handler( tenant_id = %tenant_shard_id.tenant_id, shard_id = %tenant_shard_id.shard_slug(), timeline_id = %new_timeline_id, - lsn=?request_data.ancestor_start_lsn, - pg_version=?request_data.pg_version )) .await } diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index d503b299c1..d8ce916bcb 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -737,6 +737,83 @@ impl Debug for SetStoppingError { } } +/// Arguments to [`Tenant::create_timeline`]. +/// +/// Not usable as an idempotency key for timeline creation because if [`CreateTimelineParamsBranch::ancestor_start_lsn`] +/// is `None`, the result of the timeline create call is not deterministic. +/// +/// See [`CreateTimelineIdempotency`] for an idempotency key. +#[derive(Debug)] +pub(crate) enum CreateTimelineParams { + Bootstrap(CreateTimelineParamsBootstrap), + Branch(CreateTimelineParamsBranch), +} + +#[derive(Debug)] +pub(crate) struct CreateTimelineParamsBootstrap { + pub(crate) new_timeline_id: TimelineId, + pub(crate) existing_initdb_timeline_id: Option, + pub(crate) pg_version: u32, +} + +/// NB: See comment on [`CreateTimelineIdempotency::Branch`] for why there's no `pg_version` here. +#[derive(Debug)] +pub(crate) struct CreateTimelineParamsBranch { + pub(crate) new_timeline_id: TimelineId, + pub(crate) ancestor_timeline_id: TimelineId, + pub(crate) ancestor_start_lsn: Option, +} + +/// What is used to determine idempotency of a [`Tenant::create_timeline`] call. +/// +/// Unlike [`CreateTimelineParams`], ancestor LSN is fixed, so, branching will be at a deterministic LSN. +/// +/// We make some trade-offs though, e.g., [`CreateTimelineParamsBootstrap::existing_initdb_timeline_id`] +/// is not considered for idempotency. +/// +/// We can improve on this over time. +pub(crate) enum CreateTimelineIdempotency { + Bootstrap { + pg_version: u32, + }, + /// NB: branches always have the same `pg_version` as their ancestor. + /// While [`pageserver_api::models::TimelineCreateRequestMode::Branch::pg_version`] + /// exists as a field, and is set by cplane, it has always been ignored by pageserver when + /// determining the child branch pg_version. + Branch { + ancestor_timeline_id: TimelineId, + ancestor_start_lsn: Lsn, + }, +} + +/// What is returned by [`Tenant::start_creating_timeline`]. +#[must_use] +enum StartCreatingTimelineResult<'t> { + CreateGuard(TimelineCreateGuard<'t>), + Idempotent(Arc), +} + +/// What is returned by [`Tenant::create_timeline`]. +enum CreateTimelineResult { + Created(Arc), + Idempotent(Arc), +} + +impl CreateTimelineResult { + fn timeline(&self) -> &Arc { + match self { + Self::Created(t) | Self::Idempotent(t) => t, + } + } + /// Unit test timelines aren't activated, test has to do it if it needs to. + #[cfg(test)] + fn into_timeline_for_test(self) -> Arc { + match self { + Self::Created(t) | Self::Idempotent(t) => t, + } + } +} + #[derive(thiserror::Error, Debug)] pub enum CreateTimelineError { #[error("creation of timeline with the given ID is in progress")] @@ -2090,11 +2167,7 @@ impl Tenant { #[allow(clippy::too_many_arguments)] pub(crate) async fn create_timeline( self: &Arc, - new_timeline_id: TimelineId, - ancestor_timeline_id: Option, - mut ancestor_start_lsn: Option, - pg_version: u32, - load_existing_initdb: Option, + params: CreateTimelineParams, broker_client: storage_broker::BrokerClientChannel, ctx: &RequestContext, ) -> Result, CreateTimelineError> { @@ -2113,54 +2186,25 @@ impl Tenant { .enter() .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. - 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 - // check if this request matches the existing one, so caller must try - // again later. - return Err(CreateTimelineError::AlreadyCreating); + let result: CreateTimelineResult = match params { + CreateTimelineParams::Bootstrap(CreateTimelineParamsBootstrap { + new_timeline_id, + existing_initdb_timeline_id, + pg_version, + }) => { + self.bootstrap_timeline( + new_timeline_id, + pg_version, + existing_initdb_timeline_id, + ctx, + ) + .await? } - Err(TimelineExclusionError::Other(e)) => { - return Err(CreateTimelineError::Other(e)); - } - Err(TimelineExclusionError::AlreadyExists(existing)) => { - debug!("timeline {new_timeline_id} already exists"); - - // Idempotency: creating the same timeline twice is not an error, unless - // the second creation has different parameters. - if existing.get_ancestor_timeline_id() != ancestor_timeline_id - || existing.pg_version != pg_version - || (ancestor_start_lsn.is_some() - && ancestor_start_lsn != Some(existing.get_ancestor_lsn())) - { - return Err(CreateTimelineError::Conflict); - } - - // Wait for uploads to complete, so that when we return Ok, the timeline - // is known to be durable on remote storage. Just like we do at the end of - // this function, after we have created the timeline ourselves. - // - // We only really care that the initial version of `index_part.json` has - // been uploaded. That's enough to remember that the timeline - // exists. However, there is no function to wait specifically for that so - // we just wait for all in-progress uploads to finish. - existing - .remote_client - .wait_completion() - .await - .context("wait for timeline uploads to complete")?; - - return Ok(existing); - } - }; - - pausable_failpoint!("timeline-creation-after-uninit"); - - let loaded_timeline = match ancestor_timeline_id { - Some(ancestor_timeline_id) => { + CreateTimelineParams::Branch(CreateTimelineParamsBranch { + new_timeline_id, + ancestor_timeline_id, + mut ancestor_start_lsn, + }) => { let ancestor_timeline = self .get_timeline(ancestor_timeline_id, false) .context("Cannot branch off the timeline that's not present in pageserver")?; @@ -2207,43 +2251,39 @@ impl Tenant { })?; } - self.branch_timeline( - &ancestor_timeline, - new_timeline_id, - ancestor_start_lsn, - create_guard, - ctx, - ) - .await? - } - None => { - self.bootstrap_timeline( - new_timeline_id, - pg_version, - load_existing_initdb, - create_guard, - ctx, - ) - .await? + self.branch_timeline(&ancestor_timeline, new_timeline_id, ancestor_start_lsn, ctx) + .await? } }; // At this point we have dropped our guard on [`Self::timelines_creating`], and // the timeline is visible in [`Self::timelines`], but it is _not_ durable yet. We must // not send a success to the caller until it is. The same applies to handling retries, - // see the handling of [`TimelineExclusionError::AlreadyExists`] above. - let kind = ancestor_timeline_id - .map(|_| "branched") - .unwrap_or("bootstrapped"); - loaded_timeline + // that is done in [`Self::start_creating_timeline`]. + result + .timeline() .remote_client .wait_completion() .await - .with_context(|| format!("wait for {} timeline initial uploads to complete", kind))?; + .context("wait for timeline initial uploads to complete")?; - loaded_timeline.activate(self.clone(), broker_client, None, ctx); + // The creating task is responsible for activating the timeline. + // We do this after `wait_completion()` so that we don't spin up tasks that start + // doing stuff before the IndexPart is durable in S3, which is done by the previous section. + let activated_timeline = match result { + CreateTimelineResult::Created(timeline) => { + timeline.activate(self.clone(), broker_client, None, ctx); + timeline + } + CreateTimelineResult::Idempotent(timeline) => { + info!( + "request was deemed idempotent, activation will be done by the creating task" + ); + timeline + } + }; - Ok(loaded_timeline) + Ok(activated_timeline) } pub(crate) async fn delete_timeline( @@ -3747,16 +3787,16 @@ impl Tenant { /// timeline background tasks are launched, except the flush loop. #[cfg(test)] async fn branch_timeline_test( - &self, + self: &Arc, src_timeline: &Arc, dst_id: TimelineId, ancestor_lsn: Option, ctx: &RequestContext, ) -> Result, CreateTimelineError> { - let create_guard = self.create_timeline_create_guard(dst_id).unwrap(); let tl = self - .branch_timeline_impl(src_timeline, dst_id, ancestor_lsn, create_guard, ctx) - .await?; + .branch_timeline_impl(src_timeline, dst_id, ancestor_lsn, ctx) + .await? + .into_timeline_for_test(); tl.set_state(TimelineState::Active); Ok(tl) } @@ -3765,7 +3805,7 @@ impl Tenant { #[cfg(test)] #[allow(clippy::too_many_arguments)] pub async fn branch_timeline_test_with_layers( - &self, + self: &Arc, src_timeline: &Arc, dst_id: TimelineId, ancestor_lsn: Option, @@ -3813,28 +3853,24 @@ impl Tenant { } /// Branch an existing timeline. - /// - /// The caller is responsible for activating the returned timeline. async fn branch_timeline( - &self, + self: &Arc, src_timeline: &Arc, dst_id: TimelineId, start_lsn: Option, - timeline_create_guard: TimelineCreateGuard<'_>, ctx: &RequestContext, - ) -> Result, CreateTimelineError> { - self.branch_timeline_impl(src_timeline, dst_id, start_lsn, timeline_create_guard, ctx) + ) -> Result { + self.branch_timeline_impl(src_timeline, dst_id, start_lsn, ctx) .await } async fn branch_timeline_impl( - &self, + self: &Arc, src_timeline: &Arc, dst_id: TimelineId, start_lsn: Option, - timeline_create_guard: TimelineCreateGuard<'_>, _ctx: &RequestContext, - ) -> Result, CreateTimelineError> { + ) -> Result { let src_id = src_timeline.timeline_id; // We will validate our ancestor LSN in this function. Acquire the GC lock so that @@ -3849,6 +3885,23 @@ impl Tenant { lsn }); + // we finally have determined the ancestor_start_lsn, so we can get claim exclusivity now + let timeline_create_guard = match self + .start_creating_timeline( + dst_id, + CreateTimelineIdempotency::Branch { + ancestor_timeline_id: src_timeline.timeline_id, + ancestor_start_lsn: start_lsn, + }, + ) + .await? + { + StartCreatingTimelineResult::CreateGuard(guard) => guard, + StartCreatingTimelineResult::Idempotent(timeline) => { + return Ok(CreateTimelineResult::Idempotent(timeline)); + } + }; + // Ensure that `start_lsn` is valid, i.e. the LSN is within the PITR // horizon on the source timeline // @@ -3934,28 +3987,110 @@ impl Tenant { .schedule_index_upload_for_full_metadata_update(&metadata) .context("branch initial metadata upload")?; - Ok(new_timeline) + Ok(CreateTimelineResult::Created(new_timeline)) } /// For unit tests, make this visible so that other modules can directly create timelines #[cfg(test)] #[tracing::instrument(skip_all, fields(tenant_id=%self.tenant_shard_id.tenant_id, shard_id=%self.tenant_shard_id.shard_slug(), %timeline_id))] pub(crate) async fn bootstrap_timeline_test( - &self, + self: &Arc, timeline_id: TimelineId, pg_version: u32, load_existing_initdb: Option, ctx: &RequestContext, ) -> anyhow::Result> { - let create_guard = self.create_timeline_create_guard(timeline_id).unwrap(); - self.bootstrap_timeline( - timeline_id, - pg_version, - load_existing_initdb, - create_guard, - ctx, - ) - .await + self.bootstrap_timeline(timeline_id, pg_version, load_existing_initdb, ctx) + .await + .map_err(anyhow::Error::new) + .map(|r| r.into_timeline_for_test()) + } + + /// Get exclusive access to the timeline ID for creation. + /// + /// Timeline-creating code paths must use this function before making changes + /// to in-memory or persistent state. + /// + /// The `state` parameter is a description of the timeline creation operation + /// we intend to perform. + /// If the timeline was already created in the meantime, we check whether this + /// request conflicts or is idempotent , based on `state`. + async fn start_creating_timeline( + &self, + new_timeline_id: TimelineId, + idempotency: CreateTimelineIdempotency, + ) -> Result, CreateTimelineError> { + match self.create_timeline_create_guard(new_timeline_id) { + Ok(create_guard) => { + pausable_failpoint!("timeline-creation-after-uninit"); + Ok(StartCreatingTimelineResult::CreateGuard(create_guard)) + } + Err(TimelineExclusionError::AlreadyCreating) => { + // Creation is in progress, we cannot create it again, and we cannot + // check if this request matches the existing one, so caller must try + // again later. + Err(CreateTimelineError::AlreadyCreating) + } + Err(TimelineExclusionError::Other(e)) => Err(CreateTimelineError::Other(e)), + Err(TimelineExclusionError::AlreadyExists(existing)) => { + debug!("timeline already exists"); + + // Idempotency: creating the same timeline twice is not an error, unless + // the second creation has different parameters. + // + // TODO: this is a crutch; we should store the CreateTimelineState as an + // immutable attribute in the index part, and compare them using derive(`Eq`). + match idempotency { + CreateTimelineIdempotency::Bootstrap { pg_version } => { + if existing.pg_version != pg_version { + info!("timeline already exists with different pg_version"); + return Err(CreateTimelineError::Conflict); + } + if existing.get_ancestor_timeline_id().is_some() { + info!("timeline already exists with an ancestor"); + return Err(CreateTimelineError::Conflict); + } + if existing.get_ancestor_lsn() != Lsn::INVALID { + info!("timeline already exists with an ancestor LSN"); + return Err(CreateTimelineError::Conflict); + } + } + CreateTimelineIdempotency::Branch { + ancestor_timeline_id, + ancestor_start_lsn, + } => { + if existing.get_ancestor_timeline_id() != Some(ancestor_timeline_id) { + info!("timeline already exists with different ancestor"); + return Err(CreateTimelineError::Conflict); + } + if existing.get_ancestor_lsn() != ancestor_start_lsn { + info!("timeline already exists with different ancestor LSN"); + return Err(CreateTimelineError::Conflict); + } + } + } + + // Wait for uploads to complete, so that when we return Ok, the timeline + // is known to be durable on remote storage. Just like we do at the end of + // this function, after we have created the timeline ourselves. + // + // We only really care that the initial version of `index_part.json` has + // been uploaded. That's enough to remember that the timeline + // exists. However, there is no function to wait specifically for that so + // we just wait for all in-progress uploads to finish. + existing + .remote_client + .wait_completion() + .await + .context("wait for timeline uploads to complete")?; + + // TODO: shouldn't we also wait for timeline to become active? + // Code before this(https://github.com/neondatabase/neon/pull/9366) refactoring + // didn't do it. + + Ok(StartCreatingTimelineResult::Idempotent(existing)) + } + } } async fn upload_initdb( @@ -4009,16 +4144,26 @@ impl Tenant { /// - run initdb to init temporary instance and get bootstrap data /// - after initialization completes, tar up the temp dir and upload it to S3. - /// - /// The caller is responsible for activating the returned timeline. async fn bootstrap_timeline( - &self, + self: &Arc, timeline_id: TimelineId, pg_version: u32, load_existing_initdb: Option, - timeline_create_guard: TimelineCreateGuard<'_>, ctx: &RequestContext, - ) -> anyhow::Result> { + ) -> Result { + let timeline_create_guard = match self + .start_creating_timeline( + timeline_id, + CreateTimelineIdempotency::Bootstrap { pg_version }, + ) + .await? + { + StartCreatingTimelineResult::CreateGuard(guard) => guard, + StartCreatingTimelineResult::Idempotent(timeline) => { + return Ok(CreateTimelineResult::Idempotent(timeline)) + } + }; + // create a `tenant/{tenant_id}/timelines/basebackup-{timeline_id}.{TEMP_FILE_SUFFIX}/` // temporary directory for basebackup files for the given timeline. @@ -4082,7 +4227,9 @@ impl Tenant { .context("extract initdb tar")?; } else { // Init temporarily repo to get bootstrap data, this creates a directory in the `pgdata_path` path - run_initdb(self.conf, &pgdata_path, pg_version, &self.cancel).await?; + run_initdb(self.conf, &pgdata_path, pg_version, &self.cancel) + .await + .context("run initdb")?; // Upload the created data dir to S3 if self.tenant_shard_id().is_shard_zero() { @@ -4136,7 +4283,9 @@ impl Tenant { })?; fail::fail_point!("before-checkpoint-new-timeline", |_| { - anyhow::bail!("failpoint before-checkpoint-new-timeline"); + Err(CreateTimelineError::Other(anyhow::anyhow!( + "failpoint before-checkpoint-new-timeline" + ))) }); unfinished_timeline @@ -4151,7 +4300,7 @@ impl Tenant { // All done! let timeline = raw_timeline.finish_creation()?; - Ok(timeline) + Ok(CreateTimelineResult::Created(timeline)) } fn build_timeline_remote_client(&self, timeline_id: TimelineId) -> RemoteTimelineClient { diff --git a/storage_controller/src/service.rs b/storage_controller/src/service.rs index 2cde1d6a3d..a2a6e63dd2 100644 --- a/storage_controller/src/service.rs +++ b/storage_controller/src/service.rs @@ -3130,9 +3130,11 @@ impl Service { .await?; // Propagate the LSN that shard zero picked, if caller didn't provide one - if create_req.ancestor_timeline_id.is_some() && create_req.ancestor_start_lsn.is_none() - { - create_req.ancestor_start_lsn = timeline_info.ancestor_lsn; + match &mut create_req.mode { + models::TimelineCreateRequestMode::Branch { ancestor_start_lsn, .. } if ancestor_start_lsn.is_none() => { + *ancestor_start_lsn = timeline_info.ancestor_lsn; + }, + _ => {} } // Create timeline on remaining shards with number >0 diff --git a/test_runner/fixtures/pageserver/http.py b/test_runner/fixtures/pageserver/http.py index 18d65cb7de..db83c3ec89 100644 --- a/test_runner/fixtures/pageserver/http.py +++ b/test_runner/fixtures/pageserver/http.py @@ -476,12 +476,13 @@ class PageserverHttpClient(requests.Session, MetricsGetter): ) -> dict[Any, Any]: body: dict[str, Any] = { "new_timeline_id": str(new_timeline_id), - "ancestor_start_lsn": str(ancestor_start_lsn) if ancestor_start_lsn else None, - "ancestor_timeline_id": str(ancestor_timeline_id) if ancestor_timeline_id else None, - "existing_initdb_timeline_id": str(existing_initdb_timeline_id) - if existing_initdb_timeline_id - else None, } + if ancestor_timeline_id: + body["ancestor_timeline_id"] = str(ancestor_timeline_id) + if ancestor_start_lsn: + body["ancestor_start_lsn"] = str(ancestor_start_lsn) + if existing_initdb_timeline_id: + body["existing_initdb_timeline_id"] = str(existing_initdb_timeline_id) if pg_version != PgVersion.NOT_SET: body["pg_version"] = int(pg_version)