From b4d36f572df0802da16dba902e678164277076d1 Mon Sep 17 00:00:00 2001 From: Alexander Bayandin Date: Tue, 18 Jul 2023 13:50:44 +0100 Subject: [PATCH 01/23] Use sharded-slab from crates (#4729) ## Problem We use a patched version of `sharded-slab` with increased MAX_THREADS [1]. It is not required anymore because safekeepers are async now. A valid comment from the original PR tho [1]: > Note that patch can affect other rust services, not only the safekeeper binary. - [1] https://github.com/neondatabase/neon/pull/4122 ## Summary of changes - Remove patch for `sharded-slab` --- Cargo.lock | 3 ++- Cargo.toml | 5 ----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b5f6b3b328..3c862241a4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3854,7 +3854,8 @@ dependencies = [ [[package]] name = "sharded-slab" version = "0.1.4" -source = "git+https://github.com/neondatabase/sharded-slab.git?rev=98d16753ab01c61f0a028de44167307a00efea00#98d16753ab01c61f0a028de44167307a00efea00" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "900fba806f70c630b0a382d0d825e17a0f19fcd059a2ade1ff237bcddf446b31" dependencies = [ "lazy_static", ] diff --git a/Cargo.toml b/Cargo.toml index 01e2fe7cf9..44d49d95e8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -185,11 +185,6 @@ tonic-build = "0.9" # TODO: we should probably fork `tokio-postgres-rustls` instead. tokio-postgres = { git = "https://github.com/neondatabase/rust-postgres.git", rev="1aaedab101b23f7612042850d8f2036810fa7c7f" } -# Changes the MAX_THREADS limit from 4096 to 32768. -# This is a temporary workaround for using tracing from many threads in safekeepers code, -# until async safekeepers patch is merged to the main. -sharded-slab = { git = "https://github.com/neondatabase/sharded-slab.git", rev="98d16753ab01c61f0a028de44167307a00efea00" } - ################# Binary contents sections [profile.release] From 1e7db5458f90246ff2088727c261e8f8ad97143e Mon Sep 17 00:00:00 2001 From: Arseny Sher Date: Mon, 17 Jul 2023 12:18:04 +0300 Subject: [PATCH 02/23] Add one more WAL service port allowing only tenant scoped auth tokens. It will make it easier to limit access at network level, with e.g. k8s network policies. ref https://github.com/neondatabase/neon/issues/4730 --- libs/utils/src/auth.rs | 2 +- safekeeper/src/bin/safekeeper.rs | 46 +++++++++++++++++++++++++++++--- safekeeper/src/handler.rs | 20 +++++++++++++- safekeeper/src/lib.rs | 2 ++ safekeeper/src/wal_service.rs | 14 +++++++--- 5 files changed, 75 insertions(+), 9 deletions(-) diff --git a/libs/utils/src/auth.rs b/libs/utils/src/auth.rs index 0fb45e01c6..716984b64e 100644 --- a/libs/utils/src/auth.rs +++ b/libs/utils/src/auth.rs @@ -16,7 +16,7 @@ use crate::id::TenantId; /// Algorithm to use. We require EdDSA. const STORAGE_TOKEN_ALGORITHM: Algorithm = Algorithm::EdDSA; -#[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] +#[derive(Debug, Serialize, Deserialize, Clone, Copy, PartialEq)] #[serde(rename_all = "lowercase")] pub enum Scope { // Provides access to all data for a specific tenant (specified in `struct Claims` below) diff --git a/safekeeper/src/bin/safekeeper.rs b/safekeeper/src/bin/safekeeper.rs index 0625538bf3..abede2e44d 100644 --- a/safekeeper/src/bin/safekeeper.rs +++ b/safekeeper/src/bin/safekeeper.rs @@ -37,7 +37,7 @@ use safekeeper::{http, WAL_REMOVER_RUNTIME}; use safekeeper::{remove_wal, WAL_BACKUP_RUNTIME}; use safekeeper::{wal_backup, HTTP_RUNTIME}; use storage_broker::DEFAULT_ENDPOINT; -use utils::auth::JwtAuth; +use utils::auth::{JwtAuth, Scope}; use utils::{ id::NodeId, logging::{self, LogFormat}, @@ -72,6 +72,10 @@ struct Args { /// Listen endpoint for receiving/sending WAL in the form host:port. #[arg(short, long, default_value = DEFAULT_PG_LISTEN_ADDR)] listen_pg: String, + /// Listen endpoint for receiving/sending WAL in the form host:port allowing + /// only tenant scoped auth tokens. Pointless if auth is disabled. + #[arg(long, default_value = None, verbatim_doc_comment)] + listen_pg_tenant_only: Option, /// Listen http endpoint for management and metrics in the form host:port. #[arg(long, default_value = DEFAULT_HTTP_LISTEN_ADDR)] listen_http: String, @@ -94,7 +98,7 @@ struct Args { broker_keepalive_interval: Duration, /// Peer safekeeper is considered dead after not receiving heartbeats from /// it during this period passed as a human readable duration. - #[arg(long, value_parser= humantime::parse_duration, default_value = DEFAULT_HEARTBEAT_TIMEOUT)] + #[arg(long, value_parser= humantime::parse_duration, default_value = DEFAULT_HEARTBEAT_TIMEOUT, verbatim_doc_comment)] heartbeat_timeout: Duration, /// Remote storage configuration for WAL backup (offloading to s3) as TOML /// inline table, e.g. @@ -179,6 +183,7 @@ async fn main() -> anyhow::Result<()> { workdir, my_id: id, listen_pg_addr: args.listen_pg, + listen_pg_addr_tenant_only: args.listen_pg_tenant_only, listen_http_addr: args.listen_http, availability_zone: args.availability_zone, no_sync: args.no_sync, @@ -222,6 +227,21 @@ async fn start_safekeeper(conf: SafeKeeperConf) -> Result<()> { e })?; + let pg_listener_tenant_only = + if let Some(listen_pg_addr_tenant_only) = &conf.listen_pg_addr_tenant_only { + info!( + "starting safekeeper tenant scoped WAL service on {}", + listen_pg_addr_tenant_only + ); + let listener = tcp_listener::bind(listen_pg_addr_tenant_only.clone()).map_err(|e| { + error!("failed to bind to address {}: {}", conf.listen_pg_addr, e); + e + })?; + Some(listener) + } else { + None + }; + info!( "starting safekeeper HTTP service on {}", conf.listen_http_addr @@ -253,14 +273,34 @@ async fn start_safekeeper(conf: SafeKeeperConf) -> Result<()> { let current_thread_rt = conf .current_thread_runtime .then(|| Handle::try_current().expect("no runtime in main")); + let wal_service_handle = current_thread_rt .as_ref() .unwrap_or_else(|| WAL_SERVICE_RUNTIME.handle()) - .spawn(wal_service::task_main(conf_, pg_listener)) + .spawn(wal_service::task_main( + conf_, + pg_listener, + Some(Scope::SafekeeperData), + )) // wrap with task name for error reporting .map(|res| ("WAL service main".to_owned(), res)); tasks_handles.push(Box::pin(wal_service_handle)); + if let Some(pg_listener_tenant_only) = pg_listener_tenant_only { + let conf_ = conf.clone(); + let wal_service_handle = current_thread_rt + .as_ref() + .unwrap_or_else(|| WAL_SERVICE_RUNTIME.handle()) + .spawn(wal_service::task_main( + conf_, + pg_listener_tenant_only, + Some(Scope::Tenant), + )) + // wrap with task name for error reporting + .map(|res| ("WAL service tenant only main".to_owned(), res)); + tasks_handles.push(Box::pin(wal_service_handle)); + } + let conf_ = conf.clone(); let http_handle = current_thread_rt .as_ref() diff --git a/safekeeper/src/handler.rs b/safekeeper/src/handler.rs index 1367d5eebb..5fe9db9628 100644 --- a/safekeeper/src/handler.rs +++ b/safekeeper/src/handler.rs @@ -34,6 +34,8 @@ pub struct SafekeeperPostgresHandler { pub ttid: TenantTimelineId, /// Unique connection id is logged in spans for observability. pub conn_id: ConnectionId, + /// Auth scope allowed on the connections. None if auth is not configured. + allowed_auth_scope: Option, claims: Option, io_metrics: Option, } @@ -147,6 +149,16 @@ impl postgres_backend::Handler .unwrap() .decode(str::from_utf8(jwt_response).context("jwt response is not UTF-8")?)?; + let scope = self + .allowed_auth_scope + .expect("auth is enabled but scope is not configured"); + // The handler might be configured to allow only tenant scope tokens. + if matches!(scope, Scope::Tenant) && !matches!(data.claims.scope, Scope::Tenant) { + return Err(QueryError::Other(anyhow::anyhow!( + "passed JWT token is for full access, but only tenant scope is allowed" + ))); + } + if matches!(data.claims.scope, Scope::Tenant) && data.claims.tenant_id.is_none() { return Err(QueryError::Other(anyhow::anyhow!( "jwt token scope is Tenant, but tenant id is missing" @@ -215,7 +227,12 @@ impl postgres_backend::Handler } impl SafekeeperPostgresHandler { - pub fn new(conf: SafeKeeperConf, conn_id: u32, io_metrics: Option) -> Self { + pub fn new( + conf: SafeKeeperConf, + conn_id: u32, + io_metrics: Option, + allowed_auth_scope: Option, + ) -> Self { SafekeeperPostgresHandler { conf, appname: None, @@ -224,6 +241,7 @@ impl SafekeeperPostgresHandler { ttid: TenantTimelineId::empty(), conn_id, claims: None, + allowed_auth_scope, io_metrics, } } diff --git a/safekeeper/src/lib.rs b/safekeeper/src/lib.rs index b8e1101369..1a1c0add67 100644 --- a/safekeeper/src/lib.rs +++ b/safekeeper/src/lib.rs @@ -53,6 +53,7 @@ pub struct SafeKeeperConf { pub workdir: PathBuf, pub my_id: NodeId, pub listen_pg_addr: String, + pub listen_pg_addr_tenant_only: Option, pub listen_http_addr: String, pub availability_zone: Option, pub no_sync: bool, @@ -85,6 +86,7 @@ impl SafeKeeperConf { workdir: PathBuf::from("./"), no_sync: false, listen_pg_addr: defaults::DEFAULT_PG_LISTEN_ADDR.to_string(), + listen_pg_addr_tenant_only: None, listen_http_addr: defaults::DEFAULT_HTTP_LISTEN_ADDR.to_string(), availability_zone: None, remote_storage: None, diff --git a/safekeeper/src/wal_service.rs b/safekeeper/src/wal_service.rs index 406132b2b0..43e870e621 100644 --- a/safekeeper/src/wal_service.rs +++ b/safekeeper/src/wal_service.rs @@ -8,7 +8,7 @@ use std::{future, time::Duration}; use tokio::net::TcpStream; use tokio_io_timeout::TimeoutReader; use tracing::*; -use utils::measured_stream::MeasuredStream; +use utils::{auth::Scope, measured_stream::MeasuredStream}; use crate::handler::SafekeeperPostgresHandler; use crate::metrics::TrafficMetrics; @@ -19,6 +19,7 @@ use postgres_backend::{AuthType, PostgresBackend}; pub async fn task_main( conf: SafeKeeperConf, pg_listener: std::net::TcpListener, + allowed_auth_scope: Option, ) -> anyhow::Result<()> { // Tokio's from_std won't do this for us, per its comment. pg_listener.set_nonblocking(true)?; @@ -33,7 +34,7 @@ pub async fn task_main( let conn_id = issue_connection_id(&mut connection_count); tokio::spawn(async move { - if let Err(err) = handle_socket(socket, conf, conn_id) + if let Err(err) = handle_socket(socket, conf, conn_id, allowed_auth_scope) .instrument(info_span!("", cid = %conn_id)) .await { @@ -49,6 +50,7 @@ async fn handle_socket( socket: TcpStream, conf: SafeKeeperConf, conn_id: ConnectionId, + allowed_auth_scope: Option, ) -> Result<(), QueryError> { socket.set_nodelay(true)?; let peer_addr = socket.peer_addr()?; @@ -84,8 +86,12 @@ async fn handle_socket( None => AuthType::Trust, Some(_) => AuthType::NeonJWT, }; - let mut conn_handler = - SafekeeperPostgresHandler::new(conf, conn_id, Some(traffic_metrics.clone())); + let mut conn_handler = SafekeeperPostgresHandler::new( + conf, + conn_id, + Some(traffic_metrics.clone()), + allowed_auth_scope, + ); let pgbackend = PostgresBackend::new_from_io(socket, peer_addr, auth_type, None)?; // libpq protocol between safekeeper and walproposer / pageserver // We don't use shutdown. From 921bb86909dcebc2138fcd9769244ae21dfd6f3a Mon Sep 17 00:00:00 2001 From: Arseny Sher Date: Mon, 17 Jul 2023 19:58:05 +0300 Subject: [PATCH 03/23] Use safekeeper tenant only port in all tests and actually test it. Compute now uses special safekeeper WAL service port allowing auth tokens with only tenant scope. Adds understanding of this port to neon_local and fixtures, as well as test of both ports behaviour with different tokens. ref https://github.com/neondatabase/neon/issues/4730 --- control_plane/src/endpoint.rs | 6 +-- control_plane/src/local_env.rs | 10 +++++ control_plane/src/safekeeper.rs | 50 ++++++++++++++---------- test_runner/fixtures/neon_fixtures.py | 4 ++ test_runner/regress/test_wal_acceptor.py | 37 ++++++++++++++++++ 5 files changed, 84 insertions(+), 23 deletions(-) diff --git a/control_plane/src/endpoint.rs b/control_plane/src/endpoint.rs index ff373d7111..6df6e47f29 100644 --- a/control_plane/src/endpoint.rs +++ b/control_plane/src/endpoint.rs @@ -289,7 +289,7 @@ impl Endpoint { .env .safekeepers .iter() - .map(|sk| format!("localhost:{}", sk.pg_port)) + .map(|sk| format!("localhost:{}", sk.get_compute_port())) .collect::>() .join(","); conf.append("neon.safekeepers", &safekeepers); @@ -318,7 +318,7 @@ impl Endpoint { .env .safekeepers .iter() - .map(|x| x.pg_port.to_string()) + .map(|x| x.get_compute_port().to_string()) .collect::>() .join(","); let sk_hosts = vec!["localhost"; self.env.safekeepers.len()].join(","); @@ -463,7 +463,7 @@ impl Endpoint { .iter() .find(|node| node.id == sk_id) .ok_or_else(|| anyhow!("safekeeper {sk_id} does not exist"))?; - safekeeper_connstrings.push(format!("127.0.0.1:{}", sk.pg_port)); + safekeeper_connstrings.push(format!("127.0.0.1:{}", sk.get_compute_port())); } } diff --git a/control_plane/src/local_env.rs b/control_plane/src/local_env.rs index 208eb9e7ec..9e42c2e333 100644 --- a/control_plane/src/local_env.rs +++ b/control_plane/src/local_env.rs @@ -137,6 +137,7 @@ impl Default for PageServerConf { pub struct SafekeeperConf { pub id: NodeId, pub pg_port: u16, + pub pg_tenant_only_port: Option, pub http_port: u16, pub sync: bool, pub remote_storage: Option, @@ -149,6 +150,7 @@ impl Default for SafekeeperConf { Self { id: NodeId(0), pg_port: 0, + pg_tenant_only_port: None, http_port: 0, sync: true, remote_storage: None, @@ -158,6 +160,14 @@ impl Default for SafekeeperConf { } } +impl SafekeeperConf { + /// Compute is served by port on which only tenant scoped tokens allowed, if + /// it is configured. + pub fn get_compute_port(&self) -> u16 { + self.pg_tenant_only_port.unwrap_or(self.pg_port) + } +} + impl LocalEnv { pub fn pg_distrib_dir_raw(&self) -> PathBuf { self.pg_distrib_dir.clone() diff --git a/control_plane/src/safekeeper.rs b/control_plane/src/safekeeper.rs index d5e0fb112f..be0192d137 100644 --- a/control_plane/src/safekeeper.rs +++ b/control_plane/src/safekeeper.rs @@ -120,45 +120,55 @@ impl SafekeeperNode { let availability_zone = format!("sk-{}", id_string); let mut args = vec![ - "-D", - datadir.to_str().with_context(|| { - format!("Datadir path {datadir:?} cannot be represented as a unicode string") - })?, - "--id", - &id_string, - "--listen-pg", - &listen_pg, - "--listen-http", - &listen_http, - "--availability-zone", - &availability_zone, + "-D".to_owned(), + datadir + .to_str() + .with_context(|| { + format!("Datadir path {datadir:?} cannot be represented as a unicode string") + })? + .to_owned(), + "--id".to_owned(), + id_string, + "--listen-pg".to_owned(), + listen_pg, + "--listen-http".to_owned(), + listen_http, + "--availability-zone".to_owned(), + availability_zone, ]; + if let Some(pg_tenant_only_port) = self.conf.pg_tenant_only_port { + let listen_pg_tenant_only = format!("127.0.0.1:{}", pg_tenant_only_port); + args.extend(["--listen-pg-tenant-only".to_owned(), listen_pg_tenant_only]); + } if !self.conf.sync { - args.push("--no-sync"); + args.push("--no-sync".to_owned()); } let broker_endpoint = format!("{}", self.env.broker.client_url()); - args.extend(["--broker-endpoint", &broker_endpoint]); + args.extend(["--broker-endpoint".to_owned(), broker_endpoint]); let mut backup_threads = String::new(); if let Some(threads) = self.conf.backup_threads { backup_threads = threads.to_string(); - args.extend(["--backup-threads", &backup_threads]); + args.extend(["--backup-threads".to_owned(), backup_threads]); } else { drop(backup_threads); } if let Some(ref remote_storage) = self.conf.remote_storage { - args.extend(["--remote-storage", remote_storage]); + args.extend(["--remote-storage".to_owned(), remote_storage.clone()]); } let key_path = self.env.base_data_dir.join("auth_public_key.pem"); if self.conf.auth_enabled { args.extend([ - "--auth-validation-public-key-path", - key_path.to_str().with_context(|| { - format!("Key path {key_path:?} cannot be represented as a unicode string") - })?, + "--auth-validation-public-key-path".to_owned(), + key_path + .to_str() + .with_context(|| { + format!("Key path {key_path:?} cannot be represented as a unicode string") + })? + .to_owned(), ]); } diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index 9e43a2bfdb..eafc061ab9 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -459,6 +459,7 @@ class AuthKeys: def generate_safekeeper_token(self) -> str: return self.generate_token(scope="safekeeperdata") + # generate token giving access to only one tenant def generate_tenant_token(self, tenant_id: TenantId) -> str: return self.generate_token(scope="tenant", tenant_id=str(tenant_id)) @@ -965,6 +966,7 @@ class NeonEnv: for i in range(1, config.num_safekeepers + 1): port = SafekeeperPort( pg=self.port_distributor.get_port(), + pg_tenant_only=self.port_distributor.get_port(), http=self.port_distributor.get_port(), ) id = config.safekeepers_id_start + i # assign ids sequentially @@ -973,6 +975,7 @@ class NeonEnv: [[safekeepers]] id = {id} pg_port = {port.pg} + pg_tenant_only_port = {port.pg_tenant_only} http_port = {port.http} sync = {'true' if config.safekeepers_enable_fsync else 'false'}""" ) @@ -2608,6 +2611,7 @@ class EndpointFactory: @dataclass class SafekeeperPort: pg: int + pg_tenant_only: int http: int diff --git a/test_runner/regress/test_wal_acceptor.py b/test_runner/regress/test_wal_acceptor.py index f3a6d09398..24b32ad7e7 100644 --- a/test_runner/regress/test_wal_acceptor.py +++ b/test_runner/regress/test_wal_acceptor.py @@ -13,6 +13,7 @@ from functools import partial from pathlib import Path from typing import Any, List, Optional +import psycopg2 import pytest from fixtures.log_helper import log from fixtures.neon_fixtures import ( @@ -866,6 +867,41 @@ def test_timeline_status(neon_env_builder: NeonEnvBuilder, auth_enabled: bool): assert debug_dump_1["config"]["id"] == env.safekeepers[0].id +# Test auth on WAL service (postgres protocol) ports. +def test_sk_auth(neon_env_builder: NeonEnvBuilder): + neon_env_builder.auth_enabled = True + env = neon_env_builder.init_start() + + env.neon_cli.create_branch("test_sk_auth") + endpoint = env.endpoints.create_start("test_sk_auth") + + sk = env.safekeepers[0] + + # learn neon timeline from compute + tenant_id = TenantId(endpoint.safe_psql("show neon.tenant_id")[0][0]) + timeline_id = TimelineId(endpoint.safe_psql("show neon.timeline_id")[0][0]) + + tenant_token = env.auth_keys.generate_tenant_token(tenant_id) + full_token = env.auth_keys.generate_safekeeper_token() + + conn_opts = { + "host": "127.0.0.1", + "options": f"-c timeline_id={timeline_id} tenant_id={tenant_id}", + } + connector = PgProtocol(**conn_opts) + # no password, should fail + with pytest.raises(psycopg2.OperationalError): + connector.safe_psql("IDENTIFY_SYSTEM", port=sk.port.pg) + # giving password, should be ok with either token on main pg port + connector.safe_psql("IDENTIFY_SYSTEM", port=sk.port.pg, password=tenant_token) + connector.safe_psql("IDENTIFY_SYSTEM", port=sk.port.pg, password=full_token) + # on tenant only port tenant only token should work + connector.safe_psql("IDENTIFY_SYSTEM", port=sk.port.pg_tenant_only, password=tenant_token) + # but full token should fail + with pytest.raises(psycopg2.OperationalError): + connector.safe_psql("IDENTIFY_SYSTEM", port=sk.port.pg_tenant_only, password=full_token) + + class SafekeeperEnv: def __init__( self, @@ -912,6 +948,7 @@ class SafekeeperEnv: def start_safekeeper(self, i): port = SafekeeperPort( pg=self.port_distributor.get_port(), + pg_tenant_only=self.port_distributor.get_port(), http=self.port_distributor.get_port(), ) From d4a5fd52588a130997db5bd5baf6cf5fa998acfd Mon Sep 17 00:00:00 2001 From: Alexander Bayandin Date: Wed, 19 Jul 2023 15:44:14 +0100 Subject: [PATCH 04/23] Disable extension uploading to S3 (#4751) ## Problem We're going to reset S3 buckets for extensions (https://github.com/neondatabase/aws/pull/413), and as soon as we're going to change the format we store extensions on S3. Let's stop uploading extensions in the old format. ## Summary of changes - Disable `aws s3 cp` step for extensions --- .github/workflows/build_and_test.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index 5f3e4f1145..18dfc458b5 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -1007,6 +1007,8 @@ jobs: done - name: Upload postgres-extensions to S3 + # TODO: Reenable step after switching to the new extensions format (tar-gzipped + index.json) + if: false run: | for BUCKET in $(echo ${S3_BUCKETS}); do aws s3 cp --recursive --only-show-errors ./extensions-to-upload s3://${BUCKET}/${{ needs.tag.outputs.build-tag }}/${{ matrix.version }} From f09e82270e3258508c6d6cc191486c56ee926216 Mon Sep 17 00:00:00 2001 From: Daniel <10074684+danieltprice@users.noreply.github.com> Date: Wed, 19 Jul 2023 15:08:25 -0300 Subject: [PATCH 05/23] Update comment for hnsw extension (#4755) Updated the description that appears for hnsw when you query extensions: ``` neondb=> SELECT * FROM pg_available_extensions WHERE name = 'hnsw'; name | default_version | installed_version | comment ----------------------+-----------------+-------------------+-------------------------------------------------- hnsw | 0.1.0 | | ** Deprecated ** Please use pg_embedding instead (1 row) ``` --------- Co-authored-by: Alexander Bayandin --- pgxn/hnsw/hnsw.control | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pgxn/hnsw/hnsw.control b/pgxn/hnsw/hnsw.control index 8b75c350a8..24510f6766 100644 --- a/pgxn/hnsw/hnsw.control +++ b/pgxn/hnsw/hnsw.control @@ -1,4 +1,4 @@ -comment = 'hnsw index' +comment = '** Deprecated ** Please use pg_embedding instead' default_version = '0.1.0' module_pathname = '$libdir/hnsw' relocatable = true From e1061879aac8eb979a7292cb3a46c02d7f2e07ca Mon Sep 17 00:00:00 2001 From: bojanserafimov Date: Wed, 19 Jul 2023 23:46:16 -0400 Subject: [PATCH 06/23] Improve startup python test (#4757) --- test_runner/performance/test_startup.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test_runner/performance/test_startup.py b/test_runner/performance/test_startup.py index d897df1bcb..875be3b7b0 100644 --- a/test_runner/performance/test_startup.py +++ b/test_runner/performance/test_startup.py @@ -43,7 +43,17 @@ def test_startup_simple(neon_env_builder: NeonEnvBuilder, zenbenchmark: NeonBenc if endpoint: endpoint.start() else: - endpoint = env.endpoints.create_start("test_startup") + endpoint = env.endpoints.create_start( + "test_startup", + # Shared buffers need to be allocated during startup, so they + # impact startup time. This is the default value we use for + # 1CPU pods (maybe different for VMs). + # + # TODO extensions also contribute to shared memory allocation, + # and this test doesn't include all default extensions we + # load. + config_lines=["shared_buffers=262144"], + ) endpoint.safe_psql("select 1;") # Get metrics From 9e871318a0bfb530877247724d6fe2c62b857269 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Thu, 20 Jul 2023 13:14:13 +0300 Subject: [PATCH 07/23] Wait detaches or ignores on pageserver shutdown (#4678) Adds in a barrier for the duration of the `Tenant::shutdown`. `pageserver_shutdown` will join this await, `detach`es and `ignore`s will not. Fixes #4429. --------- Co-authored-by: Christian Schwarz --- libs/pageserver_api/src/models.rs | 18 ++- libs/utils/src/completion.rs | 16 +++ libs/utils/src/tracing_span_assert.rs | 4 +- pageserver/Cargo.toml | 1 + pageserver/src/tenant.rs | 43 +++--- pageserver/src/tenant/mgr.rs | 198 +++++++++++++++++++++++--- workspace_hack/Cargo.toml | 2 +- 7 files changed, 242 insertions(+), 40 deletions(-) diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index 4c6529ffab..2f4c21326e 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -9,6 +9,7 @@ use serde::{Deserialize, Serialize}; use serde_with::{serde_as, DisplayFromStr}; use strum_macros; use utils::{ + completion, history_buffer::HistoryBufferWithDropCounter, id::{NodeId, TenantId, TimelineId}, lsn::Lsn, @@ -76,7 +77,12 @@ pub enum TenantState { /// system is being shut down. /// /// Transitions out of this state are possible through `set_broken()`. - Stopping, + Stopping { + // Because of https://github.com/serde-rs/serde/issues/2105 this has to be a named field, + // otherwise it will not be skipped during deserialization + #[serde(skip)] + progress: completion::Barrier, + }, /// The tenant is recognized by the pageserver, but can no longer be used for /// any operations. /// @@ -118,7 +124,7 @@ impl TenantState { // Why is Stopping a Maybe case? Because, during pageserver shutdown, // we set the Stopping state irrespective of whether the tenant // has finished attaching or not. - Self::Stopping => Maybe, + Self::Stopping { .. } => Maybe, } } @@ -928,7 +934,13 @@ mod tests { "Activating", ), (line!(), TenantState::Active, "Active"), - (line!(), TenantState::Stopping, "Stopping"), + ( + line!(), + TenantState::Stopping { + progress: utils::completion::Barrier::default(), + }, + "Stopping", + ), ( line!(), TenantState::Broken { diff --git a/libs/utils/src/completion.rs b/libs/utils/src/completion.rs index 2cdaee548e..e2e84dd0ee 100644 --- a/libs/utils/src/completion.rs +++ b/libs/utils/src/completion.rs @@ -12,6 +12,13 @@ pub struct Completion(mpsc::Sender<()>); #[derive(Clone)] pub struct Barrier(Arc>>); +impl Default for Barrier { + fn default() -> Self { + let (_, rx) = channel(); + rx + } +} + impl Barrier { pub async fn wait(self) { self.0.lock().await.recv().await; @@ -24,6 +31,15 @@ impl Barrier { } } +impl PartialEq for Barrier { + fn eq(&self, other: &Self) -> bool { + // we don't use dyn so this is good + Arc::ptr_eq(&self.0, &other.0) + } +} + +impl Eq for Barrier {} + /// Create new Guard and Barrier pair. pub fn channel() -> (Completion, Barrier) { let (tx, rx) = mpsc::channel::<()>(1); diff --git a/libs/utils/src/tracing_span_assert.rs b/libs/utils/src/tracing_span_assert.rs index 926bfc3188..db17f7d8cd 100644 --- a/libs/utils/src/tracing_span_assert.rs +++ b/libs/utils/src/tracing_span_assert.rs @@ -164,9 +164,7 @@ fn tracing_subscriber_configured() -> bool { tracing::dispatcher::get_default(|d| { // it is possible that this closure will not be invoked, but the current implementation // always invokes it - noop_configured = d - .downcast_ref::() - .is_some(); + noop_configured = d.is::(); }); !noop_configured diff --git a/pageserver/Cargo.toml b/pageserver/Cargo.toml index 9381ed0bfa..27e90ea97d 100644 --- a/pageserver/Cargo.toml +++ b/pageserver/Cargo.toml @@ -82,6 +82,7 @@ strum_macros.workspace = true criterion.workspace = true hex-literal.workspace = true tempfile.workspace = true +tokio = { workspace = true, features = ["process", "sync", "fs", "rt", "io-util", "time", "test-util"] } [[bench]] name = "bench_layer_map" diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 142118bf6e..379db0720f 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -281,7 +281,7 @@ pub enum DeleteTimelineError { } pub enum SetStoppingError { - AlreadyStopping, + AlreadyStopping(completion::Barrier), Broken, } @@ -318,10 +318,6 @@ impl std::fmt::Display for WaitToBecomeActiveError { } } -pub(crate) enum ShutdownError { - AlreadyStopping, -} - struct DeletionGuard(OwnedMutexGuard); impl DeletionGuard { @@ -1721,7 +1717,7 @@ impl Tenant { self.state.send_modify(|current_state| { use pageserver_api::models::ActivatingFrom; match &*current_state { - TenantState::Activating(_) | TenantState::Active | TenantState::Broken { .. } | TenantState::Stopping => { + TenantState::Activating(_) | TenantState::Active | TenantState::Broken { .. } | TenantState::Stopping { .. } => { panic!("caller is responsible for calling activate() only on Loading / Attaching tenants, got {state:?}", state = current_state); } TenantState::Loading => { @@ -1785,7 +1781,16 @@ impl Tenant { /// - detach + ignore (freeze_and_flush == false) /// /// This will attempt to shutdown even if tenant is broken. - pub(crate) async fn shutdown(&self, freeze_and_flush: bool) -> Result<(), ShutdownError> { + /// + /// `shutdown_progress` is a [`completion::Barrier`] for the shutdown initiated by this call. + /// If the tenant is already shutting down, we return a clone of the first shutdown call's + /// `Barrier` as an `Err`. This not-first caller can use the returned barrier to join with + /// the ongoing shutdown. + async fn shutdown( + &self, + shutdown_progress: completion::Barrier, + freeze_and_flush: bool, + ) -> Result<(), completion::Barrier> { span::debug_assert_current_span_has_tenant_id(); // Set tenant (and its timlines) to Stoppping state. // @@ -1804,12 +1809,16 @@ impl Tenant { // But the tenant background loops are joined-on in our caller. // It's mesed up. // we just ignore the failure to stop - match self.set_stopping().await { + + match self.set_stopping(shutdown_progress).await { Ok(()) => {} Err(SetStoppingError::Broken) => { // assume that this is acceptable } - Err(SetStoppingError::AlreadyStopping) => return Err(ShutdownError::AlreadyStopping), + Err(SetStoppingError::AlreadyStopping(other)) => { + // give caller the option to wait for this this shutdown + return Err(other); + } }; if freeze_and_flush { @@ -1841,7 +1850,7 @@ impl Tenant { /// This function waits for the tenant to become active if it isn't already, before transitioning it into Stopping state. /// /// This function is not cancel-safe! - async fn set_stopping(&self) -> Result<(), SetStoppingError> { + async fn set_stopping(&self, progress: completion::Barrier) -> Result<(), SetStoppingError> { let mut rx = self.state.subscribe(); // cannot stop before we're done activating, so wait out until we're done activating @@ -1853,7 +1862,7 @@ impl Tenant { ); false } - TenantState::Active | TenantState::Broken { .. } | TenantState::Stopping {} => true, + TenantState::Active | TenantState::Broken { .. } | TenantState::Stopping { .. } => true, }) .await .expect("cannot drop self.state while on a &self method"); @@ -1868,7 +1877,7 @@ impl Tenant { // FIXME: due to time-of-check vs time-of-use issues, it can happen that new timelines // are created after the transition to Stopping. That's harmless, as the Timelines // won't be accessible to anyone afterwards, because the Tenant is in Stopping state. - *current_state = TenantState::Stopping; + *current_state = TenantState::Stopping { progress }; // Continue stopping outside the closure. We need to grab timelines.lock() // and we plan to turn it into a tokio::sync::Mutex in a future patch. true @@ -1880,9 +1889,9 @@ impl Tenant { err = Some(SetStoppingError::Broken); false } - TenantState::Stopping => { + TenantState::Stopping { progress } => { info!("Tenant is already in Stopping state"); - err = Some(SetStoppingError::AlreadyStopping); + err = Some(SetStoppingError::AlreadyStopping(progress.clone())); false } }); @@ -1926,7 +1935,7 @@ impl Tenant { ); false } - TenantState::Active | TenantState::Broken { .. } | TenantState::Stopping {} => true, + TenantState::Active | TenantState::Broken { .. } | TenantState::Stopping { .. } => true, }) .await .expect("cannot drop self.state while on a &self method"); @@ -1949,7 +1958,7 @@ impl Tenant { warn!("Tenant is already in Broken state"); } // This is the only "expected" path, any other path is a bug. - TenantState::Stopping => { + TenantState::Stopping { .. } => { warn!( "Marking Stopping tenant as Broken state, reason: {}", reason @@ -1982,7 +1991,7 @@ impl Tenant { TenantState::Active { .. } => { return Ok(()); } - TenantState::Broken { .. } | TenantState::Stopping => { + TenantState::Broken { .. } | TenantState::Stopping { .. } => { // There's no chance the tenant can transition back into ::Active return Err(WaitToBecomeActiveError::WillNotBecomeActive { tenant_id: self.tenant_id, diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs index 2cc881ed5e..4b97871f35 100644 --- a/pageserver/src/tenant/mgr.rs +++ b/pageserver/src/tenant/mgr.rs @@ -233,11 +233,17 @@ pub fn schedule_local_tenant_processing( /// That could be easily misinterpreted by control plane, the consumer of the /// management API. For example, it could attach the tenant on a different pageserver. /// We would then be in split-brain once this pageserver restarts. -#[instrument] +#[instrument(skip_all)] pub async fn shutdown_all_tenants() { + shutdown_all_tenants0(&TENANTS).await +} + +async fn shutdown_all_tenants0(tenants: &tokio::sync::RwLock) { + use utils::completion; + // Prevent new tenants from being created. let tenants_to_shut_down = { - let mut m = TENANTS.write().await; + let mut m = tenants.write().await; match &mut *m { TenantsMap::Initializing => { *m = TenantsMap::ShuttingDown(HashMap::default()); @@ -262,14 +268,41 @@ pub async fn shutdown_all_tenants() { for (tenant_id, tenant) in tenants_to_shut_down { join_set.spawn( async move { - let freeze_and_flush = true; + // ordering shouldn't matter for this, either we store true right away or never + let ordering = std::sync::atomic::Ordering::Relaxed; + let joined_other = std::sync::atomic::AtomicBool::new(false); - match tenant.shutdown(freeze_and_flush).await { - Ok(()) => debug!("tenant successfully stopped"), - Err(super::ShutdownError::AlreadyStopping) => { - warn!("tenant was already shutting down") + let mut shutdown = std::pin::pin!(async { + let freeze_and_flush = true; + + let res = { + let (_guard, shutdown_progress) = completion::channel(); + tenant.shutdown(shutdown_progress, freeze_and_flush).await + }; + + if let Err(other_progress) = res { + // join the another shutdown in progress + joined_other.store(true, ordering); + other_progress.wait().await; } - } + }); + + // in practice we might not have a lot time to go, since systemd is going to + // SIGKILL us at 10s, but we can try. delete tenant might take a while, so put out + // a warning. + let warning = std::time::Duration::from_secs(5); + let mut warning = std::pin::pin!(tokio::time::sleep(warning)); + + tokio::select! { + _ = &mut shutdown => {}, + _ = &mut warning => { + let joined_other = joined_other.load(ordering); + warn!(%joined_other, "waiting for the shutdown to complete"); + shutdown.await; + } + }; + + debug!("tenant successfully stopped"); } .instrument(info_span!("shutdown", %tenant_id)), ); @@ -413,6 +446,15 @@ pub async fn detach_tenant( conf: &'static PageServerConf, tenant_id: TenantId, detach_ignored: bool, +) -> Result<(), TenantStateError> { + detach_tenant0(conf, &TENANTS, tenant_id, detach_ignored).await +} + +async fn detach_tenant0( + conf: &'static PageServerConf, + tenants: &tokio::sync::RwLock, + tenant_id: TenantId, + detach_ignored: bool, ) -> Result<(), TenantStateError> { let local_files_cleanup_operation = |tenant_id_to_clean| async move { let local_tenant_directory = conf.tenant_path(&tenant_id_to_clean); @@ -425,7 +467,8 @@ pub async fn detach_tenant( }; let removal_result = - remove_tenant_from_memory(tenant_id, local_files_cleanup_operation(tenant_id)).await; + remove_tenant_from_memory(tenants, tenant_id, local_files_cleanup_operation(tenant_id)) + .await; // Ignored tenants are not present in memory and will bail the removal from memory operation. // Before returning the error, check for ignored tenant removal case — we only need to clean its local files then. @@ -472,7 +515,15 @@ pub async fn ignore_tenant( conf: &'static PageServerConf, tenant_id: TenantId, ) -> Result<(), TenantStateError> { - remove_tenant_from_memory(tenant_id, async { + ignore_tenant0(conf, &TENANTS, tenant_id).await +} + +async fn ignore_tenant0( + conf: &'static PageServerConf, + tenants: &tokio::sync::RwLock, + tenant_id: TenantId, +) -> Result<(), TenantStateError> { + remove_tenant_from_memory(tenants, tenant_id, async { let ignore_mark_file = conf.tenant_ignore_mark_file_path(&tenant_id); fs::File::create(&ignore_mark_file) .await @@ -597,18 +648,21 @@ where /// If the cleanup fails, tenant will stay in memory in [`TenantState::Broken`] state, and another removal /// operation would be needed to remove it. async fn remove_tenant_from_memory( + tenants: &tokio::sync::RwLock, tenant_id: TenantId, tenant_cleanup: F, ) -> Result where F: std::future::Future>, { + use utils::completion; + // It's important to keep the tenant in memory after the final cleanup, to avoid cleanup races. // The exclusive lock here ensures we don't miss the tenant state updates before trying another removal. // tenant-wde cleanup operations may take some time (removing the entire tenant directory), we want to // avoid holding the lock for the entire process. let tenant = { - TENANTS + tenants .write() .await .get(&tenant_id) @@ -616,14 +670,20 @@ where .ok_or(TenantStateError::NotFound(tenant_id))? }; + // allow pageserver shutdown to await for our completion + let (_guard, progress) = completion::channel(); + + // whenever we remove a tenant from memory, we don't want to flush and wait for upload let freeze_and_flush = false; // shutdown is sure to transition tenant to stopping, and wait for all tasks to complete, so // that we can continue safely to cleanup. - match tenant.shutdown(freeze_and_flush).await { + match tenant.shutdown(progress, freeze_and_flush).await { Ok(()) => {} - Err(super::ShutdownError::AlreadyStopping) => { - return Err(TenantStateError::IsStopping(tenant_id)) + Err(_other) => { + // if pageserver shutdown or other detach/ignore is already ongoing, we don't want to + // wait for it but return an error right away because these are distinct requests. + return Err(TenantStateError::IsStopping(tenant_id)); } } @@ -632,14 +692,14 @@ where .with_context(|| format!("Failed to run cleanup for tenant {tenant_id}")) { Ok(hook_value) => { - let mut tenants_accessor = TENANTS.write().await; + let mut tenants_accessor = tenants.write().await; if tenants_accessor.remove(&tenant_id).is_none() { warn!("Tenant {tenant_id} got removed from memory before operation finished"); } Ok(hook_value) } Err(e) => { - let tenants_accessor = TENANTS.read().await; + let tenants_accessor = tenants.read().await; match tenants_accessor.get(&tenant_id) { Some(tenant) => { tenant.set_broken(e.to_string()).await; @@ -756,3 +816,109 @@ pub async fn immediate_compact( Ok(wait_task_done) } + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + use std::sync::Arc; + use tracing::{info_span, Instrument}; + + use super::{super::harness::TenantHarness, TenantsMap}; + + #[tokio::test(start_paused = true)] + async fn shutdown_joins_remove_tenant_from_memory() { + // the test is a bit ugly with the lockstep together with spawned tasks. the aim is to make + // sure `shutdown_all_tenants0` per-tenant processing joins in any active + // remove_tenant_from_memory calls, which is enforced by making the operation last until + // we've ran `shutdown_all_tenants0` for a long time. + + let (t, _ctx) = TenantHarness::create("shutdown_joins_detach") + .unwrap() + .load() + .await; + + // harness loads it to active, which is forced and nothing is running on the tenant + + let id = t.tenant_id(); + + // tenant harness configures the logging and we cannot escape it + let _e = info_span!("testing", tenant_id = %id).entered(); + + let tenants = HashMap::from([(id, t.clone())]); + let tenants = Arc::new(tokio::sync::RwLock::new(TenantsMap::Open(tenants))); + + let (until_cleanup_completed, can_complete_cleanup) = utils::completion::channel(); + let (until_cleanup_started, cleanup_started) = utils::completion::channel(); + + // start a "detaching operation", which will take a while, until can_complete_cleanup + let cleanup_task = { + let jh = tokio::spawn({ + let tenants = tenants.clone(); + async move { + let cleanup = async move { + drop(until_cleanup_started); + can_complete_cleanup.wait().await; + anyhow::Ok(()) + }; + super::remove_tenant_from_memory(&tenants, id, cleanup).await + } + .instrument(info_span!("foobar", tenant_id = %id)) + }); + + // now the long cleanup should be in place, with the stopping state + cleanup_started.wait().await; + jh + }; + + let mut cleanup_progress = std::pin::pin!(t + .shutdown(utils::completion::Barrier::default(), false) + .await + .unwrap_err() + .wait()); + + let mut shutdown_task = { + let (until_shutdown_started, shutdown_started) = utils::completion::channel(); + + let shutdown_task = tokio::spawn(async move { + drop(until_shutdown_started); + super::shutdown_all_tenants0(&tenants).await; + }); + + shutdown_started.wait().await; + shutdown_task + }; + + // if the joining in is removed from shutdown_all_tenants0, the shutdown_task should always + // get to complete within timeout and fail the test. it is expected to continue awaiting + // until completion or SIGKILL during normal shutdown. + // + // the timeout is long to cover anything that shutdown_task could be doing, but it is + // handled instantly because we use tokio's time pausing in this test. 100s is much more than + // what we get from systemd on shutdown (10s). + let long_time = std::time::Duration::from_secs(100); + tokio::select! { + _ = &mut shutdown_task => unreachable!("shutdown must continue, until_cleanup_completed is not dropped"), + _ = &mut cleanup_progress => unreachable!("cleanup progress must continue, until_cleanup_completed is not dropped"), + _ = tokio::time::sleep(long_time) => {}, + } + + // allow the remove_tenant_from_memory and thus eventually the shutdown to continue + drop(until_cleanup_completed); + + let (je, ()) = tokio::join!(shutdown_task, cleanup_progress); + je.expect("Tenant::shutdown shutdown not have panicked"); + cleanup_task + .await + .expect("no panicking") + .expect("remove_tenant_from_memory failed"); + + futures::future::poll_immediate( + t.shutdown(utils::completion::Barrier::default(), false) + .await + .unwrap_err() + .wait(), + ) + .await + .expect("the stopping progress must still be complete"); + } +} diff --git a/workspace_hack/Cargo.toml b/workspace_hack/Cargo.toml index 63a65d3889..3f47ef062f 100644 --- a/workspace_hack/Cargo.toml +++ b/workspace_hack/Cargo.toml @@ -46,7 +46,7 @@ scopeguard = { version = "1" } serde = { version = "1", features = ["alloc", "derive"] } serde_json = { version = "1", features = ["raw_value"] } socket2 = { version = "0.4", default-features = false, features = ["all"] } -tokio = { version = "1", features = ["fs", "io-std", "io-util", "macros", "net", "process", "rt-multi-thread", "signal", "sync", "time"] } +tokio = { version = "1", features = ["fs", "io-std", "io-util", "macros", "net", "process", "rt-multi-thread", "signal", "test-util"] } tokio-rustls = { version = "0.23" } tokio-util = { version = "0.7", features = ["codec", "io"] } toml_datetime = { version = "0.6", default-features = false, features = ["serde"] } From 27c73c87409ebe8aa9045c197f0c61ee5050a730 Mon Sep 17 00:00:00 2001 From: Alexander Bayandin Date: Thu, 20 Jul 2023 12:32:57 +0100 Subject: [PATCH 08/23] Bump pg_embedding extension (#4758) ``` % git log --pretty=oneline 2465f831ea1f8d49c1d74f8959adb7fc277d70cd..eeb3ba7c3a60c95b2604dd543c64b2f1bb4a3703 eeb3ba7c3a60c95b2604dd543c64b2f1bb4a3703 (HEAD -> main, origin/main) Fixc in-mmeory index rebuild after TRUNCATE 1d7cfcfe3d58e2cf4566900437c609725448d14b Correctly handle truncate forin-0memory HNSW index 8fd2a4a191f67858498d876ec378b58e76b5874a :Fix empty index search issue 30e9ef4064cff40c60ff2f78afeac6c296722757 Fix extensiomn name in makefile 23bb5d504aa21b1663719739f6eedfdcb139d948 Fix getting memory size at Mac OS/X 39193a38d6ad8badd2a8d1dce2dd999e1b86885d Update a comment for the extension bf3b0d62a7df56a5e4db9d9e62dc535794c425bc Merge branch 'main' of https://github.com/neondatabase/pg_embedding c2142d514280e14322d1026f0c811876ccf7a91f Update README.md 53b641880f786d2b69a75941c49e569018e8e97e Create LICENSE 093aaa36d5af183831bf370c97b563c12d15f23a Update README.md 91f0bb84d14cb26fd8b452bf9e1ecea026ac5cbc Update README.md 7f7efa38015f24ee9a09beca3009b8d0497a40b4 Update README.md 71defdd4143ecf35489d93289f6cdfa2545fbd36 Merge pull request #4 from neondatabase/danieltprice-patch-1 e06c228b99c6b7c47ebce3bb7c97dbd494088b0a Update README.md d7e52b576b47d9023743b124bdd0360a9fc98f59 Update README.md 70ab399c861330b50a9aff9ab9edc7044942a65b Merge pull request #5 from neondatabase/oom_error_reporting 0aee1d937997198fa2d2b2ed7a0886d1075fa790 Fix OOM error reporting and support vectprization for ARM 18d80079ce60b2aa81d58cefdf42fc09d2621fc1 Update README.md ``` --- Dockerfile.compute-node | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Dockerfile.compute-node b/Dockerfile.compute-node index 1b5db2af81..495ef25526 100644 --- a/Dockerfile.compute-node +++ b/Dockerfile.compute-node @@ -535,10 +535,10 @@ FROM build-deps AS pg-embedding-pg-build COPY --from=pg-build /usr/local/pgsql/ /usr/local/pgsql/ ENV PATH "/usr/local/pgsql/bin/:$PATH" -# 2465f831ea1f8d49c1d74f8959adb7fc277d70cd made on 05/07/2023 +# eeb3ba7c3a60c95b2604dd543c64b2f1bb4a3703 made on 15/07/2023 # There is no release tag yet -RUN wget https://github.com/neondatabase/pg_embedding/archive/2465f831ea1f8d49c1d74f8959adb7fc277d70cd.tar.gz -O pg_embedding.tar.gz && \ - echo "047af2b1f664a1e6e37867bd4eeaf5934fa27d6ba3d6c4461efa388ddf7cd1d5 pg_embedding.tar.gz" | sha256sum --check && \ +RUN wget https://github.com/neondatabase/pg_embedding/archive/eeb3ba7c3a60c95b2604dd543c64b2f1bb4a3703.tar.gz -O pg_embedding.tar.gz && \ + echo "030846df723652f99a8689ce63b66fa0c23477a7fd723533ab8a6b28ab70730f pg_embedding.tar.gz" | sha256sum --check && \ mkdir pg_embedding-src && cd pg_embedding-src && tar xvzf ../pg_embedding.tar.gz --strip-components=1 -C . && \ make -j $(getconf _NPROCESSORS_ONLN) && \ make -j $(getconf _NPROCESSORS_ONLN) install && \ From d98cb399787f76d61985b58648c56c050d82f206 Mon Sep 17 00:00:00 2001 From: arpad-m Date: Thu, 20 Jul 2023 16:19:38 +0200 Subject: [PATCH 09/23] pageserver: use tokio::time::timeout where possible (#4756) Removes a bunch of cases which used `tokio::select` to emulate the `tokio::time::timeout` function. I've done an additional review on the cancellation safety of these futures, all of them seem to be cancellation safe (not that `select!` allows non-cancellation-safe futures, but as we touch them, such a review makes sense). Furthermore, I correct a few mentions of a non-existent `tokio::timeout!` macro in the docs to the `tokio::time::timeout` function. --- docs/pageserver-thread-mgmt.md | 10 +++---- pageserver/src/bin/pageserver.rs | 6 ++-- pageserver/src/disk_usage_eviction_task.rs | 10 +++---- pageserver/src/task_mgr.rs | 18 +++++------ pageserver/src/tenant/tasks.rs | 30 +++++++++---------- .../src/tenant/timeline/eviction_task.rs | 10 +++---- 6 files changed, 40 insertions(+), 44 deletions(-) diff --git a/docs/pageserver-thread-mgmt.md b/docs/pageserver-thread-mgmt.md index b911933528..c911d2c53d 100644 --- a/docs/pageserver-thread-mgmt.md +++ b/docs/pageserver-thread-mgmt.md @@ -30,8 +30,8 @@ or similar, to wake up on shutdown. In async Rust, futures can be "cancelled" at any await point, by dropping the Future. For example, `tokio::select!` returns as soon as -one of the Futures returns, and drops the others. `tokio::timeout!` is -another example. In the Rust ecosystem, some functions are +one of the Futures returns, and drops the others. `tokio::time::timeout` +is another example. In the Rust ecosystem, some functions are cancellation-safe, meaning they can be safely dropped without side-effects, while others are not. See documentation of `tokio::select!` for examples. @@ -42,9 +42,9 @@ function that you call cannot be assumed to be async cancellation-safe, and must be polled to completion. The downside of non-cancellation safe code is that you have to be very -careful when using `tokio::select!`, `tokio::timeout!`, and other such -functions that can cause a Future to be dropped. They can only be used -with functions that are explicitly documented to be cancellation-safe, +careful when using `tokio::select!`, `tokio::time::timeout`, and other +such functions that can cause a Future to be dropped. They can only be +used with functions that are explicitly documented to be cancellation-safe, or you need to spawn a separate task to shield from the cancellation. At the entry points to the code, we also take care to poll futures to diff --git a/pageserver/src/bin/pageserver.rs b/pageserver/src/bin/pageserver.rs index b01ace63e4..b247fdf0ab 100644 --- a/pageserver/src/bin/pageserver.rs +++ b/pageserver/src/bin/pageserver.rs @@ -396,8 +396,8 @@ fn start_pageserver( let guard = scopeguard::guard_on_success((), |_| tracing::info!("Cancelled before initial logical sizes completed")); - let init_sizes_done = tokio::select! { - _ = &mut init_sizes_done => { + let init_sizes_done = match tokio::time::timeout(timeout, &mut init_sizes_done).await { + Ok(_) => { let now = std::time::Instant::now(); tracing::info!( from_init_done_millis = (now - init_done).as_millis(), @@ -406,7 +406,7 @@ fn start_pageserver( ); None } - _ = tokio::time::sleep(timeout) => { + Err(_) => { tracing::info!( timeout_millis = timeout.as_millis(), "Initial logical size timeout elapsed; starting background jobs" diff --git a/pageserver/src/disk_usage_eviction_task.rs b/pageserver/src/disk_usage_eviction_task.rs index b2ca9ab0bb..37c52c5423 100644 --- a/pageserver/src/disk_usage_eviction_task.rs +++ b/pageserver/src/disk_usage_eviction_task.rs @@ -166,11 +166,11 @@ async fn disk_usage_eviction_task( .await; let sleep_until = start + task_config.period; - tokio::select! { - _ = tokio::time::sleep_until(sleep_until) => {}, - _ = cancel.cancelled() => { - break - } + if tokio::time::timeout_at(sleep_until, cancel.cancelled()) + .await + .is_ok() + { + break; } } } diff --git a/pageserver/src/task_mgr.rs b/pageserver/src/task_mgr.rs index 9c6851bc71..f3a4ce6db7 100644 --- a/pageserver/src/task_mgr.rs +++ b/pageserver/src/task_mgr.rs @@ -511,17 +511,13 @@ pub async fn shutdown_tasks( warn!(name = task.name, tenant_id = ?tenant_id, timeline_id = ?timeline_id, kind = ?task_kind, "stopping left-over"); } } - let join_handle = tokio::select! { - biased; - _ = &mut join_handle => { None }, - _ = tokio::time::sleep(std::time::Duration::from_secs(1)) => { - // allow some time to elapse before logging to cut down the number of log - // lines. - info!("waiting for {} to shut down", task.name); - Some(join_handle) - } - }; - if let Some(join_handle) = join_handle { + if tokio::time::timeout(std::time::Duration::from_secs(1), &mut join_handle) + .await + .is_err() + { + // allow some time to elapse before logging to cut down the number of log + // lines. + info!("waiting for {} to shut down", task.name); // we never handled this return value, but: // - we don't deschedule which would lead to is_cancelled // - panics are already logged (is_panicked) diff --git a/pageserver/src/tenant/tasks.rs b/pageserver/src/tenant/tasks.rs index 360818b5a7..622ae371a4 100644 --- a/pageserver/src/tenant/tasks.rs +++ b/pageserver/src/tenant/tasks.rs @@ -122,12 +122,12 @@ async fn compaction_loop(tenant: Arc, cancel: CancellationToken) { warn_when_period_overrun(started_at.elapsed(), period, "compaction"); // Sleep - tokio::select! { - _ = cancel.cancelled() => { - info!("received cancellation request during idling"); - break; - }, - _ = tokio::time::sleep(sleep_duration) => {}, + if tokio::time::timeout(sleep_duration, cancel.cancelled()) + .await + .is_ok() + { + info!("received cancellation request during idling"); + break; } } } @@ -196,12 +196,12 @@ async fn gc_loop(tenant: Arc, cancel: CancellationToken) { warn_when_period_overrun(started_at.elapsed(), period, "gc"); // Sleep - tokio::select! { - _ = cancel.cancelled() => { - info!("received cancellation request during idling"); - break; - }, - _ = tokio::time::sleep(sleep_duration) => {}, + if tokio::time::timeout(sleep_duration, cancel.cancelled()) + .await + .is_ok() + { + info!("received cancellation request during idling"); + break; } } } @@ -263,9 +263,9 @@ pub(crate) async fn random_init_delay( rng.gen_range(Duration::ZERO..=period) }; - tokio::select! { - _ = cancel.cancelled() => Err(Cancelled), - _ = tokio::time::sleep(d) => Ok(()), + match tokio::time::timeout(d, cancel.cancelled()).await { + Ok(_) => Err(Cancelled), + Err(_) => Ok(()), } } diff --git a/pageserver/src/tenant/timeline/eviction_task.rs b/pageserver/src/tenant/timeline/eviction_task.rs index 80146419df..a5f570d942 100644 --- a/pageserver/src/tenant/timeline/eviction_task.rs +++ b/pageserver/src/tenant/timeline/eviction_task.rs @@ -100,11 +100,11 @@ impl Timeline { match cf { ControlFlow::Break(()) => break, ControlFlow::Continue(sleep_until) => { - tokio::select! { - _ = cancel.cancelled() => { - break; - } - _ = tokio::time::sleep_until(sleep_until) => { } + if tokio::time::timeout_at(sleep_until, cancel.cancelled()) + .await + .is_ok() + { + break; } } } From 8d27a9c54e16967103c3aeeb261933718ec5f9b1 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Thu, 20 Jul 2023 17:45:10 +0300 Subject: [PATCH 10/23] Less verbose eviction failures (#4737) As seen in staging logs with some massive compactions (create_image_layer), in addition to racing with compaction or gc or even between two invocations to `evict_layer_batch`. Cc: #4745 Fixes: #3851 (organic tech debt reduction) Solution is not to log the Not Found in such cases; it is perfectly natural to happen. Route to this is quite long, but implemented two cases of "race between two eviction processes" which are like our disk usage based eviction and eviction_task, both have the separate "lets figure out what to evict" and "lets evict" phases. --- libs/utils/src/error.rs | 111 +++++++ libs/utils/src/lib.rs | 3 + pageserver/src/disk_usage_eviction_task.rs | 25 +- pageserver/src/tenant.rs | 20 +- pageserver/src/tenant/timeline.rs | 304 ++++++++++++++---- .../src/tenant/timeline/eviction_task.rs | 23 +- 6 files changed, 406 insertions(+), 80 deletions(-) create mode 100644 libs/utils/src/error.rs diff --git a/libs/utils/src/error.rs b/libs/utils/src/error.rs new file mode 100644 index 0000000000..7ce203e918 --- /dev/null +++ b/libs/utils/src/error.rs @@ -0,0 +1,111 @@ +/// Create a reporter for an error that outputs similar to [`anyhow::Error`] with Display with alternative setting. +/// +/// It can be used with `anyhow::Error` as well. +/// +/// Why would one use this instead of converting to `anyhow::Error` on the spot? Because +/// anyhow::Error would also capture a stacktrace on the spot, which you would later discard after +/// formatting. +/// +/// ## Usage +/// +/// ```rust +/// #[derive(Debug, thiserror::Error)] +/// enum MyCoolError { +/// #[error("should never happen")] +/// Bad(#[source] std::io::Error), +/// } +/// +/// # fn failing_call() -> Result<(), MyCoolError> { Err(MyCoolError::Bad(std::io::ErrorKind::PermissionDenied.into())) } +/// +/// # fn main() { +/// use utils::error::report_compact_sources; +/// +/// if let Err(e) = failing_call() { +/// let e = report_compact_sources(&e); +/// assert_eq!(format!("{e}"), "should never happen: permission denied"); +/// } +/// # } +/// ``` +/// +/// ## TODO +/// +/// When we are able to describe return position impl trait in traits, this should of course be an +/// extension trait. Until then avoid boxing with this more ackward interface. +pub fn report_compact_sources(e: &E) -> impl std::fmt::Display + '_ { + struct AnyhowDisplayAlternateAlike<'a, E>(&'a E); + + impl std::fmt::Display for AnyhowDisplayAlternateAlike<'_, E> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0)?; + + // why is E a generic parameter here? hope that rustc will see through a default + // Error::source implementation and leave the following out if there cannot be any + // sources: + Sources(self.0.source()).try_for_each(|src| write!(f, ": {}", src)) + } + } + + struct Sources<'a>(Option<&'a (dyn std::error::Error + 'static)>); + + impl<'a> Iterator for Sources<'a> { + type Item = &'a (dyn std::error::Error + 'static); + + fn next(&mut self) -> Option { + let rem = self.0; + + let next = self.0.and_then(|x| x.source()); + self.0 = next; + rem + } + } + + AnyhowDisplayAlternateAlike(e) +} + +#[cfg(test)] +mod tests { + use super::report_compact_sources; + + #[test] + fn report_compact_sources_examples() { + use std::fmt::Write; + + #[derive(Debug, thiserror::Error)] + enum EvictionError { + #[error("cannot evict a remote layer")] + CannotEvictRemoteLayer, + #[error("stat failed")] + StatFailed(#[source] std::io::Error), + #[error("layer was no longer part of LayerMap")] + LayerNotFound(#[source] anyhow::Error), + } + + let examples = [ + ( + line!(), + EvictionError::CannotEvictRemoteLayer, + "cannot evict a remote layer", + ), + ( + line!(), + EvictionError::StatFailed(std::io::ErrorKind::PermissionDenied.into()), + "stat failed: permission denied", + ), + ( + line!(), + EvictionError::LayerNotFound(anyhow::anyhow!("foobar")), + "layer was no longer part of LayerMap: foobar", + ), + ]; + + let mut s = String::new(); + + for (line, example, expected) in examples { + s.clear(); + + write!(s, "{}", report_compact_sources(&example)).expect("string grows"); + + assert_eq!(s, expected, "example on line {line}"); + } + } +} diff --git a/libs/utils/src/lib.rs b/libs/utils/src/lib.rs index 3bcb092ba7..b591cc611a 100644 --- a/libs/utils/src/lib.rs +++ b/libs/utils/src/lib.rs @@ -63,6 +63,9 @@ pub mod rate_limit; /// Simple once-barrier and a guard which keeps barrier awaiting. pub mod completion; +/// Reporting utilities +pub mod error; + mod failpoint_macro_helpers { /// use with fail::cfg("$name", "return(2000)") diff --git a/pageserver/src/disk_usage_eviction_task.rs b/pageserver/src/disk_usage_eviction_task.rs index 37c52c5423..042d4c6d06 100644 --- a/pageserver/src/disk_usage_eviction_task.rs +++ b/pageserver/src/disk_usage_eviction_task.rs @@ -60,7 +60,7 @@ use utils::serde_percent::Percent; use crate::{ config::PageServerConf, task_mgr::{self, TaskKind, BACKGROUND_RUNTIME}, - tenant::{self, storage_layer::PersistentLayer, Timeline}, + tenant::{self, storage_layer::PersistentLayer, timeline::EvictionError, Timeline}, }; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -390,13 +390,22 @@ pub async fn disk_usage_eviction_task_iteration_impl( assert_eq!(results.len(), batch.len()); for (result, layer) in results.into_iter().zip(batch.iter()) { match result { - Some(Ok(true)) => { + Some(Ok(())) => { usage_assumed.add_available_bytes(layer.file_size()); } - Some(Ok(false)) => { - // this is: - // - Replacement::{NotFound, Unexpected} - // - it cannot be is_remote_layer, filtered already + Some(Err(EvictionError::CannotEvictRemoteLayer)) => { + unreachable!("get_local_layers_for_disk_usage_eviction finds only local layers") + } + Some(Err(EvictionError::FileNotFound)) => { + evictions_failed.file_sizes += layer.file_size(); + evictions_failed.count += 1; + } + Some(Err( + e @ EvictionError::LayerNotFound(_) + | e @ EvictionError::StatFailed(_), + )) => { + let e = utils::error::report_compact_sources(&e); + warn!(%layer, "failed to evict layer: {e}"); evictions_failed.file_sizes += layer.file_size(); evictions_failed.count += 1; } @@ -404,10 +413,6 @@ pub async fn disk_usage_eviction_task_iteration_impl( assert!(cancel.is_cancelled()); return; } - Some(Err(e)) => { - // we really shouldn't be getting this, precondition failure - error!("failed to evict layer: {:#}", e); - } } } } diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 379db0720f..44f1a6cd65 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -121,7 +121,7 @@ pub mod mgr; pub mod tasks; pub mod upload_queue; -mod timeline; +pub(crate) mod timeline; pub mod size; @@ -1168,7 +1168,7 @@ impl Tenant { ) } - /// Helper for unit tests to create an emtpy timeline. + /// Helper for unit tests to create an empty timeline. /// /// The timeline is has state value `Active` but its background loops are not running. // This makes the various functions which anyhow::ensure! for Active state work in tests. @@ -3359,14 +3359,18 @@ pub mod harness { pub async fn load(&self) -> (Arc, RequestContext) { let ctx = RequestContext::new(TaskKind::UnitTest, DownloadBehavior::Error); ( - self.try_load(&ctx) + self.try_load(&ctx, None) .await .expect("failed to load test tenant"), ctx, ) } - pub async fn try_load(&self, ctx: &RequestContext) -> anyhow::Result> { + pub async fn try_load( + &self, + ctx: &RequestContext, + remote_storage: Option, + ) -> anyhow::Result> { let walredo_mgr = Arc::new(TestRedoManager); let tenant = Arc::new(Tenant::new( @@ -3375,7 +3379,7 @@ pub mod harness { TenantConfOpt::from(self.tenant_conf), walredo_mgr, self.tenant_id, - None, + remote_storage, )); tenant .load(None, ctx) @@ -3913,7 +3917,11 @@ mod tests { metadata_bytes[8] ^= 1; std::fs::write(metadata_path, metadata_bytes)?; - let err = harness.try_load(&ctx).await.err().expect("should fail"); + let err = harness + .try_load(&ctx, None) + .await + .err() + .expect("should fail"); // get all the stack with all .context, not tonly the last one let message = format!("{err:#}"); let expected = "Failed to parse metadata bytes from path"; diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 58144d9050..887deeedf0 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -1011,11 +1011,11 @@ impl Timeline { .evict_layer_batch(remote_client, &[local_layer], cancel) .await?; assert_eq!(results.len(), 1); - let result: Option> = results.into_iter().next().unwrap(); + let result: Option> = results.into_iter().next().unwrap(); match result { None => anyhow::bail!("task_mgr shutdown requested"), - Some(Ok(b)) => Ok(Some(b)), - Some(Err(e)) => Err(e), + Some(Ok(())) => Ok(Some(true)), + Some(Err(e)) => Err(anyhow::Error::new(e)), } } @@ -1024,12 +1024,12 @@ impl Timeline { /// GenericRemoteStorage reference is required as a (witness)[witness_article] for "remote storage is configured." /// /// [witness_article]: https://willcrichton.net/rust-api-type-patterns/witnesses.html - pub async fn evict_layers( + pub(crate) async fn evict_layers( &self, _: &GenericRemoteStorage, layers_to_evict: &[Arc], cancel: CancellationToken, - ) -> anyhow::Result>>> { + ) -> anyhow::Result>>> { let remote_client = self.remote_client.clone().expect( "GenericRemoteStorage is configured, so timeline must have RemoteTimelineClient", ); @@ -1064,7 +1064,7 @@ impl Timeline { remote_client: &Arc, layers_to_evict: &[Arc], cancel: CancellationToken, - ) -> anyhow::Result>>> { + ) -> anyhow::Result>>> { // ensure that the layers have finished uploading // (don't hold the layer_removal_cs while we do it, we're not removing anything yet) remote_client @@ -1110,11 +1110,9 @@ impl Timeline { _layer_removal_cs: &tokio::sync::MutexGuard<'_, ()>, local_layer: &Arc, layer_mgr: &mut LayerManager, - ) -> anyhow::Result { + ) -> Result<(), EvictionError> { if local_layer.is_remote_layer() { - // TODO(issue #3851): consider returning an err here instead of false, - // which is the same out the match later - return Ok(false); + return Err(EvictionError::CannotEvictRemoteLayer); } let layer_file_size = local_layer.file_size(); @@ -1123,13 +1121,22 @@ impl Timeline { .local_path() .expect("local layer should have a local path") .metadata() - .context("get local layer file stat")? + // when the eviction fails because we have already deleted the layer in compaction for + // example, a NotFound error bubbles up from here. + .map_err(|e| { + if e.kind() == std::io::ErrorKind::NotFound { + EvictionError::FileNotFound + } else { + EvictionError::StatFailed(e) + } + })? .modified() - .context("get mtime of layer file")?; + .map_err(EvictionError::StatFailed)?; + let local_layer_residence_duration = match SystemTime::now().duration_since(local_layer_mtime) { Err(e) => { - warn!("layer mtime is in the future: {}", e); + warn!(layer = %local_layer, "layer mtime is in the future: {}", e); None } Ok(delta) => Some(delta), @@ -1160,54 +1167,65 @@ impl Timeline { assert_eq!(local_layer.layer_desc(), new_remote_layer.layer_desc()); - let succeed = match layer_mgr.replace_and_verify(local_layer.clone(), new_remote_layer) { - Ok(()) => { - if let Err(e) = local_layer.delete_resident_layer_file() { - error!("failed to remove layer file on evict after replacement: {e:#?}"); - } - // Always decrement the physical size gauge, even if we failed to delete the file. - // Rationale: we already replaced the layer with a remote layer in the layer map, - // and any subsequent download_remote_layer will - // 1. overwrite the file on disk and - // 2. add the downloaded size to the resident size gauge. - // - // If there is no re-download, and we restart the pageserver, then load_layer_map - // will treat the file as a local layer again, count it towards resident size, - // and it'll be like the layer removal never happened. - // The bump in resident size is perhaps unexpected but overall a robust behavior. - self.metrics - .resident_physical_size_gauge - .sub(layer_file_size); + layer_mgr + .replace_and_verify(local_layer.clone(), new_remote_layer) + .map_err(EvictionError::LayerNotFound)?; - self.metrics.evictions.inc(); + if let Err(e) = local_layer.delete_resident_layer_file() { + // this should never happen, because of layer_removal_cs usage and above stat + // access for mtime + error!("failed to remove layer file on evict after replacement: {e:#?}"); + } + // Always decrement the physical size gauge, even if we failed to delete the file. + // Rationale: we already replaced the layer with a remote layer in the layer map, + // and any subsequent download_remote_layer will + // 1. overwrite the file on disk and + // 2. add the downloaded size to the resident size gauge. + // + // If there is no re-download, and we restart the pageserver, then load_layer_map + // will treat the file as a local layer again, count it towards resident size, + // and it'll be like the layer removal never happened. + // The bump in resident size is perhaps unexpected but overall a robust behavior. + self.metrics + .resident_physical_size_gauge + .sub(layer_file_size); - if let Some(delta) = local_layer_residence_duration { - self.metrics - .evictions_with_low_residence_duration - .read() - .unwrap() - .observe(delta); - info!(layer=%local_layer, residence_millis=delta.as_millis(), "evicted layer after known residence period"); - } else { - info!(layer=%local_layer, "evicted layer after unknown residence period"); - } + self.metrics.evictions.inc(); - true - } - Err(err) => { - if cfg!(debug_assertions) { - panic!("failed to replace: {err}, evicted: {local_layer:?}"); - } else { - error!(evicted=?local_layer, "failed to replace: {err}"); - } - false - } - }; + if let Some(delta) = local_layer_residence_duration { + self.metrics + .evictions_with_low_residence_duration + .read() + .unwrap() + .observe(delta); + info!(layer=%local_layer, residence_millis=delta.as_millis(), "evicted layer after known residence period"); + } else { + info!(layer=%local_layer, "evicted layer after unknown residence period"); + } - Ok(succeed) + Ok(()) } } +#[derive(Debug, thiserror::Error)] +pub(crate) enum EvictionError { + #[error("cannot evict a remote layer")] + CannotEvictRemoteLayer, + /// Most likely the to-be evicted layer has been deleted by compaction or gc which use the same + /// locks, so they got to execute before the eviction. + #[error("file backing the layer has been removed already")] + FileNotFound, + #[error("stat failed")] + StatFailed(#[source] std::io::Error), + /// In practice, this can be a number of things, but lets assume it means only this. + /// + /// This case includes situations such as the Layer was evicted and redownloaded in between, + /// because the file existed before an replacement attempt was made but now the Layers are + /// different objects in memory. + #[error("layer was no longer part of LayerMap")] + LayerNotFound(#[source] anyhow::Error), +} + /// Number of times we will compute partition within a checkpoint distance. const REPARTITION_FREQ_IN_CHECKPOINT_DISTANCE: u64 = 10; @@ -4572,6 +4590,7 @@ impl LocalLayerInfoForDiskUsageEviction { } impl Timeline { + /// Returns non-remote layers for eviction. pub(crate) async fn get_local_layers_for_disk_usage_eviction(&self) -> DiskUsageEvictionInfo { let guard = self.layers.read().await; let layers = guard.layer_map(); @@ -4741,3 +4760,180 @@ pub fn compare_arced_layers(left: &Arc, right: &Arc) -> bool { left == right } + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use utils::{id::TimelineId, lsn::Lsn}; + + use crate::tenant::{harness::TenantHarness, storage_layer::PersistentLayer}; + + use super::{EvictionError, Timeline}; + + #[tokio::test] + async fn two_layer_eviction_attempts_at_the_same_time() { + let harness = + TenantHarness::create("two_layer_eviction_attempts_at_the_same_time").unwrap(); + + let remote_storage = { + // this is never used for anything, because of how the create_test_timeline works, but + // it is with us in spirit and a Some. + use remote_storage::{GenericRemoteStorage, RemoteStorageConfig, RemoteStorageKind}; + let path = harness.conf.workdir.join("localfs"); + std::fs::create_dir_all(&path).unwrap(); + let config = RemoteStorageConfig { + max_concurrent_syncs: std::num::NonZeroUsize::new(2_000_000).unwrap(), + max_sync_errors: std::num::NonZeroU32::new(3_000_000).unwrap(), + storage: RemoteStorageKind::LocalFs(path), + }; + GenericRemoteStorage::from_config(&config).unwrap() + }; + + let ctx = any_context(); + let tenant = harness.try_load(&ctx, Some(remote_storage)).await.unwrap(); + let timeline = tenant + .create_test_timeline(TimelineId::generate(), Lsn(0x10), 14, &ctx) + .await + .unwrap(); + + let rc = timeline + .remote_client + .clone() + .expect("just configured this"); + + let layer = find_some_layer(&timeline).await; + + let cancel = tokio_util::sync::CancellationToken::new(); + let batch = [layer]; + + let first = { + let cancel = cancel.clone(); + async { + timeline + .evict_layer_batch(&rc, &batch, cancel) + .await + .unwrap() + } + }; + let second = async { + timeline + .evict_layer_batch(&rc, &batch, cancel) + .await + .unwrap() + }; + + let (first, second) = tokio::join!(first, second); + + let (first, second) = (only_one(first), only_one(second)); + + match (first, second) { + (Ok(()), Err(EvictionError::FileNotFound)) + | (Err(EvictionError::FileNotFound), Ok(())) => { + // one of the evictions gets to do it, + // other one gets FileNotFound. all is good. + } + other => unreachable!("unexpected {:?}", other), + } + } + + #[tokio::test] + async fn layer_eviction_aba_fails() { + let harness = + TenantHarness::create("two_layer_eviction_attempts_at_the_same_time").unwrap(); + + let remote_storage = { + // this is never used for anything, because of how the create_test_timeline works, but + // it is with us in spirit and a Some. + use remote_storage::{GenericRemoteStorage, RemoteStorageConfig, RemoteStorageKind}; + let path = harness.conf.workdir.join("localfs"); + std::fs::create_dir_all(&path).unwrap(); + let config = RemoteStorageConfig { + max_concurrent_syncs: std::num::NonZeroUsize::new(2_000_000).unwrap(), + max_sync_errors: std::num::NonZeroU32::new(3_000_000).unwrap(), + storage: RemoteStorageKind::LocalFs(path), + }; + GenericRemoteStorage::from_config(&config).unwrap() + }; + + let ctx = any_context(); + let tenant = harness.try_load(&ctx, Some(remote_storage)).await.unwrap(); + let timeline = tenant + .create_test_timeline(TimelineId::generate(), Lsn(0x10), 14, &ctx) + .await + .unwrap(); + + let _e = tracing::info_span!("foobar", tenant_id = %tenant.tenant_id, timeline_id = %timeline.timeline_id).entered(); + + let rc = timeline.remote_client.clone().unwrap(); + + // TenantHarness allows uploads to happen given GenericRemoteStorage is configured + let layer = find_some_layer(&timeline).await; + + let cancel = tokio_util::sync::CancellationToken::new(); + let batch = [layer]; + + let first = { + let cancel = cancel.clone(); + async { + timeline + .evict_layer_batch(&rc, &batch, cancel) + .await + .unwrap() + } + }; + + // lets imagine this is stuck somehow, still referencing the original `Arc` + let second = { + let cancel = cancel.clone(); + async { + timeline + .evict_layer_batch(&rc, &batch, cancel) + .await + .unwrap() + } + }; + + // while it's stuck, we evict and end up redownloading it + only_one(first.await).expect("eviction succeeded"); + + let layer = find_some_layer(&timeline).await; + let layer = layer.downcast_remote_layer().unwrap(); + timeline.download_remote_layer(layer).await.unwrap(); + + let res = only_one(second.await); + + assert!( + matches!(res, Err(EvictionError::LayerNotFound(_))), + "{res:?}" + ); + + // no more specific asserting, outside of preconds this is the only valid replacement + // failure + } + + fn any_context() -> crate::context::RequestContext { + use crate::context::*; + use crate::task_mgr::*; + RequestContext::new(TaskKind::UnitTest, DownloadBehavior::Error) + } + + fn only_one(mut input: Vec>) -> T { + assert_eq!(1, input.len()); + input + .pop() + .expect("length just checked") + .expect("no cancellation") + } + + async fn find_some_layer(timeline: &Timeline) -> Arc { + let layers = timeline.layers.read().await; + let desc = layers + .layer_map() + .iter_historic_layers() + .next() + .expect("must find one layer to evict"); + + layers.get_from_desc(&desc) + } +} diff --git a/pageserver/src/tenant/timeline/eviction_task.rs b/pageserver/src/tenant/timeline/eviction_task.rs index a5f570d942..5485cc42b4 100644 --- a/pageserver/src/tenant/timeline/eviction_task.rs +++ b/pageserver/src/tenant/timeline/eviction_task.rs @@ -30,6 +30,7 @@ use crate::{ tenant::{ config::{EvictionPolicy, EvictionPolicyLayerAccessThreshold}, storage_layer::PersistentLayer, + timeline::EvictionError, LogicalSizeCalculationCause, Tenant, }, }; @@ -270,20 +271,22 @@ impl Timeline { None => { stats.skipped_for_shutdown += 1; } - Some(Ok(true)) => { - debug!("evicted layer {l:?}"); + Some(Ok(())) => { stats.evicted += 1; } - Some(Ok(false)) => { - debug!("layer is not evictable: {l:?}"); + Some(Err(EvictionError::CannotEvictRemoteLayer)) => { stats.not_evictable += 1; } - Some(Err(e)) => { - // This variant is the case where an unexpected error happened during eviction. - // Expected errors that result in non-eviction are `Some(Ok(false))`. - // So, dump Debug here to gather as much info as possible in this rare case. - warn!("failed to evict layer {l:?}: {e:?}"); - stats.errors += 1; + Some(Err(EvictionError::FileNotFound)) => { + // compaction/gc removed the file while we were waiting on layer_removal_cs + stats.not_evictable += 1; + } + Some(Err( + e @ EvictionError::LayerNotFound(_) | e @ EvictionError::StatFailed(_), + )) => { + let e = utils::error::report_compact_sources(&e); + warn!(layer = %l, "failed to evict layer: {e}"); + stats.not_evictable += 1; } } } From 89746a48c63715753fe8f68e471c9bdfb9516d24 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Thu, 20 Jul 2023 19:55:40 +0300 Subject: [PATCH 11/23] chore: fix copypaste caused flakyness (#4763) I introduced a copypaste error leading to flaky [test failure][report] in #4737. Solution is to use correct/unique test name. I also looked into providing a proper fn name via macro but ... Yeah, it's probably not a great idea. [report]: https://github.com/neondatabase/neon/actions/runs/5612473297/job/15206293430#step:15:197 --- pageserver/src/tenant/timeline.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 887deeedf0..e5fedeb73e 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -4839,8 +4839,7 @@ mod tests { #[tokio::test] async fn layer_eviction_aba_fails() { - let harness = - TenantHarness::create("two_layer_eviction_attempts_at_the_same_time").unwrap(); + let harness = TenantHarness::create("layer_eviction_aba_fails").unwrap(); let remote_storage = { // this is never used for anything, because of how the create_test_timeline works, but From 3fc3666df73f3d148973831762bf87508a92d3e3 Mon Sep 17 00:00:00 2001 From: Alex Chi Z Date: Thu, 20 Jul 2023 13:39:19 -0400 Subject: [PATCH 12/23] make flush frozen layer an atomic operation (#4720) ## Problem close https://github.com/neondatabase/neon/issues/4712 ## Summary of changes Previously, when flushing frozen layers, it was split into two operations: add delta layer to disk + remove frozen layer from memory. This would cause a short period of time where we will have the same data both in frozen and delta layer. In this PR, we merge them into one atomic operation in layer map manager, therefore simplifying the code. Note that if we decide to create image layers for L0 flush, it will still be split into two operations on layer map. --------- Signed-off-by: Alex Chi Z Co-authored-by: Joonas Koivunen --- pageserver/src/tenant/timeline.rs | 83 ++++++++++--------- .../src/tenant/timeline/layer_manager.rs | 19 ++++- test_runner/regress/test_ancestor_branch.py | 2 +- test_runner/regress/test_recovery.py | 4 +- test_runner/regress/test_remote_storage.py | 4 +- 5 files changed, 64 insertions(+), 48 deletions(-) diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index e5fedeb73e..c6ceae500b 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -2703,7 +2703,7 @@ impl Timeline { // files instead. This is possible as long as *all* the data imported into the // repository have the same LSN. let lsn_range = frozen_layer.get_lsn_range(); - let layer_paths_to_upload = + let (layer_paths_to_upload, delta_layer_to_add) = if lsn_range.start == self.initdb_lsn && lsn_range.end == Lsn(self.initdb_lsn.0 + 1) { #[cfg(test)] match &mut *self.flush_loop_state.lock().unwrap() { @@ -2722,8 +2722,12 @@ impl Timeline { let (partitioning, _lsn) = self .repartition(self.initdb_lsn, self.get_compaction_target_size(), ctx) .await?; - self.create_image_layers(&partitioning, self.initdb_lsn, true, ctx) - .await? + // For image layers, we add them immediately into the layer map. + ( + self.create_image_layers(&partitioning, self.initdb_lsn, true, ctx) + .await?, + None, + ) } else { #[cfg(test)] match &mut *self.flush_loop_state.lock().unwrap() { @@ -2737,35 +2741,50 @@ impl Timeline { assert!(!*expect_initdb_optimization, "expected initdb optimization"); } } - // normal case, write out a L0 delta layer file. - let (delta_path, metadata) = self.create_delta_layer(&frozen_layer).await?; - HashMap::from([(delta_path, metadata)]) + // Normal case, write out a L0 delta layer file. + // `create_delta_layer` will not modify the layer map. + // We will remove frozen layer and add delta layer in one atomic operation later. + let layer = self.create_delta_layer(&frozen_layer).await?; + ( + HashMap::from([(layer.filename(), LayerFileMetadata::new(layer.file_size()))]), + Some(layer), + ) }; - // FIXME: between create_delta_layer and the scheduling of the upload in `update_metadata_file`, - // a compaction can delete the file and then it won't be available for uploads any more. - // We still schedule the upload, resulting in an error, but ideally we'd somehow avoid this - // race situation. - // See https://github.com/neondatabase/neon/issues/4526 - - pausable_failpoint!("flush-frozen-before-sync"); - // The new on-disk layers are now in the layer map. We can remove the // in-memory layer from the map now. The flushed layer is stored in // the mapping in `create_delta_layer`. { let mut guard = self.layers.write().await; - let l = guard.layer_map_mut().frozen_layers.pop_front(); - // Only one thread may call this function at a time (for this - // timeline). If two threads tried to flush the same frozen - // layer to disk at the same time, that would not work. - assert!(compare_arced_layers(&l.unwrap(), &frozen_layer)); + if let Some(ref l) = delta_layer_to_add { + // TODO: move access stats, metrics update, etc. into layer manager. + l.access_stats().record_residence_event( + &guard, + LayerResidenceStatus::Resident, + LayerResidenceEventReason::LayerCreate, + ); + // update metrics + let sz = l.file_size(); + self.metrics.resident_physical_size_gauge.add(sz); + self.metrics.num_persistent_files_created.inc_by(1); + self.metrics.persistent_bytes_written.inc_by(sz); + } + + guard.finish_flush_l0_layer(delta_layer_to_add, &frozen_layer); // release lock on 'layers' } - fail_point!("checkpoint-after-sync"); + // FIXME: between create_delta_layer and the scheduling of the upload in `update_metadata_file`, + // a compaction can delete the file and then it won't be available for uploads any more. + // We still schedule the upload, resulting in an error, but ideally we'd somehow avoid this + // race situation. + // See https://github.com/neondatabase/neon/issues/4526 + pausable_failpoint!("flush-frozen-pausable"); + + // This failpoint is used by another test case `test_pageserver_recovery`. + fail_point!("flush-frozen-exit"); // Update the metadata file, with new 'disk_consistent_lsn' // @@ -2847,11 +2866,12 @@ impl Timeline { Ok(()) } - // Write out the given frozen in-memory layer as a new L0 delta file + // Write out the given frozen in-memory layer as a new L0 delta file. This L0 file will not be tracked + // in layer map immediately. The caller is responsible to put it into the layer map. async fn create_delta_layer( self: &Arc, frozen_layer: &Arc, - ) -> anyhow::Result<(LayerFileName, LayerFileMetadata)> { + ) -> anyhow::Result { let span = tracing::info_span!("blocking"); let new_delta: DeltaLayer = tokio::task::spawn_blocking({ let _g = span.entered(); @@ -2888,25 +2908,8 @@ impl Timeline { }) .await .context("spawn_blocking")??; - let new_delta_name = new_delta.filename(); - let sz = new_delta.desc.file_size; - // Add it to the layer map - let l = Arc::new(new_delta); - let mut guard = self.layers.write().await; - l.access_stats().record_residence_event( - &guard, - LayerResidenceStatus::Resident, - LayerResidenceEventReason::LayerCreate, - ); - guard.track_new_l0_delta_layer(l); - - // update metrics - self.metrics.resident_physical_size_gauge.add(sz); - self.metrics.num_persistent_files_created.inc_by(1); - self.metrics.persistent_bytes_written.inc_by(sz); - - Ok((new_delta_name, LayerFileMetadata::new(sz))) + Ok(new_delta) } async fn repartition( diff --git a/pageserver/src/tenant/timeline/layer_manager.rs b/pageserver/src/tenant/timeline/layer_manager.rs index 77f5f38314..f6f0d533d1 100644 --- a/pageserver/src/tenant/timeline/layer_manager.rs +++ b/pageserver/src/tenant/timeline/layer_manager.rs @@ -194,10 +194,23 @@ impl LayerManager { updates.flush(); } - /// Insert into the layer map when a new delta layer is created, called from `create_delta_layer`. - pub fn track_new_l0_delta_layer(&mut self, delta_layer: Arc) { + /// Flush a frozen layer and add the written delta layer to the layer map. + pub fn finish_flush_l0_layer( + &mut self, + delta_layer: Option, + frozen_layer_for_check: &Arc, + ) { + let l = self.layer_map.frozen_layers.pop_front(); let mut updates = self.layer_map.batch_update(); - Self::insert_historic_layer(delta_layer, &mut updates, &mut self.layer_fmgr); + + // Only one thread may call this function at a time (for this + // timeline). If two threads tried to flush the same frozen + // layer to disk at the same time, that would not work. + assert!(compare_arced_layers(&l.unwrap(), frozen_layer_for_check)); + + if let Some(delta_layer) = delta_layer { + Self::insert_historic_layer(Arc::new(delta_layer), &mut updates, &mut self.layer_fmgr); + } updates.flush(); } diff --git a/test_runner/regress/test_ancestor_branch.py b/test_runner/regress/test_ancestor_branch.py index e8c1a2f34c..0e390ba9e5 100644 --- a/test_runner/regress/test_ancestor_branch.py +++ b/test_runner/regress/test_ancestor_branch.py @@ -20,7 +20,7 @@ def test_ancestor_branch(neon_env_builder: NeonEnvBuilder): } ) - pageserver_http.configure_failpoints(("flush-frozen-before-sync", "sleep(10000)")) + pageserver_http.configure_failpoints(("flush-frozen-pausable", "sleep(10000)")) endpoint_branch0 = env.endpoints.create_start("main", tenant_id=tenant) branch0_cur = endpoint_branch0.connect().cursor() diff --git a/test_runner/regress/test_recovery.py b/test_runner/regress/test_recovery.py index 76e97a35a4..552825cf08 100644 --- a/test_runner/regress/test_recovery.py +++ b/test_runner/regress/test_recovery.py @@ -38,8 +38,8 @@ def test_pageserver_recovery(neon_env_builder: NeonEnvBuilder): # Configure failpoints pageserver_http.configure_failpoints( [ - ("flush-frozen-before-sync", "sleep(2000)"), - ("checkpoint-after-sync", "exit"), + ("flush-frozen-pausable", "sleep(2000)"), + ("flush-frozen-exit", "exit"), ] ) diff --git a/test_runner/regress/test_remote_storage.py b/test_runner/regress/test_remote_storage.py index 13bc01f609..f1575ae4d3 100644 --- a/test_runner/regress/test_remote_storage.py +++ b/test_runner/regress/test_remote_storage.py @@ -815,7 +815,7 @@ def test_compaction_delete_before_upload( wait_for_last_flush_lsn(env, endpoint, tenant_id, timeline_id) # Now make the flushing hang and update one small piece of data - client.configure_failpoints(("flush-frozen-before-sync", "pause")) + client.configure_failpoints(("flush-frozen-pausable", "pause")) endpoint.safe_psql("UPDATE foo SET x = 0 WHERE x = 1") @@ -841,7 +841,7 @@ def test_compaction_delete_before_upload( time.sleep(4) client.timeline_compact(tenant_id, timeline_id) - client.configure_failpoints(("flush-frozen-before-sync", "off")) + client.configure_failpoints(("flush-frozen-pausable", "off")) conflict = q.get() From 8d0f4a785742405a8ca34de01933981d100ee5bf Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 20 Jul 2023 22:33:50 +0300 Subject: [PATCH 13/23] Bump aiohttp from 3.7.4 to 3.8.5 (#4762) --- poetry.lock | 240 +++++++++++++++++++++++++++++++++++++------------ pyproject.toml | 2 +- 2 files changed, 183 insertions(+), 59 deletions(-) diff --git a/poetry.lock b/poetry.lock index b22a6a5bc9..a8ea410734 100644 --- a/poetry.lock +++ b/poetry.lock @@ -2,60 +2,111 @@ [[package]] name = "aiohttp" -version = "3.7.4" +version = "3.8.5" description = "Async http client/server framework (asyncio)" optional = false python-versions = ">=3.6" files = [ - {file = "aiohttp-3.7.4-cp36-cp36m-macosx_10_14_x86_64.whl", hash = "sha256:6c8200abc9dc5f27203986100579fc19ccad7a832c07d2bc151ce4ff17190076"}, - {file = "aiohttp-3.7.4-cp36-cp36m-manylinux1_i686.whl", hash = "sha256:dd7936f2a6daa861143e376b3a1fb56e9b802f4980923594edd9ca5670974895"}, - {file = "aiohttp-3.7.4-cp36-cp36m-manylinux2014_aarch64.whl", hash = "sha256:bc3d14bf71a3fb94e5acf5bbf67331ab335467129af6416a437bd6024e4f743d"}, - {file = "aiohttp-3.7.4-cp36-cp36m-manylinux2014_i686.whl", hash = "sha256:8ec1a38074f68d66ccb467ed9a673a726bb397142c273f90d4ba954666e87d54"}, - {file = "aiohttp-3.7.4-cp36-cp36m-manylinux2014_ppc64le.whl", hash = "sha256:b84ad94868e1e6a5e30d30ec419956042815dfaea1b1df1cef623e4564c374d9"}, - {file = "aiohttp-3.7.4-cp36-cp36m-manylinux2014_s390x.whl", hash = "sha256:d5d102e945ecca93bcd9801a7bb2fa703e37ad188a2f81b1e65e4abe4b51b00c"}, - {file = "aiohttp-3.7.4-cp36-cp36m-manylinux2014_x86_64.whl", hash = "sha256:c2a80fd9a8d7e41b4e38ea9fe149deed0d6aaede255c497e66b8213274d6d61b"}, - {file = "aiohttp-3.7.4-cp36-cp36m-win32.whl", hash = "sha256:481d4b96969fbfdcc3ff35eea5305d8565a8300410d3d269ccac69e7256b1329"}, - {file = "aiohttp-3.7.4-cp36-cp36m-win_amd64.whl", hash = "sha256:16d0683ef8a6d803207f02b899c928223eb219111bd52420ef3d7a8aa76227b6"}, - {file = "aiohttp-3.7.4-cp37-cp37m-macosx_10_14_x86_64.whl", hash = "sha256:eab51036cac2da8a50d7ff0ea30be47750547c9aa1aa2cf1a1b710a1827e7dbe"}, - {file = "aiohttp-3.7.4-cp37-cp37m-manylinux1_i686.whl", hash = "sha256:feb24ff1226beeb056e247cf2e24bba5232519efb5645121c4aea5b6ad74c1f2"}, - {file = "aiohttp-3.7.4-cp37-cp37m-manylinux2014_aarch64.whl", hash = "sha256:119feb2bd551e58d83d1b38bfa4cb921af8ddedec9fad7183132db334c3133e0"}, - {file = "aiohttp-3.7.4-cp37-cp37m-manylinux2014_i686.whl", hash = "sha256:6ca56bdfaf825f4439e9e3673775e1032d8b6ea63b8953d3812c71bd6a8b81de"}, - {file = "aiohttp-3.7.4-cp37-cp37m-manylinux2014_ppc64le.whl", hash = "sha256:5563ad7fde451b1986d42b9bb9140e2599ecf4f8e42241f6da0d3d624b776f40"}, - {file = "aiohttp-3.7.4-cp37-cp37m-manylinux2014_s390x.whl", hash = "sha256:62bc216eafac3204877241569209d9ba6226185aa6d561c19159f2e1cbb6abfb"}, - {file = "aiohttp-3.7.4-cp37-cp37m-manylinux2014_x86_64.whl", hash = "sha256:f4496d8d04da2e98cc9133e238ccebf6a13ef39a93da2e87146c8c8ac9768242"}, - {file = "aiohttp-3.7.4-cp37-cp37m-win32.whl", hash = "sha256:2ffea7904e70350da429568113ae422c88d2234ae776519549513c8f217f58a9"}, - {file = "aiohttp-3.7.4-cp37-cp37m-win_amd64.whl", hash = "sha256:5e91e927003d1ed9283dee9abcb989334fc8e72cf89ebe94dc3e07e3ff0b11e9"}, - {file = "aiohttp-3.7.4-cp38-cp38-macosx_10_14_x86_64.whl", hash = "sha256:4c1bdbfdd231a20eee3e56bd0ac1cd88c4ff41b64ab679ed65b75c9c74b6c5c2"}, - {file = "aiohttp-3.7.4-cp38-cp38-manylinux1_i686.whl", hash = "sha256:71680321a8a7176a58dfbc230789790639db78dad61a6e120b39f314f43f1907"}, - {file = "aiohttp-3.7.4-cp38-cp38-manylinux2014_aarch64.whl", hash = "sha256:7dbd087ff2f4046b9b37ba28ed73f15fd0bc9f4fdc8ef6781913da7f808d9536"}, - {file = "aiohttp-3.7.4-cp38-cp38-manylinux2014_i686.whl", hash = "sha256:dee68ec462ff10c1d836c0ea2642116aba6151c6880b688e56b4c0246770f297"}, - {file = "aiohttp-3.7.4-cp38-cp38-manylinux2014_ppc64le.whl", hash = "sha256:99c5a5bf7135607959441b7d720d96c8e5c46a1f96e9d6d4c9498be8d5f24212"}, - {file = "aiohttp-3.7.4-cp38-cp38-manylinux2014_s390x.whl", hash = "sha256:5dde6d24bacac480be03f4f864e9a67faac5032e28841b00533cd168ab39cad9"}, - {file = "aiohttp-3.7.4-cp38-cp38-manylinux2014_x86_64.whl", hash = "sha256:418597633b5cd9639e514b1d748f358832c08cd5d9ef0870026535bd5eaefdd0"}, - {file = "aiohttp-3.7.4-cp38-cp38-win32.whl", hash = "sha256:e76e78863a4eaec3aee5722d85d04dcbd9844bc6cd3bfa6aa880ff46ad16bfcb"}, - {file = "aiohttp-3.7.4-cp38-cp38-win_amd64.whl", hash = "sha256:950b7ef08b2afdab2488ee2edaff92a03ca500a48f1e1aaa5900e73d6cf992bc"}, - {file = "aiohttp-3.7.4-cp39-cp39-macosx_10_14_x86_64.whl", hash = "sha256:2eb3efe243e0f4ecbb654b08444ae6ffab37ac0ef8f69d3a2ffb958905379daf"}, - {file = "aiohttp-3.7.4-cp39-cp39-manylinux1_i686.whl", hash = "sha256:822bd4fd21abaa7b28d65fc9871ecabaddc42767884a626317ef5b75c20e8a2d"}, - {file = "aiohttp-3.7.4-cp39-cp39-manylinux2014_aarch64.whl", hash = "sha256:58c62152c4c8731a3152e7e650b29ace18304d086cb5552d317a54ff2749d32a"}, - {file = "aiohttp-3.7.4-cp39-cp39-manylinux2014_i686.whl", hash = "sha256:7c7820099e8b3171e54e7eedc33e9450afe7cd08172632d32128bd527f8cb77d"}, - {file = "aiohttp-3.7.4-cp39-cp39-manylinux2014_ppc64le.whl", hash = "sha256:5b50e0b9460100fe05d7472264d1975f21ac007b35dcd6fd50279b72925a27f4"}, - {file = "aiohttp-3.7.4-cp39-cp39-manylinux2014_s390x.whl", hash = "sha256:c44d3c82a933c6cbc21039326767e778eface44fca55c65719921c4b9661a3f7"}, - {file = "aiohttp-3.7.4-cp39-cp39-manylinux2014_x86_64.whl", hash = "sha256:cc31e906be1cc121ee201adbdf844522ea3349600dd0a40366611ca18cd40e81"}, - {file = "aiohttp-3.7.4-cp39-cp39-win32.whl", hash = "sha256:fbd3b5e18d34683decc00d9a360179ac1e7a320a5fee10ab8053ffd6deab76e0"}, - {file = "aiohttp-3.7.4-cp39-cp39-win_amd64.whl", hash = "sha256:40bd1b101b71a18a528ffce812cc14ff77d4a2a1272dfb8b11b200967489ef3e"}, - {file = "aiohttp-3.7.4.tar.gz", hash = "sha256:5d84ecc73141d0a0d61ece0742bb7ff5751b0657dab8405f899d3ceb104cc7de"}, + {file = "aiohttp-3.8.5-cp310-cp310-macosx_10_9_universal2.whl", hash = "sha256:a94159871304770da4dd371f4291b20cac04e8c94f11bdea1c3478e557fbe0d8"}, + {file = "aiohttp-3.8.5-cp310-cp310-macosx_10_9_x86_64.whl", hash = "sha256:13bf85afc99ce6f9ee3567b04501f18f9f8dbbb2ea11ed1a2e079670403a7c84"}, + {file = "aiohttp-3.8.5-cp310-cp310-macosx_11_0_arm64.whl", hash = "sha256:2ce2ac5708501afc4847221a521f7e4b245abf5178cf5ddae9d5b3856ddb2f3a"}, + {file = "aiohttp-3.8.5-cp310-cp310-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:96943e5dcc37a6529d18766597c491798b7eb7a61d48878611298afc1fca946c"}, + {file = "aiohttp-3.8.5-cp310-cp310-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:2ad5c3c4590bb3cc28b4382f031f3783f25ec223557124c68754a2231d989e2b"}, + {file = "aiohttp-3.8.5-cp310-cp310-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:0c413c633d0512df4dc7fd2373ec06cc6a815b7b6d6c2f208ada7e9e93a5061d"}, + {file = "aiohttp-3.8.5-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:df72ac063b97837a80d80dec8d54c241af059cc9bb42c4de68bd5b61ceb37caa"}, + {file = "aiohttp-3.8.5-cp310-cp310-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:c48c5c0271149cfe467c0ff8eb941279fd6e3f65c9a388c984e0e6cf57538e14"}, + {file = "aiohttp-3.8.5-cp310-cp310-musllinux_1_1_aarch64.whl", hash = "sha256:368a42363c4d70ab52c2c6420a57f190ed3dfaca6a1b19afda8165ee16416a82"}, + {file = "aiohttp-3.8.5-cp310-cp310-musllinux_1_1_i686.whl", hash = "sha256:7607ec3ce4993464368505888af5beb446845a014bc676d349efec0e05085905"}, + {file = "aiohttp-3.8.5-cp310-cp310-musllinux_1_1_ppc64le.whl", hash = "sha256:0d21c684808288a98914e5aaf2a7c6a3179d4df11d249799c32d1808e79503b5"}, + {file = "aiohttp-3.8.5-cp310-cp310-musllinux_1_1_s390x.whl", hash = "sha256:312fcfbacc7880a8da0ae8b6abc6cc7d752e9caa0051a53d217a650b25e9a691"}, + {file = "aiohttp-3.8.5-cp310-cp310-musllinux_1_1_x86_64.whl", hash = "sha256:ad093e823df03bb3fd37e7dec9d4670c34f9e24aeace76808fc20a507cace825"}, + {file = "aiohttp-3.8.5-cp310-cp310-win32.whl", hash = "sha256:33279701c04351a2914e1100b62b2a7fdb9a25995c4a104259f9a5ead7ed4802"}, + {file = "aiohttp-3.8.5-cp310-cp310-win_amd64.whl", hash = "sha256:6e4a280e4b975a2e7745573e3fc9c9ba0d1194a3738ce1cbaa80626cc9b4f4df"}, + {file = "aiohttp-3.8.5-cp311-cp311-macosx_10_9_universal2.whl", hash = "sha256:ae871a964e1987a943d83d6709d20ec6103ca1eaf52f7e0d36ee1b5bebb8b9b9"}, + {file = "aiohttp-3.8.5-cp311-cp311-macosx_10_9_x86_64.whl", hash = "sha256:461908b2578955045efde733719d62f2b649c404189a09a632d245b445c9c975"}, + {file = "aiohttp-3.8.5-cp311-cp311-macosx_11_0_arm64.whl", hash = "sha256:72a860c215e26192379f57cae5ab12b168b75db8271f111019509a1196dfc780"}, + {file = "aiohttp-3.8.5-cp311-cp311-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:cc14be025665dba6202b6a71cfcdb53210cc498e50068bc088076624471f8bb9"}, + {file = "aiohttp-3.8.5-cp311-cp311-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:8af740fc2711ad85f1a5c034a435782fbd5b5f8314c9a3ef071424a8158d7f6b"}, + {file = "aiohttp-3.8.5-cp311-cp311-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:841cd8233cbd2111a0ef0a522ce016357c5e3aff8a8ce92bcfa14cef890d698f"}, + {file = "aiohttp-3.8.5-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:5ed1c46fb119f1b59304b5ec89f834f07124cd23ae5b74288e364477641060ff"}, + {file = "aiohttp-3.8.5-cp311-cp311-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:84f8ae3e09a34f35c18fa57f015cc394bd1389bce02503fb30c394d04ee6b938"}, + {file = "aiohttp-3.8.5-cp311-cp311-musllinux_1_1_aarch64.whl", hash = "sha256:62360cb771707cb70a6fd114b9871d20d7dd2163a0feafe43fd115cfe4fe845e"}, + {file = "aiohttp-3.8.5-cp311-cp311-musllinux_1_1_i686.whl", hash = "sha256:23fb25a9f0a1ca1f24c0a371523546366bb642397c94ab45ad3aedf2941cec6a"}, + {file = "aiohttp-3.8.5-cp311-cp311-musllinux_1_1_ppc64le.whl", hash = "sha256:b0ba0d15164eae3d878260d4c4df859bbdc6466e9e6689c344a13334f988bb53"}, + {file = "aiohttp-3.8.5-cp311-cp311-musllinux_1_1_s390x.whl", hash = "sha256:5d20003b635fc6ae3f96d7260281dfaf1894fc3aa24d1888a9b2628e97c241e5"}, + {file = "aiohttp-3.8.5-cp311-cp311-musllinux_1_1_x86_64.whl", hash = "sha256:0175d745d9e85c40dcc51c8f88c74bfbaef9e7afeeeb9d03c37977270303064c"}, + {file = "aiohttp-3.8.5-cp311-cp311-win32.whl", hash = "sha256:2e1b1e51b0774408f091d268648e3d57f7260c1682e7d3a63cb00d22d71bb945"}, + {file = "aiohttp-3.8.5-cp311-cp311-win_amd64.whl", hash = "sha256:043d2299f6dfdc92f0ac5e995dfc56668e1587cea7f9aa9d8a78a1b6554e5755"}, + {file = "aiohttp-3.8.5-cp36-cp36m-macosx_10_9_x86_64.whl", hash = "sha256:cae533195e8122584ec87531d6df000ad07737eaa3c81209e85c928854d2195c"}, + {file = "aiohttp-3.8.5-cp36-cp36m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:4f21e83f355643c345177a5d1d8079f9f28b5133bcd154193b799d380331d5d3"}, + {file = "aiohttp-3.8.5-cp36-cp36m-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:a7a75ef35f2df54ad55dbf4b73fe1da96f370e51b10c91f08b19603c64004acc"}, + {file = "aiohttp-3.8.5-cp36-cp36m-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:2e2e9839e14dd5308ee773c97115f1e0a1cb1d75cbeeee9f33824fa5144c7634"}, + {file = "aiohttp-3.8.5-cp36-cp36m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:c44e65da1de4403d0576473e2344828ef9c4c6244d65cf4b75549bb46d40b8dd"}, + {file = "aiohttp-3.8.5-cp36-cp36m-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:78d847e4cde6ecc19125ccbc9bfac4a7ab37c234dd88fbb3c5c524e8e14da543"}, + {file = "aiohttp-3.8.5-cp36-cp36m-musllinux_1_1_aarch64.whl", hash = "sha256:c7a815258e5895d8900aec4454f38dca9aed71085f227537208057853f9d13f2"}, + {file = "aiohttp-3.8.5-cp36-cp36m-musllinux_1_1_i686.whl", hash = "sha256:8b929b9bd7cd7c3939f8bcfffa92fae7480bd1aa425279d51a89327d600c704d"}, + {file = "aiohttp-3.8.5-cp36-cp36m-musllinux_1_1_ppc64le.whl", hash = "sha256:5db3a5b833764280ed7618393832e0853e40f3d3e9aa128ac0ba0f8278d08649"}, + {file = "aiohttp-3.8.5-cp36-cp36m-musllinux_1_1_s390x.whl", hash = "sha256:a0215ce6041d501f3155dc219712bc41252d0ab76474615b9700d63d4d9292af"}, + {file = "aiohttp-3.8.5-cp36-cp36m-musllinux_1_1_x86_64.whl", hash = "sha256:fd1ed388ea7fbed22c4968dd64bab0198de60750a25fe8c0c9d4bef5abe13824"}, + {file = "aiohttp-3.8.5-cp36-cp36m-win32.whl", hash = "sha256:6e6783bcc45f397fdebc118d772103d751b54cddf5b60fbcc958382d7dd64f3e"}, + {file = "aiohttp-3.8.5-cp36-cp36m-win_amd64.whl", hash = "sha256:b5411d82cddd212644cf9360879eb5080f0d5f7d809d03262c50dad02f01421a"}, + {file = "aiohttp-3.8.5-cp37-cp37m-macosx_10_9_x86_64.whl", hash = "sha256:01d4c0c874aa4ddfb8098e85d10b5e875a70adc63db91f1ae65a4b04d3344cda"}, + {file = "aiohttp-3.8.5-cp37-cp37m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:e5980a746d547a6ba173fd5ee85ce9077e72d118758db05d229044b469d9029a"}, + {file = "aiohttp-3.8.5-cp37-cp37m-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:2a482e6da906d5e6e653be079b29bc173a48e381600161c9932d89dfae5942ef"}, + {file = "aiohttp-3.8.5-cp37-cp37m-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:80bd372b8d0715c66c974cf57fe363621a02f359f1ec81cba97366948c7fc873"}, + {file = "aiohttp-3.8.5-cp37-cp37m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:c1161b345c0a444ebcf46bf0a740ba5dcf50612fd3d0528883fdc0eff578006a"}, + {file = "aiohttp-3.8.5-cp37-cp37m-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:cd56db019015b6acfaaf92e1ac40eb8434847d9bf88b4be4efe5bfd260aee692"}, + {file = "aiohttp-3.8.5-cp37-cp37m-musllinux_1_1_aarch64.whl", hash = "sha256:153c2549f6c004d2754cc60603d4668899c9895b8a89397444a9c4efa282aaf4"}, + {file = "aiohttp-3.8.5-cp37-cp37m-musllinux_1_1_i686.whl", hash = "sha256:4a01951fabc4ce26ab791da5f3f24dca6d9a6f24121746eb19756416ff2d881b"}, + {file = "aiohttp-3.8.5-cp37-cp37m-musllinux_1_1_ppc64le.whl", hash = "sha256:bfb9162dcf01f615462b995a516ba03e769de0789de1cadc0f916265c257e5d8"}, + {file = "aiohttp-3.8.5-cp37-cp37m-musllinux_1_1_s390x.whl", hash = "sha256:7dde0009408969a43b04c16cbbe252c4f5ef4574ac226bc8815cd7342d2028b6"}, + {file = "aiohttp-3.8.5-cp37-cp37m-musllinux_1_1_x86_64.whl", hash = "sha256:4149d34c32f9638f38f544b3977a4c24052042affa895352d3636fa8bffd030a"}, + {file = "aiohttp-3.8.5-cp37-cp37m-win32.whl", hash = "sha256:68c5a82c8779bdfc6367c967a4a1b2aa52cd3595388bf5961a62158ee8a59e22"}, + {file = "aiohttp-3.8.5-cp37-cp37m-win_amd64.whl", hash = "sha256:2cf57fb50be5f52bda004b8893e63b48530ed9f0d6c96c84620dc92fe3cd9b9d"}, + {file = "aiohttp-3.8.5-cp38-cp38-macosx_10_9_universal2.whl", hash = "sha256:eca4bf3734c541dc4f374ad6010a68ff6c6748f00451707f39857f429ca36ced"}, + {file = "aiohttp-3.8.5-cp38-cp38-macosx_10_9_x86_64.whl", hash = "sha256:1274477e4c71ce8cfe6c1ec2f806d57c015ebf84d83373676036e256bc55d690"}, + {file = "aiohttp-3.8.5-cp38-cp38-macosx_11_0_arm64.whl", hash = "sha256:28c543e54710d6158fc6f439296c7865b29e0b616629767e685a7185fab4a6b9"}, + {file = "aiohttp-3.8.5-cp38-cp38-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:910bec0c49637d213f5d9877105d26e0c4a4de2f8b1b29405ff37e9fc0ad52b8"}, + {file = "aiohttp-3.8.5-cp38-cp38-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:5443910d662db951b2e58eb70b0fbe6b6e2ae613477129a5805d0b66c54b6cb7"}, + {file = "aiohttp-3.8.5-cp38-cp38-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:2e460be6978fc24e3df83193dc0cc4de46c9909ed92dd47d349a452ef49325b7"}, + {file = "aiohttp-3.8.5-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:fb1558def481d84f03b45888473fc5a1f35747b5f334ef4e7a571bc0dfcb11f8"}, + {file = "aiohttp-3.8.5-cp38-cp38-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:34dd0c107799dcbbf7d48b53be761a013c0adf5571bf50c4ecad5643fe9cfcd0"}, + {file = "aiohttp-3.8.5-cp38-cp38-musllinux_1_1_aarch64.whl", hash = "sha256:aa1990247f02a54185dc0dff92a6904521172a22664c863a03ff64c42f9b5410"}, + {file = "aiohttp-3.8.5-cp38-cp38-musllinux_1_1_i686.whl", hash = "sha256:0e584a10f204a617d71d359fe383406305a4b595b333721fa50b867b4a0a1548"}, + {file = "aiohttp-3.8.5-cp38-cp38-musllinux_1_1_ppc64le.whl", hash = "sha256:a3cf433f127efa43fee6b90ea4c6edf6c4a17109d1d037d1a52abec84d8f2e42"}, + {file = "aiohttp-3.8.5-cp38-cp38-musllinux_1_1_s390x.whl", hash = "sha256:c11f5b099adafb18e65c2c997d57108b5bbeaa9eeee64a84302c0978b1ec948b"}, + {file = "aiohttp-3.8.5-cp38-cp38-musllinux_1_1_x86_64.whl", hash = "sha256:84de26ddf621d7ac4c975dbea4c945860e08cccde492269db4e1538a6a6f3c35"}, + {file = "aiohttp-3.8.5-cp38-cp38-win32.whl", hash = "sha256:ab88bafedc57dd0aab55fa728ea10c1911f7e4d8b43e1d838a1739f33712921c"}, + {file = "aiohttp-3.8.5-cp38-cp38-win_amd64.whl", hash = "sha256:5798a9aad1879f626589f3df0f8b79b3608a92e9beab10e5fda02c8a2c60db2e"}, + {file = "aiohttp-3.8.5-cp39-cp39-macosx_10_9_universal2.whl", hash = "sha256:a6ce61195c6a19c785df04e71a4537e29eaa2c50fe745b732aa937c0c77169f3"}, + {file = "aiohttp-3.8.5-cp39-cp39-macosx_10_9_x86_64.whl", hash = "sha256:773dd01706d4db536335fcfae6ea2440a70ceb03dd3e7378f3e815b03c97ab51"}, + {file = "aiohttp-3.8.5-cp39-cp39-macosx_11_0_arm64.whl", hash = "sha256:f83a552443a526ea38d064588613aca983d0ee0038801bc93c0c916428310c28"}, + {file = "aiohttp-3.8.5-cp39-cp39-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:1f7372f7341fcc16f57b2caded43e81ddd18df53320b6f9f042acad41f8e049a"}, + {file = "aiohttp-3.8.5-cp39-cp39-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:ea353162f249c8097ea63c2169dd1aa55de1e8fecbe63412a9bc50816e87b761"}, + {file = "aiohttp-3.8.5-cp39-cp39-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:e5d47ae48db0b2dcf70bc8a3bc72b3de86e2a590fc299fdbbb15af320d2659de"}, + {file = "aiohttp-3.8.5-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:d827176898a2b0b09694fbd1088c7a31836d1a505c243811c87ae53a3f6273c1"}, + {file = "aiohttp-3.8.5-cp39-cp39-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:3562b06567c06439d8b447037bb655ef69786c590b1de86c7ab81efe1c9c15d8"}, + {file = "aiohttp-3.8.5-cp39-cp39-musllinux_1_1_aarch64.whl", hash = "sha256:4e874cbf8caf8959d2adf572a78bba17cb0e9d7e51bb83d86a3697b686a0ab4d"}, + {file = "aiohttp-3.8.5-cp39-cp39-musllinux_1_1_i686.whl", hash = "sha256:6809a00deaf3810e38c628e9a33271892f815b853605a936e2e9e5129762356c"}, + {file = "aiohttp-3.8.5-cp39-cp39-musllinux_1_1_ppc64le.whl", hash = "sha256:33776e945d89b29251b33a7e7d006ce86447b2cfd66db5e5ded4e5cd0340585c"}, + {file = "aiohttp-3.8.5-cp39-cp39-musllinux_1_1_s390x.whl", hash = "sha256:eaeed7abfb5d64c539e2db173f63631455f1196c37d9d8d873fc316470dfbacd"}, + {file = "aiohttp-3.8.5-cp39-cp39-musllinux_1_1_x86_64.whl", hash = "sha256:e91d635961bec2d8f19dfeb41a539eb94bd073f075ca6dae6c8dc0ee89ad6f91"}, + {file = "aiohttp-3.8.5-cp39-cp39-win32.whl", hash = "sha256:00ad4b6f185ec67f3e6562e8a1d2b69660be43070bd0ef6fcec5211154c7df67"}, + {file = "aiohttp-3.8.5-cp39-cp39-win_amd64.whl", hash = "sha256:c0a9034379a37ae42dea7ac1e048352d96286626251862e448933c0f59cbd79c"}, + {file = "aiohttp-3.8.5.tar.gz", hash = "sha256:b9552ec52cc147dbf1944ac7ac98af7602e51ea2dcd076ed194ca3c0d1c7d0bc"}, ] [package.dependencies] -async-timeout = ">=3.0,<4.0" +aiosignal = ">=1.1.2" +async-timeout = ">=4.0.0a3,<5.0" attrs = ">=17.3.0" -chardet = ">=2.0,<4.0" +charset-normalizer = ">=2.0,<4.0" +frozenlist = ">=1.1.1" multidict = ">=4.5,<7.0" -typing-extensions = ">=3.6.5" yarl = ">=1.0,<2.0" [package.extras] -speedups = ["aiodns", "brotlipy", "cchardet"] +speedups = ["Brotli", "aiodns", "cchardet"] [[package]] name = "aiopg" @@ -75,6 +126,20 @@ psycopg2-binary = ">=2.8.4" [package.extras] sa = ["sqlalchemy[postgresql-psycopg2binary] (>=1.3,<1.5)"] +[[package]] +name = "aiosignal" +version = "1.3.1" +description = "aiosignal: a list of registered asynchronous callbacks" +optional = false +python-versions = ">=3.7" +files = [ + {file = "aiosignal-1.3.1-py3-none-any.whl", hash = "sha256:f8376fb07dd1e86a584e4fcdec80b36b7f81aac666ebc724e2c090300dd83b17"}, + {file = "aiosignal-1.3.1.tar.gz", hash = "sha256:54cd96e15e1649b75d6c87526a6ff0b6c1b0dd3459f43d9ca11d48c339b68cfc"}, +] + +[package.dependencies] +frozenlist = ">=1.1.0" + [[package]] name = "allure-pytest" version = "2.13.2" @@ -107,13 +172,13 @@ pluggy = ">=0.4.0" [[package]] name = "async-timeout" -version = "3.0.1" +version = "4.0.2" description = "Timeout context manager for asyncio programs" optional = false -python-versions = ">=3.5.3" +python-versions = ">=3.6" files = [ - {file = "async-timeout-3.0.1.tar.gz", hash = "sha256:0c3c816a028d47f659d6ff5c745cb2acf1f966da1fe5c19c77a70282b25f4c5f"}, - {file = "async_timeout-3.0.1-py3-none-any.whl", hash = "sha256:4291ca197d287d274d0b6cb5d6f8f8f82d434ed288f962539ff18cc9012f9ea3"}, + {file = "async-timeout-4.0.2.tar.gz", hash = "sha256:2163e1640ddb52b7a8c80d0a67a08587e5d245cc9c553a74a847056bc2976b15"}, + {file = "async_timeout-4.0.2-py3-none-any.whl", hash = "sha256:8ca1e4fcf50d07413d66d1a5e416e42cfdf5851c981d679a09851a6853383b3c"}, ] [[package]] @@ -781,17 +846,6 @@ networkx = ">=2.4,<3.0" pyyaml = ">5.4" sarif-om = ">=1.0.4,<1.1.0" -[[package]] -name = "chardet" -version = "3.0.4" -description = "Universal encoding detector for Python 2 and 3" -optional = false -python-versions = "*" -files = [ - {file = "chardet-3.0.4-py2.py3-none-any.whl", hash = "sha256:fc323ffcaeaed0e0a02bf4d117757b98aed530d9ed4531e3e15460124c106691"}, - {file = "chardet-3.0.4.tar.gz", hash = "sha256:84ab92ed1c4d4f16916e05906b6b75a6c0fb5db821cc65e70cbd64a3e2a5eaae"}, -] - [[package]] name = "charset-normalizer" version = "2.1.0" @@ -980,6 +1034,76 @@ files = [ Flask = ">=0.9" Six = "*" +[[package]] +name = "frozenlist" +version = "1.4.0" +description = "A list-like structure which implements collections.abc.MutableSequence" +optional = false +python-versions = ">=3.8" +files = [ + {file = "frozenlist-1.4.0-cp310-cp310-macosx_10_9_universal2.whl", hash = "sha256:764226ceef3125e53ea2cb275000e309c0aa5464d43bd72abd661e27fffc26ab"}, + {file = "frozenlist-1.4.0-cp310-cp310-macosx_10_9_x86_64.whl", hash = "sha256:d6484756b12f40003c6128bfcc3fa9f0d49a687e171186c2d85ec82e3758c559"}, + {file = "frozenlist-1.4.0-cp310-cp310-macosx_11_0_arm64.whl", hash = "sha256:9ac08e601308e41eb533f232dbf6b7e4cea762f9f84f6357136eed926c15d12c"}, + {file = "frozenlist-1.4.0-cp310-cp310-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:d081f13b095d74b67d550de04df1c756831f3b83dc9881c38985834387487f1b"}, + {file = "frozenlist-1.4.0-cp310-cp310-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:71932b597f9895f011f47f17d6428252fc728ba2ae6024e13c3398a087c2cdea"}, + {file = "frozenlist-1.4.0-cp310-cp310-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:981b9ab5a0a3178ff413bca62526bb784249421c24ad7381e39d67981be2c326"}, + {file = "frozenlist-1.4.0-cp310-cp310-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:e41f3de4df3e80de75845d3e743b3f1c4c8613c3997a912dbf0229fc61a8b963"}, + {file = "frozenlist-1.4.0-cp310-cp310-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:6918d49b1f90821e93069682c06ffde41829c346c66b721e65a5c62b4bab0300"}, + {file = "frozenlist-1.4.0-cp310-cp310-musllinux_1_1_aarch64.whl", hash = "sha256:0e5c8764c7829343d919cc2dfc587a8db01c4f70a4ebbc49abde5d4b158b007b"}, + {file = "frozenlist-1.4.0-cp310-cp310-musllinux_1_1_i686.whl", hash = "sha256:8d0edd6b1c7fb94922bf569c9b092ee187a83f03fb1a63076e7774b60f9481a8"}, + {file = "frozenlist-1.4.0-cp310-cp310-musllinux_1_1_ppc64le.whl", hash = "sha256:e29cda763f752553fa14c68fb2195150bfab22b352572cb36c43c47bedba70eb"}, + {file = "frozenlist-1.4.0-cp310-cp310-musllinux_1_1_s390x.whl", hash = "sha256:0c7c1b47859ee2cac3846fde1c1dc0f15da6cec5a0e5c72d101e0f83dcb67ff9"}, + {file = "frozenlist-1.4.0-cp310-cp310-musllinux_1_1_x86_64.whl", hash = "sha256:901289d524fdd571be1c7be054f48b1f88ce8dddcbdf1ec698b27d4b8b9e5d62"}, + {file = "frozenlist-1.4.0-cp310-cp310-win32.whl", hash = "sha256:1a0848b52815006ea6596c395f87449f693dc419061cc21e970f139d466dc0a0"}, + {file = "frozenlist-1.4.0-cp310-cp310-win_amd64.whl", hash = "sha256:b206646d176a007466358aa21d85cd8600a415c67c9bd15403336c331a10d956"}, + {file = "frozenlist-1.4.0-cp311-cp311-macosx_10_9_universal2.whl", hash = "sha256:de343e75f40e972bae1ef6090267f8260c1446a1695e77096db6cfa25e759a95"}, + {file = "frozenlist-1.4.0-cp311-cp311-macosx_10_9_x86_64.whl", hash = "sha256:ad2a9eb6d9839ae241701d0918f54c51365a51407fd80f6b8289e2dfca977cc3"}, + {file = "frozenlist-1.4.0-cp311-cp311-macosx_11_0_arm64.whl", hash = "sha256:bd7bd3b3830247580de99c99ea2a01416dfc3c34471ca1298bccabf86d0ff4dc"}, + {file = "frozenlist-1.4.0-cp311-cp311-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:bdf1847068c362f16b353163391210269e4f0569a3c166bc6a9f74ccbfc7e839"}, + {file = "frozenlist-1.4.0-cp311-cp311-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:38461d02d66de17455072c9ba981d35f1d2a73024bee7790ac2f9e361ef1cd0c"}, + {file = "frozenlist-1.4.0-cp311-cp311-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:d5a32087d720c608f42caed0ef36d2b3ea61a9d09ee59a5142d6070da9041b8f"}, + {file = "frozenlist-1.4.0-cp311-cp311-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:dd65632acaf0d47608190a71bfe46b209719bf2beb59507db08ccdbe712f969b"}, + {file = "frozenlist-1.4.0-cp311-cp311-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:261b9f5d17cac914531331ff1b1d452125bf5daa05faf73b71d935485b0c510b"}, + {file = "frozenlist-1.4.0-cp311-cp311-musllinux_1_1_aarch64.whl", hash = "sha256:b89ac9768b82205936771f8d2eb3ce88503b1556324c9f903e7156669f521472"}, + {file = "frozenlist-1.4.0-cp311-cp311-musllinux_1_1_i686.whl", hash = "sha256:008eb8b31b3ea6896da16c38c1b136cb9fec9e249e77f6211d479db79a4eaf01"}, + {file = "frozenlist-1.4.0-cp311-cp311-musllinux_1_1_ppc64le.whl", hash = "sha256:e74b0506fa5aa5598ac6a975a12aa8928cbb58e1f5ac8360792ef15de1aa848f"}, + {file = "frozenlist-1.4.0-cp311-cp311-musllinux_1_1_s390x.whl", hash = "sha256:490132667476f6781b4c9458298b0c1cddf237488abd228b0b3650e5ecba7467"}, + {file = "frozenlist-1.4.0-cp311-cp311-musllinux_1_1_x86_64.whl", hash = "sha256:76d4711f6f6d08551a7e9ef28c722f4a50dd0fc204c56b4bcd95c6cc05ce6fbb"}, + {file = "frozenlist-1.4.0-cp311-cp311-win32.whl", hash = "sha256:a02eb8ab2b8f200179b5f62b59757685ae9987996ae549ccf30f983f40602431"}, + {file = "frozenlist-1.4.0-cp311-cp311-win_amd64.whl", hash = "sha256:515e1abc578dd3b275d6a5114030b1330ba044ffba03f94091842852f806f1c1"}, + {file = "frozenlist-1.4.0-cp38-cp38-macosx_10_9_universal2.whl", hash = "sha256:f0ed05f5079c708fe74bf9027e95125334b6978bf07fd5ab923e9e55e5fbb9d3"}, + {file = "frozenlist-1.4.0-cp38-cp38-macosx_10_9_x86_64.whl", hash = "sha256:ca265542ca427bf97aed183c1676e2a9c66942e822b14dc6e5f42e038f92a503"}, + {file = "frozenlist-1.4.0-cp38-cp38-macosx_11_0_arm64.whl", hash = "sha256:491e014f5c43656da08958808588cc6c016847b4360e327a62cb308c791bd2d9"}, + {file = "frozenlist-1.4.0-cp38-cp38-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:17ae5cd0f333f94f2e03aaf140bb762c64783935cc764ff9c82dff626089bebf"}, + {file = "frozenlist-1.4.0-cp38-cp38-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:1e78fb68cf9c1a6aa4a9a12e960a5c9dfbdb89b3695197aa7064705662515de2"}, + {file = "frozenlist-1.4.0-cp38-cp38-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:d5655a942f5f5d2c9ed93d72148226d75369b4f6952680211972a33e59b1dfdc"}, + {file = "frozenlist-1.4.0-cp38-cp38-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:c11b0746f5d946fecf750428a95f3e9ebe792c1ee3b1e96eeba145dc631a9672"}, + {file = "frozenlist-1.4.0-cp38-cp38-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:e66d2a64d44d50d2543405fb183a21f76b3b5fd16f130f5c99187c3fb4e64919"}, + {file = "frozenlist-1.4.0-cp38-cp38-musllinux_1_1_aarch64.whl", hash = "sha256:88f7bc0fcca81f985f78dd0fa68d2c75abf8272b1f5c323ea4a01a4d7a614efc"}, + {file = "frozenlist-1.4.0-cp38-cp38-musllinux_1_1_i686.whl", hash = "sha256:5833593c25ac59ede40ed4de6d67eb42928cca97f26feea219f21d0ed0959b79"}, + {file = "frozenlist-1.4.0-cp38-cp38-musllinux_1_1_ppc64le.whl", hash = "sha256:fec520865f42e5c7f050c2a79038897b1c7d1595e907a9e08e3353293ffc948e"}, + {file = "frozenlist-1.4.0-cp38-cp38-musllinux_1_1_s390x.whl", hash = "sha256:b826d97e4276750beca7c8f0f1a4938892697a6bcd8ec8217b3312dad6982781"}, + {file = "frozenlist-1.4.0-cp38-cp38-musllinux_1_1_x86_64.whl", hash = "sha256:ceb6ec0a10c65540421e20ebd29083c50e6d1143278746a4ef6bcf6153171eb8"}, + {file = "frozenlist-1.4.0-cp38-cp38-win32.whl", hash = "sha256:2b8bcf994563466db019fab287ff390fffbfdb4f905fc77bc1c1d604b1c689cc"}, + {file = "frozenlist-1.4.0-cp38-cp38-win_amd64.whl", hash = "sha256:a6c8097e01886188e5be3e6b14e94ab365f384736aa1fca6a0b9e35bd4a30bc7"}, + {file = "frozenlist-1.4.0-cp39-cp39-macosx_10_9_universal2.whl", hash = "sha256:6c38721585f285203e4b4132a352eb3daa19121a035f3182e08e437cface44bf"}, + {file = "frozenlist-1.4.0-cp39-cp39-macosx_10_9_x86_64.whl", hash = "sha256:a0c6da9aee33ff0b1a451e867da0c1f47408112b3391dd43133838339e410963"}, + {file = "frozenlist-1.4.0-cp39-cp39-macosx_11_0_arm64.whl", hash = "sha256:93ea75c050c5bb3d98016b4ba2497851eadf0ac154d88a67d7a6816206f6fa7f"}, + {file = "frozenlist-1.4.0-cp39-cp39-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:f61e2dc5ad442c52b4887f1fdc112f97caeff4d9e6ebe78879364ac59f1663e1"}, + {file = "frozenlist-1.4.0-cp39-cp39-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:aa384489fefeb62321b238e64c07ef48398fe80f9e1e6afeff22e140e0850eef"}, + {file = "frozenlist-1.4.0-cp39-cp39-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:10ff5faaa22786315ef57097a279b833ecab1a0bfb07d604c9cbb1c4cdc2ed87"}, + {file = "frozenlist-1.4.0-cp39-cp39-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:007df07a6e3eb3e33e9a1fe6a9db7af152bbd8a185f9aaa6ece10a3529e3e1c6"}, + {file = "frozenlist-1.4.0-cp39-cp39-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:7f4f399d28478d1f604c2ff9119907af9726aed73680e5ed1ca634d377abb087"}, + {file = "frozenlist-1.4.0-cp39-cp39-musllinux_1_1_aarch64.whl", hash = "sha256:c5374b80521d3d3f2ec5572e05adc94601985cc526fb276d0c8574a6d749f1b3"}, + {file = "frozenlist-1.4.0-cp39-cp39-musllinux_1_1_i686.whl", hash = "sha256:ce31ae3e19f3c902de379cf1323d90c649425b86de7bbdf82871b8a2a0615f3d"}, + {file = "frozenlist-1.4.0-cp39-cp39-musllinux_1_1_ppc64le.whl", hash = "sha256:7211ef110a9194b6042449431e08c4d80c0481e5891e58d429df5899690511c2"}, + {file = "frozenlist-1.4.0-cp39-cp39-musllinux_1_1_s390x.whl", hash = "sha256:556de4430ce324c836789fa4560ca62d1591d2538b8ceb0b4f68fb7b2384a27a"}, + {file = "frozenlist-1.4.0-cp39-cp39-musllinux_1_1_x86_64.whl", hash = "sha256:7645a8e814a3ee34a89c4a372011dcd817964ce8cb273c8ed6119d706e9613e3"}, + {file = "frozenlist-1.4.0-cp39-cp39-win32.whl", hash = "sha256:19488c57c12d4e8095a922f328df3f179c820c212940a498623ed39160bc3c2f"}, + {file = "frozenlist-1.4.0-cp39-cp39-win_amd64.whl", hash = "sha256:6221d84d463fb110bdd7619b69cb43878a11d51cbb9394ae3105d082d5199167"}, + {file = "frozenlist-1.4.0.tar.gz", hash = "sha256:09163bdf0b2907454042edb19f887c6d33806adc71fbd54afc14908bfdc22251"}, +] + [[package]] name = "graphql-core" version = "3.2.1" @@ -2527,4 +2651,4 @@ testing = ["func-timeout", "jaraco.itertools", "pytest (>=6)", "pytest-black (>= [metadata] lock-version = "2.0" python-versions = "^3.9" -content-hash = "e16a65d8fdff4e2173610e552e0e7306e301de2c640ae6082ef6cc5755f566d2" +content-hash = "c40f62277e788011920f4edb6f7392046ee440f792a104c903097415def9a916" diff --git a/pyproject.toml b/pyproject.toml index 726e04ea4e..2ff8f6982d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -33,7 +33,7 @@ psutil = "^5.9.4" types-psutil = "^5.9.5.12" types-toml = "^0.10.8.6" pytest-httpserver = "^1.0.8" -aiohttp = "3.7.4" +aiohttp = "3.8.5" pytest-rerunfailures = "^11.1.2" types-pytest-lazy-fixture = "^0.6.3.3" pytest-split = "^0.8.1" From 1685593f386adf57e697a21ad41700d284aeb81b Mon Sep 17 00:00:00 2001 From: Alex Chi Z Date: Fri, 21 Jul 2023 10:15:44 -0400 Subject: [PATCH 14/23] stable merge and sort in compaction (#4573) Per discussion at https://github.com/neondatabase/neon/pull/4537#discussion_r1242086217, it looks like a better idea to use `<` instead of `<=` for all these comparisons. --------- Signed-off-by: Alex Chi Z --- pageserver/src/tenant/timeline.rs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index c6ceae500b..616d184393 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -3452,7 +3452,7 @@ impl Timeline { let mut prev: Option = None; for (next_key, _next_lsn, _size) in itertools::process_results( deltas_to_compact.iter().map(|l| l.key_iter(ctx)), - |iter_iter| iter_iter.kmerge_by(|a, b| a.0 <= b.0), + |iter_iter| iter_iter.kmerge_by(|a, b| a.0 < b.0), )? { if let Some(prev_key) = prev { // just first fast filter @@ -3492,11 +3492,7 @@ impl Timeline { iter_iter.kmerge_by(|a, b| { if let Ok((a_key, a_lsn, _)) = a { if let Ok((b_key, b_lsn, _)) = b { - match a_key.cmp(b_key) { - Ordering::Less => true, - Ordering::Equal => a_lsn <= b_lsn, - Ordering::Greater => false, - } + (a_key, a_lsn) < (b_key, b_lsn) } else { false } @@ -3514,11 +3510,7 @@ impl Timeline { iter_iter.kmerge_by(|a, b| { let (a_key, a_lsn, _) = a; let (b_key, b_lsn, _) = b; - match a_key.cmp(b_key) { - Ordering::Less => true, - Ordering::Equal => a_lsn <= b_lsn, - Ordering::Greater => false, - } + (a_key, a_lsn) < (b_key, b_lsn) }) }, )?; From 25d2f4b669076a266775ea4e73c578eee2d63049 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Fri, 21 Jul 2023 18:10:55 +0300 Subject: [PATCH 15/23] metrics: chunked responses (#4768) Metrics can get really large in the order of hundreds of megabytes, which we used to buffer completly (after a few rounds of growing the buffer). --- Cargo.lock | 1 + libs/metrics/src/lib.rs | 1 + libs/utils/Cargo.toml | 4 + libs/utils/src/http/endpoint.rs | 137 +++++++++++++++++++++++++++++--- 4 files changed, 131 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3c862241a4..b9ca53064f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4867,6 +4867,7 @@ dependencies = [ "tempfile", "thiserror", "tokio", + "tokio-stream", "tracing", "tracing-error", "tracing-subscriber", diff --git a/libs/metrics/src/lib.rs b/libs/metrics/src/lib.rs index f24488b19d..b4026d93e1 100644 --- a/libs/metrics/src/lib.rs +++ b/libs/metrics/src/lib.rs @@ -6,6 +6,7 @@ use once_cell::sync::Lazy; use prometheus::core::{AtomicU64, Collector, GenericGauge, GenericGaugeVec}; pub use prometheus::opts; pub use prometheus::register; +pub use prometheus::Error; pub use prometheus::{core, default_registry, proto}; pub use prometheus::{exponential_buckets, linear_buckets}; pub use prometheus::{register_counter_vec, Counter, CounterVec}; diff --git a/libs/utils/Cargo.toml b/libs/utils/Cargo.toml index e7c8323c1d..f01131540e 100644 --- a/libs/utils/Cargo.toml +++ b/libs/utils/Cargo.toml @@ -42,6 +42,10 @@ workspace_hack.workspace = true const_format.workspace = true +# to use tokio channels as streams, this is faster to compile than async_stream +# why is it only here? no other crate should use it, streams are rarely needed. +tokio-stream = { version = "0.1.14" } + [dev-dependencies] byteorder.workspace = true bytes.workspace = true diff --git a/libs/utils/src/http/endpoint.rs b/libs/utils/src/http/endpoint.rs index 33241dbdf7..f3f5e95d0b 100644 --- a/libs/utils/src/http/endpoint.rs +++ b/libs/utils/src/http/endpoint.rs @@ -9,7 +9,6 @@ use metrics::{register_int_counter, Encoder, IntCounter, TextEncoder}; use once_cell::sync::Lazy; use routerify::ext::RequestExt; use routerify::{Middleware, RequestInfo, Router, RouterBuilder}; -use tokio::task::JoinError; use tracing::{self, debug, info, info_span, warn, Instrument}; use std::future::Future; @@ -148,26 +147,140 @@ impl Drop for RequestCancelled { } async fn prometheus_metrics_handler(_req: Request) -> Result, ApiError> { + use bytes::{Bytes, BytesMut}; + use std::io::Write as _; + use tokio::sync::mpsc; + use tokio_stream::wrappers::ReceiverStream; + SERVE_METRICS_COUNT.inc(); - let mut buffer = vec![]; - let encoder = TextEncoder::new(); + /// An [`std::io::Write`] implementation on top of a channel sending [`bytes::Bytes`] chunks. + struct ChannelWriter { + buffer: BytesMut, + tx: mpsc::Sender>, + written: usize, + } - let metrics = tokio::task::spawn_blocking(move || { - // Currently we take a lot of mutexes while collecting metrics, so it's - // better to spawn a blocking task to avoid blocking the event loop. - metrics::gather() - }) - .await - .map_err(|e: JoinError| ApiError::InternalServerError(e.into()))?; - encoder.encode(&metrics, &mut buffer).unwrap(); + impl ChannelWriter { + fn new(buf_len: usize, tx: mpsc::Sender>) -> Self { + assert_ne!(buf_len, 0); + ChannelWriter { + // split about half off the buffer from the start, because we flush depending on + // capacity. first flush will come sooner than without this, but now resizes will + // have better chance of picking up the "other" half. not guaranteed of course. + buffer: BytesMut::with_capacity(buf_len).split_off(buf_len / 2), + tx, + written: 0, + } + } + + fn flush0(&mut self) -> std::io::Result { + let n = self.buffer.len(); + if n == 0 { + return Ok(0); + } + + tracing::trace!(n, "flushing"); + let ready = self.buffer.split().freeze(); + + // not ideal to call from blocking code to block_on, but we are sure that this + // operation does not spawn_blocking other tasks + let res: Result<(), ()> = tokio::runtime::Handle::current().block_on(async { + self.tx.send(Ok(ready)).await.map_err(|_| ())?; + + // throttle sending to allow reuse of our buffer in `write`. + self.tx.reserve().await.map_err(|_| ())?; + + // now the response task has picked up the buffer and hopefully started + // sending it to the client. + Ok(()) + }); + if res.is_err() { + return Err(std::io::ErrorKind::BrokenPipe.into()); + } + self.written += n; + Ok(n) + } + + fn flushed_bytes(&self) -> usize { + self.written + } + } + + impl std::io::Write for ChannelWriter { + fn write(&mut self, mut buf: &[u8]) -> std::io::Result { + let remaining = self.buffer.capacity() - self.buffer.len(); + + let out_of_space = remaining < buf.len(); + + let original_len = buf.len(); + + if out_of_space { + let can_still_fit = buf.len() - remaining; + self.buffer.extend_from_slice(&buf[..can_still_fit]); + buf = &buf[can_still_fit..]; + self.flush0()?; + } + + // assume that this will often under normal operation just move the pointer back to the + // beginning of allocation, because previous split off parts are already sent and + // dropped. + self.buffer.extend_from_slice(buf); + Ok(original_len) + } + + fn flush(&mut self) -> std::io::Result<()> { + self.flush0().map(|_| ()) + } + } + + let started_at = std::time::Instant::now(); + + let (tx, rx) = mpsc::channel(1); + + let body = Body::wrap_stream(ReceiverStream::new(rx)); + + let mut writer = ChannelWriter::new(128 * 1024, tx); + + let encoder = TextEncoder::new(); let response = Response::builder() .status(200) .header(CONTENT_TYPE, encoder.format_type()) - .body(Body::from(buffer)) + .body(body) .unwrap(); + let span = info_span!("blocking"); + tokio::task::spawn_blocking(move || { + let _span = span.entered(); + let metrics = metrics::gather(); + let res = encoder + .encode(&metrics, &mut writer) + .and_then(|_| writer.flush().map_err(|e| e.into())); + + match res { + Ok(()) => { + tracing::info!( + bytes = writer.flushed_bytes(), + elapsed_ms = started_at.elapsed().as_millis(), + "responded /metrics" + ); + } + Err(e) => { + tracing::warn!("failed to write out /metrics response: {e:#}"); + // semantics of this error are quite... unclear. we want to error the stream out to + // abort the response to somehow notify the client that we failed. + // + // though, most likely the reason for failure is that the receiver is already gone. + drop( + writer + .tx + .blocking_send(Err(std::io::ErrorKind::BrokenPipe.into())), + ); + } + } + }); + Ok(response) } From 457e3a3ebc35b35c589532164395b1d98a4fd2a4 Mon Sep 17 00:00:00 2001 From: Konstantin Knizhnik Date: Fri, 21 Jul 2023 21:20:53 +0300 Subject: [PATCH 16/23] Mx offset bug (#4775) Fix mx_offset_to_flags_offset() function Fixes issue #4774 Postgres `MXOffsetToFlagsOffset` was not correctly converted to Rust because cast to u16 is done before division by modulo. It is possible only if divider is power of two. Add a small rust unit test to check that the function produces same results as the PostgreSQL macro, and extend the existing python test to cover this bug. Co-authored-by: Konstantin Knizhnik Co-authored-by: Heikki Linnakangas --- libs/postgres_ffi/src/nonrelfile_utils.rs | 44 +++++++++++++++++++++-- test_runner/regress/test_multixact.py | 21 ++++++++--- 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/libs/postgres_ffi/src/nonrelfile_utils.rs b/libs/postgres_ffi/src/nonrelfile_utils.rs index 5acf90be70..e3e7133b94 100644 --- a/libs/postgres_ffi/src/nonrelfile_utils.rs +++ b/libs/postgres_ffi/src/nonrelfile_utils.rs @@ -57,9 +57,9 @@ pub fn slru_may_delete_clogsegment(segpage: u32, cutoff_page: u32) -> bool { // Multixact utils pub fn mx_offset_to_flags_offset(xid: MultiXactId) -> usize { - ((xid / pg_constants::MULTIXACT_MEMBERS_PER_MEMBERGROUP as u32) as u16 - % pg_constants::MULTIXACT_MEMBERGROUPS_PER_PAGE - * pg_constants::MULTIXACT_MEMBERGROUP_SIZE) as usize + ((xid / pg_constants::MULTIXACT_MEMBERS_PER_MEMBERGROUP as u32) + % pg_constants::MULTIXACT_MEMBERGROUPS_PER_PAGE as u32 + * pg_constants::MULTIXACT_MEMBERGROUP_SIZE as u32) as usize } pub fn mx_offset_to_flags_bitshift(xid: MultiXactId) -> u16 { @@ -81,3 +81,41 @@ fn mx_offset_to_member_page(xid: u32) -> u32 { pub fn mx_offset_to_member_segment(xid: u32) -> i32 { (mx_offset_to_member_page(xid) / pg_constants::SLRU_PAGES_PER_SEGMENT) as i32 } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_multixid_calc() { + // Check that the mx_offset_* functions produce the same values as the + // corresponding PostgreSQL C macros (MXOffsetTo*). These test values + // were generated by calling the PostgreSQL macros with a little C + // program. + assert_eq!(mx_offset_to_member_segment(0), 0); + assert_eq!(mx_offset_to_member_page(0), 0); + assert_eq!(mx_offset_to_flags_offset(0), 0); + assert_eq!(mx_offset_to_flags_bitshift(0), 0); + assert_eq!(mx_offset_to_member_offset(0), 4); + assert_eq!(mx_offset_to_member_segment(1), 0); + assert_eq!(mx_offset_to_member_page(1), 0); + assert_eq!(mx_offset_to_flags_offset(1), 0); + assert_eq!(mx_offset_to_flags_bitshift(1), 8); + assert_eq!(mx_offset_to_member_offset(1), 8); + assert_eq!(mx_offset_to_member_segment(123456789), 2358); + assert_eq!(mx_offset_to_member_page(123456789), 75462); + assert_eq!(mx_offset_to_flags_offset(123456789), 4780); + assert_eq!(mx_offset_to_flags_bitshift(123456789), 8); + assert_eq!(mx_offset_to_member_offset(123456789), 4788); + assert_eq!(mx_offset_to_member_segment(u32::MAX - 1), 82040); + assert_eq!(mx_offset_to_member_page(u32::MAX - 1), 2625285); + assert_eq!(mx_offset_to_flags_offset(u32::MAX - 1), 5160); + assert_eq!(mx_offset_to_flags_bitshift(u32::MAX - 1), 16); + assert_eq!(mx_offset_to_member_offset(u32::MAX - 1), 5172); + assert_eq!(mx_offset_to_member_segment(u32::MAX), 82040); + assert_eq!(mx_offset_to_member_page(u32::MAX), 2625285); + assert_eq!(mx_offset_to_flags_offset(u32::MAX), 5160); + assert_eq!(mx_offset_to_flags_bitshift(u32::MAX), 24); + assert_eq!(mx_offset_to_member_offset(u32::MAX), 5176); + } +} diff --git a/test_runner/regress/test_multixact.py b/test_runner/regress/test_multixact.py index 78635576f1..9db463dc4a 100644 --- a/test_runner/regress/test_multixact.py +++ b/test_runner/regress/test_multixact.py @@ -8,6 +8,10 @@ from fixtures.utils import query_scalar # Now this test is very minimalistic - # it only checks next_multixact_id field in restored pg_control, # since we don't have functions to check multixact internals. +# We do check that the datadir contents exported from the +# pageserver match what the running PostgreSQL produced. This +# is enough to verify that the WAL records are handled correctly +# in the pageserver. # def test_multixact(neon_simple_env: NeonEnv, test_output_dir): env = neon_simple_env @@ -18,8 +22,8 @@ def test_multixact(neon_simple_env: NeonEnv, test_output_dir): cur = endpoint.connect().cursor() cur.execute( """ - CREATE TABLE t1(i int primary key); - INSERT INTO t1 select * from generate_series(1, 100); + CREATE TABLE t1(i int primary key, n_updated int); + INSERT INTO t1 select g, 0 from generate_series(1, 50) g; """ ) @@ -29,6 +33,7 @@ def test_multixact(neon_simple_env: NeonEnv, test_output_dir): # Lock entries using parallel connections in a round-robin fashion. nclients = 20 + update_every = 97 connections = [] for _ in range(nclients): # Do not turn on autocommit. We want to hold the key-share locks. @@ -36,14 +41,20 @@ def test_multixact(neon_simple_env: NeonEnv, test_output_dir): connections.append(conn) # On each iteration, we commit the previous transaction on a connection, - # and issue antoher select. Each SELECT generates a new multixact that + # and issue another select. Each SELECT generates a new multixact that # includes the new XID, and the XIDs of all the other parallel transactions. # This generates enough traffic on both multixact offsets and members SLRUs # to cross page boundaries. - for i in range(5000): + for i in range(20000): conn = connections[i % nclients] conn.commit() - conn.cursor().execute("select * from t1 for key share") + + # Perform some non-key UPDATEs too, to exercise different multixact + # member statuses. + if i % update_every == 0: + conn.cursor().execute(f"update t1 set n_updated = n_updated + 1 where i = {i % 50}") + else: + conn.cursor().execute("select * from t1 for key share") # We have multixacts now. We can close the connections. for c in connections: From 7feb0d1a80227c6992e488fdbda9b78769741f25 Mon Sep 17 00:00:00 2001 From: Alek Westover Date: Fri, 21 Jul 2023 15:17:16 -0400 Subject: [PATCH 17/23] `unwrap` instead of passing `anyhow::Error` on failure to spawn a thread (#4779) --- compute_tools/src/bin/compute_ctl.rs | 5 ++--- compute_tools/src/configurator.rs | 8 ++++---- compute_tools/src/monitor.rs | 8 ++++---- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/compute_tools/src/bin/compute_ctl.rs b/compute_tools/src/bin/compute_ctl.rs index 68f6bf3844..4953e5f331 100644 --- a/compute_tools/src/bin/compute_ctl.rs +++ b/compute_tools/src/bin/compute_ctl.rs @@ -223,9 +223,8 @@ fn main() -> Result<()> { drop(state); // Launch remaining service threads - let _monitor_handle = launch_monitor(&compute).expect("cannot launch compute monitor thread"); - let _configurator_handle = - launch_configurator(&compute).expect("cannot launch configurator thread"); + let _monitor_handle = launch_monitor(&compute); + let _configurator_handle = launch_configurator(&compute); // Start Postgres let mut delay_exit = false; diff --git a/compute_tools/src/configurator.rs b/compute_tools/src/configurator.rs index 13550e0176..274a221ac7 100644 --- a/compute_tools/src/configurator.rs +++ b/compute_tools/src/configurator.rs @@ -1,7 +1,6 @@ use std::sync::Arc; use std::thread; -use anyhow::Result; use tracing::{error, info, instrument}; use compute_api::responses::ComputeStatus; @@ -42,13 +41,14 @@ fn configurator_main_loop(compute: &Arc) { } } -pub fn launch_configurator(compute: &Arc) -> Result> { +pub fn launch_configurator(compute: &Arc) -> thread::JoinHandle<()> { let compute = Arc::clone(compute); - Ok(thread::Builder::new() + thread::Builder::new() .name("compute-configurator".into()) .spawn(move || { configurator_main_loop(&compute); info!("configurator thread is exited"); - })?) + }) + .expect("cannot launch configurator thread") } diff --git a/compute_tools/src/monitor.rs b/compute_tools/src/monitor.rs index d2e7b698dd..1085a27902 100644 --- a/compute_tools/src/monitor.rs +++ b/compute_tools/src/monitor.rs @@ -1,7 +1,6 @@ use std::sync::Arc; use std::{thread, time}; -use anyhow::Result; use chrono::{DateTime, Utc}; use postgres::{Client, NoTls}; use tracing::{debug, info}; @@ -105,10 +104,11 @@ fn watch_compute_activity(compute: &ComputeNode) { } /// Launch a separate compute monitor thread and return its `JoinHandle`. -pub fn launch_monitor(state: &Arc) -> Result> { +pub fn launch_monitor(state: &Arc) -> thread::JoinHandle<()> { let state = Arc::clone(state); - Ok(thread::Builder::new() + thread::Builder::new() .name("compute-monitor".into()) - .spawn(move || watch_compute_activity(&state))?) + .spawn(move || watch_compute_activity(&state)) + .expect("cannot launch compute monitor thread") } From e5b7ddfeeebc25d9ca0f183704244d6abb90a449 Mon Sep 17 00:00:00 2001 From: arpad-m Date: Mon, 24 Jul 2023 14:01:54 +0200 Subject: [PATCH 18/23] Preparatory pageserver async conversions (#4773) In #4743, we'd like to convert the read path to use `async` rust. In preparation of that, this PR switches some functions that are calling lower level functions like `BlockReader::read_blk`, `BlockCursor::read_blob`, etc into `async`. The PR does not switch all functions however, and only focuses on the ones which are easy to switch. This leaves around some async functions that are (currently) unnecessarily `async`, but on the other hand it makes future changes smaller in diff. Part of #4743 (but does not completely address it). --- Cargo.lock | 1 + pageserver/ctl/Cargo.toml | 1 + pageserver/ctl/src/layer_map_analyzer.rs | 6 +-- pageserver/ctl/src/layers.rs | 7 ++- pageserver/ctl/src/main.rs | 13 +++--- pageserver/src/tenant.rs | 14 ++++-- pageserver/src/tenant/blob_io.rs | 22 +++------ pageserver/src/tenant/ephemeral_file.rs | 2 +- pageserver/src/tenant/layer_map.rs | 6 +-- pageserver/src/tenant/storage_layer.rs | 7 +-- .../src/tenant/storage_layer/delta_layer.rs | 7 +-- .../src/tenant/storage_layer/image_layer.rs | 7 +-- .../tenant/storage_layer/inmemory_layer.rs | 7 +-- .../src/tenant/storage_layer/remote_layer.rs | 5 ++- pageserver/src/tenant/timeline.rs | 45 +++++++++++-------- 15 files changed, 82 insertions(+), 68 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b9ca53064f..05a70bfe55 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2506,6 +2506,7 @@ dependencies = [ "pageserver", "postgres_ffi", "svg_fmt", + "tokio", "utils", "workspace_hack", ] diff --git a/pageserver/ctl/Cargo.toml b/pageserver/ctl/Cargo.toml index 89e0d0486e..b3f12b72b3 100644 --- a/pageserver/ctl/Cargo.toml +++ b/pageserver/ctl/Cargo.toml @@ -13,6 +13,7 @@ clap = { workspace = true, features = ["string"] } git-version.workspace = true pageserver = { path = ".." } postgres_ffi.workspace = true +tokio.workspace = true utils.workspace = true svg_fmt.workspace = true workspace_hack.workspace = true diff --git a/pageserver/ctl/src/layer_map_analyzer.rs b/pageserver/ctl/src/layer_map_analyzer.rs index f2ced6154f..da83c061c0 100644 --- a/pageserver/ctl/src/layer_map_analyzer.rs +++ b/pageserver/ctl/src/layer_map_analyzer.rs @@ -95,7 +95,7 @@ pub(crate) fn parse_filename(name: &str) -> Option { } // Finds the max_holes largest holes, ignoring any that are smaller than MIN_HOLE_LENGTH" -fn get_holes(path: &Path, max_holes: usize) -> Result> { +async fn get_holes(path: &Path, max_holes: usize) -> Result> { let file = FileBlockReader::new(VirtualFile::open(path)?); let summary_blk = file.read_blk(0)?; let actual_summary = Summary::des_prefix(summary_blk.as_ref())?; @@ -129,7 +129,7 @@ fn get_holes(path: &Path, max_holes: usize) -> Result> { Ok(holes) } -pub(crate) fn main(cmd: &AnalyzeLayerMapCmd) -> Result<()> { +pub(crate) async fn main(cmd: &AnalyzeLayerMapCmd) -> Result<()> { let storage_path = &cmd.path; let max_holes = cmd.max_holes.unwrap_or(DEFAULT_MAX_HOLES); @@ -160,7 +160,7 @@ pub(crate) fn main(cmd: &AnalyzeLayerMapCmd) -> Result<()> { parse_filename(&layer.file_name().into_string().unwrap()) { if layer_file.is_delta { - layer_file.holes = get_holes(&layer.path(), max_holes)?; + layer_file.holes = get_holes(&layer.path(), max_holes).await?; n_deltas += 1; } layers.push(layer_file); diff --git a/pageserver/ctl/src/layers.rs b/pageserver/ctl/src/layers.rs index d77cf0908c..d81303d533 100644 --- a/pageserver/ctl/src/layers.rs +++ b/pageserver/ctl/src/layers.rs @@ -43,8 +43,7 @@ pub(crate) enum LayerCmd { }, } -fn read_delta_file(path: impl AsRef) -> Result<()> { - use pageserver::tenant::blob_io::BlobCursor; +async fn read_delta_file(path: impl AsRef) -> Result<()> { use pageserver::tenant::block_io::BlockReader; let path = path.as_ref(); @@ -78,7 +77,7 @@ fn read_delta_file(path: impl AsRef) -> Result<()> { Ok(()) } -pub(crate) fn main(cmd: &LayerCmd) -> Result<()> { +pub(crate) async fn main(cmd: &LayerCmd) -> Result<()> { match cmd { LayerCmd::List { path } => { for tenant in fs::read_dir(path.join("tenants"))? { @@ -153,7 +152,7 @@ pub(crate) fn main(cmd: &LayerCmd) -> Result<()> { ); if layer_file.is_delta { - read_delta_file(layer.path())?; + read_delta_file(layer.path()).await?; } else { anyhow::bail!("not supported yet :("); } diff --git a/pageserver/ctl/src/main.rs b/pageserver/ctl/src/main.rs index 55db9eb7e7..0d154aca0c 100644 --- a/pageserver/ctl/src/main.rs +++ b/pageserver/ctl/src/main.rs @@ -72,12 +72,13 @@ struct AnalyzeLayerMapCmd { max_holes: Option, } -fn main() -> anyhow::Result<()> { +#[tokio::main] +async fn main() -> anyhow::Result<()> { let cli = CliOpts::parse(); match cli.command { Commands::Layer(cmd) => { - layers::main(&cmd)?; + layers::main(&cmd).await?; } Commands::Metadata(cmd) => { handle_metadata(&cmd)?; @@ -86,7 +87,7 @@ fn main() -> anyhow::Result<()> { draw_timeline_dir::main()?; } Commands::AnalyzeLayerMap(cmd) => { - layer_map_analyzer::main(&cmd)?; + layer_map_analyzer::main(&cmd).await?; } Commands::PrintLayerFile(cmd) => { if let Err(e) = read_pg_control_file(&cmd.path) { @@ -94,7 +95,7 @@ fn main() -> anyhow::Result<()> { "Failed to read input file as a pg control one: {e:#}\n\ Attempting to read it as layer file" ); - print_layerfile(&cmd.path)?; + print_layerfile(&cmd.path).await?; } } }; @@ -113,12 +114,12 @@ fn read_pg_control_file(control_file_path: &Path) -> anyhow::Result<()> { Ok(()) } -fn print_layerfile(path: &Path) -> anyhow::Result<()> { +async fn print_layerfile(path: &Path) -> anyhow::Result<()> { // Basic initialization of things that don't change after startup virtual_file::init(10); page_cache::init(100); let ctx = RequestContext::new(TaskKind::DebugTool, DownloadBehavior::Error); - dump_layerfile_from_path(path, true, &ctx) + dump_layerfile_from_path(path, true, &ctx).await } fn handle_metadata( diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 44f1a6cd65..81dce84b04 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -3210,7 +3210,7 @@ impl Drop for Tenant { } } /// Dump contents of a layer file to stdout. -pub fn dump_layerfile_from_path( +pub async fn dump_layerfile_from_path( path: &Path, verbose: bool, ctx: &RequestContext, @@ -3224,8 +3224,16 @@ pub fn dump_layerfile_from_path( file.read_exact_at(&mut header_buf, 0)?; match u16::from_be_bytes(header_buf) { - crate::IMAGE_FILE_MAGIC => ImageLayer::new_for_path(path, file)?.dump(verbose, ctx)?, - crate::DELTA_FILE_MAGIC => DeltaLayer::new_for_path(path, file)?.dump(verbose, ctx)?, + crate::IMAGE_FILE_MAGIC => { + ImageLayer::new_for_path(path, file)? + .dump(verbose, ctx) + .await? + } + crate::DELTA_FILE_MAGIC => { + DeltaLayer::new_for_path(path, file)? + .dump(verbose, ctx) + .await? + } magic => bail!("unrecognized magic identifier: {:?}", magic), } diff --git a/pageserver/src/tenant/blob_io.rs b/pageserver/src/tenant/blob_io.rs index 52eafc72ee..4dcf7fe5fe 100644 --- a/pageserver/src/tenant/blob_io.rs +++ b/pageserver/src/tenant/blob_io.rs @@ -16,29 +16,19 @@ use crate::tenant::block_io::{BlockCursor, BlockReader}; use std::cmp::min; use std::io::{Error, ErrorKind}; -/// For reading -pub trait BlobCursor { +impl BlockCursor +where + R: BlockReader, +{ /// Read a blob into a new buffer. - fn read_blob(&mut self, offset: u64) -> Result, std::io::Error> { + pub fn read_blob(&mut self, offset: u64) -> Result, std::io::Error> { let mut buf = Vec::new(); self.read_blob_into_buf(offset, &mut buf)?; Ok(buf) } - /// Read blob into the given buffer. Any previous contents in the buffer /// are overwritten. - fn read_blob_into_buf( - &mut self, - offset: u64, - dstbuf: &mut Vec, - ) -> Result<(), std::io::Error>; -} - -impl BlobCursor for BlockCursor -where - R: BlockReader, -{ - fn read_blob_into_buf( + pub fn read_blob_into_buf( &mut self, offset: u64, dstbuf: &mut Vec, diff --git a/pageserver/src/tenant/ephemeral_file.rs b/pageserver/src/tenant/ephemeral_file.rs index 0d3c5da91c..f44534249c 100644 --- a/pageserver/src/tenant/ephemeral_file.rs +++ b/pageserver/src/tenant/ephemeral_file.rs @@ -328,7 +328,7 @@ fn to_io_error(e: anyhow::Error, context: &str) -> io::Error { #[cfg(test)] mod tests { use super::*; - use crate::tenant::blob_io::{BlobCursor, BlobWriter}; + use crate::tenant::blob_io::BlobWriter; use crate::tenant::block_io::BlockCursor; use rand::{seq::SliceRandom, thread_rng, RngCore}; use std::fs; diff --git a/pageserver/src/tenant/layer_map.rs b/pageserver/src/tenant/layer_map.rs index 2908d3a83c..c4b894fcf5 100644 --- a/pageserver/src/tenant/layer_map.rs +++ b/pageserver/src/tenant/layer_map.rs @@ -626,17 +626,17 @@ impl LayerMap { /// debugging function to print out the contents of the layer map #[allow(unused)] - pub fn dump(&self, verbose: bool, ctx: &RequestContext) -> Result<()> { + pub async fn dump(&self, verbose: bool, ctx: &RequestContext) -> Result<()> { println!("Begin dump LayerMap"); println!("open_layer:"); if let Some(open_layer) = &self.open_layer { - open_layer.dump(verbose, ctx)?; + open_layer.dump(verbose, ctx).await?; } println!("frozen_layers:"); for frozen_layer in self.frozen_layers.iter() { - frozen_layer.dump(verbose, ctx)?; + frozen_layer.dump(verbose, ctx).await?; } println!("historic_layers:"); diff --git a/pageserver/src/tenant/storage_layer.rs b/pageserver/src/tenant/storage_layer.rs index c6d1a0052a..05381cb56d 100644 --- a/pageserver/src/tenant/storage_layer.rs +++ b/pageserver/src/tenant/storage_layer.rs @@ -338,7 +338,8 @@ impl LayerAccessStats { /// All layers should implement a minimal `std::fmt::Debug` without tenant or /// timeline names, because those are known in the context of which the layers /// are used in (timeline). -pub trait Layer: std::fmt::Debug + std::fmt::Display + Send + Sync { +#[async_trait::async_trait] +pub trait Layer: std::fmt::Debug + std::fmt::Display + Send + Sync + 'static { /// Range of keys that this layer covers fn get_key_range(&self) -> Range; @@ -368,7 +369,7 @@ pub trait Layer: std::fmt::Debug + std::fmt::Display + Send + Sync { /// is available. If this returns ValueReconstructResult::Continue, look up /// the predecessor layer and call again with the same 'reconstruct_data' to /// collect more data. - fn get_value_reconstruct_data( + async fn get_value_reconstruct_data( &self, key: Key, lsn_range: Range, @@ -377,7 +378,7 @@ pub trait Layer: std::fmt::Debug + std::fmt::Display + Send + Sync { ) -> Result; /// Dump summary of the contents of the layer to stdout - fn dump(&self, verbose: bool, ctx: &RequestContext) -> Result<()>; + async fn dump(&self, verbose: bool, ctx: &RequestContext) -> Result<()>; } /// Returned by [`PersistentLayer::iter`] diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 83a22f9f13..9585c04120 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -31,7 +31,7 @@ use crate::config::PageServerConf; use crate::context::RequestContext; use crate::page_cache::{PageReadGuard, PAGE_SZ}; use crate::repository::{Key, Value, KEY_SIZE}; -use crate::tenant::blob_io::{BlobCursor, BlobWriter, WriteBlobWriter}; +use crate::tenant::blob_io::{BlobWriter, WriteBlobWriter}; use crate::tenant::block_io::{BlockBuf, BlockCursor, BlockReader, FileBlockReader}; use crate::tenant::disk_btree::{DiskBtreeBuilder, DiskBtreeReader, VisitDirection}; use crate::tenant::storage_layer::{ @@ -223,9 +223,10 @@ impl std::fmt::Debug for DeltaLayerInner { } } +#[async_trait::async_trait] impl Layer for DeltaLayer { /// debugging function to print out the contents of the layer - fn dump(&self, verbose: bool, ctx: &RequestContext) -> Result<()> { + async fn dump(&self, verbose: bool, ctx: &RequestContext) -> Result<()> { println!( "----- delta layer for ten {} tli {} keys {}-{} lsn {}-{} size {} ----", self.desc.tenant_id, @@ -300,7 +301,7 @@ impl Layer for DeltaLayer { Ok(()) } - fn get_value_reconstruct_data( + async fn get_value_reconstruct_data( &self, key: Key, lsn_range: Range, diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index b8601af818..53cff824e3 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -27,7 +27,7 @@ use crate::config::PageServerConf; use crate::context::RequestContext; use crate::page_cache::PAGE_SZ; use crate::repository::{Key, KEY_SIZE}; -use crate::tenant::blob_io::{BlobCursor, BlobWriter, WriteBlobWriter}; +use crate::tenant::blob_io::{BlobWriter, WriteBlobWriter}; use crate::tenant::block_io::{BlockBuf, BlockReader, FileBlockReader}; use crate::tenant::disk_btree::{DiskBtreeBuilder, DiskBtreeReader, VisitDirection}; use crate::tenant::storage_layer::{ @@ -155,9 +155,10 @@ impl std::fmt::Debug for ImageLayerInner { } } +#[async_trait::async_trait] impl Layer for ImageLayer { /// debugging function to print out the contents of the layer - fn dump(&self, verbose: bool, ctx: &RequestContext) -> Result<()> { + async fn dump(&self, verbose: bool, ctx: &RequestContext) -> Result<()> { println!( "----- image layer for ten {} tli {} key {}-{} at {} is_incremental {} size {} ----", self.desc.tenant_id, @@ -189,7 +190,7 @@ impl Layer for ImageLayer { } /// Look up given page in the file - fn get_value_reconstruct_data( + async fn get_value_reconstruct_data( &self, key: Key, lsn_range: Range, diff --git a/pageserver/src/tenant/storage_layer/inmemory_layer.rs b/pageserver/src/tenant/storage_layer/inmemory_layer.rs index 77778822cf..31d0b5997a 100644 --- a/pageserver/src/tenant/storage_layer/inmemory_layer.rs +++ b/pageserver/src/tenant/storage_layer/inmemory_layer.rs @@ -7,7 +7,7 @@ use crate::config::PageServerConf; use crate::context::RequestContext; use crate::repository::{Key, Value}; -use crate::tenant::blob_io::{BlobCursor, BlobWriter}; +use crate::tenant::blob_io::BlobWriter; use crate::tenant::block_io::BlockReader; use crate::tenant::ephemeral_file::EphemeralFile; use crate::tenant::storage_layer::{ValueReconstructResult, ValueReconstructState}; @@ -110,6 +110,7 @@ impl InMemoryLayer { } } +#[async_trait::async_trait] impl Layer for InMemoryLayer { fn get_key_range(&self) -> Range { Key::MIN..Key::MAX @@ -132,7 +133,7 @@ impl Layer for InMemoryLayer { } /// debugging function to print out the contents of the layer - fn dump(&self, verbose: bool, _ctx: &RequestContext) -> Result<()> { + async fn dump(&self, verbose: bool, _ctx: &RequestContext) -> Result<()> { let inner = self.inner.read().unwrap(); let end_str = inner @@ -183,7 +184,7 @@ impl Layer for InMemoryLayer { } /// Look up given value in the layer. - fn get_value_reconstruct_data( + async fn get_value_reconstruct_data( &self, key: Key, lsn_range: Range, diff --git a/pageserver/src/tenant/storage_layer/remote_layer.rs b/pageserver/src/tenant/storage_layer/remote_layer.rs index d3c40d93bb..e5511d6051 100644 --- a/pageserver/src/tenant/storage_layer/remote_layer.rs +++ b/pageserver/src/tenant/storage_layer/remote_layer.rs @@ -65,8 +65,9 @@ impl std::fmt::Debug for RemoteLayer { } } +#[async_trait::async_trait] impl Layer for RemoteLayer { - fn get_value_reconstruct_data( + async fn get_value_reconstruct_data( &self, _key: Key, _lsn_range: Range, @@ -77,7 +78,7 @@ impl Layer for RemoteLayer { } /// debugging function to print out the contents of the layer - fn dump(&self, _verbose: bool, _ctx: &RequestContext) -> Result<()> { + async fn dump(&self, _verbose: bool, _ctx: &RequestContext) -> Result<()> { println!( "----- remote layer for ten {} tli {} keys {}-{} lsn {}-{} is_delta {} is_incremental {} size {} ----", self.desc.tenant_id, diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 616d184393..eceb54efc5 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -2387,12 +2387,15 @@ impl Timeline { // Get all the data needed to reconstruct the page version from this layer. // But if we have an older cached page image, no need to go past that. let lsn_floor = max(cached_lsn + 1, start_lsn); - result = match open_layer.get_value_reconstruct_data( - key, - lsn_floor..cont_lsn, - reconstruct_state, - ctx, - ) { + result = match open_layer + .get_value_reconstruct_data( + key, + lsn_floor..cont_lsn, + reconstruct_state, + ctx, + ) + .await + { Ok(result) => result, Err(e) => return Err(PageReconstructError::from(e)), }; @@ -2414,12 +2417,15 @@ impl Timeline { if cont_lsn > start_lsn { //info!("CHECKING for {} at {} on frozen layer {}", key, cont_lsn, frozen_layer.filename().display()); let lsn_floor = max(cached_lsn + 1, start_lsn); - result = match frozen_layer.get_value_reconstruct_data( - key, - lsn_floor..cont_lsn, - reconstruct_state, - ctx, - ) { + result = match frozen_layer + .get_value_reconstruct_data( + key, + lsn_floor..cont_lsn, + reconstruct_state, + ctx, + ) + .await + { Ok(result) => result, Err(e) => return Err(PageReconstructError::from(e)), }; @@ -2450,12 +2456,15 @@ impl Timeline { // Get all the data needed to reconstruct the page version from this layer. // But if we have an older cached page image, no need to go past that. let lsn_floor = max(cached_lsn + 1, lsn_floor); - result = match layer.get_value_reconstruct_data( - key, - lsn_floor..cont_lsn, - reconstruct_state, - ctx, - ) { + result = match layer + .get_value_reconstruct_data( + key, + lsn_floor..cont_lsn, + reconstruct_state, + ctx, + ) + .await + { Ok(result) => result, Err(e) => return Err(PageReconstructError::from(e)), }; From 407a20ceaeed4ae4fb8e20011bfc08571190b0aa Mon Sep 17 00:00:00 2001 From: Alex Chi Z Date: Mon, 24 Jul 2023 13:41:42 -0400 Subject: [PATCH 19/23] add proxy unit tests for retry connections (#4721) Given now we've refactored `connect_to_compute` as a generic, we can test it with mock backends. In this PR, we mock the error API and connect_once API to test the retry behavior of `connect_to_compute`. In the next PR, I'll add mock for credentials so that we can also test behavior with `wake_compute`. ref https://github.com/neondatabase/neon/issues/4709 --------- Signed-off-by: Alex Chi Z --- proxy/src/auth/credentials.rs | 8 ++ proxy/src/proxy/tests.rs | 149 ++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+) diff --git a/proxy/src/auth/credentials.rs b/proxy/src/auth/credentials.rs index 6787d82b71..7dc304d7ac 100644 --- a/proxy/src/auth/credentials.rs +++ b/proxy/src/auth/credentials.rs @@ -48,6 +48,14 @@ impl ClientCredentials<'_> { } impl<'a> ClientCredentials<'a> { + #[cfg(test)] + pub fn new_noop() -> Self { + ClientCredentials { + user: "", + project: None, + } + } + pub fn parse( params: &'a StartupMessageParams, sni: Option<&str>, diff --git a/proxy/src/proxy/tests.rs b/proxy/src/proxy/tests.rs index b9215cd90e..565f86eecc 100644 --- a/proxy/src/proxy/tests.rs +++ b/proxy/src/proxy/tests.rs @@ -1,5 +1,9 @@ //! A group of high-level tests for connection establishing logic and auth. +use std::borrow::Cow; + use super::*; +use crate::auth::ClientCredentials; +use crate::console::{CachedNodeInfo, NodeInfo}; use crate::{auth, sasl, scram}; use async_trait::async_trait; use rstest::rstest; @@ -304,3 +308,148 @@ fn connect_compute_total_wait() { assert!(total_wait < tokio::time::Duration::from_secs(12)); assert!(total_wait > tokio::time::Duration::from_secs(10)); } + +#[derive(Clone, Copy)] +enum ConnectAction { + Connect, + Retry, + Fail, +} + +struct TestConnectMechanism { + counter: Arc>, + sequence: Vec, +} + +impl TestConnectMechanism { + fn new(sequence: Vec) -> Self { + Self { + counter: Arc::new(std::sync::Mutex::new(0)), + sequence, + } + } +} + +#[derive(Debug)] +struct TestConnection; + +#[derive(Debug)] +struct TestConnectError { + retryable: bool, +} + +impl std::fmt::Display for TestConnectError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{:?}", self) + } +} + +impl std::error::Error for TestConnectError {} + +impl ShouldRetry for TestConnectError { + fn could_retry(&self) -> bool { + self.retryable + } +} + +#[async_trait] +impl ConnectMechanism for TestConnectMechanism { + type Connection = TestConnection; + type ConnectError = TestConnectError; + type Error = anyhow::Error; + + async fn connect_once( + &self, + _node_info: &console::CachedNodeInfo, + _timeout: time::Duration, + ) -> Result { + let mut counter = self.counter.lock().unwrap(); + let action = self.sequence[*counter]; + *counter += 1; + match action { + ConnectAction::Connect => Ok(TestConnection), + ConnectAction::Retry => Err(TestConnectError { retryable: true }), + ConnectAction::Fail => Err(TestConnectError { retryable: false }), + } + } + + fn update_connect_config(&self, _conf: &mut compute::ConnCfg) {} +} + +fn helper_create_connect_info() -> ( + CachedNodeInfo, + console::ConsoleReqExtra<'static>, + auth::BackendType<'static, ClientCredentials<'static>>, +) { + let node = NodeInfo { + config: compute::ConnCfg::new(), + aux: Default::default(), + allow_self_signed_compute: false, + }; + let cache = CachedNodeInfo::new_uncached(node); + let extra = console::ConsoleReqExtra { + session_id: uuid::Uuid::new_v4(), + application_name: Some("TEST"), + }; + let url = "https://TEST_URL".parse().unwrap(); + let api = console::provider::mock::Api::new(url); + let creds = auth::BackendType::Postgres(Cow::Owned(api), ClientCredentials::new_noop()); + (cache, extra, creds) +} + +#[tokio::test] +async fn connect_to_compute_success() { + use ConnectAction::*; + let mechanism = TestConnectMechanism::new(vec![Connect]); + let (cache, extra, creds) = helper_create_connect_info(); + connect_to_compute(&mechanism, cache, &extra, &creds) + .await + .unwrap(); +} + +#[tokio::test] +async fn connect_to_compute_retry() { + use ConnectAction::*; + let mechanism = TestConnectMechanism::new(vec![Retry, Retry, Connect]); + let (cache, extra, creds) = helper_create_connect_info(); + connect_to_compute(&mechanism, cache, &extra, &creds) + .await + .unwrap(); +} + +/// Test that we don't retry if the error is not retryable. +#[tokio::test] +async fn connect_to_compute_non_retry_1() { + use ConnectAction::*; + let mechanism = TestConnectMechanism::new(vec![Retry, Retry, Fail]); + let (cache, extra, creds) = helper_create_connect_info(); + connect_to_compute(&mechanism, cache, &extra, &creds) + .await + .unwrap_err(); +} + +/// Even for non-retryable errors, we should retry at least once. +#[tokio::test] +async fn connect_to_compute_non_retry_2() { + use ConnectAction::*; + let mechanism = TestConnectMechanism::new(vec![Fail, Retry, Connect]); + let (cache, extra, creds) = helper_create_connect_info(); + connect_to_compute(&mechanism, cache, &extra, &creds) + .await + .unwrap(); +} + +/// Retry for at most `NUM_RETRIES_CONNECT` times. +#[tokio::test] +async fn connect_to_compute_non_retry_3() { + assert_eq!(NUM_RETRIES_CONNECT, 10); + use ConnectAction::*; + let mechanism = TestConnectMechanism::new(vec![ + Retry, Retry, Retry, Retry, Retry, Retry, Retry, Retry, Retry, Retry, + /* the 11th time */ Retry, + ]); + let (cache, extra, creds) = helper_create_connect_info(); + connect_to_compute(&mechanism, cache, &extra, &creds) + .await + .unwrap_err(); +} From 294b8a8fde50418090c8fcb2b5c1c47e8cc11557 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Tue, 25 Jul 2023 00:43:27 +0300 Subject: [PATCH 20/23] Convert per timeline metrics to global (#4769) Cut down the per-(tenant, timeline) histograms by making them global: - `pageserver_getpage_get_reconstruct_data_seconds` - `pageserver_read_num_fs_layers` - `pageserver_remote_operation_seconds` - `pageserver_remote_timeline_client_calls_started` - `pageserver_wait_lsn_seconds` - `pageserver_io_operations_seconds` --------- Co-authored-by: Shany Pozin --- pageserver/src/metrics.rs | 118 +++++------------- pageserver/src/tenant/timeline.rs | 9 +- pageserver/src/virtual_file.rs | 26 ++-- test_runner/fixtures/metrics.py | 25 ++-- test_runner/regress/test_gc_aggressive.py | 2 - test_runner/regress/test_metric_collection.py | 2 - test_runner/regress/test_ondemand_download.py | 70 +++++------ test_runner/regress/test_remote_storage.py | 52 ++++---- test_runner/regress/test_tenants.py | 3 + 9 files changed, 126 insertions(+), 181 deletions(-) diff --git a/pageserver/src/metrics.rs b/pageserver/src/metrics.rs index ee8dfba69a..bdd79711ec 100644 --- a/pageserver/src/metrics.rs +++ b/pageserver/src/metrics.rs @@ -84,11 +84,10 @@ pub static STORAGE_TIME_GLOBAL: Lazy = Lazy::new(|| { .expect("failed to define a metric") }); -static READ_NUM_FS_LAYERS: Lazy = Lazy::new(|| { - register_histogram_vec!( +pub(crate) static READ_NUM_FS_LAYERS: Lazy = Lazy::new(|| { + register_histogram!( "pageserver_read_num_fs_layers", "Number of persistent layers accessed for processing a read request, including those in the cache", - &["tenant_id", "timeline_id"], vec![1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 10.0, 20.0, 50.0, 100.0], ) .expect("failed to define a metric") @@ -112,11 +111,10 @@ pub static MATERIALIZED_PAGE_CACHE_HIT_DIRECT: Lazy = Lazy::new(|| { .expect("failed to define a metric") }); -static GET_RECONSTRUCT_DATA_TIME: Lazy = Lazy::new(|| { - register_histogram_vec!( +pub(crate) static GET_RECONSTRUCT_DATA_TIME: Lazy = Lazy::new(|| { + register_histogram!( "pageserver_getpage_get_reconstruct_data_seconds", "Time spent in get_reconstruct_value_data", - &["tenant_id", "timeline_id"], CRITICAL_OP_BUCKETS.into(), ) .expect("failed to define a metric") @@ -246,11 +244,10 @@ pub static PAGE_CACHE_SIZE: Lazy = Lazy::new(|| PageCacheS }, }); -static WAIT_LSN_TIME: Lazy = Lazy::new(|| { - register_histogram_vec!( +pub(crate) static WAIT_LSN_TIME: Lazy = Lazy::new(|| { + register_histogram!( "pageserver_wait_lsn_seconds", "Time spent waiting for WAL to arrive", - &["tenant_id", "timeline_id"], CRITICAL_OP_BUCKETS.into(), ) .expect("failed to define a metric") @@ -499,23 +496,31 @@ const STORAGE_IO_TIME_BUCKETS: &[f64] = &[ 30.000, // 30000 ms ]; -const STORAGE_IO_TIME_OPERATIONS: &[&str] = &[ - "open", "close", "read", "write", "seek", "fsync", "gc", "metadata", -]; - -const STORAGE_IO_SIZE_OPERATIONS: &[&str] = &["read", "write"]; - -pub static STORAGE_IO_TIME: Lazy = Lazy::new(|| { +/// Tracks time taken by fs operations near VirtualFile. +/// +/// Operations: +/// - open ([`std::fs::OpenOptions::open`]) +/// - close (dropping [`std::fs::File`]) +/// - close-by-replace (close by replacement algorithm) +/// - read (`read_at`) +/// - write (`write_at`) +/// - seek (modify internal position or file length query) +/// - fsync ([`std::fs::File::sync_all`]) +/// - metadata ([`std::fs::File::metadata`]) +pub(crate) static STORAGE_IO_TIME: Lazy = Lazy::new(|| { register_histogram_vec!( "pageserver_io_operations_seconds", "Time spent in IO operations", - &["operation", "tenant_id", "timeline_id"], + &["operation"], STORAGE_IO_TIME_BUCKETS.into() ) .expect("failed to define a metric") }); -pub static STORAGE_IO_SIZE: Lazy = Lazy::new(|| { +const STORAGE_IO_SIZE_OPERATIONS: &[&str] = &["read", "write"]; + +// Needed for the https://neonprod.grafana.net/d/5uK9tHL4k/picking-tenant-for-relocation?orgId=1 +pub(crate) static STORAGE_IO_SIZE: Lazy = Lazy::new(|| { register_int_gauge_vec!( "pageserver_io_operations_bytes_total", "Total amount of bytes read/written in IO operations", @@ -605,7 +610,7 @@ static REMOTE_TIMELINE_CLIENT_CALLS_STARTED_HIST: Lazy = Lazy::new at a given instant. It gives you a better idea of the queue depth \ than plotting the gauge directly, since operations may complete faster \ than the sampling interval.", - &["tenant_id", "timeline_id", "file_kind", "op_kind"], + &["file_kind", "op_kind"], // The calls_unfinished gauge is an integer gauge, hence we have integer buckets. vec![0.0, 1.0, 2.0, 4.0, 6.0, 8.0, 10.0, 15.0, 20.0, 40.0, 60.0, 80.0, 100.0, 500.0], ) @@ -662,13 +667,13 @@ impl RemoteOpFileKind { } } -pub static REMOTE_OPERATION_TIME: Lazy = Lazy::new(|| { +pub(crate) static REMOTE_OPERATION_TIME: Lazy = Lazy::new(|| { register_histogram_vec!( "pageserver_remote_operation_seconds", "Time spent on remote storage operations. \ Grouped by tenant, timeline, operation_kind and status. \ Does not account for time spent waiting in remote timeline client's queues.", - &["tenant_id", "timeline_id", "file_kind", "op_kind", "status"] + &["file_kind", "op_kind", "status"] ) .expect("failed to define a metric") }); @@ -897,7 +902,6 @@ impl StorageTimeMetrics { pub struct TimelineMetrics { tenant_id: String, timeline_id: String, - pub get_reconstruct_data_time_histo: Histogram, pub flush_time_histo: StorageTimeMetrics, pub compact_time_histo: StorageTimeMetrics, pub create_images_time_histo: StorageTimeMetrics, @@ -906,9 +910,7 @@ pub struct TimelineMetrics { pub load_layer_map_histo: StorageTimeMetrics, pub garbage_collect_histo: StorageTimeMetrics, pub last_record_gauge: IntGauge, - pub wait_lsn_time_histo: Histogram, pub resident_physical_size_gauge: UIntGauge, - pub read_num_fs_layers: Histogram, /// copy of LayeredTimeline.current_logical_size pub current_logical_size_gauge: UIntGauge, pub num_persistent_files_created: IntCounter, @@ -925,9 +927,6 @@ impl TimelineMetrics { ) -> Self { let tenant_id = tenant_id.to_string(); let timeline_id = timeline_id.to_string(); - let get_reconstruct_data_time_histo = GET_RECONSTRUCT_DATA_TIME - .get_metric_with_label_values(&[&tenant_id, &timeline_id]) - .unwrap(); let flush_time_histo = StorageTimeMetrics::new(StorageTimeOperation::LayerFlush, &tenant_id, &timeline_id); let compact_time_histo = @@ -948,9 +947,6 @@ impl TimelineMetrics { let last_record_gauge = LAST_RECORD_LSN .get_metric_with_label_values(&[&tenant_id, &timeline_id]) .unwrap(); - let wait_lsn_time_histo = WAIT_LSN_TIME - .get_metric_with_label_values(&[&tenant_id, &timeline_id]) - .unwrap(); let resident_physical_size_gauge = RESIDENT_PHYSICAL_SIZE .get_metric_with_label_values(&[&tenant_id, &timeline_id]) .unwrap(); @@ -966,16 +962,12 @@ impl TimelineMetrics { let evictions = EVICTIONS .get_metric_with_label_values(&[&tenant_id, &timeline_id]) .unwrap(); - let read_num_fs_layers = READ_NUM_FS_LAYERS - .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, timeline_id, - get_reconstruct_data_time_histo, flush_time_histo, compact_time_histo, create_images_time_histo, @@ -984,7 +976,6 @@ impl TimelineMetrics { garbage_collect_histo, load_layer_map_histo, last_record_gauge, - wait_lsn_time_histo, resident_physical_size_gauge, current_logical_size_gauge, num_persistent_files_created, @@ -993,7 +984,6 @@ impl TimelineMetrics { evictions_with_low_residence_duration: std::sync::RwLock::new( evictions_with_low_residence_duration, ), - read_num_fs_layers, } } } @@ -1002,15 +992,12 @@ impl Drop for TimelineMetrics { fn drop(&mut self) { let tenant_id = &self.tenant_id; let timeline_id = &self.timeline_id; - let _ = GET_RECONSTRUCT_DATA_TIME.remove_label_values(&[tenant_id, timeline_id]); let _ = LAST_RECORD_LSN.remove_label_values(&[tenant_id, timeline_id]); - let _ = WAIT_LSN_TIME.remove_label_values(&[tenant_id, timeline_id]); let _ = RESIDENT_PHYSICAL_SIZE.remove_label_values(&[tenant_id, timeline_id]); 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]); - let _ = READ_NUM_FS_LAYERS.remove_label_values(&[tenant_id, timeline_id]); self.evictions_with_low_residence_duration .write() @@ -1022,9 +1009,6 @@ impl Drop for TimelineMetrics { let _ = STORAGE_TIME_COUNT_PER_TIMELINE.remove_label_values(&[op, tenant_id, timeline_id]); } - for op in STORAGE_IO_TIME_OPERATIONS { - let _ = STORAGE_IO_TIME.remove_label_values(&[op, tenant_id, timeline_id]); - } for op in STORAGE_IO_SIZE_OPERATIONS { let _ = STORAGE_IO_SIZE.remove_label_values(&[op, tenant_id, timeline_id]); @@ -1056,9 +1040,7 @@ pub struct RemoteTimelineClientMetrics { tenant_id: String, timeline_id: String, remote_physical_size_gauge: Mutex>, - remote_operation_time: Mutex>, calls_unfinished_gauge: Mutex>, - calls_started_hist: Mutex>, bytes_started_counter: Mutex>, bytes_finished_counter: Mutex>, } @@ -1068,14 +1050,13 @@ impl RemoteTimelineClientMetrics { RemoteTimelineClientMetrics { tenant_id: tenant_id.to_string(), timeline_id: timeline_id.to_string(), - remote_operation_time: Mutex::new(HashMap::default()), calls_unfinished_gauge: Mutex::new(HashMap::default()), - calls_started_hist: Mutex::new(HashMap::default()), bytes_started_counter: Mutex::new(HashMap::default()), bytes_finished_counter: Mutex::new(HashMap::default()), remote_physical_size_gauge: Mutex::new(None), } } + pub fn remote_physical_size_gauge(&self) -> UIntGauge { let mut guard = self.remote_physical_size_gauge.lock().unwrap(); guard @@ -1089,26 +1070,17 @@ impl RemoteTimelineClientMetrics { }) .clone() } + pub fn remote_operation_time( &self, file_kind: &RemoteOpFileKind, op_kind: &RemoteOpKind, status: &'static str, ) -> Histogram { - let mut guard = self.remote_operation_time.lock().unwrap(); let key = (file_kind.as_str(), op_kind.as_str(), status); - let metric = guard.entry(key).or_insert_with(move || { - REMOTE_OPERATION_TIME - .get_metric_with_label_values(&[ - &self.tenant_id.to_string(), - &self.timeline_id.to_string(), - key.0, - key.1, - key.2, - ]) - .unwrap() - }); - metric.clone() + REMOTE_OPERATION_TIME + .get_metric_with_label_values(&[key.0, key.1, key.2]) + .unwrap() } fn calls_unfinished_gauge( @@ -1136,19 +1108,10 @@ impl RemoteTimelineClientMetrics { file_kind: &RemoteOpFileKind, op_kind: &RemoteOpKind, ) -> Histogram { - let mut guard = self.calls_started_hist.lock().unwrap(); let key = (file_kind.as_str(), op_kind.as_str()); - let metric = guard.entry(key).or_insert_with(move || { - REMOTE_TIMELINE_CLIENT_CALLS_STARTED_HIST - .get_metric_with_label_values(&[ - &self.tenant_id.to_string(), - &self.timeline_id.to_string(), - key.0, - key.1, - ]) - .unwrap() - }); - metric.clone() + REMOTE_TIMELINE_CLIENT_CALLS_STARTED_HIST + .get_metric_with_label_values(&[key.0, key.1]) + .unwrap() } fn bytes_started_counter( @@ -1328,15 +1291,10 @@ impl Drop for RemoteTimelineClientMetrics { tenant_id, timeline_id, remote_physical_size_gauge, - remote_operation_time, calls_unfinished_gauge, - calls_started_hist, bytes_started_counter, bytes_finished_counter, } = self; - for ((a, b, c), _) in remote_operation_time.get_mut().unwrap().drain() { - let _ = REMOTE_OPERATION_TIME.remove_label_values(&[tenant_id, timeline_id, a, b, c]); - } for ((a, b), _) in calls_unfinished_gauge.get_mut().unwrap().drain() { let _ = REMOTE_TIMELINE_CLIENT_CALLS_UNFINISHED_GAUGE.remove_label_values(&[ tenant_id, @@ -1345,14 +1303,6 @@ impl Drop for RemoteTimelineClientMetrics { b, ]); } - for ((a, b), _) in calls_started_hist.get_mut().unwrap().drain() { - let _ = REMOTE_TIMELINE_CLIENT_CALLS_STARTED_HIST.remove_label_values(&[ - tenant_id, - timeline_id, - a, - b, - ]); - } for ((a, b), _) in bytes_started_counter.get_mut().unwrap().drain() { let _ = REMOTE_TIMELINE_CLIENT_BYTES_STARTED_COUNTER.remove_label_values(&[ tenant_id, diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index eceb54efc5..3413282c5d 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -475,7 +475,7 @@ impl Timeline { img: cached_page_img, }; - let timer = self.metrics.get_reconstruct_data_time_histo.start_timer(); + let timer = crate::metrics::GET_RECONSTRUCT_DATA_TIME.start_timer(); self.get_reconstruct_data(key, lsn, &mut reconstruct_state, ctx) .await?; timer.stop_and_record(); @@ -555,7 +555,7 @@ impl Timeline { "wait_lsn cannot be called in WAL receiver" ); - let _timer = self.metrics.wait_lsn_time_histo.start_timer(); + let _timer = crate::metrics::WAIT_LSN_TIME.start_timer(); match self .last_record_lsn @@ -2252,8 +2252,9 @@ impl Timeline { let mut timeline_owned; let mut timeline = self; - let mut read_count = - scopeguard::guard(0, |cnt| self.metrics.read_num_fs_layers.observe(cnt as f64)); + let mut read_count = scopeguard::guard(0, |cnt| { + crate::metrics::READ_NUM_FS_LAYERS.observe(cnt as f64) + }); // For debugging purposes, collect the path of layers that we traversed // through. It's included in the error message if we fail to find the key. diff --git a/pageserver/src/virtual_file.rs b/pageserver/src/virtual_file.rs index b86d14f158..65b4363f60 100644 --- a/pageserver/src/virtual_file.rs +++ b/pageserver/src/virtual_file.rs @@ -149,12 +149,10 @@ impl OpenFiles { // old file. // if let Some(old_file) = slot_guard.file.take() { - // We do not have information about tenant_id/timeline_id of evicted file. - // It is possible to store path together with file or use filepath crate, - // but as far as close() is not expected to be fast, it is not so critical to gather - // precise per-tenant statistic here. + // the normal path of dropping VirtualFile uses "close", use "close-by-replace" here to + // distinguish the two. STORAGE_IO_TIME - .with_label_values(&["close", "-", "-"]) + .with_label_values(&["close-by-replace"]) .observe_closure_duration(|| drop(old_file)); } @@ -208,7 +206,7 @@ impl VirtualFile { } let (handle, mut slot_guard) = get_open_files().find_victim_slot(); let file = STORAGE_IO_TIME - .with_label_values(&["open", &tenant_id, &timeline_id]) + .with_label_values(&["open"]) .observe_closure_duration(|| open_options.open(path))?; // Strip all options other than read and write. @@ -271,7 +269,7 @@ impl VirtualFile { // Found a cached file descriptor. slot.recently_used.store(true, Ordering::Relaxed); return Ok(STORAGE_IO_TIME - .with_label_values(&[op, &self.tenant_id, &self.timeline_id]) + .with_label_values(&[op]) .observe_closure_duration(|| func(file))); } } @@ -298,12 +296,12 @@ impl VirtualFile { // Open the physical file let file = STORAGE_IO_TIME - .with_label_values(&["open", &self.tenant_id, &self.timeline_id]) + .with_label_values(&["open"]) .observe_closure_duration(|| self.open_options.open(&self.path))?; // Perform the requested operation on it let result = STORAGE_IO_TIME - .with_label_values(&[op, &self.tenant_id, &self.timeline_id]) + .with_label_values(&[op]) .observe_closure_duration(|| func(&file)); // Store the File in the slot and update the handle in the VirtualFile @@ -333,13 +331,11 @@ impl Drop for VirtualFile { let mut slot_guard = slot.inner.write().unwrap(); if slot_guard.tag == handle.tag { slot.recently_used.store(false, Ordering::Relaxed); - // Unlike files evicted by replacement algorithm, here - // we group close time by tenant_id/timeline_id. - // At allows to compare number/time of "normal" file closes - // with file eviction. + // there is also operation "close-by-replace" for closes done on eviction for + // comparison. STORAGE_IO_TIME - .with_label_values(&["close", &self.tenant_id, &self.timeline_id]) - .observe_closure_duration(|| slot_guard.file.take()); + .with_label_values(&["close"]) + .observe_closure_duration(|| drop(slot_guard.file.take())); } } } diff --git a/test_runner/fixtures/metrics.py b/test_runner/fixtures/metrics.py index 3f87aa10a3..ca468cc02c 100644 --- a/test_runner/fixtures/metrics.py +++ b/test_runner/fixtures/metrics.py @@ -40,10 +40,13 @@ def parse_metrics(text: str, name: str = "") -> Metrics: return metrics +def histogram(prefix_without_trailing_underscore: str) -> List[str]: + assert not prefix_without_trailing_underscore.endswith("_") + return [f"{prefix_without_trailing_underscore}_{x}" for x in ["bucket", "count", "sum"]] + + PAGESERVER_PER_TENANT_REMOTE_TIMELINE_CLIENT_METRICS: Tuple[str, ...] = ( "pageserver_remote_timeline_client_calls_unfinished", - *[f"pageserver_remote_timeline_client_calls_started_{x}" for x in ["bucket", "count", "sum"]], - *[f"pageserver_remote_operation_seconds_{x}" for x in ["bucket", "count", "sum"]], "pageserver_remote_physical_size", "pageserver_remote_timeline_client_bytes_started_total", "pageserver_remote_timeline_client_bytes_finished_total", @@ -67,30 +70,24 @@ PAGESERVER_GLOBAL_METRICS: Tuple[str, ...] = ( "pageserver_getpage_reconstruct_seconds_count", "pageserver_getpage_reconstruct_seconds_sum", *[f"pageserver_basebackup_query_seconds_{x}" for x in ["bucket", "count", "sum"]], + *histogram("pageserver_read_num_fs_layers"), + *histogram("pageserver_getpage_get_reconstruct_data_seconds"), + *histogram("pageserver_wait_lsn_seconds"), + *histogram("pageserver_remote_operation_seconds"), + *histogram("pageserver_remote_timeline_client_calls_started"), + *histogram("pageserver_io_operations_seconds"), ) PAGESERVER_PER_TENANT_METRICS: Tuple[str, ...] = ( "pageserver_current_logical_size", "pageserver_resident_physical_size", - "pageserver_getpage_get_reconstruct_data_seconds_bucket", - "pageserver_getpage_get_reconstruct_data_seconds_count", - "pageserver_getpage_get_reconstruct_data_seconds_sum", "pageserver_io_operations_bytes_total", - "pageserver_io_operations_seconds_bucket", - "pageserver_io_operations_seconds_count", - "pageserver_io_operations_seconds_sum", "pageserver_last_record_lsn", - "pageserver_read_num_fs_layers_bucket", - "pageserver_read_num_fs_layers_count", - "pageserver_read_num_fs_layers_sum", "pageserver_smgr_query_seconds_bucket", "pageserver_smgr_query_seconds_count", "pageserver_smgr_query_seconds_sum", "pageserver_storage_operations_seconds_count_total", "pageserver_storage_operations_seconds_sum_total", - "pageserver_wait_lsn_seconds_bucket", - "pageserver_wait_lsn_seconds_count", - "pageserver_wait_lsn_seconds_sum", "pageserver_created_persistent_files_total", "pageserver_written_persistent_bytes_total", "pageserver_tenant_states_count", diff --git a/test_runner/regress/test_gc_aggressive.py b/test_runner/regress/test_gc_aggressive.py index 18f506cfce..e26b1a980e 100644 --- a/test_runner/regress/test_gc_aggressive.py +++ b/test_runner/regress/test_gc_aggressive.py @@ -136,8 +136,6 @@ def test_gc_index_upload(neon_env_builder: NeonEnvBuilder, remote_storage_kind: for sample in ps_metrics.query_all( name="pageserver_remote_operation_seconds_count", filter={ - "tenant_id": str(tenant_id), - "timeline_id": str(timeline_id), "file_kind": str(file_kind), "op_kind": str(op_kind), }, diff --git a/test_runner/regress/test_metric_collection.py b/test_runner/regress/test_metric_collection.py index 12e695bcbd..61c8f800f8 100644 --- a/test_runner/regress/test_metric_collection.py +++ b/test_runner/regress/test_metric_collection.py @@ -140,8 +140,6 @@ def test_metric_collection( for sample in ps_metrics.query_all( name="pageserver_remote_operation_seconds_count", filter={ - "tenant_id": str(tenant_id), - "timeline_id": str(timeline_id), "file_kind": str(file_kind), "op_kind": str(op_kind), }, diff --git a/test_runner/regress/test_ondemand_download.py b/test_runner/regress/test_ondemand_download.py index a30f0f02e1..b92981a371 100644 --- a/test_runner/regress/test_ondemand_download.py +++ b/test_runner/regress/test_ondemand_download.py @@ -27,15 +27,16 @@ from fixtures.types import Lsn from fixtures.utils import query_scalar, wait_until -def get_num_downloaded_layers(client: PageserverHttpClient, tenant_id, timeline_id): +def get_num_downloaded_layers(client: PageserverHttpClient): + """ + This assumes that the pageserver only has a single tenant. + """ value = client.get_metric_value( "pageserver_remote_operation_seconds_count", { "file_kind": "layer", "op_kind": "download", "status": "success", - "tenant_id": tenant_id, - "timeline_id": timeline_id, }, ) if value is None: @@ -57,7 +58,8 @@ def test_ondemand_download_large_rel( test_name="test_ondemand_download_large_rel", ) - ##### First start, insert secret data and upload it to the remote storage + # thinking about using a shared environment? the test assumes that global + # metrics are for single tenant. env = neon_env_builder.init_start( initial_tenant_conf={ # disable background GC @@ -129,7 +131,7 @@ def test_ondemand_download_large_rel( # safekeepers, that have now been shut down. endpoint = env.endpoints.create_start("main", lsn=current_lsn) - before_downloads = get_num_downloaded_layers(client, tenant_id, timeline_id) + before_downloads = get_num_downloaded_layers(client) assert before_downloads != 0, "basebackup should on-demand non-zero layers" # Probe in the middle of the table. There's a high chance that the beginning @@ -140,7 +142,7 @@ def test_ondemand_download_large_rel( with endpoint.cursor() as cur: assert query_scalar(cur, "select count(*) from tbl where id = 500000") == 1 - after_downloads = get_num_downloaded_layers(client, tenant_id, timeline_id) + after_downloads = get_num_downloaded_layers(client) log.info(f"layers downloaded before {before_downloads} and after {after_downloads}") assert after_downloads > before_downloads @@ -159,13 +161,11 @@ def test_ondemand_download_timetravel( test_name="test_ondemand_download_timetravel", ) - ##### First start, insert data and upload it to the remote storage - env = neon_env_builder.init_start() - pageserver_http = env.pageserver.http_client() + # thinking about using a shared environment? the test assumes that global + # metrics are for single tenant. - # Override defaults, to create more layers - tenant, _ = env.neon_cli.create_tenant( - conf={ + env = neon_env_builder.init_start( + initial_tenant_conf={ # Disable background GC & compaction # We don't want GC, that would break the assertion about num downloads. # We don't want background compaction, we force a compaction every time we do explicit checkpoint. @@ -178,7 +178,7 @@ def test_ondemand_download_timetravel( "compaction_target_size": f"{1 * 1024 ** 2}", # 1 MB } ) - env.initial_tenant = tenant + pageserver_http = env.pageserver.http_client() endpoint = env.endpoints.create_start("main") @@ -283,7 +283,7 @@ def test_ondemand_download_timetravel( == table_len ) - after_downloads = get_num_downloaded_layers(client, tenant_id, timeline_id) + after_downloads = get_num_downloaded_layers(client) num_layers_downloaded.append(after_downloads) log.info(f"num_layers_downloaded[-1]={num_layers_downloaded[-1]}") @@ -324,11 +324,8 @@ def test_download_remote_layers_api( ) ##### First start, insert data and upload it to the remote storage - env = neon_env_builder.init_start() - - # Override defaults, to create more layers - tenant, _ = env.neon_cli.create_tenant( - conf={ + env = neon_env_builder.init_start( + initial_tenant_conf={ # Disable background GC & compaction # We don't want GC, that would break the assertion about num downloads. # We don't want background compaction, we force a compaction every time we do explicit checkpoint. @@ -341,7 +338,6 @@ def test_download_remote_layers_api( "compaction_target_size": f"{1 * 1024 ** 2}", # 1 MB } ) - env.initial_tenant = tenant endpoint = env.endpoints.create_start("main") @@ -489,8 +485,6 @@ def test_compaction_downloads_on_demand_without_image_creation( test_name="test_compaction_downloads_on_demand_without_image_creation", ) - env = neon_env_builder.init_start() - conf = { # Disable background GC & compaction "gc_period": "0s", @@ -506,6 +500,8 @@ def test_compaction_downloads_on_demand_without_image_creation( # pitr_interval and gc_horizon are not interesting because we dont run gc } + env = neon_env_builder.init_start(initial_tenant_conf=stringify(conf)) + def downloaded_bytes_and_count(pageserver_http: PageserverHttpClient) -> Tuple[int, int]: m = pageserver_http.get_metrics() # these are global counters @@ -517,11 +513,12 @@ def test_compaction_downloads_on_demand_without_image_creation( assert count < 2**53 and count.is_integer(), "count should still be safe integer-in-f64" return (int(total_bytes), int(count)) - # Override defaults, to create more layers - tenant_id, timeline_id = env.neon_cli.create_tenant(conf=stringify(conf)) - env.initial_tenant = tenant_id pageserver_http = env.pageserver.http_client() + tenant_id = env.initial_tenant + timeline_id = env.initial_timeline + assert timeline_id is not None + with env.endpoints.create_start("main") as endpoint: # no particular reason to create the layers like this, but we are sure # not to hit the image_creation_threshold here. @@ -577,8 +574,6 @@ def test_compaction_downloads_on_demand_with_image_creation( test_name="test_compaction_downloads_on_demand", ) - env = neon_env_builder.init_start() - conf = { # Disable background GC & compaction "gc_period": "0s", @@ -593,9 +588,11 @@ def test_compaction_downloads_on_demand_with_image_creation( # pitr_interval and gc_horizon are not interesting because we dont run gc } - # Override defaults, to create more layers - tenant_id, timeline_id = env.neon_cli.create_tenant(conf=stringify(conf)) - env.initial_tenant = tenant_id + env = neon_env_builder.init_start(initial_tenant_conf=stringify(conf)) + tenant_id = env.initial_tenant + timeline_id = env.initial_timeline + assert timeline_id is not None + pageserver_http = env.pageserver.http_client() endpoint = env.endpoints.create_start("main") @@ -664,10 +661,6 @@ def test_compaction_downloads_on_demand_with_image_creation( assert dict(kinds_after) == {"Delta": 4, "Image": 1} -def stringify(conf: Dict[str, Any]) -> Dict[str, str]: - return dict(map(lambda x: (x[0], str(x[1])), conf.items())) - - @pytest.mark.parametrize("remote_storage_kind", [RemoteStorageKind.LOCAL_FS]) def test_ondemand_download_failure_to_replace( neon_env_builder: NeonEnvBuilder, remote_storage_kind: RemoteStorageKind @@ -691,9 +684,10 @@ def test_ondemand_download_failure_to_replace( env = neon_env_builder.init_start() - tenant_id, timeline_id = env.neon_cli.create_tenant() + tenant_id = env.initial_tenant + timeline_id = env.initial_timeline + assert timeline_id is not None - env.initial_tenant = tenant_id pageserver_http = env.pageserver.http_client() lsn = Lsn(pageserver_http.timeline_detail(tenant_id, timeline_id)["last_record_lsn"]) @@ -724,3 +718,7 @@ def test_ondemand_download_failure_to_replace( env.pageserver.allowed_errors.append(".* ERROR .*Task 'initial size calculation'") # if the above returned, then we didn't have a livelock, and all is well + + +def stringify(conf: Dict[str, Any]) -> Dict[str, str]: + return dict(map(lambda x: (x[0], str(x[1])), conf.items())) diff --git a/test_runner/regress/test_remote_storage.py b/test_runner/regress/test_remote_storage.py index f1575ae4d3..7c04ed9017 100644 --- a/test_runner/regress/test_remote_storage.py +++ b/test_runner/regress/test_remote_storage.py @@ -378,12 +378,10 @@ def test_remote_timeline_client_calls_started_metric( test_name="test_remote_timeline_client_metrics", ) - env = neon_env_builder.init_start() - - # create tenant with config that will determinstically allow - # compaction and gc - tenant_id, timeline_id = env.neon_cli.create_tenant( - conf={ + # thinking about using a shared environment? the test assumes that global + # metrics are for single tenant. + env = neon_env_builder.init_start( + initial_tenant_conf={ # small checkpointing and compaction targets to ensure we generate many upload operations "checkpoint_distance": f"{128 * 1024}", "compaction_threshold": "1", @@ -398,6 +396,10 @@ def test_remote_timeline_client_calls_started_metric( } ) + tenant_id = env.initial_tenant + assert env.initial_timeline is not None + timeline_id: TimelineId = env.initial_timeline + client = env.pageserver.http_client() endpoint = env.endpoints.create_start("main", tenant_id=tenant_id) @@ -419,6 +421,7 @@ def test_remote_timeline_client_calls_started_metric( "VACUUM foo", ] ) + assert timeline_id is not None wait_for_last_flush_lsn(env, endpoint, tenant_id, timeline_id) calls_started: Dict[Tuple[str, str], List[int]] = { @@ -428,13 +431,14 @@ def test_remote_timeline_client_calls_started_metric( } def fetch_calls_started(): + assert timeline_id is not None for (file_kind, op_kind), observations in calls_started.items(): - val = client.get_remote_timeline_client_metric( - "pageserver_remote_timeline_client_calls_started_count", - tenant_id, - timeline_id, - file_kind, - op_kind, + val = client.get_metric_value( + name="pageserver_remote_timeline_client_calls_started_count", + filter={ + "file_kind": str(file_kind), + "op_kind": str(op_kind), + }, ) assert val is not None, f"expecting metric to be present: {file_kind} {op_kind}" val = int(val) @@ -518,12 +522,8 @@ def test_timeline_deletion_with_files_stuck_in_upload_queue( test_name="test_timeline_deletion_with_files_stuck_in_upload_queue", ) - env = neon_env_builder.init_start() - - # create tenant with config that will determinstically allow - # compaction and gc - tenant_id, timeline_id = env.neon_cli.create_tenant( - conf={ + env = neon_env_builder.init_start( + initial_tenant_conf={ # small checkpointing and compaction targets to ensure we generate many operations "checkpoint_distance": f"{64 * 1024}", "compaction_threshold": "1", @@ -535,6 +535,10 @@ def test_timeline_deletion_with_files_stuck_in_upload_queue( "pitr_interval": "0s", } ) + tenant_id = env.initial_tenant + assert env.initial_timeline is not None + timeline_id: TimelineId = env.initial_timeline + timeline_path = env.timeline_dir(tenant_id, timeline_id) client = env.pageserver.http_client() @@ -787,12 +791,8 @@ def test_compaction_delete_before_upload( test_name="test_compaction_delete_before_upload", ) - env = neon_env_builder.init_start() - - # create tenant with config that will determinstically allow - # compaction and disables gc - tenant_id, timeline_id = env.neon_cli.create_tenant( - conf={ + env = neon_env_builder.init_start( + initial_tenant_conf={ # Set a small compaction threshold "compaction_threshold": "3", # Disable GC @@ -802,6 +802,10 @@ def test_compaction_delete_before_upload( } ) + tenant_id = env.initial_tenant + assert env.initial_timeline is not None + timeline_id: TimelineId = env.initial_timeline + client = env.pageserver.http_client() with env.endpoints.create_start("main", tenant_id=tenant_id) as endpoint: diff --git a/test_runner/regress/test_tenants.py b/test_runner/regress/test_tenants.py index 4dbfa8bc1f..35f7941bda 100644 --- a/test_runner/regress/test_tenants.py +++ b/test_runner/regress/test_tenants.py @@ -213,6 +213,9 @@ def test_metrics_normal_work(neon_env_builder: NeonEnvBuilder): # Test (a subset of) pageserver global metrics for metric in PAGESERVER_GLOBAL_METRICS: + if metric.startswith("pageserver_remote"): + continue + ps_samples = ps_metrics.query_all(metric, {}) assert len(ps_samples) > 0, f"expected at least one sample for {metric}" for sample in ps_samples: From a25504deae98096cb0990c3c5636969f8ec47f8e Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Tue, 25 Jul 2023 10:19:04 +0300 Subject: [PATCH 21/23] Limit concurrent compactions (#4777) Compactions can create a lot of concurrent work right now with #4265. Limit compactions to use at most 6/8 background runtime threads. --- Cargo.lock | 1 + pageserver/Cargo.toml | 2 ++ pageserver/src/http/routes.rs | 32 ++++++++++---------- pageserver/src/task_mgr.rs | 16 +++++++++- pageserver/src/tenant.rs | 24 +++++++++------ pageserver/src/tenant/mgr.rs | 49 ------------------------------- pageserver/src/tenant/tasks.rs | 2 +- pageserver/src/tenant/timeline.rs | 43 ++++++++++++++++++++++++--- 8 files changed, 88 insertions(+), 81 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 05a70bfe55..506e5f6c7c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2545,6 +2545,7 @@ dependencies = [ "metrics", "nix", "num-traits", + "num_cpus", "once_cell", "pageserver_api", "pin-project-lite", diff --git a/pageserver/Cargo.toml b/pageserver/Cargo.toml index 27e90ea97d..d709fac5cb 100644 --- a/pageserver/Cargo.toml +++ b/pageserver/Cargo.toml @@ -35,6 +35,8 @@ humantime-serde.workspace = true hyper.workspace = true itertools.workspace = true nix.workspace = true +# hack to get the number of worker threads tokio uses +num_cpus = { version = "1.15" } num-traits.workspace = true once_cell.workspace = true pin-project-lite.workspace = true diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs index 08fb917fb6..c10223beed 100644 --- a/pageserver/src/http/routes.rs +++ b/pageserver/src/http/routes.rs @@ -994,31 +994,29 @@ async fn timeline_gc_handler( // Run compaction immediately on given timeline. async fn timeline_compact_handler( request: Request, - _cancel: CancellationToken, + cancel: CancellationToken, ) -> Result, ApiError> { let tenant_id: TenantId = parse_request_param(&request, "tenant_id")?; let timeline_id: TimelineId = parse_request_param(&request, "timeline_id")?; check_permission(&request, Some(tenant_id))?; - let ctx = RequestContext::new(TaskKind::MgmtRequest, DownloadBehavior::Download); - let result_receiver = mgr::immediate_compact(tenant_id, timeline_id, &ctx) - .await - .context("spawn compaction task") - .map_err(ApiError::InternalServerError)?; - - let result: anyhow::Result<()> = result_receiver - .await - .context("receive compaction result") - .map_err(ApiError::InternalServerError)?; - result.map_err(ApiError::InternalServerError)?; - - json_response(StatusCode::OK, ()) + async { + let ctx = RequestContext::new(TaskKind::MgmtRequest, DownloadBehavior::Download); + let timeline = active_timeline_of_active_tenant(tenant_id, timeline_id).await?; + timeline + .compact(&cancel, &ctx) + .await + .map_err(ApiError::InternalServerError)?; + json_response(StatusCode::OK, ()) + } + .instrument(info_span!("manual_compaction", %tenant_id, %timeline_id)) + .await } // Run checkpoint immediately on given timeline. async fn timeline_checkpoint_handler( request: Request, - _cancel: CancellationToken, + cancel: CancellationToken, ) -> Result, ApiError> { let tenant_id: TenantId = parse_request_param(&request, "tenant_id")?; let timeline_id: TimelineId = parse_request_param(&request, "timeline_id")?; @@ -1031,13 +1029,13 @@ async fn timeline_checkpoint_handler( .await .map_err(ApiError::InternalServerError)?; timeline - .compact(&ctx) + .compact(&cancel, &ctx) .await .map_err(ApiError::InternalServerError)?; json_response(StatusCode::OK, ()) } - .instrument(info_span!("manual_checkpoint", tenant_id = %tenant_id, timeline_id = %timeline_id)) + .instrument(info_span!("manual_checkpoint", %tenant_id, %timeline_id)) .await } diff --git a/pageserver/src/task_mgr.rs b/pageserver/src/task_mgr.rs index f3a4ce6db7..3c7a1115df 100644 --- a/pageserver/src/task_mgr.rs +++ b/pageserver/src/task_mgr.rs @@ -130,11 +130,25 @@ pub static WALRECEIVER_RUNTIME: Lazy = Lazy::new(|| { pub static BACKGROUND_RUNTIME: Lazy = Lazy::new(|| { tokio::runtime::Builder::new_multi_thread() .thread_name("background op worker") + // if you change the number of worker threads please change the constant below .enable_all() .build() .expect("Failed to create background op runtime") }); +pub(crate) static BACKGROUND_RUNTIME_WORKER_THREADS: Lazy = Lazy::new(|| { + // force init and thus panics + let _ = BACKGROUND_RUNTIME.handle(); + // replicates tokio-1.28.1::loom::sys::num_cpus which is not available publicly + // tokio would had already panicked for parsing errors or NotUnicode + // + // this will be wrong if any of the runtimes gets their worker threads configured to something + // else, but that has not been needed in a long time. + std::env::var("TOKIO_WORKER_THREADS") + .map(|s| s.parse::().unwrap()) + .unwrap_or_else(|_e| usize::max(1, num_cpus::get())) +}); + #[derive(Debug, Clone, Copy)] pub struct PageserverTaskId(u64); @@ -545,7 +559,7 @@ pub fn current_task_id() -> Option { pub async fn shutdown_watcher() { let token = SHUTDOWN_TOKEN .try_with(|t| t.clone()) - .expect("shutdown_requested() called in an unexpected task or thread"); + .expect("shutdown_watcher() called in an unexpected task or thread"); token.cancelled().await; } diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 81dce84b04..3bf88f2729 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -20,6 +20,7 @@ use storage_broker::BrokerClientChannel; use tokio::sync::watch; use tokio::sync::OwnedMutexGuard; use tokio::task::JoinSet; +use tokio_util::sync::CancellationToken; use tracing::*; use utils::completion; use utils::crashsafe::path_with_suffix_extension; @@ -1335,7 +1336,11 @@ impl Tenant { /// This function is periodically called by compactor task. /// Also it can be explicitly requested per timeline through page server /// api's 'compact' command. - pub async fn compaction_iteration(&self, ctx: &RequestContext) -> anyhow::Result<()> { + pub async fn compaction_iteration( + &self, + cancel: &CancellationToken, + ctx: &RequestContext, + ) -> anyhow::Result<()> { anyhow::ensure!( self.is_active(), "Cannot run compaction iteration on inactive tenant" @@ -1363,7 +1368,7 @@ impl Tenant { for (timeline_id, timeline) in &timelines_to_compact { timeline - .compact(ctx) + .compact(cancel, ctx) .instrument(info_span!("compact_timeline", %timeline_id)) .await?; } @@ -3449,6 +3454,7 @@ mod tests { use hex_literal::hex; use once_cell::sync::Lazy; use rand::{thread_rng, Rng}; + use tokio_util::sync::CancellationToken; static TEST_KEY: Lazy = Lazy::new(|| Key::from_slice(&hex!("112222222233333333444444445500000001"))); @@ -3970,7 +3976,7 @@ mod tests { drop(writer); tline.freeze_and_flush().await?; - tline.compact(&ctx).await?; + tline.compact(&CancellationToken::new(), &ctx).await?; let writer = tline.writer().await; writer @@ -3980,7 +3986,7 @@ mod tests { drop(writer); tline.freeze_and_flush().await?; - tline.compact(&ctx).await?; + tline.compact(&CancellationToken::new(), &ctx).await?; let writer = tline.writer().await; writer @@ -3990,7 +3996,7 @@ mod tests { drop(writer); tline.freeze_and_flush().await?; - tline.compact(&ctx).await?; + tline.compact(&CancellationToken::new(), &ctx).await?; let writer = tline.writer().await; writer @@ -4000,7 +4006,7 @@ mod tests { drop(writer); tline.freeze_and_flush().await?; - tline.compact(&ctx).await?; + tline.compact(&CancellationToken::new(), &ctx).await?; assert_eq!( tline.get(*TEST_KEY, Lsn(0x10), &ctx).await?, @@ -4069,7 +4075,7 @@ mod tests { .update_gc_info(Vec::new(), cutoff, Duration::ZERO, &ctx) .await?; tline.freeze_and_flush().await?; - tline.compact(&ctx).await?; + tline.compact(&CancellationToken::new(), &ctx).await?; tline.gc().await?; } @@ -4146,7 +4152,7 @@ mod tests { .update_gc_info(Vec::new(), cutoff, Duration::ZERO, &ctx) .await?; tline.freeze_and_flush().await?; - tline.compact(&ctx).await?; + tline.compact(&CancellationToken::new(), &ctx).await?; tline.gc().await?; } @@ -4234,7 +4240,7 @@ mod tests { .update_gc_info(Vec::new(), cutoff, Duration::ZERO, &ctx) .await?; tline.freeze_and_flush().await?; - tline.compact(&ctx).await?; + tline.compact(&CancellationToken::new(), &ctx).await?; tline.gc().await?; } diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs index 4b97871f35..eeb84caf13 100644 --- a/pageserver/src/tenant/mgr.rs +++ b/pageserver/src/tenant/mgr.rs @@ -768,55 +768,6 @@ pub async fn immediate_gc( Ok(wait_task_done) } -pub async fn immediate_compact( - tenant_id: TenantId, - timeline_id: TimelineId, - ctx: &RequestContext, -) -> Result>, ApiError> { - let guard = TENANTS.read().await; - - let tenant = guard - .get(&tenant_id) - .map(Arc::clone) - .with_context(|| format!("tenant {tenant_id}")) - .map_err(|e| ApiError::NotFound(e.into()))?; - - let timeline = tenant - .get_timeline(timeline_id, true) - .map_err(|e| ApiError::NotFound(e.into()))?; - - // Run in task_mgr to avoid race with tenant_detach operation - let ctx = ctx.detached_child(TaskKind::Compaction, DownloadBehavior::Download); - let (task_done, wait_task_done) = tokio::sync::oneshot::channel(); - task_mgr::spawn( - &tokio::runtime::Handle::current(), - TaskKind::Compaction, - Some(tenant_id), - Some(timeline_id), - &format!( - "timeline_compact_handler compaction run for tenant {tenant_id} timeline {timeline_id}" - ), - false, - async move { - let result = timeline - .compact(&ctx) - .instrument(info_span!("manual_compact", %tenant_id, %timeline_id)) - .await; - - match task_done.send(result) { - Ok(_) => (), - Err(result) => error!("failed to send compaction result: {result:?}"), - } - Ok(()) - }, - ); - - // drop the guard until after we've spawned the task so that timeline shutdown will wait for the task - drop(guard); - - Ok(wait_task_done) -} - #[cfg(test)] mod tests { use std::collections::HashMap; diff --git a/pageserver/src/tenant/tasks.rs b/pageserver/src/tenant/tasks.rs index 622ae371a4..a758d4da23 100644 --- a/pageserver/src/tenant/tasks.rs +++ b/pageserver/src/tenant/tasks.rs @@ -111,7 +111,7 @@ async fn compaction_loop(tenant: Arc, cancel: CancellationToken) { Duration::from_secs(10) } else { // Run compaction - if let Err(e) = tenant.compaction_iteration(&ctx).await { + if let Err(e) = tenant.compaction_iteration(&cancel, &ctx).await { error!("Compaction failed, retrying in {:?}: {e:?}", wait_duration); wait_duration } else { diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 3413282c5d..a7daf09021 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -611,9 +611,46 @@ impl Timeline { } /// Outermost timeline compaction operation; downloads needed layers. - pub async fn compact(self: &Arc, ctx: &RequestContext) -> anyhow::Result<()> { + pub async fn compact( + self: &Arc, + cancel: &CancellationToken, + ctx: &RequestContext, + ) -> anyhow::Result<()> { const ROUNDS: usize = 2; + static CONCURRENT_COMPACTIONS: once_cell::sync::Lazy = + once_cell::sync::Lazy::new(|| { + let total_threads = *task_mgr::BACKGROUND_RUNTIME_WORKER_THREADS; + let permits = usize::max( + 1, + // while a lot of the work is done on spawn_blocking, we still do + // repartitioning in the async context. this should give leave us some workers + // unblocked to be blocked on other work, hopefully easing any outside visible + // effects of restarts. + // + // 6/8 is a guess; previously we ran with unlimited 8 and more from + // spawn_blocking. + (total_threads * 3).checked_div(4).unwrap_or(0), + ); + assert_ne!(permits, 0, "we will not be adding in permits later"); + assert!( + permits < total_threads, + "need threads avail for shorter work" + ); + tokio::sync::Semaphore::new(permits) + }); + + // this wait probably never needs any "long time spent" logging, because we already nag if + // compaction task goes over it's period (20s) which is quite often in production. + let _permit = tokio::select! { + permit = CONCURRENT_COMPACTIONS.acquire() => { + permit + }, + _ = cancel.cancelled() => { + return Ok(()); + } + }; + let last_record_lsn = self.get_last_record_lsn(); // Last record Lsn could be zero in case the timeline was just created @@ -671,11 +708,9 @@ impl Timeline { let mut failed = 0; - let mut cancelled = pin!(task_mgr::shutdown_watcher()); - loop { tokio::select! { - _ = &mut cancelled => anyhow::bail!("Cancelled while downloading remote layers"), + _ = cancel.cancelled() => anyhow::bail!("Cancelled while downloading remote layers"), res = downloads.next() => { match res { Some(Ok(())) => {}, From 77a68326c5bf957e905654c69642da3caca649aa Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Tue, 25 Jul 2023 11:15:54 +0300 Subject: [PATCH 22/23] Thin out TenantState metric, keep set of broken tenants (#4796) We currently have a timeseries for each of the tenants in different states. We only want this for Broken. Other states could be counters. Fix this by making the `pageserver_tenant_states_count` a counter without a `tenant_id` and add a `pageserver_broken_tenants_count` which has a `tenant_id` label, each broken tenant being 1. --- pageserver/src/metrics.rs | 22 ++++-- pageserver/src/tenant.rs | 56 +++++++++------ pageserver/src/tenant/timeline.rs | 4 +- test_runner/fixtures/metrics.py | 3 +- test_runner/regress/test_tenant_detach.py | 88 +++++++++++++++++++++++ test_runner/regress/test_tenants.py | 4 +- 6 files changed, 145 insertions(+), 32 deletions(-) diff --git a/pageserver/src/metrics.rs b/pageserver/src/metrics.rs index bdd79711ec..c233f36da6 100644 --- a/pageserver/src/metrics.rs +++ b/pageserver/src/metrics.rs @@ -6,7 +6,6 @@ use metrics::{ IntCounterVec, IntGauge, IntGaugeVec, UIntGauge, UIntGaugeVec, }; use once_cell::sync::Lazy; -use pageserver_api::models::TenantState; use strum::VariantNames; use strum_macros::{EnumVariantNames, IntoStaticStr}; use utils::id::{TenantId, TimelineId}; @@ -306,11 +305,24 @@ static CURRENT_LOGICAL_SIZE: Lazy = Lazy::new(|| { .expect("failed to define current logical size metric") }); -pub static TENANT_STATE_METRIC: Lazy = Lazy::new(|| { +pub(crate) static TENANT_STATE_METRIC: Lazy = Lazy::new(|| { register_uint_gauge_vec!( "pageserver_tenant_states_count", "Count of tenants per state", - &["tenant_id", "state"] + &["state"] + ) + .expect("Failed to register pageserver_tenant_states_count metric") +}); + +/// A set of broken tenants. +/// +/// These are expected to be so rare that a set is fine. Set as in a new timeseries per each broken +/// tenant. +pub(crate) static BROKEN_TENANTS_SET: Lazy = Lazy::new(|| { + register_uint_gauge_vec!( + "pageserver_broken_tenants_count", + "Set of broken tenants", + &["tenant_id"] ) .expect("Failed to register pageserver_tenant_states_count metric") }); @@ -1023,9 +1035,7 @@ impl Drop for TimelineMetrics { pub fn remove_tenant_metrics(tenant_id: &TenantId) { let tid = tenant_id.to_string(); let _ = TENANT_SYNTHETIC_SIZE_METRIC.remove_label_values(&[&tid]); - for state in TenantState::VARIANTS { - let _ = TENANT_STATE_METRIC.remove_label_values(&[&tid, state]); - } + // we leave the BROKEN_TENANTS_SET entry if any } use futures::Future; diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 3bf88f2729..654f5f133f 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -2199,28 +2199,44 @@ impl Tenant { let (state, mut rx) = watch::channel(state); tokio::spawn(async move { - let mut current_state: &'static str = From::from(&*rx.borrow_and_update()); let tid = tenant_id.to_string(); - TENANT_STATE_METRIC - .with_label_values(&[&tid, current_state]) - .inc(); - loop { - match rx.changed().await { - Ok(()) => { - let new_state: &'static str = From::from(&*rx.borrow_and_update()); - TENANT_STATE_METRIC - .with_label_values(&[&tid, current_state]) - .dec(); - TENANT_STATE_METRIC - .with_label_values(&[&tid, new_state]) - .inc(); - current_state = new_state; - } - Err(_sender_dropped_error) => { - info!("Tenant dropped the state updates sender, quitting waiting for tenant state change"); - return; - } + fn inspect_state(state: &TenantState) -> ([&'static str; 1], bool) { + ([state.into()], matches!(state, TenantState::Broken { .. })) + } + + let mut tuple = inspect_state(&rx.borrow_and_update()); + + let is_broken = tuple.1; + if !is_broken { + // the tenant might be ignored and reloaded, so first remove any previous set + // element. it most likely has already been scraped, as these are manual operations + // right now. most likely we will add it back very soon. + drop(crate::metrics::BROKEN_TENANTS_SET.remove_label_values(&[&tid])); + } + + loop { + let labels = &tuple.0; + let current = TENANT_STATE_METRIC.with_label_values(labels); + current.inc(); + + if rx.changed().await.is_err() { + // tenant has been dropped; decrement the counter because a tenant with that + // state is no longer in tenant map, but allow any broken set item to exist + // still. + current.dec(); + break; + } + + current.dec(); + tuple = inspect_state(&rx.borrow_and_update()); + + let is_broken = tuple.1; + if is_broken { + // insert the tenant_id (back) into the set + crate::metrics::BROKEN_TENANTS_SET + .with_label_values(&[&tid]) + .inc(); } } }); diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index a7daf09021..c663a4f9ad 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -334,7 +334,7 @@ pub struct GcInfo { #[derive(thiserror::Error)] pub enum PageReconstructError { #[error(transparent)] - Other(#[from] anyhow::Error), // source and Display delegate to anyhow::Error + Other(#[from] anyhow::Error), /// The operation would require downloading a layer that is missing locally. NeedsDownload(TenantTimelineId, LayerFileName), @@ -925,7 +925,7 @@ impl Timeline { new_state, TimelineState::Stopping | TimelineState::Broken { .. } ) { - // drop the copmletion guard, if any; it might be holding off the completion + // drop the completion guard, if any; it might be holding off the completion // forever needlessly self.initial_logical_size_attempt .lock() diff --git a/test_runner/fixtures/metrics.py b/test_runner/fixtures/metrics.py index ca468cc02c..52d32df43e 100644 --- a/test_runner/fixtures/metrics.py +++ b/test_runner/fixtures/metrics.py @@ -76,6 +76,7 @@ PAGESERVER_GLOBAL_METRICS: Tuple[str, ...] = ( *histogram("pageserver_remote_operation_seconds"), *histogram("pageserver_remote_timeline_client_calls_started"), *histogram("pageserver_io_operations_seconds"), + "pageserver_tenant_states_count", ) PAGESERVER_PER_TENANT_METRICS: Tuple[str, ...] = ( @@ -90,8 +91,8 @@ PAGESERVER_PER_TENANT_METRICS: Tuple[str, ...] = ( "pageserver_storage_operations_seconds_sum_total", "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, + # pageserver_broken_tenants_count is a leaked "metric" which is "cleared" on restart or reload ) diff --git a/test_runner/regress/test_tenant_detach.py b/test_runner/regress/test_tenant_detach.py index 6803f6dbb1..77f93dbd92 100644 --- a/test_runner/regress/test_tenant_detach.py +++ b/test_runner/regress/test_tenant_detach.py @@ -2,6 +2,7 @@ import asyncio import random import time from threading import Thread +from typing import List, Optional import asyncpg import pytest @@ -21,6 +22,7 @@ from fixtures.pageserver.utils import ( ) from fixtures.types import Lsn, TenantId, TimelineId from fixtures.utils import query_scalar, wait_until +from prometheus_client.samples import Sample def do_gc_target( @@ -854,3 +856,89 @@ def ensure_test_data(data_id: int, data: str, endpoint: Endpoint): assert ( query_scalar(cur, f"SELECT secret FROM test WHERE id = {data_id};") == data ), "Should have timeline data back" + + +@pytest.mark.parametrize("remote_storage_kind", [RemoteStorageKind.LOCAL_FS]) +def test_metrics_while_ignoring_broken_tenant_and_reloading( + neon_env_builder: NeonEnvBuilder, + remote_storage_kind: RemoteStorageKind, +): + neon_env_builder.enable_remote_storage( + remote_storage_kind=remote_storage_kind, + test_name="test_metrics_while_ignoring_broken_tenant_and_reloading", + ) + + env = neon_env_builder.init_start() + + client = env.pageserver.http_client() + env.pageserver.allowed_errors.append( + r".* Changing Active tenant to Broken state, reason: broken from test" + ) + + def only_int(samples: List[Sample]) -> Optional[int]: + if len(samples) == 1: + return int(samples[0].value) + assert len(samples) == 0 + return None + + wait_until_tenant_state(client, env.initial_tenant, "Active", 10, 0.5) + + client.tenant_break(env.initial_tenant) + + found_broken = False + active, broken, broken_set = ([], [], []) + for _ in range(10): + m = client.get_metrics() + active = m.query_all("pageserver_tenant_states_count", {"state": "Active"}) + broken = m.query_all("pageserver_tenant_states_count", {"state": "Broken"}) + broken_set = m.query_all( + "pageserver_broken_tenants_count", {"tenant_id": str(env.initial_tenant)} + ) + found_broken = only_int(active) == 0 and only_int(broken) == 1 and only_int(broken_set) == 1 + + if found_broken: + break + log.info(f"active: {active}, broken: {broken}, broken_set: {broken_set}") + time.sleep(0.5) + assert ( + found_broken + ), f"tenant shows up as broken; active={active}, broken={broken}, broken_set={broken_set}" + + client.tenant_ignore(env.initial_tenant) + + found_broken = False + broken, broken_set = ([], []) + for _ in range(10): + m = client.get_metrics() + broken = m.query_all("pageserver_tenant_states_count", {"state": "Broken"}) + broken_set = m.query_all( + "pageserver_broken_tenants_count", {"tenant_id": str(env.initial_tenant)} + ) + found_broken = only_int(broken) == 0 and only_int(broken_set) == 1 + + if found_broken: + break + time.sleep(0.5) + assert ( + found_broken + ), f"broken should still be in set, but it is not in the tenant state count: broken={broken}, broken_set={broken_set}" + + client.tenant_load(env.initial_tenant) + + found_active = False + active, broken_set = ([], []) + for _ in range(10): + m = client.get_metrics() + active = m.query_all("pageserver_tenant_states_count", {"state": "Active"}) + broken_set = m.query_all( + "pageserver_broken_tenants_count", {"tenant_id": str(env.initial_tenant)} + ) + found_active = only_int(active) == 1 and len(broken_set) == 0 + + if found_active: + break + time.sleep(0.5) + + assert ( + found_active + ), f"reloaded tenant should be active, and broken tenant set item removed: active={active}, broken_set={broken_set}" diff --git a/test_runner/regress/test_tenants.py b/test_runner/regress/test_tenants.py index 35f7941bda..e327910138 100644 --- a/test_runner/regress/test_tenants.py +++ b/test_runner/regress/test_tenants.py @@ -383,10 +383,8 @@ def test_pageserver_with_empty_tenants( ps_metrics = client.get_metrics() broken_tenants_metric_filter = { "tenant_id": str(tenant_without_timelines_dir), - "state": "Broken", } active_tenants_metric_filter = { - "tenant_id": str(tenant_with_empty_timelines), "state": "Active", } @@ -402,7 +400,7 @@ def test_pageserver_with_empty_tenants( tenant_broken_count = int( ps_metrics.query_one( - "pageserver_tenant_states_count", filter=broken_tenants_metric_filter + "pageserver_broken_tenants_count", filter=broken_tenants_metric_filter ).value ) From f9214771b4968f4a4d12e54c4df61da944998cc8 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Tue, 25 Jul 2023 12:31:24 +0300 Subject: [PATCH 23/23] fix: count broken tenant more correct (#4800) count only once; on startup create the counter right away because we will not observe any changes later. small, probably never reachable from outside fix for #4796. --- pageserver/src/tenant.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 654f5f133f..0b1183fc50 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -2208,12 +2208,20 @@ impl Tenant { let mut tuple = inspect_state(&rx.borrow_and_update()); let is_broken = tuple.1; - if !is_broken { + let mut counted_broken = if !is_broken { // the tenant might be ignored and reloaded, so first remove any previous set // element. it most likely has already been scraped, as these are manual operations // right now. most likely we will add it back very soon. drop(crate::metrics::BROKEN_TENANTS_SET.remove_label_values(&[&tid])); - } + false + } else { + // add the id to the set right away, there should not be any updates on the channel + // after + crate::metrics::BROKEN_TENANTS_SET + .with_label_values(&[&tid]) + .set(1); + true + }; loop { let labels = &tuple.0; @@ -2232,7 +2240,8 @@ impl Tenant { tuple = inspect_state(&rx.borrow_and_update()); let is_broken = tuple.1; - if is_broken { + if is_broken && !counted_broken { + counted_broken = true; // insert the tenant_id (back) into the set crate::metrics::BROKEN_TENANTS_SET .with_label_values(&[&tid])