From d7ab69f303e51ca7f8669fb7952d13f3234606c8 Mon Sep 17 00:00:00 2001 From: andres Date: Sun, 6 Nov 2022 19:15:18 +0100 Subject: [PATCH 01/32] add test for getting branchpoints from an inactive timeline --- pageserver/src/tenant.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 9e9c98ad62..aa77196f2c 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -3176,6 +3176,35 @@ mod tests { } */ + #[tokio::test] + async fn test_get_branchpoints_from_an_inactive_timeline() -> anyhow::Result<()> { + let tenant = + TenantHarness::create("test_get_branchpoints_from_an_inactive_timeline")?.load(); + let tline = tenant + .create_empty_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION)? + .initialize()?; + tline.set_state(TimelineState::Paused); + make_some_layers(tline.as_ref(), Lsn(0x20)).await?; + + tenant.branch_timeline(TIMELINE_ID, NEW_TIMELINE_ID, Some(Lsn(0x40)))?; + let newtline = tenant + .get_timeline(NEW_TIMELINE_ID, true) + .expect("Should have a local timeline"); + + make_some_layers(newtline.as_ref(), Lsn(0x60)).await?; + tenant.gc_iteration(Some(TIMELINE_ID), 0x10, Duration::ZERO, false).await?; + + assert_eq!( + newtline.get(*TEST_KEY, Lsn(0x50))?, + TEST_IMG(&format!("foo at {}", Lsn(0x40)))); + + let branchpoints = &tline.gc_info.read().unwrap().retain_lsns; + assert_eq!(branchpoints.len(), 1); + assert_eq!(branchpoints[0], Lsn(0x40)); + + Ok(()) + } + #[tokio::test] async fn test_retain_data_in_parent_which_is_needed_for_child() -> anyhow::Result<()> { let (tenant, ctx) = From 50476a7cc77b375e3aa793ed93155715dfd1241e Mon Sep 17 00:00:00 2001 From: Dmitry Rodionov Date: Mon, 13 Mar 2023 14:30:43 +0200 Subject: [PATCH 02/32] test: update to match current interfaces --- pageserver/src/tenant.rs | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index aa77196f2c..6a373ad520 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -3178,30 +3178,39 @@ mod tests { #[tokio::test] async fn test_get_branchpoints_from_an_inactive_timeline() -> anyhow::Result<()> { - let tenant = - TenantHarness::create("test_get_branchpoints_from_an_inactive_timeline")?.load(); + let (tenant, ctx) = + TenantHarness::create("test_get_branchpoints_from_an_inactive_timeline")? + .load() + .await; let tline = tenant - .create_empty_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION)? - .initialize()?; - tline.set_state(TimelineState::Paused); + .create_empty_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)? + .initialize(&ctx)?; make_some_layers(tline.as_ref(), Lsn(0x20)).await?; - tenant.branch_timeline(TIMELINE_ID, NEW_TIMELINE_ID, Some(Lsn(0x40)))?; + tenant + .branch_timeline(&tline, NEW_TIMELINE_ID, Some(Lsn(0x40)), &ctx) + .await?; let newtline = tenant .get_timeline(NEW_TIMELINE_ID, true) .expect("Should have a local timeline"); make_some_layers(newtline.as_ref(), Lsn(0x60)).await?; - tenant.gc_iteration(Some(TIMELINE_ID), 0x10, Duration::ZERO, false).await?; - + + tline.set_state(TimelineState::Broken); + + tenant + .gc_iteration(Some(TIMELINE_ID), 0x10, Duration::ZERO, &ctx) + .await?; + assert_eq!( - newtline.get(*TEST_KEY, Lsn(0x50))?, - TEST_IMG(&format!("foo at {}", Lsn(0x40)))); + newtline.get(*TEST_KEY, Lsn(0x50), &ctx).await?, + TEST_IMG(&format!("foo at {}", Lsn(0x40))) + ); let branchpoints = &tline.gc_info.read().unwrap().retain_lsns; assert_eq!(branchpoints.len(), 1); assert_eq!(branchpoints[0], Lsn(0x40)); - + Ok(()) } From 15ed6af5f286b7e082b3ba44e26e6e66fa71d04e Mon Sep 17 00:00:00 2001 From: Dmitry Ivanov Date: Fri, 10 Mar 2023 23:10:55 +0300 Subject: [PATCH 03/32] Add descriptions to proxy's python tests. --- test_runner/regress/test_proxy.py | 45 +++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/test_runner/regress/test_proxy.py b/test_runner/regress/test_proxy.py index 99a3f2fa86..51fabdd2a1 100644 --- a/test_runner/regress/test_proxy.py +++ b/test_runner/regress/test_proxy.py @@ -4,10 +4,20 @@ from fixtures.neon_fixtures import PSQL, NeonProxy, VanillaPostgres def test_proxy_select_1(static_proxy: NeonProxy): - static_proxy.safe_psql("select 1", options="project=generic-project-name") + """ + A simplest smoke test: check proxy against a local postgres instance. + """ + + out = static_proxy.safe_psql("select 1", options="project=generic-project-name") + assert out[0][0] == 1 def test_password_hack(static_proxy: NeonProxy): + """ + Check the PasswordHack auth flow: an alternative to SCRAM auth for + clients which can't provide the project/endpoint name via SNI or `options`. + """ + user = "borat" password = "password" static_proxy.safe_psql( @@ -25,7 +35,11 @@ def test_password_hack(static_proxy: NeonProxy): @pytest.mark.asyncio -async def test_psql_session_id(vanilla_pg: VanillaPostgres, link_proxy: NeonProxy): +async def test_link_auth(vanilla_pg: VanillaPostgres, link_proxy: NeonProxy): + """ + Check the Link auth flow: a lightweight auth method which delegates + all necessary checks to the console by sending client an auth URL. + """ psql = await PSQL(host=link_proxy.host, port=link_proxy.proxy_port).run("select 42") @@ -40,16 +54,27 @@ async def test_psql_session_id(vanilla_pg: VanillaPostgres, link_proxy: NeonProx assert out == "42" -# Pass extra options to the server. def test_proxy_options(static_proxy: NeonProxy): - with static_proxy.connect(options="project=irrelevant -cproxytest.option=value") as conn: - with conn.cursor() as cur: - cur.execute("SHOW proxytest.option") - value = cur.fetchall()[0][0] - assert value == "value" + """ + Check that we pass extra `options` to the PostgreSQL server: + * `project=...` shouldn't be passed at all (otherwise postgres will raise an error). + * everything else should be passed as-is. + """ + + options = "project=irrelevant -cproxytest.option=value" + out = static_proxy.safe_psql("show proxytest.option", options=options) + assert out[0][0] == "value" + + options = "-c proxytest.foo=\\ str project=irrelevant" + out = static_proxy.safe_psql("show proxytest.foo", options=options) + assert out[0][0] == " str" def test_auth_errors(static_proxy: NeonProxy): + """ + Check that we throw very specific errors in some unsuccessful auth scenarios. + """ + # User does not exist with pytest.raises(psycopg2.Error) as exprinfo: static_proxy.connect(user="pinocchio", options="project=irrelevant") @@ -78,6 +103,10 @@ def test_auth_errors(static_proxy: NeonProxy): def test_forward_params_to_client(static_proxy: NeonProxy): + """ + Check that we forward all necessary PostgreSQL server params to client. + """ + # A subset of parameters (GUCs) which postgres # sends to the client during connection setup. # Unfortunately, `GUC_REPORT` can't be queried. From 2e4bf7cee4e43a868ad810c364efd9ea50889c4f Mon Sep 17 00:00:00 2001 From: Dmitry Ivanov Date: Fri, 10 Mar 2023 23:51:06 +0300 Subject: [PATCH 04/32] [proxy] Immediately log all compute node connection errors. --- proxy/src/compute.rs | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/proxy/src/compute.rs b/proxy/src/compute.rs index 2e12d9ee26..b5efc72803 100644 --- a/proxy/src/compute.rs +++ b/proxy/src/compute.rs @@ -6,9 +6,9 @@ use std::{io, net::SocketAddr}; use thiserror::Error; use tokio::net::TcpStream; use tokio_postgres::NoTls; -use tracing::{error, info}; +use tracing::{error, info, warn}; -const COULD_NOT_CONNECT: &str = "Could not connect to compute node"; +const COULD_NOT_CONNECT: &str = "Couldn't connect to compute node"; #[derive(Debug, Error)] pub enum ConnectionError { @@ -131,7 +131,7 @@ impl ConnCfg { use tokio_postgres::config::Host; let connect_once = |host, port| { - info!("trying to connect to a compute node at {host}:{port}"); + info!("trying to connect to compute node at {host}:{port}"); TcpStream::connect((host, port)).and_then(|socket| async { let socket_addr = socket.peer_addr()?; // This prevents load balancer from severing the connection. @@ -151,7 +151,7 @@ impl ConnCfg { return Err(io::Error::new( io::ErrorKind::Other, format!( - "couldn't connect: bad compute config, \ + "bad compute config, \ ports and hosts entries' count does not match: {:?}", self.0 ), @@ -170,7 +170,7 @@ impl ConnCfg { Ok(socket) => return Ok(socket), Err(err) => { // We can't throw an error here, as there might be more hosts to try. - error!("failed to connect to a compute node at {host}:{port}: {err}"); + warn!("couldn't connect to compute node at {host}:{port}: {err}"); connection_error = Some(err); } } @@ -179,7 +179,7 @@ impl ConnCfg { Err(connection_error.unwrap_or_else(|| { io::Error::new( io::ErrorKind::Other, - format!("couldn't connect: bad compute config: {:?}", self.0), + format!("bad compute config: {:?}", self.0), ) })) } @@ -195,12 +195,11 @@ pub struct PostgresConnection { } impl ConnCfg { - /// Connect to a corresponding compute node. - pub async fn connect(&self) -> Result { + async fn do_connect(&self) -> Result { // TODO: establish a secure connection to the DB. let (socket_addr, mut stream) = self.connect_raw().await?; let (client, connection) = self.0.connect_raw(&mut stream, NoTls).await?; - info!("connected to user's compute node at {socket_addr}"); + info!("connected to compute node at {socket_addr}"); // This is very ugly but as of now there's no better way to // extract the connection parameters from tokio-postgres' connection. @@ -219,6 +218,16 @@ impl ConnCfg { Ok(connection) } + + /// Connect to a corresponding compute node. + pub async fn connect(&self) -> Result { + self.do_connect() + .inspect_err(|err| { + // Immediately log the error we have at our disposal. + error!("couldn't connect to compute node: {err}"); + }) + .await + } } /// Retrieve `options` from a startup message, dropping all proxy-secific flags. From 319402fc746355a8e24603e6a9e9f06d2ca99c57 Mon Sep 17 00:00:00 2001 From: Alexander Bayandin Date: Tue, 14 Mar 2023 11:25:48 +0100 Subject: [PATCH 05/32] postgres_ffi: restore POSTGRES_INSTALL_DIR support (#3811) Fix path construction to `pg_config`: `pg_install_dir_versioned` already includes `pg_version` --- libs/postgres_ffi/build.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/libs/postgres_ffi/build.rs b/libs/postgres_ffi/build.rs index 25ff398bbd..66221af522 100644 --- a/libs/postgres_ffi/build.rs +++ b/libs/postgres_ffi/build.rs @@ -63,10 +63,7 @@ fn main() -> anyhow::Result<()> { pg_install_dir_versioned = cwd.join("..").join("..").join(pg_install_dir_versioned); } - let pg_config_bin = pg_install_dir_versioned - .join(pg_version) - .join("bin") - .join("pg_config"); + let pg_config_bin = pg_install_dir_versioned.join("bin").join("pg_config"); let inc_server_path: String = if pg_config_bin.exists() { let output = Command::new(pg_config_bin) .arg("--includedir-server") From d6bb8caad46e3438eb339e9d17e3f70cf3cf9db7 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Tue, 14 Mar 2023 13:18:26 +0200 Subject: [PATCH 06/32] refactor: correct return value for not found L0's on LayerMap::replace (#3805) in prev implementation, an `ok_or_else(...)?` is used to cause a "precondition error" on LayerMap::replace, however we only see this particular error if an L0 for which replace fails is not in the layermap because it is not in `l0_delta_layers`. changes or fixes this to be Replacement::NotFound instead, making it more clear that an error would only be raised for actual preconditions, like trying to replace layer with completly unrelated layer. --- pageserver/src/tenant/layer_map.rs | 44 ++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/pageserver/src/tenant/layer_map.rs b/pageserver/src/tenant/layer_map.rs index 8d7d9c6f8f..4c659be9aa 100644 --- a/pageserver/src/tenant/layer_map.rs +++ b/pageserver/src/tenant/layer_map.rs @@ -154,11 +154,7 @@ where expected: &Arc, new: Arc, ) -> anyhow::Result>> { - fail::fail_point!("layermap-replace-notfound", |_| Ok( - // this is not what happens if an L0 layer was not found a anyhow error but perhaps - // that should be changed. this is good enough to show a replacement failure. - Replacement::NotFound - )); + fail::fail_point!("layermap-replace-notfound", |_| Ok(Replacement::NotFound)); self.layer_map.replace_historic_noflush(expected, new) } @@ -340,12 +336,15 @@ where let l0_index = if expected_l0 { // find the index in case replace worked, we need to replace that as well - Some( - self.l0_delta_layers - .iter() - .position(|slot| Self::compare_arced_layers(slot, expected)) - .ok_or_else(|| anyhow::anyhow!("existing l0 delta layer was not found"))?, - ) + let pos = self + .l0_delta_layers + .iter() + .position(|slot| Self::compare_arced_layers(slot, expected)); + + if pos.is_none() { + return Ok(Replacement::NotFound); + } + pos } else { None }; @@ -804,6 +803,26 @@ mod tests { ) } + #[test] + fn replacing_missing_l0_is_notfound() { + // original impl had an oversight, and L0 was an anyhow::Error. anyhow::Error should + // however only happen for precondition failures. + + let layer = "000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000053423C21-0000000053424D69"; + let layer = LayerFileName::from_str(layer).unwrap(); + let layer = LayerDescriptor::from(layer); + + // same skeletan construction; see scenario below + let not_found: Arc = Arc::new(layer.clone()); + let new_version: Arc = Arc::new(layer); + + let mut map = LayerMap::default(); + + let res = map.batch_update().replace_historic(¬_found, new_version); + + assert!(matches!(res, Ok(Replacement::NotFound)), "{res:?}"); + } + fn l0_delta_layers_updated_scenario(layer_name: &str, expected_l0: bool) { let name = LayerFileName::from_str(layer_name).unwrap(); let skeleton = LayerDescriptor::from(name); @@ -813,7 +832,8 @@ mod tests { let mut map = LayerMap::default(); - // two disjoint Arcs in different lifecycle phases. + // two disjoint Arcs in different lifecycle phases. even if it seems they must be the + // same layer, we use LayerMap::compare_arced_layers as the identity of layers. assert!(!LayerMap::compare_arced_layers(&remote, &downloaded)); let expected_in_counts = (1, usize::from(expected_l0)); From 68ae020b3725eaac7cac80cdb512718047883341 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lassi=20P=C3=B6l=C3=B6nen?= Date: Tue, 14 Mar 2023 15:23:46 +0200 Subject: [PATCH 07/32] Use RollingUpdate strategy also for legacy proxy (#3814) ## Describe your changes We have previously changed the neon-proxy to use RollingUpdate. This should be enabled in legacy proxy too in order to avoid breaking connections for the clients and allow for example backups to run even during deployment. (https://github.com/neondatabase/neon/pull/3683) ## Issue ticket number and link https://github.com/neondatabase/neon/issues/3333 --- ...od-us-west-2-eta.neon-proxy-scram-legacy.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/.github/helm-values/prod-us-west-2-eta.neon-proxy-scram-legacy.yaml b/.github/helm-values/prod-us-west-2-eta.neon-proxy-scram-legacy.yaml index e67a3e4461..d23ea41bd7 100644 --- a/.github/helm-values/prod-us-west-2-eta.neon-proxy-scram-legacy.yaml +++ b/.github/helm-values/prod-us-west-2-eta.neon-proxy-scram-legacy.yaml @@ -1,6 +1,22 @@ # Helm chart values for neon-proxy-scram. # This is a YAML-formatted file. +deploymentStrategy: + type: RollingUpdate + rollingUpdate: + maxSurge: 100% + maxUnavailable: 50% + +# Delay the kill signal by 7 days (7 * 24 * 60 * 60) +# The pod(s) will stay in Terminating, keeps the existing connections +# but doesn't receive new ones +containerLifecycle: + preStop: + exec: + command: ["/bin/sh", "-c", "sleep 604800"] +terminationGracePeriodSeconds: 604800 + + image: repository: neondatabase/neon From 3d869cbcde47ebcadf1d5250509e5dda1a491d77 Mon Sep 17 00:00:00 2001 From: Alexander Bayandin Date: Tue, 14 Mar 2023 14:25:44 +0100 Subject: [PATCH 08/32] Replace flake8 and isort with ruff (#3810) - Introduce ruff (https://beta.ruff.rs/) to replace flake8 and isort - Update mypy and black --- .github/workflows/build_and_test.yml | 7 +- docs/sourcetree.md | 5 +- poetry.lock | 264 ++++++++---------- pre-commit.py | 23 +- pyproject.toml | 27 +- scripts/export_import_between_pageservers.py | 5 +- scripts/reformat | 3 +- scripts/sk_cleanup_tenants/script.py | 1 - setup.cfg | 8 - test_runner/fixtures/benchmark_fixture.py | 1 + test_runner/fixtures/compare_fixtures.py | 9 +- test_runner/fixtures/neon_fixtures.py | 20 +- test_runner/fixtures/types.py | 4 +- test_runner/fixtures/utils.py | 3 +- test_runner/performance/test_branching.py | 1 + test_runner/performance/test_bulk_update.py | 1 - .../performance/test_compare_pg_stats.py | 1 + test_runner/performance/test_gist_build.py | 1 - test_runner/performance/test_latency.py | 1 + test_runner/performance/test_layer_map.py | 1 - .../performance/test_parallel_copy_to.py | 1 - .../performance/test_wal_backpressure.py | 1 + .../performance/test_write_amplification.py | 1 - test_runner/regress/test_crafted_wal_end.py | 1 + test_runner/regress/test_fullbackup.py | 7 +- test_runner/regress/test_gc_aggressive.py | 2 - test_runner/regress/test_layer_eviction.py | 1 - test_runner/regress/test_ondemand_download.py | 2 +- test_runner/regress/test_read_validation.py | 2 - test_runner/regress/test_readonly_node.py | 2 +- test_runner/regress/test_remote_storage.py | 2 - test_runner/regress/test_tenant_detach.py | 1 + test_runner/regress/test_tenant_relocation.py | 1 - test_runner/regress/test_timeline_delete.py | 1 - test_runner/regress/test_timeline_size.py | 4 - test_runner/regress/test_truncate.py | 1 - test_runner/regress/test_wal_acceptor.py | 1 - .../regress/test_wal_acceptor_async.py | 2 - test_runner/regress/test_wal_restore.py | 7 +- 39 files changed, 177 insertions(+), 249 deletions(-) delete mode 100644 setup.cfg diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index d479201305..e056cf0fcf 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -74,15 +74,12 @@ jobs: - name: Install Python deps run: ./scripts/pysync - - name: Run isort to ensure code format - run: poetry run isort --diff --check . + - name: Run ruff to ensure code format + run: poetry run ruff . - name: Run black to ensure code format run: poetry run black --diff --check . - - name: Run flake8 to ensure code format - run: poetry run flake8 . - - name: Run mypy to check types run: poetry run mypy . diff --git a/docs/sourcetree.md b/docs/sourcetree.md index db57338a71..95bed83ae5 100644 --- a/docs/sourcetree.md +++ b/docs/sourcetree.md @@ -129,13 +129,12 @@ Run `poetry shell` to activate the virtual environment. Alternatively, use `poetry run` to run a single command in the venv, e.g. `poetry run pytest`. ### Obligatory checks -We force code formatting via `black`, `isort` and type hints via `mypy`. +We force code formatting via `black`, `ruff`, and type hints via `mypy`. Run the following commands in the repository's root (next to `pyproject.toml`): ```bash -poetry run isort . # Imports are reformatted poetry run black . # All code is reformatted -poetry run flake8 . # Python linter +poetry run ruff . # Python linter poetry run mypy . # Ensure there are no typing errors ``` diff --git a/poetry.lock b/poetry.lock index bc2c56d74c..011d5d7817 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1,4 +1,4 @@ -# This file is automatically @generated by Poetry and should not be changed by hand. +# This file is automatically @generated by Poetry 1.4.0 and should not be changed by hand. [[package]] name = "aiohttp" @@ -253,43 +253,46 @@ files = [ [[package]] name = "black" -version = "22.6.0" +version = "23.1.0" description = "The uncompromising code formatter." category = "dev" optional = false -python-versions = ">=3.6.2" +python-versions = ">=3.7" files = [ - {file = "black-22.6.0-cp310-cp310-macosx_10_9_universal2.whl", hash = "sha256:f586c26118bc6e714ec58c09df0157fe2d9ee195c764f630eb0d8e7ccce72e69"}, - {file = "black-22.6.0-cp310-cp310-macosx_10_9_x86_64.whl", hash = "sha256:b270a168d69edb8b7ed32c193ef10fd27844e5c60852039599f9184460ce0807"}, - {file = "black-22.6.0-cp310-cp310-macosx_11_0_arm64.whl", hash = "sha256:6797f58943fceb1c461fb572edbe828d811e719c24e03375fd25170ada53825e"}, - {file = "black-22.6.0-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:c85928b9d5f83b23cee7d0efcb310172412fbf7cb9d9ce963bd67fd141781def"}, - {file = "black-22.6.0-cp310-cp310-win_amd64.whl", hash = "sha256:f6fe02afde060bbeef044af7996f335fbe90b039ccf3f5eb8f16df8b20f77666"}, - {file = "black-22.6.0-cp36-cp36m-macosx_10_9_x86_64.whl", hash = "sha256:cfaf3895a9634e882bf9d2363fed5af8888802d670f58b279b0bece00e9a872d"}, - {file = "black-22.6.0-cp36-cp36m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:94783f636bca89f11eb5d50437e8e17fbc6a929a628d82304c80fa9cd945f256"}, - {file = "black-22.6.0-cp36-cp36m-win_amd64.whl", hash = "sha256:2ea29072e954a4d55a2ff58971b83365eba5d3d357352a07a7a4df0d95f51c78"}, - {file = "black-22.6.0-cp37-cp37m-macosx_10_9_x86_64.whl", hash = "sha256:e439798f819d49ba1c0bd9664427a05aab79bfba777a6db94fd4e56fae0cb849"}, - {file = "black-22.6.0-cp37-cp37m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:187d96c5e713f441a5829e77120c269b6514418f4513a390b0499b0987f2ff1c"}, - {file = "black-22.6.0-cp37-cp37m-win_amd64.whl", hash = "sha256:074458dc2f6e0d3dab7928d4417bb6957bb834434516f21514138437accdbe90"}, - {file = "black-22.6.0-cp38-cp38-macosx_10_9_universal2.whl", hash = "sha256:a218d7e5856f91d20f04e931b6f16d15356db1c846ee55f01bac297a705ca24f"}, - {file = "black-22.6.0-cp38-cp38-macosx_10_9_x86_64.whl", hash = "sha256:568ac3c465b1c8b34b61cd7a4e349e93f91abf0f9371eda1cf87194663ab684e"}, - {file = "black-22.6.0-cp38-cp38-macosx_11_0_arm64.whl", hash = "sha256:6c1734ab264b8f7929cef8ae5f900b85d579e6cbfde09d7387da8f04771b51c6"}, - {file = "black-22.6.0-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:c9a3ac16efe9ec7d7381ddebcc022119794872abce99475345c5a61aa18c45ad"}, - {file = "black-22.6.0-cp38-cp38-win_amd64.whl", hash = "sha256:b9fd45787ba8aa3f5e0a0a98920c1012c884622c6c920dbe98dbd05bc7c70fbf"}, - {file = "black-22.6.0-cp39-cp39-macosx_10_9_universal2.whl", hash = "sha256:7ba9be198ecca5031cd78745780d65a3f75a34b2ff9be5837045dce55db83d1c"}, - {file = "black-22.6.0-cp39-cp39-macosx_10_9_x86_64.whl", hash = "sha256:a3db5b6409b96d9bd543323b23ef32a1a2b06416d525d27e0f67e74f1446c8f2"}, - {file = "black-22.6.0-cp39-cp39-macosx_11_0_arm64.whl", hash = "sha256:560558527e52ce8afba936fcce93a7411ab40c7d5fe8c2463e279e843c0328ee"}, - {file = "black-22.6.0-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:b154e6bbde1e79ea3260c4b40c0b7b3109ffcdf7bc4ebf8859169a6af72cd70b"}, - {file = "black-22.6.0-cp39-cp39-win_amd64.whl", hash = "sha256:4af5bc0e1f96be5ae9bd7aaec219c901a94d6caa2484c21983d043371c733fc4"}, - {file = "black-22.6.0-py3-none-any.whl", hash = "sha256:ac609cf8ef5e7115ddd07d85d988d074ed00e10fbc3445aee393e70164a2219c"}, - {file = "black-22.6.0.tar.gz", hash = "sha256:6c6d39e28aed379aec40da1c65434c77d75e65bb59a1e1c283de545fb4e7c6c9"}, + {file = "black-23.1.0-cp310-cp310-macosx_10_16_arm64.whl", hash = "sha256:b6a92a41ee34b883b359998f0c8e6eb8e99803aa8bf3123bf2b2e6fec505a221"}, + {file = "black-23.1.0-cp310-cp310-macosx_10_16_universal2.whl", hash = "sha256:57c18c5165c1dbe291d5306e53fb3988122890e57bd9b3dcb75f967f13411a26"}, + {file = "black-23.1.0-cp310-cp310-macosx_10_16_x86_64.whl", hash = "sha256:9880d7d419bb7e709b37e28deb5e68a49227713b623c72b2b931028ea65f619b"}, + {file = "black-23.1.0-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:e6663f91b6feca5d06f2ccd49a10f254f9298cc1f7f49c46e498a0771b507104"}, + {file = "black-23.1.0-cp310-cp310-win_amd64.whl", hash = "sha256:9afd3f493666a0cd8f8df9a0200c6359ac53940cbde049dcb1a7eb6ee2dd7074"}, + {file = "black-23.1.0-cp311-cp311-macosx_10_16_arm64.whl", hash = "sha256:bfffba28dc52a58f04492181392ee380e95262af14ee01d4bc7bb1b1c6ca8d27"}, + {file = "black-23.1.0-cp311-cp311-macosx_10_16_universal2.whl", hash = "sha256:c1c476bc7b7d021321e7d93dc2cbd78ce103b84d5a4cf97ed535fbc0d6660648"}, + {file = "black-23.1.0-cp311-cp311-macosx_10_16_x86_64.whl", hash = "sha256:382998821f58e5c8238d3166c492139573325287820963d2f7de4d518bd76958"}, + {file = "black-23.1.0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:2bf649fda611c8550ca9d7592b69f0637218c2369b7744694c5e4902873b2f3a"}, + {file = "black-23.1.0-cp311-cp311-win_amd64.whl", hash = "sha256:121ca7f10b4a01fd99951234abdbd97728e1240be89fde18480ffac16503d481"}, + {file = "black-23.1.0-cp37-cp37m-macosx_10_16_x86_64.whl", hash = "sha256:a8471939da5e824b891b25751955be52ee7f8a30a916d570a5ba8e0f2eb2ecad"}, + {file = "black-23.1.0-cp37-cp37m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:8178318cb74f98bc571eef19068f6ab5613b3e59d4f47771582f04e175570ed8"}, + {file = "black-23.1.0-cp37-cp37m-win_amd64.whl", hash = "sha256:a436e7881d33acaf2536c46a454bb964a50eff59b21b51c6ccf5a40601fbef24"}, + {file = "black-23.1.0-cp38-cp38-macosx_10_16_arm64.whl", hash = "sha256:a59db0a2094d2259c554676403fa2fac3473ccf1354c1c63eccf7ae65aac8ab6"}, + {file = "black-23.1.0-cp38-cp38-macosx_10_16_universal2.whl", hash = "sha256:0052dba51dec07ed029ed61b18183942043e00008ec65d5028814afaab9a22fd"}, + {file = "black-23.1.0-cp38-cp38-macosx_10_16_x86_64.whl", hash = "sha256:49f7b39e30f326a34b5c9a4213213a6b221d7ae9d58ec70df1c4a307cf2a1580"}, + {file = "black-23.1.0-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:162e37d49e93bd6eb6f1afc3e17a3d23a823042530c37c3c42eeeaf026f38468"}, + {file = "black-23.1.0-cp38-cp38-win_amd64.whl", hash = "sha256:8b70eb40a78dfac24842458476135f9b99ab952dd3f2dab738c1881a9b38b753"}, + {file = "black-23.1.0-cp39-cp39-macosx_10_16_arm64.whl", hash = "sha256:a29650759a6a0944e7cca036674655c2f0f63806ddecc45ed40b7b8aa314b651"}, + {file = "black-23.1.0-cp39-cp39-macosx_10_16_universal2.whl", hash = "sha256:bb460c8561c8c1bec7824ecbc3ce085eb50005883a6203dcfb0122e95797ee06"}, + {file = "black-23.1.0-cp39-cp39-macosx_10_16_x86_64.whl", hash = "sha256:c91dfc2c2a4e50df0026f88d2215e166616e0c80e86004d0003ece0488db2739"}, + {file = "black-23.1.0-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:2a951cc83ab535d248c89f300eccbd625e80ab880fbcfb5ac8afb5f01a258ac9"}, + {file = "black-23.1.0-cp39-cp39-win_amd64.whl", hash = "sha256:0680d4380db3719ebcfb2613f34e86c8e6d15ffeabcf8ec59355c5e7b85bb555"}, + {file = "black-23.1.0-py3-none-any.whl", hash = "sha256:7a0f701d314cfa0896b9001df70a530eb2472babb76086344e688829efd97d32"}, + {file = "black-23.1.0.tar.gz", hash = "sha256:b0bd97bea8903f5a2ba7219257a44e3f1f9d00073d6cc1add68f0beec69692ac"}, ] [package.dependencies] click = ">=8.0.0" mypy-extensions = ">=0.4.3" +packaging = ">=22.0" pathspec = ">=0.9.0" platformdirs = ">=2" -tomli = {version = ">=1.1.0", markers = "python_full_version < \"3.11.0a7\""} +tomli = {version = ">=1.1.0", markers = "python_version < \"3.11\""} typing-extensions = {version = ">=3.10.0.0", markers = "python_version < \"3.10\""} [package.extras] @@ -884,6 +887,8 @@ files = [ {file = "cryptography-39.0.1-cp36-abi3-win32.whl", hash = "sha256:fe913f20024eb2cb2f323e42a64bdf2911bb9738a15dba7d3cce48151034e3a8"}, {file = "cryptography-39.0.1-cp36-abi3-win_amd64.whl", hash = "sha256:ced4e447ae29ca194449a3f1ce132ded8fcab06971ef5f618605aacaa612beac"}, {file = "cryptography-39.0.1-pp38-pypy38_pp73-macosx_10_12_x86_64.whl", hash = "sha256:807ce09d4434881ca3a7594733669bd834f5b2c6d5c7e36f8c00f691887042ad"}, + {file = "cryptography-39.0.1-pp38-pypy38_pp73-manylinux_2_28_aarch64.whl", hash = "sha256:c5caeb8188c24888c90b5108a441c106f7faa4c4c075a2bcae438c6e8ca73cef"}, + {file = "cryptography-39.0.1-pp38-pypy38_pp73-manylinux_2_28_x86_64.whl", hash = "sha256:4789d1e3e257965e960232345002262ede4d094d1a19f4d3b52e48d4d8f3b885"}, {file = "cryptography-39.0.1-pp38-pypy38_pp73-win_amd64.whl", hash = "sha256:96f1157a7c08b5b189b16b47bc9db2332269d6680a196341bf30046330d15388"}, {file = "cryptography-39.0.1-pp39-pypy39_pp73-macosx_10_12_x86_64.whl", hash = "sha256:e422abdec8b5fa8462aa016786680720d78bdce7a30c652b7fadf83a4ba35336"}, {file = "cryptography-39.0.1-pp39-pypy39_pp73-manylinux_2_17_aarch64.manylinux2014_aarch64.manylinux_2_24_aarch64.whl", hash = "sha256:b0afd054cd42f3d213bf82c629efb1ee5f22eba35bf0eec88ea9ea7304f511a2"}, @@ -963,23 +968,6 @@ files = [ [package.extras] testing = ["pre-commit"] -[[package]] -name = "flake8" -version = "5.0.4" -description = "the modular source code checker: pep8 pyflakes and co" -category = "dev" -optional = false -python-versions = ">=3.6.1" -files = [ - {file = "flake8-5.0.4-py2.py3-none-any.whl", hash = "sha256:7a1cf6b73744f5806ab95e526f6f0d8c01c66d7bbe349562d22dfca20610b248"}, - {file = "flake8-5.0.4.tar.gz", hash = "sha256:6fbe320aad8d6b95cec8b8e47bc933004678dc63095be98528b7bdd2a9f510db"}, -] - -[package.dependencies] -mccabe = ">=0.7.0,<0.8.0" -pycodestyle = ">=2.9.0,<2.10.0" -pyflakes = ">=2.5.0,<2.6.0" - [[package]] name = "flask" version = "2.1.3" @@ -1075,24 +1063,6 @@ files = [ {file = "iniconfig-1.1.1.tar.gz", hash = "sha256:bc3af051d7d14b2ee5ef9969666def0cd1a000e121eaea580d4a313df4b37f32"}, ] -[[package]] -name = "isort" -version = "5.10.1" -description = "A Python utility / library to sort Python imports." -category = "dev" -optional = false -python-versions = ">=3.6.1,<4.0" -files = [ - {file = "isort-5.10.1-py3-none-any.whl", hash = "sha256:6f62d78e2f89b4500b080fe3a81690850cd254227f27f75c3a0c491a1f351ba7"}, - {file = "isort-5.10.1.tar.gz", hash = "sha256:e8443a5e7a020e9d7f97f1d7d9cd17c88bcb3bc7e218bf9cf5095fe550be2951"}, -] - -[package.extras] -colors = ["colorama (>=0.4.3,<0.5.0)"] -pipfile-deprecated-finder = ["pipreqs", "requirementslib"] -plugins = ["setuptools"] -requirements-deprecated-finder = ["pip-api", "pipreqs"] - [[package]] name = "itsdangerous" version = "2.1.2" @@ -1238,6 +1208,7 @@ category = "main" optional = false python-versions = "*" files = [ + {file = "junit-xml-1.9.tar.gz", hash = "sha256:de16a051990d4e25a3982b2dd9e89d671067548718866416faec14d9de56db9f"}, {file = "junit_xml-1.9-py2.py3-none-any.whl", hash = "sha256:ec5ca1a55aefdd76d28fcc0b135251d156c7106fa979686a4b48d62b761b4732"}, ] @@ -1294,18 +1265,6 @@ files = [ {file = "MarkupSafe-2.1.1.tar.gz", hash = "sha256:7f91197cc9e48f989d12e4e6fbc46495c446636dfc81b9ccf50bb0ec74b91d4b"}, ] -[[package]] -name = "mccabe" -version = "0.7.0" -description = "McCabe checker, plugin for flake8" -category = "dev" -optional = false -python-versions = ">=3.6" -files = [ - {file = "mccabe-0.7.0-py2.py3-none-any.whl", hash = "sha256:6c2d30ab6be0e4a46919781807b4f0d834ebdd6c6e3dca0bda5a15f863427b6e"}, - {file = "mccabe-0.7.0.tar.gz", hash = "sha256:348e0240c33b60bbdf4e523192ef919f28cb2c3d7d5c7794f74009290f236325"}, -] - [[package]] name = "moto" version = "4.1.2" @@ -1453,46 +1412,42 @@ files = [ [[package]] name = "mypy" -version = "0.991" +version = "1.1.1" description = "Optional static typing for Python" category = "dev" optional = false python-versions = ">=3.7" files = [ - {file = "mypy-0.991-cp310-cp310-macosx_10_9_universal2.whl", hash = "sha256:7d17e0a9707d0772f4a7b878f04b4fd11f6f5bcb9b3813975a9b13c9332153ab"}, - {file = "mypy-0.991-cp310-cp310-macosx_10_9_x86_64.whl", hash = "sha256:0714258640194d75677e86c786e80ccf294972cc76885d3ebbb560f11db0003d"}, - {file = "mypy-0.991-cp310-cp310-macosx_11_0_arm64.whl", hash = "sha256:0c8f3be99e8a8bd403caa8c03be619544bc2c77a7093685dcf308c6b109426c6"}, - {file = "mypy-0.991-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:bc9ec663ed6c8f15f4ae9d3c04c989b744436c16d26580eaa760ae9dd5d662eb"}, - {file = "mypy-0.991-cp310-cp310-musllinux_1_1_x86_64.whl", hash = "sha256:4307270436fd7694b41f913eb09210faff27ea4979ecbcd849e57d2da2f65305"}, - {file = "mypy-0.991-cp310-cp310-win_amd64.whl", hash = "sha256:901c2c269c616e6cb0998b33d4adbb4a6af0ac4ce5cd078afd7bc95830e62c1c"}, - {file = "mypy-0.991-cp311-cp311-macosx_10_9_universal2.whl", hash = "sha256:d13674f3fb73805ba0c45eb6c0c3053d218aa1f7abead6e446d474529aafc372"}, - {file = "mypy-0.991-cp311-cp311-macosx_10_9_x86_64.whl", hash = "sha256:1c8cd4fb70e8584ca1ed5805cbc7c017a3d1a29fb450621089ffed3e99d1857f"}, - {file = "mypy-0.991-cp311-cp311-macosx_11_0_arm64.whl", hash = "sha256:209ee89fbb0deed518605edddd234af80506aec932ad28d73c08f1400ef80a33"}, - {file = "mypy-0.991-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:37bd02ebf9d10e05b00d71302d2c2e6ca333e6c2a8584a98c00e038db8121f05"}, - {file = "mypy-0.991-cp311-cp311-musllinux_1_1_x86_64.whl", hash = "sha256:26efb2fcc6b67e4d5a55561f39176821d2adf88f2745ddc72751b7890f3194ad"}, - {file = "mypy-0.991-cp311-cp311-win_amd64.whl", hash = "sha256:3a700330b567114b673cf8ee7388e949f843b356a73b5ab22dd7cff4742a5297"}, - {file = "mypy-0.991-cp37-cp37m-macosx_10_9_x86_64.whl", hash = "sha256:1f7d1a520373e2272b10796c3ff721ea1a0712288cafaa95931e66aa15798813"}, - {file = "mypy-0.991-cp37-cp37m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:641411733b127c3e0dab94c45af15fea99e4468f99ac88b39efb1ad677da5711"}, - {file = "mypy-0.991-cp37-cp37m-musllinux_1_1_x86_64.whl", hash = "sha256:3d80e36b7d7a9259b740be6d8d906221789b0d836201af4234093cae89ced0cd"}, - {file = "mypy-0.991-cp37-cp37m-win_amd64.whl", hash = "sha256:e62ebaad93be3ad1a828a11e90f0e76f15449371ffeecca4a0a0b9adc99abcef"}, - {file = "mypy-0.991-cp38-cp38-macosx_10_9_universal2.whl", hash = "sha256:b86ce2c1866a748c0f6faca5232059f881cda6dda2a893b9a8373353cfe3715a"}, - {file = "mypy-0.991-cp38-cp38-macosx_10_9_x86_64.whl", hash = "sha256:ac6e503823143464538efda0e8e356d871557ef60ccd38f8824a4257acc18d93"}, - {file = "mypy-0.991-cp38-cp38-macosx_11_0_arm64.whl", hash = "sha256:0cca5adf694af539aeaa6ac633a7afe9bbd760df9d31be55ab780b77ab5ae8bf"}, - {file = "mypy-0.991-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:a12c56bf73cdab116df96e4ff39610b92a348cc99a1307e1da3c3768bbb5b135"}, - {file = "mypy-0.991-cp38-cp38-musllinux_1_1_x86_64.whl", hash = "sha256:652b651d42f155033a1967739788c436491b577b6a44e4c39fb340d0ee7f0d70"}, - {file = "mypy-0.991-cp38-cp38-win_amd64.whl", hash = "sha256:4175593dc25d9da12f7de8de873a33f9b2b8bdb4e827a7cae952e5b1a342e243"}, - {file = "mypy-0.991-cp39-cp39-macosx_10_9_universal2.whl", hash = "sha256:98e781cd35c0acf33eb0295e8b9c55cdbef64fcb35f6d3aa2186f289bed6e80d"}, - {file = "mypy-0.991-cp39-cp39-macosx_10_9_x86_64.whl", hash = "sha256:6d7464bac72a85cb3491c7e92b5b62f3dcccb8af26826257760a552a5e244aa5"}, - {file = "mypy-0.991-cp39-cp39-macosx_11_0_arm64.whl", hash = "sha256:c9166b3f81a10cdf9b49f2d594b21b31adadb3d5e9db9b834866c3258b695be3"}, - {file = "mypy-0.991-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:b8472f736a5bfb159a5e36740847808f6f5b659960115ff29c7cecec1741c648"}, - {file = "mypy-0.991-cp39-cp39-musllinux_1_1_x86_64.whl", hash = "sha256:5e80e758243b97b618cdf22004beb09e8a2de1af481382e4d84bc52152d1c476"}, - {file = "mypy-0.991-cp39-cp39-win_amd64.whl", hash = "sha256:74e259b5c19f70d35fcc1ad3d56499065c601dfe94ff67ae48b85596b9ec1461"}, - {file = "mypy-0.991-py3-none-any.whl", hash = "sha256:de32edc9b0a7e67c2775e574cb061a537660e51210fbf6006b0b36ea695ae9bb"}, - {file = "mypy-0.991.tar.gz", hash = "sha256:3c0165ba8f354a6d9881809ef29f1a9318a236a6d81c690094c5df32107bde06"}, + {file = "mypy-1.1.1-cp310-cp310-macosx_10_9_x86_64.whl", hash = "sha256:39c7119335be05630611ee798cc982623b9e8f0cff04a0b48dfc26100e0b97af"}, + {file = "mypy-1.1.1-cp310-cp310-macosx_11_0_arm64.whl", hash = "sha256:61bf08362e93b6b12fad3eab68c4ea903a077b87c90ac06c11e3d7a09b56b9c1"}, + {file = "mypy-1.1.1-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:dbb19c9f662e41e474e0cff502b7064a7edc6764f5262b6cd91d698163196799"}, + {file = "mypy-1.1.1-cp310-cp310-musllinux_1_1_x86_64.whl", hash = "sha256:315ac73cc1cce4771c27d426b7ea558fb4e2836f89cb0296cbe056894e3a1f78"}, + {file = "mypy-1.1.1-cp310-cp310-win_amd64.whl", hash = "sha256:5cb14ff9919b7df3538590fc4d4c49a0f84392237cbf5f7a816b4161c061829e"}, + {file = "mypy-1.1.1-cp311-cp311-macosx_10_9_x86_64.whl", hash = "sha256:26cdd6a22b9b40b2fd71881a8a4f34b4d7914c679f154f43385ca878a8297389"}, + {file = "mypy-1.1.1-cp311-cp311-macosx_11_0_arm64.whl", hash = "sha256:5b5f81b40d94c785f288948c16e1f2da37203c6006546c5d947aab6f90aefef2"}, + {file = "mypy-1.1.1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:21b437be1c02712a605591e1ed1d858aba681757a1e55fe678a15c2244cd68a5"}, + {file = "mypy-1.1.1-cp311-cp311-musllinux_1_1_x86_64.whl", hash = "sha256:d809f88734f44a0d44959d795b1e6f64b2bbe0ea4d9cc4776aa588bb4229fc1c"}, + {file = "mypy-1.1.1-cp311-cp311-win_amd64.whl", hash = "sha256:a380c041db500e1410bb5b16b3c1c35e61e773a5c3517926b81dfdab7582be54"}, + {file = "mypy-1.1.1-cp37-cp37m-macosx_10_9_x86_64.whl", hash = "sha256:b7c7b708fe9a871a96626d61912e3f4ddd365bf7f39128362bc50cbd74a634d5"}, + {file = "mypy-1.1.1-cp37-cp37m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:c1c10fa12df1232c936830839e2e935d090fc9ee315744ac33b8a32216b93707"}, + {file = "mypy-1.1.1-cp37-cp37m-musllinux_1_1_x86_64.whl", hash = "sha256:0a28a76785bf57655a8ea5eb0540a15b0e781c807b5aa798bd463779988fa1d5"}, + {file = "mypy-1.1.1-cp37-cp37m-win_amd64.whl", hash = "sha256:ef6a01e563ec6a4940784c574d33f6ac1943864634517984471642908b30b6f7"}, + {file = "mypy-1.1.1-cp38-cp38-macosx_10_9_x86_64.whl", hash = "sha256:d64c28e03ce40d5303450f547e07418c64c241669ab20610f273c9e6290b4b0b"}, + {file = "mypy-1.1.1-cp38-cp38-macosx_11_0_arm64.whl", hash = "sha256:64cc3afb3e9e71a79d06e3ed24bb508a6d66f782aff7e56f628bf35ba2e0ba51"}, + {file = "mypy-1.1.1-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:ce61663faf7a8e5ec6f456857bfbcec2901fbdb3ad958b778403f63b9e606a1b"}, + {file = "mypy-1.1.1-cp38-cp38-musllinux_1_1_x86_64.whl", hash = "sha256:2b0c373d071593deefbcdd87ec8db91ea13bd8f1328d44947e88beae21e8d5e9"}, + {file = "mypy-1.1.1-cp38-cp38-win_amd64.whl", hash = "sha256:2888ce4fe5aae5a673386fa232473014056967f3904f5abfcf6367b5af1f612a"}, + {file = "mypy-1.1.1-cp39-cp39-macosx_10_9_x86_64.whl", hash = "sha256:19ba15f9627a5723e522d007fe708007bae52b93faab00f95d72f03e1afa9598"}, + {file = "mypy-1.1.1-cp39-cp39-macosx_11_0_arm64.whl", hash = "sha256:59bbd71e5c58eed2e992ce6523180e03c221dcd92b52f0e792f291d67b15a71c"}, + {file = "mypy-1.1.1-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:9401e33814cec6aec8c03a9548e9385e0e228fc1b8b0a37b9ea21038e64cdd8a"}, + {file = "mypy-1.1.1-cp39-cp39-musllinux_1_1_x86_64.whl", hash = "sha256:4b398d8b1f4fba0e3c6463e02f8ad3346f71956b92287af22c9b12c3ec965a9f"}, + {file = "mypy-1.1.1-cp39-cp39-win_amd64.whl", hash = "sha256:69b35d1dcb5707382810765ed34da9db47e7f95b3528334a3c999b0c90fe523f"}, + {file = "mypy-1.1.1-py3-none-any.whl", hash = "sha256:4e4e8b362cdf99ba00c2b218036002bdcdf1e0de085cdb296a49df03fb31dfc4"}, + {file = "mypy-1.1.1.tar.gz", hash = "sha256:ae9ceae0f5b9059f33dbc62dea087e942c0ccab4b7a003719cb70f9b8abfa32f"}, ] [package.dependencies] -mypy-extensions = ">=0.4.3" +mypy-extensions = ">=1.0.0" tomli = {version = ">=1.1.0", markers = "python_version < \"3.11\""} typing-extensions = ">=3.10" @@ -1519,14 +1474,14 @@ typing-extensions = ">=4.1.0" [[package]] name = "mypy-extensions" -version = "0.4.3" -description = "Experimental type system extensions for programs checked with the mypy typechecker." +version = "1.0.0" +description = "Type system extensions for programs checked with the mypy type checker." category = "dev" optional = false -python-versions = "*" +python-versions = ">=3.5" files = [ - {file = "mypy_extensions-0.4.3-py2.py3-none-any.whl", hash = "sha256:090fedd75945a69ae91ce1303b5824f428daf5a028d2f6ab8a299250a846f15d"}, - {file = "mypy_extensions-0.4.3.tar.gz", hash = "sha256:2d82818f5bb3e369420cb3c4060a7970edba416647068eb4c5343488a6c604a8"}, + {file = "mypy_extensions-1.0.0-py3-none-any.whl", hash = "sha256:4392f6c0eb8a5668a69e23d168ffa70f0be9ccfd32b5cc2d26a34ae5b844552d"}, + {file = "mypy_extensions-1.0.0.tar.gz", hash = "sha256:75dbf8955dc00442a438fc4d0666508a9a97b6bd41aa2f0ffe9d2f2725af0782"}, ] [[package]] @@ -1591,19 +1546,16 @@ requests = ["requests"] [[package]] name = "packaging" -version = "21.3" +version = "23.0" description = "Core utilities for Python packages" category = "main" optional = false -python-versions = ">=3.6" +python-versions = ">=3.7" files = [ - {file = "packaging-21.3-py3-none-any.whl", hash = "sha256:ef103e05f519cdc783ae24ea4e2e0f508a9c99b2d4969652eed6a2e1ea5bd522"}, - {file = "packaging-21.3.tar.gz", hash = "sha256:dd47c42927d89ab911e606518907cc2d3a1f38bbd026385970643f9c5b8ecfeb"}, + {file = "packaging-23.0-py3-none-any.whl", hash = "sha256:714ac14496c3e68c99c29b00845f7a2b85f3bb6f1078fd9f72fd20f0570002b2"}, + {file = "packaging-23.0.tar.gz", hash = "sha256:b6ad297f8907de0fa2fe1ccbd26fdaf387f5f47c7275fedf8cce89f99446cf97"}, ] -[package.dependencies] -pyparsing = ">=2.0.2,<3.0.5 || >3.0.5" - [[package]] name = "pathspec" version = "0.9.0" @@ -1712,6 +1664,7 @@ python-versions = ">=3.6" files = [ {file = "psycopg2-binary-2.9.3.tar.gz", hash = "sha256:761df5313dc15da1502b21453642d7599d26be88bff659382f8f9747c7ebea4e"}, {file = "psycopg2_binary-2.9.3-cp310-cp310-macosx_10_14_x86_64.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl", hash = "sha256:539b28661b71da7c0e428692438efbcd048ca21ea81af618d845e06ebfd29478"}, + {file = "psycopg2_binary-2.9.3-cp310-cp310-macosx_11_0_arm64.whl", hash = "sha256:2f2534ab7dc7e776a263b463a16e189eb30e85ec9bbe1bff9e78dae802608932"}, {file = "psycopg2_binary-2.9.3-cp310-cp310-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:6e82d38390a03da28c7985b394ec3f56873174e2c88130e6966cb1c946508e65"}, {file = "psycopg2_binary-2.9.3-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:57804fc02ca3ce0dbfbef35c4b3a4a774da66d66ea20f4bda601294ad2ea6092"}, {file = "psycopg2_binary-2.9.3-cp310-cp310-manylinux_2_24_aarch64.whl", hash = "sha256:083a55275f09a62b8ca4902dd11f4b33075b743cf0d360419e2051a8a5d5ff76"}, @@ -1745,6 +1698,7 @@ files = [ {file = "psycopg2_binary-2.9.3-cp37-cp37m-win32.whl", hash = "sha256:adf20d9a67e0b6393eac162eb81fb10bc9130a80540f4df7e7355c2dd4af9fba"}, {file = "psycopg2_binary-2.9.3-cp37-cp37m-win_amd64.whl", hash = "sha256:2f9ffd643bc7349eeb664eba8864d9e01f057880f510e4681ba40a6532f93c71"}, {file = "psycopg2_binary-2.9.3-cp38-cp38-macosx_10_14_x86_64.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl", hash = "sha256:def68d7c21984b0f8218e8a15d514f714d96904265164f75f8d3a70f9c295667"}, + {file = "psycopg2_binary-2.9.3-cp38-cp38-macosx_11_0_arm64.whl", hash = "sha256:e6aa71ae45f952a2205377773e76f4e3f27951df38e69a4c95440c779e013560"}, {file = "psycopg2_binary-2.9.3-cp38-cp38-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:dffc08ca91c9ac09008870c9eb77b00a46b3378719584059c034b8945e26b272"}, {file = "psycopg2_binary-2.9.3-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:280b0bb5cbfe8039205c7981cceb006156a675362a00fe29b16fbc264e242834"}, {file = "psycopg2_binary-2.9.3-cp38-cp38-manylinux_2_24_aarch64.whl", hash = "sha256:af9813db73395fb1fc211bac696faea4ca9ef53f32dc0cfa27e4e7cf766dcf24"}, @@ -1756,6 +1710,7 @@ files = [ {file = "psycopg2_binary-2.9.3-cp38-cp38-win32.whl", hash = "sha256:6472a178e291b59e7f16ab49ec8b4f3bdada0a879c68d3817ff0963e722a82ce"}, {file = "psycopg2_binary-2.9.3-cp38-cp38-win_amd64.whl", hash = "sha256:35168209c9d51b145e459e05c31a9eaeffa9a6b0fd61689b48e07464ffd1a83e"}, {file = "psycopg2_binary-2.9.3-cp39-cp39-macosx_10_14_x86_64.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl", hash = "sha256:47133f3f872faf28c1e87d4357220e809dfd3fa7c64295a4a148bcd1e6e34ec9"}, + {file = "psycopg2_binary-2.9.3-cp39-cp39-macosx_11_0_arm64.whl", hash = "sha256:b3a24a1982ae56461cc24f6680604fffa2c1b818e9dc55680da038792e004d18"}, {file = "psycopg2_binary-2.9.3-cp39-cp39-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:91920527dea30175cc02a1099f331aa8c1ba39bf8b7762b7b56cbf54bc5cce42"}, {file = "psycopg2_binary-2.9.3-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:887dd9aac71765ac0d0bac1d0d4b4f2c99d5f5c1382d8b770404f0f3d0ce8a39"}, {file = "psycopg2_binary-2.9.3-cp39-cp39-manylinux_2_24_aarch64.whl", hash = "sha256:1f14c8b0942714eb3c74e1e71700cbbcb415acbc311c730370e70c578a44a25c"}, @@ -1788,33 +1743,10 @@ category = "main" optional = false python-versions = "*" files = [ - {file = "pyasn1-0.4.8-py2.4.egg", hash = "sha256:fec3e9d8e36808a28efb59b489e4528c10ad0f480e57dcc32b4de5c9d8c9fdf3"}, - {file = "pyasn1-0.4.8-py2.5.egg", hash = "sha256:0458773cfe65b153891ac249bcf1b5f8f320b7c2ce462151f8fa74de8934becf"}, - {file = "pyasn1-0.4.8-py2.6.egg", hash = "sha256:5c9414dcfede6e441f7e8f81b43b34e834731003427e5b09e4e00e3172a10f00"}, - {file = "pyasn1-0.4.8-py2.7.egg", hash = "sha256:6e7545f1a61025a4e58bb336952c5061697da694db1cae97b116e9c46abcf7c8"}, {file = "pyasn1-0.4.8-py2.py3-none-any.whl", hash = "sha256:39c7e2ec30515947ff4e87fb6f456dfc6e84857d34be479c9d4a4ba4bf46aa5d"}, - {file = "pyasn1-0.4.8-py3.1.egg", hash = "sha256:78fa6da68ed2727915c4767bb386ab32cdba863caa7dbe473eaae45f9959da86"}, - {file = "pyasn1-0.4.8-py3.2.egg", hash = "sha256:08c3c53b75eaa48d71cf8c710312316392ed40899cb34710d092e96745a358b7"}, - {file = "pyasn1-0.4.8-py3.3.egg", hash = "sha256:03840c999ba71680a131cfaee6fab142e1ed9bbd9c693e285cc6aca0d555e576"}, - {file = "pyasn1-0.4.8-py3.4.egg", hash = "sha256:7ab8a544af125fb704feadb008c99a88805126fb525280b2270bb25cc1d78a12"}, - {file = "pyasn1-0.4.8-py3.5.egg", hash = "sha256:e89bf84b5437b532b0803ba5c9a5e054d21fec423a89952a74f87fa2c9b7bce2"}, - {file = "pyasn1-0.4.8-py3.6.egg", hash = "sha256:014c0e9976956a08139dc0712ae195324a75e142284d5f87f1a87ee1b068a359"}, - {file = "pyasn1-0.4.8-py3.7.egg", hash = "sha256:99fcc3c8d804d1bc6d9a099921e39d827026409a58f2a720dcdb89374ea0c776"}, {file = "pyasn1-0.4.8.tar.gz", hash = "sha256:aef77c9fb94a3ac588e87841208bdec464471d9871bd5050a287cc9a475cd0ba"}, ] -[[package]] -name = "pycodestyle" -version = "2.9.1" -description = "Python style guide checker" -category = "dev" -optional = false -python-versions = ">=3.6" -files = [ - {file = "pycodestyle-2.9.1-py2.py3-none-any.whl", hash = "sha256:d1735fc58b418fd7c5f658d28d943854f8a849b01a5d0a1e6f3f3fdd0166804b"}, - {file = "pycodestyle-2.9.1.tar.gz", hash = "sha256:2c9607871d58c76354b697b42f5d57e1ada7d261c261efac224b664affdc5785"}, -] - [[package]] name = "pycparser" version = "2.21" @@ -1827,18 +1759,6 @@ files = [ {file = "pycparser-2.21.tar.gz", hash = "sha256:e644fdec12f7872f86c58ff790da456218b10f863970249516d60a5eaca77206"}, ] -[[package]] -name = "pyflakes" -version = "2.5.0" -description = "passive checker of Python programs" -category = "dev" -optional = false -python-versions = ">=3.6" -files = [ - {file = "pyflakes-2.5.0-py2.py3-none-any.whl", hash = "sha256:4579f67d887f804e67edb544428f264b7b24f435b263c4614f384135cea553d2"}, - {file = "pyflakes-2.5.0.tar.gz", hash = "sha256:491feb020dca48ccc562a8c0cbe8df07ee13078df59813b83959cbdada312ea3"}, -] - [[package]] name = "pyjwt" version = "2.4.0" @@ -2008,8 +1928,8 @@ files = [ [package.dependencies] pytest = [ - {version = ">=6.2.4", markers = "python_version >= \"3.10\""}, {version = ">=5.0", markers = "python_version < \"3.10\""}, + {version = ">=6.2.4", markers = "python_version >= \"3.10\""}, ] [[package]] @@ -2121,6 +2041,13 @@ files = [ {file = "PyYAML-6.0-cp310-cp310-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_12_x86_64.manylinux2010_x86_64.whl", hash = "sha256:f84fbc98b019fef2ee9a1cb3ce93e3187a6df0b2538a651bfb890254ba9f90b5"}, {file = "PyYAML-6.0-cp310-cp310-win32.whl", hash = "sha256:2cd5df3de48857ed0544b34e2d40e9fac445930039f3cfe4bcc592a1f836d513"}, {file = "PyYAML-6.0-cp310-cp310-win_amd64.whl", hash = "sha256:daf496c58a8c52083df09b80c860005194014c3698698d1a57cbcfa182142a3a"}, + {file = "PyYAML-6.0-cp311-cp311-macosx_10_9_x86_64.whl", hash = "sha256:d4b0ba9512519522b118090257be113b9468d804b19d63c71dbcf4a48fa32358"}, + {file = "PyYAML-6.0-cp311-cp311-macosx_11_0_arm64.whl", hash = "sha256:81957921f441d50af23654aa6c5e5eaf9b06aba7f0a19c18a538dc7ef291c5a1"}, + {file = "PyYAML-6.0-cp311-cp311-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:afa17f5bc4d1b10afd4466fd3a44dc0e245382deca5b3c353d8b757f9e3ecb8d"}, + {file = "PyYAML-6.0-cp311-cp311-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:dbad0e9d368bb989f4515da330b88a057617d16b6a8245084f1b05400f24609f"}, + {file = "PyYAML-6.0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:432557aa2c09802be39460360ddffd48156e30721f5e8d917f01d31694216782"}, + {file = "PyYAML-6.0-cp311-cp311-win32.whl", hash = "sha256:bfaef573a63ba8923503d27530362590ff4f576c626d86a9fed95822a8255fd7"}, + {file = "PyYAML-6.0-cp311-cp311-win_amd64.whl", hash = "sha256:01b45c0191e6d66c470b6cf1b9531a771a83c1c4208272ead47a3ae4f2f603bf"}, {file = "PyYAML-6.0-cp36-cp36m-macosx_10_9_x86_64.whl", hash = "sha256:897b80890765f037df3403d22bab41627ca8811ae55e9a722fd0392850ec4d86"}, {file = "PyYAML-6.0-cp36-cp36m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:50602afada6d6cbfad699b0c7bb50d5ccffa7e46a3d738092afddc1f9758427f"}, {file = "PyYAML-6.0-cp36-cp36m-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:48c346915c114f5fdb3ead70312bd042a953a8ce5c7106d5bfb1a5254e47da92"}, @@ -2205,6 +2132,33 @@ files = [ [package.dependencies] pyasn1 = ">=0.1.3" +[[package]] +name = "ruff" +version = "0.0.255" +description = "An extremely fast Python linter, written in Rust." +category = "dev" +optional = false +python-versions = ">=3.7" +files = [ + {file = "ruff-0.0.255-py3-none-macosx_10_7_x86_64.whl", hash = "sha256:b2d71fb6a7e50501a2473864acffc85dee6b750c25db198f7e71fe1dbbff1aad"}, + {file = "ruff-0.0.255-py3-none-macosx_10_9_x86_64.macosx_11_0_arm64.macosx_10_9_universal2.whl", hash = "sha256:6c97d746861a6010f941179e84bba9feb8a871815667471d9ed6beb98d45c252"}, + {file = "ruff-0.0.255-py3-none-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:9a7fa60085079b91a298b963361be9b1b1c724582af6c84be954cbabdbd9309a"}, + {file = "ruff-0.0.255-py3-none-manylinux_2_17_armv7l.manylinux2014_armv7l.whl", hash = "sha256:c089f7141496334ab5a127b54ce55e41f0d6714e68a4453a1e09d2204cdea8c3"}, + {file = "ruff-0.0.255-py3-none-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:0423908caa7d437a416b853214565b9c33bbd1106c4f88147982216dddcbbd96"}, + {file = "ruff-0.0.255-py3-none-manylinux_2_17_ppc64.manylinux2014_ppc64.whl", hash = "sha256:981493e92547cacbb8e0874904ec049fe744507ee890dc8736caf89a8864f9a7"}, + {file = "ruff-0.0.255-py3-none-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:59d5193d2aedb35db180824462b374dbcfc306b2e76076245088afa6e5837df2"}, + {file = "ruff-0.0.255-py3-none-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:dd5e00733c9d160c8a34a22e62b390da9d1e9f326676402421cb8c1236beefc3"}, + {file = "ruff-0.0.255-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:694418cf41838bd19c6229e4e1b2d04505b1e6b86fe3ab81165484fc96d36f01"}, + {file = "ruff-0.0.255-py3-none-musllinux_1_2_aarch64.whl", hash = "sha256:5d0408985c9777369daebb5d3340a99e9f7294bdd7120642239261508185cf89"}, + {file = "ruff-0.0.255-py3-none-musllinux_1_2_armv7l.whl", hash = "sha256:abd6376ef9d12f370d95a8c7c98682fbb9bfedfba59f40e84a816fef8ddcb8de"}, + {file = "ruff-0.0.255-py3-none-musllinux_1_2_i686.whl", hash = "sha256:f9b1a5df0bc09193cbef58a6f78e4a9a0b058a4f9733c0442866d078006d1bb9"}, + {file = "ruff-0.0.255-py3-none-musllinux_1_2_x86_64.whl", hash = "sha256:6a25c5f4ff087445b2e1bbcb9963f2ae7c868d65e4a8d5f84c36c12f71571179"}, + {file = "ruff-0.0.255-py3-none-win32.whl", hash = "sha256:1ff87a8310354f9f1a099625e54a27fdd6756d9cd2a40b45922f2e943daf982d"}, + {file = "ruff-0.0.255-py3-none-win_amd64.whl", hash = "sha256:f3d8416be618f023f93ec4fd6ee3048585ef85dba9563b2a7e38fc7e5131d5b1"}, + {file = "ruff-0.0.255-py3-none-win_arm64.whl", hash = "sha256:8ba124819624145d7b6b53add40c367c44318893215ffc1bfe3d72e0225a1c9c"}, + {file = "ruff-0.0.255.tar.gz", hash = "sha256:f9eb1d3b2eecbeedae419fa494c4e2a5e4484baf93a1ce0f81eddb005e1919c5"}, +] + [[package]] name = "s3transfer" version = "0.6.0" @@ -2643,4 +2597,4 @@ testing = ["func-timeout", "jaraco.itertools", "pytest (>=6)", "pytest-black (>= [metadata] lock-version = "2.0" python-versions = "^3.9" -content-hash = "3038940781ef59d1ed28cedf46120ad6623e21e602c38ad3c359428d79fa1efd" +content-hash = "2515a9320c2960076012fbc036fb33c4f6a23515c8d143785931dc18c6722d91" diff --git a/pre-commit.py b/pre-commit.py index 560df6cd0c..dc0b9ed588 100755 --- a/pre-commit.py +++ b/pre-commit.py @@ -43,17 +43,13 @@ def black(fix_inplace: bool) -> str: return cmd -def isort(fix_inplace: bool) -> str: - cmd = "poetry run isort" - if not fix_inplace: - cmd += " --diff --check" +def ruff(fix_inplace: bool) -> str: + cmd = "poetry run ruff" + if fix_inplace: + cmd += " --fix" return cmd -def flake8() -> str: - return "poetry run flake8" - - def mypy() -> str: return "poetry run mypy" @@ -112,13 +108,6 @@ if __name__ == "__main__": changed_files=files, no_color=args.no_color, ) - check( - name="isort", - suffix=".py", - cmd=isort(fix_inplace=args.fix_inplace), - changed_files=files, - no_color=args.no_color, - ) check( name="black", suffix=".py", @@ -127,9 +116,9 @@ if __name__ == "__main__": no_color=args.no_color, ) check( - name="flake8", + name="ruff", suffix=".py", - cmd=flake8(), + cmd=ruff(fix_inplace=args.fix_inplace), changed_files=files, no_color=args.no_color, ) diff --git a/pyproject.toml b/pyproject.toml index 415f7f1ae7..f21c12b2e3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -35,11 +35,10 @@ types-toml = "^0.10.8" pytest-httpserver = "^1.0.6" aiohttp = "3.7.4" -[tool.poetry.dev-dependencies] -flake8 = "^5.0.4" -mypy = "==0.991" -black = "^22.6.0" -isort = "^5.10.1" +[tool.poetry.group.dev.dependencies] +black = "^23.1.0" +mypy = "==1.1.1" +ruff = "^0.0.255" [build-system] requires = ["poetry-core>=1.0.0"] @@ -53,14 +52,6 @@ extend-exclude = ''' )/ ''' -[tool.isort] -profile = "black" -line_length = 100 -skip_gitignore = true -skip = [ - "vendor", -] - [tool.mypy] exclude = "^vendor/" check_untyped_defs = true @@ -80,3 +71,13 @@ module = [ "pg8000.*", ] ignore_missing_imports = true + +[tool.ruff] +extend-exclude = ["vendor/"] +ignore = ["E501"] +select = [ + "E", # pycodestyle + "F", # Pyflakes + "I", # isort + "W", # pycodestyle +] diff --git a/scripts/export_import_between_pageservers.py b/scripts/export_import_between_pageservers.py index db2b5e81ab..4292c981a9 100755 --- a/scripts/export_import_between_pageservers.py +++ b/scripts/export_import_between_pageservers.py @@ -308,8 +308,8 @@ def lsn_to_hex(num: int) -> str: def lsn_from_hex(lsn_hex: str) -> int: """Convert lsn from hex notation to int.""" - l, r = lsn_hex.split("/") - return (int(l, 16) << 32) + int(r, 16) + left, right = lsn_hex.split("/") + return (int(left, 16) << 32) + int(right, 16) def remote_consistent_lsn( @@ -398,7 +398,6 @@ def reconstruct_paths(log_dir, pg_bin, base_tar, port: int): result = vanilla_pg.safe_psql(query, user="cloud_admin", dbname=database) for relname, filepath in result: if filepath is not None: - if database == "template0copy": # Add all template0copy paths to template0 prefix = f"base/{oid}/" diff --git a/scripts/reformat b/scripts/reformat index 5346c78ead..8688044f66 100755 --- a/scripts/reformat +++ b/scripts/reformat @@ -6,6 +6,5 @@ set -euox pipefail echo 'Reformatting Rust code' cargo fmt echo 'Reformatting Python code' -poetry run isort test_runner scripts -poetry run flake8 test_runner scripts +poetry run ruff --fix test_runner scripts poetry run black test_runner scripts diff --git a/scripts/sk_cleanup_tenants/script.py b/scripts/sk_cleanup_tenants/script.py index 4d010d85ea..fa22433614 100644 --- a/scripts/sk_cleanup_tenants/script.py +++ b/scripts/sk_cleanup_tenants/script.py @@ -68,7 +68,6 @@ def call_delete_tenant_api(tenant_id): def cleanup_tenant(tenant_id): - tenant_dir = Path(f"/storage/safekeeper/data/{tenant_id}") if not tenant_dir.exists(): diff --git a/setup.cfg b/setup.cfg deleted file mode 100644 index a067ee731d..0000000000 --- a/setup.cfg +++ /dev/null @@ -1,8 +0,0 @@ -[flake8] -# Move config to pyproject.toml as soon as flake8 supports it -# https://github.com/PyCQA/flake8/issues/234 -extend-ignore = - E203, # Whitespace before ':' -- conflicts with black - E266, # Too many leading '#' for block comment -- we use it for formatting sometimes - E501 # Line too long -- black sorts it out -extend-exclude = vendor/ diff --git a/test_runner/fixtures/benchmark_fixture.py b/test_runner/fixtures/benchmark_fixture.py index a39aaf8241..67a99aa452 100644 --- a/test_runner/fixtures/benchmark_fixture.py +++ b/test_runner/fixtures/benchmark_fixture.py @@ -17,6 +17,7 @@ import pytest from _pytest.config import Config from _pytest.config.argparsing import Parser from _pytest.terminal import TerminalReporter + from fixtures.neon_fixtures import NeonPageserver from fixtures.types import TenantId, TimelineId diff --git a/test_runner/fixtures/compare_fixtures.py b/test_runner/fixtures/compare_fixtures.py index 0ba926c8d2..b328cea5c6 100644 --- a/test_runner/fixtures/compare_fixtures.py +++ b/test_runner/fixtures/compare_fixtures.py @@ -6,8 +6,15 @@ from typing import Dict, Iterator, List import pytest from _pytest.fixtures import FixtureRequest + from fixtures.benchmark_fixture import MetricReport, NeonBenchmarker -from fixtures.neon_fixtures import NeonEnv, PgBin, PgProtocol, RemotePostgres, VanillaPostgres +from fixtures.neon_fixtures import ( + NeonEnv, + PgBin, + PgProtocol, + RemotePostgres, + VanillaPostgres, +) from fixtures.pg_stats import PgStatTable diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index 70a6f1809e..a25709a305 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -35,6 +35,13 @@ import requests from _pytest.config import Config from _pytest.config.argparsing import Parser from _pytest.fixtures import FixtureRequest + +# Type-related stuff +from psycopg2.extensions import connection as PgConnection +from psycopg2.extensions import cursor as PgCursor +from psycopg2.extensions import make_dsn, parse_dsn +from typing_extensions import Literal + from fixtures.log_helper import log from fixtures.metrics import Metrics, parse_metrics from fixtures.types import Lsn, TenantId, TimelineId @@ -46,12 +53,6 @@ from fixtures.utils import ( subprocess_capture, ) -# Type-related stuff -from psycopg2.extensions import connection as PgConnection -from psycopg2.extensions import cursor as PgCursor -from psycopg2.extensions import make_dsn, parse_dsn -from typing_extensions import Literal - """ This file contains pytest fixtures. A fixture is a test resource that can be summoned by placing its name in the test's arguments. @@ -1243,7 +1244,6 @@ class PageserverHttpClient(requests.Session): include_non_incremental_logical_size: bool = False, include_timeline_dir_layer_file_size_sum: bool = False, ) -> List[Dict[str, Any]]: - params = {} if include_non_incremental_logical_size: params["include-non-incremental-logical-size"] = "true" @@ -1375,7 +1375,6 @@ class PageserverHttpClient(requests.Session): timeline_id: TimelineId, max_concurrent_downloads: int, ) -> dict[str, Any]: - body = { "max_concurrent_downloads": max_concurrent_downloads, } @@ -1668,7 +1667,7 @@ class AbstractNeonCli(abc.ABC): env_vars["POSTGRES_DISTRIB_DIR"] = str(self.env.pg_distrib_dir) if self.env.rust_log_override is not None: env_vars["RUST_LOG"] = self.env.rust_log_override - for (extra_env_key, extra_env_value) in (extra_env_vars or {}).items(): + for extra_env_key, extra_env_value in (extra_env_vars or {}).items(): env_vars[extra_env_key] = extra_env_value # Pass coverage settings @@ -2852,7 +2851,6 @@ class PostgresFactory: lsn: Optional[Lsn] = None, config_lines: Optional[List[str]] = None, ) -> Postgres: - pg = Postgres( self.env, tenant_id=tenant_id or self.env.initial_tenant, @@ -2876,7 +2874,6 @@ class PostgresFactory: lsn: Optional[Lsn] = None, config_lines: Optional[List[str]] = None, ) -> Postgres: - pg = Postgres( self.env, tenant_id=tenant_id or self.env.initial_tenant, @@ -3323,7 +3320,6 @@ def check_restored_datadir_content( log.info(f"filecmp result mismatch and error lists:\n\t mismatch={mismatch}\n\t error={error}") for f in mismatch: - f1 = os.path.join(pg.pgdata_dir, f) f2 = os.path.join(restored_dir_path, f) stdout_filename = "{}.filediff".format(f2) diff --git a/test_runner/fixtures/types.py b/test_runner/fixtures/types.py index 2bb962d44a..7d179cc7fb 100644 --- a/test_runner/fixtures/types.py +++ b/test_runner/fixtures/types.py @@ -17,8 +17,8 @@ class Lsn: self.lsn_int = x else: """Convert lsn from hex notation to int.""" - l, r = x.split("/") - self.lsn_int = (int(l, 16) << 32) + int(r, 16) + left, right = x.split("/") + self.lsn_int = (int(left, 16) << 32) + int(right, 16) assert 0 <= self.lsn_int <= 0xFFFFFFFF_FFFFFFFF def __str__(self) -> str: diff --git a/test_runner/fixtures/utils.py b/test_runner/fixtures/utils.py index df83fc6377..ce03658e8f 100644 --- a/test_runner/fixtures/utils.py +++ b/test_runner/fixtures/utils.py @@ -8,9 +8,10 @@ from pathlib import Path from typing import Any, Callable, Dict, List, Tuple, TypeVar import allure # type: ignore -from fixtures.log_helper import log from psycopg2.extensions import cursor +from fixtures.log_helper import log + Fn = TypeVar("Fn", bound=Callable[..., Any]) diff --git a/test_runner/performance/test_branching.py b/test_runner/performance/test_branching.py index 0fe7306f87..4eaec40096 100644 --- a/test_runner/performance/test_branching.py +++ b/test_runner/performance/test_branching.py @@ -5,6 +5,7 @@ from typing import List from fixtures.benchmark_fixture import PgBenchRunResult from fixtures.compare_fixtures import NeonCompare from fixtures.neon_fixtures import fork_at_current_lsn + from performance.test_perf_pgbench import utc_now_timestamp # ----------------------------------------------------------------------- diff --git a/test_runner/performance/test_bulk_update.py b/test_runner/performance/test_bulk_update.py index f8e29cda69..7aa6f09a40 100644 --- a/test_runner/performance/test_bulk_update.py +++ b/test_runner/performance/test_bulk_update.py @@ -13,7 +13,6 @@ from fixtures.neon_fixtures import NeonEnvBuilder, wait_for_last_flush_lsn @pytest.mark.timeout(10000) @pytest.mark.parametrize("fillfactor", [10, 50, 100]) def test_bulk_update(neon_env_builder: NeonEnvBuilder, zenbenchmark, fillfactor): - env = neon_env_builder.init_start() n_records = 1000000 diff --git a/test_runner/performance/test_compare_pg_stats.py b/test_runner/performance/test_compare_pg_stats.py index d39ea55fbb..d5dd1b4bd0 100644 --- a/test_runner/performance/test_compare_pg_stats.py +++ b/test_runner/performance/test_compare_pg_stats.py @@ -6,6 +6,7 @@ from typing import List import pytest from fixtures.compare_fixtures import PgCompare from fixtures.pg_stats import PgStatTable + from performance.test_perf_pgbench import get_durations_matrix, get_scales_matrix diff --git a/test_runner/performance/test_gist_build.py b/test_runner/performance/test_gist_build.py index 311030b99d..45900d0c7f 100644 --- a/test_runner/performance/test_gist_build.py +++ b/test_runner/performance/test_gist_build.py @@ -13,7 +13,6 @@ def test_gist_buffering_build(neon_with_baseline: PgCompare): with closing(env.pg.connect()) as conn: with conn.cursor() as cur: - # Create test table. cur.execute("create table gist_point_tbl(id int4, p point)") cur.execute( diff --git a/test_runner/performance/test_latency.py b/test_runner/performance/test_latency.py index 9aa618650d..257e0421af 100644 --- a/test_runner/performance/test_latency.py +++ b/test_runner/performance/test_latency.py @@ -3,6 +3,7 @@ import threading import pytest from fixtures.compare_fixtures import PgCompare from fixtures.neon_fixtures import Postgres + from performance.test_perf_pgbench import get_scales_matrix from performance.test_wal_backpressure import record_read_latency diff --git a/test_runner/performance/test_layer_map.py b/test_runner/performance/test_layer_map.py index ac49ea9051..fb29c05273 100644 --- a/test_runner/performance/test_layer_map.py +++ b/test_runner/performance/test_layer_map.py @@ -7,7 +7,6 @@ from fixtures.neon_fixtures import NeonEnvBuilder # Benchmark searching the layer map, when there are a lot of small layer files. # def test_layer_map(neon_env_builder: NeonEnvBuilder, zenbenchmark): - env = neon_env_builder.init_start() n_iters = 10 n_records = 100000 diff --git a/test_runner/performance/test_parallel_copy_to.py b/test_runner/performance/test_parallel_copy_to.py index b4a25e0edc..746c1b73dd 100644 --- a/test_runner/performance/test_parallel_copy_to.py +++ b/test_runner/performance/test_parallel_copy_to.py @@ -36,7 +36,6 @@ async def parallel_load_different_tables(pg: PgProtocol, n_parallel: int): # Load 5 different tables in parallel with COPY TO def test_parallel_copy_different_tables(neon_with_baseline: PgCompare, n_parallel=5): - env = neon_with_baseline conn = env.pg.connect() cur = conn.cursor() diff --git a/test_runner/performance/test_wal_backpressure.py b/test_runner/performance/test_wal_backpressure.py index f9a18c84fd..3939ca30b6 100644 --- a/test_runner/performance/test_wal_backpressure.py +++ b/test_runner/performance/test_wal_backpressure.py @@ -10,6 +10,7 @@ from fixtures.compare_fixtures import NeonCompare, PgCompare, VanillaCompare from fixtures.log_helper import log from fixtures.neon_fixtures import DEFAULT_BRANCH_NAME, NeonEnvBuilder, PgBin from fixtures.types import Lsn + from performance.test_perf_pgbench import get_durations_matrix, get_scales_matrix diff --git a/test_runner/performance/test_write_amplification.py b/test_runner/performance/test_write_amplification.py index 30c217e392..3e290b3996 100644 --- a/test_runner/performance/test_write_amplification.py +++ b/test_runner/performance/test_write_amplification.py @@ -22,7 +22,6 @@ def test_write_amplification(neon_with_baseline: PgCompare): with conn.cursor() as cur: with env.record_pageserver_writes("pageserver_writes"): with env.record_duration("run"): - # NOTE: Because each iteration updates every table already created, # the runtime and write amplification is O(n^2), where n is the # number of iterations. diff --git a/test_runner/regress/test_crafted_wal_end.py b/test_runner/regress/test_crafted_wal_end.py index e94c9a2bd0..9899d424d1 100644 --- a/test_runner/regress/test_crafted_wal_end.py +++ b/test_runner/regress/test_crafted_wal_end.py @@ -2,6 +2,7 @@ import pytest from fixtures.log_helper import log from fixtures.neon_fixtures import NeonEnvBuilder, WalCraft + # Restart nodes with WAL end having specially crafted shape, like last record # crossing segment boundary, to test decoding issues. diff --git a/test_runner/regress/test_fullbackup.py b/test_runner/regress/test_fullbackup.py index fc515e5878..b3d58edf6b 100644 --- a/test_runner/regress/test_fullbackup.py +++ b/test_runner/regress/test_fullbackup.py @@ -2,7 +2,12 @@ import os from pathlib import Path from fixtures.log_helper import log -from fixtures.neon_fixtures import NeonEnvBuilder, PgBin, PortDistributor, VanillaPostgres +from fixtures.neon_fixtures import ( + NeonEnvBuilder, + PgBin, + PortDistributor, + VanillaPostgres, +) from fixtures.types import Lsn, TimelineId from fixtures.utils import query_scalar, subprocess_capture diff --git a/test_runner/regress/test_gc_aggressive.py b/test_runner/regress/test_gc_aggressive.py index 77438e1b64..702d94c691 100644 --- a/test_runner/regress/test_gc_aggressive.py +++ b/test_runner/regress/test_gc_aggressive.py @@ -68,7 +68,6 @@ async def update_and_gc(env: NeonEnv, pg: Postgres, timeline: TimelineId): # (repro for https://github.com/neondatabase/neon/issues/1047) # def test_gc_aggressive(neon_env_builder: NeonEnvBuilder): - # Disable pitr, because here we want to test branch creation after GC neon_env_builder.pageserver_config_override = "tenant_config={pitr_interval = '0 sec'}" env = neon_env_builder.init_start() @@ -101,7 +100,6 @@ def test_gc_aggressive(neon_env_builder: NeonEnvBuilder): # @pytest.mark.parametrize("remote_storage_kind", [RemoteStorageKind.LOCAL_FS]) def test_gc_index_upload(neon_env_builder: NeonEnvBuilder, remote_storage_kind: RemoteStorageKind): - # Disable time-based pitr, we will use LSN-based thresholds in the manual GC calls neon_env_builder.pageserver_config_override = "tenant_config={pitr_interval = '0 sec'}" diff --git a/test_runner/regress/test_layer_eviction.py b/test_runner/regress/test_layer_eviction.py index e7c9713f98..80e7ae8d7e 100644 --- a/test_runner/regress/test_layer_eviction.py +++ b/test_runner/regress/test_layer_eviction.py @@ -146,7 +146,6 @@ def test_basic_eviction( def test_gc_of_remote_layers(neon_env_builder: NeonEnvBuilder): - neon_env_builder.enable_remote_storage( remote_storage_kind=RemoteStorageKind.LOCAL_FS, test_name="test_gc_of_remote_layers", diff --git a/test_runner/regress/test_ondemand_download.py b/test_runner/regress/test_ondemand_download.py index 12088c3353..fd13651427 100644 --- a/test_runner/regress/test_ondemand_download.py +++ b/test_runner/regress/test_ondemand_download.py @@ -250,7 +250,7 @@ def test_ondemand_download_timetravel( # Run queries at different points in time num_layers_downloaded = [0] resident_size = [get_resident_physical_size()] - for (checkpoint_number, lsn) in lsns: + for checkpoint_number, lsn in lsns: pg_old = env.postgres.create_start( branch_name="main", node_name=f"test_old_lsn_{checkpoint_number}", lsn=lsn ) diff --git a/test_runner/regress/test_read_validation.py b/test_runner/regress/test_read_validation.py index 1e49c3b69f..47135dc56c 100644 --- a/test_runner/regress/test_read_validation.py +++ b/test_runner/regress/test_read_validation.py @@ -22,7 +22,6 @@ def test_read_validation(neon_simple_env: NeonEnv): with closing(pg.connect()) as con: with con.cursor() as c: - for e in extensions: c.execute("create extension if not exists {};".format(e)) @@ -150,7 +149,6 @@ def test_read_validation_neg(neon_simple_env: NeonEnv): with closing(pg.connect()) as con: with con.cursor() as c: - for e in extensions: c.execute("create extension if not exists {};".format(e)) diff --git a/test_runner/regress/test_readonly_node.py b/test_runner/regress/test_readonly_node.py index 62c3ead0a7..7487757071 100644 --- a/test_runner/regress/test_readonly_node.py +++ b/test_runner/regress/test_readonly_node.py @@ -146,7 +146,7 @@ def test_timetravel(neon_simple_env: NeonEnv): env.pageserver.stop() env.pageserver.start() - for (i, lsn) in lsns: + for i, lsn in lsns: pg_old = env.postgres.create_start( branch_name="test_timetravel", node_name=f"test_old_lsn_{i}", lsn=lsn ) diff --git a/test_runner/regress/test_remote_storage.py b/test_runner/regress/test_remote_storage.py index dd0b576c5e..1f6f0c67cc 100644 --- a/test_runner/regress/test_remote_storage.py +++ b/test_runner/regress/test_remote_storage.py @@ -212,7 +212,6 @@ def test_remote_storage_upload_queue_retries( neon_env_builder: NeonEnvBuilder, remote_storage_kind: RemoteStorageKind, ): - neon_env_builder.enable_remote_storage( remote_storage_kind=remote_storage_kind, test_name="test_remote_storage_upload_queue_retries", @@ -374,7 +373,6 @@ def test_remote_timeline_client_calls_started_metric( neon_env_builder: NeonEnvBuilder, remote_storage_kind: RemoteStorageKind, ): - neon_env_builder.enable_remote_storage( remote_storage_kind=remote_storage_kind, test_name="test_remote_timeline_client_metrics", diff --git a/test_runner/regress/test_tenant_detach.py b/test_runner/regress/test_tenant_detach.py index ac1f7b2891..27ec38e1be 100644 --- a/test_runner/regress/test_tenant_detach.py +++ b/test_runner/regress/test_tenant_detach.py @@ -177,6 +177,7 @@ async def reattach_while_busy( # running, and when we retry the queries, they should start working # after the attach has finished. + # FIXME: # # This is pretty unstable at the moment. I've seen it fail with a warning like this: diff --git a/test_runner/regress/test_tenant_relocation.py b/test_runner/regress/test_tenant_relocation.py index 1b58937e2a..247fed846e 100644 --- a/test_runner/regress/test_tenant_relocation.py +++ b/test_runner/regress/test_tenant_relocation.py @@ -375,7 +375,6 @@ def test_tenant_relocation( neon_env_builder.broker, neon_env_builder.pg_distrib_dir, ): - # Migrate either by attaching from s3 or import/export basebackup if method == "major": cmd = [ diff --git a/test_runner/regress/test_timeline_delete.py b/test_runner/regress/test_timeline_delete.py index d8f9ef2f89..2226cab8ff 100644 --- a/test_runner/regress/test_timeline_delete.py +++ b/test_runner/regress/test_timeline_delete.py @@ -40,7 +40,6 @@ def test_timeline_delete(neon_simple_env: NeonEnv): with pytest.raises( PageserverApiException, match="Cannot delete timeline which has child timelines" ): - timeline_path = ( env.repo_dir / "tenants" diff --git a/test_runner/regress/test_timeline_size.py b/test_runner/regress/test_timeline_size.py index ea4b65c9a8..c4e8e7aa07 100644 --- a/test_runner/regress/test_timeline_size.py +++ b/test_runner/regress/test_timeline_size.py @@ -90,7 +90,6 @@ def test_timeline_size_createdropdb(neon_simple_env: NeonEnv): cur.execute("CREATE DATABASE foodb") with closing(pgmain.connect(dbname="foodb")) as conn: with conn.cursor() as cur2: - cur2.execute("CREATE TABLE foo (t text)") cur2.execute( """ @@ -308,7 +307,6 @@ def test_timeline_initial_logical_size_calculation_cancellation( def test_timeline_physical_size_init( neon_env_builder: NeonEnvBuilder, remote_storage_kind: Optional[RemoteStorageKind] ): - if remote_storage_kind is not None: neon_env_builder.enable_remote_storage( remote_storage_kind, "test_timeline_physical_size_init" @@ -385,7 +383,6 @@ def test_timeline_physical_size_post_checkpoint( def test_timeline_physical_size_post_compaction( neon_env_builder: NeonEnvBuilder, remote_storage_kind: Optional[RemoteStorageKind] ): - if remote_storage_kind is not None: neon_env_builder.enable_remote_storage( remote_storage_kind, "test_timeline_physical_size_init" @@ -440,7 +437,6 @@ def test_timeline_physical_size_post_compaction( def test_timeline_physical_size_post_gc( neon_env_builder: NeonEnvBuilder, remote_storage_kind: Optional[RemoteStorageKind] ): - if remote_storage_kind is not None: neon_env_builder.enable_remote_storage( remote_storage_kind, "test_timeline_physical_size_init" diff --git a/test_runner/regress/test_truncate.py b/test_runner/regress/test_truncate.py index a358f94192..cfe8a7f067 100644 --- a/test_runner/regress/test_truncate.py +++ b/test_runner/regress/test_truncate.py @@ -7,7 +7,6 @@ from fixtures.neon_fixtures import NeonEnvBuilder # Test truncation of FSM and VM forks of a relation # def test_truncate(neon_env_builder: NeonEnvBuilder, zenbenchmark): - env = neon_env_builder.init_start() n_records = 10000 n_iter = 10 diff --git a/test_runner/regress/test_wal_acceptor.py b/test_runner/regress/test_wal_acceptor.py index 489afb7b93..407085a01a 100644 --- a/test_runner/regress/test_wal_acceptor.py +++ b/test_runner/regress/test_wal_acceptor.py @@ -709,7 +709,6 @@ def test_sync_safekeepers( pg_bin: PgBin, port_distributor: PortDistributor, ): - # We don't really need the full environment for this test, just the # safekeepers would be enough. neon_env_builder.num_safekeepers = 3 diff --git a/test_runner/regress/test_wal_acceptor_async.py b/test_runner/regress/test_wal_acceptor_async.py index 70ae6bae18..f10a40690e 100644 --- a/test_runner/regress/test_wal_acceptor_async.py +++ b/test_runner/regress/test_wal_acceptor_async.py @@ -521,7 +521,6 @@ async def run_race_conditions(env: NeonEnv, pg: Postgres): # do inserts while concurrently getting up/down subsets of acceptors def test_race_conditions(neon_env_builder: NeonEnvBuilder): - neon_env_builder.num_safekeepers = 3 env = neon_env_builder.init_start() @@ -588,7 +587,6 @@ async def run_wal_lagging(env: NeonEnv, pg: Postgres): # do inserts while restarting postgres and messing with safekeeper addresses def test_wal_lagging(neon_env_builder: NeonEnvBuilder): - neon_env_builder.num_safekeepers = 3 env = neon_env_builder.init_start() diff --git a/test_runner/regress/test_wal_restore.py b/test_runner/regress/test_wal_restore.py index e1b1e03515..63d0b46f63 100644 --- a/test_runner/regress/test_wal_restore.py +++ b/test_runner/regress/test_wal_restore.py @@ -1,6 +1,11 @@ from pathlib import Path -from fixtures.neon_fixtures import NeonEnvBuilder, PgBin, PortDistributor, VanillaPostgres +from fixtures.neon_fixtures import ( + NeonEnvBuilder, + PgBin, + PortDistributor, + VanillaPostgres, +) from fixtures.types import TenantId From 15b692ccc945de0179cefe0815aebf8554649533 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Tue, 14 Mar 2023 15:27:52 +0200 Subject: [PATCH 09/32] test: more strict finding of WARN, ERROR lines (#3798) this prevents flakyness when `WARN|ERROR` appears in some other part of the line, for example in a random filename. --- test_runner/fixtures/neon_fixtures.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index a25709a305..c5e260a962 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -2154,7 +2154,7 @@ class NeonPageserver(PgProtocol): def assert_no_errors(self): logfile = open(os.path.join(self.env.repo_dir, "pageserver.log"), "r") - error_or_warn = re.compile("ERROR|WARN") + error_or_warn = re.compile(r"\s(ERROR|WARN)") errors = [] while True: line = logfile.readline() From c23c8946a3a867229735ae7732417fa2b0cb98f9 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Tue, 14 Mar 2023 15:29:02 +0200 Subject: [PATCH 10/32] chore: clippies introduced with rust 1.68 (#3781) - handle automatically fixable future clippies - tune run-clippy.sh to remove macos specifics which we no longer have Co-authored-by: Alexander Bayandin --- libs/pq_proto/src/lib.rs | 8 +++----- libs/utils/src/fs_ext.rs | 2 +- pageserver/src/tenant.rs | 7 ++----- pageserver/src/tenant/timeline.rs | 4 +--- proxy/src/scram/messages.rs | 2 +- run_clippy.sh | 18 +++++------------- 6 files changed, 13 insertions(+), 28 deletions(-) diff --git a/libs/pq_proto/src/lib.rs b/libs/pq_proto/src/lib.rs index 46d531239a..656c0ff312 100644 --- a/libs/pq_proto/src/lib.rs +++ b/libs/pq_proto/src/lib.rs @@ -265,11 +265,9 @@ impl FeMessage { b'c' => Ok(Some(FeMessage::CopyDone)), b'f' => Ok(Some(FeMessage::CopyFail)), b'p' => Ok(Some(FeMessage::PasswordMessage(msg))), - tag => { - return Err(ProtocolError::Protocol(format!( - "unknown message tag: {tag},'{msg:?}'" - ))) - } + tag => Err(ProtocolError::Protocol(format!( + "unknown message tag: {tag},'{msg:?}'" + ))), } } } diff --git a/libs/utils/src/fs_ext.rs b/libs/utils/src/fs_ext.rs index b8d00df409..d2cb7be816 100644 --- a/libs/utils/src/fs_ext.rs +++ b/libs/utils/src/fs_ext.rs @@ -11,7 +11,7 @@ where P: AsRef, { fn is_empty_dir(&self) -> io::Result { - Ok(fs::read_dir(self)?.into_iter().next().is_none()) + Ok(fs::read_dir(self)?.next().is_none()) } } diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 6a373ad520..1fb312fe07 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -1243,11 +1243,8 @@ impl Tenant { "Cannot run GC iteration on inactive tenant" ); - let gc_result = self - .gc_iteration_internal(target_timeline_id, horizon, pitr, ctx) - .await; - - gc_result + self.gc_iteration_internal(target_timeline_id, horizon, pitr, ctx) + .await } /// Perform one compaction iteration. diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index c304791ee2..a8847ec773 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -3147,9 +3147,7 @@ impl Timeline { } fail_point!("delta-layer-writer-fail-before-finish", |_| { - return Err( - anyhow::anyhow!("failpoint delta-layer-writer-fail-before-finish").into(), - ); + Err(anyhow::anyhow!("failpoint delta-layer-writer-fail-before-finish").into()) }); writer.as_mut().unwrap().put_value(key, lsn, value)?; diff --git a/proxy/src/scram/messages.rs b/proxy/src/scram/messages.rs index 05855e74df..b59baec508 100644 --- a/proxy/src/scram/messages.rs +++ b/proxy/src/scram/messages.rs @@ -14,7 +14,7 @@ pub const SCRAM_RAW_NONCE_LEN: usize = 18; fn validate_sasl_extensions<'a>(parts: impl Iterator) -> Option<()> { for mut chars in parts.map(|s| s.chars()) { let attr = chars.next()?; - if !('a'..='z').contains(&attr) && !('A'..='Z').contains(&attr) { + if !attr.is_ascii_alphabetic() { return None; } let eq = chars.next()?; diff --git a/run_clippy.sh b/run_clippy.sh index be07a0110a..ae9482ee96 100755 --- a/run_clippy.sh +++ b/run_clippy.sh @@ -8,21 +8,13 @@ # warnings and errors right in the editor. # In vscode, this setting is Rust-analyzer>Check On Save:Command - -# Not every feature is supported in macOS builds. Avoid running regular linting -# script that checks every feature. -# # manual-range-contains wants # !(4..=MAX_STARTUP_PACKET_LENGTH).contains(&len) # instead of # len < 4 || len > MAX_STARTUP_PACKET_LENGTH # , let's disagree. -if [[ "$OSTYPE" == "darwin"* ]]; then - # no extra features to test currently, add more here when needed - cargo clippy --locked --all --all-targets --features testing -- -A unknown_lints -A clippy::manual-range-contains -D warnings -else - # * `-A unknown_lints` – do not warn about unknown lint suppressions - # that people with newer toolchains might use - # * `-D warnings` - fail on any warnings (`cargo` returns non-zero exit status) - cargo clippy --locked --all --all-targets --all-features -- -A unknown_lints -A clippy::manual-range-contains -D warnings -fi + +# * `-A unknown_lints` – do not warn about unknown lint suppressions +# that people with newer toolchains might use +# * `-D warnings` - fail on any warnings (`cargo` returns non-zero exit status) +cargo clippy --locked --all --all-targets --all-features -- -A unknown_lints -A clippy::manual-range-contains -D warnings From 5396273541ebcb4b22b5577bfbae29c3a809353f Mon Sep 17 00:00:00 2001 From: Konstantin Knizhnik Date: Tue, 14 Mar 2023 16:15:07 +0200 Subject: [PATCH 11/32] Avoid holes between generated image layers (#3771) ## Describe your changes When we perform partitioning of the whole key space, we take in account actual ranges of relation present in the database. So if we have relation with relid=1 and size 100 and relation with relid=2 with size 200 then result of KeySpace::partition may contain partitions <100000000..100000099> and <200000000..200000199>. Generated image layers will contain the same boundaries. But when GC is checking image coverage to find out of old layer is fully covered by newer image layer and so can be deleted, it takes in account only full key range. I.e. if there is delta layer <100000000..300000000> then it never be garbage collected because image layers <100000000..100000099> and <200000000..200000199> are not completely covering it.This is how it looks in practice: 000000067F000032AC00000A300000000000-000000067F000032AC00000A330000000000__000000000F761828 000000067F000032AC00000A31000000001F-000000067F000032AC00000A620000000005__0000000001696070-000000000442A551 000000067F000032AC00000A3300FFFFFFFF-000000067F000032AC00000A650100000000__000000000F761828 So there are two image layers covering delta layer but ... there is a hole: A330000000000...A3300FFFFFFFF and as a result delta layer is not collected. ## Issue ticket number and link This PR is deeply related with #3673 because it is addressing the same problem: old layers are not utilized by GC. The test test_gc_old_layers.py in #3673 can be used to see effect of this patch. ## Checklist before requesting a review - [ ] I have performed a self-review of my code. - [ ] If it is a core feature, I have added thorough tests. - [ ] Do we need to implement analytics? if so did you add the relevant metrics to the dashboard? - [ ] If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section. --------- Co-authored-by: Joonas Koivunen --- pageserver/src/tenant/timeline.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index a8847ec773..fff288c683 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -2715,10 +2715,22 @@ impl Timeline { ) -> Result, PageReconstructError> { let timer = self.metrics.create_images_time_histo.start_timer(); let mut image_layers: Vec = Vec::new(); + + // We need to avoid holes between generated image layers. + // Otherwise LayerMap::image_layer_exists will return false if key range of some layer is covered by more than one + // image layer with hole between them. In this case such layer can not be utilized by GC. + // + // How such hole between partitions can appear? + // if we have relation with relid=1 and size 100 and relation with relid=2 with size 200 then result of + // KeySpace::partition may contain partitions <100000000..100000099> and <200000000..200000199>. + // If there is delta layer <100000000..300000000> then it never be garbage collected because + // image layers <100000000..100000099> and <200000000..200000199> are not completely covering it. + let mut start = Key::MIN; + for partition in partitioning.parts.iter() { + let img_range = start..partition.ranges.last().unwrap().end; + start = img_range.end; if force || self.time_for_new_image_layer(partition, lsn)? { - let img_range = - partition.ranges.first().unwrap().start..partition.ranges.last().unwrap().end; let mut image_layer_writer = ImageLayerWriter::new( self.conf, self.timeline_id, @@ -2732,7 +2744,6 @@ impl Timeline { "failpoint image-layer-writer-fail-before-finish" ))) }); - for range in &partition.ranges { let mut key = range.start; while key < range.end { From 4a92799f24a6b470b3ba1bddebf228589fca5ba2 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Tue, 14 Mar 2023 13:10:00 +0200 Subject: [PATCH 12/32] Fix check for trailing garbage in basebackup import. There was a warning for trailing garbage after end-of-tar archive, but it didn't always work. The reason is that we created a StreamReader over the original copyin-stream, but performed the check for garbage on the copyin-stream. There could be some garbage bytes buffered in the StreamReader, which were not caught by the warning. I considered turning the the warning into a fatal error, aborting the import, but I wasn't sure if we handle aborting the import properly. Do we clean up the timeline directory on error? If we don't, we should make that more robust, but that's a different story. Also, normally a valid tar archive ends with two 512-byte blocks of zeros. The tokio_tar crate stops at the first all-zeros block. Read and check the second all-zeros block, and error out if it's not there, or contains something unexpected. --- pageserver/src/page_service.rs | 77 +++++++++++++++++++++--------- pageserver/src/tenant.rs | 7 +-- test_runner/regress/test_import.py | 19 +++++++- 3 files changed, 74 insertions(+), 29 deletions(-) diff --git a/pageserver/src/page_service.rs b/pageserver/src/page_service.rs index dc9bf955f7..aad6099952 100644 --- a/pageserver/src/page_service.rs +++ b/pageserver/src/page_service.rs @@ -12,7 +12,7 @@ use anyhow::Context; use bytes::Buf; use bytes::Bytes; -use futures::{Stream, StreamExt}; +use futures::Stream; use pageserver_api::models::TenantState; use pageserver_api::models::{ PagestreamBeMessage, PagestreamDbSizeRequest, PagestreamDbSizeResponse, @@ -31,6 +31,7 @@ use std::str; use std::str::FromStr; use std::sync::Arc; use std::time::Duration; +use tokio_util::io::StreamReader; use tracing::*; use utils::id::ConnectionId; use utils::{ @@ -115,6 +116,49 @@ fn copyin_stream(pgb: &mut PostgresBackendTCP) -> impl Stream anyhow::Result<()> { + use tokio::io::AsyncReadExt; + let mut buf = [0u8; 512]; + + // Read the all-zeros block, and verify it + let mut total_bytes = 0; + while total_bytes < 512 { + let nbytes = reader.read(&mut buf[total_bytes..]).await?; + total_bytes += nbytes; + if nbytes == 0 { + break; + } + } + if total_bytes < 512 { + anyhow::bail!("incomplete or invalid tar EOF marker"); + } + if !buf.iter().all(|&x| x == 0) { + anyhow::bail!("invalid tar EOF marker"); + } + + // Drain any data after the EOF marker + let mut trailing_bytes = 0; + loop { + let nbytes = reader.read(&mut buf).await?; + trailing_bytes += nbytes; + if nbytes == 0 { + break; + } + } + if trailing_bytes > 0 { + warn!("ignored {trailing_bytes} unexpected bytes after the tar archive"); + } + Ok(()) +} + /////////////////////////////////////////////////////////////////////////////// /// @@ -422,19 +466,14 @@ impl PageServerHandler { pgb.write_message_noflush(&BeMessage::CopyInResponse)?; pgb.flush().await?; - let mut copyin_stream = Box::pin(copyin_stream(pgb)); + let copyin_reader = StreamReader::new(copyin_stream(pgb)); + tokio::pin!(copyin_reader); timeline - .import_basebackup_from_tar(&mut copyin_stream, base_lsn, &ctx) + .import_basebackup_from_tar(&mut copyin_reader, base_lsn, &ctx) .await?; - // Drain the rest of the Copy data - let mut bytes_after_tar = 0; - while let Some(bytes) = copyin_stream.next().await { - bytes_after_tar += bytes?.len(); - } - if bytes_after_tar > 0 { - warn!("ignored {bytes_after_tar} unexpected bytes after the tar archive"); - } + // Read the end of the tar archive. + read_tar_eof(copyin_reader).await?; // TODO check checksum // Meanwhile you can verify client-side by taking fullbackup @@ -473,19 +512,13 @@ impl PageServerHandler { info!("importing wal"); pgb.write_message_noflush(&BeMessage::CopyInResponse)?; pgb.flush().await?; - let mut copyin_stream = Box::pin(copyin_stream(pgb)); - let mut reader = tokio_util::io::StreamReader::new(&mut copyin_stream); - import_wal_from_tar(&timeline, &mut reader, start_lsn, end_lsn, &ctx).await?; + let copyin_reader = StreamReader::new(copyin_stream(pgb)); + tokio::pin!(copyin_reader); + import_wal_from_tar(&timeline, &mut copyin_reader, start_lsn, end_lsn, &ctx).await?; info!("wal import complete"); - // Drain the rest of the Copy data - let mut bytes_after_tar = 0; - while let Some(bytes) = copyin_stream.next().await { - bytes_after_tar += bytes?.len(); - } - if bytes_after_tar > 0 { - warn!("ignored {bytes_after_tar} unexpected bytes after the tar archive"); - } + // Read the end of the tar archive. + read_tar_eof(copyin_reader).await?; // TODO Does it make sense to overshoot? if timeline.get_last_record_lsn() < end_lsn { diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 1fb312fe07..5f1e23b873 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -12,9 +12,7 @@ //! use anyhow::{bail, Context}; -use bytes::Bytes; use futures::FutureExt; -use futures::Stream; use pageserver_api::models::TimelineState; use remote_storage::DownloadError; use remote_storage::GenericRemoteStorage; @@ -239,14 +237,13 @@ impl UninitializedTimeline<'_> { /// Prepares timeline data by loading it from the basebackup archive. pub async fn import_basebackup_from_tar( self, - copyin_stream: &mut (impl Stream> + Sync + Send + Unpin), + copyin_read: &mut (impl tokio::io::AsyncRead + Send + Sync + Unpin), base_lsn: Lsn, ctx: &RequestContext, ) -> anyhow::Result> { let raw_timeline = self.raw_timeline()?; - let mut reader = tokio_util::io::StreamReader::new(copyin_stream); - import_datadir::import_basebackup_from_tar(raw_timeline, &mut reader, base_lsn, ctx) + import_datadir::import_basebackup_from_tar(raw_timeline, copyin_read, base_lsn, ctx) .await .context("Failed to import basebackup")?; diff --git a/test_runner/regress/test_import.py b/test_runner/regress/test_import.py index 0388e24e98..1dc10fbf4f 100644 --- a/test_runner/regress/test_import.py +++ b/test_runner/regress/test_import.py @@ -61,6 +61,12 @@ def test_import_from_vanilla(test_output_dir, pg_bin, vanilla_pg, neon_env_build cwd=unpacked_base, ) + # Make copy of base.tar and append some garbage to it. + base_plus_garbage_tar = os.path.join(basebackup_dir, "base-plus-garbage.tar") + shutil.copyfile(base_tar, base_plus_garbage_tar) + with open(base_plus_garbage_tar, "a") as f: + f.write("trailing garbage") + # Get start_lsn and end_lsn with open(os.path.join(basebackup_dir, "backup_manifest")) as f: manifest = json.load(f) @@ -74,7 +80,8 @@ def test_import_from_vanilla(test_output_dir, pg_bin, vanilla_pg, neon_env_build # Set up pageserver for import neon_env_builder.enable_local_fs_remote_storage() env = neon_env_builder.init_start() - env.pageserver.http_client().tenant_create(tenant) + client = env.pageserver.http_client() + client.tenant_create(tenant) env.pageserver.allowed_errors.extend( [ @@ -85,6 +92,7 @@ def test_import_from_vanilla(test_output_dir, pg_bin, vanilla_pg, neon_env_build ".*InternalServerError.*Tenant .* not found.*", ".*InternalServerError.*Timeline .* not found.*", ".*InternalServerError.*Cannot delete timeline which has child timelines.*", + ".*ignored .* unexpected bytes after the tar archive.*", ] ) @@ -130,11 +138,18 @@ def test_import_from_vanilla(test_output_dir, pg_bin, vanilla_pg, neon_env_build with pytest.raises(Exception): import_tar(corrupt_base_tar, wal_tar) + # A tar with trailing garbage is currently accepted. It prints a warnings + # to the pageserver log, however. Check that. + import_tar(base_plus_garbage_tar, wal_tar) + assert env.pageserver.log_contains( + ".*WARN.*ignored .* unexpected bytes after the tar archive.*" + ) + client.timeline_delete(tenant, timeline) + # Importing correct backup works import_tar(base_tar, wal_tar) # Wait for data to land in s3 - client = env.pageserver.http_client() wait_for_last_record_lsn(client, tenant, timeline, Lsn(end_lsn)) wait_for_upload(client, tenant, timeline, Lsn(end_lsn)) From 2672fd09d808deaaf1ce6da11d9c22e02570282f Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Tue, 14 Mar 2023 17:36:10 +0200 Subject: [PATCH 13/32] Make test independent of the order of config lines. --- test_runner/regress/test_tenant_relocation.py | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/test_runner/regress/test_tenant_relocation.py b/test_runner/regress/test_tenant_relocation.py index 247fed846e..aaf33c0d59 100644 --- a/test_runner/regress/test_tenant_relocation.py +++ b/test_runner/regress/test_tenant_relocation.py @@ -434,10 +434,13 @@ def test_tenant_relocation( ) # rewrite neon cli config to use new pageserver for basebackup to start new compute - cli_config_lines = (env.repo_dir / "config").read_text().splitlines() - cli_config_lines[-2] = f"listen_http_addr = 'localhost:{new_pageserver_http_port}'" - cli_config_lines[-1] = f"listen_pg_addr = 'localhost:{new_pageserver_pg_port}'" - (env.repo_dir / "config").write_text("\n".join(cli_config_lines)) + lines = (env.repo_dir / "config").read_text().splitlines() + for i, line in enumerate(lines): + if line.startswith("listen_http_addr"): + lines[i] = f"listen_http_addr = 'localhost:{new_pageserver_http_port}'" + if line.startswith("listen_pg_addr"): + lines[i] = f"listen_pg_addr = 'localhost:{new_pageserver_pg_port}'" + (env.repo_dir / "config").write_text("\n".join(lines)) old_local_path_main = switch_pg_to_new_pageserver( env, @@ -496,7 +499,10 @@ def test_tenant_relocation( # bring old pageserver back for clean shutdown via neon cli # new pageserver will be shut down by the context manager - cli_config_lines = (env.repo_dir / "config").read_text().splitlines() - cli_config_lines[-2] = f"listen_http_addr = 'localhost:{env.pageserver.service_port.http}'" - cli_config_lines[-1] = f"listen_pg_addr = 'localhost:{env.pageserver.service_port.pg}'" - (env.repo_dir / "config").write_text("\n".join(cli_config_lines)) + lines = (env.repo_dir / "config").read_text().splitlines() + for i, line in enumerate(lines): + if line.startswith("listen_http_addr"): + lines[i] = f"listen_http_addr = 'localhost:{env.pageserver.service_port.http}'" + if line.startswith("listen_pg_addr"): + lines[i] = f"listen_pg_addr = 'localhost:{env.pageserver.service_port.pg}'" + (env.repo_dir / "config").write_text("\n".join(lines)) From a7ab53c80cea7ab7922ebd2a5a4d71963ffcb3ba Mon Sep 17 00:00:00 2001 From: Arseny Sher Date: Wed, 15 Mar 2023 11:44:55 +0400 Subject: [PATCH 14/32] Forward framed read buf contents to compute before proxy pass. Otherwise they get lost. Normally buffer is empty before proxy pass, but this is not the case with pipeline mode of out npm driver; fixes connection hangup introduced by b80fe41af3e for it. fixes https://github.com/neondatabase/neon/issues/3822 --- libs/pq_proto/src/framed.rs | 6 +++--- proxy/src/proxy.rs | 27 +++++++++++++++++++++------ proxy/src/stream.rs | 5 +++-- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/libs/pq_proto/src/framed.rs b/libs/pq_proto/src/framed.rs index 972730cbab..3cdca45009 100644 --- a/libs/pq_proto/src/framed.rs +++ b/libs/pq_proto/src/framed.rs @@ -63,9 +63,9 @@ impl Framed { &self.stream } - /// Extract the underlying stream. - pub fn into_inner(self) -> S { - self.stream + /// Deconstruct into the underlying stream and read buffer. + pub fn into_inner(self) -> (S, BytesMut) { + (self.stream, self.read_buf) } /// Return new Framed with stream type transformed by async f, for TLS diff --git a/proxy/src/proxy.rs b/proxy/src/proxy.rs index abeff6a33b..efe0e8795b 100644 --- a/proxy/src/proxy.rs +++ b/proxy/src/proxy.rs @@ -16,7 +16,7 @@ use metrics::{register_int_counter, register_int_counter_vec, IntCounter, IntCou use once_cell::sync::Lazy; use pq_proto::{BeMessage as Be, FeStartupPacket, StartupMessageParams}; use std::sync::Arc; -use tokio::io::{AsyncRead, AsyncWrite}; +use tokio::io::{AsyncRead, AsyncWrite, AsyncWriteExt}; use tracing::{error, info, warn}; use utils::measured_stream::MeasuredStream; @@ -209,9 +209,18 @@ async fn handshake( if let Some(tls) = tls.take() { // Upgrade raw stream into a secure TLS-backed stream. // NOTE: We've consumed `tls`; this fact will be used later. - stream = PqStream::new( - stream.into_inner().upgrade(tls.to_server_config()).await?, - ); + + let (raw, read_buf) = stream.into_inner(); + // TODO: Normally, client doesn't send any data before + // server says TLS handshake is ok and read_buf is empy. + // However, you could imagine pipelining of postgres + // SSLRequest + TLS ClientHello in one hunk similar to + // pipelining in our node js driver. We should probably + // support that by chaining read_buf with the stream. + if !read_buf.is_empty() { + bail!("data is sent before server replied with EncryptionResponse"); + } + stream = PqStream::new(raw.upgrade(tls.to_server_config()).await?); } } _ => bail!(ERR_PROTO_VIOLATION), @@ -443,11 +452,17 @@ impl Client<'_, S> { value: mut node_info, } = auth_result; - let node = connect_to_compute(&mut node_info, params, &extra, &creds) + let mut node = connect_to_compute(&mut node_info, params, &extra, &creds) .or_else(|e| stream.throw_error(e)) .await?; prepare_client_connection(&node, reported_auth_ok, session, &mut stream).await?; - proxy_pass(stream.into_inner(), node.stream, &node_info.aux).await + // Before proxy passing, forward to compute whatever data is left in the + // PqStream input buffer. Normally there is none, but our serverless npm + // driver in pipeline mode sends startup, password and first query + // immediately after opening the connection. + let (stream, read_buf) = stream.into_inner(); + node.stream.write_all(&read_buf).await?; + proxy_pass(stream, node.stream, &node_info.aux).await } } diff --git a/proxy/src/stream.rs b/proxy/src/stream.rs index 9dfc435e39..7cb292ed58 100644 --- a/proxy/src/stream.rs +++ b/proxy/src/stream.rs @@ -1,5 +1,6 @@ use crate::error::UserFacingError; use anyhow::bail; +use bytes::BytesMut; use pin_project_lite::pin_project; use pq_proto::framed::{ConnectionError, Framed}; use pq_proto::{BeMessage, FeMessage, FeStartupPacket, ProtocolError}; @@ -27,8 +28,8 @@ impl PqStream { } } - /// Extract the underlying stream. - pub fn into_inner(self) -> S { + /// Extract the underlying stream and read buffer. + pub fn into_inner(self) -> (S, BytesMut) { self.framed.into_inner() } From 10a5d36af80984d2314fa0d372a4b56574f69cac Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Wed, 15 Mar 2023 13:52:29 +0200 Subject: [PATCH 15/32] Separate mgmt and libpq authentication configs in pageserver. (#3773) This makes it possible to enable authentication only for the mgmt HTTP API or the compute API. The HTTP API doesn't need to be directly accessible from compute nodes, and it can be secured through network policies. This also allows rolling out authentication in a piecemeal fashion. --- control_plane/safekeepers.conf | 3 +- control_plane/simple.conf | 3 +- control_plane/src/bin/neon_local.rs | 7 ++-- control_plane/src/compute.rs | 2 +- control_plane/src/local_env.rs | 9 +++-- control_plane/src/pageserver.rs | 22 ++++++++---- docs/authentication.md | 8 +++-- pageserver/src/bin/pageserver.rs | 40 ++++++++++++++------- pageserver/src/config.rs | 43 ++++++++++++++++------- pageserver/src/http/routes.rs | 1 + test_runner/fixtures/neon_fixtures.py | 6 ++-- test_runner/regress/test_compatibility.py | 7 ++++ 12 files changed, 107 insertions(+), 44 deletions(-) diff --git a/control_plane/safekeepers.conf b/control_plane/safekeepers.conf index df7dd2adca..576cc4a3a9 100644 --- a/control_plane/safekeepers.conf +++ b/control_plane/safekeepers.conf @@ -2,7 +2,8 @@ [pageserver] listen_pg_addr = '127.0.0.1:64000' listen_http_addr = '127.0.0.1:9898' -auth_type = 'Trust' +pg_auth_type = 'Trust' +http_auth_type = 'Trust' [[safekeepers]] id = 1 diff --git a/control_plane/simple.conf b/control_plane/simple.conf index 6014e8dffd..243e13f3d3 100644 --- a/control_plane/simple.conf +++ b/control_plane/simple.conf @@ -3,7 +3,8 @@ [pageserver] listen_pg_addr = '127.0.0.1:64000' listen_http_addr = '127.0.0.1:9898' -auth_type = 'Trust' +pg_auth_type = 'Trust' +http_auth_type = 'Trust' [[safekeepers]] id = 1 diff --git a/control_plane/src/bin/neon_local.rs b/control_plane/src/bin/neon_local.rs index 49b1d31dbc..a9b66f479a 100644 --- a/control_plane/src/bin/neon_local.rs +++ b/control_plane/src/bin/neon_local.rs @@ -53,14 +53,15 @@ listen_addr = '{DEFAULT_BROKER_ADDR}' id = {DEFAULT_PAGESERVER_ID} listen_pg_addr = '{DEFAULT_PAGESERVER_PG_ADDR}' listen_http_addr = '{DEFAULT_PAGESERVER_HTTP_ADDR}' -auth_type = '{pageserver_auth_type}' +pg_auth_type = '{trust_auth}' +http_auth_type = '{trust_auth}' [[safekeepers]] id = {DEFAULT_SAFEKEEPER_ID} pg_port = {DEFAULT_SAFEKEEPER_PG_PORT} http_port = {DEFAULT_SAFEKEEPER_HTTP_PORT} "#, - pageserver_auth_type = AuthType::Trust, + trust_auth = AuthType::Trust, ) } @@ -627,7 +628,7 @@ fn handle_pg(pg_match: &ArgMatches, env: &local_env::LocalEnv) -> Result<()> { let node = cplane.nodes.get(&(tenant_id, node_name.to_string())); - let auth_token = if matches!(env.pageserver.auth_type, AuthType::NeonJWT) { + let auth_token = if matches!(env.pageserver.pg_auth_type, AuthType::NeonJWT) { let claims = Claims::new(Some(tenant_id), Scope::Tenant); Some(env.generate_auth_token(&claims)?) diff --git a/control_plane/src/compute.rs b/control_plane/src/compute.rs index b7029aabc5..094d2add8d 100644 --- a/control_plane/src/compute.rs +++ b/control_plane/src/compute.rs @@ -97,7 +97,7 @@ impl ComputeControlPlane { }); node.create_pgdata()?; - node.setup_pg_conf(self.env.pageserver.auth_type)?; + node.setup_pg_conf(self.env.pageserver.pg_auth_type)?; self.nodes .insert((tenant_id, node.name.clone()), Arc::clone(&node)); diff --git a/control_plane/src/local_env.rs b/control_plane/src/local_env.rs index 09180d96c4..630f8bb664 100644 --- a/control_plane/src/local_env.rs +++ b/control_plane/src/local_env.rs @@ -110,12 +110,14 @@ impl NeonBroker { pub struct PageServerConf { // node id pub id: NodeId, + // Pageserver connection settings pub listen_pg_addr: String, pub listen_http_addr: String, - // used to determine which auth type is used - pub auth_type: AuthType, + // auth type used for the PG and HTTP ports + pub pg_auth_type: AuthType, + pub http_auth_type: AuthType, // jwt auth token used for communication with pageserver pub auth_token: String, @@ -127,7 +129,8 @@ impl Default for PageServerConf { id: NodeId(0), listen_pg_addr: String::new(), listen_http_addr: String::new(), - auth_type: AuthType::Trust, + pg_auth_type: AuthType::Trust, + http_auth_type: AuthType::Trust, auth_token: String::new(), } } diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index 4b7180c250..07ead45d5b 100644 --- a/control_plane/src/pageserver.rs +++ b/control_plane/src/pageserver.rs @@ -82,7 +82,7 @@ impl PageServerNode { let (host, port) = parse_host_port(&env.pageserver.listen_pg_addr) .expect("Unable to parse listen_pg_addr"); let port = port.unwrap_or(5432); - let password = if env.pageserver.auth_type == AuthType::NeonJWT { + let password = if env.pageserver.pg_auth_type == AuthType::NeonJWT { Some(env.pageserver.auth_token.clone()) } else { None @@ -106,25 +106,32 @@ impl PageServerNode { self.env.pg_distrib_dir_raw().display() ); - let authg_type_param = format!("auth_type='{}'", self.env.pageserver.auth_type); + let http_auth_type_param = + format!("http_auth_type='{}'", self.env.pageserver.http_auth_type); let listen_http_addr_param = format!( "listen_http_addr='{}'", self.env.pageserver.listen_http_addr ); + + let pg_auth_type_param = format!("pg_auth_type='{}'", self.env.pageserver.pg_auth_type); let listen_pg_addr_param = format!("listen_pg_addr='{}'", self.env.pageserver.listen_pg_addr); + let broker_endpoint_param = format!("broker_endpoint='{}'", self.env.broker.client_url()); let mut overrides = vec![ id, pg_distrib_dir_param, - authg_type_param, + http_auth_type_param, + pg_auth_type_param, listen_http_addr_param, listen_pg_addr_param, broker_endpoint_param, ]; - if self.env.pageserver.auth_type != AuthType::Trust { + if self.env.pageserver.http_auth_type != AuthType::Trust + || self.env.pageserver.pg_auth_type != AuthType::Trust + { overrides.push("auth_validation_public_key_path='auth_public_key.pem'".to_owned()); } overrides @@ -247,7 +254,10 @@ impl PageServerNode { } fn pageserver_env_variables(&self) -> anyhow::Result> { - Ok(if self.env.pageserver.auth_type != AuthType::Trust { + // FIXME: why is this tied to pageserver's auth type? Whether or not the safekeeper + // needs a token, and how to generate that token, seems independent to whether + // the pageserver requires a token in incoming requests. + Ok(if self.env.pageserver.http_auth_type != AuthType::Trust { // Generate a token to connect from the pageserver to a safekeeper let token = self .env @@ -283,7 +293,7 @@ impl PageServerNode { fn http_request(&self, method: Method, url: U) -> RequestBuilder { let mut builder = self.http_client.request(method, url); - if self.env.pageserver.auth_type == AuthType::NeonJWT { + if self.env.pageserver.http_auth_type == AuthType::NeonJWT { builder = builder.bearer_auth(&self.env.pageserver.auth_token) } builder diff --git a/docs/authentication.md b/docs/authentication.md index 1637519211..e6b5fa5707 100644 --- a/docs/authentication.md +++ b/docs/authentication.md @@ -137,10 +137,12 @@ Each compute should present a token valid for the timeline's tenant. Pageserver also has HTTP API: some parts are per-tenant, some parts are server-wide, these are different scopes. -The `auth_type` configuration variable in Pageserver's config may have -either of three values: +Authentication can be enabled separately for the HTTP mgmt API, and +for the libpq connections from compute. The `http_auth_type` and +`pg_auth_type` configuration variables in Pageserver's config may +have one of these values: -* `Trust` removes all authentication. The outdated `MD5` value does likewise +* `Trust` removes all authentication. * `NeonJWT` enables JWT validation. Tokens are validated using the public key which lies in a PEM file specified in the `auth_validation_public_key_path` config. diff --git a/pageserver/src/bin/pageserver.rs b/pageserver/src/bin/pageserver.rs index 564a3de82c..14e86ddcb6 100644 --- a/pageserver/src/bin/pageserver.rs +++ b/pageserver/src/bin/pageserver.rs @@ -270,15 +270,31 @@ fn start_pageserver( WALRECEIVER_RUNTIME.block_on(pageserver::broker_client::init_broker_client(conf))?; // Initialize authentication for incoming connections - let auth = match &conf.auth_type { - AuthType::Trust => None, - AuthType::NeonJWT => { - // unwrap is ok because check is performed when creating config, so path is set and file exists - let key_path = conf.auth_validation_public_key_path.as_ref().unwrap(); - Some(JwtAuth::from_key_path(key_path)?.into()) - } - }; - info!("Using auth: {:#?}", conf.auth_type); + let http_auth; + let pg_auth; + if conf.http_auth_type == AuthType::NeonJWT || conf.pg_auth_type == AuthType::NeonJWT { + // unwrap is ok because check is performed when creating config, so path is set and file exists + let key_path = conf.auth_validation_public_key_path.as_ref().unwrap(); + info!( + "Loading public key for verifying JWT tokens from {:#?}", + key_path + ); + let auth: Arc = Arc::new(JwtAuth::from_key_path(key_path)?); + + http_auth = match &conf.http_auth_type { + AuthType::Trust => None, + AuthType::NeonJWT => Some(auth.clone()), + }; + pg_auth = match &conf.pg_auth_type { + AuthType::Trust => None, + AuthType::NeonJWT => Some(auth), + }; + } else { + http_auth = None; + pg_auth = None; + } + info!("Using auth for http API: {:#?}", conf.http_auth_type); + info!("Using auth for pg connections: {:#?}", conf.pg_auth_type); match var("NEON_AUTH_TOKEN") { Ok(v) => { @@ -308,7 +324,7 @@ fn start_pageserver( { let _rt_guard = MGMT_REQUEST_RUNTIME.enter(); - let router = http::make_router(conf, launch_ts, auth.clone(), remote_storage)? + let router = http::make_router(conf, launch_ts, http_auth, remote_storage)? .build() .map_err(|err| anyhow!(err))?; let service = utils::http::RouterService::new(router).unwrap(); @@ -382,9 +398,9 @@ fn start_pageserver( async move { page_service::libpq_listener_main( conf, - auth, + pg_auth, pageserver_listener, - conf.auth_type, + conf.pg_auth_type, libpq_ctx, ) .await diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs index fde889d01a..d17f0bc143 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -138,9 +138,15 @@ pub struct PageServerConf { pub pg_distrib_dir: PathBuf, - pub auth_type: AuthType, - + // Authentication + /// authentication method for the HTTP mgmt API + pub http_auth_type: AuthType, + /// authentication method for libpq connections from compute + pub pg_auth_type: AuthType, + /// Path to a file containing public key for verifying JWT tokens. + /// Used for both mgmt and compute auth, if enabled. pub auth_validation_public_key_path: Option, + pub remote_storage_config: Option, pub default_tenant_conf: TenantConf, @@ -208,7 +214,8 @@ struct PageServerConfigBuilder { pg_distrib_dir: BuilderValue, - auth_type: BuilderValue, + http_auth_type: BuilderValue, + pg_auth_type: BuilderValue, // auth_validation_public_key_path: BuilderValue>, @@ -251,7 +258,8 @@ impl Default for PageServerConfigBuilder { pg_distrib_dir: Set(env::current_dir() .expect("cannot access current directory") .join("pg_install")), - auth_type: Set(AuthType::Trust), + http_auth_type: Set(AuthType::Trust), + pg_auth_type: Set(AuthType::Trust), auth_validation_public_key_path: Set(None), remote_storage_config: Set(None), id: NotSet, @@ -323,8 +331,12 @@ impl PageServerConfigBuilder { self.pg_distrib_dir = BuilderValue::Set(pg_distrib_dir) } - pub fn auth_type(&mut self, auth_type: AuthType) { - self.auth_type = BuilderValue::Set(auth_type) + pub fn http_auth_type(&mut self, auth_type: AuthType) { + self.http_auth_type = BuilderValue::Set(auth_type) + } + + pub fn pg_auth_type(&mut self, auth_type: AuthType) { + self.pg_auth_type = BuilderValue::Set(auth_type) } pub fn auth_validation_public_key_path( @@ -419,7 +431,10 @@ impl PageServerConfigBuilder { pg_distrib_dir: self .pg_distrib_dir .ok_or(anyhow!("missing pg_distrib_dir"))?, - auth_type: self.auth_type.ok_or(anyhow!("missing auth_type"))?, + http_auth_type: self + .http_auth_type + .ok_or(anyhow!("missing http_auth_type"))?, + pg_auth_type: self.pg_auth_type.ok_or(anyhow!("missing pg_auth_type"))?, auth_validation_public_key_path: self .auth_validation_public_key_path .ok_or(anyhow!("missing auth_validation_public_key_path"))?, @@ -612,7 +627,8 @@ impl PageServerConf { "auth_validation_public_key_path" => builder.auth_validation_public_key_path(Some( PathBuf::from(parse_toml_string(key, item)?), )), - "auth_type" => builder.auth_type(parse_toml_from_str(key, item)?), + "http_auth_type" => builder.http_auth_type(parse_toml_from_str(key, item)?), + "pg_auth_type" => builder.pg_auth_type(parse_toml_from_str(key, item)?), "remote_storage" => { builder.remote_storage_config(RemoteStorageConfig::from_toml(item)?) } @@ -647,7 +663,7 @@ impl PageServerConf { let mut conf = builder.build().context("invalid config")?; - if conf.auth_type == AuthType::NeonJWT { + if conf.http_auth_type == AuthType::NeonJWT || conf.pg_auth_type == AuthType::NeonJWT { let auth_validation_public_key_path = conf .auth_validation_public_key_path .get_or_insert_with(|| workdir.join("auth_public_key.pem")); @@ -766,7 +782,8 @@ impl PageServerConf { superuser: "cloud_admin".to_string(), workdir: repo_dir, pg_distrib_dir, - auth_type: AuthType::Trust, + http_auth_type: AuthType::Trust, + pg_auth_type: AuthType::Trust, auth_validation_public_key_path: None, remote_storage_config: None, default_tenant_conf: TenantConf::default(), @@ -951,7 +968,8 @@ log_format = 'json' max_file_descriptors: defaults::DEFAULT_MAX_FILE_DESCRIPTORS, workdir, pg_distrib_dir, - auth_type: AuthType::Trust, + http_auth_type: AuthType::Trust, + pg_auth_type: AuthType::Trust, auth_validation_public_key_path: None, remote_storage_config: None, default_tenant_conf: TenantConf::default(), @@ -1008,7 +1026,8 @@ log_format = 'json' max_file_descriptors: 333, workdir, pg_distrib_dir, - auth_type: AuthType::Trust, + http_auth_type: AuthType::Trust, + pg_auth_type: AuthType::Trust, auth_validation_public_key_path: None, remote_storage_config: None, default_tenant_conf: TenantConf::default(), diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs index 111bc480c4..9faa994f16 100644 --- a/pageserver/src/http/routes.rs +++ b/pageserver/src/http/routes.rs @@ -82,6 +82,7 @@ fn get_config(request: &Request) -> &'static PageServerConf { get_state(request).conf } +/// Check that the requester is authorized to operate on given tenant fn check_permission(request: &Request, tenant_id: Option) -> Result<(), ApiError> { check_permission_with(request, |claims| { crate::auth::check_permission(claims, tenant_id) diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index c5e260a962..8a64be51f1 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -924,7 +924,8 @@ class NeonEnv: pg=self.port_distributor.get_port(), http=self.port_distributor.get_port(), ) - pageserver_auth_type = "NeonJWT" if config.auth_enabled else "Trust" + http_auth_type = "NeonJWT" if config.auth_enabled else "Trust" + pg_auth_type = "NeonJWT" if config.auth_enabled else "Trust" toml += textwrap.dedent( f""" @@ -932,7 +933,8 @@ class NeonEnv: id=1 listen_pg_addr = 'localhost:{pageserver_port.pg}' listen_http_addr = 'localhost:{pageserver_port.http}' - auth_type = '{pageserver_auth_type}' + pg_auth_type = '{pg_auth_type}' + http_auth_type = '{http_auth_type}' """ ) diff --git a/test_runner/regress/test_compatibility.py b/test_runner/regress/test_compatibility.py index 731e78a3e3..66625dd6f8 100644 --- a/test_runner/regress/test_compatibility.py +++ b/test_runner/regress/test_compatibility.py @@ -246,6 +246,13 @@ def prepare_snapshot( if get_neon_version(neon_binpath) == "49da498f651b9f3a53b56c7c0697636d880ddfe0": pageserver_config["broker_endpoints"] = etcd_broker_endpoints # old etcd version + # Older pageserver versions had just one `auth_type` setting. Now there + # are separate settings for pg and http ports. We don't use authentication + # in compatibility tests so just remove authentication related settings. + pageserver_config.pop("auth_type", None) + pageserver_config.pop("pg_auth_type", None) + pageserver_config.pop("http_auth_type", None) + if pg_distrib_dir: pageserver_config["pg_distrib_dir"] = str(pg_distrib_dir) From cd17802b1f20b2066546e143f48dd348ce2ae86c Mon Sep 17 00:00:00 2001 From: Arthur Petukhovsky Date: Wed, 15 Mar 2023 16:18:38 +0300 Subject: [PATCH 16/32] Add pg_clients test for serverless driver (#3827) Fixes #3819 --- test_runner/pg_clients/README.md | 10 ++++ test_runner/pg_clients/test_pg_clients.py | 1 + .../serverless-driver/.dockerignore | 1 + .../typescript/serverless-driver/.gitignore | 1 + .../typescript/serverless-driver/Dockerfile | 7 +++ .../typescript/serverless-driver/index.js | 20 ++++++++ .../serverless-driver/package-lock.json | 51 +++++++++++++++++++ .../typescript/serverless-driver/package.json | 7 +++ 8 files changed, 98 insertions(+) create mode 100644 test_runner/pg_clients/README.md create mode 100644 test_runner/pg_clients/typescript/serverless-driver/.dockerignore create mode 100644 test_runner/pg_clients/typescript/serverless-driver/.gitignore create mode 100644 test_runner/pg_clients/typescript/serverless-driver/Dockerfile create mode 100755 test_runner/pg_clients/typescript/serverless-driver/index.js create mode 100644 test_runner/pg_clients/typescript/serverless-driver/package-lock.json create mode 100644 test_runner/pg_clients/typescript/serverless-driver/package.json diff --git a/test_runner/pg_clients/README.md b/test_runner/pg_clients/README.md new file mode 100644 index 0000000000..dc316a17ef --- /dev/null +++ b/test_runner/pg_clients/README.md @@ -0,0 +1,10 @@ +# pg_clients + +To run a single test locally: + +```bash +export BENCHMARK_CONNSTR=postgres://user:pass@ep-abc-xyz-123.us-east-2.aws.neon.build/neondb + +# will filter only tests with "serverless" in the name +./scripts/pytest -m remote_cluster -k serverless +``` diff --git a/test_runner/pg_clients/test_pg_clients.py b/test_runner/pg_clients/test_pg_clients.py index f44fdd1724..18e3f379da 100644 --- a/test_runner/pg_clients/test_pg_clients.py +++ b/test_runner/pg_clients/test_pg_clients.py @@ -22,6 +22,7 @@ from fixtures.utils import subprocess_capture ), "swift/PostgresNIOExample", "typescript/postgresql-client", + "typescript/serverless-driver", ], ) def test_pg_clients(test_output_dir: Path, remote_pg: RemotePostgres, client: str): diff --git a/test_runner/pg_clients/typescript/serverless-driver/.dockerignore b/test_runner/pg_clients/typescript/serverless-driver/.dockerignore new file mode 100644 index 0000000000..c2658d7d1b --- /dev/null +++ b/test_runner/pg_clients/typescript/serverless-driver/.dockerignore @@ -0,0 +1 @@ +node_modules/ diff --git a/test_runner/pg_clients/typescript/serverless-driver/.gitignore b/test_runner/pg_clients/typescript/serverless-driver/.gitignore new file mode 100644 index 0000000000..c2658d7d1b --- /dev/null +++ b/test_runner/pg_clients/typescript/serverless-driver/.gitignore @@ -0,0 +1 @@ +node_modules/ diff --git a/test_runner/pg_clients/typescript/serverless-driver/Dockerfile b/test_runner/pg_clients/typescript/serverless-driver/Dockerfile new file mode 100644 index 0000000000..a5ad832a5c --- /dev/null +++ b/test_runner/pg_clients/typescript/serverless-driver/Dockerfile @@ -0,0 +1,7 @@ +FROM node:18 +WORKDIR /source + +COPY . . +RUN npm clean-install + +CMD ["/source/index.js"] diff --git a/test_runner/pg_clients/typescript/serverless-driver/index.js b/test_runner/pg_clients/typescript/serverless-driver/index.js new file mode 100755 index 0000000000..91db4037e1 --- /dev/null +++ b/test_runner/pg_clients/typescript/serverless-driver/index.js @@ -0,0 +1,20 @@ +#! /usr/bin/env node + +import { Client } from '@neondatabase/serverless' + +(async () => { + const client = new Client({ + host: process.env.NEON_HOST, + database: process.env.NEON_DATABASE, + user: process.env.NEON_USER, + password: process.env.NEON_PASSWORD, + }); + client.connect(); + const result = await client.query({ + text: 'select 1', + rowMode: 'array', + }); + const rows = result.rows; + await client.end(); + console.log(rows[0][0]); +})() diff --git a/test_runner/pg_clients/typescript/serverless-driver/package-lock.json b/test_runner/pg_clients/typescript/serverless-driver/package-lock.json new file mode 100644 index 0000000000..c4d03bffcb --- /dev/null +++ b/test_runner/pg_clients/typescript/serverless-driver/package-lock.json @@ -0,0 +1,51 @@ +{ + "name": "serverless-driver", + "lockfileVersion": 2, + "requires": true, + "packages": { + "": { + "dependencies": { + "@neondatabase/serverless": "^0.2.8", + "ws": "^8.13.0" + } + }, + "node_modules/@neondatabase/serverless": { + "version": "0.2.8", + "resolved": "https://registry.npmjs.org/@neondatabase/serverless/-/serverless-0.2.8.tgz", + "integrity": "sha512-+yWjIOJsFnrtt2xvtLVEzWM2lfvemawk/DBg4mD2cZOF/IC6Jn4wEctZyk60TscZMSxfozNkPoxmZvBmNuQ0vA==" + }, + "node_modules/ws": { + "version": "8.13.0", + "resolved": "https://registry.npmjs.org/ws/-/ws-8.13.0.tgz", + "integrity": "sha512-x9vcZYTrFPC7aSIbj7sRCYo7L/Xb8Iy+pW0ng0wt2vCJv7M9HOMy0UoN3rr+IFC7hb7vXoqS+P9ktyLLLhO+LA==", + "engines": { + "node": ">=10.0.0" + }, + "peerDependencies": { + "bufferutil": "^4.0.1", + "utf-8-validate": ">=5.0.2" + }, + "peerDependenciesMeta": { + "bufferutil": { + "optional": true + }, + "utf-8-validate": { + "optional": true + } + } + } + }, + "dependencies": { + "@neondatabase/serverless": { + "version": "0.2.8", + "resolved": "https://registry.npmjs.org/@neondatabase/serverless/-/serverless-0.2.8.tgz", + "integrity": "sha512-+yWjIOJsFnrtt2xvtLVEzWM2lfvemawk/DBg4mD2cZOF/IC6Jn4wEctZyk60TscZMSxfozNkPoxmZvBmNuQ0vA==" + }, + "ws": { + "version": "8.13.0", + "resolved": "https://registry.npmjs.org/ws/-/ws-8.13.0.tgz", + "integrity": "sha512-x9vcZYTrFPC7aSIbj7sRCYo7L/Xb8Iy+pW0ng0wt2vCJv7M9HOMy0UoN3rr+IFC7hb7vXoqS+P9ktyLLLhO+LA==", + "requires": {} + } + } +} diff --git a/test_runner/pg_clients/typescript/serverless-driver/package.json b/test_runner/pg_clients/typescript/serverless-driver/package.json new file mode 100644 index 0000000000..2b01a22d3e --- /dev/null +++ b/test_runner/pg_clients/typescript/serverless-driver/package.json @@ -0,0 +1,7 @@ +{ + "type": "module", + "dependencies": { + "@neondatabase/serverless": "^0.2.8", + "ws": "^8.13.0" + } +} From f1d960d2c2e88bd6e92d249b86bc0f6b87d9c3cf Mon Sep 17 00:00:00 2001 From: Rahul Patil Date: Wed, 15 Mar 2023 16:58:28 +0100 Subject: [PATCH 17/32] Add new pageserver to eu-central-1 (#3829) ## Describe your changes ## Issue ticket number and link ## Checklist before requesting a review - [x] I have performed a self-review of my code. - [ ] If it is a core feature, I have added thorough tests. - [ ] Do we need to implement analytics? if so did you add the relevant metrics to the dashboard? - [ ] If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section. --- .github/ansible/prod.eu-central-1.hosts.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/ansible/prod.eu-central-1.hosts.yaml b/.github/ansible/prod.eu-central-1.hosts.yaml index 2236dcbc06..b3cd5de01c 100644 --- a/.github/ansible/prod.eu-central-1.hosts.yaml +++ b/.github/ansible/prod.eu-central-1.hosts.yaml @@ -27,6 +27,8 @@ storage: ansible_host: i-0cd8d316ecbb715be pageserver-1.eu-central-1.aws.neon.tech: ansible_host: i-090044ed3d383fef0 + pageserver-2.eu-central-1.aws.neon.tech: + ansible_host: i-033584edf3f4b6742 safekeepers: hosts: From 768c8d9972b7064b6e50b7d421506b76b6074221 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Thu, 16 Mar 2023 11:03:21 +0200 Subject: [PATCH 18/32] test: allow gc to get unlucky (#3826) this failure case was probably introduced by b220ba6, because earlier the gc would always have run fast enough for restart every 1s. however, test got added later, so we have just been lucky. fixes #3824 by allowing this error to happen. --- .../regress/test_pageserver_restarts_under_workload.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test_runner/regress/test_pageserver_restarts_under_workload.py b/test_runner/regress/test_pageserver_restarts_under_workload.py index 28159778fe..eab8b112f0 100644 --- a/test_runner/regress/test_pageserver_restarts_under_workload.py +++ b/test_runner/regress/test_pageserver_restarts_under_workload.py @@ -17,6 +17,12 @@ def test_pageserver_restarts_under_worload(neon_simple_env: NeonEnv, pg_bin: PgB n_restarts = 10 scale = 10 + # the background task may complete the init task delay after finding an + # active tenant, but shutdown starts right before Tenant::gc_iteration + env.pageserver.allowed_errors.append( + r".*Gc failed, retrying in \S+: Cannot run GC iteration on inactive tenant" + ) + def run_pgbench(pg: Postgres): connstr = pg.connstr() log.info(f"Start a pgbench workload on pg {connstr}") From b067378d0d4f2e5ba39fd4ddf8bcedb6ef75ce98 Mon Sep 17 00:00:00 2001 From: Arthur Petukhovsky Date: Thu, 16 Mar 2023 16:24:01 +0200 Subject: [PATCH 19/32] Measure cross-AZ traffic in safekeepers (#3806) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Create `safekeeper_pg_io_bytes_total` metric to track total amount of bytes written/read in a postgres connections to safekeepers. This metric has the following labels: - `client_az` – availability zone of the connection initiator, or `"unknown"` - `sk_az` – availability zone of the safekeeper, or `"unknown"` - `app_name` – `application_name` of the postgres client - `dir` – data direction, either `"read"` or `"write"` - `same_az` – `"true"`, `"false"` or `"unknown"`. Can be derived from `client_az` and `sk_az`, exists purely for convenience. This is implemented by passing availability zone in the connection string, like this: `-c tenant_id=AAA timeline_id=BBB availability-zone=AZ-1`. Update ansible deployment scripts to add availability_zone argument to safekeeper and pageserver in systemd service files. --- .github/ansible/deploy.yaml | 18 +++ .github/ansible/systemd/pageserver.service | 2 +- .github/ansible/systemd/safekeeper.service | 2 +- control_plane/src/safekeeper.rs | 6 + pageserver/src/config.rs | 17 +++ pageserver/src/tenant/timeline.rs | 1 + .../walreceiver/connection_manager.rs | 18 ++- safekeeper/src/bin/safekeeper.rs | 4 + safekeeper/src/handler.rs | 13 +- safekeeper/src/lib.rs | 2 + safekeeper/src/metrics.rs | 131 +++++++++++++++++- safekeeper/src/wal_service.rs | 19 +-- test_runner/regress/test_tenants.py | 47 +++++++ 13 files changed, 263 insertions(+), 17 deletions(-) diff --git a/.github/ansible/deploy.yaml b/.github/ansible/deploy.yaml index 0243e91f37..d4c1dec8ea 100644 --- a/.github/ansible/deploy.yaml +++ b/.github/ansible/deploy.yaml @@ -91,6 +91,15 @@ tags: - pageserver + # used in `pageserver.service` template + - name: learn current availability_zone + shell: + cmd: "curl -s http://169.254.169.254/latest/meta-data/placement/availability-zone" + register: ec2_availability_zone + + - set_fact: + ec2_availability_zone={{ ec2_availability_zone.stdout }} + - name: upload systemd service definition ansible.builtin.template: src: systemd/pageserver.service @@ -153,6 +162,15 @@ tags: - safekeeper + # used in `safekeeper.service` template + - name: learn current availability_zone + shell: + cmd: "curl -s http://169.254.169.254/latest/meta-data/placement/availability-zone" + register: ec2_availability_zone + + - set_fact: + ec2_availability_zone={{ ec2_availability_zone.stdout }} + # in the future safekeepers should discover pageservers byself # but currently use first pageserver that was discovered - name: set first pageserver var for safekeepers diff --git a/.github/ansible/systemd/pageserver.service b/.github/ansible/systemd/pageserver.service index 4570a666fa..177fc9f9d2 100644 --- a/.github/ansible/systemd/pageserver.service +++ b/.github/ansible/systemd/pageserver.service @@ -6,7 +6,7 @@ After=network.target auditd.service Type=simple User=pageserver Environment=RUST_BACKTRACE=1 NEON_REPO_DIR=/storage/pageserver LD_LIBRARY_PATH=/usr/local/v14/lib SENTRY_DSN={{ SENTRY_URL_PAGESERVER }} SENTRY_ENVIRONMENT={{ sentry_environment }} -ExecStart=/usr/local/bin/pageserver -c "pg_distrib_dir='/usr/local'" -c "listen_pg_addr='0.0.0.0:6400'" -c "listen_http_addr='0.0.0.0:9898'" -c "broker_endpoint='{{ broker_endpoint }}'" -D /storage/pageserver/data +ExecStart=/usr/local/bin/pageserver -c "pg_distrib_dir='/usr/local'" -c "listen_pg_addr='0.0.0.0:6400'" -c "listen_http_addr='0.0.0.0:9898'" -c "broker_endpoint='{{ broker_endpoint }}'" -c "availability_zone='{{ ec2_availability_zone }}'" -D /storage/pageserver/data ExecReload=/bin/kill -HUP $MAINPID KillMode=mixed KillSignal=SIGINT diff --git a/.github/ansible/systemd/safekeeper.service b/.github/ansible/systemd/safekeeper.service index d7d8d26b1a..a8ef4bc0f5 100644 --- a/.github/ansible/systemd/safekeeper.service +++ b/.github/ansible/systemd/safekeeper.service @@ -6,7 +6,7 @@ After=network.target auditd.service Type=simple User=safekeeper Environment=RUST_BACKTRACE=1 NEON_REPO_DIR=/storage/safekeeper/data LD_LIBRARY_PATH=/usr/local/v14/lib SENTRY_DSN={{ SENTRY_URL_SAFEKEEPER }} SENTRY_ENVIRONMENT={{ sentry_environment }} -ExecStart=/usr/local/bin/safekeeper -l {{ inventory_hostname }}{{ hostname_suffix }}:6500 --listen-http {{ inventory_hostname }}{{ hostname_suffix }}:7676 -D /storage/safekeeper/data --broker-endpoint={{ broker_endpoint }} --remote-storage='{bucket_name="{{bucket_name}}", bucket_region="{{bucket_region}}", prefix_in_bucket="{{ safekeeper_s3_prefix }}"}' +ExecStart=/usr/local/bin/safekeeper -l {{ inventory_hostname }}{{ hostname_suffix }}:6500 --listen-http {{ inventory_hostname }}{{ hostname_suffix }}:7676 -D /storage/safekeeper/data --broker-endpoint={{ broker_endpoint }} --remote-storage='{bucket_name="{{bucket_name}}", bucket_region="{{bucket_region}}", prefix_in_bucket="{{ safekeeper_s3_prefix }}"}' --availability-zone={{ ec2_availability_zone }} ExecReload=/bin/kill -HUP $MAINPID KillMode=mixed KillSignal=SIGINT diff --git a/control_plane/src/safekeeper.rs b/control_plane/src/safekeeper.rs index 4c0812a5e3..228968be80 100644 --- a/control_plane/src/safekeeper.rs +++ b/control_plane/src/safekeeper.rs @@ -115,6 +115,10 @@ impl SafekeeperNode { let datadir = self.datadir_path(); let id_string = id.to_string(); + // TODO: add availability_zone to the config. + // Right now we just specify any value here and use it to check metrics in tests. + let availability_zone = format!("sk-{}", id_string); + let mut args = vec![ "-D", datadir.to_str().with_context(|| { @@ -126,6 +130,8 @@ impl SafekeeperNode { &listen_pg, "--listen-http", &listen_http, + "--availability-zone", + &availability_zone, ]; if !self.conf.sync { args.push("--no-sync"); diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs index d17f0bc143..16f35355af 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -118,6 +118,9 @@ pub struct PageServerConf { /// Example (default): 127.0.0.1:9898 pub listen_http_addr: String, + /// Current availability zone. Used for traffic metrics. + pub availability_zone: Option, + // Timeout when waiting for WAL receiver to catch up to an LSN given in a GetPage@LSN call. pub wait_lsn_timeout: Duration, // How long to wait for WAL redo to complete. @@ -202,6 +205,8 @@ struct PageServerConfigBuilder { listen_http_addr: BuilderValue, + availability_zone: BuilderValue>, + wait_lsn_timeout: BuilderValue, wal_redo_timeout: BuilderValue, @@ -247,6 +252,7 @@ impl Default for PageServerConfigBuilder { Self { listen_pg_addr: Set(DEFAULT_PG_LISTEN_ADDR.to_string()), listen_http_addr: Set(DEFAULT_HTTP_LISTEN_ADDR.to_string()), + availability_zone: Set(None), wait_lsn_timeout: Set(humantime::parse_duration(DEFAULT_WAIT_LSN_TIMEOUT) .expect("cannot parse default wait lsn timeout")), wal_redo_timeout: Set(humantime::parse_duration(DEFAULT_WAL_REDO_TIMEOUT) @@ -303,6 +309,10 @@ impl PageServerConfigBuilder { self.listen_http_addr = BuilderValue::Set(listen_http_addr) } + pub fn availability_zone(&mut self, availability_zone: Option) { + self.availability_zone = BuilderValue::Set(availability_zone) + } + pub fn wait_lsn_timeout(&mut self, wait_lsn_timeout: Duration) { self.wait_lsn_timeout = BuilderValue::Set(wait_lsn_timeout) } @@ -414,6 +424,9 @@ impl PageServerConfigBuilder { listen_http_addr: self .listen_http_addr .ok_or(anyhow!("missing listen_http_addr"))?, + availability_zone: self + .availability_zone + .ok_or(anyhow!("missing availability_zone"))?, wait_lsn_timeout: self .wait_lsn_timeout .ok_or(anyhow!("missing wait_lsn_timeout"))?, @@ -614,6 +627,7 @@ impl PageServerConf { match key { "listen_pg_addr" => builder.listen_pg_addr(parse_toml_string(key, item)?), "listen_http_addr" => builder.listen_http_addr(parse_toml_string(key, item)?), + "availability_zone" => builder.availability_zone(Some(parse_toml_string(key, item)?)), "wait_lsn_timeout" => builder.wait_lsn_timeout(parse_toml_duration(key, item)?), "wal_redo_timeout" => builder.wal_redo_timeout(parse_toml_duration(key, item)?), "initial_superuser_name" => builder.superuser(parse_toml_string(key, item)?), @@ -779,6 +793,7 @@ impl PageServerConf { max_file_descriptors: defaults::DEFAULT_MAX_FILE_DESCRIPTORS, listen_pg_addr: defaults::DEFAULT_PG_LISTEN_ADDR.to_string(), listen_http_addr: defaults::DEFAULT_HTTP_LISTEN_ADDR.to_string(), + availability_zone: None, superuser: "cloud_admin".to_string(), workdir: repo_dir, pg_distrib_dir, @@ -961,6 +976,7 @@ log_format = 'json' id: NodeId(10), listen_pg_addr: defaults::DEFAULT_PG_LISTEN_ADDR.to_string(), listen_http_addr: defaults::DEFAULT_HTTP_LISTEN_ADDR.to_string(), + availability_zone: None, wait_lsn_timeout: humantime::parse_duration(defaults::DEFAULT_WAIT_LSN_TIMEOUT)?, wal_redo_timeout: humantime::parse_duration(defaults::DEFAULT_WAL_REDO_TIMEOUT)?, superuser: defaults::DEFAULT_SUPERUSER.to_string(), @@ -1019,6 +1035,7 @@ log_format = 'json' id: NodeId(10), listen_pg_addr: "127.0.0.1:64000".to_string(), listen_http_addr: "127.0.0.1:9898".to_string(), + availability_zone: None, wait_lsn_timeout: Duration::from_secs(111), wal_redo_timeout: Duration::from_secs(111), superuser: "zzzz".to_string(), diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index fff288c683..0c42ed3079 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -1335,6 +1335,7 @@ impl Timeline { lagging_wal_timeout, max_lsn_wal_lag, crate::config::SAFEKEEPER_AUTH_TOKEN.get().cloned(), + self.conf.availability_zone.clone(), background_ctx, ); } diff --git a/pageserver/src/tenant/timeline/walreceiver/connection_manager.rs b/pageserver/src/tenant/timeline/walreceiver/connection_manager.rs index 64a79b6d1b..0c770136db 100644 --- a/pageserver/src/tenant/timeline/walreceiver/connection_manager.rs +++ b/pageserver/src/tenant/timeline/walreceiver/connection_manager.rs @@ -45,6 +45,7 @@ pub fn spawn_connection_manager_task( lagging_wal_timeout: Duration, max_lsn_wal_lag: NonZeroU64, auth_token: Option>, + availability_zone: Option, ctx: RequestContext, ) { let mut broker_client = get_broker_client().clone(); @@ -67,6 +68,7 @@ pub fn spawn_connection_manager_task( lagging_wal_timeout, max_lsn_wal_lag, auth_token, + availability_zone, ); loop { select! { @@ -334,6 +336,7 @@ struct WalreceiverState { /// Data about all timelines, available for connection, fetched from storage broker, grouped by their corresponding safekeeper node id. wal_stream_candidates: HashMap, auth_token: Option>, + availability_zone: Option, } /// Current connection data. @@ -381,6 +384,7 @@ impl WalreceiverState { lagging_wal_timeout: Duration, max_lsn_wal_lag: NonZeroU64, auth_token: Option>, + availability_zone: Option, ) -> Self { let id = TenantTimelineId { tenant_id: timeline.tenant_id, @@ -396,6 +400,7 @@ impl WalreceiverState { wal_stream_candidates: HashMap::new(), wal_connection_retries: HashMap::new(), auth_token, + availability_zone, } } @@ -740,6 +745,7 @@ impl WalreceiverState { None => None, Some(x) => Some(x), }, + self.availability_zone.as_deref(), ) { Ok(connstr) => Some((*sk_id, info, connstr)), Err(e) => { @@ -824,17 +830,24 @@ fn wal_stream_connection_config( }: TenantTimelineId, listen_pg_addr_str: &str, auth_token: Option<&str>, + availability_zone: Option<&str>, ) -> anyhow::Result { let (host, port) = parse_host_port(listen_pg_addr_str).context("Unable to parse listen_pg_addr_str")?; let port = port.unwrap_or(5432); - Ok(PgConnectionConfig::new_host_port(host, port) + let mut connstr = PgConnectionConfig::new_host_port(host, port) .extend_options([ "-c".to_owned(), format!("timeline_id={}", timeline_id), format!("tenant_id={}", tenant_id), ]) - .set_password(auth_token.map(|s| s.to_owned()))) + .set_password(auth_token.map(|s| s.to_owned())); + + if let Some(availability_zone) = availability_zone { + connstr = connstr.extend_options([format!("availability_zone={}", availability_zone)]); + } + + Ok(connstr) } #[cfg(test)] @@ -1273,6 +1286,7 @@ mod tests { wal_stream_candidates: HashMap::new(), wal_connection_retries: HashMap::new(), auth_token: None, + availability_zone: None, } } } diff --git a/safekeeper/src/bin/safekeeper.rs b/safekeeper/src/bin/safekeeper.rs index d2cb9f79b9..848b955af8 100644 --- a/safekeeper/src/bin/safekeeper.rs +++ b/safekeeper/src/bin/safekeeper.rs @@ -71,6 +71,9 @@ struct Args { /// Listen http endpoint for management and metrics in the form host:port. #[arg(long, default_value = DEFAULT_HTTP_LISTEN_ADDR)] listen_http: String, + /// Availability zone of the safekeeper. + #[arg(long)] + availability_zone: Option, /// Do not wait for changes to be written safely to disk. Unsafe. #[arg(short, long)] no_sync: bool, @@ -166,6 +169,7 @@ fn main() -> anyhow::Result<()> { my_id: id, listen_pg_addr: args.listen_pg, listen_http_addr: args.listen_http, + availability_zone: args.availability_zone, no_sync: args.no_sync, broker_endpoint: args.broker_endpoint, broker_keepalive_interval: args.broker_keepalive_interval, diff --git a/safekeeper/src/handler.rs b/safekeeper/src/handler.rs index 3b8434b2de..a589fe1869 100644 --- a/safekeeper/src/handler.rs +++ b/safekeeper/src/handler.rs @@ -9,6 +9,7 @@ use tracing::{info, info_span, Instrument}; use crate::auth::check_permission; use crate::json_ctrl::{handle_json_ctrl, AppendLogicalMessage}; +use crate::metrics::TrafficMetrics; use crate::wal_service::ConnectionId; use crate::{GlobalTimelines, SafeKeeperConf}; use postgres_backend::QueryError; @@ -33,6 +34,7 @@ pub struct SafekeeperPostgresHandler { /// Unique connection id is logged in spans for observability. pub conn_id: ConnectionId, claims: Option, + io_metrics: Option, } /// Parsed Postgres command. @@ -94,6 +96,11 @@ impl postgres_backend::Handler format!("Failed to parse {value} as timeline id") })?); } + Some(("availability_zone", client_az)) => { + if let Some(metrics) = self.io_metrics.as_ref() { + metrics.set_client_az(client_az) + } + } _ => continue, } } @@ -101,6 +108,9 @@ impl postgres_backend::Handler if let Some(app_name) = params.get("application_name") { self.appname = Some(app_name.to_owned()); + if let Some(metrics) = self.io_metrics.as_ref() { + metrics.set_app_name(app_name) + } } Ok(()) @@ -187,7 +197,7 @@ impl postgres_backend::Handler } impl SafekeeperPostgresHandler { - pub fn new(conf: SafeKeeperConf, conn_id: u32) -> Self { + pub fn new(conf: SafeKeeperConf, conn_id: u32, io_metrics: Option) -> Self { SafekeeperPostgresHandler { conf, appname: None, @@ -196,6 +206,7 @@ impl SafekeeperPostgresHandler { ttid: TenantTimelineId::empty(), conn_id, claims: None, + io_metrics, } } diff --git a/safekeeper/src/lib.rs b/safekeeper/src/lib.rs index f4e753cdbf..2c28c5218d 100644 --- a/safekeeper/src/lib.rs +++ b/safekeeper/src/lib.rs @@ -52,6 +52,7 @@ pub struct SafeKeeperConf { pub my_id: NodeId, pub listen_pg_addr: String, pub listen_http_addr: String, + pub availability_zone: Option, pub no_sync: bool, pub broker_endpoint: Uri, pub broker_keepalive_interval: Duration, @@ -82,6 +83,7 @@ impl SafeKeeperConf { no_sync: false, listen_pg_addr: defaults::DEFAULT_PG_LISTEN_ADDR.to_string(), listen_http_addr: defaults::DEFAULT_HTTP_LISTEN_ADDR.to_string(), + availability_zone: None, remote_storage: None, my_id: NodeId(0), broker_endpoint: storage_broker::DEFAULT_ENDPOINT diff --git a/safekeeper/src/metrics.rs b/safekeeper/src/metrics.rs index 16aca24927..c3077b6dc5 100644 --- a/safekeeper/src/metrics.rs +++ b/safekeeper/src/metrics.rs @@ -1,15 +1,19 @@ //! Global safekeeper mertics and per-timeline safekeeper metrics. -use std::time::{Instant, SystemTime}; +use std::{ + sync::{Arc, RwLock}, + time::{Instant, SystemTime}, +}; use ::metrics::{register_histogram, GaugeVec, Histogram, IntGauge, DISK_WRITE_SECONDS_BUCKETS}; use anyhow::Result; use metrics::{ - core::{AtomicU64, Collector, Desc, GenericGaugeVec, Opts}, + core::{AtomicU64, Collector, Desc, GenericCounter, GenericGaugeVec, Opts}, proto::MetricFamily, register_int_counter_vec, Gauge, IntCounterVec, IntGaugeVec, }; use once_cell::sync::Lazy; + use postgres_ffi::XLogSegNo; use utils::{id::TenantTimelineId, lsn::Lsn}; @@ -63,13 +67,132 @@ pub static PERSIST_CONTROL_FILE_SECONDS: Lazy = Lazy::new(|| { }); pub static PG_IO_BYTES: Lazy = Lazy::new(|| { register_int_counter_vec!( - "safekeeper_pg_io_bytes", + "safekeeper_pg_io_bytes_total", "Bytes read from or written to any PostgreSQL connection", - &["direction"] + &["client_az", "sk_az", "app_name", "dir", "same_az"] ) .expect("Failed to register safekeeper_pg_io_bytes gauge") }); +pub const LABEL_UNKNOWN: &str = "unknown"; + +/// Labels for traffic metrics. +#[derive(Clone)] +struct ConnectionLabels { + /// Availability zone of the connection origin. + client_az: String, + /// Availability zone of the current safekeeper. + sk_az: String, + /// Client application name. + app_name: String, +} + +impl ConnectionLabels { + fn new() -> Self { + Self { + client_az: LABEL_UNKNOWN.to_string(), + sk_az: LABEL_UNKNOWN.to_string(), + app_name: LABEL_UNKNOWN.to_string(), + } + } + + fn build_metrics( + &self, + ) -> ( + GenericCounter, + GenericCounter, + ) { + let same_az = match (self.client_az.as_str(), self.sk_az.as_str()) { + (LABEL_UNKNOWN, _) | (_, LABEL_UNKNOWN) => LABEL_UNKNOWN, + (client_az, sk_az) => { + if client_az == sk_az { + "true" + } else { + "false" + } + } + }; + + let read = PG_IO_BYTES.with_label_values(&[ + &self.client_az, + &self.sk_az, + &self.app_name, + "read", + same_az, + ]); + let write = PG_IO_BYTES.with_label_values(&[ + &self.client_az, + &self.sk_az, + &self.app_name, + "write", + same_az, + ]); + (read, write) + } +} + +struct TrafficMetricsState { + /// Labels for traffic metrics. + labels: ConnectionLabels, + /// Total bytes read from this connection. + read: GenericCounter, + /// Total bytes written to this connection. + write: GenericCounter, +} + +/// Metrics for measuring traffic (r/w bytes) in a single PostgreSQL connection. +#[derive(Clone)] +pub struct TrafficMetrics { + state: Arc>, +} + +impl Default for TrafficMetrics { + fn default() -> Self { + Self::new() + } +} + +impl TrafficMetrics { + pub fn new() -> Self { + let labels = ConnectionLabels::new(); + let (read, write) = labels.build_metrics(); + let state = TrafficMetricsState { + labels, + read, + write, + }; + Self { + state: Arc::new(RwLock::new(state)), + } + } + + pub fn set_client_az(&self, value: &str) { + let mut state = self.state.write().unwrap(); + state.labels.client_az = value.to_string(); + (state.read, state.write) = state.labels.build_metrics(); + } + + pub fn set_sk_az(&self, value: &str) { + let mut state = self.state.write().unwrap(); + state.labels.sk_az = value.to_string(); + (state.read, state.write) = state.labels.build_metrics(); + } + + pub fn set_app_name(&self, value: &str) { + let mut state = self.state.write().unwrap(); + state.labels.app_name = value.to_string(); + (state.read, state.write) = state.labels.build_metrics(); + } + + pub fn observe_read(&self, cnt: usize) { + self.state.read().unwrap().read.inc_by(cnt as u64) + } + + pub fn observe_write(&self, cnt: usize) { + self.state.read().unwrap().write.inc_by(cnt as u64) + } +} + /// Metrics for WalStorage in a single timeline. #[derive(Clone, Default)] pub struct WalStorageMetrics { diff --git a/safekeeper/src/wal_service.rs b/safekeeper/src/wal_service.rs index 5f58c4f7fc..22f50c3428 100644 --- a/safekeeper/src/wal_service.rs +++ b/safekeeper/src/wal_service.rs @@ -9,8 +9,9 @@ use tokio::net::TcpStream; use tracing::*; use utils::measured_stream::MeasuredStream; +use crate::handler::SafekeeperPostgresHandler; +use crate::metrics::TrafficMetrics; use crate::SafeKeeperConf; -use crate::{handler::SafekeeperPostgresHandler, metrics::PG_IO_BYTES}; use postgres_backend::{AuthType, PostgresBackend}; /// Accept incoming TCP connections and spawn them into a background thread. @@ -68,20 +69,21 @@ fn handle_socket( .build()?; let local = tokio::task::LocalSet::new(); - let read_metrics = PG_IO_BYTES.with_label_values(&["read"]); - let write_metrics = PG_IO_BYTES.with_label_values(&["write"]); - socket.set_nodelay(true)?; let peer_addr = socket.peer_addr()?; - // TODO: measure cross-az traffic + let traffic_metrics = TrafficMetrics::new(); + if let Some(current_az) = conf.availability_zone.as_deref() { + traffic_metrics.set_sk_az(current_az); + } + let socket = MeasuredStream::new( socket, |cnt| { - read_metrics.inc_by(cnt as u64); + traffic_metrics.observe_read(cnt); }, |cnt| { - write_metrics.inc_by(cnt as u64); + traffic_metrics.observe_write(cnt); }, ); @@ -89,7 +91,8 @@ fn handle_socket( None => AuthType::Trust, Some(_) => AuthType::NeonJWT, }; - let mut conn_handler = SafekeeperPostgresHandler::new(conf, conn_id); + let mut conn_handler = + SafekeeperPostgresHandler::new(conf, conn_id, Some(traffic_metrics.clone())); let pgbackend = PostgresBackend::new_from_io(socket, peer_addr, auth_type, None)?; // libpq protocol between safekeeper and walproposer / pageserver // We don't use shutdown. diff --git a/test_runner/regress/test_tenants.py b/test_runner/regress/test_tenants.py index bf87cb3ad4..8021bf9914 100644 --- a/test_runner/regress/test_tenants.py +++ b/test_runner/regress/test_tenants.py @@ -3,6 +3,7 @@ import shutil import time from contextlib import closing from datetime import datetime +from itertools import chain from pathlib import Path from typing import List @@ -87,6 +88,7 @@ def test_tenants_normal_work(neon_env_builder: NeonEnvBuilder): def test_metrics_normal_work(neon_env_builder: NeonEnvBuilder): neon_env_builder.num_safekeepers = 3 + neon_env_builder.pageserver_config_override = "availability_zone='test_ps_az'" env = neon_env_builder.init_start() tenant_1, _ = env.neon_cli.create_tenant() @@ -122,6 +124,17 @@ def test_metrics_normal_work(neon_env_builder: NeonEnvBuilder): ps_metrics = all_metrics[0] sk_metrics = all_metrics[1:] + # Find all metrics among all safekeepers, accepts the same arguments as query_all() + def query_all_safekeepers(name, filter): + return list( + chain.from_iterable( + map( + lambda sk: sk.query_all(name, filter), + sk_metrics, + ) + ) + ) + ttids = [ {"tenant_id": str(tenant_1), "timeline_id": str(timeline_1)}, {"tenant_id": str(tenant_2), "timeline_id": str(timeline_2)}, @@ -162,6 +175,40 @@ def test_metrics_normal_work(neon_env_builder: NeonEnvBuilder): f"process_start_time_seconds (UTC): {datetime.fromtimestamp(metrics.query_one('process_start_time_seconds').value)}" ) + for io_direction in ["read", "write"]: + # Querying all metrics for number of bytes read/written by pageserver in another AZ + io_metrics = query_all_safekeepers( + "safekeeper_pg_io_bytes_total", + { + "app_name": "pageserver", + "client_az": "test_ps_az", + "dir": io_direction, + "same_az": "false", + }, + ) + total_bytes = sum(int(metric.value) for metric in io_metrics) + log.info(f"Pageserver {io_direction} bytes from another AZ: {total_bytes}") + # We expect some bytes to be read/written, to make sure metrics are working + assert total_bytes > 0 + + # Test (a subset of) safekeeper global metrics + for sk_m in sk_metrics: + # Test that every safekeeper has read some bytes + assert any( + map( + lambda x: x.value > 0, + sk_m.query_all("safekeeper_pg_io_bytes_total", {"dir": "read"}), + ) + ), f"{sk_m.name} has not read bytes" + + # Test that every safekeeper has written some bytes + assert any( + map( + lambda x: x.value > 0, + sk_m.query_all("safekeeper_pg_io_bytes_total", {"dir": "write"}), + ) + ), f"{sk_m.name} has not written bytes" + # Test (a subset of) pageserver global metrics for metric in PAGESERVER_GLOBAL_METRICS: ps_samples = ps_metrics.query_all(metric, {}) From b917270c676142e1017ddae89e981b60cf072ed7 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Thu, 16 Mar 2023 15:47:37 +0100 Subject: [PATCH 20/32] remove unused TenantConfig::update function --- pageserver/src/tenant/config.rs | 42 --------------------------------- 1 file changed, 42 deletions(-) diff --git a/pageserver/src/tenant/config.rs b/pageserver/src/tenant/config.rs index 1a52b26ae7..48cb6be121 100644 --- a/pageserver/src/tenant/config.rs +++ b/pageserver/src/tenant/config.rs @@ -222,48 +222,6 @@ impl TenantConfOpt { eviction_policy: self.eviction_policy.unwrap_or(global_conf.eviction_policy), } } - - pub fn update(&mut self, other: &TenantConfOpt) { - if let Some(checkpoint_distance) = other.checkpoint_distance { - self.checkpoint_distance = Some(checkpoint_distance); - } - if let Some(checkpoint_timeout) = other.checkpoint_timeout { - self.checkpoint_timeout = Some(checkpoint_timeout); - } - if let Some(compaction_target_size) = other.compaction_target_size { - self.compaction_target_size = Some(compaction_target_size); - } - if let Some(compaction_period) = other.compaction_period { - self.compaction_period = Some(compaction_period); - } - if let Some(compaction_threshold) = other.compaction_threshold { - self.compaction_threshold = Some(compaction_threshold); - } - if let Some(gc_horizon) = other.gc_horizon { - self.gc_horizon = Some(gc_horizon); - } - if let Some(gc_period) = other.gc_period { - self.gc_period = Some(gc_period); - } - if let Some(image_creation_threshold) = other.image_creation_threshold { - self.image_creation_threshold = Some(image_creation_threshold); - } - if let Some(pitr_interval) = other.pitr_interval { - self.pitr_interval = Some(pitr_interval); - } - if let Some(walreceiver_connect_timeout) = other.walreceiver_connect_timeout { - self.walreceiver_connect_timeout = Some(walreceiver_connect_timeout); - } - if let Some(lagging_wal_timeout) = other.lagging_wal_timeout { - self.lagging_wal_timeout = Some(lagging_wal_timeout); - } - if let Some(max_lsn_wal_lag) = other.max_lsn_wal_lag { - self.max_lsn_wal_lag = Some(max_lsn_wal_lag); - } - if let Some(trace_read_requests) = other.trace_read_requests { - self.trace_read_requests = Some(trace_read_requests); - } - } } impl Default for TenantConf { From f6e2e0042d66b87b09b8e03a1ab9026eaf545b12 Mon Sep 17 00:00:00 2001 From: sharnoff Date: Thu, 16 Mar 2023 17:09:45 -0700 Subject: [PATCH 21/32] Fix + re-enable VM cgroup creation + running (#3820) Re-enable cgroup shenanigans in VMs, with some special care taken to make sure that our version of cgroup-tools supports cgroup v2 (debian bullseye does not, and probably won't because it requires a breaking change in libcgroup). This involves manually building libcgroup / cgroup-tools from source, then copying the output into the final build stage. We originally considered pulling the package from debian's testing repo (which is up-to-date), but decided against it. Refer to the PR for more details. Prior work, for reference: * 2153d2e0 - Run compute_ctl in a cgroup in VMs * 1360361f - Fix missing VM cgconfig.conf * 8dae8799 - Disable VM cgroup shenanigans --- Dockerfile.vm-compute-node | 61 ++++++++++++++++++++++++++++++++------ vm-cgconfig.conf | 12 ++++++++ 2 files changed, 64 insertions(+), 9 deletions(-) create mode 100644 vm-cgconfig.conf diff --git a/Dockerfile.vm-compute-node b/Dockerfile.vm-compute-node index faea311708..957166ecd1 100644 --- a/Dockerfile.vm-compute-node +++ b/Dockerfile.vm-compute-node @@ -2,26 +2,69 @@ ARG SRC_IMAGE ARG VM_INFORMANT_VERSION=v0.1.14 +# on libcgroup update, make sure to check bootstrap.sh for changes +ARG LIBCGROUP_VERSION=v2.0.3 -# Pull VM informant and set up inittab +# Pull VM informant, to copy from later FROM neondatabase/vm-informant:$VM_INFORMANT_VERSION as informant +# Build cgroup-tools +# +# At time of writing (2023-03-14), debian bullseye has a version of cgroup-tools (technically +# libcgroup) that doesn't support cgroup v2 (version 0.41-11). Unfortunately, the vm-informant +# requires cgroup v2, so we'll build cgroup-tools ourselves. +FROM debian:bullseye-slim as libcgroup-builder +ARG LIBCGROUP_VERSION + +RUN set -exu \ + && apt update \ + && apt install --no-install-recommends -y \ + git \ + ca-certificates \ + automake \ + cmake \ + make \ + gcc \ + byacc \ + flex \ + libtool \ + libpam0g-dev \ + && git clone --depth 1 -b $LIBCGROUP_VERSION https://github.com/libcgroup/libcgroup \ + && INSTALL_DIR="/libcgroup-install" \ + && mkdir -p "$INSTALL_DIR/bin" "$INSTALL_DIR/include" \ + && cd libcgroup \ + # extracted from bootstrap.sh, with modified flags: + && (test -d m4 || mkdir m4) \ + && autoreconf -fi \ + && rm -rf autom4te.cache \ + && CFLAGS="-O3" ./configure --prefix="$INSTALL_DIR" --sysconfdir=/etc --localstatedir=/var --enable-opaque-hierarchy="name=systemd" \ + # actually build the thing... + && make install + +# Combine, starting from non-VM compute node image. +FROM $SRC_IMAGE as base + +# Temporarily set user back to root so we can run adduser, set inittab +USER root +RUN adduser vm-informant --disabled-password --no-create-home + RUN set -e \ && rm -f /etc/inittab \ && touch /etc/inittab RUN set -e \ + && echo "::sysinit:cgconfigparser -l /etc/cgconfig.conf -s 1664" >> /etc/inittab \ && CONNSTR="dbname=neondb user=cloud_admin sslmode=disable" \ - && ARGS="--auto-restart --pgconnstr=\"$CONNSTR\"" \ + && ARGS="--auto-restart --cgroup=neon-postgres --pgconnstr=\"$CONNSTR\"" \ && echo "::respawn:su vm-informant -c '/usr/local/bin/vm-informant $ARGS'" >> /etc/inittab -# Combine, starting from non-VM compute node image. -FROM $SRC_IMAGE as base - -# Temporarily set user back to root so we can run adduser -USER root -RUN adduser vm-informant --disabled-password --no-create-home USER postgres -COPY --from=informant /etc/inittab /etc/inittab +ADD vm-cgconfig.conf /etc/cgconfig.conf COPY --from=informant /usr/bin/vm-informant /usr/local/bin/vm-informant + +COPY --from=libcgroup-builder /libcgroup-install/bin/* /usr/bin/ +COPY --from=libcgroup-builder /libcgroup-install/lib/* /usr/lib/ +COPY --from=libcgroup-builder /libcgroup-install/sbin/* /usr/sbin/ + +ENTRYPOINT ["/usr/sbin/cgexec", "-g", "*:neon-postgres", "/usr/local/bin/compute_ctl"] diff --git a/vm-cgconfig.conf b/vm-cgconfig.conf new file mode 100644 index 0000000000..a2e201708e --- /dev/null +++ b/vm-cgconfig.conf @@ -0,0 +1,12 @@ +# Configuration for cgroups in VM compute nodes +group neon-postgres { + perm { + admin { + uid = vm-informant; + } + task { + gid = users; + } + } + memory {} +} From 93f3f4ab5fb589296ea44929f5c113f680777ee6 Mon Sep 17 00:00:00 2001 From: Shany Pozin Date: Sun, 19 Mar 2023 10:44:42 +0200 Subject: [PATCH 22/32] Return NotFound in mgmt API requests when tenant is not present in the pageserver (#3818) ## Describe your changes Add Error enum for tenant state response to allow better error handling in mgmt api ## Issue ticket number and link #2238 ## Checklist before requesting a review - [x] I have performed a self-review of my code. - [ ] If it is a core feature, I have added thorough tests. - [ ] Do we need to implement analytics? if so did you add the relevant metrics to the dashboard? - [ ] If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section. --- pageserver/src/http/routes.rs | 116 ++++++++---------- pageserver/src/page_service.rs | 5 +- pageserver/src/tenant/mgr.rs | 64 ++++++---- test_runner/fixtures/neon_fixtures.py | 4 +- test_runner/regress/test_tenant_detach.py | 12 +- test_runner/regress/test_timeline_delete.py | 4 +- .../test_walredo_not_left_behind_on_detach.py | 4 +- 7 files changed, 104 insertions(+), 105 deletions(-) diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs index 9faa994f16..2f03e251fd 100644 --- a/pageserver/src/http/routes.rs +++ b/pageserver/src/http/routes.rs @@ -21,7 +21,7 @@ use crate::context::{DownloadBehavior, RequestContext}; use crate::pgdatadir_mapping::LsnForTimestamp; use crate::task_mgr::TaskKind; use crate::tenant::config::TenantConfOpt; -use crate::tenant::mgr::TenantMapInsertError; +use crate::tenant::mgr::{TenantMapInsertError, TenantStateError}; use crate::tenant::size::ModelInputs; use crate::tenant::storage_layer::LayerAccessStatsReset; use crate::tenant::{PageReconstructError, Timeline}; @@ -89,32 +89,45 @@ fn check_permission(request: &Request, tenant_id: Option) -> Res }) } -fn apierror_from_prerror(err: PageReconstructError) -> ApiError { - match err { - PageReconstructError::Other(err) => ApiError::InternalServerError(err), - PageReconstructError::NeedsDownload(_, _) => { - // This shouldn't happen, because we use a RequestContext that requests to - // download any missing layer files on-demand. - ApiError::InternalServerError(anyhow::anyhow!("need to download remote layer file")) - } - PageReconstructError::Cancelled => { - ApiError::InternalServerError(anyhow::anyhow!("request was cancelled")) - } - PageReconstructError::WalRedo(err) => { - ApiError::InternalServerError(anyhow::Error::new(err)) +impl From for ApiError { + fn from(pre: PageReconstructError) -> ApiError { + match pre { + PageReconstructError::Other(pre) => ApiError::InternalServerError(pre), + PageReconstructError::NeedsDownload(_, _) => { + // This shouldn't happen, because we use a RequestContext that requests to + // download any missing layer files on-demand. + ApiError::InternalServerError(anyhow::anyhow!("need to download remote layer file")) + } + PageReconstructError::Cancelled => { + ApiError::InternalServerError(anyhow::anyhow!("request was cancelled")) + } + PageReconstructError::WalRedo(pre) => { + ApiError::InternalServerError(anyhow::Error::new(pre)) + } } } } -fn apierror_from_tenant_map_insert_error(e: TenantMapInsertError) -> ApiError { - match e { - TenantMapInsertError::StillInitializing | TenantMapInsertError::ShuttingDown => { - ApiError::InternalServerError(anyhow::Error::new(e)) +impl From for ApiError { + fn from(tmie: TenantMapInsertError) -> ApiError { + match tmie { + TenantMapInsertError::StillInitializing | TenantMapInsertError::ShuttingDown => { + ApiError::InternalServerError(anyhow::Error::new(tmie)) + } + TenantMapInsertError::TenantAlreadyExists(id, state) => { + ApiError::Conflict(format!("tenant {id} already exists, state: {state:?}")) + } + TenantMapInsertError::Closure(e) => ApiError::InternalServerError(e), } - TenantMapInsertError::TenantAlreadyExists(id, state) => { - ApiError::Conflict(format!("tenant {id} already exists, state: {state:?}")) + } +} + +impl From for ApiError { + fn from(tse: TenantStateError) -> ApiError { + match tse { + TenantStateError::NotFound(tid) => ApiError::NotFound(anyhow!("tenant {}", tid)), + _ => ApiError::InternalServerError(anyhow::Error::new(tse)), } - TenantMapInsertError::Closure(e) => ApiError::InternalServerError(e), } } @@ -218,9 +231,7 @@ async fn timeline_create_handler(mut request: Request) -> Result) -> Result, let ctx = RequestContext::new(TaskKind::MgmtRequest, DownloadBehavior::Download); let response_data = async { - let tenant = mgr::get_tenant(tenant_id, true) - .await - .map_err(ApiError::NotFound)?; + let tenant = mgr::get_tenant(tenant_id, true).await?; let timelines = tenant.list_timelines(); let mut response_data = Vec::with_capacity(timelines.len()); @@ -268,7 +277,7 @@ async fn timeline_list_handler(request: Request) -> Result, response_data.push(timeline_info); } - Ok(response_data) + Ok::, ApiError>(response_data) } .instrument(info_span!("timeline_list", tenant = %tenant_id)) .await?; @@ -287,9 +296,7 @@ async fn timeline_detail_handler(request: Request) -> Result) -> Result format!("{lsn}"), @@ -353,8 +357,7 @@ async fn tenant_attach_handler(request: Request) -> Result, if let Some(remote_storage) = &state.remote_storage { mgr::attach_tenant(state.conf, tenant_id, remote_storage.clone(), &ctx) .instrument(info_span!("tenant_attach", tenant = %tenant_id)) - .await - .map_err(apierror_from_tenant_map_insert_error)?; + .await?; } else { return Err(ApiError::BadRequest(anyhow!( "attach_tenant is not possible because pageserver was configured without remote storage" @@ -373,11 +376,7 @@ async fn timeline_delete_handler(request: Request) -> Result) -> Result, let conf = state.conf; mgr::detach_tenant(conf, tenant_id) .instrument(info_span!("tenant_detach", tenant = %tenant_id)) - .await - // FIXME: Errors from `detach_tenant` can be caused by both both user and internal errors. - // Replace this with better handling once the error type permits it. - .map_err(ApiError::InternalServerError)?; + .await?; json_response(StatusCode::OK, ()) } @@ -407,8 +403,7 @@ async fn tenant_load_handler(request: Request) -> Result, A let state = get_state(&request); mgr::load_tenant(state.conf, tenant_id, state.remote_storage.clone(), &ctx) .instrument(info_span!("load", tenant = %tenant_id)) - .await - .map_err(apierror_from_tenant_map_insert_error)?; + .await?; json_response(StatusCode::ACCEPTED, ()) } @@ -421,10 +416,7 @@ async fn tenant_ignore_handler(request: Request) -> Result, let conf = state.conf; mgr::ignore_tenant(conf, tenant_id) .instrument(info_span!("ignore_tenant", tenant = %tenant_id)) - .await - // FIXME: Errors from `ignore_tenant` can be caused by both both user and internal errors. - // Replace this with better handling once the error type permits it. - .map_err(ApiError::InternalServerError)?; + .await?; json_response(StatusCode::OK, ()) } @@ -498,9 +490,7 @@ async fn tenant_size_handler(request: Request) -> Result, A let headers = request.headers(); let ctx = RequestContext::new(TaskKind::MgmtRequest, DownloadBehavior::Download); - let tenant = mgr::get_tenant(tenant_id, true) - .await - .map_err(ApiError::InternalServerError)?; + let tenant = mgr::get_tenant(tenant_id, true).await?; // this can be long operation let inputs = tenant @@ -763,8 +753,7 @@ async fn tenant_create_handler(mut request: Request) -> Result) -> Result Result, ApiError> { - let tenant = mgr::get_tenant(tenant_id, true) - .await - .map_err(ApiError::NotFound)?; + let tenant = mgr::get_tenant(tenant_id, true).await?; tenant .get_timeline(timeline_id, true) .map_err(ApiError::NotFound) diff --git a/pageserver/src/page_service.rs b/pageserver/src/page_service.rs index aad6099952..b63ee31d5e 100644 --- a/pageserver/src/page_service.rs +++ b/pageserver/src/page_service.rs @@ -1107,7 +1107,10 @@ async fn get_active_tenant_with_timeout( tenant_id: TenantId, _ctx: &RequestContext, /* require get a context to support cancellation in the future */ ) -> Result, GetActiveTenantError> { - let tenant = mgr::get_tenant(tenant_id, false).await?; + let tenant = match mgr::get_tenant(tenant_id, false).await { + Ok(tenant) => tenant, + Err(e) => return Err(GetActiveTenantError::Other(e.into())), + }; let wait_time = Duration::from_secs(30); match tokio::time::timeout(wait_time, tenant.wait_to_become_active()).await { Ok(Ok(())) => Ok(tenant), diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs index a44cb02b4d..a4212ea8a6 100644 --- a/pageserver/src/tenant/mgr.rs +++ b/pageserver/src/tenant/mgr.rs @@ -289,7 +289,7 @@ pub async fn set_new_tenant_config( conf: &'static PageServerConf, new_tenant_conf: TenantConfOpt, tenant_id: TenantId, -) -> anyhow::Result<()> { +) -> Result<(), TenantStateError> { info!("configuring tenant {tenant_id}"); let tenant = get_tenant(tenant_id, true).await?; @@ -306,16 +306,20 @@ pub async fn set_new_tenant_config( /// Gets the tenant from the in-memory data, erroring if it's absent or is not fitting to the query. /// `active_only = true` allows to query only tenants that are ready for operations, erroring on other kinds of tenants. -pub async fn get_tenant(tenant_id: TenantId, active_only: bool) -> anyhow::Result> { +pub async fn get_tenant( + tenant_id: TenantId, + active_only: bool, +) -> Result, TenantStateError> { let m = TENANTS.read().await; let tenant = m .get(&tenant_id) - .with_context(|| format!("Tenant {tenant_id} not found in the local state"))?; + .ok_or(TenantStateError::NotFound(tenant_id))?; if active_only && !tenant.is_active() { - anyhow::bail!( + tracing::warn!( "Tenant {tenant_id} is not active. Current state: {:?}", tenant.current_state() - ) + ); + Err(TenantStateError::NotActive(tenant_id)) } else { Ok(Arc::clone(tenant)) } @@ -325,21 +329,28 @@ pub async fn delete_timeline( tenant_id: TenantId, timeline_id: TimelineId, ctx: &RequestContext, -) -> anyhow::Result<()> { - match get_tenant(tenant_id, true).await { - Ok(tenant) => { - tenant.delete_timeline(timeline_id, ctx).await?; - } - Err(e) => anyhow::bail!("Cannot access tenant {tenant_id} in local tenant state: {e:?}"), - } - +) -> Result<(), TenantStateError> { + let tenant = get_tenant(tenant_id, true).await?; + tenant.delete_timeline(timeline_id, ctx).await?; Ok(()) } +#[derive(Debug, thiserror::Error)] +pub enum TenantStateError { + #[error("Tenant {0} not found")] + NotFound(TenantId), + #[error("Tenant {0} is stopping")] + IsStopping(TenantId), + #[error("Tenant {0} is not active")] + NotActive(TenantId), + #[error(transparent)] + Other(#[from] anyhow::Error), +} + pub async fn detach_tenant( conf: &'static PageServerConf, tenant_id: TenantId, -) -> anyhow::Result<()> { +) -> Result<(), TenantStateError> { remove_tenant_from_memory(tenant_id, async { let local_tenant_directory = conf.tenant_path(&tenant_id); fs::remove_dir_all(&local_tenant_directory) @@ -379,7 +390,7 @@ pub async fn load_tenant( pub async fn ignore_tenant( conf: &'static PageServerConf, tenant_id: TenantId, -) -> anyhow::Result<()> { +) -> Result<(), TenantStateError> { remove_tenant_from_memory(tenant_id, async { let ignore_mark_file = conf.tenant_ignore_mark_file_path(tenant_id); fs::File::create(&ignore_mark_file) @@ -489,7 +500,7 @@ where async fn remove_tenant_from_memory( tenant_id: TenantId, tenant_cleanup: F, -) -> anyhow::Result +) -> Result where F: std::future::Future>, { @@ -505,11 +516,9 @@ where | TenantState::Loading | TenantState::Broken | TenantState::Active => tenant.set_stopping(), - TenantState::Stopping => { - anyhow::bail!("Tenant {tenant_id} is stopping already") - } + TenantState::Stopping => return Err(TenantStateError::IsStopping(tenant_id)), }, - None => anyhow::bail!("Tenant not found for id {tenant_id}"), + None => return Err(TenantStateError::NotFound(tenant_id)), } } @@ -532,10 +541,15 @@ where Err(e) => { let tenants_accessor = TENANTS.read().await; match tenants_accessor.get(&tenant_id) { - Some(tenant) => tenant.set_broken(&e.to_string()), - None => warn!("Tenant {tenant_id} got removed from memory"), + Some(tenant) => { + tenant.set_broken(&e.to_string()); + } + None => { + warn!("Tenant {tenant_id} got removed from memory"); + return Err(TenantStateError::NotFound(tenant_id)); + } } - Err(e) + Err(TenantStateError::Other(e)) } } } @@ -555,7 +569,7 @@ pub async fn immediate_gc( let tenant = guard .get(&tenant_id) .map(Arc::clone) - .with_context(|| format!("Tenant {tenant_id} not found")) + .with_context(|| format!("tenant {tenant_id}")) .map_err(ApiError::NotFound)?; let gc_horizon = gc_req.gc_horizon.unwrap_or_else(|| tenant.get_gc_horizon()); @@ -605,7 +619,7 @@ pub async fn immediate_compact( let tenant = guard .get(&tenant_id) .map(Arc::clone) - .with_context(|| format!("Tenant {tenant_id} not found")) + .with_context(|| format!("tenant {tenant_id}")) .map_err(ApiError::NotFound)?; let timeline = tenant diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index 8a64be51f1..8b228ad804 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -2086,8 +2086,8 @@ class NeonPageserver(PgProtocol): # https://github.com/neondatabase/neon/issues/2442 ".*could not remove ephemeral file.*No such file or directory.*", # FIXME: These need investigation - ".*gc_loop.*Failed to get a tenant .* Tenant .* not found in the local state.*", - ".*compaction_loop.*Failed to get a tenant .* Tenant .* not found in the local state.*", + ".*gc_loop.*Failed to get a tenant .* Tenant .* not found.*", + ".*compaction_loop.*Failed to get a tenant .* Tenant .* not found.*", ".*manual_gc.*is_shutdown_requested\\(\\) called in an unexpected task or thread.*", ".*tenant_list: timeline is not found in remote index while it is present in the tenants registry.*", ".*Removing intermediate uninit mark file.*", diff --git a/test_runner/regress/test_tenant_detach.py b/test_runner/regress/test_tenant_detach.py index 27ec38e1be..e061ab92a4 100644 --- a/test_runner/regress/test_tenant_detach.py +++ b/test_runner/regress/test_tenant_detach.py @@ -225,7 +225,7 @@ def test_tenant_reattach_while_busy( # Attempts to connect from compute to pageserver while the tenant is # temporarily detached produces these errors in the pageserver log. - env.pageserver.allowed_errors.append(".*Tenant .* not found in the local state.*") + env.pageserver.allowed_errors.append(".*Tenant .* not found.*") env.pageserver.allowed_errors.append( ".*Tenant .* will not become active\\. Current state: Stopping.*" ) @@ -257,18 +257,18 @@ def test_tenant_detach_smoke(neon_env_builder: NeonEnvBuilder): env = neon_env_builder.init_start() pageserver_http = env.pageserver.http_client() - env.pageserver.allowed_errors.append(".*NotFound: Tenant .* not found") + env.pageserver.allowed_errors.append(".*NotFound: Tenant .*") # first check for non existing tenant tenant_id = TenantId.generate() with pytest.raises( expected_exception=PageserverApiException, - match=f"Tenant not found for id {tenant_id}", + match=f"NotFound: tenant {tenant_id}", ): pageserver_http.tenant_detach(tenant_id) # the error will be printed to the log too - env.pageserver.allowed_errors.append(".*Tenant not found for id.*") + env.pageserver.allowed_errors.append(".*NotFound: tenant *") # create new nenant tenant_id, timeline_id = env.neon_cli.create_tenant() @@ -294,7 +294,7 @@ def test_tenant_detach_smoke(neon_env_builder: NeonEnvBuilder): # the error will be printed to the log too env.pageserver.allowed_errors.append(".*gc target timeline does not exist.*") - # Timelines get stopped during detach, ignore the gc calls that error, whitnessing that + # Timelines get stopped during detach, ignore the gc calls that error, witnessing that env.pageserver.allowed_errors.append(".*InternalServerError\\(timeline is Stopping.*") # Detach while running manual GC. @@ -320,7 +320,7 @@ def test_tenant_detach_smoke(neon_env_builder: NeonEnvBuilder): assert not (env.repo_dir / "tenants" / str(tenant_id)).exists() with pytest.raises( - expected_exception=PageserverApiException, match=f"Tenant {tenant_id} not found" + expected_exception=PageserverApiException, match=f"NotFound: tenant {tenant_id}" ): pageserver_http.timeline_gc(tenant_id, timeline_id, 0) diff --git a/test_runner/regress/test_timeline_delete.py b/test_runner/regress/test_timeline_delete.py index 2226cab8ff..b9c4f5b83f 100644 --- a/test_runner/regress/test_timeline_delete.py +++ b/test_runner/regress/test_timeline_delete.py @@ -10,7 +10,7 @@ def test_timeline_delete(neon_simple_env: NeonEnv): env.pageserver.allowed_errors.append(".*Timeline .* was not found.*") env.pageserver.allowed_errors.append(".*timeline not found.*") env.pageserver.allowed_errors.append(".*Cannot delete timeline which has child timelines.*") - env.pageserver.allowed_errors.append(".*Tenant .* not found in the local state.*") + env.pageserver.allowed_errors.append(".*NotFound: tenant .*") ps_http = env.pageserver.http_client() @@ -24,7 +24,7 @@ def test_timeline_delete(neon_simple_env: NeonEnv): invalid_tenant_id = TenantId.generate() with pytest.raises( PageserverApiException, - match=f"Tenant {invalid_tenant_id} not found in the local state", + match=f"NotFound: tenant {invalid_tenant_id}", ): ps_http.timeline_delete(tenant_id=invalid_tenant_id, timeline_id=invalid_timeline_id) diff --git a/test_runner/regress/test_walredo_not_left_behind_on_detach.py b/test_runner/regress/test_walredo_not_left_behind_on_detach.py index 24045e2eb7..395d54b8c3 100644 --- a/test_runner/regress/test_walredo_not_left_behind_on_detach.py +++ b/test_runner/regress/test_walredo_not_left_behind_on_detach.py @@ -23,7 +23,7 @@ def assert_child_processes(pageserver_pid, wal_redo_present=False, defunct_prese def test_walredo_not_left_behind_on_detach(neon_env_builder: NeonEnvBuilder): env = neon_env_builder.init_start() # We intentionally test for a non-existent tenant. - env.pageserver.allowed_errors.append(".*Tenant not found.*") + env.pageserver.allowed_errors.append(".*NotFound: tenant.*") pageserver_http = env.pageserver.http_client() pagserver_pid = int((env.repo_dir / "pageserver.pid").read_text()) @@ -34,7 +34,7 @@ def test_walredo_not_left_behind_on_detach(neon_env_builder: NeonEnvBuilder): tenant_id = TenantId.generate() with pytest.raises( expected_exception=PageserverApiException, - match=f"Tenant not found for id {tenant_id}", + match=f"NotFound: tenant {tenant_id}", ): pageserver_http.tenant_detach(tenant_id) From 3c15874c48863cd1f0f4dc8c4bb6d0899874df36 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Fri, 17 Mar 2023 18:50:42 +0100 Subject: [PATCH 23/32] allow specifying eviction_policy in TenantCreateRequest This was on oversight from 175a577ad42476a49978d277a7428e8a078dd6ae. Nothing uses this AFAIK, but, let's fix it anyways. Noticed while working on https://github.com/neondatabase/neon/issues/3728 --- control_plane/src/pageserver.rs | 5 +++++ libs/pageserver_api/src/models.rs | 5 +++++ pageserver/src/http/routes.rs | 8 ++++++++ 3 files changed, 18 insertions(+) diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index 07ead45d5b..db8cb61395 100644 --- a/control_plane/src/pageserver.rs +++ b/control_plane/src/pageserver.rs @@ -362,6 +362,11 @@ impl PageServerNode { .map(|x| x.parse::()) .transpose() .context("Failed to parse 'trace_read_requests' as bool")?, + eviction_policy: settings + .get("eviction_policy") + .map(|x| serde_json::from_str(x)) + .transpose() + .context("Failed to parse 'eviction_policy' json")?, }; if !settings.is_empty() { bail!("Unrecognized tenant settings: {settings:?}") diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index 3ac7e31ec2..7a43100ba5 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -115,6 +115,11 @@ pub struct TenantCreateRequest { pub lagging_wal_timeout: Option, pub max_lsn_wal_lag: Option, pub trace_read_requests: Option, + // We defer the parsing of the eviction_policy field to the request handler. + // Otherwise we'd have to move the types for eviction policy into this package. + // We might do that once the eviction feature has stabilizied. + // For now, this field is not even documented in the openapi_spec.yml. + pub eviction_policy: Option, } #[serde_as] diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs index 2f03e251fd..39f2776952 100644 --- a/pageserver/src/http/routes.rs +++ b/pageserver/src/http/routes.rs @@ -738,6 +738,14 @@ async fn tenant_create_handler(mut request: Request) -> Result Date: Mon, 20 Mar 2023 11:57:27 +0200 Subject: [PATCH 24/32] feat: store initial timeline in env fixture (#3839) minor change, but will allow more use in future for the default tenants. Co-authored-by: Alexander Bayandin --- test_runner/fixtures/neon_fixtures.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index 8b228ad804..cd4f0678b5 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -643,6 +643,7 @@ class NeonEnvBuilder: f"Services started, creating initial tenant {env.initial_tenant} and its initial timeline" ) initial_tenant, initial_timeline = env.neon_cli.create_tenant(tenant_id=env.initial_tenant) + env.initial_timeline = initial_timeline log.info(f"Initial timeline {initial_tenant}/{initial_timeline} created successfully") return env @@ -904,6 +905,7 @@ class NeonEnv: # generate initial tenant ID here instead of letting 'neon init' generate it, # so that we don't need to dig it out of the config file afterwards. self.initial_tenant = config.initial_tenant + self.initial_timeline: Optional[TimelineId] = None # Create a config file corresponding to the options toml = textwrap.dedent( From 1ddb9249aaf25436cf49e466392a90537268ed3b Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Mon, 20 Mar 2023 15:49:16 +0200 Subject: [PATCH 25/32] Reduce the # of histogram buckets in metrics. (#3850) Shrinks the total number of metrics collected for each timeline by about 50%. See https://github.com/neondatabase/neon/issues/2848. This doesn't fully solve the problem, we still collect a lot of metrics even with this, but this gives us a lot of headroom. --- pageserver/src/metrics.rs | 60 ++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 35 deletions(-) diff --git a/pageserver/src/metrics.rs b/pageserver/src/metrics.rs index 4826a0b7ae..6a8aecfd25 100644 --- a/pageserver/src/metrics.rs +++ b/pageserver/src/metrics.rs @@ -9,22 +9,18 @@ use once_cell::sync::Lazy; use pageserver_api::models::state; use utils::id::{TenantId, TimelineId}; -/// Prometheus histogram buckets (in seconds) that capture the majority of -/// latencies in the microsecond range but also extend far enough up to distinguish -/// "bad" from "really bad". -fn get_buckets_for_critical_operations() -> Vec { - let buckets_per_digit = 5; - let min_exponent = -6; - let max_exponent = 2; - - let mut buckets = vec![]; - // Compute 10^(exp / buckets_per_digit) instead of 10^(1/buckets_per_digit)^exp - // because it's more numerically stable and doesn't result in numbers like 9.999999 - for exp in (min_exponent * buckets_per_digit)..=(max_exponent * buckets_per_digit) { - buckets.push(10_f64.powf(exp as f64 / buckets_per_digit as f64)) - } - buckets -} +/// Prometheus histogram buckets (in seconds) for operations in the critical +/// path. In other words, operations that directly affect that latency of user +/// queries. +/// +/// The buckets capture the majority of latencies in the microsecond and +/// millisecond range but also extend far enough up to distinguish "bad" from +/// "really bad". +const CRITICAL_OP_BUCKETS: &[f64] = &[ + 0.000_001, 0.000_010, 0.000_100, // 1 us, 10 us, 100 us + 0.001_000, 0.010_000, 0.100_000, // 1 ms, 10 ms, 100 ms + 1.0, 10.0, 100.0, // 1 s, 10 s, 100 s +]; // Metrics collected on operations on the storage repository. const STORAGE_TIME_OPERATIONS: &[&str] = &[ @@ -55,12 +51,15 @@ pub static STORAGE_TIME_COUNT_PER_TIMELINE: Lazy = Lazy::new(|| { .expect("failed to define a metric") }); +// Buckets for background operations like compaction, GC, size calculation +const STORAGE_OP_BUCKETS: &[f64] = &[0.010, 0.100, 1.0, 10.0, 100.0, 1000.0]; + pub static STORAGE_TIME_GLOBAL: Lazy = Lazy::new(|| { register_histogram_vec!( "pageserver_storage_operations_seconds_global", "Time spent on storage operations", &["operation"], - get_buckets_for_critical_operations(), + STORAGE_OP_BUCKETS.into(), ) .expect("failed to define a metric") }); @@ -71,7 +70,7 @@ static RECONSTRUCT_TIME: Lazy = Lazy::new(|| { "pageserver_getpage_reconstruct_seconds", "Time spent in reconstruct_value", &["tenant_id", "timeline_id"], - get_buckets_for_critical_operations(), + CRITICAL_OP_BUCKETS.into(), ) .expect("failed to define a metric") }); @@ -90,7 +89,7 @@ static WAIT_LSN_TIME: Lazy = Lazy::new(|| { "pageserver_wait_lsn_seconds", "Time spent waiting for WAL to arrive", &["tenant_id", "timeline_id"], - get_buckets_for_critical_operations(), + CRITICAL_OP_BUCKETS.into(), ) .expect("failed to define a metric") }); @@ -196,14 +195,13 @@ static PERSISTENT_BYTES_WRITTEN: Lazy = Lazy::new(|| { }); // Metrics collected on disk IO operations +// +// Roughly logarithmic scale. const STORAGE_IO_TIME_BUCKETS: &[f64] = &[ - 0.000001, // 1 usec - 0.00001, // 10 usec - 0.0001, // 100 usec - 0.001, // 1 msec - 0.01, // 10 msec - 0.1, // 100 msec - 1.0, // 1 sec + 0.000030, // 30 usec + 0.001000, // 1000 usec + 0.030, // 30 ms + 1.000, // 1000 ms ]; const STORAGE_IO_TIME_OPERATIONS: &[&str] = &[ @@ -238,20 +236,12 @@ const SMGR_QUERY_TIME_OPERATIONS: &[&str] = &[ "get_db_size", ]; -const SMGR_QUERY_TIME_BUCKETS: &[f64] = &[ - 0.00001, // 1/100000 s - 0.0001, 0.00015, 0.0002, 0.00025, 0.0003, 0.00035, 0.0005, 0.00075, // 1/10000 s - 0.001, 0.0025, 0.005, 0.0075, // 1/1000 s - 0.01, 0.0125, 0.015, 0.025, 0.05, // 1/100 s - 0.1, // 1/10 s -]; - pub static SMGR_QUERY_TIME: Lazy = Lazy::new(|| { register_histogram_vec!( "pageserver_smgr_query_seconds", "Time spent on smgr query handling", &["smgr_query_type", "tenant_id", "timeline_id"], - SMGR_QUERY_TIME_BUCKETS.into() + CRITICAL_OP_BUCKETS.into(), ) .expect("failed to define a metric") }); From 1da963b2f91309a16aa3b98d6b0ce376d47eb3f8 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Mon, 20 Mar 2023 15:50:23 +0200 Subject: [PATCH 26/32] Remove some unused code in control plane. --- control_plane/src/pageserver.rs | 7 ------- control_plane/src/safekeeper.rs | 7 ------- 2 files changed, 14 deletions(-) diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index db8cb61395..2a8b889d84 100644 --- a/control_plane/src/pageserver.rs +++ b/control_plane/src/pageserver.rs @@ -280,13 +280,6 @@ impl PageServerNode { background_process::stop_process(immediate, "pageserver", &self.pid_file()) } - pub fn page_server_psql(&self, sql: &str) -> Vec { - let mut client = self.pg_connection_config.connect_no_tls().unwrap(); - - println!("Pageserver query: '{sql}'"); - client.simple_query(sql).unwrap() - } - pub fn page_server_psql_client(&self) -> result::Result { self.pg_connection_config.connect_no_tls() } diff --git a/control_plane/src/safekeeper.rs b/control_plane/src/safekeeper.rs index 228968be80..84d6320573 100644 --- a/control_plane/src/safekeeper.rs +++ b/control_plane/src/safekeeper.rs @@ -1,7 +1,6 @@ use std::io::Write; use std::path::PathBuf; use std::process::Child; -use std::sync::Arc; use std::{io, result}; use anyhow::Context; @@ -11,7 +10,6 @@ use reqwest::{IntoUrl, Method}; use thiserror::Error; use utils::{http::error::HttpErrorBody, id::NodeId}; -use crate::pageserver::PageServerNode; use crate::{ background_process, local_env::{LocalEnv, SafekeeperConf}, @@ -65,14 +63,10 @@ pub struct SafekeeperNode { pub env: LocalEnv, pub http_client: Client, pub http_base_url: String, - - pub pageserver: Arc, } impl SafekeeperNode { pub fn from_env(env: &LocalEnv, conf: &SafekeeperConf) -> SafekeeperNode { - let pageserver = Arc::new(PageServerNode::from_env(env)); - SafekeeperNode { id: conf.id, conf: conf.clone(), @@ -80,7 +74,6 @@ impl SafekeeperNode { env: env.clone(), http_client: Client::new(), http_base_url: format!("http://127.0.0.1:{}/v1", conf.http_port), - pageserver, } } From 77107607f3d69c21d4a17b20a533385ff653b639 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Mon, 20 Mar 2023 15:50:27 +0200 Subject: [PATCH 27/32] Allow JWT key generation to fail if authentication is not enabled. This allows you to run without the 'openssl' binary as long as you don't enable authentication. This becomes more important with the next commit, which switches the JWT algorithm to EdDSA. LibreSSL does not support EdDSA, and LibreSSL comes with macOS, so the next commit makes it much more likely for the key generation to fail for macOS users. To allow running without a keypair, don't generate the authentication token in the 'neon_local init' step. Instead, generate a new token on every request that needs one, using the private key. --- control_plane/src/local_env.rs | 106 +++++++++++++--------- control_plane/src/pageserver.rs | 43 +++++---- test_runner/regress/test_compatibility.py | 2 - 3 files changed, 86 insertions(+), 65 deletions(-) diff --git a/control_plane/src/local_env.rs b/control_plane/src/local_env.rs index 630f8bb664..d4de72b6bf 100644 --- a/control_plane/src/local_env.rs +++ b/control_plane/src/local_env.rs @@ -18,7 +18,7 @@ use std::net::SocketAddr; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use utils::{ - auth::{encode_from_key_file, Claims, Scope}, + auth::{encode_from_key_file, Claims}, id::{NodeId, TenantId, TenantTimelineId, TimelineId}, }; @@ -118,9 +118,6 @@ pub struct PageServerConf { // auth type used for the PG and HTTP ports pub pg_auth_type: AuthType, pub http_auth_type: AuthType, - - // jwt auth token used for communication with pageserver - pub auth_token: String, } impl Default for PageServerConf { @@ -131,7 +128,6 @@ impl Default for PageServerConf { listen_http_addr: String::new(), pg_auth_type: AuthType::Trust, http_auth_type: AuthType::Trust, - auth_token: String::new(), } } } @@ -404,48 +400,33 @@ impl LocalEnv { fs::create_dir(base_path)?; - // generate keys for jwt - // openssl genrsa -out private_key.pem 2048 - let private_key_path; + // Generate keypair for JWT. + // + // The keypair is only needed if authentication is enabled in any of the + // components. For convenience, we generate the keypair even if authentication + // is not enabled, so that you can easily enable it after the initialization + // step. However, if the key generation fails, we treat it as non-fatal if + // authentication was not enabled. if self.private_key_path == PathBuf::new() { - private_key_path = base_path.join("auth_private_key.pem"); - let keygen_output = Command::new("openssl") - .arg("genrsa") - .args(["-out", private_key_path.to_str().unwrap()]) - .arg("2048") - .stdout(Stdio::null()) - .output() - .context("failed to generate auth private key")?; - if !keygen_output.status.success() { - bail!( - "openssl failed: '{}'", - String::from_utf8_lossy(&keygen_output.stderr) - ); - } - self.private_key_path = PathBuf::from("auth_private_key.pem"); - - let public_key_path = base_path.join("auth_public_key.pem"); - // openssl rsa -in private_key.pem -pubout -outform PEM -out public_key.pem - let keygen_output = Command::new("openssl") - .arg("rsa") - .args(["-in", private_key_path.to_str().unwrap()]) - .arg("-pubout") - .args(["-outform", "PEM"]) - .args(["-out", public_key_path.to_str().unwrap()]) - .stdout(Stdio::null()) - .output() - .context("failed to generate auth private key")?; - if !keygen_output.status.success() { - bail!( - "openssl failed: '{}'", - String::from_utf8_lossy(&keygen_output.stderr) - ); + match generate_auth_keys( + base_path.join("auth_private_key.pem").as_path(), + base_path.join("auth_public_key.pem").as_path(), + ) { + Ok(()) => { + self.private_key_path = PathBuf::from("auth_private_key.pem"); + } + Err(e) => { + if !self.auth_keys_needed() { + eprintln!("Could not generate keypair for JWT authentication: {e}"); + eprintln!("Continuing anyway because authentication was not enabled"); + self.private_key_path = PathBuf::from("auth_private_key.pem"); + } else { + return Err(e); + } + } } } - self.pageserver.auth_token = - self.generate_auth_token(&Claims::new(None, Scope::PageServerApi))?; - fs::create_dir_all(self.pg_data_dirs_path())?; for safekeeper in &self.safekeepers { @@ -454,6 +435,12 @@ impl LocalEnv { self.persist_config(base_path) } + + fn auth_keys_needed(&self) -> bool { + self.pageserver.pg_auth_type == AuthType::NeonJWT + || self.pageserver.http_auth_type == AuthType::NeonJWT + || self.safekeepers.iter().any(|sk| sk.auth_enabled) + } } fn base_path() -> PathBuf { @@ -463,6 +450,39 @@ fn base_path() -> PathBuf { } } +/// Generate a public/private key pair for JWT authentication +fn generate_auth_keys(private_key_path: &Path, public_key_path: &Path) -> anyhow::Result<()> { + let keygen_output = Command::new("openssl") + .arg("genrsa") + .args(["-out", private_key_path.to_str().unwrap()]) + .arg("2048") + .stdout(Stdio::null()) + .output() + .context("failed to generate auth private key")?; + if !keygen_output.status.success() { + bail!( + "openssl failed: '{}'", + String::from_utf8_lossy(&keygen_output.stderr) + ); + } + // openssl rsa -in private_key.pem -pubout -outform PEM -out public_key.pem + let keygen_output = Command::new("openssl") + .arg("rsa") + .args(["-in", private_key_path.to_str().unwrap()]) + .arg("-pubout") + .args(["-outform", "PEM"]) + .args(["-out", public_key_path.to_str().unwrap()]) + .output() + .context("failed to extract public key from private key")?; + if !keygen_output.status.success() { + bail!( + "openssl failed: '{}'", + String::from_utf8_lossy(&keygen_output.stderr) + ); + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index 2a8b889d84..3c66400a05 100644 --- a/control_plane/src/pageserver.rs +++ b/control_plane/src/pageserver.rs @@ -82,15 +82,8 @@ impl PageServerNode { let (host, port) = parse_host_port(&env.pageserver.listen_pg_addr) .expect("Unable to parse listen_pg_addr"); let port = port.unwrap_or(5432); - let password = if env.pageserver.pg_auth_type == AuthType::NeonJWT { - Some(env.pageserver.auth_token.clone()) - } else { - None - }; - Self { - pg_connection_config: PgConnectionConfig::new_host_port(host, port) - .set_password(password), + pg_connection_config: PgConnectionConfig::new_host_port(host, port), env: env.clone(), http_client: Client::new(), http_base_url: format!("http://{}/v1", env.pageserver.listen_http_addr), @@ -280,20 +273,30 @@ impl PageServerNode { background_process::stop_process(immediate, "pageserver", &self.pid_file()) } - pub fn page_server_psql_client(&self) -> result::Result { - self.pg_connection_config.connect_no_tls() + pub fn page_server_psql_client(&self) -> anyhow::Result { + let mut config = self.pg_connection_config.clone(); + if self.env.pageserver.pg_auth_type == AuthType::NeonJWT { + let token = self + .env + .generate_auth_token(&Claims::new(None, Scope::PageServerApi))?; + config = config.set_password(Some(token)); + } + Ok(config.connect_no_tls()?) } - fn http_request(&self, method: Method, url: U) -> RequestBuilder { + fn http_request(&self, method: Method, url: U) -> anyhow::Result { let mut builder = self.http_client.request(method, url); if self.env.pageserver.http_auth_type == AuthType::NeonJWT { - builder = builder.bearer_auth(&self.env.pageserver.auth_token) + let token = self + .env + .generate_auth_token(&Claims::new(None, Scope::PageServerApi))?; + builder = builder.bearer_auth(token) } - builder + Ok(builder) } pub fn check_status(&self) -> Result<()> { - self.http_request(Method::GET, format!("{}/status", self.http_base_url)) + self.http_request(Method::GET, format!("{}/status", self.http_base_url))? .send()? .error_from_body()?; Ok(()) @@ -301,7 +304,7 @@ impl PageServerNode { pub fn tenant_list(&self) -> Result> { Ok(self - .http_request(Method::GET, format!("{}/tenant", self.http_base_url)) + .http_request(Method::GET, format!("{}/tenant", self.http_base_url))? .send()? .error_from_body()? .json()?) @@ -364,7 +367,7 @@ impl PageServerNode { if !settings.is_empty() { bail!("Unrecognized tenant settings: {settings:?}") } - self.http_request(Method::POST, format!("{}/tenant", self.http_base_url)) + self.http_request(Method::POST, format!("{}/tenant", self.http_base_url))? .json(&request) .send()? .error_from_body()? @@ -381,7 +384,7 @@ impl PageServerNode { } pub fn tenant_config(&self, tenant_id: TenantId, settings: HashMap<&str, &str>) -> Result<()> { - self.http_request(Method::PUT, format!("{}/tenant/config", self.http_base_url)) + self.http_request(Method::PUT, format!("{}/tenant/config", self.http_base_url))? .json(&TenantConfigRequest { tenant_id, checkpoint_distance: settings @@ -444,7 +447,7 @@ impl PageServerNode { .http_request( Method::GET, format!("{}/tenant/{}/timeline", self.http_base_url, tenant_id), - ) + )? .send()? .error_from_body()? .json()?; @@ -463,7 +466,7 @@ impl PageServerNode { self.http_request( Method::POST, format!("{}/tenant/{}/timeline", self.http_base_url, tenant_id), - ) + )? .json(&TimelineCreateRequest { new_timeline_id, ancestor_start_lsn, @@ -500,7 +503,7 @@ impl PageServerNode { pg_wal: Option<(Lsn, PathBuf)>, pg_version: u32, ) -> anyhow::Result<()> { - let mut client = self.pg_connection_config.connect_no_tls().unwrap(); + let mut client = self.page_server_psql_client()?; // Init base reader let (start_lsn, base_tarfile_path) = base; diff --git a/test_runner/regress/test_compatibility.py b/test_runner/regress/test_compatibility.py index 66625dd6f8..e9dadb5348 100644 --- a/test_runner/regress/test_compatibility.py +++ b/test_runner/regress/test_compatibility.py @@ -365,11 +365,9 @@ def check_neon_works( tenant_id = snapshot_config["default_tenant_id"] timeline_id = dict(snapshot_config["branch_name_mappings"]["main"])[tenant_id] pageserver_port = snapshot_config["pageserver"]["listen_http_addr"].split(":")[-1] - auth_token = snapshot_config["pageserver"]["auth_token"] pageserver_http = PageserverHttpClient( port=pageserver_port, is_testing_enabled_or_skip=lambda: True, # TODO: check if testing really enabled - auth_token=auth_token, ) shutil.rmtree(repo_dir / "local_fs_remote_storage") From fea4b5f5512ac80057fc6aa2b82d027fdd3fb85d Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Mon, 20 Mar 2023 15:50:30 +0200 Subject: [PATCH 28/32] Switch to EdDSA algorithm for the storage JWT authentication tokens. The control plane currently only supports EdDSA. We need to either teach the storage to use EdDSA, or the control plane to use RSA. EdDSA is more modern, so let's use that. We could support both, but it would require a little more code and tests, and we don't really need the flexibility since we control both sides. --- README.md | 5 +- control_plane/src/local_env.rs | 14 ++-- docs/authentication.md | 15 ++-- libs/utils/src/auth.rs | 99 +++++++-------------------- safekeeper/src/bin/safekeeper.rs | 2 +- test_runner/fixtures/neon_fixtures.py | 2 +- 6 files changed, 49 insertions(+), 88 deletions(-) diff --git a/README.md b/README.md index 819693f1f3..43f3e3a02b 100644 --- a/README.md +++ b/README.md @@ -46,11 +46,14 @@ postgresql-libs cmake postgresql protobuf curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh ``` -#### Installing dependencies on OSX (12.3.1) +#### Installing dependencies on macOS (12.3.1) 1. Install XCode and dependencies ``` xcode-select --install brew install protobuf openssl flex bison + +# add openssl to PATH, required for ed25519 keys generation in neon_local +echo 'export PATH="$(brew --prefix openssl)/bin:$PATH"' >> ~/.zshrc ``` 2. [Install Rust](https://www.rust-lang.org/tools/install) diff --git a/control_plane/src/local_env.rs b/control_plane/src/local_env.rs index d4de72b6bf..8cc6329ce6 100644 --- a/control_plane/src/local_env.rs +++ b/control_plane/src/local_env.rs @@ -452,10 +452,13 @@ fn base_path() -> PathBuf { /// Generate a public/private key pair for JWT authentication fn generate_auth_keys(private_key_path: &Path, public_key_path: &Path) -> anyhow::Result<()> { + // Generate the key pair + // + // openssl genpkey -algorithm ed25519 -out auth_private_key.pem let keygen_output = Command::new("openssl") - .arg("genrsa") + .arg("genpkey") + .args(["-algorithm", "ed25519"]) .args(["-out", private_key_path.to_str().unwrap()]) - .arg("2048") .stdout(Stdio::null()) .output() .context("failed to generate auth private key")?; @@ -465,12 +468,13 @@ fn generate_auth_keys(private_key_path: &Path, public_key_path: &Path) -> anyhow String::from_utf8_lossy(&keygen_output.stderr) ); } - // openssl rsa -in private_key.pem -pubout -outform PEM -out public_key.pem + // Extract the public key from the private key file + // + // openssl pkey -in auth_private_key.pem -pubout -out auth_public_key.pem let keygen_output = Command::new("openssl") - .arg("rsa") + .arg("pkey") .args(["-in", private_key_path.to_str().unwrap()]) .arg("-pubout") - .args(["-outform", "PEM"]) .args(["-out", public_key_path.to_str().unwrap()]) .output() .context("failed to extract public key from private key")?; diff --git a/docs/authentication.md b/docs/authentication.md index e6b5fa5707..dc402d1bca 100644 --- a/docs/authentication.md +++ b/docs/authentication.md @@ -29,15 +29,22 @@ These components should not have access to the private key and may only get toke The key pair is generated once for an installation of compute/pageserver/safekeeper, e.g. by `neon_local init`. There is currently no way to rotate the key without bringing down all components. +### Best practices + +See [RFC 8725: JSON Web Token Best Current Practices](https://www.rfc-editor.org/rfc/rfc8725) + + ### Token format -The JWT tokens in Neon use RSA as the algorithm. Example: +The JWT tokens in Neon use "EdDSA" as the algorithm (defined in [RFC8037](https://www.rfc-editor.org/rfc/rfc8037)). + +Example: Header: ``` { - "alg": "RS512", # RS256, RS384, or RS512 + "alg": "EdDSA", "typ": "JWT" } ``` @@ -68,8 +75,8 @@ Currently also used for connection from any pageserver to any safekeeper. CLI generates a key pair during call to `neon_local init` with the following commands: ```bash -openssl genrsa -out auth_private_key.pem 2048 -openssl rsa -in auth_private_key.pem -pubout -outform PEM -out auth_public_key.pem +openssl genpkey -algorithm ed25519 -out auth_private_key.pem +openssl pkey -in auth_private_key.pem -pubout -out auth_public_key.pem ``` Configuration files for all components point to `public_key.pem` for JWT validation. diff --git a/libs/utils/src/auth.rs b/libs/utils/src/auth.rs index 027950cb39..0fb45e01c6 100644 --- a/libs/utils/src/auth.rs +++ b/libs/utils/src/auth.rs @@ -1,7 +1,4 @@ // For details about authentication see docs/authentication.md -// -// TODO: use ed25519 keys -// Relevant issue: https://github.com/Keats/jsonwebtoken/issues/162 use serde; use std::fs; @@ -9,26 +6,15 @@ use std::path::Path; use anyhow::Result; use jsonwebtoken::{ - decode, encode, Algorithm, Algorithm::*, DecodingKey, EncodingKey, Header, TokenData, - Validation, + decode, encode, Algorithm, DecodingKey, EncodingKey, Header, TokenData, Validation, }; use serde::{Deserialize, Serialize}; use serde_with::{serde_as, DisplayFromStr}; use crate::id::TenantId; -/// Algorithms accepted during validation. -/// -/// Accept all RSA-based algorithms. We pass this list to jsonwebtoken::decode, -/// which checks that the algorithm in the token is one of these. -/// -/// XXX: It also fails the validation if there are any algorithms in this list that belong -/// to different family than the token's algorithm. In other words, we can *not* list any -/// non-RSA algorithms here, or the validation always fails with InvalidAlgorithm error. -const ACCEPTED_ALGORITHMS: &[Algorithm] = &[RS256, RS384, RS512]; - -/// Algorithm to use when generating a new token in [`encode_from_key_file`] -const ENCODE_ALGORITHM: Algorithm = Algorithm::RS256; +/// Algorithm to use. We require EdDSA. +const STORAGE_TOKEN_ALGORITHM: Algorithm = Algorithm::EdDSA; #[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] #[serde(rename_all = "lowercase")] @@ -69,7 +55,7 @@ pub struct JwtAuth { impl JwtAuth { pub fn new(decoding_key: DecodingKey) -> Self { let mut validation = Validation::default(); - validation.algorithms = ACCEPTED_ALGORITHMS.into(); + validation.algorithms = vec![STORAGE_TOKEN_ALGORITHM]; // The default 'required_spec_claims' is 'exp'. But we don't want to require // expiration. validation.required_spec_claims = [].into(); @@ -81,7 +67,7 @@ impl JwtAuth { pub fn from_key_path(key_path: &Path) -> Result { let public_key = fs::read(key_path)?; - Ok(Self::new(DecodingKey::from_rsa_pem(&public_key)?)) + Ok(Self::new(DecodingKey::from_ed_pem(&public_key)?)) } pub fn decode(&self, token: &str) -> Result> { @@ -99,8 +85,8 @@ impl std::fmt::Debug for JwtAuth { // this function is used only for testing purposes in CLI e g generate tokens during init pub fn encode_from_key_file(claims: &Claims, key_data: &[u8]) -> Result { - let key = EncodingKey::from_rsa_pem(key_data)?; - Ok(encode(&Header::new(ENCODE_ALGORITHM), claims, &key)?) + let key = EncodingKey::from_ed_pem(key_data)?; + Ok(encode(&Header::new(STORAGE_TOKEN_ALGORITHM), claims, &key)?) } #[cfg(test)] @@ -108,49 +94,19 @@ mod tests { use super::*; use std::str::FromStr; - // generated with: + // Generated with: // - // openssl genpkey -algorithm rsa -out storage-auth-priv.pem - // openssl pkey -in storage-auth-priv.pem -pubout -out storage-auth-pub.pem - const TEST_PUB_KEY_RSA: &[u8] = br#" + // openssl genpkey -algorithm ed25519 -out ed25519-priv.pem + // openssl pkey -in ed25519-priv.pem -pubout -out ed25519-pub.pem + const TEST_PUB_KEY_ED25519: &[u8] = br#" -----BEGIN PUBLIC KEY----- -MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAy6OZ+/kQXcueVJA/KTzO -v4ljxylc/Kcb0sXWuXg1GB8k3nDA1gK66LFYToH0aTnqrnqG32Vu6wrhwuvqsZA7 -jQvP0ZePAbWhpEqho7EpNunDPcxZ/XDy5TQlB1P58F9I3lkJXDC+DsHYLuuzwhAv -vo2MtWRdYlVHblCVLyZtANHhUMp2HUhgjHnJh5UrLIKOl4doCBxkM3rK0wjKsNCt -M92PCR6S9rvYzldfeAYFNppBkEQrXt2CgUqZ4KaS4LXtjTRUJxljijA4HWffhxsr -euRu3ufq8kVqie7fum0rdZZSkONmce0V0LesQ4aE2jB+2Sn48h6jb4dLXGWdq8TV -wQIDAQAB +MCowBQYDK2VwAyEARYwaNBayR+eGI0iXB4s3QxE3Nl2g1iWbr6KtLWeVD/w= -----END PUBLIC KEY----- "#; - const TEST_PRIV_KEY_RSA: &[u8] = br#" + + const TEST_PRIV_KEY_ED25519: &[u8] = br#" -----BEGIN PRIVATE KEY----- -MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQDLo5n7+RBdy55U -kD8pPM6/iWPHKVz8pxvSxda5eDUYHyTecMDWArrosVhOgfRpOequeobfZW7rCuHC -6+qxkDuNC8/Rl48BtaGkSqGjsSk26cM9zFn9cPLlNCUHU/nwX0jeWQlcML4Owdgu -67PCEC++jYy1ZF1iVUduUJUvJm0A0eFQynYdSGCMecmHlSssgo6Xh2gIHGQzesrT -CMqw0K0z3Y8JHpL2u9jOV194BgU2mkGQRCte3YKBSpngppLgte2NNFQnGWOKMDgd -Z9+HGyt65G7e5+ryRWqJ7t+6bSt1llKQ42Zx7RXQt6xDhoTaMH7ZKfjyHqNvh0tc -ZZ2rxNXBAgMBAAECggEAVz3u4Wlx3o02dsoZlSQs+xf0PEX3RXKeU+1YMbtTG9Nz -6yxpIQaoZrpbt76rJE2gwkFR+PEu1NmjoOuLb6j4KlQuI4AHz1auOoGSwFtM6e66 -K4aZ4x95oEJ3vqz2fkmEIWYJwYpMUmwvnuJx76kZm0xvROMLsu4QHS2+zCVtO5Tr -hvS05IMVuZ2TdQBZw0+JaFdwXbgDjQnQGY5n9MoTWSx1a4s/FF4Eby65BbDutcpn -Vt3jQAOmO1X2kbPeWSGuPJRzyUs7Kg8qfeglBIR3ppGP3vPYAdWX+ho00bmsVkSp -Q8vjul6C3WiM+kjwDxotHSDgbl/xldAl7OqPh0bfAQKBgQDnycXuq14Vg8nZvyn9 -rTnvucO8RBz5P6G+FZ+44cAS2x79+85onARmMnm+9MKYLSMo8fOvsK034NDI68XM -04QQ/vlfouvFklMTGJIurgEImTZbGCmlMYCvFyIxaEWixon8OpeI4rFe4Hmbiijh -PxhxWg221AwvBS2sco8J/ylEkQKBgQDg6Rh2QYb/j0Wou1rJPbuy3NhHofd5Rq35 -4YV3f2lfVYcPrgRhwe3T9SVII7Dx8LfwzsX5TAlf48ESlI3Dzv40uOCDM+xdtBRI -r96SfSm+jup6gsXU3AsdNkrRK3HoOG9Z/TkrUp213QAIlVnvIx65l4ckFMlpnPJ0 -lo1LDXZWMQKBgFArzjZ7N5OhfdO+9zszC3MLgdRAivT7OWqR+CjujIz5FYMr8Xzl -WfAvTUTrS9Nu6VZkObFvHrrRG+YjBsuN7YQjbQXTSFGSBwH34bgbn2fl9pMTjHQC -50uoaL9GHa/rlBaV/YvvPQJgCi/uXa1rMX0jdNLkDULGO8IF7cu7Yf7BAoGBAIUU -J29BkpmAst0GDs/ogTlyR18LTR0rXyHt+UUd1MGeH859TwZw80JpWWf4BmkB4DTS -hH3gKePdJY7S65ci0XNsuRupC4DeXuorde0DtkGU2tUmr9wlX0Ynq9lcdYfMbMa4 -eK1TsxG69JwfkxlWlIWITWRiEFM3lJa7xlrUWmLhAoGAFpKWF/hn4zYg3seU9gai -EYHKSbhxA4mRb+F0/9IlCBPMCqFrL5yftUsYIh2XFKn8+QhO97Nmk8wJSK6TzQ5t -ZaSRmgySrUUhx4nZ/MgqWCFv8VUbLM5MBzwxPKhXkSTfR4z2vLYLJwVY7Tb4kZtp -8ismApXVGHpOCstzikV9W7k= +MC4CAQAwBQYDK2VwBCIEID/Drmc1AA6U/znNRWpF3zEGegOATQxfkdWxitcOMsIH -----END PRIVATE KEY----- "#; @@ -161,8 +117,7 @@ ZaSRmgySrUUhx4nZ/MgqWCFv8VUbLM5MBzwxPKhXkSTfR4z2vLYLJwVY7Tb4kZtp scope: Scope::Tenant, }; - // Here are tokens containing the following payload, signed using TEST_PRIV_KEY_RSA - // using RS512, RS384 and RS256 algorithms: + // A test token containing the following payload, signed using TEST_PRIV_KEY_ED25519: // // ``` // { @@ -174,21 +129,13 @@ ZaSRmgySrUUhx4nZ/MgqWCFv8VUbLM5MBzwxPKhXkSTfR4z2vLYLJwVY7Tb4kZtp // } // ``` // - // These were encoded with the online debugger at https://jwt.io - // - let encoded_rs512 = "eyJhbGciOiJSUzUxMiIsInR5cCI6IkpXVCJ9.eyJzY29wZSI6InRlbmFudCIsInRlbmFudF9pZCI6IjNkMWY3NTk1YjQ2ODIzMDMwNGUwYjczY2VjYmNiMDgxIiwiaXNzIjoibmVvbi5jb250cm9scGxhbmUiLCJleHAiOjE3MDkyMDA4NzksImlhdCI6MTY3ODQ0MjQ3OX0.QmqfteDQmDGoxQ5EFkasbt35Lx0W0Nh63muQnYZvFq93DSh4ZbOG9Mc4yaiXZoiS5HgeKtFKv3mbWkDqjz3En06aY17hWwguBtAsGASX48lYeCPADYGlGAuaWnOnVRwe3iiOC7tvPFvwX_45S84X73sNUXyUiXv6nLdcDqVXudtNrGST_DnZDnjuUJX11w7sebtKqQQ8l9-iGHiXOl5yevpMCoB1OcTWcT6DfDtffoNuMHDC3fyhmEGG5oKAt1qBybqAIiyC9-UBAowRZXhdfxrzUl-I9jzKWvk85c5ulhVRwbPeP6TTTlPKwFzBNHg1i2U-1GONew5osQ3aoptwsA"; + let encoded_eddsa = "eyJhbGciOiJFZERTQSIsInR5cCI6IkpXVCJ9.eyJzY29wZSI6InRlbmFudCIsInRlbmFudF9pZCI6IjNkMWY3NTk1YjQ2ODIzMDMwNGUwYjczY2VjYmNiMDgxIiwiaXNzIjoibmVvbi5jb250cm9scGxhbmUiLCJleHAiOjE3MDkyMDA4NzksImlhdCI6MTY3ODQ0MjQ3OX0.U3eA8j-uU-JnhzeO3EDHRuXLwkAUFCPxtGHEgw6p7Ccc3YRbFs2tmCdbD9PZEXP-XsxSeBQi1FY0YPcT3NXADw"; - let encoded_rs384 = "eyJhbGciOiJSUzM4NCIsInR5cCI6IkpXVCJ9.eyJzY29wZSI6InRlbmFudCIsInRlbmFudF9pZCI6IjNkMWY3NTk1YjQ2ODIzMDMwNGUwYjczY2VjYmNiMDgxIiwiaXNzIjoibmVvbi5jb250cm9scGxhbmUiLCJleHAiOjE3MDkyMDA4NzksImlhdCI6MTY3ODQ0MjQ3OX0.qqk4nkxKzOJP38c_g57_w_SfdQVmCsDT_bsLmdFj_N6LIB22gr6U6_P_5mvk3pIAsp0VCTDwPrCU908TxqjibEkwvQoJwbogHamSGHpD7eJBxGblSnA-Nr3MlEMxpFtec8QokSm6C5mH7DoBYjB2xzeOlxAmpR2GAzInKiMkU4kZ_OcqqrmVcMXY_6VnbxZWMekuw56zE1-PP_qNF1HvYOH-P08ONP8qdo5UPtBG7QBEFlCqZXJZCFihQaI4Vzil9rDuZGCm3I7xQJ8-yh1PX3BTbGo8EzqLdRyBeTpr08UTuRbp_MJDWevHpP3afvJetAItqZXIoZQrbJjcByHqKw"; + // Check it can be validated with the public key + let auth = JwtAuth::new(DecodingKey::from_ed_pem(TEST_PUB_KEY_ED25519)?); + let claims_from_token = auth.decode(encoded_eddsa)?.claims; + assert_eq!(claims_from_token, expected_claims); - let encoded_rs256 = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzY29wZSI6InRlbmFudCIsInRlbmFudF9pZCI6IjNkMWY3NTk1YjQ2ODIzMDMwNGUwYjczY2VjYmNiMDgxIiwiaXNzIjoibmVvbi5jb250cm9scGxhbmUiLCJleHAiOjE3MDkyMDA4NzksImlhdCI6MTY3ODQ0MjQ3OX0.dF2N9KXG8ftFKHYbd5jQtXMQqv0Ej8FISGp1b_dmqOCotXj5S1y2AWjwyB_EXHM77JXfbEoJPAPrFFBNfd8cWtkCSTvpxWoHaecGzegDFGv5ZSc5AECFV1Daahc3PI3jii9wEiGkFOiwiBNfZ5INomOAsV--XXxlqIwKbTcgSYI7lrOTfecXAbAHiMKQlQYiIBSGnytRCgafhRkyGzPAL8ismthFJ9RHfeejyskht-9GbVHURw02bUyijuHEulpf9eEY3ZiB28de6jnCdU7ftIYaUMaYWt0nZQGkzxKPSfSLZNy14DTOYLDS04DVstWQPqnCUW_ojg0wJETOOfo9Zw"; - - // Check that RS512, RS384 and RS256 tokens can all be validated - let auth = JwtAuth::new(DecodingKey::from_rsa_pem(TEST_PUB_KEY_RSA)?); - - for encoded in [encoded_rs512, encoded_rs384, encoded_rs256] { - let claims_from_token = auth.decode(encoded)?.claims; - assert_eq!(claims_from_token, expected_claims); - } Ok(()) } @@ -199,10 +146,10 @@ ZaSRmgySrUUhx4nZ/MgqWCFv8VUbLM5MBzwxPKhXkSTfR4z2vLYLJwVY7Tb4kZtp scope: Scope::Tenant, }; - let encoded = encode_from_key_file(&claims, TEST_PRIV_KEY_RSA)?; + let encoded = encode_from_key_file(&claims, TEST_PRIV_KEY_ED25519)?; // decode it back - let auth = JwtAuth::new(DecodingKey::from_rsa_pem(TEST_PUB_KEY_RSA)?); + let auth = JwtAuth::new(DecodingKey::from_ed_pem(TEST_PUB_KEY_ED25519)?); let decoded = auth.decode(&encoded)?; assert_eq!(decoded.claims, claims); diff --git a/safekeeper/src/bin/safekeeper.rs b/safekeeper/src/bin/safekeeper.rs index 848b955af8..8966e8c49b 100644 --- a/safekeeper/src/bin/safekeeper.rs +++ b/safekeeper/src/bin/safekeeper.rs @@ -111,7 +111,7 @@ struct Args { /// WAL backup horizon. #[arg(long)] disable_wal_backup: bool, - /// Path to an RSA .pem public key which is used to check JWT tokens. + /// Path to a .pem public key which is used to check JWT tokens. #[arg(long)] auth_validation_public_key_path: Option, /// Format for logging, either 'plain' or 'json'. diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index cd4f0678b5..6429b1e940 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -431,7 +431,7 @@ class AuthKeys: priv: str def generate_token(self, *, scope: str, **token_data: str) -> str: - token = jwt.encode({"scope": scope, **token_data}, self.priv, algorithm="RS256") + token = jwt.encode({"scope": scope, **token_data}, self.priv, algorithm="EdDSA") # cast(Any, self.priv) # jwt.encode can return 'bytes' or 'str', depending on Python version or type From 881356c417c56344553763f0e5fffcc1760ef93e Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Mon, 20 Mar 2023 16:11:36 +0100 Subject: [PATCH 29/32] add metrics to detect eviction-induced thrashing (#3837) This patch adds two metrics that will enable us to detect *thrashing* of layers, i.e., repetitions of `eviction, on-demand-download, eviction, ... ` for a given layer. The first metric counts all layer evictions per timeline. It requires no further explanation. The second metric counts the layer evictions where the layer was resident for less than a given threshold. We can alert on increments to the second metric. The first metric will serve as a baseline, and further, it's generally interesting, outside of thrashing. The second metric's threshold is configurable in PageServerConf and defaults to 24h. The threshold value is reproduced as a label in the metric because the counter's value is semantically tied to that threshold. Since changes to the config and hence the label value are infrequent, this will have low storage overhead in the metrics storage. The data source to determine the time that the layer was resident is the file's `mtime`. Using `mtime` is more of a crutch. It would be better if Pageserver did its own persistent bookkeeping of residence change events instead of relying on the filesystem. We had some discussion about this: https://github.com/neondatabase/neon/pull/3809#issuecomment-1470448900 My position is that `mtime` is good enough for now. It can theoretically jump forward if someone copies files without resetting `mtime`. But that shouldn't happen in practice. Note that moving files back and forth doesn't change `mtime`, nor does `chown` or `chmod`. Lastly, `rsync -a`, which is typically used for filesystem-level backup / restore, correctly syncs `mtime`. I've added a label that identifies the data source to keep options open for a future, better data source than `mtime`. Since this value will stay the same for the time being, it's not a problem for metrics storage. refs https://github.com/neondatabase/neon/issues/3728 --- pageserver/src/config.rs | 34 ++++++++++ pageserver/src/metrics.rs | 108 +++++++++++++++++++++++++++++- pageserver/src/tenant/timeline.rs | 33 ++++++++- test_runner/fixtures/metrics.py | 2 + 4 files changed, 173 insertions(+), 4 deletions(-) diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs index 16f35355af..39282ce320 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -61,6 +61,7 @@ pub mod defaults { pub const DEFAULT_CACHED_METRIC_COLLECTION_INTERVAL: &str = "1 hour"; pub const DEFAULT_METRIC_COLLECTION_ENDPOINT: Option = None; pub const DEFAULT_SYNTHETIC_SIZE_CALCULATION_INTERVAL: &str = "10 min"; + pub const DEFAULT_EVICTIONS_LOW_RESIDENCE_DURATION_METRIC_THRESHOLD: &str = "24 hour"; /// /// Default built-in configuration file. @@ -89,6 +90,8 @@ pub mod defaults { #cached_metric_collection_interval = '{DEFAULT_CACHED_METRIC_COLLECTION_INTERVAL}' #synthetic_size_calculation_interval = '{DEFAULT_SYNTHETIC_SIZE_CALCULATION_INTERVAL}' +#evictions_low_residence_duration_metric_threshold = '{DEFAULT_EVICTIONS_LOW_RESIDENCE_DURATION_METRIC_THRESHOLD}' + # [tenant_config] #checkpoint_distance = {DEFAULT_CHECKPOINT_DISTANCE} # in bytes #checkpoint_timeout = {DEFAULT_CHECKPOINT_TIMEOUT} @@ -170,6 +173,9 @@ pub struct PageServerConf { pub metric_collection_endpoint: Option, pub synthetic_size_calculation_interval: Duration, + // See the corresponding metric's help string. + pub evictions_low_residence_duration_metric_threshold: Duration, + pub test_remote_failures: u64, pub ondemand_download_behavior_treat_error_as_warn: bool, @@ -240,6 +246,8 @@ struct PageServerConfigBuilder { metric_collection_endpoint: BuilderValue>, synthetic_size_calculation_interval: BuilderValue, + evictions_low_residence_duration_metric_threshold: BuilderValue, + test_remote_failures: BuilderValue, ondemand_download_behavior_treat_error_as_warn: BuilderValue, @@ -293,6 +301,11 @@ impl Default for PageServerConfigBuilder { .expect("cannot parse default synthetic size calculation interval")), metric_collection_endpoint: Set(DEFAULT_METRIC_COLLECTION_ENDPOINT), + evictions_low_residence_duration_metric_threshold: Set(humantime::parse_duration( + DEFAULT_EVICTIONS_LOW_RESIDENCE_DURATION_METRIC_THRESHOLD, + ) + .expect("cannot parse DEFAULT_EVICTIONS_LOW_RESIDENCE_DURATION_METRIC_THRESHOLD")), + test_remote_failures: Set(0), ondemand_download_behavior_treat_error_as_warn: Set(false), @@ -408,6 +421,10 @@ impl PageServerConfigBuilder { self.test_remote_failures = BuilderValue::Set(fail_first); } + pub fn evictions_low_residence_duration_metric_threshold(&mut self, value: Duration) { + self.evictions_low_residence_duration_metric_threshold = BuilderValue::Set(value); + } + pub fn ondemand_download_behavior_treat_error_as_warn( &mut self, ondemand_download_behavior_treat_error_as_warn: bool, @@ -481,6 +498,11 @@ impl PageServerConfigBuilder { synthetic_size_calculation_interval: self .synthetic_size_calculation_interval .ok_or(anyhow!("missing synthetic_size_calculation_interval"))?, + evictions_low_residence_duration_metric_threshold: self + .evictions_low_residence_duration_metric_threshold + .ok_or(anyhow!( + "missing evictions_low_residence_duration_metric_threshold" + ))?, test_remote_failures: self .test_remote_failures .ok_or(anyhow!("missing test_remote_failuers"))?, @@ -670,6 +692,7 @@ impl PageServerConf { "synthetic_size_calculation_interval" => builder.synthetic_size_calculation_interval(parse_toml_duration(key, item)?), "test_remote_failures" => builder.test_remote_failures(parse_toml_u64(key, item)?), + "evictions_low_residence_duration_metric_threshold" => builder.evictions_low_residence_duration_metric_threshold(parse_toml_duration(key, item)?), "ondemand_download_behavior_treat_error_as_warn" => builder.ondemand_download_behavior_treat_error_as_warn(parse_toml_bool(key, item)?), _ => bail!("unrecognized pageserver option '{key}'"), } @@ -810,6 +833,10 @@ impl PageServerConf { cached_metric_collection_interval: Duration::from_secs(60 * 60), metric_collection_endpoint: defaults::DEFAULT_METRIC_COLLECTION_ENDPOINT, synthetic_size_calculation_interval: Duration::from_secs(60), + evictions_low_residence_duration_metric_threshold: humantime::parse_duration( + defaults::DEFAULT_EVICTIONS_LOW_RESIDENCE_DURATION_METRIC_THRESHOLD, + ) + .unwrap(), test_remote_failures: 0, ondemand_download_behavior_treat_error_as_warn: false, } @@ -951,6 +978,9 @@ metric_collection_interval = '222 s' cached_metric_collection_interval = '22200 s' metric_collection_endpoint = 'http://localhost:80/metrics' synthetic_size_calculation_interval = '333 s' + +evictions_low_residence_duration_metric_threshold = '444 s' + log_format = 'json' "#; @@ -1005,6 +1035,9 @@ log_format = 'json' synthetic_size_calculation_interval: humantime::parse_duration( defaults::DEFAULT_SYNTHETIC_SIZE_CALCULATION_INTERVAL )?, + evictions_low_residence_duration_metric_threshold: humantime::parse_duration( + defaults::DEFAULT_EVICTIONS_LOW_RESIDENCE_DURATION_METRIC_THRESHOLD + )?, test_remote_failures: 0, ondemand_download_behavior_treat_error_as_warn: false, }, @@ -1056,6 +1089,7 @@ log_format = 'json' cached_metric_collection_interval: Duration::from_secs(22200), metric_collection_endpoint: Some(Url::parse("http://localhost:80/metrics")?), synthetic_size_calculation_interval: Duration::from_secs(333), + evictions_low_residence_duration_metric_threshold: Duration::from_secs(444), test_remote_failures: 0, ondemand_download_behavior_treat_error_as_warn: false, }, diff --git a/pageserver/src/metrics.rs b/pageserver/src/metrics.rs index 6a8aecfd25..b5563ad186 100644 --- a/pageserver/src/metrics.rs +++ b/pageserver/src/metrics.rs @@ -194,6 +194,93 @@ static PERSISTENT_BYTES_WRITTEN: Lazy = Lazy::new(|| { .expect("failed to define a metric") }); +static EVICTIONS: Lazy = Lazy::new(|| { + register_int_counter_vec!( + "pageserver_evictions", + "Number of layers evicted from the pageserver", + &["tenant_id", "timeline_id"] + ) + .expect("failed to define a metric") +}); + +static EVICTIONS_WITH_LOW_RESIDENCE_DURATION: Lazy = Lazy::new(|| { + register_int_counter_vec!( + "pageserver_evictions_with_low_residence_duration", + "If a layer is evicted that was resident for less than `low_threshold`, it is counted to this counter. \ + Residence duration is determined using the `residence_duration_data_source`.", + &["tenant_id", "timeline_id", "residence_duration_data_source", "low_threshold_secs"] + ) + .expect("failed to define a metric") +}); + +/// Each [`Timeline`]'s [`EVICTIONS_WITH_LOW_RESIDENCE_DURATION`] metric. +#[derive(Debug)] +pub struct EvictionsWithLowResidenceDuration { + data_source: &'static str, + threshold: Duration, + counter: Option, +} + +pub struct EvictionsWithLowResidenceDurationBuilder { + data_source: &'static str, + threshold: Duration, +} + +impl EvictionsWithLowResidenceDurationBuilder { + pub fn new(data_source: &'static str, threshold: Duration) -> Self { + Self { + data_source, + threshold, + } + } + + fn build(&self, tenant_id: &str, timeline_id: &str) -> EvictionsWithLowResidenceDuration { + let counter = EVICTIONS_WITH_LOW_RESIDENCE_DURATION + .get_metric_with_label_values(&[ + tenant_id, + timeline_id, + self.data_source, + &EvictionsWithLowResidenceDuration::threshold_label_value(self.threshold), + ]) + .unwrap(); + EvictionsWithLowResidenceDuration { + data_source: self.data_source, + threshold: self.threshold, + counter: Some(counter), + } + } +} + +impl EvictionsWithLowResidenceDuration { + fn threshold_label_value(threshold: Duration) -> String { + format!("{}", threshold.as_secs()) + } + + pub fn observe(&self, observed_value: Duration) { + if self.threshold < observed_value { + self.counter + .as_ref() + .expect("nobody calls this function after `remove_from_vec`") + .inc(); + } + } + + // This could be a `Drop` impl, but, we need the `tenant_id` and `timeline_id`. + fn remove(&mut self, tenant_id: &str, timeline_id: &str) { + let Some(_counter) = self.counter.take() else { + return; + }; + EVICTIONS_WITH_LOW_RESIDENCE_DURATION + .remove_label_values(&[ + tenant_id, + timeline_id, + self.data_source, + &Self::threshold_label_value(self.threshold), + ]) + .expect("we own the metric, no-one else should remove it"); + } +} + // Metrics collected on disk IO operations // // Roughly logarithmic scale. @@ -510,10 +597,16 @@ pub struct TimelineMetrics { pub current_logical_size_gauge: UIntGauge, pub num_persistent_files_created: IntCounter, pub persistent_bytes_written: IntCounter, + pub evictions: IntCounter, + pub evictions_with_low_residence_duration: EvictionsWithLowResidenceDuration, } impl TimelineMetrics { - pub fn new(tenant_id: &TenantId, timeline_id: &TimelineId) -> Self { + pub fn new( + tenant_id: &TenantId, + timeline_id: &TimelineId, + evictions_with_low_residence_duration_builder: EvictionsWithLowResidenceDurationBuilder, + ) -> Self { let tenant_id = tenant_id.to_string(); let timeline_id = timeline_id.to_string(); let reconstruct_time_histo = RECONSTRUCT_TIME @@ -550,6 +643,11 @@ impl TimelineMetrics { let persistent_bytes_written = PERSISTENT_BYTES_WRITTEN .get_metric_with_label_values(&[&tenant_id, &timeline_id]) .unwrap(); + let evictions = EVICTIONS + .get_metric_with_label_values(&[&tenant_id, &timeline_id]) + .unwrap(); + let evictions_with_low_residence_duration = + evictions_with_low_residence_duration_builder.build(&tenant_id, &timeline_id); TimelineMetrics { tenant_id, @@ -569,6 +667,8 @@ impl TimelineMetrics { current_logical_size_gauge, num_persistent_files_created, persistent_bytes_written, + evictions, + evictions_with_low_residence_duration, } } } @@ -585,7 +685,9 @@ impl Drop for TimelineMetrics { let _ = CURRENT_LOGICAL_SIZE.remove_label_values(&[tenant_id, timeline_id]); let _ = NUM_PERSISTENT_FILES_CREATED.remove_label_values(&[tenant_id, timeline_id]); let _ = PERSISTENT_BYTES_WRITTEN.remove_label_values(&[tenant_id, timeline_id]); - + let _ = EVICTIONS.remove_label_values(&[tenant_id, timeline_id]); + self.evictions_with_low_residence_duration + .remove(tenant_id, timeline_id); for op in STORAGE_TIME_OPERATIONS { let _ = STORAGE_TIME_SUM_PER_TIMELINE.remove_label_values(&[op, tenant_id, timeline_id]); @@ -620,7 +722,7 @@ use std::collections::HashMap; use std::pin::Pin; use std::sync::{Arc, Mutex}; use std::task::{Context, Poll}; -use std::time::Instant; +use std::time::{Duration, Instant}; pub struct RemoteTimelineClientMetrics { tenant_id: String, diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 0c42ed3079..f5dbe63b0b 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -1051,6 +1051,22 @@ impl Timeline { .file_size() .expect("Local layer should have a file size"); + let local_layer_mtime = local_layer + .local_path() + .expect("local layer should have a local path") + .metadata() + .context("get local layer file stat")? + .modified() + .context("get mtime of layer file")?; + let local_layer_residence_duration = + match SystemTime::now().duration_since(local_layer_mtime) { + Err(e) => { + warn!("layer mtime is in the future: {}", e); + None + } + Ok(delta) => Some(delta), + }; + let layer_metadata = LayerFileMetadata::new(layer_file_size); let new_remote_layer = Arc::new(match local_layer.filename() { @@ -1093,6 +1109,14 @@ impl Timeline { .resident_physical_size_gauge .sub(layer_file_size); + self.metrics.evictions.inc(); + + if let Some(delta) = local_layer_residence_duration { + self.metrics + .evictions_with_low_residence_duration + .observe(delta); + } + true } Replacement::NotFound => { @@ -1208,7 +1232,14 @@ impl Timeline { ancestor_timeline: ancestor, ancestor_lsn: metadata.ancestor_lsn(), - metrics: TimelineMetrics::new(&tenant_id, &timeline_id), + metrics: TimelineMetrics::new( + &tenant_id, + &timeline_id, + crate::metrics::EvictionsWithLowResidenceDurationBuilder::new( + "mtime", + conf.evictions_low_residence_duration_metric_threshold, + ), + ), flush_loop_state: Mutex::new(FlushLoopState::NotStarted), diff --git a/test_runner/fixtures/metrics.py b/test_runner/fixtures/metrics.py index 450c02735a..2984f2c7d3 100644 --- a/test_runner/fixtures/metrics.py +++ b/test_runner/fixtures/metrics.py @@ -78,5 +78,7 @@ PAGESERVER_PER_TENANT_METRICS: Tuple[str, ...] = ( "pageserver_created_persistent_files_total", "pageserver_written_persistent_bytes_total", "pageserver_tenant_states_count", + "pageserver_evictions_total", + "pageserver_evictions_with_low_residence_duration_total", *PAGESERVER_PER_TENANT_REMOTE_TIMELINE_CLIENT_METRICS, ) From 699f20081171c716ea35e1549294888f15fe6caf Mon Sep 17 00:00:00 2001 From: Arseny Sher Date: Mon, 20 Mar 2023 12:49:56 +0400 Subject: [PATCH 30/32] Send error context chain to the client when Copy stream errors. --- libs/postgres_backend/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/postgres_backend/src/lib.rs b/libs/postgres_backend/src/lib.rs index 4d88b958f0..60932a5950 100644 --- a/libs/postgres_backend/src/lib.rs +++ b/libs/postgres_backend/src/lib.rs @@ -767,7 +767,7 @@ impl PostgresBackend { let err_to_send_and_errcode = match &end { ServerInitiated(_) => Some((end.to_string(), SQLSTATE_SUCCESSFUL_COMPLETION)), - Other(_) => Some((end.to_string(), SQLSTATE_INTERNAL_ERROR)), + Other(_) => Some((format!("{end:#}"), SQLSTATE_INTERNAL_ERROR)), // Note: CopyFail in duplex copy is somewhat unexpected (at least to // PG walsender; evidently and per my docs reading client should // finish it with CopyDone). It is not a problem to recover from it From 5a786fab4f2877bc94f47ff2e369efd081baf846 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Mon, 20 Mar 2023 20:51:32 +0200 Subject: [PATCH 31/32] Remove duplicated global variables in neon extension. Walproposer used to live in the backend, while pagestore_smgr was an extension. But now that both are part of the neon extension, walproposer can access the same 'neon_tenant' and 'neon_timeline' variables as the pageserver_smgr code. --- pgxn/neon/libpagestore.c | 8 ++------ pgxn/neon/neon.h | 5 +++++ pgxn/neon/walproposer.c | 28 ++++++++++++---------------- pgxn/neon/walproposer.h | 4 ---- 4 files changed, 19 insertions(+), 26 deletions(-) diff --git a/pgxn/neon/libpagestore.c b/pgxn/neon/libpagestore.c index 3fe6d38251..a3f34247bb 100644 --- a/pgxn/neon/libpagestore.c +++ b/pgxn/neon/libpagestore.c @@ -536,10 +536,6 @@ pg_init_libpagestore(void) /* substitute password in pageserver_connstring */ page_server_connstring = substitute_pageserver_password(page_server_connstring_raw); - /* Is there more correct way to pass CustomGUC to postgres code? */ - neon_timeline_walproposer = neon_timeline; - neon_tenant_walproposer = neon_tenant; - /* retrieve the token for Safekeeper, if present */ if (safekeeper_token_env != NULL) { if (safekeeper_token_env[0] != '$') { @@ -548,8 +544,8 @@ pg_init_libpagestore(void) errmsg("expected safekeeper auth token environment variable's name starting with $ but found: %s", safekeeper_token_env))); } - neon_safekeeper_token_walproposer = getenv(&safekeeper_token_env[1]); - if (!neon_safekeeper_token_walproposer) { + neon_safekeeper_token = getenv(&safekeeper_token_env[1]); + if (!neon_safekeeper_token) { ereport(ERROR, (errcode(ERRCODE_CONNECTION_EXCEPTION), errmsg("cannot get safekeeper auth token, environment variable %s is not set", diff --git a/pgxn/neon/neon.h b/pgxn/neon/neon.h index 6b9ba372fb..da441b783d 100644 --- a/pgxn/neon/neon.h +++ b/pgxn/neon/neon.h @@ -12,6 +12,11 @@ #ifndef NEON_H #define NEON_H +/* GUCs */ +extern char *neon_safekeeper_token; +extern char *neon_timeline; +extern char *neon_tenant; + extern void pg_init_libpagestore(void); extern void pg_init_walproposer(void); diff --git a/pgxn/neon/walproposer.c b/pgxn/neon/walproposer.c index bf8bb02493..aef2465e54 100644 --- a/pgxn/neon/walproposer.c +++ b/pgxn/neon/walproposer.c @@ -78,10 +78,6 @@ int wal_acceptor_reconnect_timeout; int wal_acceptor_connection_timeout; bool am_wal_proposer; -char *neon_timeline_walproposer = NULL; -char *neon_tenant_walproposer = NULL; -char *neon_safekeeper_token_walproposer = NULL; - #define WAL_PROPOSER_SLOT_NAME "wal_proposer_slot" static int n_safekeepers = 0; @@ -514,15 +510,15 @@ WalProposerInit(XLogRecPtr flushRecPtr, uint64 systemId) Safekeeper *sk = &safekeeper[n_safekeepers]; int written = 0; - if (neon_safekeeper_token_walproposer != NULL) { + if (neon_safekeeper_token != NULL) { written = snprintf((char *) &sk->conninfo, MAXCONNINFO, "host=%s port=%s password=%s dbname=replication options='-c timeline_id=%s tenant_id=%s'", - sk->host, sk->port, neon_safekeeper_token_walproposer, neon_timeline_walproposer, - neon_tenant_walproposer); + sk->host, sk->port, neon_safekeeper_token, neon_timeline, + neon_tenant); } else { written = snprintf((char *) &sk->conninfo, MAXCONNINFO, "host=%s port=%s dbname=replication options='-c timeline_id=%s tenant_id=%s'", - sk->host, sk->port, neon_timeline_walproposer, neon_tenant_walproposer); + sk->host, sk->port, neon_timeline, neon_tenant); } if (written > MAXCONNINFO || written < 0) @@ -550,16 +546,16 @@ WalProposerInit(XLogRecPtr flushRecPtr, uint64 systemId) greetRequest.pgVersion = PG_VERSION_NUM; pg_strong_random(&greetRequest.proposerId, sizeof(greetRequest.proposerId)); greetRequest.systemId = systemId; - if (!neon_timeline_walproposer) + if (!neon_timeline) elog(FATAL, "neon.timeline_id is not provided"); - if (*neon_timeline_walproposer != '\0' && - !HexDecodeString(greetRequest.timeline_id, neon_timeline_walproposer, 16)) - elog(FATAL, "Could not parse neon.timeline_id, %s", neon_timeline_walproposer); - if (!neon_tenant_walproposer) + if (*neon_timeline != '\0' && + !HexDecodeString(greetRequest.timeline_id, neon_timeline, 16)) + elog(FATAL, "Could not parse neon.timeline_id, %s", neon_timeline); + if (!neon_tenant) elog(FATAL, "neon.tenant_id is not provided"); - if (*neon_tenant_walproposer != '\0' && - !HexDecodeString(greetRequest.tenant_id, neon_tenant_walproposer, 16)) - elog(FATAL, "Could not parse neon.tenant_id, %s", neon_tenant_walproposer); + if (*neon_tenant != '\0' && + !HexDecodeString(greetRequest.tenant_id, neon_tenant, 16)) + elog(FATAL, "Could not parse neon.tenant_id, %s", neon_tenant); #if PG_VERSION_NUM >= 150000 /* FIXME don't use hardcoded timeline id */ diff --git a/pgxn/neon/walproposer.h b/pgxn/neon/walproposer.h index 1abaab2cc6..357d6378f8 100644 --- a/pgxn/neon/walproposer.h +++ b/pgxn/neon/walproposer.h @@ -39,10 +39,6 @@ typedef struct WalProposerConn WalProposerConn; struct WalMessage; typedef struct WalMessage WalMessage; -extern char *neon_timeline_walproposer; -extern char *neon_tenant_walproposer; -extern char *neon_safekeeper_token_walproposer; - /* Possible return values from ReadPGAsync */ typedef enum { From 299db9d0288d28bf6b8205140ce443c5c54e215d Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Mon, 20 Mar 2023 20:51:36 +0200 Subject: [PATCH 32/32] Simplify and clean up the $NEON_AUTH_TOKEN stuff in compute - Remove the neon.safekeeper_token_env GUC. It was used to set the name of an environment variable, which was then used in pageserver and safekeeper connection strings to in place of the password. Instead, always look up the environment variable called NEON_AUTH_TOKEN. That's what neon.safekeeper_token_env was always set to in practice, and I don't see the need for the extra level of indirection or configurability. - Instead of substituting $NEON_AUTH_TOKEN in the connection strings, pass $NEON_AUTH_TOKEN "out-of-band" as the password, when we connect to the pageserver or safekeepers. That's simpler. - Also use the password from $NEON_AUTH_TOKEN in compute_ctl, when it connects to the pageserver to get the "base backup". --- compute_tools/src/compute.rs | 18 +++- control_plane/src/compute.rs | 28 ++---- docs/authentication.md | 26 +++--- pgxn/neon/libpagestore.c | 173 ++++++++--------------------------- pgxn/neon/libpqwalproposer.c | 31 ++++++- pgxn/neon/neon.h | 2 +- pgxn/neon/pagestore_smgr.c | 8 -- pgxn/neon/walproposer.c | 16 +--- pgxn/neon/walproposer.h | 2 +- 9 files changed, 112 insertions(+), 192 deletions(-) diff --git a/compute_tools/src/compute.rs b/compute_tools/src/compute.rs index 8ceef44d61..09272262de 100644 --- a/compute_tools/src/compute.rs +++ b/compute_tools/src/compute.rs @@ -18,6 +18,7 @@ use std::fs; use std::os::unix::fs::PermissionsExt; use std::path::Path; use std::process::{Command, Stdio}; +use std::str::FromStr; use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::RwLock; @@ -126,7 +127,22 @@ impl ComputeNode { fn get_basebackup(&self, lsn: &str) -> Result<()> { let start_time = Utc::now(); - let mut client = Client::connect(&self.pageserver_connstr, NoTls)?; + let mut config = postgres::Config::from_str(&self.pageserver_connstr)?; + + // Like in the neon extension, if the $NEON_AUTH_TOKEN env variable is + // set, use it as the password when connecting to pageserver. + // + // Note: this overrides any password set in the connection string. + match std::env::var("NEON_AUTH_TOKEN") { + Ok(val) => { + info!("Got pageserver auth token from NEON_AUTH_TOKEN env variable"); + config.password(val); + } + Err(std::env::VarError::NotPresent) => info!("NEON_AUTH_TOKEN env variable not set"), + Err(e) => info!("could not parse NEON_AUTH_TOKEN env variable: {}", e), + }; + + let mut client = config.connect(NoTls)?; let basebackup_cmd = match lsn { "0/0" => format!("basebackup {} {}", &self.tenant, &self.timeline), // First start of the compute _ => format!("basebackup {} {} {}", &self.tenant, &self.timeline, lsn), diff --git a/control_plane/src/compute.rs b/control_plane/src/compute.rs index 094d2add8d..730cacf40b 100644 --- a/control_plane/src/compute.rs +++ b/control_plane/src/compute.rs @@ -11,7 +11,6 @@ use std::sync::Arc; use std::time::Duration; use anyhow::{Context, Result}; -use postgres_backend::AuthType; use utils::{ id::{TenantId, TimelineId}, lsn::Lsn, @@ -97,7 +96,7 @@ impl ComputeControlPlane { }); node.create_pgdata()?; - node.setup_pg_conf(self.env.pageserver.pg_auth_type)?; + node.setup_pg_conf()?; self.nodes .insert((tenant_id, node.name.clone()), Arc::clone(&node)); @@ -278,7 +277,7 @@ impl PostgresNode { // Write postgresql.conf with default configuration // and PG_VERSION file to the data directory of a new node. - fn setup_pg_conf(&self, auth_type: AuthType) -> Result<()> { + fn setup_pg_conf(&self) -> Result<()> { let mut conf = PostgresConf::new(); conf.append("max_wal_senders", "10"); conf.append("wal_log_hints", "off"); @@ -302,29 +301,12 @@ impl PostgresNode { let config = &self.pageserver.pg_connection_config; let (host, port) = (config.host(), config.port()); - // Set up authentication - // - // $NEON_AUTH_TOKEN will be replaced with value from environment - // variable during compute pg startup. It is done this way because - // otherwise user will be able to retrieve the value using SHOW - // command or pg_settings - let password = if let AuthType::NeonJWT = auth_type { - "$NEON_AUTH_TOKEN" - } else { - "" - }; - // NOTE avoiding spaces in connection string, because it is less error prone if we forward it somewhere. - // Also note that not all parameters are supported here. Because in compute we substitute $NEON_AUTH_TOKEN - // We parse this string and build it back with token from env var, and for simplicity rebuild - // uses only needed variables namely host, port, user, password. - format!("postgresql://no_user:{password}@{host}:{port}") + // NOTE: avoid spaces in connection string, because it is less error prone if we forward it somewhere. + format!("postgresql://no_user@{host}:{port}") }; conf.append("shared_preload_libraries", "neon"); conf.append_line(""); conf.append("neon.pageserver_connstring", &pageserver_connstr); - if let AuthType::NeonJWT = auth_type { - conf.append("neon.safekeeper_token_env", "$NEON_AUTH_TOKEN"); - } conf.append("neon.tenant_id", &self.tenant_id.to_string()); conf.append("neon.timeline_id", &self.timeline_id.to_string()); if let Some(lsn) = self.lsn { @@ -447,6 +429,8 @@ impl PostgresNode { "DYLD_LIBRARY_PATH", self.env.pg_lib_dir(self.pg_version)?.to_str().unwrap(), ); + + // Pass authentication token used for the connections to pageserver and safekeepers if let Some(token) = auth_token { cmd.env("NEON_AUTH_TOKEN", token); } diff --git a/docs/authentication.md b/docs/authentication.md index dc402d1bca..f768b04c5b 100644 --- a/docs/authentication.md +++ b/docs/authentication.md @@ -106,20 +106,22 @@ Their authentication is just plain PostgreSQL authentication and out of scope fo There is no administrative API except those provided by PostgreSQL. #### Outgoing connections -Compute connects to Pageserver for getting pages. -The connection string is configured by the `neon.pageserver_connstring` PostgreSQL GUC, e.g. `postgresql://no_user:$NEON_AUTH_TOKEN@localhost:15028`. -The environment variable inside the connection string is substituted with -the JWT token. +Compute connects to Pageserver for getting pages. The connection string is +configured by the `neon.pageserver_connstring` PostgreSQL GUC, +e.g. `postgresql://no_user@localhost:15028`. If the `$NEON_AUTH_TOKEN` +environment variable is set, it is used as the password for the connection. (The +pageserver uses JWT tokens for authentication, so the password is really a +token.) -Compute connects to Safekeepers to write and commit data. -The token is the same for all safekeepers. -It's stored in an environment variable, whose name is configured -by the `neon.safekeeper_token_env` PostgreSQL GUC. -If the GUC is unset, no token is passed. +Compute connects to Safekeepers to write and commit data. The list of safekeeper +addresses is given in the `neon.safekeepers` GUC. The connections to the +safekeepers take the password from the `$NEON_AUTH_TOKEN` environment +variable, if set. -Note that both tokens can be (and typically are) the same; -the scope is the tenant and the token is usually passed through the -`$NEON_AUTH_TOKEN` environment variable. +The `compute_ctl` binary that runs before the PostgreSQL server, and launches +PostgreSQL, also makes a connection to the pageserver. It uses it to fetch the +initial "base backup" dump, to initialize the PostgreSQL data directory. It also +uses `$NEON_AUTH_TOKEN` as the password for the connection. ### Pageserver #### Overview diff --git a/pgxn/neon/libpagestore.c b/pgxn/neon/libpagestore.c index a3f34247bb..c44e8fcda5 100644 --- a/pgxn/neon/libpagestore.c +++ b/pgxn/neon/libpagestore.c @@ -46,8 +46,12 @@ PGconn *pageserver_conn = NULL; */ WaitEventSet *pageserver_conn_wes = NULL; -char *page_server_connstring_raw; -char *safekeeper_token_env; +/* GUCs */ +char *neon_timeline; +char *neon_tenant; +int32 max_cluster_size; +char *page_server_connstring; +char *neon_auth_token; int n_unflushed_requests = 0; int flush_every_n_requests = 8; @@ -60,10 +64,37 @@ pageserver_connect(int elevel) { char *query; int ret; + const char *keywords[3]; + const char *values[3]; + int n; Assert(!connected); - pageserver_conn = PQconnectdb(page_server_connstring); + /* + * Connect using the connection string we got from the + * neon.pageserver_connstring GUC. If the NEON_AUTH_TOKEN environment + * variable was set, use that as the password. + * + * The connection options are parsed in the order they're given, so + * when we set the password before the connection string, the + * connection string can override the password from the env variable. + * Seems useful, although we don't currently use that capability + * anywhere. + */ + n = 0; + if (neon_auth_token) + { + keywords[n] = "password"; + values[n] = neon_auth_token; + n++; + } + keywords[n] = "dbname"; + values[n] = page_server_connstring; + n++; + keywords[n] = NULL; + values[n] = NULL; + n++; + pageserver_conn = PQconnectdbParams(keywords, values, 1); if (PQstatus(pageserver_conn) == CONNECTION_BAD) { @@ -125,7 +156,7 @@ pageserver_connect(int elevel) } } - neon_log(LOG, "libpagestore: connected to '%s'", page_server_connstring_raw); + neon_log(LOG, "libpagestore: connected to '%s'", page_server_connstring); connected = true; return true; @@ -354,105 +385,6 @@ check_neon_id(char **newval, void **extra, GucSource source) return **newval == '\0' || HexDecodeString(id, *newval, 16); } -static char * -substitute_pageserver_password(const char *page_server_connstring_raw) -{ - char *host = NULL; - char *port = NULL; - char *user = NULL; - char *auth_token = NULL; - char *err = NULL; - char *page_server_connstring = NULL; - PQconninfoOption *conn_options; - PQconninfoOption *conn_option; - MemoryContext oldcontext; - - /* - * Here we substitute password in connection string with an environment - * variable. To simplify things we construct a connection string back with - * only known options. In particular: host port user and password. We do - * not currently use other options and constructing full connstring in an - * URI shape is quite messy. - */ - - if (page_server_connstring_raw == NULL || page_server_connstring_raw[0] == '\0') - return NULL; - - /* extract the auth token from the connection string */ - conn_options = PQconninfoParse(page_server_connstring_raw, &err); - if (conn_options == NULL) - { - /* The error string is malloc'd, so we must free it explicitly */ - char *errcopy = err ? pstrdup(err) : "out of memory"; - - PQfreemem(err); - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("invalid connection string syntax: %s", errcopy))); - } - - /* - * Trying to populate pageserver connection string with auth token from - * environment. We are looking for password in with placeholder value like - * $ENV_VAR_NAME, so if password field is present and starts with $ we try - * to fetch environment variable value and fail loudly if it is not set. - */ - for (conn_option = conn_options; conn_option->keyword != NULL; conn_option++) - { - if (strcmp(conn_option->keyword, "host") == 0) - { - if (conn_option->val != NULL && conn_option->val[0] != '\0') - host = conn_option->val; - } - else if (strcmp(conn_option->keyword, "port") == 0) - { - if (conn_option->val != NULL && conn_option->val[0] != '\0') - port = conn_option->val; - } - else if (strcmp(conn_option->keyword, "user") == 0) - { - if (conn_option->val != NULL && conn_option->val[0] != '\0') - user = conn_option->val; - } - else if (strcmp(conn_option->keyword, "password") == 0) - { - if (conn_option->val != NULL && conn_option->val[0] != '\0') - { - /* ensure that this is a template */ - if (strncmp(conn_option->val, "$", 1) != 0) - ereport(ERROR, - (errcode(ERRCODE_CONNECTION_EXCEPTION), - errmsg("expected placeholder value in pageserver password starting from $ but found: %s", &conn_option->val[1]))); - - neon_log(LOG, "found auth token placeholder in pageserver conn string '%s'", &conn_option->val[1]); - auth_token = getenv(&conn_option->val[1]); - if (!auth_token) - { - ereport(ERROR, - (errcode(ERRCODE_CONNECTION_EXCEPTION), - errmsg("cannot get auth token, environment variable %s is not set", &conn_option->val[1]))); - } - else - { - neon_log(LOG, "using auth token from environment passed via env"); - } - } - } - } - - /* - * allocate connection string in TopMemoryContext to make sure it is not - * freed - */ - oldcontext = CurrentMemoryContext; - MemoryContextSwitchTo(TopMemoryContext); - page_server_connstring = psprintf("postgresql://%s:%s@%s:%s", user, auth_token ? auth_token : "", host, port); - MemoryContextSwitchTo(oldcontext); - - PQconninfoFree(conn_options); - return page_server_connstring; -} - /* * Module initialization function */ @@ -462,21 +394,12 @@ pg_init_libpagestore(void) DefineCustomStringVariable("neon.pageserver_connstring", "connection string to the page server", NULL, - &page_server_connstring_raw, + &page_server_connstring, "", PGC_POSTMASTER, 0, /* no flags required */ NULL, NULL, NULL); - DefineCustomStringVariable("neon.safekeeper_token_env", - "the environment variable containing JWT token for authentication with Safekeepers, the convention is to either unset or set to $NEON_AUTH_TOKEN", - NULL, - &safekeeper_token_env, - NULL, - PGC_POSTMASTER, - 0, /* no flags required */ - NULL, NULL, NULL); - DefineCustomStringVariable("neon.timeline_id", "Neon timeline_id the server is running on", NULL, @@ -533,26 +456,10 @@ pg_init_libpagestore(void) neon_log(PageStoreTrace, "libpagestore already loaded"); page_server = &api; - /* substitute password in pageserver_connstring */ - page_server_connstring = substitute_pageserver_password(page_server_connstring_raw); - - /* retrieve the token for Safekeeper, if present */ - if (safekeeper_token_env != NULL) { - if (safekeeper_token_env[0] != '$') { - ereport(ERROR, - (errcode(ERRCODE_CONNECTION_EXCEPTION), - errmsg("expected safekeeper auth token environment variable's name starting with $ but found: %s", - safekeeper_token_env))); - } - neon_safekeeper_token = getenv(&safekeeper_token_env[1]); - if (!neon_safekeeper_token) { - ereport(ERROR, - (errcode(ERRCODE_CONNECTION_EXCEPTION), - errmsg("cannot get safekeeper auth token, environment variable %s is not set", - &safekeeper_token_env[1]))); - } - neon_log(LOG, "using safekeeper auth token from environment variable"); - } + /* Retrieve the auth token to use when connecting to pageserver and safekeepers */ + neon_auth_token = getenv("NEON_AUTH_TOKEN"); + if (neon_auth_token) + neon_log(LOG, "using storage auth token from NEON_AUTH_TOKEN environment variable"); if (page_server_connstring && page_server_connstring[0]) { diff --git a/pgxn/neon/libpqwalproposer.c b/pgxn/neon/libpqwalproposer.c index 6b1e6a8bcc..9b6175a621 100644 --- a/pgxn/neon/libpqwalproposer.c +++ b/pgxn/neon/libpqwalproposer.c @@ -51,12 +51,39 @@ walprop_status(WalProposerConn *conn) } WalProposerConn * -walprop_connect_start(char *conninfo) +walprop_connect_start(char *conninfo, char *password) { WalProposerConn *conn; PGconn *pg_conn; + const char *keywords[3]; + const char *values[3]; + int n; - pg_conn = PQconnectStart(conninfo); + /* + * Connect using the given connection string. If the + * NEON_AUTH_TOKEN environment variable was set, use that as + * the password. + * + * The connection options are parsed in the order they're given, so + * when we set the password before the connection string, the + * connection string can override the password from the env variable. + * Seems useful, although we don't currently use that capability + * anywhere. + */ + n = 0; + if (password) + { + keywords[n] = "password"; + values[n] = neon_auth_token; + n++; + } + keywords[n] = "dbname"; + values[n] = conninfo; + n++; + keywords[n] = NULL; + values[n] = NULL; + n++; + pg_conn = PQconnectStartParams(keywords, values, 1); /* * Allocation of a PQconn can fail, and will return NULL. We want to fully diff --git a/pgxn/neon/neon.h b/pgxn/neon/neon.h index da441b783d..3eac8f4570 100644 --- a/pgxn/neon/neon.h +++ b/pgxn/neon/neon.h @@ -13,7 +13,7 @@ #define NEON_H /* GUCs */ -extern char *neon_safekeeper_token; +extern char *neon_auth_token; extern char *neon_timeline; extern char *neon_tenant; diff --git a/pgxn/neon/pagestore_smgr.c b/pgxn/neon/pagestore_smgr.c index ca91112195..5b30641856 100644 --- a/pgxn/neon/pagestore_smgr.c +++ b/pgxn/neon/pagestore_smgr.c @@ -92,14 +92,6 @@ const int SmgrTrace = DEBUG5; page_server_api *page_server; -/* GUCs */ -char *page_server_connstring; - -/*with substituted password*/ -char *neon_timeline; -char *neon_tenant; -int32 max_cluster_size; - /* unlogged relation build states */ typedef enum { diff --git a/pgxn/neon/walproposer.c b/pgxn/neon/walproposer.c index aef2465e54..b0b2a23e3c 100644 --- a/pgxn/neon/walproposer.c +++ b/pgxn/neon/walproposer.c @@ -510,17 +510,9 @@ WalProposerInit(XLogRecPtr flushRecPtr, uint64 systemId) Safekeeper *sk = &safekeeper[n_safekeepers]; int written = 0; - if (neon_safekeeper_token != NULL) { - written = snprintf((char *) &sk->conninfo, MAXCONNINFO, - "host=%s port=%s password=%s dbname=replication options='-c timeline_id=%s tenant_id=%s'", - sk->host, sk->port, neon_safekeeper_token, neon_timeline, - neon_tenant); - } else { - written = snprintf((char *) &sk->conninfo, MAXCONNINFO, - "host=%s port=%s dbname=replication options='-c timeline_id=%s tenant_id=%s'", - sk->host, sk->port, neon_timeline, neon_tenant); - } - + written = snprintf((char *) &sk->conninfo, MAXCONNINFO, + "host=%s port=%s dbname=replication options='-c timeline_id=%s tenant_id=%s'", + sk->host, sk->port, neon_timeline, neon_tenant); if (written > MAXCONNINFO || written < 0) elog(FATAL, "could not create connection string for safekeeper %s:%s", sk->host, sk->port); } @@ -696,7 +688,7 @@ ResetConnection(Safekeeper *sk) /* * Try to establish new connection */ - sk->conn = walprop_connect_start((char *) &sk->conninfo); + sk->conn = walprop_connect_start((char *) &sk->conninfo, neon_auth_token); /* * "If the result is null, then libpq has been unable to allocate a new diff --git a/pgxn/neon/walproposer.h b/pgxn/neon/walproposer.h index 357d6378f8..537c733850 100644 --- a/pgxn/neon/walproposer.h +++ b/pgxn/neon/walproposer.h @@ -454,7 +454,7 @@ extern char *walprop_error_message(WalProposerConn *conn); extern WalProposerConnStatusType walprop_status(WalProposerConn *conn); /* Re-exported PQconnectStart */ -extern WalProposerConn * walprop_connect_start(char *conninfo); +extern WalProposerConn * walprop_connect_start(char *conninfo, char *password); /* Re-exported PQconectPoll */ extern WalProposerConnectPollStatusType walprop_connect_poll(WalProposerConn *conn);