diff --git a/Dockerfile.compute-node b/Dockerfile.compute-node index 14ba1b5b9a..908460018f 100644 --- a/Dockerfile.compute-node +++ b/Dockerfile.compute-node @@ -883,8 +883,10 @@ FROM debian:bullseye-slim RUN mkdir /var/db && useradd -m -d /var/db/postgres postgres && \ echo "postgres:test_console_pass" | chpasswd && \ mkdir /var/db/postgres/compute && mkdir /var/db/postgres/specs && \ + mkdir /var/db/postgres/pgbouncer && \ chown -R postgres:postgres /var/db/postgres && \ chmod 0750 /var/db/postgres/compute && \ + chmod 0750 /var/db/postgres/pgbouncer && \ echo '/usr/local/lib' >> /etc/ld.so.conf && /sbin/ldconfig && \ # create folder for file cache mkdir -p -m 777 /neon/cache diff --git a/compute_tools/src/bin/compute_ctl.rs b/compute_tools/src/bin/compute_ctl.rs index 47a00277dd..a7e10d0aee 100644 --- a/compute_tools/src/bin/compute_ctl.rs +++ b/compute_tools/src/bin/compute_ctl.rs @@ -32,8 +32,6 @@ //! -S /var/db/postgres/specs/current.json \ //! -b /usr/local/bin/postgres \ //! -r http://pg-ext-s3-gateway \ -//! --pgbouncer-connstr 'host=localhost port=6432 dbname=pgbouncer user=cloud_admin sslmode=disable' -//! --pgbouncer-ini-path /etc/pgbouncer.ini \ //! ``` //! use std::collections::HashMap; @@ -112,9 +110,6 @@ fn main() -> Result<()> { let spec_json = matches.get_one::("spec"); let spec_path = matches.get_one::("spec-path"); - let pgbouncer_connstr = matches.get_one::("pgbouncer-connstr"); - let pgbouncer_ini_path = matches.get_one::("pgbouncer-ini-path"); - // Extract OpenTelemetry context for the startup actions from the // TRACEPARENT and TRACESTATE env variables, and attach it to the current // tracing context. @@ -225,8 +220,6 @@ fn main() -> Result<()> { ext_remote_storage: ext_remote_storage.map(|s| s.to_string()), ext_download_progress: RwLock::new(HashMap::new()), build_tag, - pgbouncer_connstr: pgbouncer_connstr.map(|s| s.to_string()), - pgbouncer_ini_path: pgbouncer_ini_path.map(|s| s.to_string()), }; let compute = Arc::new(compute_node); @@ -523,23 +516,6 @@ fn cli() -> clap::Command { ) .value_name("FILECACHE_CONNSTR"), ) - .arg( - Arg::new("pgbouncer-connstr") - .long("pgbouncer-connstr") - .default_value( - "host=localhost port=6432 dbname=pgbouncer user=cloud_admin sslmode=disable", - ) - .value_name("PGBOUNCER_CONNSTR"), - ) - .arg( - Arg::new("pgbouncer-ini-path") - .long("pgbouncer-ini-path") - // Note: this doesn't match current path for pgbouncer.ini. - // Until we fix it, we need to pass the path explicitly - // or this will be effectively no-op. - .default_value("/etc/pgbouncer.ini") - .value_name("PGBOUNCER_INI_PATH"), - ) } /// When compute_ctl is killed, send also termination signal to sync-safekeepers diff --git a/compute_tools/src/compute.rs b/compute_tools/src/compute.rs index deea2375c2..5f5363105c 100644 --- a/compute_tools/src/compute.rs +++ b/compute_tools/src/compute.rs @@ -71,10 +71,6 @@ pub struct ComputeNode { // key: ext_archive_name, value: started download time, download_completed? pub ext_download_progress: RwLock, bool)>>, pub build_tag: String, - // connection string to pgbouncer to change settings - pub pgbouncer_connstr: Option, - // path to pgbouncer.ini to change settings - pub pgbouncer_ini_path: Option, } // store some metrics about download size that might impact startup time @@ -769,8 +765,8 @@ impl ComputeNode { pub fn reconfigure(&self) -> Result<()> { let spec = self.state.lock().unwrap().pspec.clone().unwrap().spec; - if let Some(connstr) = &self.pgbouncer_connstr { - info!("tuning pgbouncer with connstr: {:?}", connstr); + if let Some(ref pgbouncer_settings) = spec.pgbouncer_settings { + info!("tuning pgbouncer"); let rt = tokio::runtime::Builder::new_current_thread() .enable_all() @@ -779,15 +775,9 @@ impl ComputeNode { // Spawn a thread to do the tuning, // so that we don't block the main thread that starts Postgres. - let pgbouncer_settings = spec.pgbouncer_settings.clone(); - let connstr_clone = connstr.clone(); - let pgbouncer_ini_path = self.pgbouncer_ini_path.clone(); + let pgbouncer_settings = pgbouncer_settings.clone(); let _handle = thread::spawn(move || { - let res = rt.block_on(tune_pgbouncer( - pgbouncer_settings, - &connstr_clone, - pgbouncer_ini_path, - )); + let res = rt.block_on(tune_pgbouncer(pgbouncer_settings)); if let Err(err) = res { error!("error while tuning pgbouncer: {err:?}"); } @@ -852,8 +842,8 @@ impl ComputeNode { ); // tune pgbouncer - if let Some(connstr) = &self.pgbouncer_connstr { - info!("tuning pgbouncer with connstr: {:?}", connstr); + if let Some(pgbouncer_settings) = &pspec.spec.pgbouncer_settings { + info!("tuning pgbouncer"); let rt = tokio::runtime::Builder::new_current_thread() .enable_all() @@ -862,15 +852,9 @@ impl ComputeNode { // Spawn a thread to do the tuning, // so that we don't block the main thread that starts Postgres. - let pgbouncer_settings = pspec.spec.pgbouncer_settings.clone(); - let connstr_clone = connstr.clone(); - let pgbouncer_ini_path = self.pgbouncer_ini_path.clone(); + let pgbouncer_settings = pgbouncer_settings.clone(); let _handle = thread::spawn(move || { - let res = rt.block_on(tune_pgbouncer( - pgbouncer_settings, - &connstr_clone, - pgbouncer_ini_path, - )); + let res = rt.block_on(tune_pgbouncer(pgbouncer_settings)); if let Err(err) = res { error!("error while tuning pgbouncer: {err:?}"); } diff --git a/compute_tools/src/pg_helpers.rs b/compute_tools/src/pg_helpers.rs index b53ac3f02c..ce704385c6 100644 --- a/compute_tools/src/pg_helpers.rs +++ b/compute_tools/src/pg_helpers.rs @@ -366,7 +366,7 @@ pub fn create_pgdata(pgdata: &str) -> Result<()> { } /// Update pgbouncer.ini with provided options -pub fn update_pgbouncer_ini( +fn update_pgbouncer_ini( pgbouncer_config: HashMap, pgbouncer_ini_path: &str, ) -> Result<()> { @@ -375,6 +375,10 @@ pub fn update_pgbouncer_ini( for (option_name, value) in pgbouncer_config.iter() { section.insert(option_name, value); + debug!( + "Updating pgbouncer.ini with new values {}={}", + option_name, value + ); } conf.write_to_file(pgbouncer_ini_path)?; @@ -384,49 +388,80 @@ pub fn update_pgbouncer_ini( /// Tune pgbouncer. /// 1. Apply new config using pgbouncer admin console /// 2. Add new values to pgbouncer.ini to preserve them after restart -pub async fn tune_pgbouncer( - pgbouncer_settings: Option>, - pgbouncer_connstr: &str, - pgbouncer_ini_path: Option, -) -> Result<()> { - if let Some(pgbouncer_config) = pgbouncer_settings { - // Apply new config - let connect_result = tokio_postgres::connect(pgbouncer_connstr, NoTls).await; - let (client, connection) = connect_result.unwrap(); - tokio::spawn(async move { - if let Err(e) = connection.await { - eprintln!("connection error: {}", e); +pub async fn tune_pgbouncer(pgbouncer_config: HashMap) -> Result<()> { + let pgbouncer_connstr = if std::env::var_os("AUTOSCALING").is_some() { + // for VMs use pgbouncer specific way to connect to + // pgbouncer admin console without password + // when pgbouncer is running under the same user. + "host=/tmp port=6432 dbname=pgbouncer user=pgbouncer".to_string() + } else { + // for k8s use normal connection string with password + // to connect to pgbouncer admin console + let mut pgbouncer_connstr = + "host=localhost port=6432 dbname=pgbouncer user=postgres sslmode=disable".to_string(); + if let Ok(pass) = std::env::var("PGBOUNCER_PASSWORD") { + pgbouncer_connstr.push_str(format!(" password={}", pass).as_str()); + } + pgbouncer_connstr + }; + + info!( + "Connecting to pgbouncer with connection string: {}", + pgbouncer_connstr + ); + + // connect to pgbouncer, retrying several times + // because pgbouncer may not be ready yet + let mut retries = 3; + let client = loop { + match tokio_postgres::connect(&pgbouncer_connstr, NoTls).await { + Ok((client, connection)) => { + tokio::spawn(async move { + if let Err(e) = connection.await { + eprintln!("connection error: {}", e); + } + }); + break client; } - }); + Err(e) => { + if retries == 0 { + return Err(e.into()); + } + error!("Failed to connect to pgbouncer: pgbouncer_connstr {}", e); + retries -= 1; + tokio::time::sleep(Duration::from_secs(1)).await; + } + } + }; - for (option_name, value) in pgbouncer_config.iter() { - info!( - "Applying pgbouncer setting change: {} = {}", - option_name, value + // Apply new config + for (option_name, value) in pgbouncer_config.iter() { + let query = format!("SET {}={}", option_name, value); + // keep this log line for debugging purposes + info!("Applying pgbouncer setting change: {}", query); + + if let Err(err) = client.simple_query(&query).await { + // Don't fail on error, just print it into log + error!( + "Failed to apply pgbouncer setting change: {}, {}", + query, err ); - let query = format!("SET {} = {}", option_name, value); - - let result = client.simple_query(&query).await; - - info!("Applying pgbouncer setting change: {}", query); - info!("pgbouncer setting change result: {:?}", result); - - if let Err(err) = result { - // Don't fail on error, just print it into log - error!( - "Failed to apply pgbouncer setting change: {}, {}", - query, err - ); - }; - } - - // save values to pgbouncer.ini - // so that they are preserved after pgbouncer restart - if let Some(pgbouncer_ini_path) = pgbouncer_ini_path { - update_pgbouncer_ini(pgbouncer_config, &pgbouncer_ini_path)?; - } + }; } + // save values to pgbouncer.ini + // so that they are preserved after pgbouncer restart + let pgbouncer_ini_path = if std::env::var_os("AUTOSCALING").is_some() { + // in VMs we use /etc/pgbouncer.ini + "/etc/pgbouncer.ini".to_string() + } else { + // in pods we use /var/db/postgres/pgbouncer/pgbouncer.ini + // this is a shared volume between pgbouncer and postgres containers + // FIXME: fix permissions for this file + "/var/db/postgres/pgbouncer/pgbouncer.ini".to_string() + }; + update_pgbouncer_ini(pgbouncer_config, &pgbouncer_ini_path)?; + Ok(()) } diff --git a/vm-image-spec.yaml b/vm-image-spec.yaml index 704e3721d6..bbe80ceeb1 100644 --- a/vm-image-spec.yaml +++ b/vm-image-spec.yaml @@ -6,7 +6,7 @@ commands: sysvInitAction: sysinit shell: 'cgconfigparser -l /etc/cgconfig.conf -s 1664' - name: pgbouncer - user: nobody + user: postgres sysvInitAction: respawn shell: '/usr/local/bin/pgbouncer /etc/pgbouncer.ini' - name: postgres-exporter @@ -36,7 +36,9 @@ files: max_client_conn=10000 default_pool_size=64 max_prepared_statements=0 - admin_users=cloud_admin + admin_users=postgres + unix_socket_dir=/tmp/ + unix_socket_mode=0777 - filename: cgconfig.conf content: | # Configuration for cgroups in VM compute nodes @@ -198,7 +200,7 @@ merge: | RUN set -e \ && chown postgres:postgres /etc/pgbouncer.ini \ - && chmod 0644 /etc/pgbouncer.ini \ + && chmod 0666 /etc/pgbouncer.ini \ && chmod 0644 /etc/cgconfig.conf \ && chmod 0644 /etc/sql_exporter.yml \ && chmod 0644 /etc/neon_collector.yml