neon_local: remove --pageserver-config-overrides, neon_local init takes a toml tempfile

Cargo.lock

@@ -1348,6 +1348,7 @@ dependencies = [
  "tokio-postgres",
  "tokio-util",
  "toml",
+ "toml_edit",
  "tracing",
  "url",
  "utils",

control_plane/Cargo.toml

@@ -28,6 +28,7 @@ serde_with.workspace = true
 tar.workspace = true
 thiserror.workspace = true
 toml.workspace = true
+toml_edit.workspace = true
 tokio.workspace = true
 tokio-postgres.workspace = true
 tokio-util.workspace = true

control_plane/src/bin/neon_local.rs

@@ -133,7 +133,7 @@ fn main() -> Result<()> {
         let subcommand_result = match sub_name {
             "tenant" => rt.block_on(handle_tenant(sub_args, &mut env)),
             "timeline" => rt.block_on(handle_timeline(sub_args, &mut env)),
-            "start" => rt.block_on(handle_start_all(sub_args, &env)),
+            "start" => rt.block_on(handle_start_all(&env)),
             "stop" => rt.block_on(handle_stop_all(sub_args, &env)),
             "pageserver" => rt.block_on(handle_pageserver(sub_args, &env)),
             "storage_controller" => rt.block_on(handle_storage_controller(sub_args, &env)),
@@ -358,6 +358,16 @@ fn handle_init(init_match: &ArgMatches) -> anyhow::Result<LocalEnv> {
         default_conf(*num_pageservers)
     };
 
+    let pageserver_config_overrides: toml_edit::Document =
+        if let Some(path) = init_match.get_one::<PathBuf>("pageserver-config-overrides-file") {
+            std::fs::read_to_string(path)
+                .context("load pageserver config overrides file")?
+                .parse()
+                .context("parse pageserver config overrides file")?
+        } else {
+            toml_edit::Document::new()
+        };
+
     let pg_version = init_match
         .get_one::<u32>("pg-version")
         .copied()
@@ -375,7 +385,7 @@ fn handle_init(init_match: &ArgMatches) -> anyhow::Result<LocalEnv> {
     // Initialize pageserver, create initial tenant and timeline.
     for ps_conf in &env.pageservers {
         PageServerNode::from_env(&env, ps_conf)
-            .initialize()
+            .initialize(&pageserver_config_overrides)
             .unwrap_or_else(|e| {
                 eprintln!("pageserver init failed: {e:?}");
                 exit(1);
@@ -1085,10 +1095,7 @@ async fn handle_pageserver(sub_match: &ArgMatches, env: &local_env::LocalEnv) ->
                 exit(1);
             }
 
-            if let Err(e) = pageserver
-                .start(&pageserver_config_overrides(subcommand_args))
-                .await
-            {
+            if let Err(e) = pageserver.start().await {
                 eprintln!("pageserver start failed: {e}");
                 exit(1);
             }
@@ -1215,7 +1222,7 @@ async fn handle_safekeeper(sub_match: &ArgMatches, env: &local_env::LocalEnv) ->
     Ok(())
 }
 
-async fn handle_start_all(sub_match: &ArgMatches, env: &local_env::LocalEnv) -> anyhow::Result<()> {
+async fn handle_start_all(env: &local_env::LocalEnv) -> anyhow::Result<()> {
     // Endpoints are not started automatically
 
     broker::start_broker_process(env).await?;
@@ -1428,7 +1435,6 @@ fn cli() -> Command {
         .subcommand(
             Command::new("init")
                 .about("Initialize a new Neon repository, preparing configs for services to start with")
-                .arg(pageserver_config_args.clone())
                 .arg(num_pageservers_arg.clone())
                 .arg(
                     Arg::new("config")
@@ -1437,6 +1443,13 @@ fn cli() -> Command {
                         .value_parser(value_parser!(PathBuf))
                         .value_name("config"),
                 )
+                .arg(
+                    Arg::new("pageserver-config-overrides-file")
+                        .long("pageserver-config-overrides-file")
+                        .required(false)
+                        .value_parser(value_parser!(PathBuf))
+                        .value_name("pageserver-config-overrides-file"),
+                )
                 .arg(pg_version_arg.clone())
                 .arg(force_arg)
         )
@@ -1517,7 +1530,6 @@ fn cli() -> Command {
                 .subcommand(Command::new("status"))
                 .subcommand(Command::new("start")
                     .about("Start local pageserver")
-                    .arg(pageserver_config_args.clone())
                 )
                 .subcommand(Command::new("stop")
                     .about("Stop local pageserver")
@@ -1525,7 +1537,6 @@ fn cli() -> Command {
                 )
                 .subcommand(Command::new("restart")
                     .about("Restart local pageserver")
-                    .arg(pageserver_config_args.clone())
                 )
         )
         .subcommand(
@@ -1638,7 +1649,6 @@ fn cli() -> Command {
         .subcommand(
             Command::new("start")
                 .about("Start page server and safekeepers")
-                .arg(pageserver_config_args)
         )
         .subcommand(
             Command::new("stop")

control_plane/src/pageserver.rs

@@ -4,7 +4,6 @@
 //!
 //!   .neon/
 //!
-use std::borrow::Cow;
 use std::collections::HashMap;
 
 use std::io;
@@ -75,9 +74,9 @@ impl PageServerNode {
     }
 
     /// Initializes a pageserver node by creating its config with the overrides provided.
-    pub fn initialize(&self) -> anyhow::Result<()> {
+    pub fn initialize(&self, cli_overrides: &toml_edit::Document) -> anyhow::Result<()> {
         // First, run `pageserver --init` and wait for it to write a config into FS and exit.
-        self.pageserver_init()
+        self.pageserver_init(cli_overrides)
             .with_context(|| format!("Failed to run init for pageserver node {}", self.conf.id))
     }
 
@@ -97,7 +96,7 @@ impl PageServerNode {
         self.start_node().await
     }
 
-    fn pageserver_init(&self) -> anyhow::Result<()> {
+    fn pageserver_init(&self, cli_overrides: &toml_edit::Document) -> anyhow::Result<()> {
         let datadir = self.repo_path();
         let node_id = self.conf.id;
         println!(
@@ -115,128 +114,103 @@ impl PageServerNode {
         let datadir_path_str = datadir.to_str().with_context(|| {
             format!("Cannot start pageserver node {node_id} in path that has no string representation: {datadir:?}")
         })?;
-        let mut args = {
-            let this = &self;
-            let mut args = vec![Cow::Borrowed("-D"), Cow::Borrowed(datadir_path_str)];
 
-            let overrides = {
-                let this = &this;
-                // FIXME: the paths should be shell-escaped to handle paths with spaces, quotas etc.
-                let pg_distrib_dir_param = format!(
-                    "pg_distrib_dir='{}'",
-                    this.env.pg_distrib_dir_raw().display()
-                );
-
-                let PageServerConf {
-                    id,
-                    listen_pg_addr,
-                    listen_http_addr,
-                    pg_auth_type,
-                    http_auth_type,
-                    virtual_file_io_engine,
-                    get_vectored_impl,
-                    get_impl,
-                    validate_vectored_get,
-                } = &this.conf;
-
-                let id = format!("id={}", id);
-
-                let http_auth_type_param = format!("http_auth_type='{}'", http_auth_type);
-                let listen_http_addr_param = format!("listen_http_addr='{}'", listen_http_addr);
-
-                let pg_auth_type_param = format!("pg_auth_type='{}'", pg_auth_type);
-                let listen_pg_addr_param = format!("listen_pg_addr='{}'", listen_pg_addr);
-                let virtual_file_io_engine =
-                    if let Some(virtual_file_io_engine) = virtual_file_io_engine {
-                        format!("virtual_file_io_engine='{virtual_file_io_engine}'")
-                    } else {
-                        String::new()
-                    };
-                let get_vectored_impl = if let Some(get_vectored_impl) = get_vectored_impl {
-                    format!("get_vectored_impl='{get_vectored_impl}'")
-                } else {
-                    String::new()
-                };
-                let get_impl = if let Some(get_impl) = get_impl {
-                    format!("get_impl='{get_impl}'")
-                } else {
-                    String::new()
-                };
-                let validate_vectored_get =
-                    if let Some(validate_vectored_get) = validate_vectored_get {
-                        format!("validate_vectored_get={validate_vectored_get}")
-                    } else {
-                        String::new()
-                    };
-
-                let broker_endpoint_param =
-                    format!("broker_endpoint='{}'", this.env.broker.client_url());
-
-                let mut overrides = vec![
-                    id,
-                    pg_distrib_dir_param,
-                    http_auth_type_param,
-                    pg_auth_type_param,
-                    listen_http_addr_param,
-                    listen_pg_addr_param,
-                    broker_endpoint_param,
-                    virtual_file_io_engine,
-                    get_vectored_impl,
-                    get_impl,
-                    validate_vectored_get,
-                ];
-
-                if let Some(control_plane_api) = &this.env.control_plane_api {
-                    overrides.push(format!(
-                        "control_plane_api='{}'",
-                        control_plane_api.as_str()
-                    ));
-
-                    // Storage controller uses the same auth as pageserver: if JWT is enabled
-                    // for us, we will also need it to talk to them.
-                    if matches!(http_auth_type, AuthType::NeonJWT) {
-                        let jwt_token = this
-                            .env
-                            .generate_auth_token(&Claims::new(None, Scope::GenerationsApi))
-                            .unwrap();
-                        overrides.push(format!("control_plane_api_token='{}'", jwt_token));
-                    }
-                }
-
-                if !config_overrides
-                    .iter()
-                    .any(|c| c.starts_with("remote_storage"))
-                {
-                    overrides.push(format!(
-                        "remote_storage={{local_path='../{PAGESERVER_REMOTE_STORAGE_DIR}'}}"
-                    ));
-                }
-
-                if *http_auth_type != AuthType::Trust || *pg_auth_type != AuthType::Trust {
-                    // Keys are generated in the toplevel repo dir, pageservers' workdirs
-                    // are one level below that, so refer to keys with ../
-                    overrides.push(
-                        "auth_validation_public_key_path='../auth_public_key.pem'".to_owned(),
-                    );
-                }
-
-                // Apply the user-provided overrides
-                overrides.extend(config_overrides.iter().map(|&c| c.to_owned()));
-
-                overrides
-            };
-
-            for config_override in overrides {
-                args.push(Cow::Borrowed("--config-override"));
-                args.push(Cow::Owned(config_override));
-            }
-
-            args
+        let pg_distrib_dir_param = format!(
+            "pg_distrib_dir='{}'",
+            self.env.pg_distrib_dir_raw().display()
+        );
+        let PageServerConf {
+            id,
+            listen_pg_addr,
+            listen_http_addr,
+            pg_auth_type,
+            http_auth_type,
+            virtual_file_io_engine,
+            get_vectored_impl,
+            get_impl,
+            validate_vectored_get,
+        } = &self.conf;
+        let id = format!("id={}", id);
+        let http_auth_type_param = format!("http_auth_type='{}'", http_auth_type);
+        let listen_http_addr_param = format!("listen_http_addr='{}'", listen_http_addr);
+        let pg_auth_type_param = format!("pg_auth_type='{}'", pg_auth_type);
+        let listen_pg_addr_param = format!("listen_pg_addr='{}'", listen_pg_addr);
+        let virtual_file_io_engine = if let Some(virtual_file_io_engine) = virtual_file_io_engine {
+            format!("virtual_file_io_engine='{virtual_file_io_engine}'")
+        } else {
+            String::new()
         };
-        args.push(Cow::Borrowed("--init"));
+        let get_vectored_impl = if let Some(get_vectored_impl) = get_vectored_impl {
+            format!("get_vectored_impl='{get_vectored_impl}'")
+        } else {
+            String::new()
+        };
+        let get_impl = if let Some(get_impl) = get_impl {
+            format!("get_impl='{get_impl}'")
+        } else {
+            String::new()
+        };
+        let validate_vectored_get = if let Some(validate_vectored_get) = validate_vectored_get {
+            format!("validate_vectored_get={validate_vectored_get}")
+        } else {
+            String::new()
+        };
+        let broker_endpoint_param = format!("broker_endpoint='{}'", self.env.broker.client_url());
+        let mut config_pieces = vec![
+            id,
+            pg_distrib_dir_param,
+            http_auth_type_param,
+            pg_auth_type_param,
+            listen_http_addr_param,
+            listen_pg_addr_param,
+            broker_endpoint_param,
+            virtual_file_io_engine,
+            get_vectored_impl,
+            get_impl,
+            validate_vectored_get,
+        ];
+        if let Some(control_plane_api) = &self.env.control_plane_api {
+            config_pieces.push(format!(
+                "control_plane_api='{}'",
+                control_plane_api.as_str()
+            ));
+
+            // Storage controller uses the same auth as pageserver: if JWT is enabled
+            // for us, we will also need it to talk to them.
+            if matches!(http_auth_type, AuthType::NeonJWT) {
+                let jwt_token = self
+                    .env
+                    .generate_auth_token(&Claims::new(None, Scope::GenerationsApi))
+                    .unwrap();
+                config_pieces.push(format!("control_plane_api_token='{}'", jwt_token));
+            }
+        }
+        if !cli_overrides.contains_key("remote_storage") {
+            config_pieces.push(format!(
+                "remote_storage={{local_path='../{PAGESERVER_REMOTE_STORAGE_DIR}'}}"
+            ));
+        }
+        if *http_auth_type != AuthType::Trust || *pg_auth_type != AuthType::Trust {
+            // Keys are generated in the toplevel repo dir, pageservers' workdirs
+            // are one level below that, so refer to keys with ../
+            config_pieces
+                .push("auth_validation_public_key_path='../auth_public_key.pem'".to_owned());
+        }
+
+        // Add the CLI overrides last, so they can override any of the above.
+        config_pieces.push(cli_overrides.to_string());
+
+        // `pageserver --init` merges the `--config-override`s into the default config,
+        // then writes out the merged product to `pageserver.toml`.
+        // TODO: just write the full `pageserver.toml` and get rid of `--config-override`.
+        let mut args = vec!["--init", "-D", datadir_path_str];
+        for piece in &config_pieces {
+            args.push("--config-override");
+            args.push(piece);
+        }
 
         let init_output = Command::new(self.env.pageserver_bin())
-            .args(args.iter().map(Cow::as_ref))
+            .args(args)
             .envs(self.pageserver_env_variables()?)
             .output()
             .with_context(|| format!("Failed to run pageserver init for node {node_id}"))?;
@@ -292,11 +266,7 @@ impl PageServerNode {
                 self.conf.id, datadir,
             )
         })?;
-        let mut args = vec!["-D", datadir_path_str];
-        for config_override in config_overrides {
-            args.push("--config-override");
-            args.push(*config_override);
-        }
+        let args = vec!["-D", datadir_path_str];
         background_process::start_process(
             "pageserver",
             &datadir,

pageserver/src/bin/pageserver.rs

@@ -6,7 +6,7 @@ use std::env::{var, VarError};
 use std::io::Read;
 use std::sync::Arc;
 use std::time::Duration;
-use std::{env, ops::ControlFlow, str::FromStr};
+use std::{env, ops::ControlFlow};
 
 use anyhow::{anyhow, Context};
 use camino::Utf8Path;
@@ -183,13 +183,13 @@ fn initialize_config(
     };
 
     // Construct the runtime representation
-    let conf = PageServerConf::parse_and_validate(&effective_config, workdir)
+    let conf = PageServerConf::parse_and_validate(&config, workdir)
         .context("Failed to parse pageserver configuration")?;
 
     if init {
         info!("Writing pageserver config to '{cfg_file_path}'");
 
-        std::fs::write(cfg_file_path, effective_config.to_string())
+        std::fs::write(cfg_file_path, config.to_string())
             .with_context(|| format!("Failed to write pageserver config to '{cfg_file_path}'"))?;
         info!("Config successfully written to '{cfg_file_path}'")
     }

test_runner/fixtures/neon_fixtures.py

@@ -14,7 +14,7 @@ import textwrap
 import threading
 import time
 import uuid
-from contextlib import closing, contextmanager
+from contextlib import ExitStack, closing, contextmanager
 from dataclasses import dataclass
 from datetime import datetime
 from enum import Enum
@@ -1709,42 +1709,52 @@ class NeonCli(AbstractNeonCli):
         force: Optional[str] = None,
         pageserver_config_override: Optional[str] = None,
     ) -> "subprocess.CompletedProcess[str]":
-        with tempfile.NamedTemporaryFile(mode="w+") as tmp:
-            tmp.write(toml.dumps(config))
-            tmp.flush()
+        remote_storage = self.env.pageserver_remote_storage
 
-            cmd = ["init", f"--config={tmp.name}", "--pg-version", self.env.pg_version]
+        ps_toml = {}
+        if remote_storage is not None:
+            remote_storage_toml_table = remote_storage_to_toml_inline_table(remote_storage)
+            ps_toml["remote_storage"] = remote_storage_toml_table
+
+        env_overrides = os.getenv("NEON_PAGESERVER_OVERRIDES")
+        if env_overrides is not None:
+            for o in env_overrides.split(";"):
+                override = toml.loads(o)
+                for key, value in override.items():
+                    ps_toml[key] = value
+
+        if pageserver_config_override is not None:
+            for o in pageserver_config_override.split(";"):
+                override = toml.loads(o)
+                for key, value in override.items():
+                    ps_toml[key] = value
+
+        with ExitStack() as stack:
+            ps_toml_file = stack.enter_context(tempfile.NamedTemporaryFile(mode="w+"))
+            ps_toml_file.write(toml.dumps(ps_toml))
+            ps_toml_file.flush()
+
+            neon_local_config = stack.enter_context(tempfile.NamedTemporaryFile(mode="w+"))
+            neon_local_config.write(toml.dumps(config))
+            neon_local_config.flush()
+
+            cmd = [
+                "init",
+                f"--config={neon_local_config.name}",
+                "--pg-version",
+                self.env.pg_version,
+                f"--pageserver-config-overrides-file={ps_toml_file.name}",
+            ]
 
             if force is not None:
                 cmd.extend(["--force", force])
 
-            remote_storage = self.env.pageserver_remote_storage
-
-            if remote_storage is not None:
-                remote_storage_toml_table = remote_storage_to_toml_inline_table(remote_storage)
-
-                cmd.append(
-                    f"--pageserver-config-override=remote_storage={remote_storage_toml_table}"
-                )
-
-            env_overrides = os.getenv("NEON_PAGESERVER_OVERRIDES")
-            if env_overrides is not None:
-                cmd += [
-                    f"--pageserver-config-override={o.strip()}" for o in env_overrides.split(";")
-                ]
-
-            if pageserver_config_override is not None:
-                cmd += [
-                    f"--pageserver-config-override={o.strip()}"
-                    for o in pageserver_config_override.split(";")
-                ]
-
             s3_env_vars = None
             if isinstance(remote_storage, S3Storage):
                 s3_env_vars = remote_storage.access_env_vars()
             res = self.raw_cli(cmd, extra_env_vars=s3_env_vars)
             res.check_returncode()
-            return res
+        return res
 
     def storage_controller_start(self):
         cmd = ["storage_controller", "start"]