From 5ba1c13fa6c876a893f5faa4d860c71e86b79ef7 Mon Sep 17 00:00:00 2001 From: Rahul Modpur Date: Mon, 18 Sep 2023 09:51:08 +0530 Subject: [PATCH] use serde_path_to_error for deserializing tenant_conf from pageserver config Signed-off-by: Rahul Modpur --- Cargo.lock | 2 + Cargo.toml | 1 + libs/utils/Cargo.toml | 1 + pageserver/Cargo.toml | 1 + pageserver/src/config.rs | 126 +++++--------------------------- pageserver/src/tenant.rs | 4 +- pageserver/src/tenant/config.rs | 15 +++- 7 files changed, 41 insertions(+), 109 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e811a06fac84d..ff24875a6a953 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2713,6 +2713,7 @@ dependencies = [ "scopeguard", "serde", "serde_json", + "serde_path_to_error", "serde_with", "signal-hook", "smallvec", @@ -5151,6 +5152,7 @@ dependencies = [ "sentry", "serde", "serde_json", + "serde_path_to_error", "serde_with", "signal-hook", "strum", diff --git a/Cargo.toml b/Cargo.toml index 847865cfe8187..49b65f6c47f40 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -115,6 +115,7 @@ sysinfo = "0.29.2" 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" sha2 = "0.10.2" signal-hook = "0.3" diff --git a/libs/utils/Cargo.toml b/libs/utils/Cargo.toml index 1eb9e6ab4dadf..7ecc6e8d17cb8 100644 --- a/libs/utils/Cargo.toml +++ b/libs/utils/Cargo.toml @@ -31,6 +31,7 @@ tracing.workspace = true tracing-error.workspace = true tracing-subscriber = { workspace = true, features = ["json", "registry"] } rand.workspace = true +serde_path_to_error.workspace = true serde_with.workspace = true strum.workspace = true strum_macros.workspace = true diff --git a/pageserver/Cargo.toml b/pageserver/Cargo.toml index 9cb71dea09615..4f4f8dc6ff919 100644 --- a/pageserver/Cargo.toml +++ b/pageserver/Cargo.toml @@ -49,6 +49,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 8ee7f28c11756..ea1f2cad5885e 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -716,7 +716,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")?), @@ -772,111 +772,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) -> PathBuf { PathBuf::from(format!("../tmp_check/test_{test_name}")) @@ -1359,6 +1254,25 @@ trace_read_requests = {trace_read_requests}"#, Ok(()) } + #[test] + fn parse_incorrect_tenant_config() -> anyhow::Result<()> { + let config_string = format!( + r#" + [tenant_config] + checkpoint_distance = -1 # supposed to be an u64 + "# + ); + + 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 eviction_pageserver_config_parse() -> anyhow::Result<()> { let tempdir = tempdir()?; diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index f0639844bdef5..6f4333186f02f 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -2357,8 +2357,8 @@ impl Tenant { for (key, item) in toml.iter() { match key { "tenant_config" => { - tenant_conf = PageServerConf::parse_toml_tenant_conf(item).with_context(|| { - format!("Failed to parse config from file '{target_config_display}' as pageserver config") + tenant_conf = TenantConfOpt::try_from(item.to_owned()).with_context(|| { + format!("Failed to parse '{key}' from file '{target_config_display}'") })?; } _ => bail!("config file {target_config_display} has unrecognized pageserver option '{key}'"), diff --git a/pageserver/src/tenant/config.rs b/pageserver/src/tenant/config.rs index ffe2c5eab6517..9be0d3880fd03 100644 --- a/pageserver/src/tenant/config.rs +++ b/pageserver/src/tenant/config.rs @@ -8,7 +8,7 @@ //! We cannot use global or default config instead, because wrong settings //! may lead to a data loss. //! -use anyhow::Context; +use anyhow::{bail, Context}; use pageserver_api::models; use serde::{Deserialize, Serialize}; use std::num::NonZeroU64; @@ -385,6 +385,19 @@ impl TryFrom<&'_ models::TenantConfig> for TenantConfOpt { } } +impl TryFrom for TenantConfOpt { + type Error = anyhow::Error; + + fn try_from(item: toml_edit::Item) -> Result { + let toml_edit::Item::Table(table) = item else { + bail!("expected non-inline table but found {item}") + }; + let deserializer = toml_edit::de::Deserializer::new(table.into()); + serde_path_to_error::deserialize(deserializer) + .map_err(|e| anyhow::anyhow!("{}: {}", e.path(), e.inner().message())) + } +} + #[cfg(test)] mod tests { use super::*;