From bd722f24ae55cdbfa0588e043939007b925f9884 Mon Sep 17 00:00:00 2001 From: Anastasia Lubennikova Date: Tue, 26 Nov 2024 12:36:27 +0000 Subject: [PATCH] Update compute_installed_extensions metric: add owned_by_superuser field to filter out system extensions. While on it, also correct related code: - fix the metric setting: use set() instead of inc() in a loop. inc() is not idempotent and can lead to incorrect results if the function called multiple times. Currently it is only called at compute start, but this will change soon. - fix the return type of the installed_extensions endpoint to match the metric. Currently it is only used in the test. --- compute_tools/src/http/openapi_spec.yaml | 6 +- compute_tools/src/installed_extensions.rs | 55 ++++++++++++------- libs/compute_api/src/responses.rs | 4 +- .../regress/test_installed_extensions.py | 28 +++++----- 4 files changed, 56 insertions(+), 37 deletions(-) diff --git a/compute_tools/src/http/openapi_spec.yaml b/compute_tools/src/http/openapi_spec.yaml index 7b9a62c545..24a67cac71 100644 --- a/compute_tools/src/http/openapi_spec.yaml +++ b/compute_tools/src/http/openapi_spec.yaml @@ -537,12 +537,14 @@ components: properties: extname: type: string - versions: - type: array + version: + type: string items: type: string n_databases: type: integer + owned_by_superuser: + type: integer SetRoleGrantsRequest: type: object diff --git a/compute_tools/src/installed_extensions.rs b/compute_tools/src/installed_extensions.rs index 5f62f08858..0ab259ddf1 100644 --- a/compute_tools/src/installed_extensions.rs +++ b/compute_tools/src/installed_extensions.rs @@ -1,7 +1,6 @@ use compute_api::responses::{InstalledExtension, InstalledExtensions}; use metrics::proto::MetricFamily; use std::collections::HashMap; -use std::collections::HashSet; use anyhow::Result; use postgres::{Client, NoTls}; @@ -38,61 +37,77 @@ fn list_dbs(client: &mut Client) -> Result> { /// Connect to every database (see list_dbs above) and get the list of installed extensions. /// /// Same extension can be installed in multiple databases with different versions, -/// we only keep the highest and lowest version across all databases. +/// so we report a separate metric (number of databases where it is installed) +/// for each extension version. pub fn get_installed_extensions(mut conf: postgres::config::Config) -> Result { conf.application_name("compute_ctl:get_installed_extensions"); let mut client = conf.connect(NoTls)?; - let databases: Vec = list_dbs(&mut client)?; - let mut extensions_map: HashMap = HashMap::new(); + let mut extensions_map: HashMap<(String, String, String), InstalledExtension> = HashMap::new(); for db in databases.iter() { conf.dbname(db); let mut db_client = conf.connect(NoTls)?; - let extensions: Vec<(String, String)> = db_client + let extensions: Vec<(String, String, i32)> = db_client .query( - "SELECT extname, extversion FROM pg_catalog.pg_extension;", + "SELECT extname, extversion, extowner::integer FROM pg_catalog.pg_extension", &[], )? .iter() - .map(|row| (row.get("extname"), row.get("extversion"))) + .map(|row| { + ( + row.get("extname"), + row.get("extversion"), + row.get("extowner"), + ) + }) .collect(); - for (extname, v) in extensions.iter() { + for (extname, v, extowner) in extensions.iter() { let version = v.to_string(); - // increment the number of databases where the version of extension is installed - INSTALLED_EXTENSIONS - .with_label_values(&[extname, &version]) - .inc(); + // check if the extension is owned by superuser + // 10 is the oid of superuser + let owned_by_superuser = if *extowner == 10 { "1" } else { "0" }; extensions_map - .entry(extname.to_string()) + .entry(( + extname.to_string(), + version.clone(), + owned_by_superuser.to_string(), + )) .and_modify(|e| { - e.versions.insert(version.clone()); // count the number of databases where the extension is installed e.n_databases += 1; }) .or_insert(InstalledExtension { extname: extname.to_string(), - versions: HashSet::from([version.clone()]), + version: version.clone(), n_databases: 1, + owned_by_superuser: owned_by_superuser.to_string(), }); } } - let res = InstalledExtensions { - extensions: extensions_map.into_values().collect(), - }; + for (key, ext) in extensions_map.iter() { + let (extname, version, owned_by_superuser) = key; + let n_databases = ext.n_databases as u64; - Ok(res) + INSTALLED_EXTENSIONS + .with_label_values(&[extname, version, owned_by_superuser]) + .set(n_databases); + } + + Ok(InstalledExtensions { + extensions: extensions_map.into_values().collect(), + }) } static INSTALLED_EXTENSIONS: Lazy = Lazy::new(|| { register_uint_gauge_vec!( "compute_installed_extensions", "Number of databases where the version of extension is installed", - &["extension_name", "version"] + &["extension_name", "version", "owned_by_superuser"] ) .expect("failed to define a metric") }); diff --git a/libs/compute_api/src/responses.rs b/libs/compute_api/src/responses.rs index 79234be720..0d65f6a38d 100644 --- a/libs/compute_api/src/responses.rs +++ b/libs/compute_api/src/responses.rs @@ -1,6 +1,5 @@ //! Structs representing the JSON formats used in the compute_ctl's HTTP API. -use std::collections::HashSet; use std::fmt::Display; use chrono::{DateTime, Utc}; @@ -163,8 +162,9 @@ pub enum ControlPlaneComputeStatus { #[derive(Clone, Debug, Default, Serialize)] pub struct InstalledExtension { pub extname: String, - pub versions: HashSet, + pub version: String, pub n_databases: u32, // Number of databases using this extension + pub owned_by_superuser: String, } #[derive(Clone, Debug, Default, Serialize)] diff --git a/test_runner/regress/test_installed_extensions.py b/test_runner/regress/test_installed_extensions.py index 04ccec5875..4e51e7e10c 100644 --- a/test_runner/regress/test_installed_extensions.py +++ b/test_runner/regress/test_installed_extensions.py @@ -30,7 +30,7 @@ def test_installed_extensions(neon_simple_env: NeonEnv): info("Extensions: %s", res["extensions"]) # 'plpgsql' is a default extension that is always installed. assert any( - ext["extname"] == "plpgsql" and ext["versions"] == ["1.0"] for ext in res["extensions"] + ext["extname"] == "plpgsql" and ext["version"] == "1.0" for ext in res["extensions"] ), "The 'plpgsql' extension is missing" # check that the neon_test_utils extension is not installed @@ -63,7 +63,7 @@ def test_installed_extensions(neon_simple_env: NeonEnv): # and has the expected version assert any( ext["extname"] == "neon_test_utils" - and ext["versions"] == [neon_test_utils_version] + and ext["version"] == neon_test_utils_version and ext["n_databases"] == 1 for ext in res["extensions"] ) @@ -75,9 +75,8 @@ def test_installed_extensions(neon_simple_env: NeonEnv): # check that the neon extension is installed and has expected versions for ext in res["extensions"]: if ext["extname"] == "neon": - assert ext["n_databases"] == 2 - ext["versions"].sort() - assert ext["versions"] == ["1.1", "1.2"] + assert ext["version"] in ["1.1", "1.2"] + assert ext["n_databases"] == 1 with pg_conn.cursor() as cur: cur.execute("ALTER EXTENSION neon UPDATE TO '1.3'") @@ -90,9 +89,8 @@ def test_installed_extensions(neon_simple_env: NeonEnv): # check that the neon_test_utils extension is updated for ext in res["extensions"]: if ext["extname"] == "neon": - assert ext["n_databases"] == 2 - ext["versions"].sort() - assert ext["versions"] == ["1.2", "1.3"] + assert ext["version"] in ["1.2", "1.3"] + assert ext["n_databases"] == 1 # check that /metrics endpoint is available # ensure that we see the metric before and after restart @@ -100,13 +98,15 @@ def test_installed_extensions(neon_simple_env: NeonEnv): info("Metrics: %s", res) m = parse_metrics(res) neon_m = m.query_all( - "compute_installed_extensions", {"extension_name": "neon", "version": "1.2"} + "compute_installed_extensions", + {"extension_name": "neon", "version": "1.2", "owned_by_superuser": "1"}, ) assert len(neon_m) == 1 for sample in neon_m: - assert sample.value == 2 + assert sample.value == 1 neon_m = m.query_all( - "compute_installed_extensions", {"extension_name": "neon", "version": "1.3"} + "compute_installed_extensions", + {"extension_name": "neon", "version": "1.3", "owned_by_superuser": "1"}, ) assert len(neon_m) == 1 for sample in neon_m: @@ -138,14 +138,16 @@ def test_installed_extensions(neon_simple_env: NeonEnv): info("After restart metrics: %s", res) m = parse_metrics(res) neon_m = m.query_all( - "compute_installed_extensions", {"extension_name": "neon", "version": "1.2"} + "compute_installed_extensions", + {"extension_name": "neon", "version": "1.2", "owned_by_superuser": "1"}, ) assert len(neon_m) == 1 for sample in neon_m: assert sample.value == 1 neon_m = m.query_all( - "compute_installed_extensions", {"extension_name": "neon", "version": "1.3"} + "compute_installed_extensions", + {"extension_name": "neon", "version": "1.3", "owned_by_superuser": "1"}, ) assert len(neon_m) == 1 for sample in neon_m: