add comments

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

Cargo.lock

@@ -2617,6 +2617,7 @@ dependencies = [
  "anyhow",
  "byteorder",
  "bytes",
+ "clap 4.3.0",
  "const_format",
  "enum-map",
  "postgres_ffi",

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};
@@ -313,68 +312,8 @@ impl PageServerNode {
         new_tenant_id: Option<TenantId>,
         settings: HashMap<&str, &str>,
     ) -> anyhow::Result<TenantId> {
-        let mut settings = settings.clone();
+        let settings = settings.clone();
-
+        let config = models::TenantConfig::deserialize_from_settings(settings)?;
-        let config = models::TenantConfig {
-            checkpoint_distance: settings
-                .remove("checkpoint_distance")
-                .map(|x| x.parse::<u64>())
-                .transpose()?,
-            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()?,
-            compaction_period: settings.remove("compaction_period").map(|x| x.to_string()),
-            compaction_threshold: settings
-                .remove("compaction_threshold")
-                .map(|x| x.parse::<usize>())
-                .transpose()?,
-            gc_horizon: settings
-                .remove("gc_horizon")
-                .map(|x| x.parse::<u64>())
-                .transpose()?,
-            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()?,
-            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 integer")?,
-            evictions_low_residence_duration_metric_threshold: settings
-                .remove("evictions_low_residence_duration_metric_threshold")
-                .map(|x| x.to_string()),
-            gc_feedback: settings
-                .remove("gc_feedback")
-                .map(|x| x.parse::<bool>())
-                .transpose()
-                .context("Failed to parse 'gc_feedback' as bool")?,
-        };
 
         // If tenant ID was not specified, generate one
         let new_tenant_id = new_tenant_id.unwrap_or(TenantId::generate());
@@ -383,9 +322,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()?
@@ -405,81 +341,9 @@ impl PageServerNode {
     pub fn tenant_config(
         &self,
         tenant_id: TenantId,
-        mut settings: HashMap<&str, &str>,
+        settings: HashMap<&str, &str>,
     ) -> anyhow::Result<()> {
-        let config = {
+        let config = models::TenantConfig::deserialize_from_settings(settings)?;
-            // 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()),
-                gc_feedback: settings
-                    .remove("gc_feedback")
-                    .map(|x| x.parse::<bool>())
-                    .transpose()
-                    .context("Failed to parse 'gc_feedback' as bool")?,
-            }
-        };
-
-        if !settings.is_empty() {
-            bail!("Unrecognized tenant settings: {settings:?}")
-        }
-
         self.http_request(Method::PUT, format!("{}/tenant/config", self.http_base_url))?
             .json(&models::TenantConfigRequest { tenant_id, config })
             .send()?

libs/pageserver_api/Cargo.toml

@@ -5,6 +5,7 @@ edition.workspace = true
 license.workspace = true
 
 [dependencies]
+clap.workspace = true
 serde.workspace = true
 serde_with.workspace = true
 serde_json.workspace = true

libs/pageserver_api/src/models.rs

@@ -5,6 +5,7 @@ use std::{
 };
 
 use byteorder::{BigEndian, ReadBytesExt};
+use clap::Parser;
 use serde::{Deserialize, Serialize};
 use serde_with::{serde_as, DisplayFromStr};
 use strum_macros;
@@ -201,31 +202,74 @@ impl std::ops::Deref for TenantCreateRequest {
     }
 }
 
-#[derive(Serialize, Deserialize, Debug, Default)]
+#[derive(Serialize, Deserialize, Debug, Default, clap::Parser)]
+#[clap(rename_all = "snake_case")]
 pub struct TenantConfig {
+    #[clap(long)]
     pub checkpoint_distance: Option<u64>,
+    #[clap(long)]
     pub checkpoint_timeout: Option<String>,
+    #[clap(long)]
     pub compaction_target_size: Option<u64>,
+    #[clap(long)]
     pub compaction_period: Option<String>,
+    #[clap(long)]
     pub compaction_threshold: Option<usize>,
+    #[clap(long)]
     pub gc_horizon: Option<u64>,
+    #[clap(long)]
     pub gc_period: Option<String>,
+    #[clap(long)]
     pub image_creation_threshold: Option<usize>,
+    #[clap(long)]
     pub pitr_interval: Option<String>,
+    #[clap(long)]
     pub walreceiver_connect_timeout: Option<String>,
+    #[clap(long)]
     pub lagging_wal_timeout: Option<String>,
+    #[clap(long)]
     pub max_lsn_wal_lag: Option<NonZeroU64>,
+    #[clap(long)]
     pub trace_read_requests: Option<bool>,
     // 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.
+    #[clap(long, value_parser = parse_json)]
     pub eviction_policy: Option<serde_json::Value>,
+    #[clap(long)]
     pub min_resident_size_override: Option<u64>,
+    #[clap(long)]
     pub evictions_low_residence_duration_metric_threshold: Option<String>,
     pub gc_feedback: Option<bool>,
 }
 
+fn parse_json(s: &str) -> Result<serde_json::Value, serde_json::Error> {
+    serde_json::from_str(s)
+}
+
+impl TenantConfig {
+    pub fn deserialize_from_settings(settings: HashMap<&str, &str>) -> Result<Self, anyhow::Error> {
+        // Here we are using `clap` to parse the settings. This is not ideal, but it's the easiest
+        // way to simplify th code. To convert settings into a list of command line arguments, we
+        // need the program name argv0, each key into a long-form option, and each value proceeding it.
+        let config = TenantConfig::try_parse_from(
+            std::iter::once("argv0".to_string()).chain(
+                settings
+                    .iter()
+                    .flat_map(|(k, v)| [format!("--{k}"), v.to_string()]),
+            ),
+        )
+        .map_err(|e| {
+            anyhow::anyhow!(
+                "failed to parse: {}",
+                e.to_string().split('\n').next().unwrap_or_default()
+            ) // only get the first line as other lines in clap errors are not useful
+        })?;
+        Ok(config)
+    }
+}
+
 #[serde_as]
 #[derive(Serialize, Deserialize)]
 #[serde(transparent)]
@@ -873,17 +917,28 @@ mod tests {
             err
         );
 
-        let attach_request = json!({
+        let config = HashMap::from_iter(std::iter::once(("unknown_field", "unknown_value")));
-            "config": {
+        let err = TenantConfig::deserialize_from_settings(config).unwrap_err();
-                "unknown_field": "unknown_value".to_string(),
-            },
-        });
-        let err = serde_json::from_value::<TenantAttachRequest>(attach_request).unwrap_err();
         assert!(
-            err.to_string().contains("unknown field `unknown_field`"),
+            err.to_string()
+                .contains("unexpected argument '--unknown_field' found"),
             "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!(
+            err.to_string().contains("invalid digit found in string") && err.to_string().contains("checkpoint_distance"),
+            "expect error to contain both 'invalid digit found in string' and the field 'checkpoint_distance', got: {}",
+            err
+        );
+    }
+
+    #[test]
+    fn test_parse_json_field() {
+        let config = vec![("eviction_policy", "{\"kind\": \"NoEviction\"}")];
+        TenantConfig::deserialize_from_settings(config.into_iter().collect()).unwrap();
     }
 
     #[test]