diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index 149cfd00cb..20d3550a0a 100644 --- a/control_plane/src/pageserver.rs +++ b/control_plane/src/pageserver.rs @@ -2,12 +2,11 @@ use std::borrow::Cow; use std::collections::HashMap; use std::fs::File; use std::io::{BufReader, Write}; -use std::num::NonZeroU64; use std::path::PathBuf; use std::process::{Child, Command}; use std::{io, result}; -use anyhow::{bail, Context}; +use anyhow::Context; use pageserver_api::models::{self, TenantInfo, TimelineInfo}; use postgres_backend::AuthType; use postgres_connection::{parse_host_port, PgConnectionConfig}; @@ -378,9 +377,6 @@ impl PageServerNode { new_tenant_id, config, }; - if !settings.is_empty() { - bail!("Unrecognized tenant settings: {settings:?}") - } self.http_request(Method::POST, format!("{}/tenant", self.http_base_url))? .json(&request) .send()? @@ -400,76 +396,10 @@ impl PageServerNode { pub fn tenant_config( &self, tenant_id: TenantId, - mut settings: HashMap<&str, &str>, + settings: HashMap<&str, &str>, ) -> anyhow::Result<()> { - let config = { - // Braces to make the diff easier to read - models::TenantConfig { - checkpoint_distance: settings - .remove("checkpoint_distance") - .map(|x| x.parse::()) - .transpose() - .context("Failed to parse 'checkpoint_distance' as an integer")?, - checkpoint_timeout: settings.remove("checkpoint_timeout").map(|x| x.to_string()), - compaction_target_size: settings - .remove("compaction_target_size") - .map(|x| x.parse::()) - .transpose() - .context("Failed to parse 'compaction_target_size' as an integer")?, - compaction_period: settings.remove("compaction_period").map(|x| x.to_string()), - compaction_threshold: settings - .remove("compaction_threshold") - .map(|x| x.parse::()) - .transpose() - .context("Failed to parse 'compaction_threshold' as an integer")?, - gc_horizon: settings - .remove("gc_horizon") - .map(|x| x.parse::()) - .transpose() - .context("Failed to parse 'gc_horizon' as an integer")?, - gc_period: settings.remove("gc_period").map(|x| x.to_string()), - image_creation_threshold: settings - .remove("image_creation_threshold") - .map(|x| x.parse::()) - .transpose() - .context("Failed to parse 'image_creation_threshold' as non zero integer")?, - pitr_interval: settings.remove("pitr_interval").map(|x| x.to_string()), - walreceiver_connect_timeout: settings - .remove("walreceiver_connect_timeout") - .map(|x| x.to_string()), - lagging_wal_timeout: settings - .remove("lagging_wal_timeout") - .map(|x| x.to_string()), - max_lsn_wal_lag: settings - .remove("max_lsn_wal_lag") - .map(|x| x.parse::()) - .transpose() - .context("Failed to parse 'max_lsn_wal_lag' as non zero integer")?, - trace_read_requests: settings - .remove("trace_read_requests") - .map(|x| x.parse::()) - .transpose() - .context("Failed to parse 'trace_read_requests' as bool")?, - eviction_policy: settings - .remove("eviction_policy") - .map(serde_json::from_str) - .transpose() - .context("Failed to parse 'eviction_policy' json")?, - min_resident_size_override: settings - .remove("min_resident_size_override") - .map(|x| x.parse::()) - .transpose() - .context("Failed to parse 'min_resident_size_override' as an integer")?, - evictions_low_residence_duration_metric_threshold: settings - .remove("evictions_low_residence_duration_metric_threshold") - .map(|x| x.to_string()), - } - }; - - if !settings.is_empty() { - bail!("Unrecognized tenant settings: {settings:?}") - } - + println!("{:#?}", settings); + let config = models::TenantConfig::deserialize_from_settings(settings)?; self.http_request(Method::PUT, format!("{}/tenant/config", self.http_base_url))? .json(&models::TenantConfigRequest { tenant_id, config }) .send()? diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index 540633d113..819bee6de7 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -1,11 +1,13 @@ use std::{ collections::HashMap, + fmt::Display, num::{NonZeroU64, NonZeroUsize}, + str::FromStr, time::SystemTime, }; use byteorder::{BigEndian, ReadBytesExt}; -use serde::{Deserialize, Serialize}; +use serde::{de::value::MapDeserializer, Deserialize, Deserializer, Serialize}; use serde_with::{serde_as, DisplayFromStr}; use strum_macros; use utils::{ @@ -147,19 +149,56 @@ impl std::ops::Deref for TenantCreateRequest { } } +pub fn deserialize_option_number_from_string<'de, T, D>( + deserializer: D, +) -> Result, D::Error> +where + D: Deserializer<'de>, + T: FromStr + serde::Deserialize<'de>, + ::Err: Display, +{ + #[derive(Deserialize)] + #[serde(untagged)] + enum NumericOrNull<'a, T> { + String(String), + Str(&'a str), + FromStr(T), + Null, + } + + match NumericOrNull::::deserialize(deserializer)? { + NumericOrNull::String(s) => match s.as_str() { + "" => Ok(None), + _ => T::from_str(&s).map(Some).map_err(serde::de::Error::custom), + }, + NumericOrNull::Str(s) => match s { + "" => Ok(None), + _ => T::from_str(s).map(Some).map_err(serde::de::Error::custom), + }, + NumericOrNull::FromStr(i) => Ok(Some(i)), + NumericOrNull::Null => Ok(None), + } +} + #[derive(Serialize, Deserialize, Debug, Default)] pub struct TenantConfig { + #[serde(default, deserialize_with = "deserialize_option_number_from_string")] pub checkpoint_distance: Option, pub checkpoint_timeout: Option, + #[serde(default, deserialize_with = "deserialize_option_number_from_string")] pub compaction_target_size: Option, pub compaction_period: Option, + #[serde(default, deserialize_with = "deserialize_option_number_from_string")] pub compaction_threshold: Option, + #[serde(default, deserialize_with = "deserialize_option_number_from_string")] pub gc_horizon: Option, pub gc_period: Option, + #[serde(default, deserialize_with = "deserialize_option_number_from_string")] pub image_creation_threshold: Option, pub pitr_interval: Option, pub walreceiver_connect_timeout: Option, pub lagging_wal_timeout: Option, + #[serde(default, deserialize_with = "deserialize_option_number_from_string")] pub max_lsn_wal_lag: Option, pub trace_read_requests: Option, // We defer the parsing of the eviction_policy field to the request handler. @@ -167,10 +206,29 @@ pub struct TenantConfig { // 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, + #[serde(default)] + #[serde(deserialize_with = "deserialize_option_number_from_string")] pub min_resident_size_override: Option, pub evictions_low_residence_duration_metric_threshold: Option, } +impl TenantConfig { + pub fn deserialize_from_settings( + settings: HashMap<&str, &str>, + ) -> Result { + #[derive(Deserialize)] + #[serde(deny_unknown_fields)] + struct TenantConfigWrapper { + #[serde(flatten)] + config: TenantConfig, + } + let config = TenantConfigWrapper::deserialize( + MapDeserializer::<_, serde_json::Error>::new(settings.into_iter()), + )?; + Ok(config.config) + } +} + #[serde_as] #[derive(Serialize, Deserialize)] #[serde(transparent)] @@ -817,16 +875,29 @@ mod tests { err ); - let attach_request = json!({ - "config": { - "unknown_field": "unknown_value".to_string(), - }, - }); - let err = serde_json::from_value::(attach_request).unwrap_err(); + let config = HashMap::from_iter(std::iter::once(("unknown_field", "unknown_value"))); + let err = TenantConfig::deserialize_from_settings(config).unwrap_err(); assert!( err.to_string().contains("unknown field `unknown_field`"), "expect unknown field `unknown_field` error, got: {}", err ); + + let config = HashMap::from_iter(std::iter::once(("checkpoint_distance", "not_a_number"))); + let err = TenantConfig::deserialize_from_settings(config).unwrap_err(); + assert_eq!(err.to_string(), "invalid digit found in string"); + } + + #[test] + fn test_deserialize_maybe_number() { + let res = + serde_json::from_str::(r#"{ "checkpoint_distance": 233 }"#).unwrap(); + assert_eq!(res.checkpoint_distance, Some(233)); + let res = + serde_json::from_str::(r#"{ "checkpoint_distance": "233" }"#).unwrap(); + assert_eq!(res.checkpoint_distance, Some(233)); + let config = HashMap::from_iter(std::iter::once(("checkpoint_distance", "233"))); + let res = TenantConfig::deserialize_from_settings(config).unwrap(); + assert_eq!(res.checkpoint_distance, Some(233)); } }