From aacf8110a0d6ce31342334708f48e9738cd8b3aa Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Sat, 4 May 2024 16:11:18 +0000 Subject: [PATCH] pretty up the inlined override code, long option --config-override --- control_plane/src/pageserver.rs | 216 ++++++++++++++----------------- pageserver/src/bin/pageserver.rs | 1 + 2 files changed, 97 insertions(+), 120 deletions(-) diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index 94d6ab0264..f179226b91 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; @@ -97,7 +96,7 @@ impl PageServerNode { self.start_node(config_overrides).await } - fn pageserver_init(&self, config_overrides: &[&str]) -> anyhow::Result<()> { + fn pageserver_init(&self, cli_overrides: &[&str]) -> anyhow::Result<()> { let datadir = self.repo_path(); let node_id = self.conf.id; println!( @@ -115,128 +114,105 @@ 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 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) = &self.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 = self + .env + .generate_auth_token(&Claims::new(None, Scope::GenerationsApi)) + .unwrap(); + overrides.push(format!("control_plane_api_token='{}'", jwt_token)); + } + } + // Pageserver doesn't support running without a remote storage anymore. + if !cli_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(cli_overrides.iter().map(|&c| c.to_owned())); + + // `pageserver --init` merges the `--config-override`s into a built-in 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", "--workdir", datadir_path_str]; + for piece in &overrides { + 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}"))?; diff --git a/pageserver/src/bin/pageserver.rs b/pageserver/src/bin/pageserver.rs index e9433de05b..eb4b8bb8bb 100644 --- a/pageserver/src/bin/pageserver.rs +++ b/pageserver/src/bin/pageserver.rs @@ -755,6 +755,7 @@ fn cli() -> Command { // See `settings.md` for more details on the extra configuration patameters pageserver can process .arg( Arg::new("config-override") + .long("config-override") .short('c') .num_args(1) .action(ArgAction::Append)