diff --git a/Cargo.lock b/Cargo.lock index dc463942ed..23c64cf0e9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2977,6 +2977,7 @@ dependencies = [ "scopeguard", "serde", "serde_json", + "serde_path_to_error", "serde_with", "signal-hook", "smallvec", diff --git a/Cargo.toml b/Cargo.toml index 6df48ffc55..ab00c11dda 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -126,6 +126,7 @@ sd-notify = "0.4.1" sentry = { version = "0.31", default-features = false, features = ["backtrace", "contexts", "panic", "rustls", "reqwest" ] } serde = { version = "1.0", features = ["derive"] } serde_json = "1" +serde_path_to_error = "0.1" serde_with = "2.0" serde_assert = "0.5.0" sha2 = "0.10.2" diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index c792a5eff7..60c508037e 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -314,25 +314,7 @@ impl std::ops::Deref for TenantConfigRequest { impl TenantConfigRequest { pub fn new(tenant_id: TenantId) -> TenantConfigRequest { - let config = TenantConfig { - checkpoint_distance: None, - checkpoint_timeout: None, - compaction_target_size: None, - compaction_period: None, - compaction_threshold: None, - gc_horizon: None, - gc_period: None, - image_creation_threshold: None, - pitr_interval: None, - walreceiver_connect_timeout: None, - lagging_wal_timeout: None, - max_lsn_wal_lag: None, - trace_read_requests: None, - eviction_policy: None, - min_resident_size_override: None, - evictions_low_residence_duration_metric_threshold: None, - gc_feedback: None, - }; + let config = TenantConfig::default(); TenantConfigRequest { tenant_id, config } } } diff --git a/pageserver/Cargo.toml b/pageserver/Cargo.toml index 3eb01003df..35c260740c 100644 --- a/pageserver/Cargo.toml +++ b/pageserver/Cargo.toml @@ -51,6 +51,7 @@ regex.workspace = true scopeguard.workspace = true serde.workspace = true serde_json = { workspace = true, features = ["raw_value"] } +serde_path_to_error.workspace = true serde_with.workspace = true signal-hook.workspace = true smallvec = { workspace = true, features = ["write"] } diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs index 5b170af4ef..737495d414 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -779,7 +779,7 @@ impl PageServerConf { builder.remote_storage_config(RemoteStorageConfig::from_toml(item)?) } "tenant_config" => { - t_conf = Self::parse_toml_tenant_conf(item)?; + t_conf = TenantConfOpt::try_from(item.to_owned()).context(format!("failed to parse: '{key}'"))?; } "id" => builder.id(NodeId(parse_toml_u64(key, item)?)), "broker_endpoint" => builder.broker_endpoint(parse_toml_string(key, item)?.parse().context("failed to parse broker endpoint")?), @@ -853,111 +853,6 @@ impl PageServerConf { Ok(conf) } - // subroutine of parse_and_validate to parse `[tenant_conf]` section - - pub fn parse_toml_tenant_conf(item: &toml_edit::Item) -> Result { - let mut t_conf: TenantConfOpt = Default::default(); - if let Some(checkpoint_distance) = item.get("checkpoint_distance") { - t_conf.checkpoint_distance = - Some(parse_toml_u64("checkpoint_distance", checkpoint_distance)?); - } - - if let Some(checkpoint_timeout) = item.get("checkpoint_timeout") { - t_conf.checkpoint_timeout = Some(parse_toml_duration( - "checkpoint_timeout", - checkpoint_timeout, - )?); - } - - if let Some(compaction_target_size) = item.get("compaction_target_size") { - t_conf.compaction_target_size = Some(parse_toml_u64( - "compaction_target_size", - compaction_target_size, - )?); - } - - if let Some(compaction_period) = item.get("compaction_period") { - t_conf.compaction_period = - Some(parse_toml_duration("compaction_period", compaction_period)?); - } - - if let Some(compaction_threshold) = item.get("compaction_threshold") { - t_conf.compaction_threshold = - Some(parse_toml_u64("compaction_threshold", compaction_threshold)?.try_into()?); - } - - if let Some(image_creation_threshold) = item.get("image_creation_threshold") { - t_conf.image_creation_threshold = Some( - parse_toml_u64("image_creation_threshold", image_creation_threshold)?.try_into()?, - ); - } - - if let Some(gc_horizon) = item.get("gc_horizon") { - t_conf.gc_horizon = Some(parse_toml_u64("gc_horizon", gc_horizon)?); - } - - if let Some(gc_period) = item.get("gc_period") { - t_conf.gc_period = Some(parse_toml_duration("gc_period", gc_period)?); - } - - if let Some(pitr_interval) = item.get("pitr_interval") { - t_conf.pitr_interval = Some(parse_toml_duration("pitr_interval", pitr_interval)?); - } - if let Some(walreceiver_connect_timeout) = item.get("walreceiver_connect_timeout") { - t_conf.walreceiver_connect_timeout = Some(parse_toml_duration( - "walreceiver_connect_timeout", - walreceiver_connect_timeout, - )?); - } - if let Some(lagging_wal_timeout) = item.get("lagging_wal_timeout") { - t_conf.lagging_wal_timeout = Some(parse_toml_duration( - "lagging_wal_timeout", - lagging_wal_timeout, - )?); - } - if let Some(max_lsn_wal_lag) = item.get("max_lsn_wal_lag") { - t_conf.max_lsn_wal_lag = - Some(deserialize_from_item("max_lsn_wal_lag", max_lsn_wal_lag)?); - } - if let Some(trace_read_requests) = item.get("trace_read_requests") { - t_conf.trace_read_requests = - Some(trace_read_requests.as_bool().with_context(|| { - "configure option trace_read_requests is not a bool".to_string() - })?); - } - - if let Some(eviction_policy) = item.get("eviction_policy") { - t_conf.eviction_policy = Some( - deserialize_from_item("eviction_policy", eviction_policy) - .context("parse eviction_policy")?, - ); - } - - if let Some(item) = item.get("min_resident_size_override") { - t_conf.min_resident_size_override = Some( - deserialize_from_item("min_resident_size_override", item) - .context("parse min_resident_size_override")?, - ); - } - - if let Some(item) = item.get("evictions_low_residence_duration_metric_threshold") { - t_conf.evictions_low_residence_duration_metric_threshold = Some(parse_toml_duration( - "evictions_low_residence_duration_metric_threshold", - item, - )?); - } - - if let Some(gc_feedback) = item.get("gc_feedback") { - t_conf.gc_feedback = Some( - gc_feedback - .as_bool() - .with_context(|| "configure option gc_feedback is not a bool".to_string())?, - ); - } - - Ok(t_conf) - } - #[cfg(test)] pub fn test_repo_dir(test_name: &str) -> Utf8PathBuf { Utf8PathBuf::from(format!("../tmp_check/test_{test_name}")) @@ -1429,6 +1324,37 @@ trace_read_requests = {trace_read_requests}"#, Ok(()) } + #[test] + fn parse_incorrect_tenant_config() -> anyhow::Result<()> { + let config_string = r#" + [tenant_config] + checkpoint_distance = -1 # supposed to be an u64 + "# + .to_string(); + + let toml: Document = config_string.parse()?; + let item = toml.get("tenant_config").unwrap(); + let error = TenantConfOpt::try_from(item.to_owned()).unwrap_err(); + + let expected_error_str = "checkpoint_distance: invalid value: integer `-1`, expected u64"; + assert_eq!(error.to_string(), expected_error_str); + + Ok(()) + } + + #[test] + fn parse_override_tenant_config() -> anyhow::Result<()> { + let config_string = r#"tenant_config={ min_resident_size_override = 400 }"#.to_string(); + + let toml: Document = config_string.parse()?; + let item = toml.get("tenant_config").unwrap(); + let conf = TenantConfOpt::try_from(item.to_owned()).unwrap(); + + assert_eq!(conf.min_resident_size_override, Some(400)); + + Ok(()) + } + #[test] fn eviction_pageserver_config_parse() -> anyhow::Result<()> { let tempdir = tempdir()?; diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index fe3a5bfb79..2a63f193e3 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -2442,9 +2442,7 @@ impl Tenant { for (key, item) in deserialized.iter() { match key { "tenant_config" => { - tenant_conf = PageServerConf::parse_toml_tenant_conf(item).with_context(|| { - format!("Failed to parse config from file '{legacy_config_path}' as pageserver config") - })?; + tenant_conf = TenantConfOpt::try_from(item.to_owned()).context(format!("Failed to parse config from file '{legacy_config_path}' as pageserver config"))?; } _ => bail!( "config file {legacy_config_path} has unrecognized pageserver option '{key}'" diff --git a/pageserver/src/tenant/config.rs b/pageserver/src/tenant/config.rs index 4ad6a71f67..7a454b53d2 100644 --- a/pageserver/src/tenant/config.rs +++ b/pageserver/src/tenant/config.rs @@ -8,10 +8,12 @@ //! We cannot use global or default config instead, because wrong settings //! may lead to a data loss. //! -use anyhow::Context; +use anyhow::bail; use pageserver_api::models; use pageserver_api::shard::{ShardCount, ShardIdentity, ShardNumber, ShardStripeSize}; +use serde::de::IntoDeserializer; use serde::{Deserialize, Serialize}; +use serde_json::Value; use std::num::NonZeroU64; use std::time::Duration; use utils::generation::Generation; @@ -521,105 +523,49 @@ impl Default for TenantConf { } } -// Helper function to standardize the error messages we produce on bad durations -// -// Intended to be used with anyhow's `with_context`, e.g.: -// -// let value = result.with_context(bad_duration("name", &value))?; -// -fn bad_duration<'a>(field_name: &'static str, value: &'a str) -> impl 'a + Fn() -> String { - move || format!("Cannot parse `{field_name}` duration {value:?}") -} - impl TryFrom<&'_ models::TenantConfig> for TenantConfOpt { type Error = anyhow::Error; fn try_from(request_data: &'_ models::TenantConfig) -> Result { - let mut tenant_conf = TenantConfOpt::default(); + // Convert the request_data to a JSON Value + let json_value: Value = serde_json::to_value(request_data)?; - 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 = request_data.gc_horizon; - tenant_conf.image_creation_threshold = request_data.image_creation_threshold; + // Create a Deserializer from the JSON Value + let deserializer = json_value.into_deserializer(); - 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) = &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) = &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) = 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); - } - - 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 = request_data.compaction_target_size; - tenant_conf.compaction_threshold = request_data.compaction_threshold; - - 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) = &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 = request_data.min_resident_size_override; - - if let Some(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) - .with_context(bad_duration( - "evictions_low_residence_duration_metric_threshold", - evictions_low_residence_duration_metric_threshold, - ))?, - ); - } - tenant_conf.gc_feedback = request_data.gc_feedback; + // Use serde_path_to_error to deserialize the JSON Value into TenantConfOpt + let tenant_conf: TenantConfOpt = serde_path_to_error::deserialize(deserializer)?; Ok(tenant_conf) } } +impl TryFrom for TenantConfOpt { + type Error = anyhow::Error; + + fn try_from(item: toml_edit::Item) -> Result { + match item { + toml_edit::Item::Value(value) => { + let d = value.into_deserializer(); + return serde_path_to_error::deserialize(d) + .map_err(|e| anyhow::anyhow!("{}: {}", e.path(), e.inner().message())); + } + toml_edit::Item::Table(table) => { + let deserializer = toml_edit::de::Deserializer::new(table.into()); + return serde_path_to_error::deserialize(deserializer) + .map_err(|e| anyhow::anyhow!("{}: {}", e.path(), e.inner().message())); + } + _ => { + bail!("expected non-inline table but found {item}") + } + } + } +} + #[cfg(test)] mod tests { use super::*; + use models::TenantConfig; #[test] fn de_serializing_pageserver_config_omits_empty_values() { @@ -636,4 +582,38 @@ mod tests { assert_eq!(json_form, "{\"gc_horizon\":42}"); assert_eq!(small_conf, serde_json::from_str(&json_form).unwrap()); } + + #[test] + fn test_try_from_models_tenant_config_err() { + let tenant_config = models::TenantConfig { + lagging_wal_timeout: Some("5a".to_string()), + ..TenantConfig::default() + }; + + let tenant_conf_opt = TenantConfOpt::try_from(&tenant_config); + + assert!( + tenant_conf_opt.is_err(), + "Suceeded to convert TenantConfig to TenantConfOpt" + ); + + let expected_error_str = + "lagging_wal_timeout: invalid value: string \"5a\", expected a duration"; + assert_eq!(tenant_conf_opt.unwrap_err().to_string(), expected_error_str); + } + + #[test] + fn test_try_from_models_tenant_config_success() { + let tenant_config = models::TenantConfig { + lagging_wal_timeout: Some("5s".to_string()), + ..TenantConfig::default() + }; + + let tenant_conf_opt = TenantConfOpt::try_from(&tenant_config).unwrap(); + + assert_eq!( + tenant_conf_opt.lagging_wal_timeout, + Some(Duration::from_secs(5)) + ); + } }