From 89307822b01453fa6b560d9f3bcd943d681e4267 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 17 May 2023 12:31:17 +0200 Subject: [PATCH] mgmt api: share a single tenant config model struct in Rust and OpenAPI (#4252) This is prep for https://github.com/neondatabase/neon/pull/4255 [1/X] OpenAPI: share a single definition of TenantConfig DRYs up the pageserver OpenAPI YAML's representation of tenant config. All the fields of tenant config are now located in a model schema called TenantConfig. The tenant create & config-change endpoints have separate schemas, TenantCreateInfo and TenantConfigureArg, respectively. These schemas inherit from TenantConfig, using allOf 1. The tenant config-GET handler's response was previously named TenantConfig. It's now named TenantConfigResponse. None of these changes affect how the request looks on the wire. The generated Go code will change for Console because the OpenAPI code generator maps `allOf` to a Go struct embedding. Luckily, usage of tenant config in Console is still very lightweigt, but that will change in the near future. So, this is a good chance to set things straight. The console changes are tracked in https://github.com/neondatabase/cloud/pull/5046 [2/x]: extract the tenant config parts of create & config requests [3/x]: code movement: move TenantConfigRequestConfig next to TenantCreateRequestConfig [4/x] type-alias TenantConfigRequestConfig = TenantCreateRequestConfig; They are exactly the same. [5/x] switch to qualified use for tenant create/config request api models [6/x] rename models::TenantConfig{RequestConfig,} and remove the alias [7/x] OpenAPI: sync tenant create & configure body names from Rust code [8/x]: dedupe the two TryFrom<...> for TenantConfOpt impls The only difference is that the TenantConfigRequest impl does ``` tenant_conf.max_lsn_wal_lag = request_data.max_lsn_wal_lag; tenant_conf.trace_read_requests = request_data.trace_read_requests; ``` and the TenantCreateRequest impl does ``` if let Some(max_lsn_wal_lag) = request_data.max_lsn_wal_lag { tenant_conf.max_lsn_wal_lag = Some(max_lsn_wal_lag); } if let Some(trace_read_requests) = request_data.trace_read_requests { tenant_conf.trace_read_requests = Some(trace_read_requests); } ``` As far as I can tell, these are identical. --- control_plane/src/pageserver.rs | 26 ++++--- libs/pageserver_api/src/models.rs | 51 +++++++------ pageserver/src/http/openapi_spec.yml | 58 +++++++------- pageserver/src/http/routes.rs | 6 +- pageserver/src/tenant/config.rs | 108 ++++++--------------------- vendor/postgres-v14 | 2 +- vendor/postgres-v15 | 2 +- 7 files changed, 95 insertions(+), 158 deletions(-) diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index 75991045a4..f022be3910 100644 --- a/control_plane/src/pageserver.rs +++ b/control_plane/src/pageserver.rs @@ -8,9 +8,7 @@ use std::process::{Child, Command}; use std::{io, result}; use anyhow::{bail, Context}; -use pageserver_api::models::{ - TenantConfigRequest, TenantCreateRequest, TenantInfo, TimelineCreateRequest, TimelineInfo, -}; +use pageserver_api::models::{self, TenantInfo, TimelineInfo}; use postgres_backend::AuthType; use postgres_connection::{parse_host_port, PgConnectionConfig}; use reqwest::blocking::{Client, RequestBuilder, Response}; @@ -316,8 +314,8 @@ impl PageServerNode { settings: HashMap<&str, &str>, ) -> anyhow::Result { let mut settings = settings.clone(); - let request = TenantCreateRequest { - new_tenant_id, + + let config = models::TenantConfig { checkpoint_distance: settings .remove("checkpoint_distance") .map(|x| x.parse::()) @@ -372,6 +370,10 @@ impl PageServerNode { .remove("evictions_low_residence_duration_metric_threshold") .map(|x| x.to_string()), }; + let request = models::TenantCreateRequest { + new_tenant_id, + config, + }; if !settings.is_empty() { bail!("Unrecognized tenant settings: {settings:?}") } @@ -392,9 +394,9 @@ impl PageServerNode { } pub fn tenant_config(&self, tenant_id: TenantId, settings: HashMap<&str, &str>) -> Result<()> { - self.http_request(Method::PUT, format!("{}/tenant/config", self.http_base_url))? - .json(&TenantConfigRequest { - tenant_id, + let config = { + // Braces to make the diff easier to read + models::TenantConfig { checkpoint_distance: settings .get("checkpoint_distance") .map(|x| x.parse::()) @@ -451,7 +453,11 @@ impl PageServerNode { evictions_low_residence_duration_metric_threshold: settings .get("evictions_low_residence_duration_metric_threshold") .map(|x| x.to_string()), - }) + } + }; + + self.http_request(Method::PUT, format!("{}/tenant/config", self.http_base_url))? + .json(&models::TenantConfigRequest { tenant_id, config }) .send()? .error_from_body()?; @@ -483,7 +489,7 @@ impl PageServerNode { Method::POST, format!("{}/tenant/{}/timeline", self.http_base_url, tenant_id), )? - .json(&TimelineCreateRequest { + .json(&models::TimelineCreateRequest { new_timeline_id, ancestor_start_lsn, ancestor_timeline_id, diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index e4df81c9ad..0bcdb3c3a8 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -136,6 +136,20 @@ pub struct TenantCreateRequest { #[serde(default)] #[serde_as(as = "Option")] pub new_tenant_id: Option, + #[serde(flatten)] + pub config: TenantConfig, +} + +impl std::ops::Deref for TenantCreateRequest { + type Target = TenantConfig; + + fn deref(&self) -> &Self::Target { + &self.config + } +} + +#[derive(Serialize, Deserialize, Default)] +pub struct TenantConfig { pub checkpoint_distance: Option, pub checkpoint_timeout: Option, pub compaction_target_size: Option, @@ -182,33 +196,21 @@ impl TenantCreateRequest { pub struct TenantConfigRequest { #[serde_as(as = "DisplayFromStr")] pub tenant_id: TenantId, - #[serde(default)] - pub checkpoint_distance: Option, - pub checkpoint_timeout: Option, - pub compaction_target_size: Option, - pub compaction_period: Option, - pub compaction_threshold: Option, - pub gc_horizon: Option, - pub gc_period: Option, - pub image_creation_threshold: Option, - pub pitr_interval: Option, - pub walreceiver_connect_timeout: Option, - pub lagging_wal_timeout: Option, - pub max_lsn_wal_lag: Option, - pub trace_read_requests: Option, - // We defer the parsing of the eviction_policy field to the request handler. - // Otherwise we'd have to move the types for eviction policy into this package. - // We might do that once the eviction feature has stabilizied. - // For now, this field is not even documented in the openapi_spec.yml. - pub eviction_policy: Option, - pub min_resident_size_override: Option, - pub evictions_low_residence_duration_metric_threshold: Option, + #[serde(flatten)] + pub config: TenantConfig, +} + +impl std::ops::Deref for TenantConfigRequest { + type Target = TenantConfig; + + fn deref(&self) -> &Self::Target { + &self.config + } } impl TenantConfigRequest { pub fn new(tenant_id: TenantId) -> TenantConfigRequest { - TenantConfigRequest { - tenant_id, + let config = TenantConfig { checkpoint_distance: None, checkpoint_timeout: None, compaction_target_size: None, @@ -225,7 +227,8 @@ impl TenantConfigRequest { eviction_policy: None, min_resident_size_override: None, evictions_low_residence_duration_metric_threshold: None, - } + }; + TenantConfigRequest { tenant_id, config } } } diff --git a/pageserver/src/http/openapi_spec.yml b/pageserver/src/http/openapi_spec.yml index 330587310f..62664733ea 100644 --- a/pageserver/src/http/openapi_spec.yml +++ b/pageserver/src/http/openapi_spec.yml @@ -747,7 +747,7 @@ paths: content: application/json: schema: - $ref: "#/components/schemas/TenantCreateInfo" + $ref: "#/components/schemas/TenantCreateRequest" responses: "201": description: New tenant created successfully @@ -794,7 +794,7 @@ paths: content: application/json: schema: - $ref: "#/components/schemas/TenantConfigInfo" + $ref: "#/components/schemas/TenantConfigRequest" responses: "200": description: OK @@ -846,7 +846,7 @@ paths: content: application/json: schema: - $ref: "#/components/schemas/TenantConfig" + $ref: "#/components/schemas/TenantConfigResponse" "400": description: Malformed get tenanant config request content: @@ -909,35 +909,27 @@ components: See the tenant `/attach` endpoint for more information. type: string enum: [ "maybe", "attached" ] - TenantCreateInfo: + TenantCreateRequest: + allOf: + - $ref: '#/components/schemas/TenantConfig' + - type: object + properties: + new_tenant_id: + type: string + format: hex + TenantConfigRequest: + allOf: + - $ref: '#/components/schemas/TenantConfig' + - type: object + required: + - tenant_id + properties: + tenant_id: + type: string + format: hex + TenantConfig: type: object properties: - new_tenant_id: - type: string - format: hex - tenant_id: - type: string - format: hex - gc_period: - type: string - gc_horizon: - type: integer - pitr_interval: - type: string - checkpoint_distance: - type: integer - checkpoint_timeout: - type: string - compaction_period: - type: string - compaction_threshold: - type: string - TenantConfigInfo: - type: object - properties: - tenant_id: - type: string - format: hex gc_period: type: string gc_horizon: @@ -964,13 +956,13 @@ components: type: integer trace_read_requests: type: boolean - TenantConfig: + TenantConfigResponse: type: object properties: tenant_specific_overrides: - $ref: "#/components/schemas/TenantConfigInfo" + $ref: "#/components/schemas/TenantConfig" effective_config: - $ref: "#/components/schemas/TenantConfigInfo" + $ref: "#/components/schemas/TenantConfig" TimelineInfo: type: object required: diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs index 6457d55dff..7d60d3568a 100644 --- a/pageserver/src/http/routes.rs +++ b/pageserver/src/http/routes.rs @@ -726,7 +726,8 @@ async fn tenant_create_handler(mut request: Request) -> Result(field_name: &'static str, value: &'a str) -> impl 'a + Fn() move || format!("Cannot parse `{field_name}` duration {value:?}") } -impl TenantConfOpt { - #[allow(clippy::too_many_arguments)] - fn from_request( - checkpoint_distance: Option, - checkpoint_timeout: &Option, - compaction_target_size: Option, - compaction_period: &Option, - compaction_threshold: Option, - gc_horizon: Option, - gc_period: &Option, - image_creation_threshold: Option, - pitr_interval: &Option, - walreceiver_connect_timeout: &Option, - lagging_wal_timeout: &Option, - max_lsn_wal_lag: Option, - trace_read_requests: Option, - eviction_policy: &Option, - min_resident_size_override: Option, - evictions_low_residence_duration_metric_threshold: &Option, - ) -> Result { +impl TryFrom<&'_ models::TenantConfig> for TenantConfOpt { + type Error = anyhow::Error; + + fn try_from(request_data: &'_ models::TenantConfig) -> Result { let mut tenant_conf = TenantConfOpt::default(); - if let Some(gc_period) = &gc_period { + if let Some(gc_period) = &request_data.gc_period { tenant_conf.gc_period = Some( humantime::parse_duration(gc_period) .with_context(bad_duration("gc_period", gc_period))?, ); } - tenant_conf.gc_horizon = gc_horizon; - tenant_conf.image_creation_threshold = image_creation_threshold; + tenant_conf.gc_horizon = request_data.gc_horizon; + tenant_conf.image_creation_threshold = request_data.image_creation_threshold; - if let Some(pitr_interval) = &pitr_interval { + if let Some(pitr_interval) = &request_data.pitr_interval { tenant_conf.pitr_interval = Some( humantime::parse_duration(pitr_interval) .with_context(bad_duration("pitr_interval", pitr_interval))?, ); } - if let Some(walreceiver_connect_timeout) = &walreceiver_connect_timeout { + if let Some(walreceiver_connect_timeout) = &request_data.walreceiver_connect_timeout { tenant_conf.walreceiver_connect_timeout = Some( humantime::parse_duration(walreceiver_connect_timeout).with_context( bad_duration("walreceiver_connect_timeout", walreceiver_connect_timeout), )?, ); } - if let Some(lagging_wal_timeout) = &lagging_wal_timeout { + if let Some(lagging_wal_timeout) = &request_data.lagging_wal_timeout { tenant_conf.lagging_wal_timeout = Some( humantime::parse_duration(lagging_wal_timeout) .with_context(bad_duration("lagging_wal_timeout", lagging_wal_timeout))?, ); } - if let Some(max_lsn_wal_lag) = max_lsn_wal_lag { + if let Some(max_lsn_wal_lag) = request_data.max_lsn_wal_lag { tenant_conf.max_lsn_wal_lag = Some(max_lsn_wal_lag); } - if let Some(trace_read_requests) = trace_read_requests { + if let Some(trace_read_requests) = request_data.trace_read_requests { tenant_conf.trace_read_requests = Some(trace_read_requests); } - tenant_conf.checkpoint_distance = checkpoint_distance; - if let Some(checkpoint_timeout) = &checkpoint_timeout { + tenant_conf.checkpoint_distance = request_data.checkpoint_distance; + if let Some(checkpoint_timeout) = &request_data.checkpoint_timeout { tenant_conf.checkpoint_timeout = Some( humantime::parse_duration(checkpoint_timeout) .with_context(bad_duration("checkpoint_timeout", checkpoint_timeout))?, ); } - tenant_conf.compaction_target_size = compaction_target_size; - tenant_conf.compaction_threshold = compaction_threshold; + tenant_conf.compaction_target_size = request_data.compaction_target_size; + tenant_conf.compaction_threshold = request_data.compaction_threshold; - if let Some(compaction_period) = &compaction_period { + if let Some(compaction_period) = &request_data.compaction_period { tenant_conf.compaction_period = Some( humantime::parse_duration(compaction_period) .with_context(bad_duration("compaction_period", compaction_period))?, ); } - if let Some(eviction_policy) = &eviction_policy { + if let Some(eviction_policy) = &request_data.eviction_policy { tenant_conf.eviction_policy = Some( serde::Deserialize::deserialize(eviction_policy) .context("parse field `eviction_policy`")?, ); } - tenant_conf.min_resident_size_override = min_resident_size_override; + tenant_conf.min_resident_size_override = request_data.min_resident_size_override; if let Some(evictions_low_residence_duration_metric_threshold) = - &evictions_low_residence_duration_metric_threshold + &request_data.evictions_low_residence_duration_metric_threshold { tenant_conf.evictions_low_residence_duration_metric_threshold = Some( humantime::parse_duration(evictions_low_residence_duration_metric_threshold) @@ -393,56 +377,6 @@ impl TenantConfOpt { } } -impl TryFrom<&'_ TenantCreateRequest> for TenantConfOpt { - type Error = anyhow::Error; - - fn try_from(request_data: &TenantCreateRequest) -> Result { - Self::from_request( - request_data.checkpoint_distance, - &request_data.checkpoint_timeout, - request_data.compaction_target_size, - &request_data.compaction_period, - request_data.compaction_threshold, - request_data.gc_horizon, - &request_data.gc_period, - request_data.image_creation_threshold, - &request_data.pitr_interval, - &request_data.walreceiver_connect_timeout, - &request_data.lagging_wal_timeout, - request_data.max_lsn_wal_lag, - request_data.trace_read_requests, - &request_data.eviction_policy, - request_data.min_resident_size_override, - &request_data.evictions_low_residence_duration_metric_threshold, - ) - } -} - -impl TryFrom<&'_ TenantConfigRequest> for TenantConfOpt { - type Error = anyhow::Error; - - fn try_from(request_data: &TenantConfigRequest) -> Result { - Self::from_request( - request_data.checkpoint_distance, - &request_data.checkpoint_timeout, - request_data.compaction_target_size, - &request_data.compaction_period, - request_data.compaction_threshold, - request_data.gc_horizon, - &request_data.gc_period, - request_data.image_creation_threshold, - &request_data.pitr_interval, - &request_data.walreceiver_connect_timeout, - &request_data.lagging_wal_timeout, - request_data.max_lsn_wal_lag, - request_data.trace_read_requests, - &request_data.eviction_policy, - request_data.min_resident_size_override, - &request_data.evictions_low_residence_duration_metric_threshold, - ) - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/vendor/postgres-v14 b/vendor/postgres-v14 index 1144aee166..a2daebc6b4 160000 --- a/vendor/postgres-v14 +++ b/vendor/postgres-v14 @@ -1 +1 @@ -Subproject commit 1144aee1661c79eec65e784a8dad8bd450d9df79 +Subproject commit a2daebc6b445dcbcca9c18e1711f47c1db7ffb04 diff --git a/vendor/postgres-v15 b/vendor/postgres-v15 index 1984832c74..2df2ce3744 160000 --- a/vendor/postgres-v15 +++ b/vendor/postgres-v15 @@ -1 +1 @@ -Subproject commit 1984832c740a7fa0e468bb720f40c525b652835d +Subproject commit 2df2ce374464a7449e15dfa46c956b73b4f4098b