From 7363b44b5042c501ca8045d8a785db0d400302c1 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Sat, 4 May 2024 15:13:28 +0000 Subject: [PATCH] neon_local: remove --pageserver-config-overrides, `neon_local init` takes a toml tempfile --- Cargo.lock | 1 + control_plane/Cargo.toml | 1 + control_plane/src/bin/neon_local.rs | 32 ++-- control_plane/src/pageserver.rs | 224 +++++++++++--------------- pageserver/src/bin/pageserver.rs | 6 +- test_runner/fixtures/neon_fixtures.py | 64 ++++---- 6 files changed, 160 insertions(+), 168 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8438dad41b..b0c7aec6ae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1348,6 +1348,7 @@ dependencies = [ "tokio-postgres", "tokio-util", "toml", + "toml_edit", "tracing", "url", "utils", diff --git a/control_plane/Cargo.toml b/control_plane/Cargo.toml index 2ce041068e..e62f3b8a47 100644 --- a/control_plane/Cargo.toml +++ b/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 diff --git a/control_plane/src/bin/neon_local.rs b/control_plane/src/bin/neon_local.rs index a307e7cceb..c8f8ef3646 100644 --- a/control_plane/src/bin/neon_local.rs +++ b/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 { default_conf(*num_pageservers) }; + let pageserver_config_overrides: toml_edit::Document = + if let Some(path) = init_match.get_one::("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::("pg-version") .copied() @@ -375,7 +385,7 @@ fn handle_init(init_match: &ArgMatches) -> anyhow::Result { // 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") diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index dd6c8cf0a3..4b1dca2170 100644 --- a/control_plane/src/pageserver.rs +++ b/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, diff --git a/pageserver/src/bin/pageserver.rs b/pageserver/src/bin/pageserver.rs index c71f436cf3..1bd7fac343 100644 --- a/pageserver/src/bin/pageserver.rs +++ b/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}'") } diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index c3e16385e7..dbb9fa9bd3 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/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"]