use serde for cli config parse

Signed-off-by: Alex Chi <iskyzh@gmail.com>

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::<u64>())
-                    .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::<u64>())
-                    .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::<usize>())
-                    .transpose()
-                    .context("Failed to parse 'compaction_threshold' as an integer")?,
-                gc_horizon: settings
-                    .remove("gc_horizon")
-                    .map(|x| x.parse::<u64>())
-                    .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::<usize>())
-                    .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::<NonZeroU64>())
-                    .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::<bool>())
-                    .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::<u64>())
-                    .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()?

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<Option<T>, D::Error>
+where
+    D: Deserializer<'de>,
+    T: FromStr + serde::Deserialize<'de>,
+    <T as FromStr>::Err: Display,
+{
+    #[derive(Deserialize)]
+    #[serde(untagged)]
+    enum NumericOrNull<'a, T> {
+        String(String),
+        Str(&'a str),
+        FromStr(T),
+        Null,
+    }
+
+    match NumericOrNull::<T>::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<u64>,
     pub checkpoint_timeout: Option<String>,
+    #[serde(default, deserialize_with = "deserialize_option_number_from_string")]
     pub compaction_target_size: Option<u64>,
     pub compaction_period: Option<String>,
+    #[serde(default, deserialize_with = "deserialize_option_number_from_string")]
     pub compaction_threshold: Option<usize>,
+    #[serde(default, deserialize_with = "deserialize_option_number_from_string")]
     pub gc_horizon: Option<u64>,
     pub gc_period: Option<String>,
+    #[serde(default, deserialize_with = "deserialize_option_number_from_string")]
     pub image_creation_threshold: Option<usize>,
     pub pitr_interval: Option<String>,
     pub walreceiver_connect_timeout: Option<String>,
     pub lagging_wal_timeout: Option<String>,
+    #[serde(default, deserialize_with = "deserialize_option_number_from_string")]
     pub max_lsn_wal_lag: Option<NonZeroU64>,
     pub trace_read_requests: Option<bool>,
     // 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_json::Value>,
+    #[serde(default)]
+    #[serde(deserialize_with = "deserialize_option_number_from_string")]
     pub min_resident_size_override: Option<u64>,
     pub evictions_low_residence_duration_metric_threshold: Option<String>,
 }
 
+impl TenantConfig {
+    pub fn deserialize_from_settings(
+        settings: HashMap<&str, &str>,
+    ) -> Result<Self, serde_json::Error> {
+        #[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::<TenantAttachRequest>(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::<TenantConfig>(r#"{ "checkpoint_distance": 233 }"#).unwrap();
+        assert_eq!(res.checkpoint_distance, Some(233));
+        let res =
+            serde_json::from_str::<TenantConfig>(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));
     }
 }