final bugfix

neondatabase · Oct 14, 2024 · dd1ed55 · dd1ed55
1 parent bfd72b0
commit dd1ed55
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 41 deletions.
diff --git a/control_plane/src/bin/neon_local.rs b/control_plane/src/bin/neon_local.rs
@@ -26,7 +26,7 @@ use pageserver_api::config::{
 use pageserver_api::controller_api::{
     NodeAvailabilityWrapper, PlacementPolicy, TenantCreateRequest,
 };
-use pageserver_api::models::{ShardParameters, TimelineCreateRequest, TimelineInfo};
+use pageserver_api::models::{ShardParameters, TimelineInfo};
 use pageserver_api::shard::{ShardCount, ShardStripeSize, TenantShardId};
 use postgres_backend::AuthType;
 use postgres_connection::parse_host_port;
@@ -1071,12 +1071,10 @@ async fn handle_tenant(subcmd: &TenantCmd, env: &mut local_env::LocalEnv) -> any
             storage_controller
                 .tenant_timeline_create(
                     tenant_id,
-                    TimelineCreateRequest {
+                    pageserver_api::models::TimelineCreateRequest::Bootstrap {
                         new_timeline_id,
-                        mode: pageserver_api::models::TimelineCreateRequestMode::Bootstrap {
-                            existing_initdb_timeline_id: None,
-                            pg_version: Some(args.pg_version),
-                        },
+                        existing_initdb_timeline_id: None,
+                        pg_version: Some(args.pg_version),
                     },
                 )
                 .await?;
@@ -1131,12 +1129,10 @@ async fn handle_timeline(cmd: &TimelineCmd, env: &mut local_env::LocalEnv) -> Re
             let new_timeline_id = new_timeline_id_opt.unwrap_or(TimelineId::generate());
 
             let storage_controller = StorageController::from_env(env);
-            let create_req = TimelineCreateRequest {
+            let create_req = pageserver_api::models::TimelineCreateRequest::Bootstrap {
                 new_timeline_id,
-                mode: pageserver_api::models::TimelineCreateRequestMode::Bootstrap {
-                    existing_initdb_timeline_id: None,
-                    pg_version: Some(args.pg_version),
-                },
+                existing_initdb_timeline_id: None,
+                pg_version: Some(args.pg_version),
             };
             let timeline_info = storage_controller
                 .tenant_timeline_create(tenant_id, create_req)
@@ -1187,13 +1183,11 @@ async fn handle_timeline(cmd: &TimelineCmd, env: &mut local_env::LocalEnv) -> Re
 
             let start_lsn = args.ancestor_start_lsn;
             let storage_controller = StorageController::from_env(env);
-            let create_req = TimelineCreateRequest {
+            let create_req = pageserver_api::models::TimelineCreateRequest::Branch {
                 new_timeline_id,
-                mode: pageserver_api::models::TimelineCreateRequestMode::Branch {
-                    ancestor_timeline_id,
-                    ancestor_start_lsn: start_lsn,
-                    pg_version: None,
-                },
+                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/storage_controller.rs b/control_plane/src/storage_controller.rs
@@ -851,7 +851,7 @@ impl StorageController {
             .await
     }
 
-    #[instrument(skip_all, fields(%tenant_id, timeline_id=%req.new_timeline_id))]
+    #[instrument(skip_all, fields(%tenant_id, timeline_id=%req.new_timeline_id()))]
     pub async fn tenant_timeline_create(
         &self,
         tenant_id: TenantId,

diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs
@@ -208,29 +208,38 @@ pub enum TimelineState {
     Broken { reason: String, backtrace: String },
 }
 
-#[derive(Serialize, Deserialize, Clone)]
-pub struct TimelineCreateRequest {
-    pub new_timeline_id: TimelineId,
-    #[serde(flatten)]
-    pub mode: TimelineCreateRequestMode,
-}
-
 #[derive(Serialize, Deserialize, Clone)]
 #[serde(untagged)]
-pub enum TimelineCreateRequestMode {
-    Bootstrap {
-        #[serde(default)]
-        existing_initdb_timeline_id: Option<TimelineId>,
-        pg_version: Option<u32>,
-    },
+pub enum TimelineCreateRequest {
     Branch {
+        new_timeline_id: TimelineId,
         ancestor_timeline_id: TimelineId,
         #[serde(default)]
         ancestor_start_lsn: Option<Lsn>,
         // TODO: cplane sets this, but, the current branching code always
         // inherits the ancestor's pg_version. This field is effectively ignored.
         pg_version: Option<u32>,
     },
+    // NB: ordered after Branch because serde(untagged) requires that
+    Bootstrap {
+        new_timeline_id: TimelineId,
+        #[serde(default)]
+        existing_initdb_timeline_id: Option<TimelineId>,
+        pg_version: Option<u32>,
+    },
+}
+
+impl TimelineCreateRequest {
+    pub fn new_timeline_id(&self) -> TimelineId {
+        match self {
+            TimelineCreateRequest::Bootstrap {
+                new_timeline_id, ..
+            } => *new_timeline_id,
+            TimelineCreateRequest::Branch {
+                new_timeline_id, ..
+            } => *new_timeline_id,
+        }
+    }
 }
 
 #[derive(Serialize, Deserialize, Clone)]

diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs
@@ -38,7 +38,6 @@ 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::TopTenantShardItem;
 use pageserver_api::models::TopTenantShardsRequest;
 use pageserver_api::models::TopTenantShardsResponse;
@@ -528,18 +527,20 @@ async fn timeline_create_handler(
     let request_data: TimelineCreateRequest = json_request(&mut request).await?;
     check_permission(&request, Some(tenant_shard_id.tenant_id))?;
 
-    let new_timeline_id = request_data.new_timeline_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 {
+    let params: tenant::CreateTimelineParams = match request_data {
+        TimelineCreateRequest::Bootstrap {
+            new_timeline_id,
             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 {
+        TimelineCreateRequest::Branch {
+            new_timeline_id,
             ancestor_timeline_id,
             ancestor_start_lsn,
             pg_version: _,

diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs
@@ -3599,6 +3599,8 @@ impl Tenant {
             src_timeline.pg_version,
         );
 
+        debug!(?metadata, "timeline metadata");
+
         let uninitialized_timeline = self
             .prepare_new_timeline(
                 dst_id,

diff --git a/storage_controller/src/service.rs b/storage_controller/src/service.rs
@@ -3035,7 +3035,7 @@ impl Service {
         tracing::info!(
             "Creating timeline {}/{}",
             tenant_id,
-            create_req.new_timeline_id,
+            create_req.new_timeline_id(),
         );
 
         let _tenant_lock = trace_shared_lock(
@@ -3068,7 +3068,7 @@ impl Service {
                 tracing::info!(
                     "Creating timeline on shard {}/{}, attached to node {latest} in generation {:?}",
                     tenant_shard_id,
-                    create_req.new_timeline_id,
+                    create_req.new_timeline_id(),
                     locations.latest.generation
                 );
 
@@ -3087,7 +3087,7 @@ impl Service {
                     tracing::info!(
                         "Creating timeline on shard {}/{}, stale attached to node {} in generation {:?}",
                         tenant_shard_id,
-                        create_req.new_timeline_id,
+                        create_req.new_timeline_id(),
                         location.node,
                         location.generation
                     );
@@ -3130,8 +3130,8 @@ impl Service {
             .await?;
 
             // Propagate the LSN that shard zero picked, if caller didn't provide one
-            match &mut create_req.mode {
-                models::TimelineCreateRequestMode::Branch { ancestor_start_lsn, .. } if ancestor_start_lsn.is_none() => {
+            match &mut create_req{
+                models::TimelineCreateRequest::Branch { ancestor_start_lsn, .. } if ancestor_start_lsn.is_none() => {
                     *ancestor_start_lsn = timeline_info.ancestor_lsn;
                 },
                 _ => {}