move cockroach-admin-client NodeId to cockroach-admin-types (#8865)

Currently, the OpenAPI manager has a circular dependency with
cockroach-admin-client and cockroach-admin-api, making it hard to make
changes to the API that need to happen before the client is updated. The
only dependency is `NodeId`, though, which can be moved to this more
central location.

Author: sunshowers

Cargo.lock

@@ -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<Self, Self::Err> {
+        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<D>(deserializer: D) -> Result<Self, D::Error>
+    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<E>(self, value: &str) -> Result<Self::Value, E>
+            where
+                E: Error,
+            {
+                Ok(NodeId(value.to_string()))
+            }
+
+            fn visit_string<E>(self, value: String) -> Result<Self::Value, E>
+            where
+                E: Error,
+            {
+                Ok(NodeId(value))
+            }
+
+            fn visit_i64<E>(self, value: i64) -> Result<Self::Value, E>
+            where
+                E: Error,
+            {
+                Ok(NodeId(value.to_string()))
+            }
+
+            fn visit_u64<E>(self, value: u64) -> Result<Self::Value, E>
+            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,

cockroach-admin/types/src/lib.rs

@@ -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

cockroach-metrics/Cargo.toml

@@ -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<Self, Self::Err> {
-        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<D>(deserializer: D) -> Result<Self, D::Error>
-    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<E>(self, value: &str) -> Result<Self::Value, E>
-            where
-                E: Error,
-            {
-                Ok(NodeId(value.to_string()))
-            }
-
-            fn visit_string<E>(self, value: String) -> Result<Self::Value, E>
-            where
-                E: Error,
-            {
-                Ok(NodeId(value))
-            }
-
-            fn visit_i64<E>(self, value: i64) -> Result<Self::Value, E>
-            where
-                E: Error,
-            {
-                Ok(NodeId(value.to_string()))
-            }
-
-            fn visit_u64<E>(self, value: u64) -> Result<Self::Value, E>
-            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)
@@ -1281,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\x20
 metric_name_multiple spaces here
 = value_no_name
  leading_space_metric 123
-trailing_space_metric 456 
+trailing_space_metric 456\x20
 metric{label=value} 789
 metric{malformed=label value} 999
-"#;
+";
         let result = PrometheusMetrics::parse(malformed_input);
         assert!(result.is_ok());
         // Should ignore malformed lines gracefully

cockroach-metrics/src/lib.rs

@@ -12,10 +12,8 @@ Clickhouse Single-Node Cluster Admin (client: clickhouse-admin-single-client)
     consumed by: omicron-nexus (omicron/nexus) via 2 paths
 
 CockroachDB Cluster Admin (client: cockroach-admin-client)
-    consumed by: omicron-gateway (omicron/gateway) via 1 path
     consumed by: omicron-nexus (omicron/nexus) via 3 paths
-    consumed by: omicron-sled-agent (omicron/sled-agent) via 2 paths
-    consumed by: oximeter-collector (omicron/oximeter/collector) via 1 path
+    consumed by: omicron-sled-agent (omicron/sled-agent) via 1 path
 
 Crucible Agent (client: crucible-agent-client)
     consumed by: omicron-nexus (omicron/nexus) via 1 path

dev-tools/ls-apis/tests/api_dependencies.out

@@ -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

nexus/db-model/Cargo.toml

@@ -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<Self, anyhow::Error> {
         Ok(Self {

nexus/db-model/src/inventory.rs

@@ -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

nexus/db-queries/Cargo.toml

@@ -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;

nexus/db-queries/src/db/datastore/inventory.rs

@@ -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