From 8ab4c8a050895a8e6fe5ba53708d9f3c42e59ed4 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 11 Jan 2022 14:36:45 +0200 Subject: [PATCH] Code review fixes --- control_plane/src/lib.rs | 5 + control_plane/src/safekeeper.rs | 5 - control_plane/src/storage.rs | 9 -- pageserver/src/remote_storage/storage_sync.rs | 8 +- .../batch_others/test_remote_storage.py | 101 +++++++++--------- test_runner/fixtures/zenith_fixtures.py | 13 ++- 6 files changed, 69 insertions(+), 72 deletions(-) diff --git a/control_plane/src/lib.rs b/control_plane/src/lib.rs index 4254f9465cc9..a2ecdd3d6416 100644 --- a/control_plane/src/lib.rs +++ b/control_plane/src/lib.rs @@ -36,6 +36,11 @@ pub fn read_pidfile(pidfile: &Path) -> Result { fn fill_rust_env_vars(cmd: &mut Command) -> &mut Command { let cmd = cmd.env_clear().env("RUST_BACKTRACE", "1"); + let var = "LLVM_PROFILE_FILE"; + if let Some(val) = std::env::var_os(var) { + cmd.env(var, val); + } + const RUST_LOG_KEY: &str = "RUST_LOG"; if let Ok(rust_log_value) = std::env::var(RUST_LOG_KEY) { cmd.env(RUST_LOG_KEY, rust_log_value) diff --git a/control_plane/src/safekeeper.rs b/control_plane/src/safekeeper.rs index 1c57bd9f2687..94cbb4254c91 100644 --- a/control_plane/src/safekeeper.rs +++ b/control_plane/src/safekeeper.rs @@ -129,11 +129,6 @@ impl SafekeeperNode { cmd.arg("--no-sync"); } - let var = "LLVM_PROFILE_FILE"; - if let Some(val) = std::env::var_os(var) { - cmd.env(var, val); - } - if !cmd.status()?.success() { bail!( "Safekeeper failed to start. See '{}' for details.", diff --git a/control_plane/src/storage.rs b/control_plane/src/storage.rs index cf4b7d802e83..be594889ab02 100644 --- a/control_plane/src/storage.rs +++ b/control_plane/src/storage.rs @@ -102,10 +102,6 @@ impl PageServerNode { config_overrides: &[&str], ) -> anyhow::Result<()> { let mut cmd = Command::new(self.env.pageserver_bin()?); - let var = "LLVM_PROFILE_FILE"; - if let Some(val) = std::env::var_os(var) { - cmd.env(var, val); - } // FIXME: the paths should be shell-escaped to handle paths with spaces, quotas etc. let base_data_dir_param = self.env.base_data_dir.display().to_string(); @@ -180,11 +176,6 @@ impl PageServerNode { fill_rust_env_vars(cmd.args(&args).arg("--daemonize")); - let var = "LLVM_PROFILE_FILE"; - if let Some(val) = std::env::var_os(var) { - cmd.env(var, val); - } - if !cmd.status()?.success() { bail!( "Pageserver failed to start. See '{}' for details.", diff --git a/pageserver/src/remote_storage/storage_sync.rs b/pageserver/src/remote_storage/storage_sync.rs index f89964e4bf83..19590d16a7b4 100644 --- a/pageserver/src/remote_storage/storage_sync.rs +++ b/pageserver/src/remote_storage/storage_sync.rs @@ -426,7 +426,7 @@ fn storage_sync_loop< max_sync_errors: NonZeroU32, ) -> anyhow::Result<()> { let remote_assets = Arc::new((storage, RwLock::new(index))); - while !crate::tenant_mgr::shutdown_requested() { + loop { let loop_step = runtime.block_on(async { tokio::select! { new_timeline_states = loop_step( @@ -447,11 +447,13 @@ fn storage_sync_loop< set_timeline_states(conf, new_timeline_states); debug!("Sync loop step completed"); } - LoopStep::Shutdown => {} + LoopStep::Shutdown => { + debug!("Shutdown requested, stopping"); + break; + } } } - debug!("Shutdown requested, stopping"); Ok(()) } diff --git a/test_runner/batch_others/test_remote_storage.py b/test_runner/batch_others/test_remote_storage.py index 2a53eca55f45..f24048d4713d 100644 --- a/test_runner/batch_others/test_remote_storage.py +++ b/test_runner/batch_others/test_remote_storage.py @@ -26,64 +26,63 @@ # * queries the specific data, ensuring that it matches the one stored before # def test_remote_storage_backup_and_restore(zenith_env_builder: ZenithEnvBuilder): - with tempfile.TemporaryDirectory() as tmp_root_dir: - zenith_env_builder.rust_log_override = 'debug' - zenith_env_builder.pageserver_remote_storage = LocalFsStorage(Path(tmp_root_dir)) - zenith_env_builder.num_safekeepers = 1 + zenith_env_builder.rust_log_override = 'debug' + zenith_env_builder.num_safekeepers = 1 + zenith_env_builder.enable_local_fs_remote_storage() - data_id = 1 - data_secret = 'very secret secret' + data_id = 1 + data_secret = 'very secret secret' - ##### First start, insert secret data and upload it to the remote storage - env = zenith_env_builder.init() - pg = env.postgres.create_start() + ##### First start, insert secret data and upload it to the remote storage + env = zenith_env_builder.init() + pg = env.postgres.create_start() - tenant_id = pg.safe_psql("show zenith.zenith_tenant")[0][0] - timeline_id = pg.safe_psql("show zenith.zenith_timeline")[0][0] + tenant_id = pg.safe_psql("show zenith.zenith_tenant")[0][0] + timeline_id = pg.safe_psql("show zenith.zenith_timeline")[0][0] - with closing(pg.connect()) as conn: - with conn.cursor() as cur: - cur.execute(f''' - CREATE TABLE t1(id int primary key, secret text); - INSERT INTO t1 VALUES ({data_id}, '{data_secret}'); - ''') + with closing(pg.connect()) as conn: + with conn.cursor() as cur: + cur.execute(f''' + CREATE TABLE t1(id int primary key, secret text); + INSERT INTO t1 VALUES ({data_id}, '{data_secret}'); + ''') - # run checkpoint manually to be sure that data landed in remote storage - with closing(env.pageserver.connect()) as psconn: - with psconn.cursor() as pscur: - pscur.execute(f"do_gc {tenant_id} {timeline_id}") - log.info("waiting for upload") # TODO api to check if upload is done - time.sleep(2) + # run checkpoint manually to be sure that data landed in remote storage + with closing(env.pageserver.connect()) as psconn: + with psconn.cursor() as pscur: + pscur.execute(f"do_gc {tenant_id} {timeline_id}") + log.info("waiting for upload") # TODO api to check if upload is done + time.sleep(2) - ##### Stop the first pageserver instance, erase all its data - env.postgres.stop_all() - env.pageserver.stop() + ##### Stop the first pageserver instance, erase all its data + env.postgres.stop_all() + env.pageserver.stop() - dir_to_clear = Path(env.repo_dir) / 'tenants' - shutil.rmtree(dir_to_clear) - os.mkdir(dir_to_clear) + dir_to_clear = Path(env.repo_dir) / 'tenants' + shutil.rmtree(dir_to_clear) + os.mkdir(dir_to_clear) - ##### Second start, restore the data and ensure it's the same - env.pageserver.start() + ##### Second start, restore the data and ensure it's the same + env.pageserver.start() - log.info("waiting for timeline redownload") - client = env.pageserver.http_client() - attempts = 0 - while True: - timeline_details = client.timeline_details(tenant_id, timeline_id) - assert timeline_details['timeline_id'] == timeline_id - assert timeline_details['tenant_id'] == tenant_id - if timeline_details['type'] == 'Local': - log.info("timeline downloaded, checking its data") - break - attempts += 1 - if attempts > 10: - raise Exception("timeline redownload failed") - log.debug("still waiting") - time.sleep(1) + log.info("waiting for timeline redownload") + client = env.pageserver.http_client() + attempts = 0 + while True: + timeline_details = client.timeline_details(tenant_id, timeline_id) + assert timeline_details['timeline_id'] == timeline_id + assert timeline_details['tenant_id'] == tenant_id + if timeline_details['type'] == 'Local': + log.info("timeline downloaded, checking its data") + break + attempts += 1 + if attempts > 10: + raise Exception("timeline redownload failed") + log.debug("still waiting") + time.sleep(1) - pg = env.postgres.create_start() - with closing(pg.connect()) as conn: - with conn.cursor() as cur: - cur.execute(f'SELECT secret FROM t1 WHERE id = {data_id};') - assert cur.fetchone() == (data_secret, ) + pg = env.postgres.create_start() + with closing(pg.connect()) as conn: + with conn.cursor() as cur: + cur.execute(f'SELECT secret FROM t1 WHERE id = {data_id};') + assert cur.fetchone() == (data_secret, ) diff --git a/test_runner/fixtures/zenith_fixtures.py b/test_runner/fixtures/zenith_fixtures.py index eedc780fefc2..d2c5973bfcf6 100644 --- a/test_runner/fixtures/zenith_fixtures.py +++ b/test_runner/fixtures/zenith_fixtures.py @@ -360,6 +360,11 @@ def init(self) -> ZenithEnv: self.env = ZenithEnv(self) return self.env + def enable_local_fs_remote_storage(self): + assert self.pageserver_remote_storage is None, "remote storage is enabled already" + self.pageserver_remote_storage = LocalFsStorage( + Path(self.repo_dir / 'local_fs_remote_storage')) + def __enter__(self): return self @@ -472,7 +477,7 @@ def __init__(self, config: ZenithEnvBuilder): tmp.flush() cmd = ['init', f'--config={tmp.name}'] - _append_pageserver_param_overrides(cmd, config.pageserver_remote_storage) + append_pageserver_param_overrides(cmd, config.pageserver_remote_storage) self.zenith_cli(cmd) @@ -744,7 +749,7 @@ def start(self) -> 'ZenithPageserver': assert self.running == False start_args = ['pageserver', 'start'] - _append_pageserver_param_overrides(start_args, self.remote_storage) + append_pageserver_param_overrides(start_args, self.remote_storage) self.env.zenith_cli(start_args) self.running = True @@ -779,8 +784,8 @@ def http_client(self, auth_token: Optional[str] = None) -> ZenithPageserverHttpC ) -def _append_pageserver_param_overrides(params_to_update: List[str], - pageserver_remote_storage: Optional[RemoteStorage]): +def append_pageserver_param_overrides(params_to_update: List[str], + pageserver_remote_storage: Optional[RemoteStorage]): if pageserver_remote_storage is not None: if isinstance(pageserver_remote_storage, LocalFsStorage): pageserver_storage_override = f"local_path='{pageserver_remote_storage.root}'"