From 7329413705be0939b550553be2f40d4bb11a1a9b Mon Sep 17 00:00:00 2001 From: John Spray Date: Fri, 8 Mar 2024 15:34:53 +0000 Subject: [PATCH] storage controller: enable setting PlacementPolicy in tenant creation (#7037) ## Problem Tenants created via the storage controller have a `PlacementPolicy` that defines their HA/secondary/detach intent. For backward compat we can just set it to Single, for onboarding tenants using /location_conf it is automatically set to Double(1) if there are at least two pageservers, but for freshly created tenants we didn't have a way to specify it. This unblocks writing tests that create HA tenants on the storage controller and do failure injection testing. ## Summary of changes - Add optional fields to TenantCreateRequest for specifying PlacementPolicy. This request structure is used both on pageserver API and storage controller API, but this method is only meaningful for the storage controller (same as existing `shard_parameters` attribute). - Use the value from the creation request in tenant creation, if provided. --- control_plane/attachment_service/src/http.rs | 7 +-- control_plane/attachment_service/src/lib.rs | 25 +-------- .../attachment_service/src/persistence.rs | 11 ++-- .../attachment_service/src/service.rs | 55 ++++++++++--------- .../attachment_service/src/tenant_state.rs | 3 +- control_plane/src/bin/neon_local.rs | 9 ++- control_plane/src/pageserver.rs | 2 + libs/pageserver_api/src/controller_api.rs | 40 ++++++++++++++ libs/pageserver_api/src/models.rs | 6 ++ 9 files changed, 92 insertions(+), 66 deletions(-) diff --git a/control_plane/attachment_service/src/http.rs b/control_plane/attachment_service/src/http.rs index 384bdcef0c..7e4030b221 100644 --- a/control_plane/attachment_service/src/http.rs +++ b/control_plane/attachment_service/src/http.rs @@ -1,6 +1,5 @@ use crate::reconciler::ReconcileError; use crate::service::{Service, STARTUP_RECONCILE_TIMEOUT}; -use crate::PlacementPolicy; use hyper::{Body, Request, Response}; use hyper::{StatusCode, Uri}; use pageserver_api::models::{ @@ -119,13 +118,9 @@ async fn handle_tenant_create( let create_req = json_request::(&mut req).await?; - // TODO: enable specifying this. Using Single as a default helps legacy tests to work (they - // have no expectation of HA). - let placement_policy = PlacementPolicy::Single; - json_response( StatusCode::CREATED, - service.tenant_create(create_req, placement_policy).await?, + service.tenant_create(create_req).await?, ) } diff --git a/control_plane/attachment_service/src/lib.rs b/control_plane/attachment_service/src/lib.rs index 7ae7e264c7..796b465c10 100644 --- a/control_plane/attachment_service/src/lib.rs +++ b/control_plane/attachment_service/src/lib.rs @@ -1,4 +1,4 @@ -use serde::{Deserialize, Serialize}; +use serde::Serialize; use utils::seqwait::MonotonicCounter; mod auth; @@ -13,23 +13,6 @@ mod schema; pub mod service; mod tenant_state; -#[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq)] -enum PlacementPolicy { - /// Cheapest way to attach a tenant: just one pageserver, no secondary - Single, - /// Production-ready way to attach a tenant: one attached pageserver and - /// some number of secondaries. - Double(usize), - /// Create one secondary mode locations. This is useful when onboarding - /// a tenant, or for an idle tenant that we might want to bring online quickly. - Secondary, - - /// Do not attach to any pageservers. This is appropriate for tenants that - /// have been idle for a long time, where we do not mind some delay in making - /// them available in future. - Detached, -} - #[derive(Ord, PartialOrd, Eq, PartialEq, Copy, Clone, Serialize)] struct Sequence(u64); @@ -66,9 +49,3 @@ impl Sequence { Sequence(self.0 + 1) } } - -impl Default for PlacementPolicy { - fn default() -> Self { - PlacementPolicy::Double(1) - } -} diff --git a/control_plane/attachment_service/src/persistence.rs b/control_plane/attachment_service/src/persistence.rs index d5c304385c..d5c6d74ebe 100644 --- a/control_plane/attachment_service/src/persistence.rs +++ b/control_plane/attachment_service/src/persistence.rs @@ -7,11 +7,9 @@ use self::split_state::SplitState; use camino::Utf8Path; use camino::Utf8PathBuf; use diesel::pg::PgConnection; -use diesel::{ - Connection, ExpressionMethods, Insertable, QueryDsl, QueryResult, Queryable, RunQueryDsl, - Selectable, SelectableHelper, -}; -use pageserver_api::controller_api::NodeSchedulingPolicy; +use diesel::prelude::*; +use diesel::Connection; +use pageserver_api::controller_api::{NodeSchedulingPolicy, PlacementPolicy}; use pageserver_api::models::TenantConfig; use pageserver_api::shard::{ShardCount, ShardNumber, TenantShardId}; use serde::{Deserialize, Serialize}; @@ -19,7 +17,6 @@ use utils::generation::Generation; use utils::id::{NodeId, TenantId}; use crate::node::Node; -use crate::PlacementPolicy; /// ## What do we store? /// @@ -210,7 +207,7 @@ impl Persistence { tenant.tenant_id = tenant_id.to_string(); tenant.config = serde_json::to_string(&TenantConfig::default()) .map_err(|e| DatabaseError::Logical(format!("Serialization error: {e}")))?; - tenant.placement_policy = serde_json::to_string(&PlacementPolicy::default()) + tenant.placement_policy = serde_json::to_string(&PlacementPolicy::Single) .map_err(|e| DatabaseError::Logical(format!("Serialization error: {e}")))?; } } diff --git a/control_plane/attachment_service/src/service.rs b/control_plane/attachment_service/src/service.rs index f41c4f89b9..556d6a6828 100644 --- a/control_plane/attachment_service/src/service.rs +++ b/control_plane/attachment_service/src/service.rs @@ -16,9 +16,9 @@ use futures::{stream::FuturesUnordered, StreamExt}; use hyper::StatusCode; use pageserver_api::{ controller_api::{ - NodeAvailability, NodeConfigureRequest, NodeRegisterRequest, TenantCreateResponse, - TenantCreateResponseShard, TenantLocateResponse, TenantShardMigrateRequest, - TenantShardMigrateResponse, + NodeAvailability, NodeConfigureRequest, NodeRegisterRequest, PlacementPolicy, + TenantCreateResponse, TenantCreateResponseShard, TenantLocateResponse, + TenantShardMigrateRequest, TenantShardMigrateResponse, }, models::TenantConfigRequest, }; @@ -57,7 +57,7 @@ use crate::{ IntentState, ObservedState, ObservedStateLocation, ReconcileResult, ReconcileWaitError, ReconcilerWaiter, TenantState, }, - PlacementPolicy, Sequence, + Sequence, }; // For operations that should be quick, like attaching a new tenant @@ -176,7 +176,7 @@ impl From for ApiError { #[allow(clippy::large_enum_variant)] enum TenantCreateOrUpdate { - Create((TenantCreateRequest, PlacementPolicy)), + Create(TenantCreateRequest), Update(Vec), } @@ -792,7 +792,7 @@ impl Service { shard_stripe_size: 0, generation: Some(0), generation_pageserver: None, - placement_policy: serde_json::to_string(&PlacementPolicy::default()).unwrap(), + placement_policy: serde_json::to_string(&PlacementPolicy::Single).unwrap(), config: serde_json::to_string(&TenantConfig::default()).unwrap(), splitting: SplitState::default(), }; @@ -1053,9 +1053,8 @@ impl Service { pub(crate) async fn tenant_create( &self, create_req: TenantCreateRequest, - placement_policy: PlacementPolicy, ) -> Result { - let (response, waiters) = self.do_tenant_create(create_req, placement_policy).await?; + let (response, waiters) = self.do_tenant_create(create_req).await?; self.await_waiters(waiters, SHORT_RECONCILE_TIMEOUT).await?; Ok(response) @@ -1064,8 +1063,13 @@ impl Service { pub(crate) async fn do_tenant_create( &self, create_req: TenantCreateRequest, - placement_policy: PlacementPolicy, ) -> Result<(TenantCreateResponse, Vec), ApiError> { + // As a default, single is convenient for tests that don't choose a policy. + let placement_policy = create_req + .placement_policy + .clone() + .unwrap_or(PlacementPolicy::Single); + // This service expects to handle sharding itself: it is an error to try and directly create // a particular shard here. let tenant_id = if !create_req.new_tenant_id.is_unsharded() { @@ -1339,22 +1343,20 @@ impl Service { TenantCreateOrUpdate::Create( // Synthesize a creation request - ( - TenantCreateRequest { - new_tenant_id: TenantShardId::unsharded(tenant_id), - generation, - shard_parameters: ShardParameters { - // Must preserve the incoming shard_count do distinguish unsharded (0) - // from single-sharded (1): this distinction appears in the S3 keys of the tenant. - count: req.tenant_id.shard_count, - // We only import un-sharded or single-sharded tenants, so stripe - // size can be made up arbitrarily here. - stripe_size: ShardParameters::DEFAULT_STRIPE_SIZE, - }, - config: req.config.tenant_conf, + TenantCreateRequest { + new_tenant_id: TenantShardId::unsharded(tenant_id), + generation, + shard_parameters: ShardParameters { + // Must preserve the incoming shard_count do distinguish unsharded (0) + // from single-sharded (1): this distinction appears in the S3 keys of the tenant. + count: req.tenant_id.shard_count, + // We only import un-sharded or single-sharded tenants, so stripe + // size can be made up arbitrarily here. + stripe_size: ShardParameters::DEFAULT_STRIPE_SIZE, }, - placement_policy, - ), + placement_policy: Some(placement_policy), + config: req.config.tenant_conf, + }, ) } else { TenantCreateOrUpdate::Update(updates) @@ -1393,9 +1395,8 @@ impl Service { stripe_size: None, }; let waiters = match create_or_update { - TenantCreateOrUpdate::Create((create_req, placement_policy)) => { - let (create_resp, waiters) = - self.do_tenant_create(create_req, placement_policy).await?; + TenantCreateOrUpdate::Create(create_req) => { + let (create_resp, waiters) = self.do_tenant_create(create_req).await?; result.shards = create_resp .shards .into_iter() diff --git a/control_plane/attachment_service/src/tenant_state.rs b/control_plane/attachment_service/src/tenant_state.rs index ddb9866527..c775736b31 100644 --- a/control_plane/attachment_service/src/tenant_state.rs +++ b/control_plane/attachment_service/src/tenant_state.rs @@ -5,6 +5,7 @@ use std::{ }; use crate::{metrics, persistence::TenantShardPersistence}; +use pageserver_api::controller_api::PlacementPolicy; use pageserver_api::{ models::{LocationConfig, LocationConfigMode, TenantConfig}, shard::{ShardIdentity, TenantShardId}, @@ -28,7 +29,7 @@ use crate::{ attached_location_conf, secondary_location_conf, ReconcileError, Reconciler, TargetState, }, scheduler::{ScheduleError, Scheduler}, - service, PlacementPolicy, Sequence, + service, Sequence, }; /// Serialization helper diff --git a/control_plane/src/bin/neon_local.rs b/control_plane/src/bin/neon_local.rs index 1feec5cd9b..27abcb182a 100644 --- a/control_plane/src/bin/neon_local.rs +++ b/control_plane/src/bin/neon_local.rs @@ -15,7 +15,7 @@ use control_plane::pageserver::{PageServerNode, PAGESERVER_REMOTE_STORAGE_DIR}; use control_plane::safekeeper::SafekeeperNode; use control_plane::{broker, local_env}; use pageserver_api::controller_api::{ - NodeAvailability, NodeConfigureRequest, NodeSchedulingPolicy, + NodeAvailability, NodeConfigureRequest, NodeSchedulingPolicy, PlacementPolicy, }; use pageserver_api::models::{ ShardParameters, TenantCreateRequest, TimelineCreateRequest, TimelineInfo, @@ -435,6 +435,11 @@ async fn handle_tenant( let shard_stripe_size: Option = create_match.get_one::("shard-stripe-size").cloned(); + let placement_policy = match create_match.get_one::("placement-policy") { + Some(s) if !s.is_empty() => serde_json::from_str::(s)?, + _ => PlacementPolicy::Single, + }; + let tenant_conf = PageServerNode::parse_config(tenant_conf)?; // If tenant ID was not specified, generate one @@ -456,6 +461,7 @@ async fn handle_tenant( .map(ShardStripeSize) .unwrap_or(ShardParameters::DEFAULT_STRIPE_SIZE), }, + placement_policy: Some(placement_policy), config: tenant_conf, }) .await?; @@ -1562,6 +1568,7 @@ fn cli() -> Command { .help("Use this tenant in future CLI commands where tenant_id is needed, but not specified")) .arg(Arg::new("shard-count").value_parser(value_parser!(u8)).long("shard-count").action(ArgAction::Set).help("Number of shards in the new tenant (default 1)")) .arg(Arg::new("shard-stripe-size").value_parser(value_parser!(u32)).long("shard-stripe-size").action(ArgAction::Set).help("Sharding stripe size in pages")) + .arg(Arg::new("placement-policy").value_parser(value_parser!(String)).long("placement-policy").action(ArgAction::Set).help("Placement policy shards in this tenant")) ) .subcommand(Command::new("set-default").arg(tenant_id_arg.clone().required(true)) .about("Set a particular tenant as default in future CLI commands where tenant_id is needed, but not specified")) diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index b2904c1191..ae1bd60c52 100644 --- a/control_plane/src/pageserver.rs +++ b/control_plane/src/pageserver.rs @@ -429,6 +429,8 @@ impl PageServerNode { generation, config, shard_parameters: ShardParameters::default(), + // Placement policy is not meaningful for creations not done via storage controller + placement_policy: None, }; if !settings.is_empty() { bail!("Unrecognized tenant settings: {settings:?}") diff --git a/libs/pageserver_api/src/controller_api.rs b/libs/pageserver_api/src/controller_api.rs index 64b70a1a51..38e61239c5 100644 --- a/libs/pageserver_api/src/controller_api.rs +++ b/libs/pageserver_api/src/controller_api.rs @@ -125,5 +125,45 @@ impl From for String { } } +/// Controls how tenant shards are mapped to locations on pageservers, e.g. whether +/// to create secondary locations. +#[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq)] +pub enum PlacementPolicy { + /// Cheapest way to attach a tenant: just one pageserver, no secondary + Single, + /// Production-ready way to attach a tenant: one attached pageserver and + /// some number of secondaries. + Double(usize), + /// Create one secondary mode locations. This is useful when onboarding + /// a tenant, or for an idle tenant that we might want to bring online quickly. + Secondary, + + /// Do not attach to any pageservers. This is appropriate for tenants that + /// have been idle for a long time, where we do not mind some delay in making + /// them available in future. + Detached, +} + #[derive(Serialize, Deserialize, Debug)] pub struct TenantShardMigrateResponse {} + +#[cfg(test)] +mod test { + use super::*; + use serde_json; + + /// Check stability of PlacementPolicy's serialization + #[test] + fn placement_policy_encoding() -> anyhow::Result<()> { + let v = PlacementPolicy::Double(1); + let encoded = serde_json::to_string(&v)?; + assert_eq!(encoded, "{\"Double\":1}"); + assert_eq!(serde_json::from_str::(&encoded)?, v); + + let v = PlacementPolicy::Single; + let encoded = serde_json::to_string(&v)?; + assert_eq!(encoded, "\"Single\""); + assert_eq!(serde_json::from_str::(&encoded)?, v); + Ok(()) + } +} diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index 57497e3831..fe5bbd1c06 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -21,6 +21,7 @@ use utils::{ lsn::Lsn, }; +use crate::controller_api::PlacementPolicy; use crate::{ reltag::RelTag, shard::{ShardCount, ShardStripeSize, TenantShardId}, @@ -242,6 +243,11 @@ pub struct TenantCreateRequest { #[serde(skip_serializing_if = "ShardParameters::is_unsharded")] pub shard_parameters: ShardParameters, + // This parameter is only meaningful in requests sent to the storage controller + #[serde(default)] + #[serde(skip_serializing_if = "Option::is_none")] + pub placement_policy: Option, + #[serde(flatten)] pub config: TenantConfig, // as we have a flattened field, we should reject all unknown fields in it }