From 4ee4bfc06055e9c925ed51412ede4819fb47cc16 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 19 Aug 2025 20:54:48 +0000 Subject: [PATCH 1/4] [spr] initial version Created using spr 1.3.6-beta.1 --- Cargo.lock | 3 +- cockroach-admin/types/src/lib.rs | 88 ++++++++++++++++++++++++++++++ cockroach-metrics/Cargo.toml | 1 + cockroach-metrics/src/lib.rs | 92 ++------------------------------ nexus/types/Cargo.toml | 2 +- nexus/types/src/inventory.rs | 2 +- 6 files changed, 96 insertions(+), 92 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fa352d4a6e6..e219f944585 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7098,6 +7098,7 @@ dependencies = [ "chrono", "clap", "clickhouse-admin-types", + "cockroach-admin-types", "cookie", "daft", "derive-where", @@ -7118,7 +7119,6 @@ dependencies = [ "newtype-uuid", "newtype_derive", "nexus-sled-agent-shared", - "omicron-cockroach-metrics", "omicron-common", "omicron-passwords", "omicron-test-utils", @@ -7610,6 +7610,7 @@ dependencies = [ "anyhow", "chrono", "cockroach-admin-client", + "cockroach-admin-types", "futures", "omicron-workspace-hack", "parallel-task-set", diff --git a/cockroach-admin/types/src/lib.rs b/cockroach-admin/types/src/lib.rs index e5234028deb..bef9ffc407c 100644 --- a/cockroach-admin/types/src/lib.rs +++ b/cockroach-admin/types/src/lib.rs @@ -42,9 +42,97 @@ pub enum DecommissionError { ParseRow(#[from] csv::Error), } +/// CockroachDB Node ID +/// +/// This field is stored internally as a String to avoid questions +/// about size, signedness, etc - it can be treated as an arbitrary +/// unique identifier. +#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize)] +pub struct NodeId(pub String); + +impl NodeId { + pub fn new(id: String) -> Self { + Self(id) + } + + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl std::fmt::Display for NodeId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl std::str::FromStr for NodeId { + type Err = std::convert::Infallible; + + fn from_str(s: &str) -> Result { + Ok(Self(s.to_string())) + } +} + +// When parsing the underlying NodeId, we force it to be interpreted +// as a String. Without this custom Deserialize implementation, we +// encounter parsing errors when querying endpoints which return the +// NodeId as an integer. +impl<'de> serde::Deserialize<'de> for NodeId { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + use serde::de::{Error, Visitor}; + use std::fmt; + + struct NodeIdVisitor; + + impl<'de> Visitor<'de> for NodeIdVisitor { + type Value = NodeId; + + fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { + formatter + .write_str("a string or integer representing a node ID") + } + + fn visit_str(self, value: &str) -> Result + where + E: Error, + { + Ok(NodeId(value.to_string())) + } + + fn visit_string(self, value: String) -> Result + where + E: Error, + { + Ok(NodeId(value)) + } + + fn visit_i64(self, value: i64) -> Result + where + E: Error, + { + Ok(NodeId(value.to_string())) + } + + fn visit_u64(self, value: u64) -> Result + where + E: Error, + { + Ok(NodeId(value.to_string())) + } + } + + deserializer.deserialize_any(NodeIdVisitor) + } +} + #[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize, JsonSchema)] #[serde(rename_all = "snake_case")] pub struct NodeStatus { + // TODO use NodeId pub node_id: String, pub address: SocketAddr, pub sql_address: SocketAddr, diff --git a/cockroach-metrics/Cargo.toml b/cockroach-metrics/Cargo.toml index 02d5edd2327..19e98fb810c 100644 --- a/cockroach-metrics/Cargo.toml +++ b/cockroach-metrics/Cargo.toml @@ -8,6 +8,7 @@ license = "MPL-2.0" anyhow.workspace = true chrono.workspace = true cockroach-admin-client.workspace = true +cockroach-admin-types.workspace = true futures.workspace = true parallel-task-set.workspace = true reqwest.workspace = true diff --git a/cockroach-metrics/src/lib.rs b/cockroach-metrics/src/lib.rs index fb1339c5acb..b58d60d1be4 100644 --- a/cockroach-metrics/src/lib.rs +++ b/cockroach-metrics/src/lib.rs @@ -10,6 +10,7 @@ use anyhow::{Context, Result}; use chrono::{DateTime, Utc}; use cockroach_admin_client::Client; +use cockroach_admin_types::NodeId; use futures::stream::{FuturesUnordered, StreamExt}; use parallel_task_set::ParallelTaskSet; use serde::{Deserialize, Serialize}; @@ -760,93 +761,6 @@ impl PrometheusMetrics { } } -/// CockroachDB Node ID -/// -/// This field is stored internally as a String to avoid questions -/// about size, signedness, etc - it can be treated as an arbitrary -/// unique identifier. -#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize)] -pub struct NodeId(pub String); - -impl NodeId { - pub fn new(id: String) -> Self { - Self(id) - } - - pub fn as_str(&self) -> &str { - &self.0 - } -} - -impl std::fmt::Display for NodeId { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.0) - } -} - -impl std::str::FromStr for NodeId { - type Err = std::convert::Infallible; - - fn from_str(s: &str) -> Result { - Ok(Self(s.to_string())) - } -} - -// When parsing the underlying NodeId, we force it to be interpreted -// as a String. Without this custom Deserialize implementation, we -// encounter parsing errors when querying endpoints which return the -// NodeId as an integer. -impl<'de> serde::Deserialize<'de> for NodeId { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - use serde::de::{Error, Visitor}; - use std::fmt; - - struct NodeIdVisitor; - - impl<'de> Visitor<'de> for NodeIdVisitor { - type Value = NodeId; - - fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { - formatter - .write_str("a string or integer representing a node ID") - } - - fn visit_str(self, value: &str) -> Result - where - E: Error, - { - Ok(NodeId(value.to_string())) - } - - fn visit_string(self, value: String) -> Result - where - E: Error, - { - Ok(NodeId(value)) - } - - fn visit_i64(self, value: i64) -> Result - where - E: Error, - { - Ok(NodeId(value.to_string())) - } - - fn visit_u64(self, value: u64) -> Result - where - E: Error, - { - Ok(NodeId(value.to_string())) - } - } - - deserializer.deserialize_any(NodeIdVisitor) - } -} - /// CockroachDB node liveness status /// /// From CockroachDB's [NodeLivenessStatus protobuf enum](https://github.com/cockroachdb/cockroach/blob/release-21.1/pkg/kv/kvserver/liveness/livenesspb/liveness.proto#L107-L138) @@ -1283,11 +1197,11 @@ sql_exec_latency_bucket{le="0.01"} 25 // Malformed lines (missing values, extra spaces, etc.) let malformed_input = r#" metric_name_no_value -metric_name_with_space +metric_name_with_space metric_name_multiple spaces here = value_no_name leading_space_metric 123 -trailing_space_metric 456 +trailing_space_metric 456 metric{label=value} 789 metric{malformed=label value} 999 "#; diff --git a/nexus/types/Cargo.toml b/nexus/types/Cargo.toml index 87c8e4b992d..496f7bf4d13 100644 --- a/nexus/types/Cargo.toml +++ b/nexus/types/Cargo.toml @@ -14,6 +14,7 @@ base64.workspace = true chrono.workspace = true clap.workspace = true clickhouse-admin-types.workspace = true +cockroach-admin-types.workspace = true cookie.workspace = true daft.workspace = true derive-where.workspace = true @@ -29,7 +30,6 @@ indent_write.workspace = true ipnetwork.workspace = true itertools.workspace = true newtype_derive.workspace = true -omicron-cockroach-metrics.workspace = true omicron-uuid-kinds.workspace = true openssl.workspace = true oxql-types.workspace = true diff --git a/nexus/types/src/inventory.rs b/nexus/types/src/inventory.rs index 0bac20f86e8..603085d9fde 100644 --- a/nexus/types/src/inventory.rs +++ b/nexus/types/src/inventory.rs @@ -175,7 +175,7 @@ pub struct Collection { /// The status of our cockroachdb cluster, keyed by node identifier pub cockroach_status: - BTreeMap, + BTreeMap, /// The status of time synchronization pub ntp_timesync: IdOrdMap, From b93df4a08bca6e8106dbf4ce76894951f6e3ada4 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 19 Aug 2025 20:56:37 +0000 Subject: [PATCH 2/4] fix malformed input Created using spr 1.3.6-beta.1 --- cockroach-metrics/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cockroach-metrics/src/lib.rs b/cockroach-metrics/src/lib.rs index b58d60d1be4..266e1b51f56 100644 --- a/cockroach-metrics/src/lib.rs +++ b/cockroach-metrics/src/lib.rs @@ -1195,16 +1195,16 @@ sql_exec_latency_bucket{le="0.01"} 25 assert!(result.unwrap().metrics.is_empty()); // Malformed lines (missing values, extra spaces, etc.) - let malformed_input = r#" + let malformed_input = " metric_name_no_value -metric_name_with_space +metric_name_with_space \ metric_name_multiple spaces here = value_no_name leading_space_metric 123 -trailing_space_metric 456 +trailing_space_metric 456 \ metric{label=value} 789 metric{malformed=label value} 999 -"#; +"; let result = PrometheusMetrics::parse(malformed_input); assert!(result.is_ok()); // Should ignore malformed lines gracefully From 5199ede3bfa3bc5752a8cb365cbc5fb74d5fe147 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 19 Aug 2025 20:58:57 +0000 Subject: [PATCH 3/4] fix for real Created using spr 1.3.6-beta.1 --- cockroach-metrics/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cockroach-metrics/src/lib.rs b/cockroach-metrics/src/lib.rs index 266e1b51f56..9523eea3b29 100644 --- a/cockroach-metrics/src/lib.rs +++ b/cockroach-metrics/src/lib.rs @@ -1197,11 +1197,11 @@ sql_exec_latency_bucket{le="0.01"} 25 // Malformed lines (missing values, extra spaces, etc.) let malformed_input = " metric_name_no_value -metric_name_with_space \ +metric_name_with_space \n\ metric_name_multiple spaces here = value_no_name leading_space_metric 123 -trailing_space_metric 456 \ +trailing_space_metric 456 \n\ metric{label=value} 789 metric{malformed=label value} 999 "; From d6f50ab59f808f7703033560e8e23afe3418599b Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 19 Aug 2025 21:08:56 +0000 Subject: [PATCH 4/4] hope this works Created using spr 1.3.6-beta.1 --- Cargo.lock | 5 ++++- cockroach-metrics/src/lib.rs | 4 ++-- nexus/db-model/Cargo.toml | 1 + nexus/db-model/src/inventory.rs | 2 +- nexus/db-queries/Cargo.toml | 1 + nexus/db-queries/src/db/datastore/inventory.rs | 2 +- nexus/inventory/Cargo.toml | 1 + nexus/inventory/src/builder.rs | 2 +- nexus/inventory/src/examples.rs | 2 +- nexus/reconfigurator/planning/Cargo.toml | 2 +- nexus/reconfigurator/planning/src/planner.rs | 6 +++--- 11 files changed, 17 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e219f944585..7130a6faa10 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6400,6 +6400,7 @@ dependencies = [ "camino-tempfile", "chrono", "clickhouse-admin-types", + "cockroach-admin-types", "db-macros", "derive-where", "diesel", @@ -6455,6 +6456,7 @@ dependencies = [ "camino-tempfile", "chrono", "clickhouse-admin-types", + "cockroach-admin-types", "const_format", "criterion", "db-macros", @@ -6600,6 +6602,7 @@ dependencies = [ "clickhouse-admin-keeper-client", "clickhouse-admin-server-client", "clickhouse-admin-types", + "cockroach-admin-types", "dns-service-client", "expectorate", "futures", @@ -6842,6 +6845,7 @@ dependencies = [ "anyhow", "chrono", "clickhouse-admin-types", + "cockroach-admin-types", "daft", "debug-ignore", "dropshot", @@ -6861,7 +6865,6 @@ dependencies = [ "nexus-reconfigurator-blippy", "nexus-sled-agent-shared", "nexus-types", - "omicron-cockroach-metrics", "omicron-common", "omicron-test-utils", "omicron-uuid-kinds", diff --git a/cockroach-metrics/src/lib.rs b/cockroach-metrics/src/lib.rs index 9523eea3b29..50303274cab 100644 --- a/cockroach-metrics/src/lib.rs +++ b/cockroach-metrics/src/lib.rs @@ -1197,11 +1197,11 @@ sql_exec_latency_bucket{le="0.01"} 25 // Malformed lines (missing values, extra spaces, etc.) let malformed_input = " metric_name_no_value -metric_name_with_space \n\ +metric_name_with_space\x20 metric_name_multiple spaces here = value_no_name leading_space_metric 123 -trailing_space_metric 456 \n\ +trailing_space_metric 456\x20 metric{label=value} 789 metric{malformed=label value} 999 "; diff --git a/nexus/db-model/Cargo.toml b/nexus/db-model/Cargo.toml index ea2338eb594..fa00e70c2c2 100644 --- a/nexus/db-model/Cargo.toml +++ b/nexus/db-model/Cargo.toml @@ -15,6 +15,7 @@ anyhow.workspace = true camino.workspace = true chrono.workspace = true clickhouse-admin-types.workspace = true +cockroach-admin-types.workspace = true derive-where.workspace = true diesel = { workspace = true, features = ["postgres", "r2d2", "chrono", "serde_json", "network-address", "uuid"] } hex.workspace = true diff --git a/nexus/db-model/src/inventory.rs b/nexus/db-model/src/inventory.rs index 61f904bba0f..d05e8c0cb3a 100644 --- a/nexus/db-model/src/inventory.rs +++ b/nexus/db-model/src/inventory.rs @@ -2918,7 +2918,7 @@ pub struct InvCockroachStatus { impl InvCockroachStatus { pub fn new( inv_collection_id: CollectionUuid, - node_id: omicron_cockroach_metrics::NodeId, + node_id: cockroach_admin_types::NodeId, status: &CockroachStatus, ) -> Result { Ok(Self { diff --git a/nexus/db-queries/Cargo.toml b/nexus/db-queries/Cargo.toml index b9c3e5098a0..381c139fd77 100644 --- a/nexus/db-queries/Cargo.toml +++ b/nexus/db-queries/Cargo.toml @@ -17,6 +17,7 @@ async-trait.workspace = true camino.workspace = true chrono.workspace = true clickhouse-admin-types.workspace = true +cockroach-admin-types.workspace = true const_format.workspace = true diesel.workspace = true diesel-dtrace.workspace = true diff --git a/nexus/db-queries/src/db/datastore/inventory.rs b/nexus/db-queries/src/db/datastore/inventory.rs index ea39feab4b2..2af070e3571 100644 --- a/nexus/db-queries/src/db/datastore/inventory.rs +++ b/nexus/db-queries/src/db/datastore/inventory.rs @@ -12,6 +12,7 @@ use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; use async_bb8_diesel::AsyncSimpleConnection; use clickhouse_admin_types::ClickhouseKeeperClusterMembership; +use cockroach_admin_types::NodeId as CockroachNodeId; use diesel::BoolExpressionMethods; use diesel::ExpressionMethods; use diesel::IntoSql; @@ -102,7 +103,6 @@ use nexus_types::inventory::InternalDnsGenerationStatus; use nexus_types::inventory::PhysicalDiskFirmware; use nexus_types::inventory::SledAgent; use nexus_types::inventory::TimeSync; -use omicron_cockroach_metrics::NodeId as CockroachNodeId; use omicron_common::api::external::Error; use omicron_common::api::external::InternalContext; use omicron_common::api::external::LookupType; diff --git a/nexus/inventory/Cargo.toml b/nexus/inventory/Cargo.toml index f6b90fb6f30..afc439b8727 100644 --- a/nexus/inventory/Cargo.toml +++ b/nexus/inventory/Cargo.toml @@ -16,6 +16,7 @@ clickhouse-admin-keeper-client.workspace = true dns-service-client.workspace = true clickhouse-admin-server-client.workspace = true clickhouse-admin-types.workspace = true +cockroach-admin-types.workspace = true futures.workspace = true gateway-client.workspace = true gateway-messages.workspace = true diff --git a/nexus/inventory/src/builder.rs b/nexus/inventory/src/builder.rs index 6a30c02584c..44bc6360a9c 100644 --- a/nexus/inventory/src/builder.rs +++ b/nexus/inventory/src/builder.rs @@ -13,6 +13,7 @@ use anyhow::anyhow; use chrono::DateTime; use chrono::Utc; use clickhouse_admin_types::ClickhouseKeeperClusterMembership; +use cockroach_admin_types::NodeId; use gateway_client::types::SpComponentCaboose; use gateway_client::types::SpState; use gateway_client::types::SpType; @@ -37,7 +38,6 @@ use nexus_types::inventory::SledAgent; use nexus_types::inventory::TimeSync; use nexus_types::inventory::Zpool; use omicron_cockroach_metrics::CockroachMetric; -use omicron_cockroach_metrics::NodeId; use omicron_cockroach_metrics::PrometheusMetrics; use omicron_common::disk::M2Slot; use omicron_uuid_kinds::CollectionKind; diff --git a/nexus/inventory/src/examples.rs b/nexus/inventory/src/examples.rs index d0fca08aa50..e4605863d90 100644 --- a/nexus/inventory/src/examples.rs +++ b/nexus/inventory/src/examples.rs @@ -653,7 +653,7 @@ pub fn representative() -> Representative { ); builder.found_cockroach_metrics( - omicron_cockroach_metrics::NodeId::new("1".to_string()), + cockroach_admin_types::NodeId::new("1".to_string()), PrometheusMetrics { metrics: BTreeMap::from([( "ranges_underreplicated".to_string(), diff --git a/nexus/reconfigurator/planning/Cargo.toml b/nexus/reconfigurator/planning/Cargo.toml index 417ca9678ae..cb441c9fde1 100644 --- a/nexus/reconfigurator/planning/Cargo.toml +++ b/nexus/reconfigurator/planning/Cargo.toml @@ -9,6 +9,7 @@ workspace = true [dependencies] anyhow.workspace = true clickhouse-admin-types.workspace = true +cockroach-admin-types.workspace = true chrono.workspace = true debug-ignore.workspace = true daft.workspace = true @@ -50,7 +51,6 @@ omicron-workspace-hack.workspace = true dropshot.workspace = true expectorate.workspace = true maplit.workspace = true -omicron-cockroach-metrics.workspace = true omicron-common = { workspace = true, features = ["testing"] } omicron-test-utils.workspace = true proptest.workspace = true diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index d9633258d02..070808a8875 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -5970,7 +5970,7 @@ pub(crate) mod test { let mut result = BTreeMap::new(); for i in 1..=COCKROACHDB_REDUNDANCY { result.insert( - omicron_cockroach_metrics::NodeId(i.to_string()), + cockroach_admin_types::NodeId(i.to_string()), CockroachStatus { ranges_underreplicated: Some(0), liveness_live_nodes: Some(GOAL_REDUNDANCY), @@ -6011,7 +6011,7 @@ pub(crate) mod test { *example .collection .cockroach_status - .get_mut(&omicron_cockroach_metrics::NodeId("1".to_string())) + .get_mut(&cockroach_admin_types::NodeId("1".to_string())) .unwrap() = CockroachStatus { ranges_underreplicated: Some(1), liveness_live_nodes: Some(GOAL_REDUNDANCY), @@ -6029,7 +6029,7 @@ pub(crate) mod test { *example .collection .cockroach_status - .get_mut(&omicron_cockroach_metrics::NodeId("1".to_string())) + .get_mut(&cockroach_admin_types::NodeId("1".to_string())) .unwrap() = CockroachStatus { ranges_underreplicated: Some(0), liveness_live_nodes: Some(GOAL_REDUNDANCY - 1),