From a8ec18c0f4ca9d5b31333d00cd30cf8b0053ee9e Mon Sep 17 00:00:00 2001 From: John Spray Date: Tue, 27 Feb 2024 17:24:01 +0000 Subject: [PATCH] refactor: move storage controller API structs into pageserver_api (#6927) ## Problem This is a precursor to adding a convenience CLI for the storage controller. ## Summary of changes - move controller api structs into pageserver_api::controller_api to make them visible to other crates - rename pageserver_api::control_api to pageserver_api::upcall_api to match the /upcall/v1/ naming in the storage controller. Why here rather than a totally separate crate? It's convenient to have all the pageserver-related stuff in one place, and if we ever wanted to move it to a different crate it's super easy to do that later. --- control_plane/attachment_service/src/http.rs | 10 +- control_plane/attachment_service/src/node.rs | 2 +- .../attachment_service/src/persistence.rs | 2 +- .../attachment_service/src/reconciler.rs | 2 +- .../attachment_service/src/scheduler.rs | 2 +- .../attachment_service/src/service.rs | 18 +-- .../attachment_service/src/tenant_state.rs | 2 +- control_plane/src/attachment_service.rs | 126 +---------------- control_plane/src/bin/neon_local.rs | 7 +- control_plane/src/pageserver.rs | 3 +- libs/pageserver_api/src/controller_api.rs | 129 ++++++++++++++++++ libs/pageserver_api/src/lib.rs | 5 +- .../src/{control_api.rs => upcall_api.rs} | 0 pageserver/src/control_plane_client.rs | 4 +- 14 files changed, 165 insertions(+), 147 deletions(-) create mode 100644 libs/pageserver_api/src/controller_api.rs rename libs/pageserver_api/src/{control_api.rs => upcall_api.rs} (100%) diff --git a/control_plane/attachment_service/src/http.rs b/control_plane/attachment_service/src/http.rs index d341187ef7..f1153c2c18 100644 --- a/control_plane/attachment_service/src/http.rs +++ b/control_plane/attachment_service/src/http.rs @@ -25,12 +25,12 @@ use utils::{ id::NodeId, }; -use pageserver_api::control_api::{ReAttachRequest, ValidateRequest}; - -use control_plane::attachment_service::{ - AttachHookRequest, InspectRequest, NodeConfigureRequest, NodeRegisterRequest, - TenantShardMigrateRequest, +use pageserver_api::controller_api::{ + NodeConfigureRequest, NodeRegisterRequest, TenantShardMigrateRequest, }; +use pageserver_api::upcall_api::{ReAttachRequest, ValidateRequest}; + +use control_plane::attachment_service::{AttachHookRequest, InspectRequest}; /// State available to HTTP request handlers #[derive(Clone)] diff --git a/control_plane/attachment_service/src/node.rs b/control_plane/attachment_service/src/node.rs index 09162701ac..1f9dcef033 100644 --- a/control_plane/attachment_service/src/node.rs +++ b/control_plane/attachment_service/src/node.rs @@ -1,4 +1,4 @@ -use control_plane::attachment_service::{NodeAvailability, NodeSchedulingPolicy}; +use pageserver_api::controller_api::{NodeAvailability, NodeSchedulingPolicy}; use serde::Serialize; use utils::id::NodeId; diff --git a/control_plane/attachment_service/src/persistence.rs b/control_plane/attachment_service/src/persistence.rs index 4f336093cf..1b98cc7655 100644 --- a/control_plane/attachment_service/src/persistence.rs +++ b/control_plane/attachment_service/src/persistence.rs @@ -6,10 +6,10 @@ use std::time::Duration; use self::split_state::SplitState; use camino::Utf8Path; use camino::Utf8PathBuf; -use control_plane::attachment_service::NodeSchedulingPolicy; use diesel::pg::PgConnection; use diesel::prelude::*; use diesel::Connection; +use pageserver_api::controller_api::NodeSchedulingPolicy; use pageserver_api::models::TenantConfig; use pageserver_api::shard::{ShardCount, ShardNumber, TenantShardId}; use serde::{Deserialize, Serialize}; diff --git a/control_plane/attachment_service/src/reconciler.rs b/control_plane/attachment_service/src/reconciler.rs index 751b06f93a..ce91c1f5e9 100644 --- a/control_plane/attachment_service/src/reconciler.rs +++ b/control_plane/attachment_service/src/reconciler.rs @@ -1,6 +1,6 @@ use crate::persistence::Persistence; use crate::service; -use control_plane::attachment_service::NodeAvailability; +use pageserver_api::controller_api::NodeAvailability; use pageserver_api::models::{ LocationConfig, LocationConfigMode, LocationConfigSecondary, TenantConfig, }; diff --git a/control_plane/attachment_service/src/scheduler.rs b/control_plane/attachment_service/src/scheduler.rs index 7059071bee..3224751e47 100644 --- a/control_plane/attachment_service/src/scheduler.rs +++ b/control_plane/attachment_service/src/scheduler.rs @@ -255,7 +255,7 @@ impl Scheduler { pub(crate) mod test_utils { use crate::node::Node; - use control_plane::attachment_service::{NodeAvailability, NodeSchedulingPolicy}; + use pageserver_api::controller_api::{NodeAvailability, NodeSchedulingPolicy}; use std::collections::HashMap; use utils::id::NodeId; /// Test helper: synthesize the requested number of nodes, all in active state. diff --git a/control_plane/attachment_service/src/service.rs b/control_plane/attachment_service/src/service.rs index 8a80d0c746..02c1a65545 100644 --- a/control_plane/attachment_service/src/service.rs +++ b/control_plane/attachment_service/src/service.rs @@ -9,19 +9,17 @@ use std::{ use anyhow::Context; use control_plane::attachment_service::{ - AttachHookRequest, AttachHookResponse, InspectRequest, InspectResponse, NodeAvailability, - NodeConfigureRequest, NodeRegisterRequest, NodeSchedulingPolicy, TenantCreateResponse, - TenantCreateResponseShard, TenantLocateResponse, TenantLocateResponseShard, - TenantShardMigrateRequest, TenantShardMigrateResponse, + AttachHookRequest, AttachHookResponse, InspectRequest, InspectResponse, }; use diesel::result::DatabaseErrorKind; use futures::{stream::FuturesUnordered, StreamExt}; use hyper::StatusCode; +use pageserver_api::controller_api::{ + NodeAvailability, NodeConfigureRequest, NodeRegisterRequest, NodeSchedulingPolicy, + TenantCreateResponse, TenantCreateResponseShard, TenantLocateResponse, + TenantLocateResponseShard, TenantShardMigrateRequest, TenantShardMigrateResponse, +}; use pageserver_api::{ - control_api::{ - ReAttachRequest, ReAttachResponse, ReAttachResponseTenant, ValidateRequest, - ValidateResponse, ValidateResponseTenant, - }, models::{ self, LocationConfig, LocationConfigListResponse, LocationConfigMode, ShardParameters, TenantConfig, TenantCreateRequest, TenantLocationConfigRequest, @@ -29,6 +27,10 @@ use pageserver_api::{ TenantShardSplitResponse, TenantTimeTravelRequest, TimelineCreateRequest, TimelineInfo, }, shard::{ShardCount, ShardIdentity, ShardNumber, ShardStripeSize, TenantShardId}, + upcall_api::{ + ReAttachRequest, ReAttachResponse, ReAttachResponseTenant, ValidateRequest, + ValidateResponse, ValidateResponseTenant, + }, }; use pageserver_client::mgmt_api; use tokio_util::sync::CancellationToken; diff --git a/control_plane/attachment_service/src/tenant_state.rs b/control_plane/attachment_service/src/tenant_state.rs index 02f0171c29..c14fe6699e 100644 --- a/control_plane/attachment_service/src/tenant_state.rs +++ b/control_plane/attachment_service/src/tenant_state.rs @@ -1,7 +1,7 @@ use std::{collections::HashMap, sync::Arc, time::Duration}; use crate::{metrics, persistence::TenantShardPersistence}; -use control_plane::attachment_service::NodeAvailability; +use pageserver_api::controller_api::NodeAvailability; use pageserver_api::{ models::{LocationConfig, LocationConfigMode, TenantConfig}, shard::{ShardIdentity, TenantShardId}, diff --git a/control_plane/src/attachment_service.rs b/control_plane/src/attachment_service.rs index f0bee1ce08..0c416267fb 100644 --- a/control_plane/src/attachment_service.rs +++ b/control_plane/src/attachment_service.rs @@ -2,8 +2,12 @@ use crate::{background_process, local_env::LocalEnv}; use camino::{Utf8Path, Utf8PathBuf}; use hyper::Method; use pageserver_api::{ + controller_api::{ + NodeConfigureRequest, NodeRegisterRequest, TenantCreateResponse, TenantLocateResponse, + TenantShardMigrateRequest, TenantShardMigrateResponse, + }, models::{ - ShardParameters, TenantCreateRequest, TenantShardSplitRequest, TenantShardSplitResponse, + TenantCreateRequest, TenantShardSplitRequest, TenantShardSplitResponse, TimelineCreateRequest, TimelineInfo, }, shard::TenantShardId, @@ -55,126 +59,6 @@ pub struct InspectResponse { pub attachment: Option<(u32, NodeId)>, } -#[derive(Serialize, Deserialize)] -pub struct TenantCreateResponseShard { - pub shard_id: TenantShardId, - pub node_id: NodeId, - pub generation: u32, -} - -#[derive(Serialize, Deserialize)] -pub struct TenantCreateResponse { - pub shards: Vec, -} - -#[derive(Serialize, Deserialize)] -pub struct NodeRegisterRequest { - pub node_id: NodeId, - - pub listen_pg_addr: String, - pub listen_pg_port: u16, - - pub listen_http_addr: String, - pub listen_http_port: u16, -} - -#[derive(Serialize, Deserialize)] -pub struct NodeConfigureRequest { - pub node_id: NodeId, - - pub availability: Option, - pub scheduling: Option, -} - -#[derive(Serialize, Deserialize, Debug)] -pub struct TenantLocateResponseShard { - pub shard_id: TenantShardId, - pub node_id: NodeId, - - pub listen_pg_addr: String, - pub listen_pg_port: u16, - - pub listen_http_addr: String, - pub listen_http_port: u16, -} - -#[derive(Serialize, Deserialize)] -pub struct TenantLocateResponse { - pub shards: Vec, - pub shard_params: ShardParameters, -} - -/// Explicitly migrating a particular shard is a low level operation -/// TODO: higher level "Reschedule tenant" operation where the request -/// specifies some constraints, e.g. asking it to get off particular node(s) -#[derive(Serialize, Deserialize, Debug)] -pub struct TenantShardMigrateRequest { - pub tenant_shard_id: TenantShardId, - pub node_id: NodeId, -} - -#[derive(Serialize, Deserialize, Clone, Copy, Eq, PartialEq)] -pub enum NodeAvailability { - // Normal, happy state - Active, - // Offline: Tenants shouldn't try to attach here, but they may assume that their - // secondary locations on this node still exist. Newly added nodes are in this - // state until we successfully contact them. - Offline, -} - -impl FromStr for NodeAvailability { - type Err = anyhow::Error; - - fn from_str(s: &str) -> Result { - match s { - "active" => Ok(Self::Active), - "offline" => Ok(Self::Offline), - _ => Err(anyhow::anyhow!("Unknown availability state '{s}'")), - } - } -} - -/// FIXME: this is a duplicate of the type in the attachment_service crate, because the -/// type needs to be defined with diesel traits in there. -#[derive(Serialize, Deserialize, Clone, Copy, Eq, PartialEq)] -pub enum NodeSchedulingPolicy { - Active, - Filling, - Pause, - Draining, -} - -impl FromStr for NodeSchedulingPolicy { - type Err = anyhow::Error; - - fn from_str(s: &str) -> Result { - match s { - "active" => Ok(Self::Active), - "filling" => Ok(Self::Filling), - "pause" => Ok(Self::Pause), - "draining" => Ok(Self::Draining), - _ => Err(anyhow::anyhow!("Unknown scheduling state '{s}'")), - } - } -} - -impl From for String { - fn from(value: NodeSchedulingPolicy) -> String { - use NodeSchedulingPolicy::*; - match value { - Active => "active", - Filling => "filling", - Pause => "pause", - Draining => "draining", - } - .to_string() - } -} - -#[derive(Serialize, Deserialize, Debug)] -pub struct TenantShardMigrateResponse {} - impl AttachmentService { pub fn from_env(env: &LocalEnv) -> Self { let path = Utf8PathBuf::from_path_buf(env.base_data_dir.clone()) diff --git a/control_plane/src/bin/neon_local.rs b/control_plane/src/bin/neon_local.rs index 5c0d008943..cf647a5f9b 100644 --- a/control_plane/src/bin/neon_local.rs +++ b/control_plane/src/bin/neon_local.rs @@ -8,14 +8,15 @@ use anyhow::{anyhow, bail, Context, Result}; use clap::{value_parser, Arg, ArgAction, ArgMatches, Command, ValueEnum}; use compute_api::spec::ComputeMode; -use control_plane::attachment_service::{ - AttachmentService, NodeAvailability, NodeConfigureRequest, NodeSchedulingPolicy, -}; +use control_plane::attachment_service::AttachmentService; use control_plane::endpoint::ComputeControlPlane; use control_plane::local_env::{InitForceMode, LocalEnv}; 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, +}; use pageserver_api::models::{ ShardParameters, TenantCreateRequest, TimelineCreateRequest, TimelineInfo, }; diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index 59cd4789a8..642f153f2d 100644 --- a/control_plane/src/pageserver.rs +++ b/control_plane/src/pageserver.rs @@ -17,6 +17,7 @@ use std::time::Duration; use anyhow::{bail, Context}; use camino::Utf8PathBuf; use futures::SinkExt; +use pageserver_api::controller_api::NodeRegisterRequest; use pageserver_api::models::{ self, LocationConfig, ShardParameters, TenantHistorySize, TenantInfo, TimelineInfo, }; @@ -30,7 +31,7 @@ use utils::{ lsn::Lsn, }; -use crate::attachment_service::{AttachmentService, NodeRegisterRequest}; +use crate::attachment_service::AttachmentService; use crate::local_env::PageServerConf; use crate::{background_process, local_env::LocalEnv}; diff --git a/libs/pageserver_api/src/controller_api.rs b/libs/pageserver_api/src/controller_api.rs new file mode 100644 index 0000000000..64b70a1a51 --- /dev/null +++ b/libs/pageserver_api/src/controller_api.rs @@ -0,0 +1,129 @@ +use std::str::FromStr; + +/// Request/response types for the storage controller +/// API (`/control/v1` prefix). Implemented by the server +/// in [`attachment_service::http`] +use serde::{Deserialize, Serialize}; +use utils::id::NodeId; + +use crate::{models::ShardParameters, shard::TenantShardId}; + +#[derive(Serialize, Deserialize)] +pub struct TenantCreateResponseShard { + pub shard_id: TenantShardId, + pub node_id: NodeId, + pub generation: u32, +} + +#[derive(Serialize, Deserialize)] +pub struct TenantCreateResponse { + pub shards: Vec, +} + +#[derive(Serialize, Deserialize)] +pub struct NodeRegisterRequest { + pub node_id: NodeId, + + pub listen_pg_addr: String, + pub listen_pg_port: u16, + + pub listen_http_addr: String, + pub listen_http_port: u16, +} + +#[derive(Serialize, Deserialize)] +pub struct NodeConfigureRequest { + pub node_id: NodeId, + + pub availability: Option, + pub scheduling: Option, +} + +#[derive(Serialize, Deserialize, Debug)] +pub struct TenantLocateResponseShard { + pub shard_id: TenantShardId, + pub node_id: NodeId, + + pub listen_pg_addr: String, + pub listen_pg_port: u16, + + pub listen_http_addr: String, + pub listen_http_port: u16, +} + +#[derive(Serialize, Deserialize)] +pub struct TenantLocateResponse { + pub shards: Vec, + pub shard_params: ShardParameters, +} + +/// Explicitly migrating a particular shard is a low level operation +/// TODO: higher level "Reschedule tenant" operation where the request +/// specifies some constraints, e.g. asking it to get off particular node(s) +#[derive(Serialize, Deserialize, Debug)] +pub struct TenantShardMigrateRequest { + pub tenant_shard_id: TenantShardId, + pub node_id: NodeId, +} + +#[derive(Serialize, Deserialize, Clone, Copy, Eq, PartialEq)] +pub enum NodeAvailability { + // Normal, happy state + Active, + // Offline: Tenants shouldn't try to attach here, but they may assume that their + // secondary locations on this node still exist. Newly added nodes are in this + // state until we successfully contact them. + Offline, +} + +impl FromStr for NodeAvailability { + type Err = anyhow::Error; + + fn from_str(s: &str) -> Result { + match s { + "active" => Ok(Self::Active), + "offline" => Ok(Self::Offline), + _ => Err(anyhow::anyhow!("Unknown availability state '{s}'")), + } + } +} + +/// FIXME: this is a duplicate of the type in the attachment_service crate, because the +/// type needs to be defined with diesel traits in there. +#[derive(Serialize, Deserialize, Clone, Copy, Eq, PartialEq)] +pub enum NodeSchedulingPolicy { + Active, + Filling, + Pause, + Draining, +} + +impl FromStr for NodeSchedulingPolicy { + type Err = anyhow::Error; + + fn from_str(s: &str) -> Result { + match s { + "active" => Ok(Self::Active), + "filling" => Ok(Self::Filling), + "pause" => Ok(Self::Pause), + "draining" => Ok(Self::Draining), + _ => Err(anyhow::anyhow!("Unknown scheduling state '{s}'")), + } + } +} + +impl From for String { + fn from(value: NodeSchedulingPolicy) -> String { + use NodeSchedulingPolicy::*; + match value { + Active => "active", + Filling => "filling", + Pause => "pause", + Draining => "draining", + } + .to_string() + } +} + +#[derive(Serialize, Deserialize, Debug)] +pub struct TenantShardMigrateResponse {} diff --git a/libs/pageserver_api/src/lib.rs b/libs/pageserver_api/src/lib.rs index b236b93428..1b948d60c3 100644 --- a/libs/pageserver_api/src/lib.rs +++ b/libs/pageserver_api/src/lib.rs @@ -2,13 +2,14 @@ #![deny(clippy::undocumented_unsafe_blocks)] use const_format::formatcp; -/// Public API types -pub mod control_api; +pub mod controller_api; pub mod key; pub mod keyspace; pub mod models; pub mod reltag; pub mod shard; +/// Public API types +pub mod upcall_api; pub const DEFAULT_PG_LISTEN_PORT: u16 = 64000; pub const DEFAULT_PG_LISTEN_ADDR: &str = formatcp!("127.0.0.1:{DEFAULT_PG_LISTEN_PORT}"); diff --git a/libs/pageserver_api/src/control_api.rs b/libs/pageserver_api/src/upcall_api.rs similarity index 100% rename from libs/pageserver_api/src/control_api.rs rename to libs/pageserver_api/src/upcall_api.rs diff --git a/pageserver/src/control_plane_client.rs b/pageserver/src/control_plane_client.rs index 61c7d03408..3fcf3a983b 100644 --- a/pageserver/src/control_plane_client.rs +++ b/pageserver/src/control_plane_client.rs @@ -2,10 +2,10 @@ use std::collections::HashMap; use futures::Future; use pageserver_api::{ - control_api::{ + shard::TenantShardId, + upcall_api::{ ReAttachRequest, ReAttachResponse, ValidateRequest, ValidateRequestTenant, ValidateResponse, }, - shard::TenantShardId, }; use serde::{de::DeserializeOwned, Serialize}; use tokio_util::sync::CancellationToken;