From 087526b81be6f428fd7f4cdf42b4e941469d5a38 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Thu, 11 Jan 2024 19:11:44 +0100 Subject: [PATCH] neon_local init: add `--force` mode that allows an empty dir (#6328) Need this in https://github.com/neondatabase/neon/pull/6214 refs https://github.com/neondatabase/neon/issues/5771 --- control_plane/src/bin/neon_local.rs | 16 ++++-- control_plane/src/local_env.rs | 71 ++++++++++++++++++++------- test_runner/fixtures/neon_fixtures.py | 7 ++- 3 files changed, 70 insertions(+), 24 deletions(-) diff --git a/control_plane/src/bin/neon_local.rs b/control_plane/src/bin/neon_local.rs index 03e69010f7..dd09d47655 100644 --- a/control_plane/src/bin/neon_local.rs +++ b/control_plane/src/bin/neon_local.rs @@ -6,11 +6,11 @@ //! rely on `neon_local` to set up the environment for each test. //! use anyhow::{anyhow, bail, Context, Result}; -use clap::{value_parser, Arg, ArgAction, ArgMatches, Command}; +use clap::{value_parser, Arg, ArgAction, ArgMatches, Command, ValueEnum}; use compute_api::spec::ComputeMode; use control_plane::attachment_service::AttachmentService; use control_plane::endpoint::ComputeControlPlane; -use control_plane::local_env::LocalEnv; +use control_plane::local_env::{InitForceMode, LocalEnv}; use control_plane::pageserver::{PageServerNode, PAGESERVER_REMOTE_STORAGE_DIR}; use control_plane::safekeeper::SafekeeperNode; use control_plane::tenant_migration::migrate_tenant; @@ -338,7 +338,7 @@ fn handle_init(init_match: &ArgMatches) -> anyhow::Result { let mut env = LocalEnv::parse_config(&toml_file).context("Failed to create neon configuration")?; - let force = init_match.get_flag("force"); + let force = init_match.get_one("force").expect("we set a default value"); env.init(pg_version, force) .context("Failed to initialize neon repository")?; @@ -1266,9 +1266,15 @@ fn cli() -> Command { .required(false); let force_arg = Arg::new("force") - .value_parser(value_parser!(bool)) + .value_parser(value_parser!(InitForceMode)) .long("force") - .action(ArgAction::SetTrue) + .default_value( + InitForceMode::MustNotExist + .to_possible_value() + .unwrap() + .get_name() + .to_owned(), + ) .help("Force initialization even if the repository is not empty") .required(false); diff --git a/control_plane/src/local_env.rs b/control_plane/src/local_env.rs index b9c8aeddcb..c87875457c 100644 --- a/control_plane/src/local_env.rs +++ b/control_plane/src/local_env.rs @@ -5,6 +5,7 @@ use anyhow::{bail, ensure, Context}; +use clap::ValueEnum; use postgres_backend::AuthType; use reqwest::Url; use serde::{Deserialize, Serialize}; @@ -162,6 +163,31 @@ impl Default for SafekeeperConf { } } +#[derive(Clone, Copy)] +pub enum InitForceMode { + MustNotExist, + EmptyDirOk, + RemoveAllContents, +} + +impl ValueEnum for InitForceMode { + fn value_variants<'a>() -> &'a [Self] { + &[ + Self::MustNotExist, + Self::EmptyDirOk, + Self::RemoveAllContents, + ] + } + + fn to_possible_value(&self) -> Option { + Some(clap::builder::PossibleValue::new(match self { + InitForceMode::MustNotExist => "must-not-exist", + InitForceMode::EmptyDirOk => "empty-dir-ok", + InitForceMode::RemoveAllContents => "remove-all-contents", + })) + } +} + impl SafekeeperConf { /// Compute is served by port on which only tenant scoped tokens allowed, if /// it is configured. @@ -384,7 +410,7 @@ impl LocalEnv { // // Initialize a new Neon repository // - pub fn init(&mut self, pg_version: u32, force: bool) -> anyhow::Result<()> { + pub fn init(&mut self, pg_version: u32, force: &InitForceMode) -> anyhow::Result<()> { // check if config already exists let base_path = &self.base_data_dir; ensure!( @@ -393,25 +419,34 @@ impl LocalEnv { ); if base_path.exists() { - if force { - println!("removing all contents of '{}'", base_path.display()); - // instead of directly calling `remove_dir_all`, we keep the original dir but removing - // all contents inside. This helps if the developer symbol links another directory (i.e., - // S3 local SSD) to the `.neon` base directory. - for entry in std::fs::read_dir(base_path)? { - let entry = entry?; - let path = entry.path(); - if path.is_dir() { - fs::remove_dir_all(&path)?; - } else { - fs::remove_file(&path)?; + match force { + InitForceMode::MustNotExist => { + bail!( + "directory '{}' already exists. Perhaps already initialized?", + base_path.display() + ); + } + InitForceMode::EmptyDirOk => { + if let Some(res) = std::fs::read_dir(base_path)?.next() { + res.context("check if directory is empty")?; + anyhow::bail!("directory not empty: {base_path:?}"); + } + } + InitForceMode::RemoveAllContents => { + println!("removing all contents of '{}'", base_path.display()); + // instead of directly calling `remove_dir_all`, we keep the original dir but removing + // all contents inside. This helps if the developer symbol links another directory (i.e., + // S3 local SSD) to the `.neon` base directory. + for entry in std::fs::read_dir(base_path)? { + let entry = entry?; + let path = entry.path(); + if path.is_dir() { + fs::remove_dir_all(&path)?; + } else { + fs::remove_file(&path)?; + } } } - } else { - bail!( - "directory '{}' already exists. Perhaps already initialized? (Hint: use --force to remove all contents)", - base_path.display() - ); } } diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index 0fa4cfb18a..111070bd93 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -472,6 +472,7 @@ class NeonEnvBuilder: self.test_output_dir = test_output_dir self.test_overlay_dir = test_overlay_dir self.overlay_mounts_created_by_us: List[Tuple[str, Path]] = [] + self.config_init_force: Optional[str] = None assert test_name.startswith( "test_" @@ -935,7 +936,7 @@ class NeonEnv: cfg["safekeepers"].append(sk_cfg) log.info(f"Config: {cfg}") - self.neon_cli.init(cfg) + self.neon_cli.init(cfg, force=config.config_init_force) def start(self): # Start up broker, pageserver and all safekeepers @@ -1423,6 +1424,7 @@ class NeonCli(AbstractNeonCli): def init( self, config: Dict[str, Any], + force: Optional[str] = None, ) -> "subprocess.CompletedProcess[str]": with tempfile.NamedTemporaryFile(mode="w+") as tmp: tmp.write(toml.dumps(config)) @@ -1430,6 +1432,9 @@ class NeonCli(AbstractNeonCli): cmd = ["init", f"--config={tmp.name}", "--pg-version", self.env.pg_version] + if force is not None: + cmd.extend(["--force", force]) + storage = self.env.pageserver_remote_storage append_pageserver_param_overrides(