From dfb34d4b32511173134ce036ab01dd6d5cba51d2 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 21 Jan 2026 21:56:05 -0800 Subject: [PATCH 01/19] ls-apis needs to detect cycles in dependency unit graph --- dev-tools/ls-apis/src/system_apis.rs | 74 ++++++++++++++++++++++++---- 1 file changed, 65 insertions(+), 9 deletions(-) diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index e61af0b7c50..549adac214a 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -474,6 +474,21 @@ impl SystemApis { /// Returns a string that can be passed to `dot(1)` to render a graph of /// API dependencies among deployment units pub fn dot_by_unit(&self, filter: ApiDependencyFilter) -> Result { + let (graph, _) = self.make_deployment_unit_graph(filter, false)?; + Ok(Dot::new(&graph).to_string()) + } + + // The complex type below is only used in this one place: the return value + // of this internal helper function. A type alias doesn't seem better. + #[allow(clippy::type_complexity)] + fn make_deployment_unit_graph( + &self, + dependency_filter: ApiDependencyFilter, + versioned_on_server_only: bool, + ) -> Result<( + petgraph::graph::Graph<&DeploymentUnitName, &ClientPackageName>, + BTreeMap<&DeploymentUnitName, NodeIndex>, + )> { let mut graph = petgraph::graph::Graph::new(); let nodes: BTreeMap<_, _> = self .deployment_units() @@ -489,8 +504,38 @@ impl SystemApis { let my_node = nodes.get(deployment_unit).unwrap(); for server_pkg in server_components { for (client_pkg, _) in - self.component_apis_consumed(server_pkg, filter)? + self.component_apis_consumed(server_pkg, dependency_filter)? { + if versioned_on_server_only { + let api = self + .api_metadata + .client_pkgname_lookup(client_pkg) + .unwrap(); + if api.versioned_how != VersionedHow::Server { + continue; + } + + // XXX-dap relationships we want to ignore because + // they're local-machine only + match (server_pkg.as_str(), client_pkg.as_str()) { + ("ddmd", "dpd-client") + | ("dpd", "gateway-client") + | ("lldpd", "dpd-client") + | ("mgd", "ddm-admin-client") + | ("mgd", "dpd-client") + | ("mgd", "gateway-client") + | ("omicron-sled-agent", "gateway-client") + | ("omicron-sled-agent", "ddm-admin-client") + | ("omicron-sled-agent", "dpd-client") // XXX-dap + | ("omicron-sled-agent", "mg-admin-client") + | ("omicron-sled-agent", "propolis-client") + | ("tfportd", "dpd-client") + | ("tfportd", "lldpd-client") + | ("wicketd", _) => continue, + _ => (), + } + } + // Multiple server components may produce an API. However, // if an API is produced by multiple server components // within the same deployment unit, we would like to only @@ -506,17 +551,16 @@ impl SystemApis { .collect(); for other_unit in other_units { let other_node = nodes.get(other_unit).unwrap(); - graph.update_edge( - *my_node, - *other_node, - client_pkg.clone(), + eprintln!( + "dap: edge: {deployment_unit} {other_unit} {server_pkg} {client_pkg}" ); + graph.update_edge(*my_node, *other_node, client_pkg); } } } } - Ok(Dot::new(&graph).to_string()) + Ok((graph, nodes)) } /// Returns a string that can be passed to `dot(1)` to render a graph of @@ -530,7 +574,7 @@ impl SystemApis { } // The complex type below is only used in this one place: the return value - // of an internal helper function. A type alias doesn't seem better. + // of this internal helper function. A type alias doesn't seem better. #[allow(clippy::type_complexity)] fn make_component_graph( &self, @@ -643,8 +687,20 @@ impl SystemApis { nodes.iter().map(|(s_c, node)| (node, s_c)).collect(); if let Err(error) = petgraph::algo::toposort(&graph, None) { bail!( - "graph of server-managed components has a cycle (includes \ - node: {:?})", + "graph of server-managed API dependencies between components \ + has a cycle (includes node: {:?})", + reverse_nodes.get(&error.node_id()).unwrap() + ); + } + + // Do the same with a graph of deployment units. + let (graph, nodes) = self.make_deployment_unit_graph(filter, true)?; + let reverse_nodes: BTreeMap<_, _> = + nodes.iter().map(|(d_u, node)| (node, d_u)).collect(); + if let Err(error) = petgraph::algo::toposort(&graph, None) { + bail!( + "graph of server-managed API dependencies between deployment \ + units has a cycle (includes node: {:?})", reverse_nodes.get(&error.node_id()).unwrap() ); } From 172dfef6e4435edc8ca884a97d098a404862556c Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 21 Jan 2026 22:02:08 -0800 Subject: [PATCH 02/19] remove eprintln --- dev-tools/ls-apis/src/system_apis.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index 549adac214a..196bc1f30bf 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -551,9 +551,6 @@ impl SystemApis { .collect(); for other_unit in other_units { let other_node = nodes.get(other_unit).unwrap(); - eprintln!( - "dap: edge: {deployment_unit} {other_unit} {server_pkg} {client_pkg}" - ); graph.update_edge(*my_node, *other_node, client_pkg); } } From d1335578d8496cffd6645ff376bcf77a2a622446 Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 23 Jan 2026 21:37:07 +0000 Subject: [PATCH 03/19] move to api-manifest.toml --- dev-tools/ls-apis/api-manifest.toml | 80 +++++++++++++++++++++ dev-tools/ls-apis/src/api_metadata.rs | 100 +++++++++++++++++++++++++- dev-tools/ls-apis/src/bin/ls-apis.rs | 6 +- dev-tools/ls-apis/src/system_apis.rs | 49 ++++++++----- 4 files changed, 215 insertions(+), 20 deletions(-) diff --git a/dev-tools/ls-apis/api-manifest.toml b/dev-tools/ls-apis/api-manifest.toml index 5a9c64560d5..41e3d731fe2 100644 --- a/dev-tools/ls-apis/api-manifest.toml +++ b/dev-tools/ls-apis/api-manifest.toml @@ -593,3 +593,83 @@ note = """ Sled Agent uses the Crucible Agent client types only, and only in the simulated sled agent. """ + +################################################################################ +# Localhost-only edges +# +# These edges are excluded from the deployment unit dependency graph because +# they represent communication that only happens locally within a deployment +# unit, not across deployment unit boundaries. A single deployment unit is +# updated atomically, so there's no risk of version skew with localhost-only +# edges. +################################################################################ + +[[localhost_only_edges]] +server = "ddmd" +client = "dpd-client" +note = "still to verify: ddmd communicates with dendrite locally within the switch zone" + +[[localhost_only_edges]] +server = "dpd" +client = "gateway-client" +note = "still to verify: dendrite communicates with MGS locally" + +[[localhost_only_edges]] +server = "lldpd" +client = "dpd-client" +note = "still to verify: lldpd communicates with dendrite locally within the switch zone" + +[[localhost_only_edges]] +server = "mgd" +client = "ddm-admin-client" +note = "still to verify: mgd communicates with ddmd locally within the switch zone" + +[[localhost_only_edges]] +server = "mgd" +client = "dpd-client" +note = "still to verify: mgd communicates with dendrite locally within the switch zone" + +[[localhost_only_edges]] +server = "mgd" +client = "gateway-client" +note = "still to verify: mgd communicates with MGS locally" + +[[localhost_only_edges]] +server = "omicron-sled-agent" +client = "gateway-client" +note = "still to verify: sled-agent communicates with MGS locally" + +[[localhost_only_edges]] +server = "omicron-sled-agent" +client = "ddm-admin-client" +note = "still to verify: sled-agent communicates with ddmd locally" + +[[localhost_only_edges]] +server = "omicron-sled-agent" +client = "dpd-client" +note = "still to verify: sled-agent communicates with dendrite locally" + +[[localhost_only_edges]] +server = "omicron-sled-agent" +client = "mg-admin-client" +note = "still to verify: sled-agent communicates with mgd locally" + +[[localhost_only_edges]] +server = "omicron-sled-agent" +client = "propolis-client" +note = "still to verify: sled-agent communicates with propolis instances locally" + +[[localhost_only_edges]] +server = "tfportd" +client = "dpd-client" +note = "still to verify: tfportd communicates with dendrite locally within the switch zone" + +[[localhost_only_edges]] +server = "tfportd" +client = "lldpd-client" +note = "still to verify: tfportd communicates with lldpd locally within the switch zone" + +[[localhost_only_edges]] +server = "wicketd" +client = "*" +note = "still to verify: wicketd communicates with local services; enumerate specific clients later" diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index b0e3dfa4247..611f09de2a1 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -30,6 +30,7 @@ pub struct AllApiMetadata { deployment_units: BTreeMap, dependency_rules: BTreeMap>, ignored_non_clients: BTreeSet, + localhost_only_edges: Vec, } impl AllApiMetadata { @@ -73,6 +74,11 @@ impl AllApiMetadata { &self.ignored_non_clients } + /// Returns the list of localhost-only edges + pub fn localhost_only_edges(&self) -> &[LocalhostOnlyEdge] { + &self.localhost_only_edges + } + /// Returns how we should filter the given dependency pub(crate) fn evaluate_dependency( &self, @@ -138,6 +144,7 @@ struct RawApiMetadata { deployment_units: Vec, dependency_filter_rules: Vec, ignored_non_clients: Vec, + localhost_only_edges: Vec, } impl TryFrom for AllApiMetadata { @@ -188,17 +195,42 @@ impl TryFrom for AllApiMetadata { for client_pkg in raw.ignored_non_clients { if !ignored_non_clients.insert(client_pkg.clone()) { bail!( - "entry in ignored_non_clients appearead twice: {:?}", + "entry in ignored_non_clients appeared twice: {:?}", &client_pkg ); } } + // Validate localhost_only_edges reference known server components and + // APIs. + let known_components: BTreeSet<_> = deployment_units + .values() + .flat_map(|u| u.packages.iter()) + .collect(); + for edge in &raw.localhost_only_edges { + if !known_components.contains(&edge.server) { + bail!( + "localhost_only_edges: unknown server component {:?}", + edge.server + ); + } + // Validate non-wildcard clients reference known APIs. + if let Some(client_name) = edge.client.as_specific() { + if !apis.contains_key(client_name) { + bail!( + "localhost_only_edges: unknown client {:?}", + client_name + ); + } + } + } + Ok(AllApiMetadata { apis, deployment_units, dependency_rules, ignored_non_clients, + localhost_only_edges: raw.localhost_only_edges, }) } } @@ -416,3 +448,69 @@ pub enum Evaluation { /// This dependency should be part of the update DAG Dag, } + +/// Specifies which client(s) to match in a localhost-only edge rule. +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum ClientMatcher { + /// Match all clients (represented as "*" in TOML). + Wildcard, + /// Match a specific client package. + Specific(ClientPackageName), +} + +impl<'de> Deserialize<'de> for ClientMatcher { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + if s == "*" { + Ok(ClientMatcher::Wildcard) + } else { + Ok(ClientMatcher::Specific(ClientPackageName::from(s))) + } + } +} + +impl ClientMatcher { + /// Returns true if this matcher matches the given client package. + pub fn matches(&self, client: &ClientPackageName) -> bool { + match self { + ClientMatcher::Wildcard => true, + ClientMatcher::Specific(name) => name == client, + } + } + + /// Returns the specific client name, if not a wildcard. + pub fn as_specific(&self) -> Option<&ClientPackageName> { + match self { + ClientMatcher::Wildcard => None, + ClientMatcher::Specific(name) => Some(name), + } + } +} + +/// An edge that should be excluded from the deployment unit dependency graph +/// because it represents communication that only happens locally within a +/// deployment unit. +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +pub struct LocalhostOnlyEdge { + /// The server component that consumes the API. + pub server: ServerComponentName, + /// The client package consumed, or "*" to match all clients. + pub client: ClientMatcher, + /// Explanation of why this edge is localhost-only. + pub note: String, +} + +impl LocalhostOnlyEdge { + /// Returns true if this rule matches the given (server, client) pair. + pub fn matches( + &self, + server: &ServerComponentName, + client: &ClientPackageName, + ) -> bool { + self.server == *server && self.client.matches(client) + } +} diff --git a/dev-tools/ls-apis/src/bin/ls-apis.rs b/dev-tools/ls-apis/src/bin/ls-apis.rs index df7fd9617a6..edc1679a50e 100644 --- a/dev-tools/ls-apis/src/bin/ls-apis.rs +++ b/dev-tools/ls-apis/src/bin/ls-apis.rs @@ -252,7 +252,11 @@ fn print_server_components<'a>( ); } for (c, path) in apis.component_apis_consumed(s, filter)? { - println!("{} consumes: {}", prefix, c); + if let Some(note) = apis.localhost_only_edge_note(s, c) { + println!("{} consumes: {} (localhost-only: {})", prefix, c, note); + } else { + println!("{} consumes: {}", prefix, c); + } if show_deps { for p in path.nodes() { println!("{} via: {}", prefix, p); diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index 196bc1f30bf..600e40c129d 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -330,6 +330,32 @@ impl SystemApis { &self.api_metadata } + /// Returns true if this (server, client) pair is a localhost-only edge. + /// + /// Localhost-only edges are excluded from the deployment unit dependency + /// graph because they represent communication that only happens locally + /// within a deployment unit, not across deployment unit boundaries. + fn is_localhost_only_edge( + &self, + server: &ServerComponentName, + client: &ClientPackageName, + ) -> bool { + self.localhost_only_edge_note(server, client).is_some() + } + + /// Returns the note for a localhost-only edge, if one matches. + pub fn localhost_only_edge_note( + &self, + server: &ServerComponentName, + client: &ClientPackageName, + ) -> Option<&str> { + self.api_metadata + .localhost_only_edges() + .iter() + .find(|edge| edge.matches(server, client)) + .map(|edge| edge.note.as_str()) + } + /// Given a server component, return the APIs consumed by this component pub fn component_apis_consumed( &self, @@ -515,24 +541,11 @@ impl SystemApis { continue; } - // XXX-dap relationships we want to ignore because - // they're local-machine only - match (server_pkg.as_str(), client_pkg.as_str()) { - ("ddmd", "dpd-client") - | ("dpd", "gateway-client") - | ("lldpd", "dpd-client") - | ("mgd", "ddm-admin-client") - | ("mgd", "dpd-client") - | ("mgd", "gateway-client") - | ("omicron-sled-agent", "gateway-client") - | ("omicron-sled-agent", "ddm-admin-client") - | ("omicron-sled-agent", "dpd-client") // XXX-dap - | ("omicron-sled-agent", "mg-admin-client") - | ("omicron-sled-agent", "propolis-client") - | ("tfportd", "dpd-client") - | ("tfportd", "lldpd-client") - | ("wicketd", _) => continue, - _ => (), + // Skip edges that represent localhost-only communication + // (communication within a deployment unit that doesn't + // cross deployment unit boundaries). + if self.is_localhost_only_edge(server_pkg, client_pkg) { + continue; } } From 5e04994f65d51b09e62d6499004687c7a2437ccf Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 23 Jan 2026 22:26:10 +0000 Subject: [PATCH 04/19] make check exhaustive --- dev-tools/ls-apis/api-manifest.toml | 44 +++++--- dev-tools/ls-apis/src/api_metadata.rs | 37 ++----- dev-tools/ls-apis/src/bin/ls-apis.rs | 5 +- dev-tools/ls-apis/src/system_apis.rs | 141 ++++++++++++++++++++++---- 4 files changed, 167 insertions(+), 60 deletions(-) diff --git a/dev-tools/ls-apis/api-manifest.toml b/dev-tools/ls-apis/api-manifest.toml index 41e3d731fe2..46adc8f0a70 100644 --- a/dev-tools/ls-apis/api-manifest.toml +++ b/dev-tools/ls-apis/api-manifest.toml @@ -607,69 +607,83 @@ sled agent. [[localhost_only_edges]] server = "ddmd" client = "dpd-client" -note = "still to verify: ddmd communicates with dendrite locally within the switch zone" +note = "verified: sled-agent configures ddmd with dpd_host=[::1] (services.rs:3127)" [[localhost_only_edges]] server = "dpd" client = "gateway-client" -note = "still to verify: dendrite communicates with MGS locally" +note = "verified: dpd defaults to [::1]:MGS_PORT (dendrite switch_identifiers.rs:60)" [[localhost_only_edges]] server = "lldpd" client = "dpd-client" -note = "still to verify: lldpd communicates with dendrite locally within the switch zone" +note = "verified: lldpd defaults to localhost for dpd (lldp dendrite.rs:194)" [[localhost_only_edges]] server = "mgd" client = "ddm-admin-client" -note = "still to verify: mgd communicates with ddmd locally within the switch zone" +note = "verified: mg-lower hardcodes localhost:8000 (mg-lower ddm.rs:110)" [[localhost_only_edges]] server = "mgd" client = "dpd-client" -note = "still to verify: mgd communicates with dendrite locally within the switch zone" +note = "verified: mg-lower hardcodes localhost (mg-lower dendrite.rs:491)" [[localhost_only_edges]] server = "mgd" client = "gateway-client" -note = "still to verify: mgd communicates with MGS locally" +note = "verified: mgd defaults to [::1]:12225 (mgd main.rs:93)" + +# NOTE: The following sled-agent edges are BUGS - they use underlay IPs, not +# localhost. They are kept here to avoid breaking the build, but need to be +# fixed, either through client-side versioning or by some other means. [[localhost_only_edges]] server = "omicron-sled-agent" client = "gateway-client" -note = "still to verify: sled-agent communicates with MGS locally" +note = "BUG: uses underlay IP, not localhost (early_networking.rs:376)" [[localhost_only_edges]] server = "omicron-sled-agent" client = "ddm-admin-client" -note = "still to verify: sled-agent communicates with ddmd locally" +note = "verified: uses Client::localhost() (ddm_reconciler.rs:56)" [[localhost_only_edges]] server = "omicron-sled-agent" client = "dpd-client" -note = "still to verify: sled-agent communicates with dendrite locally" +note = "BUG: uses underlay IP, not localhost (services.rs:1090)" [[localhost_only_edges]] server = "omicron-sled-agent" client = "mg-admin-client" -note = "still to verify: sled-agent communicates with mgd locally" +note = "BUG: uses underlay IP, not localhost (early_networking.rs:483)" [[localhost_only_edges]] server = "omicron-sled-agent" client = "propolis-client" -note = "still to verify: sled-agent communicates with propolis instances locally" +note = "BUG: uses zone IP, not localhost (instance.rs:2283)" [[localhost_only_edges]] server = "tfportd" client = "dpd-client" -note = "still to verify: tfportd communicates with dendrite locally within the switch zone" +note = "verified: configured with dpd_host=[::1] (services.rs:2852)" [[localhost_only_edges]] server = "tfportd" client = "lldpd-client" -note = "still to verify: tfportd communicates with lldpd locally within the switch zone" +note = "verified: hardcodes localhost (tfportd tfport.rs:88)" + +[[localhost_only_edges]] +server = "wicketd" +client = "ddm-admin-client" +note = "verified: uses DdmAdminClient::localhost() (bootstrap_addrs.rs:162)" [[localhost_only_edges]] server = "wicketd" -client = "*" -note = "still to verify: wicketd communicates with local services; enumerate specific clients later" +client = "dpd-client" +note = "verified: hardcodes [::1]:DENDRITE_PORT (preflight_check/uplink.rs:83)" + +[[localhost_only_edges]] +server = "wicketd" +client = "gateway-client" +note = "verified: --mgs-address CLI expects localhost (bin/wicketd.rs:35)" diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index 611f09de2a1..3d0ff0052e2 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -203,10 +203,8 @@ impl TryFrom for AllApiMetadata { // Validate localhost_only_edges reference known server components and // APIs. - let known_components: BTreeSet<_> = deployment_units - .values() - .flat_map(|u| u.packages.iter()) - .collect(); + let known_components: BTreeSet<_> = + deployment_units.values().flat_map(|u| u.packages.iter()).collect(); for edge in &raw.localhost_only_edges { if !known_components.contains(&edge.server) { bail!( @@ -214,14 +212,9 @@ impl TryFrom for AllApiMetadata { edge.server ); } - // Validate non-wildcard clients reference known APIs. - if let Some(client_name) = edge.client.as_specific() { - if !apis.contains_key(client_name) { - bail!( - "localhost_only_edges: unknown client {:?}", - client_name - ); - } + let client_name = edge.client.as_specific(); + if !apis.contains_key(client_name) { + bail!("localhost_only_edges: unknown client {:?}", client_name); } } @@ -449,11 +442,9 @@ pub enum Evaluation { Dag, } -/// Specifies which client(s) to match in a localhost-only edge rule. +/// Specifies which client to match in a localhost-only edge rule. #[derive(Clone, Debug, Eq, PartialEq)] pub enum ClientMatcher { - /// Match all clients (represented as "*" in TOML). - Wildcard, /// Match a specific client package. Specific(ClientPackageName), } @@ -464,11 +455,7 @@ impl<'de> Deserialize<'de> for ClientMatcher { D: serde::Deserializer<'de>, { let s = String::deserialize(deserializer)?; - if s == "*" { - Ok(ClientMatcher::Wildcard) - } else { - Ok(ClientMatcher::Specific(ClientPackageName::from(s))) - } + Ok(ClientMatcher::Specific(ClientPackageName::from(s))) } } @@ -476,16 +463,14 @@ impl ClientMatcher { /// Returns true if this matcher matches the given client package. pub fn matches(&self, client: &ClientPackageName) -> bool { match self { - ClientMatcher::Wildcard => true, ClientMatcher::Specific(name) => name == client, } } - /// Returns the specific client name, if not a wildcard. - pub fn as_specific(&self) -> Option<&ClientPackageName> { + /// Returns the specific client name. + pub fn as_specific(&self) -> &ClientPackageName { match self { - ClientMatcher::Wildcard => None, - ClientMatcher::Specific(name) => Some(name), + ClientMatcher::Specific(name) => name, } } } @@ -498,7 +483,7 @@ impl ClientMatcher { pub struct LocalhostOnlyEdge { /// The server component that consumes the API. pub server: ServerComponentName, - /// The client package consumed, or "*" to match all clients. + /// The client package consumed. pub client: ClientMatcher, /// Explanation of why this edge is localhost-only. pub note: String, diff --git a/dev-tools/ls-apis/src/bin/ls-apis.rs b/dev-tools/ls-apis/src/bin/ls-apis.rs index edc1679a50e..54169e076c0 100644 --- a/dev-tools/ls-apis/src/bin/ls-apis.rs +++ b/dev-tools/ls-apis/src/bin/ls-apis.rs @@ -253,7 +253,10 @@ fn print_server_components<'a>( } for (c, path) in apis.component_apis_consumed(s, filter)? { if let Some(note) = apis.localhost_only_edge_note(s, c) { - println!("{} consumes: {} (localhost-only: {})", prefix, c, note); + println!( + "{} consumes: {} (localhost-only: {})", + prefix, c, note + ); } else { println!("{} consumes: {}", prefix, c); } diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index 600e40c129d..56f0e179483 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -15,6 +15,7 @@ use crate::api_metadata::ApiConsumerStatus; use crate::api_metadata::ApiExpectedConsumer; use crate::api_metadata::ApiExpectedConsumers; use crate::api_metadata::ApiMetadata; +use crate::api_metadata::ClientMatcher; use crate::api_metadata::Evaluation; use crate::api_metadata::VersionedHow; use crate::cargo::DepPath; @@ -330,19 +331,6 @@ impl SystemApis { &self.api_metadata } - /// Returns true if this (server, client) pair is a localhost-only edge. - /// - /// Localhost-only edges are excluded from the deployment unit dependency - /// graph because they represent communication that only happens locally - /// within a deployment unit, not across deployment unit boundaries. - fn is_localhost_only_edge( - &self, - server: &ServerComponentName, - client: &ClientPackageName, - ) -> bool { - self.localhost_only_edge_note(server, client).is_some() - } - /// Returns the note for a localhost-only edge, if one matches. pub fn localhost_only_edge_note( &self, @@ -500,7 +488,7 @@ impl SystemApis { /// Returns a string that can be passed to `dot(1)` to render a graph of /// API dependencies among deployment units pub fn dot_by_unit(&self, filter: ApiDependencyFilter) -> Result { - let (graph, _) = self.make_deployment_unit_graph(filter, false)?; + let (graph, _) = self.make_deployment_unit_graph(filter, None)?; Ok(Dot::new(&graph).to_string()) } @@ -510,7 +498,9 @@ impl SystemApis { fn make_deployment_unit_graph( &self, dependency_filter: ApiDependencyFilter, - versioned_on_server_only: bool, + localhost_only_edges: Option< + &BTreeSet<(ServerComponentName, ClientPackageName)>, + >, ) -> Result<( petgraph::graph::Graph<&DeploymentUnitName, &ClientPackageName>, BTreeMap<&DeploymentUnitName, NodeIndex>, @@ -532,7 +522,9 @@ impl SystemApis { for (client_pkg, _) in self.component_apis_consumed(server_pkg, dependency_filter)? { - if versioned_on_server_only { + // When localhost_only_edges is provided, we're building a + // graph of server-side-versioned API dependencies only. + if let Some(localhost_edges) = localhost_only_edges { let api = self .api_metadata .client_pkgname_lookup(client_pkg) @@ -544,7 +536,9 @@ impl SystemApis { // Skip edges that represent localhost-only communication // (communication within a deployment unit that doesn't // cross deployment unit boundaries). - if self.is_localhost_only_edge(server_pkg, client_pkg) { + if localhost_edges + .contains(&(server_pkg.clone(), client_pkg.clone())) + { continue; } } @@ -631,6 +625,112 @@ impl SystemApis { Ok((graph, nodes)) } + /// Computes the set of (server, client) edges that must be excluded from + /// the deployment unit dependency graph because they represent intra-unit + /// communication for server-side-versioned APIs. + /// + /// These are edges where all of the below are true: + /// + /// 1. The server consumes the client. + /// 2. The API is server-side-versioned. + /// 3. The server and API producer are in the same deployment unit. + /// + /// Returns a set of (server, client) pairs. + fn compute_required_localhost_edges( + &self, + ) -> Result> { + let filter = ApiDependencyFilter::Default; + let mut required = BTreeSet::new(); + + for server in self.server_component_units.keys() { + let Some(server_unit) = self.server_component_units.get(server) + else { + continue; + }; + + for (client, _) in self.component_apis_consumed(server, filter)? { + // Only consider server-side-versioned APIs. + let Some(api) = self.api_metadata.client_pkgname_lookup(client) + else { + continue; + }; + if api.versioned_how != VersionedHow::Server { + continue; + } + + // Check if any producer is in the same deployment unit. + for producer in self.api_producers(client) { + let Some(producer_unit) = + self.server_component_units.get(producer) + else { + continue; + }; + + if server_unit == producer_unit { + // This edge would create an intra-unit dependency for + // a server-versioned API, so it must be in the + // localhost-only list. + required.insert((server.clone(), client.clone())); + break; + } + } + } + } + + Ok(required) + } + + /// Validates that the configured localhost_only_edges exactly match the + /// required set of edges that must be excluded. + /// + /// Returns the validated set of edges for use by make_deployment_unit_graph. + fn validate_localhost_only_edges( + &self, + ) -> Result> { + let required = self.compute_required_localhost_edges()?; + + // Build the configured set from the manifest. + let mut configured = BTreeSet::new(); + for edge in self.api_metadata.localhost_only_edges() { + let ClientMatcher::Specific(client) = &edge.client; + configured.insert((edge.server.clone(), client.clone())); + } + + // Compare the two sets. + let missing: BTreeSet<_> = required.difference(&configured).collect(); + let extra: BTreeSet<_> = configured.difference(&required).collect(); + + if !missing.is_empty() || !extra.is_empty() { + let mut msg = String::from( + "localhost_only_edges configuration does not match required edges:\n", + ); + + if !missing.is_empty() { + msg.push_str("\nMissing entries (these edges exist and need localhost-only exclusion):\n"); + for (server, client) in &missing { + msg.push_str(&format!( + " - server = {:?}, client = {:?}\n", + server, client + )); + } + } + + if !extra.is_empty() { + msg.push_str("\nExtra entries (these edges don't exist or don't need exclusion):\n"); + for (server, client) in &extra { + msg.push_str(&format!( + " - server = {:?}, client = {:?}\n", + server, client + )); + } + } + + bail!("{}", msg); + } + + Ok(required) + } + /// Verifies various important properties about the assignment of which APIs /// are server-managed vs. client-managed. /// @@ -685,6 +785,10 @@ impl SystemApis { // can't be part of a cycle. let filter = ApiDependencyFilter::Default; + // Validate that all configured localhost_only_edges are correct and + // match the required set exactly. + let localhost_only_edges = self.validate_localhost_only_edges()?; + // Construct a graph where: // // - nodes are all the API producer and consumer components @@ -704,7 +808,8 @@ impl SystemApis { } // Do the same with a graph of deployment units. - let (graph, nodes) = self.make_deployment_unit_graph(filter, true)?; + let (graph, nodes) = self + .make_deployment_unit_graph(filter, Some(&localhost_only_edges))?; let reverse_nodes: BTreeMap<_, _> = nodes.iter().map(|(d_u, node)| (node, d_u)).collect(); if let Err(error) = petgraph::algo::toposort(&graph, None) { From d714b7dd54ac820dc0711833e5bbb6d59c651d38 Mon Sep 17 00:00:00 2001 From: Rain Date: Mon, 26 Jan 2026 23:20:06 +0000 Subject: [PATCH 05/19] use the dependency_filter_rules list, and add more notes and permalinks --- dev-tools/ls-apis/api-manifest.toml | 208 ++++++++++++++----- dev-tools/ls-apis/src/api_metadata.rs | 284 ++++++++++++++++---------- dev-tools/ls-apis/src/bin/ls-apis.rs | 6 +- dev-tools/ls-apis/src/system_apis.rs | 149 ++++++++------ 4 files changed, 432 insertions(+), 215 deletions(-) diff --git a/dev-tools/ls-apis/api-manifest.toml b/dev-tools/ls-apis/api-manifest.toml index 46adc8f0a70..ba098d5b829 100644 --- a/dev-tools/ls-apis/api-manifest.toml +++ b/dev-tools/ls-apis/api-manifest.toml @@ -594,96 +594,212 @@ Sled Agent uses the Crucible Agent client types only, and only in the simulated sled agent. """ -################################################################################ -# Localhost-only edges +# +# Same-deployment-unit edges # # These edges are excluded from the deployment unit dependency graph because # they represent communication that only happens locally within a deployment # unit, not across deployment unit boundaries. A single deployment unit is -# updated atomically, so there's no risk of version skew with localhost-only -# edges. -################################################################################ +# updated atomically, so there's no risk of version skew with these edges. +# +# These rules always use `server` (instead of `ancestor`) as the matcher, and +# `evaluation = "same-deployment-unit"`. +# -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "ddmd" client = "dpd-client" -note = "verified: sled-agent configures ddmd with dpd_host=[::1] (services.rs:3127)" +evaluation = "same-deployment-unit" +note = """ +ddmd runs in both the global zone and the switch zone. The switch zone is +configured with dpd_host=[::1] (services.rs:3127). The global zone configures +"dendrite" to false, meaning it does not expose a dendrite server +(ddm/manifest.xml:25). +""" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/services.rs#L3127", + "https://github.com/oxidecomputer/maghemite/blob/396bb3c/smf/ddm/manifest.xml#L25", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "dpd" client = "gateway-client" -note = "verified: dpd defaults to [::1]:MGS_PORT (dendrite switch_identifiers.rs:60)" +evaluation = "same-deployment-unit" +note = """ +dpd defaults to [::1]:MGS_PORT (switch_identifiers.rs:60), and the SMF start +script doesn't pass in --mgs-address. +""" +permalinks = [ + "https://github.com/oxidecomputer/dendrite/blob/606c0be/dpd/src/switch_identifiers.rs#L60", + "https://github.com/oxidecomputer/dendrite/blob/606c0be/dpd/misc/dpd.xml", + "https://github.com/oxidecomputer/dendrite/blob/606c0be/tools/svc-dpd", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "lldpd" client = "dpd-client" -note = "verified: lldpd defaults to localhost for dpd (lldp dendrite.rs:194)" +evaluation = "same-deployment-unit" +note = """ +lldpd defaults to localhost for dpd (dendrite.rs:194), and the SMF start +script doesn't pass in --host. +""" +permalinks = [ + "https://github.com/oxidecomputer/lldp/blob/61479b6/lldpd/src/dendrite.rs#L194", + "https://github.com/oxidecomputer/lldp/blob/61479b6/lldpd/misc/lldpd.xml", + "https://github.com/oxidecomputer/lldp/blob/61479b6/lldpd/misc/svc-lldpd", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "mgd" client = "ddm-admin-client" -note = "verified: mg-lower hardcodes localhost:8000 (mg-lower ddm.rs:110)" +evaluation = "same-deployment-unit" +note = "mg-lower hardcodes localhost:8000." +permalinks = [ + "https://github.com/oxidecomputer/maghemite/blob/396bb3c/mg-lower/src/ddm.rs#L110", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "mgd" client = "dpd-client" -note = "verified: mg-lower hardcodes localhost (mg-lower dendrite.rs:491)" +evaluation = "same-deployment-unit" +note = "mg-lower hardcodes localhost." +permalinks = [ + "https://github.com/oxidecomputer/maghemite/blob/396bb3c/mg-lower/src/dendrite.rs#L491", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "mgd" client = "gateway-client" -note = "verified: mgd defaults to [::1]:12225 (mgd main.rs:93)" - -# NOTE: The following sled-agent edges are BUGS - they use underlay IPs, not -# localhost. They are kept here to avoid breaking the build, but need to be -# fixed, either through client-side versioning or by some other means. +evaluation = "same-deployment-unit" +note = """ +mgd defaults to [::1]:12225 (mgd main.rs:93), and the SMF start script +doesn't set --mgs-addr. +""" +permalinks = [ + "https://github.com/oxidecomputer/maghemite/blob/396bb3c/mgd/src/main.rs#L93", + "https://github.com/oxidecomputer/maghemite/blob/396bb3c/smf/mgd/manifest.xml", + "https://github.com/oxidecomputer/maghemite/blob/396bb3c/smf/mgd_method_script.sh", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "omicron-sled-agent" -client = "gateway-client" -note = "BUG: uses underlay IP, not localhost (early_networking.rs:376)" +client = "ddm-admin-client" +evaluation = "same-deployment-unit" +note = "Sled Agent uses Client::localhost() to connect to ddm." +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/ddm_reconciler.rs#L56", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "omicron-sled-agent" -client = "ddm-admin-client" -note = "verified: uses Client::localhost() (ddm_reconciler.rs:56)" +client = "mg-admin-client" +evaluation = "same-deployment-unit" +note = """ +Sled Agent only queries the switch zone on the same sled, +which is within the same deployment unit as the global +zone. +""" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/bootstrap/early_networking.rs#L483", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "omicron-sled-agent" -client = "dpd-client" -note = "BUG: uses underlay IP, not localhost (services.rs:1090)" +client = "propolis-client" +evaluation = "same-deployment-unit" +note = "Unsure: it looks like this only queries the Propolis server out of the same deployment unit?" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/instance.rs#L2283", +] + +# NOTE: The following sled-agent edges are bugs: they cross deployment units. +# They are kept here to avoid breaking the build, but need to be fixed, either +# through client-side versioning or by some other means. -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "omicron-sled-agent" -client = "mg-admin-client" -note = "BUG: uses underlay IP, not localhost (early_networking.rs:483)" +client = "gateway-client" +evaluation = "same-deployment-unit" +note = """ +BUG: Sled Agent creates two MGS clients. One of them +(early_networking.rs:376) queries the switch zone on +the same sled, which is within the same deployment unit +as the global zone, so this is okay. + +The other one (early_networking.rs:277) queries both +switch zones, so it crosses deployment units. This needs +to be fixed. +""" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/bootstrap/early_networking.rs#L376", + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/bootstrap/early_networking.rs#L277-L280", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "omicron-sled-agent" -client = "propolis-client" -note = "BUG: uses zone IP, not localhost (instance.rs:2283)" +client = "dpd-client" +evaluation = "same-deployment-unit" +note = """ +BUG: Sled Agent reaches out to both switch zones, which crosses deployment units. +""" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/services.rs#L1090", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "tfportd" client = "dpd-client" -note = "verified: configured with dpd_host=[::1] (services.rs:2852)" +evaluation = "same-deployment-unit" +note = """ +The tfportd service has the dpd_host SMF property (tfport.xml:52) set to [::1] +(services.rs:2852). tfportd has a --dpd-host option (main.rs:81) but the SMF +start script (svc-tfportd:12) does not use it. +""" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/services.rs#L2852", + "https://github.com/oxidecomputer/dendrite/blob/606c0be/tfportd/misc/tfport.xml#L52-L53", + "https://github.com/oxidecomputer/dendrite/blob/cf31be9/tfportd/src/main.rs#L81-L83", + "https://github.com/oxidecomputer/dendrite/blob/606c0be/tools/svc-tfportd#L12", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "tfportd" client = "lldpd-client" -note = "verified: hardcodes localhost (tfportd tfport.rs:88)" +evaluation = "same-deployment-unit" +note = "tfportd hardcodes localhost for its lldpd-client (tfport.rs:88)." +permalinks = [ + "https://github.com/oxidecomputer/dendrite/blob/606c0be/tfportd/src/tfport.rs#L88", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "wicketd" client = "ddm-admin-client" -note = "verified: uses DdmAdminClient::localhost() (bootstrap_addrs.rs:162)" +evaluation = "same-deployment-unit" +note = "wicketd uses DdmAdminClient::localhost() (bootstrap_addrs.rs:162)." +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/wicketd/src/bootstrap_addrs.rs#L162", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "wicketd" client = "dpd-client" -note = "verified: hardcodes [::1]:DENDRITE_PORT (preflight_check/uplink.rs:83)" +evaluation = "same-deployment-unit" +note = "wicketd hardcodes [::1] (uplink.rs:83)." +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/wicketd/src/preflight_check/uplink.rs#L83", +] -[[localhost_only_edges]] +[[dependency_filter_rules]] server = "wicketd" client = "gateway-client" -note = "verified: --mgs-address CLI expects localhost (bin/wicketd.rs:35)" +evaluation = "same-deployment-unit" +note = """ +wicketd's mgs-address config (wicketd.rs:35) is always set to [::1] +(services.rs:2586). This config is passed into wicketd at startup +(manifest.xml:20). +""" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/services.rs#L2685-L2689", + "https://github.com/oxidecomputer/omicron/blob/6cef874/wicketd/src/bin/wicketd.rs#L35", + "https://github.com/oxidecomputer/omicron/blob/6cef874/smf/wicketd/manifest.xml#L20", +] diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index 3d0ff0052e2..8a7d510439c 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -30,7 +30,6 @@ pub struct AllApiMetadata { deployment_units: BTreeMap, dependency_rules: BTreeMap>, ignored_non_clients: BTreeSet, - localhost_only_edges: Vec, } impl AllApiMetadata { @@ -74,48 +73,91 @@ impl AllApiMetadata { &self.ignored_non_clients } - /// Returns the list of localhost-only edges - pub fn localhost_only_edges(&self) -> &[LocalhostOnlyEdge] { - &self.localhost_only_edges + /// Returns an iterator over rules that have `SameDeploymentUnit` + /// evaluation. + pub fn same_deployment_unit_rules( + &self, + ) -> impl Iterator { + self.dependency_rules + .values() + .flatten() + .filter(|r| r.evaluation == Evaluation::SameDeploymentUnit) } - /// Returns how we should filter the given dependency + /// Returns how we should filter the given dependency. + /// + /// Returns all matching evaluations: a dependency may match up to one + /// server rule and up to one ancestor rule. This allows filters to check if + /// any matching rule would cause exclusion. pub(crate) fn evaluate_dependency( &self, workspaces: &Workspaces, + server: &ServerComponentName, client_pkgname: &ClientPackageName, dep_path: &DepPath, - ) -> Result { + ) -> Result { let Some(rules) = self.dependency_rules.get(client_pkgname) else { - return Ok(Evaluation::default()); + return Ok(DependencyEvaluation::default()); }; - let which_rules: Vec<_> = rules + let mut result = DependencyEvaluation::default(); + + let server_rules: Vec<_> = rules .iter() - .filter(|r| { - assert_eq!(r.client, *client_pkgname); - let pkgids = workspaces.workspace_pkgids(&r.ancestor); - dep_path.contains_any(&pkgids) + .filter(|r| match &r.matcher { + RuleMatcher::Server(s) => s == server, + RuleMatcher::Ancestor(_) => false, }) .collect(); - if which_rules.is_empty() { - return Ok(Evaluation::default()); + if server_rules.len() > 1 { + bail!( + "client package {:?}: dependency matched multiple server \ + filters for server {:?}", + client_pkgname, + server + ); + } + + if let Some(rule) = server_rules.first() { + result.server = Some(rule.evaluation); } - if which_rules.len() > 1 { + let ancestor_rules: Vec<_> = rules + .iter() + .filter(|r| { + assert_eq!(r.client, *client_pkgname); + match &r.matcher { + RuleMatcher::Ancestor(ancestor) => { + let pkgids = workspaces.workspace_pkgids(ancestor); + dep_path.contains_any(&pkgids) + } + RuleMatcher::Server(_) => false, + } + }) + .collect(); + + if ancestor_rules.len() > 1 { bail!( - "client package {:?}: dependency matched multiple filters: {}", + "client package {:?}: dependency matched multiple ancestor \ + filters: {}", client_pkgname, - which_rules + ancestor_rules .into_iter() - .map(|r| r.ancestor.as_str()) + .map(|r| match &r.matcher { + RuleMatcher::Ancestor(a) => a.as_str(), + RuleMatcher::Server(_) => unreachable!(), + }) .collect::>() .join(", ") ); } - Ok(which_rules[0].evaluation) + if let Some(rule) = ancestor_rules.first() { + result.ancestor = Some(rule.evaluation); + } + + Ok(result) } /// Returns the list of APIs that have non-DAG dependency rules @@ -142,9 +184,8 @@ impl AllApiMetadata { struct RawApiMetadata { apis: Vec, deployment_units: Vec, - dependency_filter_rules: Vec, + dependency_filter_rules: Vec, ignored_non_clients: Vec, - localhost_only_edges: Vec, } impl TryFrom for AllApiMetadata { @@ -176,15 +217,33 @@ impl TryFrom for AllApiMetadata { } } + // Build set of known server components for validation. + let known_components: BTreeSet<_> = + deployment_units.values().flat_map(|u| u.packages.iter()).collect(); + let mut dependency_rules = BTreeMap::new(); - for rule in raw.dependency_filter_rules { - if !apis.contains_key(&rule.client) { + for raw_rule in raw.dependency_filter_rules { + if !apis.contains_key(&raw_rule.client) { bail!( "dependency rule references unknown client: {:?}", - rule.client + raw_rule.client ); } + let rule = DependencyFilterRule::try_from_raw(raw_rule)?; + + // Validate that server matchers reference known server components. + if let RuleMatcher::Server(ref server) = rule.matcher { + if !known_components.contains(server) { + bail!( + "dependency filter rule for client {:?}: \ + unknown server component {:?}", + rule.client, + server + ); + } + } + dependency_rules .entry(rule.client.clone()) .or_insert_with(Vec::new) @@ -201,33 +260,24 @@ impl TryFrom for AllApiMetadata { } } - // Validate localhost_only_edges reference known server components and - // APIs. - let known_components: BTreeSet<_> = - deployment_units.values().flat_map(|u| u.packages.iter()).collect(); - for edge in &raw.localhost_only_edges { - if !known_components.contains(&edge.server) { - bail!( - "localhost_only_edges: unknown server component {:?}", - edge.server - ); - } - let client_name = edge.client.as_specific(); - if !apis.contains_key(client_name) { - bail!("localhost_only_edges: unknown client {:?}", client_name); - } - } - Ok(AllApiMetadata { apis, deployment_units, dependency_rules, ignored_non_clients, - localhost_only_edges: raw.localhost_only_edges, }) } } +/// Specifies which aspect of a dependency to match against. +#[derive(Clone, Debug)] +pub enum RuleMatcher { + /// Match when the dependency path contains this ancestor package. + Ancestor(String), + /// Match when the server component equals this value. + Server(ServerComponentName), +} + /// Describes how an API in the system manages drift between client and server #[derive(Deserialize, PartialEq, Eq)] #[serde(rename_all = "snake_case")] @@ -409,19 +459,94 @@ pub struct DeploymentUnitInfo { pub packages: Vec, } +/// Raw deserialization struct for dependency filter rules. +/// +/// This is validated into `DependencyFilterRule`. #[derive(Deserialize)] #[serde(deny_unknown_fields)] +struct RawDependencyFilterRule { + // Only one of `ancestor` and `server` should be specified. + ancestor: Option, + server: Option, + client: ClientPackageName, + #[serde(default)] + evaluation: Evaluation, + note: String, + #[serde(default)] + permalinks: Vec, +} + +/// A validated dependency filter rule. +#[derive(Clone, Debug)] pub struct DependencyFilterRule { - pub ancestor: String, + pub matcher: RuleMatcher, pub client: ClientPackageName, - #[serde(default)] pub evaluation: Evaluation, - // These notes are not currently used, but they are required. They could as - // well just be TOML comments. But it seems nice to enforce that they're - // present. And this would let us include this explanation in output in the - // future (e.g., to explain why some dependency was filtered out). - #[allow(dead_code)] + // These notes and permalinks are not currently used, but they are required. + // They could as well just be TOML comments. But it seems nice to enforce + // that they're present. And this would let us include this explanation in + // output in the future (e.g., to explain why some dependency was filtered + // out). + #[expect(dead_code)] pub note: String, + #[expect(dead_code)] + pub permalinks: Vec, +} + +impl DependencyFilterRule { + fn try_from_raw(raw: RawDependencyFilterRule) -> Result { + let matcher = match (raw.ancestor, raw.server) { + (Some(a), None) => RuleMatcher::Ancestor(a), + (None, Some(s)) => RuleMatcher::Server(s), + (Some(_), Some(_)) => bail!( + "dependency filter rule for client {:?} cannot have both \ + 'ancestor' and 'server'", + raw.client + ), + (None, None) => bail!( + "dependency filter rule for client {:?} must have either \ + 'ancestor' or 'server'", + raw.client + ), + }; + + // SameDeploymentUnit requires the server matcher since it goes through + // explicit validation. + if raw.evaluation == Evaluation::SameDeploymentUnit { + if !matches!(matcher, RuleMatcher::Server(_)) { + bail!( + "dependency filter rule for client {:?}: \ + 'same-deployment-unit' evaluation requires 'server' field, found {:?}", + raw.client, + matcher, + ); + } + } + + Ok(DependencyFilterRule { + matcher, + client: raw.client, + evaluation: raw.evaluation, + note: raw.note, + permalinks: raw.permalinks, + }) + } +} + +/// Result of evaluating a dependency against all matching rules. +#[derive(Clone, Debug, Default)] +pub struct DependencyEvaluation { + /// Evaluation from a server rule, if one matched. + pub server: Option, + /// Evaluation from an ancestor rule, if one matched. + pub ancestor: Option, +} + +impl DependencyEvaluation { + /// Returns true if any matching rule has the given evaluation. + pub fn any_matches(&self, target: Evaluation) -> bool { + self.server == Some(target) || self.ancestor == Some(target) + } } #[derive(Clone, Copy, Debug, Default, Deserialize, Eq, PartialEq)] @@ -440,62 +565,7 @@ pub enum Evaluation { NonDag, /// This dependency should be part of the update DAG Dag, -} - -/// Specifies which client to match in a localhost-only edge rule. -#[derive(Clone, Debug, Eq, PartialEq)] -pub enum ClientMatcher { - /// Match a specific client package. - Specific(ClientPackageName), -} - -impl<'de> Deserialize<'de> for ClientMatcher { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - let s = String::deserialize(deserializer)?; - Ok(ClientMatcher::Specific(ClientPackageName::from(s))) - } -} - -impl ClientMatcher { - /// Returns true if this matcher matches the given client package. - pub fn matches(&self, client: &ClientPackageName) -> bool { - match self { - ClientMatcher::Specific(name) => name == client, - } - } - - /// Returns the specific client name. - pub fn as_specific(&self) -> &ClientPackageName { - match self { - ClientMatcher::Specific(name) => name, - } - } -} - -/// An edge that should be excluded from the deployment unit dependency graph -/// because it represents communication that only happens locally within a -/// deployment unit. -#[derive(Deserialize)] -#[serde(deny_unknown_fields)] -pub struct LocalhostOnlyEdge { - /// The server component that consumes the API. - pub server: ServerComponentName, - /// The client package consumed. - pub client: ClientMatcher, - /// Explanation of why this edge is localhost-only. - pub note: String, -} - -impl LocalhostOnlyEdge { - /// Returns true if this rule matches the given (server, client) pair. - pub fn matches( - &self, - server: &ServerComponentName, - client: &ClientPackageName, - ) -> bool { - self.server == *server && self.client.matches(client) - } + /// Intra-deployment-unit communication. Only valid with the `server` + /// matcher and server-side-versioned APIs. Subject to auto-validation. + SameDeploymentUnit, } diff --git a/dev-tools/ls-apis/src/bin/ls-apis.rs b/dev-tools/ls-apis/src/bin/ls-apis.rs index 54169e076c0..aaf07f9ee69 100644 --- a/dev-tools/ls-apis/src/bin/ls-apis.rs +++ b/dev-tools/ls-apis/src/bin/ls-apis.rs @@ -252,10 +252,10 @@ fn print_server_components<'a>( ); } for (c, path) in apis.component_apis_consumed(s, filter)? { - if let Some(note) = apis.localhost_only_edge_note(s, c) { + if apis.is_same_deployment_unit_edge(s, c) { println!( - "{} consumes: {} (localhost-only: {})", - prefix, c, note + "{} consumes: {} (same-deployment-unit)", + prefix, c ); } else { println!("{} consumes: {}", prefix, c); diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index 56f0e179483..d854c785353 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -15,8 +15,8 @@ use crate::api_metadata::ApiConsumerStatus; use crate::api_metadata::ApiExpectedConsumer; use crate::api_metadata::ApiExpectedConsumers; use crate::api_metadata::ApiMetadata; -use crate::api_metadata::ClientMatcher; use crate::api_metadata::Evaluation; +use crate::api_metadata::RuleMatcher; use crate::api_metadata::VersionedHow; use crate::cargo::DepPath; use crate::parse_toml_file; @@ -30,6 +30,7 @@ use iddqd::IdOrdMap; use iddqd::id_upcast; use parse_display::{Display, FromStr}; use petgraph::dot::Dot; +use petgraph::graph::Graph; use petgraph::graph::NodeIndex; use std::collections::BTreeMap; use std::collections::BTreeSet; @@ -331,17 +332,22 @@ impl SystemApis { &self.api_metadata } - /// Returns the note for a localhost-only edge, if one matches. - pub fn localhost_only_edge_note( + /// Returns true if this (server, client) edge is a same-deployment-unit + /// edge. + pub fn is_same_deployment_unit_edge( &self, server: &ServerComponentName, client: &ClientPackageName, - ) -> Option<&str> { - self.api_metadata - .localhost_only_edges() - .iter() - .find(|edge| edge.matches(server, client)) - .map(|edge| edge.note.as_str()) + ) -> bool { + self.api_metadata.same_deployment_unit_rules().any(|rule| { + if rule.client != *client { + return false; + } + match &rule.matcher { + RuleMatcher::Server(s) => s == server, + RuleMatcher::Ancestor(_) => false, + } + }) } /// Given a server component, return the APIs consumed by this component @@ -364,6 +370,7 @@ impl SystemApis { if filter.should_include( &self.api_metadata, &self.workspaces, + server_component, client_pkgname, p, )? { @@ -412,7 +419,8 @@ impl SystemApis { if filter.should_include( &self.api_metadata, &self.workspaces, - &client, + &api_consumer.server_pkgname, + client, p, )? { include.push(p); @@ -488,24 +496,23 @@ impl SystemApis { /// Returns a string that can be passed to `dot(1)` to render a graph of /// API dependencies among deployment units pub fn dot_by_unit(&self, filter: ApiDependencyFilter) -> Result { - let (graph, _) = self.make_deployment_unit_graph(filter, None)?; + let (graph, _) = + self.make_deployment_unit_graph(filter, VersionedHowFilter::All)?; Ok(Dot::new(&graph).to_string()) } // The complex type below is only used in this one place: the return value // of this internal helper function. A type alias doesn't seem better. - #[allow(clippy::type_complexity)] + #[expect(clippy::type_complexity)] fn make_deployment_unit_graph( &self, dependency_filter: ApiDependencyFilter, - localhost_only_edges: Option< - &BTreeSet<(ServerComponentName, ClientPackageName)>, - >, + versioned_how_filter: VersionedHowFilter<'_>, ) -> Result<( - petgraph::graph::Graph<&DeploymentUnitName, &ClientPackageName>, + Graph<&DeploymentUnitName, &ClientPackageName>, BTreeMap<&DeploymentUnitName, NodeIndex>, )> { - let mut graph = petgraph::graph::Graph::new(); + let mut graph = Graph::new(); let nodes: BTreeMap<_, _> = self .deployment_units() .map(|name| (name, graph.add_node(name))) @@ -522,9 +529,13 @@ impl SystemApis { for (client_pkg, _) in self.component_apis_consumed(server_pkg, dependency_filter)? { - // When localhost_only_edges is provided, we're building a - // graph of server-side-versioned API dependencies only. - if let Some(localhost_edges) = localhost_only_edges { + // When building a server-side-versioned-only graph, + // filter to only server-versioned APIs and skip edges + // that represent same-deployment-unit communication. + if let VersionedHowFilter::ServerSideOnly { + same_unit_edges, + } = &versioned_how_filter + { let api = self .api_metadata .client_pkgname_lookup(client_pkg) @@ -533,10 +544,7 @@ impl SystemApis { continue; } - // Skip edges that represent localhost-only communication - // (communication within a deployment unit that doesn't - // cross deployment unit boundaries). - if localhost_edges + if same_unit_edges .contains(&(server_pkg.clone(), client_pkg.clone())) { continue; @@ -585,10 +593,10 @@ impl SystemApis { dependency_filter: ApiDependencyFilter, versioned_on_server_only: bool, ) -> Result<( - petgraph::graph::Graph<&ServerComponentName, &ClientPackageName>, + Graph<&ServerComponentName, &ClientPackageName>, BTreeMap<&ServerComponentName, NodeIndex>, )> { - let mut graph = petgraph::graph::Graph::new(); + let mut graph = Graph::new(); let nodes: BTreeMap<_, _> = self .server_component_units .keys() @@ -639,7 +647,11 @@ impl SystemApis { fn compute_required_localhost_edges( &self, ) -> Result> { - let filter = ApiDependencyFilter::Default; + // Use the IncludeNonDag filter to include SameDeploymentUnit edges. We + // need to see all real edges (excluding only the Bogus and NotDeployed + // ones) to determine which ones should be in the same-deployment-unit + // set. + let filter = ApiDependencyFilter::IncludeNonDag; let mut required = BTreeSet::new(); for server in self.server_component_units.keys() { @@ -680,33 +692,37 @@ impl SystemApis { Ok(required) } - /// Validates that the configured localhost_only_edges exactly match the - /// required set of edges that must be excluded. + /// Validates that the configured same-deployment-unit rules exactly match + /// the required set of edges that must be excluded. /// - /// Returns the validated set of edges for use by make_deployment_unit_graph. - fn validate_localhost_only_edges( + /// Returns the validated set of edges for use by `make_deployment_unit_graph`. + fn validate_same_deployment_unit_rules( &self, ) -> Result> { let required = self.compute_required_localhost_edges()?; // Build the configured set from the manifest. let mut configured = BTreeSet::new(); - for edge in self.api_metadata.localhost_only_edges() { - let ClientMatcher::Specific(client) = &edge.client; - configured.insert((edge.server.clone(), client.clone())); + for rule in self.api_metadata.same_deployment_unit_rules() { + let RuleMatcher::Server(server) = &rule.matcher else { + unreachable!( + "same-deployment-unit rules must have server matcher \ + (validated in api_metadata.rs)" + ); + }; + configured.insert((server.clone(), rule.client.clone())); } - // Compare the two sets. let missing: BTreeSet<_> = required.difference(&configured).collect(); let extra: BTreeSet<_> = configured.difference(&required).collect(); if !missing.is_empty() || !extra.is_empty() { let mut msg = String::from( - "localhost_only_edges configuration does not match required edges:\n", + "same-deployment-unit rules do not match required edges:\n", ); if !missing.is_empty() { - msg.push_str("\nMissing entries (these edges exist and need localhost-only exclusion):\n"); + msg.push_str("\nmissing entries (these edges exist and need same-deployment-unit exclusion):\n"); for (server, client) in &missing { msg.push_str(&format!( " - server = {:?}, client = {:?}\n", @@ -716,7 +732,7 @@ impl SystemApis { } if !extra.is_empty() { - msg.push_str("\nExtra entries (these edges don't exist or don't need exclusion):\n"); + msg.push_str("\nextra entries (these edges don't exist or don't need exclusion):\n"); for (server, client) in &extra { msg.push_str(&format!( " - server = {:?}, client = {:?}\n", @@ -785,9 +801,9 @@ impl SystemApis { // can't be part of a cycle. let filter = ApiDependencyFilter::Default; - // Validate that all configured localhost_only_edges are correct and - // match the required set exactly. - let localhost_only_edges = self.validate_localhost_only_edges()?; + // Validate that all configured same-deployment-unit rules are correct + // and match the required set exactly. + let same_unit_edges = self.validate_same_deployment_unit_rules()?; // Construct a graph where: // @@ -808,8 +824,12 @@ impl SystemApis { } // Do the same with a graph of deployment units. - let (graph, nodes) = self - .make_deployment_unit_graph(filter, Some(&localhost_only_edges))?; + let (graph, nodes) = self.make_deployment_unit_graph( + filter, + VersionedHowFilter::ServerSideOnly { + same_unit_edges: &same_unit_edges, + }, + )?; let reverse_nodes: BTreeMap<_, _> = nodes.iter().map(|(d_u, node)| (node, d_u)).collect(); if let Err(error) = petgraph::algo::toposort(&graph, None) { @@ -1414,6 +1434,19 @@ impl<'a> ClientDependenciesTracker<'a> { } } +/// Specifies whether to filter to only server-versioned APIs when building +/// dependency graphs. +#[derive(Clone, Debug)] +pub enum VersionedHowFilter<'a> { + /// Include all APIs regardless of versioning strategy. + All, + /// Include only APIs with server-side versioning, and skip edges that + /// represent same-deployment-unit communication. + ServerSideOnly { + same_unit_edges: &'a BTreeSet<(ServerComponentName, ClientPackageName)>, + }, +} + /// Specifies which API dependencies to include vs. ignore when iterating /// dependencies #[derive(Clone, Copy, Debug, Default, Display, FromStr)] @@ -1455,31 +1488,29 @@ impl ApiDependencyFilter { &self, api_metadata: &AllApiMetadata, workspaces: &Workspaces, + server: &ServerComponentName, client_pkgname: &ClientPackageName, dep_path: &DepPath, ) -> Result { - let evaluation = api_metadata - .evaluate_dependency(workspaces, client_pkgname, dep_path) + let eval = api_metadata + .evaluate_dependency(workspaces, server, client_pkgname, dep_path) .with_context(|| format!("error applying filter {:?}", self))?; Ok(match self { ApiDependencyFilter::All => true, - ApiDependencyFilter::Bogus => { - matches!(evaluation, Evaluation::Bogus) - } + ApiDependencyFilter::Bogus => eval.any_matches(Evaluation::Bogus), ApiDependencyFilter::NonBogus => { - !matches!(evaluation, Evaluation::Bogus) + !eval.any_matches(Evaluation::Bogus) + } + ApiDependencyFilter::IncludeNonDag => { + !eval.any_matches(Evaluation::Bogus) + && !eval.any_matches(Evaluation::NotDeployed) + } + ApiDependencyFilter::Default => { + !eval.any_matches(Evaluation::NonDag) + && !eval.any_matches(Evaluation::Bogus) + && !eval.any_matches(Evaluation::NotDeployed) } - ApiDependencyFilter::IncludeNonDag => !matches!( - evaluation, - Evaluation::Bogus | Evaluation::NotDeployed - ), - ApiDependencyFilter::Default => !matches!( - evaluation, - Evaluation::NonDag - | Evaluation::Bogus - | Evaluation::NotDeployed - ), }) } } From 0e0857803460bd7e907a70445e8d42d54d588216 Mon Sep 17 00:00:00 2001 From: Rain Date: Thu, 29 Jan 2026 00:23:00 +0000 Subject: [PATCH 06/19] self-review fixes --- dev-tools/ls-apis/src/api_metadata.rs | 9 +++++++-- dev-tools/ls-apis/src/system_apis.rs | 15 +++++++++------ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index 8a7d510439c..b64c58b4178 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -487,9 +487,7 @@ pub struct DependencyFilterRule { // that they're present. And this would let us include this explanation in // output in the future (e.g., to explain why some dependency was filtered // out). - #[expect(dead_code)] pub note: String, - #[expect(dead_code)] pub permalinks: Vec, } @@ -567,5 +565,12 @@ pub enum Evaluation { Dag, /// Intra-deployment-unit communication. Only valid with the `server` /// matcher and server-side-versioned APIs. Subject to auto-validation. + /// + /// Note: This evaluation is intentionally *not* filtered out by + /// `ApiDependencyFilter`. These edges are real dependencies that should + /// appear in component listings and most graphs. They are only excluded + /// when building the deployment unit dependency graph for cycle detection + /// (via `VersionedHowFilter::ServerSideOnly`), because intra-unit + /// communication doesn't create cross-unit version skew. SameDeploymentUnit, } diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index d854c785353..ee58884d791 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -654,11 +654,7 @@ impl SystemApis { let filter = ApiDependencyFilter::IncludeNonDag; let mut required = BTreeSet::new(); - for server in self.server_component_units.keys() { - let Some(server_unit) = self.server_component_units.get(server) - else { - continue; - }; + for (server, server_unit) in &self.server_component_units { for (client, _) in self.component_apis_consumed(server, filter)? { // Only consider server-side-versioned APIs. @@ -1483,7 +1479,14 @@ pub enum ApiDependencyFilter { impl ApiDependencyFilter { /// Return whether this filter should include a dependency on - /// `client_pkgname` that goes through dependency path `dep_path` + /// `client_pkgname` that goes through dependency path `dep_path`. + /// + /// Note: `Evaluation::SameDeploymentUnit` edges are included by all filters + /// here. These edges represent real dependencies and should appear in + /// component listings and graphs. They are only excluded when building the + /// deployment unit graph for cycle detection, which is handled separately + /// via `VersionedHowFilter::ServerSideOnly` in + /// `make_deployment_unit_graph`. fn should_include( &self, api_metadata: &AllApiMetadata, From c74b4293d058c2885683edc08315b7ad0a162ec8 Mon Sep 17 00:00:00 2001 From: Rain Date: Thu, 29 Jan 2026 19:55:44 +0000 Subject: [PATCH 07/19] more self-review fixes --- Cargo.lock | 1 + dev-tools/ls-apis/Cargo.toml | 1 + dev-tools/ls-apis/src/api_metadata.rs | 15 ++-- dev-tools/ls-apis/src/system_apis.rs | 116 +++++++++++++------------- 4 files changed, 69 insertions(+), 64 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9637a9a9263..d338dbe408e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8342,6 +8342,7 @@ dependencies = [ "petgraph 0.8.2", "serde", "subprocess", + "swrite", "toml 0.8.23", ] diff --git a/dev-tools/ls-apis/Cargo.toml b/dev-tools/ls-apis/Cargo.toml index 4b24b7d1b7d..de824349a1d 100644 --- a/dev-tools/ls-apis/Cargo.toml +++ b/dev-tools/ls-apis/Cargo.toml @@ -18,6 +18,7 @@ newtype_derive.workspace = true parse-display.workspace = true petgraph.workspace = true serde.workspace = true +swrite.workspace = true toml.workspace = true omicron-workspace-hack.workspace = true diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index b64c58b4178..9b0d0ab4ea5 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -86,9 +86,8 @@ impl AllApiMetadata { /// Returns how we should filter the given dependency. /// - /// Returns all matching evaluations: a dependency may match up to one - /// server rule and up to one ancestor rule. This allows filters to check if - /// any matching rule would cause exclusion. + /// A dependency may match up to one server rule and up to one ancestor + /// rule. Filters can then check if any matching rule would cause exclusion. pub(crate) fn evaluate_dependency( &self, workspaces: &Workspaces, @@ -482,11 +481,11 @@ pub struct DependencyFilterRule { pub matcher: RuleMatcher, pub client: ClientPackageName, pub evaluation: Evaluation, - // These notes and permalinks are not currently used, but they are required. - // They could as well just be TOML comments. But it seems nice to enforce - // that they're present. And this would let us include this explanation in - // output in the future (e.g., to explain why some dependency was filtered - // out). + // These notes and permalinks are not currently used, but are stored in + // api-manifest.toml (and notes are required). They could as well just be + // TOML comments. But it seems nice to enforce that notes are present. And + // this would let us include this explanation in output in the future (e.g., + // to explain why some dependency was filtered out). pub note: String, pub permalinks: Vec, } diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index ee58884d791..87448dad665 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -35,6 +35,7 @@ use petgraph::graph::NodeIndex; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::fmt; +use swrite::{SWrite, swriteln}; /// Query information about the Dropshot/OpenAPI/Progenitor-based APIs within /// the Oxide system @@ -63,6 +64,10 @@ pub struct SystemApis { /// maps an API name to the server component(s) that expose that API api_producers: BTreeMap, + /// index of (server, client) pairs configured as same-deployment-unit edges + same_deployment_unit_edges: + BTreeSet<(ServerComponentName, ClientPackageName)>, + /// source of developer-maintained API metadata api_metadata: AllApiMetadata, /// source of Cargo package metadata @@ -126,6 +131,22 @@ impl SystemApis { // Load the API manifest. let api_metadata: AllApiMetadata = parse_toml_file(&args.api_manifest_path)?; + + // Build an index of configured same-deployment-unit edges. We use this + // index for validation and lookups. + let same_deployment_unit_edges: BTreeSet<_> = api_metadata + .same_deployment_unit_rules() + .filter_map(|rule| match &rule.matcher { + RuleMatcher::Server(server) => { + Some((server.clone(), rule.client.clone())) + } + RuleMatcher::Ancestor(_) => { + // This case is rejected at API construction time. + None + } + }) + .collect(); + // Load Cargo metadata and validate it against the manifest. let (workspaces, warnings) = Workspaces::load(&api_metadata)?; if !warnings.is_empty() { @@ -295,6 +316,7 @@ impl SystemApis { api_consumers, missing_expected_consumers, api_producers, + same_deployment_unit_edges, api_metadata, workspaces, }) @@ -339,15 +361,8 @@ impl SystemApis { server: &ServerComponentName, client: &ClientPackageName, ) -> bool { - self.api_metadata.same_deployment_unit_rules().any(|rule| { - if rule.client != *client { - return false; - } - match &rule.matcher { - RuleMatcher::Server(s) => s == server, - RuleMatcher::Ancestor(_) => false, - } - }) + self.same_deployment_unit_edges + .contains(&(server.clone(), client.clone())) } /// Given a server component, return the APIs consumed by this component @@ -507,7 +522,7 @@ impl SystemApis { fn make_deployment_unit_graph( &self, dependency_filter: ApiDependencyFilter, - versioned_how_filter: VersionedHowFilter<'_>, + versioned_how_filter: VersionedHowFilter, ) -> Result<( Graph<&DeploymentUnitName, &ClientPackageName>, BTreeMap<&DeploymentUnitName, NodeIndex>, @@ -532,9 +547,8 @@ impl SystemApis { // When building a server-side-versioned-only graph, // filter to only server-versioned APIs and skip edges // that represent same-deployment-unit communication. - if let VersionedHowFilter::ServerSideOnly { - same_unit_edges, - } = &versioned_how_filter + if let VersionedHowFilter::ServerSideOnly = + versioned_how_filter { let api = self .api_metadata @@ -544,7 +558,8 @@ impl SystemApis { continue; } - if same_unit_edges + if self + .same_deployment_unit_edges .contains(&(server_pkg.clone(), client_pkg.clone())) { continue; @@ -644,7 +659,7 @@ impl SystemApis { /// 3. The server and API producer are in the same deployment unit. /// /// Returns a set of (server, client) pairs. - fn compute_required_localhost_edges( + fn compute_required_same_deployment_unit_edges( &self, ) -> Result> { // Use the IncludeNonDag filter to include SameDeploymentUnit edges. We @@ -655,7 +670,6 @@ impl SystemApis { let mut required = BTreeSet::new(); for (server, server_unit) in &self.server_component_units { - for (client, _) in self.component_apis_consumed(server, filter)? { // Only consider server-side-versioned APIs. let Some(api) = self.api_metadata.client_pkgname_lookup(client) @@ -677,7 +691,7 @@ impl SystemApis { if server_unit == producer_unit { // This edge would create an intra-unit dependency for // a server-versioned API, so it must be in the - // localhost-only list. + // same-deployment-unit-only set. required.insert((server.clone(), client.clone())); break; } @@ -690,26 +704,11 @@ impl SystemApis { /// Validates that the configured same-deployment-unit rules exactly match /// the required set of edges that must be excluded. - /// - /// Returns the validated set of edges for use by `make_deployment_unit_graph`. - fn validate_same_deployment_unit_rules( - &self, - ) -> Result> { - let required = self.compute_required_localhost_edges()?; - - // Build the configured set from the manifest. - let mut configured = BTreeSet::new(); - for rule in self.api_metadata.same_deployment_unit_rules() { - let RuleMatcher::Server(server) = &rule.matcher else { - unreachable!( - "same-deployment-unit rules must have server matcher \ - (validated in api_metadata.rs)" - ); - }; - configured.insert((server.clone(), rule.client.clone())); - } + fn validate_same_deployment_unit_rules(&self) -> Result<()> { + let required = self.compute_required_same_deployment_unit_edges()?; + let configured = &self.same_deployment_unit_edges; - let missing: BTreeSet<_> = required.difference(&configured).collect(); + let missing: BTreeSet<_> = required.difference(configured).collect(); let extra: BTreeSet<_> = configured.difference(&required).collect(); if !missing.is_empty() || !extra.is_empty() { @@ -718,29 +717,37 @@ impl SystemApis { ); if !missing.is_empty() { - msg.push_str("\nmissing entries (these edges exist and need same-deployment-unit exclusion):\n"); + swriteln!( + msg, + "\nmissing entries (these edges exist and need \ + same-deployment-unit exclusion):" + ); for (server, client) in &missing { - msg.push_str(&format!( - " - server = {:?}, client = {:?}\n", - server, client - )); + swriteln!( + msg, + " - server = {server:?}, client = {client:?}" + ); } } if !extra.is_empty() { - msg.push_str("\nextra entries (these edges don't exist or don't need exclusion):\n"); + swriteln!( + msg, + "\nextra entries (these edges don't exist or don't need \ + exclusion):" + ); for (server, client) in &extra { - msg.push_str(&format!( - " - server = {:?}, client = {:?}\n", - server, client - )); + swriteln!( + msg, + " - server = {server:?}, client = {client:?}" + ); } } bail!("{}", msg); } - Ok(required) + Ok(()) } /// Verifies various important properties about the assignment of which APIs @@ -799,7 +806,7 @@ impl SystemApis { // Validate that all configured same-deployment-unit rules are correct // and match the required set exactly. - let same_unit_edges = self.validate_same_deployment_unit_rules()?; + self.validate_same_deployment_unit_rules()?; // Construct a graph where: // @@ -822,9 +829,7 @@ impl SystemApis { // Do the same with a graph of deployment units. let (graph, nodes) = self.make_deployment_unit_graph( filter, - VersionedHowFilter::ServerSideOnly { - same_unit_edges: &same_unit_edges, - }, + VersionedHowFilter::ServerSideOnly, )?; let reverse_nodes: BTreeMap<_, _> = nodes.iter().map(|(d_u, node)| (node, d_u)).collect(); @@ -1433,14 +1438,12 @@ impl<'a> ClientDependenciesTracker<'a> { /// Specifies whether to filter to only server-versioned APIs when building /// dependency graphs. #[derive(Clone, Debug)] -pub enum VersionedHowFilter<'a> { +pub enum VersionedHowFilter { /// Include all APIs regardless of versioning strategy. All, /// Include only APIs with server-side versioning, and skip edges that /// represent same-deployment-unit communication. - ServerSideOnly { - same_unit_edges: &'a BTreeSet<(ServerComponentName, ClientPackageName)>, - }, + ServerSideOnly, } /// Specifies which API dependencies to include vs. ignore when iterating @@ -1479,7 +1482,8 @@ pub enum ApiDependencyFilter { impl ApiDependencyFilter { /// Return whether this filter should include a dependency on - /// `client_pkgname` that goes through dependency path `dep_path`. + /// `client_pkgname` that goes either through `server` or through + /// `dep_path`. /// /// Note: `Evaluation::SameDeploymentUnit` edges are included by all filters /// here. These edges represent real dependencies and should appear in From c2e885ac2ac7e06348a65f5e87389584d86f040e Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 5 Feb 2026 14:08:39 -0800 Subject: [PATCH 08/19] wind back the clock to when the new metadata was a new type of thing at the top level of the manifest --- Cargo.lock | 1 - dev-tools/ls-apis/Cargo.toml | 1 - dev-tools/ls-apis/api-manifest.toml | 208 ++++--------------- dev-tools/ls-apis/src/api_metadata.rs | 288 ++++++++++---------------- dev-tools/ls-apis/src/bin/ls-apis.rs | 6 +- dev-tools/ls-apis/src/system_apis.rs | 210 ++++++++----------- 6 files changed, 242 insertions(+), 472 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d338dbe408e..9637a9a9263 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8342,7 +8342,6 @@ dependencies = [ "petgraph 0.8.2", "serde", "subprocess", - "swrite", "toml 0.8.23", ] diff --git a/dev-tools/ls-apis/Cargo.toml b/dev-tools/ls-apis/Cargo.toml index de824349a1d..4b24b7d1b7d 100644 --- a/dev-tools/ls-apis/Cargo.toml +++ b/dev-tools/ls-apis/Cargo.toml @@ -18,7 +18,6 @@ newtype_derive.workspace = true parse-display.workspace = true petgraph.workspace = true serde.workspace = true -swrite.workspace = true toml.workspace = true omicron-workspace-hack.workspace = true diff --git a/dev-tools/ls-apis/api-manifest.toml b/dev-tools/ls-apis/api-manifest.toml index ba098d5b829..46adc8f0a70 100644 --- a/dev-tools/ls-apis/api-manifest.toml +++ b/dev-tools/ls-apis/api-manifest.toml @@ -594,212 +594,96 @@ Sled Agent uses the Crucible Agent client types only, and only in the simulated sled agent. """ -# -# Same-deployment-unit edges +################################################################################ +# Localhost-only edges # # These edges are excluded from the deployment unit dependency graph because # they represent communication that only happens locally within a deployment # unit, not across deployment unit boundaries. A single deployment unit is -# updated atomically, so there's no risk of version skew with these edges. -# -# These rules always use `server` (instead of `ancestor`) as the matcher, and -# `evaluation = "same-deployment-unit"`. -# +# updated atomically, so there's no risk of version skew with localhost-only +# edges. +################################################################################ -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "ddmd" client = "dpd-client" -evaluation = "same-deployment-unit" -note = """ -ddmd runs in both the global zone and the switch zone. The switch zone is -configured with dpd_host=[::1] (services.rs:3127). The global zone configures -"dendrite" to false, meaning it does not expose a dendrite server -(ddm/manifest.xml:25). -""" -permalinks = [ - "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/services.rs#L3127", - "https://github.com/oxidecomputer/maghemite/blob/396bb3c/smf/ddm/manifest.xml#L25", -] +note = "verified: sled-agent configures ddmd with dpd_host=[::1] (services.rs:3127)" -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "dpd" client = "gateway-client" -evaluation = "same-deployment-unit" -note = """ -dpd defaults to [::1]:MGS_PORT (switch_identifiers.rs:60), and the SMF start -script doesn't pass in --mgs-address. -""" -permalinks = [ - "https://github.com/oxidecomputer/dendrite/blob/606c0be/dpd/src/switch_identifiers.rs#L60", - "https://github.com/oxidecomputer/dendrite/blob/606c0be/dpd/misc/dpd.xml", - "https://github.com/oxidecomputer/dendrite/blob/606c0be/tools/svc-dpd", -] +note = "verified: dpd defaults to [::1]:MGS_PORT (dendrite switch_identifiers.rs:60)" -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "lldpd" client = "dpd-client" -evaluation = "same-deployment-unit" -note = """ -lldpd defaults to localhost for dpd (dendrite.rs:194), and the SMF start -script doesn't pass in --host. -""" -permalinks = [ - "https://github.com/oxidecomputer/lldp/blob/61479b6/lldpd/src/dendrite.rs#L194", - "https://github.com/oxidecomputer/lldp/blob/61479b6/lldpd/misc/lldpd.xml", - "https://github.com/oxidecomputer/lldp/blob/61479b6/lldpd/misc/svc-lldpd", -] +note = "verified: lldpd defaults to localhost for dpd (lldp dendrite.rs:194)" -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "mgd" client = "ddm-admin-client" -evaluation = "same-deployment-unit" -note = "mg-lower hardcodes localhost:8000." -permalinks = [ - "https://github.com/oxidecomputer/maghemite/blob/396bb3c/mg-lower/src/ddm.rs#L110", -] +note = "verified: mg-lower hardcodes localhost:8000 (mg-lower ddm.rs:110)" -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "mgd" client = "dpd-client" -evaluation = "same-deployment-unit" -note = "mg-lower hardcodes localhost." -permalinks = [ - "https://github.com/oxidecomputer/maghemite/blob/396bb3c/mg-lower/src/dendrite.rs#L491", -] +note = "verified: mg-lower hardcodes localhost (mg-lower dendrite.rs:491)" -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "mgd" client = "gateway-client" -evaluation = "same-deployment-unit" -note = """ -mgd defaults to [::1]:12225 (mgd main.rs:93), and the SMF start script -doesn't set --mgs-addr. -""" -permalinks = [ - "https://github.com/oxidecomputer/maghemite/blob/396bb3c/mgd/src/main.rs#L93", - "https://github.com/oxidecomputer/maghemite/blob/396bb3c/smf/mgd/manifest.xml", - "https://github.com/oxidecomputer/maghemite/blob/396bb3c/smf/mgd_method_script.sh", -] +note = "verified: mgd defaults to [::1]:12225 (mgd main.rs:93)" -[[dependency_filter_rules]] -server = "omicron-sled-agent" -client = "ddm-admin-client" -evaluation = "same-deployment-unit" -note = "Sled Agent uses Client::localhost() to connect to ddm." -permalinks = [ - "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/ddm_reconciler.rs#L56", -] +# NOTE: The following sled-agent edges are BUGS - they use underlay IPs, not +# localhost. They are kept here to avoid breaking the build, but need to be +# fixed, either through client-side versioning or by some other means. -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "omicron-sled-agent" -client = "mg-admin-client" -evaluation = "same-deployment-unit" -note = """ -Sled Agent only queries the switch zone on the same sled, -which is within the same deployment unit as the global -zone. -""" -permalinks = [ - "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/bootstrap/early_networking.rs#L483", -] +client = "gateway-client" +note = "BUG: uses underlay IP, not localhost (early_networking.rs:376)" -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "omicron-sled-agent" -client = "propolis-client" -evaluation = "same-deployment-unit" -note = "Unsure: it looks like this only queries the Propolis server out of the same deployment unit?" -permalinks = [ - "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/instance.rs#L2283", -] +client = "ddm-admin-client" +note = "verified: uses Client::localhost() (ddm_reconciler.rs:56)" -# NOTE: The following sled-agent edges are bugs: they cross deployment units. -# They are kept here to avoid breaking the build, but need to be fixed, either -# through client-side versioning or by some other means. +[[localhost_only_edges]] +server = "omicron-sled-agent" +client = "dpd-client" +note = "BUG: uses underlay IP, not localhost (services.rs:1090)" -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "omicron-sled-agent" -client = "gateway-client" -evaluation = "same-deployment-unit" -note = """ -BUG: Sled Agent creates two MGS clients. One of them -(early_networking.rs:376) queries the switch zone on -the same sled, which is within the same deployment unit -as the global zone, so this is okay. - -The other one (early_networking.rs:277) queries both -switch zones, so it crosses deployment units. This needs -to be fixed. -""" -permalinks = [ - "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/bootstrap/early_networking.rs#L376", - "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/bootstrap/early_networking.rs#L277-L280", -] +client = "mg-admin-client" +note = "BUG: uses underlay IP, not localhost (early_networking.rs:483)" -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "omicron-sled-agent" -client = "dpd-client" -evaluation = "same-deployment-unit" -note = """ -BUG: Sled Agent reaches out to both switch zones, which crosses deployment units. -""" -permalinks = [ - "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/services.rs#L1090", -] +client = "propolis-client" +note = "BUG: uses zone IP, not localhost (instance.rs:2283)" -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "tfportd" client = "dpd-client" -evaluation = "same-deployment-unit" -note = """ -The tfportd service has the dpd_host SMF property (tfport.xml:52) set to [::1] -(services.rs:2852). tfportd has a --dpd-host option (main.rs:81) but the SMF -start script (svc-tfportd:12) does not use it. -""" -permalinks = [ - "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/services.rs#L2852", - "https://github.com/oxidecomputer/dendrite/blob/606c0be/tfportd/misc/tfport.xml#L52-L53", - "https://github.com/oxidecomputer/dendrite/blob/cf31be9/tfportd/src/main.rs#L81-L83", - "https://github.com/oxidecomputer/dendrite/blob/606c0be/tools/svc-tfportd#L12", -] +note = "verified: configured with dpd_host=[::1] (services.rs:2852)" -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "tfportd" client = "lldpd-client" -evaluation = "same-deployment-unit" -note = "tfportd hardcodes localhost for its lldpd-client (tfport.rs:88)." -permalinks = [ - "https://github.com/oxidecomputer/dendrite/blob/606c0be/tfportd/src/tfport.rs#L88", -] +note = "verified: hardcodes localhost (tfportd tfport.rs:88)" -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "wicketd" client = "ddm-admin-client" -evaluation = "same-deployment-unit" -note = "wicketd uses DdmAdminClient::localhost() (bootstrap_addrs.rs:162)." -permalinks = [ - "https://github.com/oxidecomputer/omicron/blob/6cef874/wicketd/src/bootstrap_addrs.rs#L162", -] +note = "verified: uses DdmAdminClient::localhost() (bootstrap_addrs.rs:162)" -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "wicketd" client = "dpd-client" -evaluation = "same-deployment-unit" -note = "wicketd hardcodes [::1] (uplink.rs:83)." -permalinks = [ - "https://github.com/oxidecomputer/omicron/blob/6cef874/wicketd/src/preflight_check/uplink.rs#L83", -] +note = "verified: hardcodes [::1]:DENDRITE_PORT (preflight_check/uplink.rs:83)" -[[dependency_filter_rules]] +[[localhost_only_edges]] server = "wicketd" client = "gateway-client" -evaluation = "same-deployment-unit" -note = """ -wicketd's mgs-address config (wicketd.rs:35) is always set to [::1] -(services.rs:2586). This config is passed into wicketd at startup -(manifest.xml:20). -""" -permalinks = [ - "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/services.rs#L2685-L2689", - "https://github.com/oxidecomputer/omicron/blob/6cef874/wicketd/src/bin/wicketd.rs#L35", - "https://github.com/oxidecomputer/omicron/blob/6cef874/smf/wicketd/manifest.xml#L20", -] +note = "verified: --mgs-address CLI expects localhost (bin/wicketd.rs:35)" diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index 9b0d0ab4ea5..3d0ff0052e2 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -30,6 +30,7 @@ pub struct AllApiMetadata { deployment_units: BTreeMap, dependency_rules: BTreeMap>, ignored_non_clients: BTreeSet, + localhost_only_edges: Vec, } impl AllApiMetadata { @@ -73,90 +74,48 @@ impl AllApiMetadata { &self.ignored_non_clients } - /// Returns an iterator over rules that have `SameDeploymentUnit` - /// evaluation. - pub fn same_deployment_unit_rules( - &self, - ) -> impl Iterator { - self.dependency_rules - .values() - .flatten() - .filter(|r| r.evaluation == Evaluation::SameDeploymentUnit) + /// Returns the list of localhost-only edges + pub fn localhost_only_edges(&self) -> &[LocalhostOnlyEdge] { + &self.localhost_only_edges } - /// Returns how we should filter the given dependency. - /// - /// A dependency may match up to one server rule and up to one ancestor - /// rule. Filters can then check if any matching rule would cause exclusion. + /// Returns how we should filter the given dependency pub(crate) fn evaluate_dependency( &self, workspaces: &Workspaces, - server: &ServerComponentName, client_pkgname: &ClientPackageName, dep_path: &DepPath, - ) -> Result { + ) -> Result { let Some(rules) = self.dependency_rules.get(client_pkgname) else { - return Ok(DependencyEvaluation::default()); + return Ok(Evaluation::default()); }; - let mut result = DependencyEvaluation::default(); - - let server_rules: Vec<_> = rules - .iter() - .filter(|r| match &r.matcher { - RuleMatcher::Server(s) => s == server, - RuleMatcher::Ancestor(_) => false, - }) - .collect(); - - if server_rules.len() > 1 { - bail!( - "client package {:?}: dependency matched multiple server \ - filters for server {:?}", - client_pkgname, - server - ); - } - - if let Some(rule) = server_rules.first() { - result.server = Some(rule.evaluation); - } - - let ancestor_rules: Vec<_> = rules + let which_rules: Vec<_> = rules .iter() .filter(|r| { assert_eq!(r.client, *client_pkgname); - match &r.matcher { - RuleMatcher::Ancestor(ancestor) => { - let pkgids = workspaces.workspace_pkgids(ancestor); - dep_path.contains_any(&pkgids) - } - RuleMatcher::Server(_) => false, - } + let pkgids = workspaces.workspace_pkgids(&r.ancestor); + dep_path.contains_any(&pkgids) }) .collect(); - if ancestor_rules.len() > 1 { + if which_rules.is_empty() { + return Ok(Evaluation::default()); + } + + if which_rules.len() > 1 { bail!( - "client package {:?}: dependency matched multiple ancestor \ - filters: {}", + "client package {:?}: dependency matched multiple filters: {}", client_pkgname, - ancestor_rules + which_rules .into_iter() - .map(|r| match &r.matcher { - RuleMatcher::Ancestor(a) => a.as_str(), - RuleMatcher::Server(_) => unreachable!(), - }) + .map(|r| r.ancestor.as_str()) .collect::>() .join(", ") ); } - if let Some(rule) = ancestor_rules.first() { - result.ancestor = Some(rule.evaluation); - } - - Ok(result) + Ok(which_rules[0].evaluation) } /// Returns the list of APIs that have non-DAG dependency rules @@ -183,8 +142,9 @@ impl AllApiMetadata { struct RawApiMetadata { apis: Vec, deployment_units: Vec, - dependency_filter_rules: Vec, + dependency_filter_rules: Vec, ignored_non_clients: Vec, + localhost_only_edges: Vec, } impl TryFrom for AllApiMetadata { @@ -216,33 +176,15 @@ impl TryFrom for AllApiMetadata { } } - // Build set of known server components for validation. - let known_components: BTreeSet<_> = - deployment_units.values().flat_map(|u| u.packages.iter()).collect(); - let mut dependency_rules = BTreeMap::new(); - for raw_rule in raw.dependency_filter_rules { - if !apis.contains_key(&raw_rule.client) { + for rule in raw.dependency_filter_rules { + if !apis.contains_key(&rule.client) { bail!( "dependency rule references unknown client: {:?}", - raw_rule.client + rule.client ); } - let rule = DependencyFilterRule::try_from_raw(raw_rule)?; - - // Validate that server matchers reference known server components. - if let RuleMatcher::Server(ref server) = rule.matcher { - if !known_components.contains(server) { - bail!( - "dependency filter rule for client {:?}: \ - unknown server component {:?}", - rule.client, - server - ); - } - } - dependency_rules .entry(rule.client.clone()) .or_insert_with(Vec::new) @@ -259,24 +201,33 @@ impl TryFrom for AllApiMetadata { } } + // Validate localhost_only_edges reference known server components and + // APIs. + let known_components: BTreeSet<_> = + deployment_units.values().flat_map(|u| u.packages.iter()).collect(); + for edge in &raw.localhost_only_edges { + if !known_components.contains(&edge.server) { + bail!( + "localhost_only_edges: unknown server component {:?}", + edge.server + ); + } + let client_name = edge.client.as_specific(); + if !apis.contains_key(client_name) { + bail!("localhost_only_edges: unknown client {:?}", client_name); + } + } + Ok(AllApiMetadata { apis, deployment_units, dependency_rules, ignored_non_clients, + localhost_only_edges: raw.localhost_only_edges, }) } } -/// Specifies which aspect of a dependency to match against. -#[derive(Clone, Debug)] -pub enum RuleMatcher { - /// Match when the dependency path contains this ancestor package. - Ancestor(String), - /// Match when the server component equals this value. - Server(ServerComponentName), -} - /// Describes how an API in the system manages drift between client and server #[derive(Deserialize, PartialEq, Eq)] #[serde(rename_all = "snake_case")] @@ -458,92 +409,19 @@ pub struct DeploymentUnitInfo { pub packages: Vec, } -/// Raw deserialization struct for dependency filter rules. -/// -/// This is validated into `DependencyFilterRule`. #[derive(Deserialize)] #[serde(deny_unknown_fields)] -struct RawDependencyFilterRule { - // Only one of `ancestor` and `server` should be specified. - ancestor: Option, - server: Option, - client: ClientPackageName, - #[serde(default)] - evaluation: Evaluation, - note: String, - #[serde(default)] - permalinks: Vec, -} - -/// A validated dependency filter rule. -#[derive(Clone, Debug)] pub struct DependencyFilterRule { - pub matcher: RuleMatcher, + pub ancestor: String, pub client: ClientPackageName, + #[serde(default)] pub evaluation: Evaluation, - // These notes and permalinks are not currently used, but are stored in - // api-manifest.toml (and notes are required). They could as well just be - // TOML comments. But it seems nice to enforce that notes are present. And - // this would let us include this explanation in output in the future (e.g., - // to explain why some dependency was filtered out). + // These notes are not currently used, but they are required. They could as + // well just be TOML comments. But it seems nice to enforce that they're + // present. And this would let us include this explanation in output in the + // future (e.g., to explain why some dependency was filtered out). + #[allow(dead_code)] pub note: String, - pub permalinks: Vec, -} - -impl DependencyFilterRule { - fn try_from_raw(raw: RawDependencyFilterRule) -> Result { - let matcher = match (raw.ancestor, raw.server) { - (Some(a), None) => RuleMatcher::Ancestor(a), - (None, Some(s)) => RuleMatcher::Server(s), - (Some(_), Some(_)) => bail!( - "dependency filter rule for client {:?} cannot have both \ - 'ancestor' and 'server'", - raw.client - ), - (None, None) => bail!( - "dependency filter rule for client {:?} must have either \ - 'ancestor' or 'server'", - raw.client - ), - }; - - // SameDeploymentUnit requires the server matcher since it goes through - // explicit validation. - if raw.evaluation == Evaluation::SameDeploymentUnit { - if !matches!(matcher, RuleMatcher::Server(_)) { - bail!( - "dependency filter rule for client {:?}: \ - 'same-deployment-unit' evaluation requires 'server' field, found {:?}", - raw.client, - matcher, - ); - } - } - - Ok(DependencyFilterRule { - matcher, - client: raw.client, - evaluation: raw.evaluation, - note: raw.note, - permalinks: raw.permalinks, - }) - } -} - -/// Result of evaluating a dependency against all matching rules. -#[derive(Clone, Debug, Default)] -pub struct DependencyEvaluation { - /// Evaluation from a server rule, if one matched. - pub server: Option, - /// Evaluation from an ancestor rule, if one matched. - pub ancestor: Option, -} - -impl DependencyEvaluation { - /// Returns true if any matching rule has the given evaluation. - pub fn any_matches(&self, target: Evaluation) -> bool { - self.server == Some(target) || self.ancestor == Some(target) - } } #[derive(Clone, Copy, Debug, Default, Deserialize, Eq, PartialEq)] @@ -562,14 +440,62 @@ pub enum Evaluation { NonDag, /// This dependency should be part of the update DAG Dag, - /// Intra-deployment-unit communication. Only valid with the `server` - /// matcher and server-side-versioned APIs. Subject to auto-validation. - /// - /// Note: This evaluation is intentionally *not* filtered out by - /// `ApiDependencyFilter`. These edges are real dependencies that should - /// appear in component listings and most graphs. They are only excluded - /// when building the deployment unit dependency graph for cycle detection - /// (via `VersionedHowFilter::ServerSideOnly`), because intra-unit - /// communication doesn't create cross-unit version skew. - SameDeploymentUnit, +} + +/// Specifies which client to match in a localhost-only edge rule. +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum ClientMatcher { + /// Match a specific client package. + Specific(ClientPackageName), +} + +impl<'de> Deserialize<'de> for ClientMatcher { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + Ok(ClientMatcher::Specific(ClientPackageName::from(s))) + } +} + +impl ClientMatcher { + /// Returns true if this matcher matches the given client package. + pub fn matches(&self, client: &ClientPackageName) -> bool { + match self { + ClientMatcher::Specific(name) => name == client, + } + } + + /// Returns the specific client name. + pub fn as_specific(&self) -> &ClientPackageName { + match self { + ClientMatcher::Specific(name) => name, + } + } +} + +/// An edge that should be excluded from the deployment unit dependency graph +/// because it represents communication that only happens locally within a +/// deployment unit. +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +pub struct LocalhostOnlyEdge { + /// The server component that consumes the API. + pub server: ServerComponentName, + /// The client package consumed. + pub client: ClientMatcher, + /// Explanation of why this edge is localhost-only. + pub note: String, +} + +impl LocalhostOnlyEdge { + /// Returns true if this rule matches the given (server, client) pair. + pub fn matches( + &self, + server: &ServerComponentName, + client: &ClientPackageName, + ) -> bool { + self.server == *server && self.client.matches(client) + } } diff --git a/dev-tools/ls-apis/src/bin/ls-apis.rs b/dev-tools/ls-apis/src/bin/ls-apis.rs index aaf07f9ee69..54169e076c0 100644 --- a/dev-tools/ls-apis/src/bin/ls-apis.rs +++ b/dev-tools/ls-apis/src/bin/ls-apis.rs @@ -252,10 +252,10 @@ fn print_server_components<'a>( ); } for (c, path) in apis.component_apis_consumed(s, filter)? { - if apis.is_same_deployment_unit_edge(s, c) { + if let Some(note) = apis.localhost_only_edge_note(s, c) { println!( - "{} consumes: {} (same-deployment-unit)", - prefix, c + "{} consumes: {} (localhost-only: {})", + prefix, c, note ); } else { println!("{} consumes: {}", prefix, c); diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index 87448dad665..56f0e179483 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -15,8 +15,8 @@ use crate::api_metadata::ApiConsumerStatus; use crate::api_metadata::ApiExpectedConsumer; use crate::api_metadata::ApiExpectedConsumers; use crate::api_metadata::ApiMetadata; +use crate::api_metadata::ClientMatcher; use crate::api_metadata::Evaluation; -use crate::api_metadata::RuleMatcher; use crate::api_metadata::VersionedHow; use crate::cargo::DepPath; use crate::parse_toml_file; @@ -30,12 +30,10 @@ use iddqd::IdOrdMap; use iddqd::id_upcast; use parse_display::{Display, FromStr}; use petgraph::dot::Dot; -use petgraph::graph::Graph; use petgraph::graph::NodeIndex; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::fmt; -use swrite::{SWrite, swriteln}; /// Query information about the Dropshot/OpenAPI/Progenitor-based APIs within /// the Oxide system @@ -64,10 +62,6 @@ pub struct SystemApis { /// maps an API name to the server component(s) that expose that API api_producers: BTreeMap, - /// index of (server, client) pairs configured as same-deployment-unit edges - same_deployment_unit_edges: - BTreeSet<(ServerComponentName, ClientPackageName)>, - /// source of developer-maintained API metadata api_metadata: AllApiMetadata, /// source of Cargo package metadata @@ -131,22 +125,6 @@ impl SystemApis { // Load the API manifest. let api_metadata: AllApiMetadata = parse_toml_file(&args.api_manifest_path)?; - - // Build an index of configured same-deployment-unit edges. We use this - // index for validation and lookups. - let same_deployment_unit_edges: BTreeSet<_> = api_metadata - .same_deployment_unit_rules() - .filter_map(|rule| match &rule.matcher { - RuleMatcher::Server(server) => { - Some((server.clone(), rule.client.clone())) - } - RuleMatcher::Ancestor(_) => { - // This case is rejected at API construction time. - None - } - }) - .collect(); - // Load Cargo metadata and validate it against the manifest. let (workspaces, warnings) = Workspaces::load(&api_metadata)?; if !warnings.is_empty() { @@ -316,7 +294,6 @@ impl SystemApis { api_consumers, missing_expected_consumers, api_producers, - same_deployment_unit_edges, api_metadata, workspaces, }) @@ -354,15 +331,17 @@ impl SystemApis { &self.api_metadata } - /// Returns true if this (server, client) edge is a same-deployment-unit - /// edge. - pub fn is_same_deployment_unit_edge( + /// Returns the note for a localhost-only edge, if one matches. + pub fn localhost_only_edge_note( &self, server: &ServerComponentName, client: &ClientPackageName, - ) -> bool { - self.same_deployment_unit_edges - .contains(&(server.clone(), client.clone())) + ) -> Option<&str> { + self.api_metadata + .localhost_only_edges() + .iter() + .find(|edge| edge.matches(server, client)) + .map(|edge| edge.note.as_str()) } /// Given a server component, return the APIs consumed by this component @@ -385,7 +364,6 @@ impl SystemApis { if filter.should_include( &self.api_metadata, &self.workspaces, - server_component, client_pkgname, p, )? { @@ -434,8 +412,7 @@ impl SystemApis { if filter.should_include( &self.api_metadata, &self.workspaces, - &api_consumer.server_pkgname, - client, + &client, p, )? { include.push(p); @@ -511,23 +488,24 @@ impl SystemApis { /// Returns a string that can be passed to `dot(1)` to render a graph of /// API dependencies among deployment units pub fn dot_by_unit(&self, filter: ApiDependencyFilter) -> Result { - let (graph, _) = - self.make_deployment_unit_graph(filter, VersionedHowFilter::All)?; + let (graph, _) = self.make_deployment_unit_graph(filter, None)?; Ok(Dot::new(&graph).to_string()) } // The complex type below is only used in this one place: the return value // of this internal helper function. A type alias doesn't seem better. - #[expect(clippy::type_complexity)] + #[allow(clippy::type_complexity)] fn make_deployment_unit_graph( &self, dependency_filter: ApiDependencyFilter, - versioned_how_filter: VersionedHowFilter, + localhost_only_edges: Option< + &BTreeSet<(ServerComponentName, ClientPackageName)>, + >, ) -> Result<( - Graph<&DeploymentUnitName, &ClientPackageName>, + petgraph::graph::Graph<&DeploymentUnitName, &ClientPackageName>, BTreeMap<&DeploymentUnitName, NodeIndex>, )> { - let mut graph = Graph::new(); + let mut graph = petgraph::graph::Graph::new(); let nodes: BTreeMap<_, _> = self .deployment_units() .map(|name| (name, graph.add_node(name))) @@ -544,12 +522,9 @@ impl SystemApis { for (client_pkg, _) in self.component_apis_consumed(server_pkg, dependency_filter)? { - // When building a server-side-versioned-only graph, - // filter to only server-versioned APIs and skip edges - // that represent same-deployment-unit communication. - if let VersionedHowFilter::ServerSideOnly = - versioned_how_filter - { + // When localhost_only_edges is provided, we're building a + // graph of server-side-versioned API dependencies only. + if let Some(localhost_edges) = localhost_only_edges { let api = self .api_metadata .client_pkgname_lookup(client_pkg) @@ -558,8 +533,10 @@ impl SystemApis { continue; } - if self - .same_deployment_unit_edges + // Skip edges that represent localhost-only communication + // (communication within a deployment unit that doesn't + // cross deployment unit boundaries). + if localhost_edges .contains(&(server_pkg.clone(), client_pkg.clone())) { continue; @@ -608,10 +585,10 @@ impl SystemApis { dependency_filter: ApiDependencyFilter, versioned_on_server_only: bool, ) -> Result<( - Graph<&ServerComponentName, &ClientPackageName>, + petgraph::graph::Graph<&ServerComponentName, &ClientPackageName>, BTreeMap<&ServerComponentName, NodeIndex>, )> { - let mut graph = Graph::new(); + let mut graph = petgraph::graph::Graph::new(); let nodes: BTreeMap<_, _> = self .server_component_units .keys() @@ -659,17 +636,18 @@ impl SystemApis { /// 3. The server and API producer are in the same deployment unit. /// /// Returns a set of (server, client) pairs. - fn compute_required_same_deployment_unit_edges( + fn compute_required_localhost_edges( &self, ) -> Result> { - // Use the IncludeNonDag filter to include SameDeploymentUnit edges. We - // need to see all real edges (excluding only the Bogus and NotDeployed - // ones) to determine which ones should be in the same-deployment-unit - // set. - let filter = ApiDependencyFilter::IncludeNonDag; + let filter = ApiDependencyFilter::Default; let mut required = BTreeSet::new(); - for (server, server_unit) in &self.server_component_units { + for server in self.server_component_units.keys() { + let Some(server_unit) = self.server_component_units.get(server) + else { + continue; + }; + for (client, _) in self.component_apis_consumed(server, filter)? { // Only consider server-side-versioned APIs. let Some(api) = self.api_metadata.client_pkgname_lookup(client) @@ -691,7 +669,7 @@ impl SystemApis { if server_unit == producer_unit { // This edge would create an intra-unit dependency for // a server-versioned API, so it must be in the - // same-deployment-unit-only set. + // localhost-only list. required.insert((server.clone(), client.clone())); break; } @@ -702,52 +680,55 @@ impl SystemApis { Ok(required) } - /// Validates that the configured same-deployment-unit rules exactly match - /// the required set of edges that must be excluded. - fn validate_same_deployment_unit_rules(&self) -> Result<()> { - let required = self.compute_required_same_deployment_unit_edges()?; - let configured = &self.same_deployment_unit_edges; + /// Validates that the configured localhost_only_edges exactly match the + /// required set of edges that must be excluded. + /// + /// Returns the validated set of edges for use by make_deployment_unit_graph. + fn validate_localhost_only_edges( + &self, + ) -> Result> { + let required = self.compute_required_localhost_edges()?; - let missing: BTreeSet<_> = required.difference(configured).collect(); + // Build the configured set from the manifest. + let mut configured = BTreeSet::new(); + for edge in self.api_metadata.localhost_only_edges() { + let ClientMatcher::Specific(client) = &edge.client; + configured.insert((edge.server.clone(), client.clone())); + } + + // Compare the two sets. + let missing: BTreeSet<_> = required.difference(&configured).collect(); let extra: BTreeSet<_> = configured.difference(&required).collect(); if !missing.is_empty() || !extra.is_empty() { let mut msg = String::from( - "same-deployment-unit rules do not match required edges:\n", + "localhost_only_edges configuration does not match required edges:\n", ); if !missing.is_empty() { - swriteln!( - msg, - "\nmissing entries (these edges exist and need \ - same-deployment-unit exclusion):" - ); + msg.push_str("\nMissing entries (these edges exist and need localhost-only exclusion):\n"); for (server, client) in &missing { - swriteln!( - msg, - " - server = {server:?}, client = {client:?}" - ); + msg.push_str(&format!( + " - server = {:?}, client = {:?}\n", + server, client + )); } } if !extra.is_empty() { - swriteln!( - msg, - "\nextra entries (these edges don't exist or don't need \ - exclusion):" - ); + msg.push_str("\nExtra entries (these edges don't exist or don't need exclusion):\n"); for (server, client) in &extra { - swriteln!( - msg, - " - server = {server:?}, client = {client:?}" - ); + msg.push_str(&format!( + " - server = {:?}, client = {:?}\n", + server, client + )); } } bail!("{}", msg); } - Ok(()) + Ok(required) } /// Verifies various important properties about the assignment of which APIs @@ -804,9 +785,9 @@ impl SystemApis { // can't be part of a cycle. let filter = ApiDependencyFilter::Default; - // Validate that all configured same-deployment-unit rules are correct - // and match the required set exactly. - self.validate_same_deployment_unit_rules()?; + // Validate that all configured localhost_only_edges are correct and + // match the required set exactly. + let localhost_only_edges = self.validate_localhost_only_edges()?; // Construct a graph where: // @@ -827,10 +808,8 @@ impl SystemApis { } // Do the same with a graph of deployment units. - let (graph, nodes) = self.make_deployment_unit_graph( - filter, - VersionedHowFilter::ServerSideOnly, - )?; + let (graph, nodes) = self + .make_deployment_unit_graph(filter, Some(&localhost_only_edges))?; let reverse_nodes: BTreeMap<_, _> = nodes.iter().map(|(d_u, node)| (node, d_u)).collect(); if let Err(error) = petgraph::algo::toposort(&graph, None) { @@ -1435,17 +1414,6 @@ impl<'a> ClientDependenciesTracker<'a> { } } -/// Specifies whether to filter to only server-versioned APIs when building -/// dependency graphs. -#[derive(Clone, Debug)] -pub enum VersionedHowFilter { - /// Include all APIs regardless of versioning strategy. - All, - /// Include only APIs with server-side versioning, and skip edges that - /// represent same-deployment-unit communication. - ServerSideOnly, -} - /// Specifies which API dependencies to include vs. ignore when iterating /// dependencies #[derive(Clone, Copy, Debug, Default, Display, FromStr)] @@ -1482,42 +1450,36 @@ pub enum ApiDependencyFilter { impl ApiDependencyFilter { /// Return whether this filter should include a dependency on - /// `client_pkgname` that goes either through `server` or through - /// `dep_path`. - /// - /// Note: `Evaluation::SameDeploymentUnit` edges are included by all filters - /// here. These edges represent real dependencies and should appear in - /// component listings and graphs. They are only excluded when building the - /// deployment unit graph for cycle detection, which is handled separately - /// via `VersionedHowFilter::ServerSideOnly` in - /// `make_deployment_unit_graph`. + /// `client_pkgname` that goes through dependency path `dep_path` fn should_include( &self, api_metadata: &AllApiMetadata, workspaces: &Workspaces, - server: &ServerComponentName, client_pkgname: &ClientPackageName, dep_path: &DepPath, ) -> Result { - let eval = api_metadata - .evaluate_dependency(workspaces, server, client_pkgname, dep_path) + let evaluation = api_metadata + .evaluate_dependency(workspaces, client_pkgname, dep_path) .with_context(|| format!("error applying filter {:?}", self))?; Ok(match self { ApiDependencyFilter::All => true, - ApiDependencyFilter::Bogus => eval.any_matches(Evaluation::Bogus), - ApiDependencyFilter::NonBogus => { - !eval.any_matches(Evaluation::Bogus) + ApiDependencyFilter::Bogus => { + matches!(evaluation, Evaluation::Bogus) } - ApiDependencyFilter::IncludeNonDag => { - !eval.any_matches(Evaluation::Bogus) - && !eval.any_matches(Evaluation::NotDeployed) - } - ApiDependencyFilter::Default => { - !eval.any_matches(Evaluation::NonDag) - && !eval.any_matches(Evaluation::Bogus) - && !eval.any_matches(Evaluation::NotDeployed) + ApiDependencyFilter::NonBogus => { + !matches!(evaluation, Evaluation::Bogus) } + ApiDependencyFilter::IncludeNonDag => !matches!( + evaluation, + Evaluation::Bogus | Evaluation::NotDeployed + ), + ApiDependencyFilter::Default => !matches!( + evaluation, + Evaluation::NonDag + | Evaluation::Bogus + | Evaluation::NotDeployed + ), }) } } From b7ac647bced1eb4325e19f50e25b622a4879f00b Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 5 Feb 2026 14:05:18 -0800 Subject: [PATCH 09/19] localhost -> intra-deployment-unit --- dev-tools/ls-apis/api-manifest.toml | 48 +++++++++++---------- dev-tools/ls-apis/src/api_metadata.rs | 39 ++++++++++------- dev-tools/ls-apis/src/bin/ls-apis.rs | 4 +- dev-tools/ls-apis/src/system_apis.rs | 60 +++++++++++++++------------ 4 files changed, 86 insertions(+), 65 deletions(-) diff --git a/dev-tools/ls-apis/api-manifest.toml b/dev-tools/ls-apis/api-manifest.toml index 46adc8f0a70..8e1d8685f5c 100644 --- a/dev-tools/ls-apis/api-manifest.toml +++ b/dev-tools/ls-apis/api-manifest.toml @@ -595,41 +595,47 @@ sled agent. """ ################################################################################ -# Localhost-only edges +# Intra-deployment-unit-only edges # # These edges are excluded from the deployment unit dependency graph because -# they represent communication that only happens locally within a deployment -# unit, not across deployment unit boundaries. A single deployment unit is -# updated atomically, so there's no risk of version skew with localhost-only -# edges. +# they represent communication that only happens locally within a *single +# instance* of a single deployment unit, not across deployment unit boundaries. +# A single deployment unit is updated atomically, so there's no risk of version +# skew with intra_deployment_unit-only edges. +# +# Note that things do not belong here when they cross different instances of the +# same deployment unit. For example, Sled Agent and MGS are in the same +# deployment unit (the host OS), but Sled Agent's use of MGS crosses different +# instances of the deployment unit (i.e., different sleds). Those don't belong +# in this category. ################################################################################ -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "ddmd" client = "dpd-client" note = "verified: sled-agent configures ddmd with dpd_host=[::1] (services.rs:3127)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "dpd" client = "gateway-client" note = "verified: dpd defaults to [::1]:MGS_PORT (dendrite switch_identifiers.rs:60)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "lldpd" client = "dpd-client" note = "verified: lldpd defaults to localhost for dpd (lldp dendrite.rs:194)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "mgd" client = "ddm-admin-client" note = "verified: mg-lower hardcodes localhost:8000 (mg-lower ddm.rs:110)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "mgd" client = "dpd-client" note = "verified: mg-lower hardcodes localhost (mg-lower dendrite.rs:491)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "mgd" client = "gateway-client" note = "verified: mgd defaults to [::1]:12225 (mgd main.rs:93)" @@ -638,52 +644,52 @@ note = "verified: mgd defaults to [::1]:12225 (mgd main.rs:93)" # localhost. They are kept here to avoid breaking the build, but need to be # fixed, either through client-side versioning or by some other means. -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "omicron-sled-agent" client = "gateway-client" note = "BUG: uses underlay IP, not localhost (early_networking.rs:376)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "omicron-sled-agent" client = "ddm-admin-client" note = "verified: uses Client::localhost() (ddm_reconciler.rs:56)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "omicron-sled-agent" client = "dpd-client" note = "BUG: uses underlay IP, not localhost (services.rs:1090)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "omicron-sled-agent" client = "mg-admin-client" note = "BUG: uses underlay IP, not localhost (early_networking.rs:483)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "omicron-sled-agent" client = "propolis-client" note = "BUG: uses zone IP, not localhost (instance.rs:2283)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "tfportd" client = "dpd-client" note = "verified: configured with dpd_host=[::1] (services.rs:2852)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "tfportd" client = "lldpd-client" note = "verified: hardcodes localhost (tfportd tfport.rs:88)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "wicketd" client = "ddm-admin-client" note = "verified: uses DdmAdminClient::localhost() (bootstrap_addrs.rs:162)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "wicketd" client = "dpd-client" note = "verified: hardcodes [::1]:DENDRITE_PORT (preflight_check/uplink.rs:83)" -[[localhost_only_edges]] +[[intra_deployment_unit_only_edges]] server = "wicketd" client = "gateway-client" note = "verified: --mgs-address CLI expects localhost (bin/wicketd.rs:35)" diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index 3d0ff0052e2..44b353a61b7 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -30,7 +30,7 @@ pub struct AllApiMetadata { deployment_units: BTreeMap, dependency_rules: BTreeMap>, ignored_non_clients: BTreeSet, - localhost_only_edges: Vec, + intra_deployment_unit_only_edges: Vec, } impl AllApiMetadata { @@ -74,9 +74,11 @@ impl AllApiMetadata { &self.ignored_non_clients } - /// Returns the list of localhost-only edges - pub fn localhost_only_edges(&self) -> &[LocalhostOnlyEdge] { - &self.localhost_only_edges + /// Returns the list of intra-deployment-unit-only edges + pub fn intra_deployment_unit_only_edges( + &self, + ) -> &[IntraDeploymentUnitOnlyEdge] { + &self.intra_deployment_unit_only_edges } /// Returns how we should filter the given dependency @@ -144,7 +146,7 @@ struct RawApiMetadata { deployment_units: Vec, dependency_filter_rules: Vec, ignored_non_clients: Vec, - localhost_only_edges: Vec, + intra_deployment_unit_only_edges: Vec, } impl TryFrom for AllApiMetadata { @@ -201,20 +203,24 @@ impl TryFrom for AllApiMetadata { } } - // Validate localhost_only_edges reference known server components and - // APIs. + // Validate IDU-only edges reference known server components and APIs. let known_components: BTreeSet<_> = deployment_units.values().flat_map(|u| u.packages.iter()).collect(); - for edge in &raw.localhost_only_edges { + for edge in &raw.intra_deployment_unit_only_edges { if !known_components.contains(&edge.server) { bail!( - "localhost_only_edges: unknown server component {:?}", + "intra_deployment_unit_only_edges: \ + unknown server component {:?}", edge.server ); } let client_name = edge.client.as_specific(); if !apis.contains_key(client_name) { - bail!("localhost_only_edges: unknown client {:?}", client_name); + bail!( + "intra_deployment_unit_only_edges: + unknown client {:?}", + client_name + ); } } @@ -223,7 +229,8 @@ impl TryFrom for AllApiMetadata { deployment_units, dependency_rules, ignored_non_clients, - localhost_only_edges: raw.localhost_only_edges, + intra_deployment_unit_only_edges: raw + .intra_deployment_unit_only_edges, }) } } @@ -442,7 +449,7 @@ pub enum Evaluation { Dag, } -/// Specifies which client to match in a localhost-only edge rule. +/// Specifies which client to match in an IDU-only edge rule. #[derive(Clone, Debug, Eq, PartialEq)] pub enum ClientMatcher { /// Match a specific client package. @@ -477,19 +484,19 @@ impl ClientMatcher { /// An edge that should be excluded from the deployment unit dependency graph /// because it represents communication that only happens locally within a -/// deployment unit. +/// single instance of a single deployment unit. #[derive(Deserialize)] #[serde(deny_unknown_fields)] -pub struct LocalhostOnlyEdge { +pub struct IntraDeploymentUnitOnlyEdge { /// The server component that consumes the API. pub server: ServerComponentName, /// The client package consumed. pub client: ClientMatcher, - /// Explanation of why this edge is localhost-only. + /// Explanation of why this edge is intra-deployment-unit-only. pub note: String, } -impl LocalhostOnlyEdge { +impl IntraDeploymentUnitOnlyEdge { /// Returns true if this rule matches the given (server, client) pair. pub fn matches( &self, diff --git a/dev-tools/ls-apis/src/bin/ls-apis.rs b/dev-tools/ls-apis/src/bin/ls-apis.rs index 54169e076c0..b2c7844218e 100644 --- a/dev-tools/ls-apis/src/bin/ls-apis.rs +++ b/dev-tools/ls-apis/src/bin/ls-apis.rs @@ -252,9 +252,9 @@ fn print_server_components<'a>( ); } for (c, path) in apis.component_apis_consumed(s, filter)? { - if let Some(note) = apis.localhost_only_edge_note(s, c) { + if let Some(note) = apis.idu_only_edge_note(s, c) { println!( - "{} consumes: {} (localhost-only: {})", + "{} consumes: {} (intra-deployment-unit-only: {})", prefix, c, note ); } else { diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index 56f0e179483..1e4c184fb7d 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -331,14 +331,14 @@ impl SystemApis { &self.api_metadata } - /// Returns the note for a localhost-only edge, if one matches. - pub fn localhost_only_edge_note( + /// Returns the note for a intra-deployment-unit-only edge, if one matches. + pub fn idu_only_edge_note( &self, server: &ServerComponentName, client: &ClientPackageName, ) -> Option<&str> { self.api_metadata - .localhost_only_edges() + .intra_deployment_unit_only_edges() .iter() .find(|edge| edge.matches(server, client)) .map(|edge| edge.note.as_str()) @@ -498,7 +498,7 @@ impl SystemApis { fn make_deployment_unit_graph( &self, dependency_filter: ApiDependencyFilter, - localhost_only_edges: Option< + idu_only_edges: Option< &BTreeSet<(ServerComponentName, ClientPackageName)>, >, ) -> Result<( @@ -522,9 +522,9 @@ impl SystemApis { for (client_pkg, _) in self.component_apis_consumed(server_pkg, dependency_filter)? { - // When localhost_only_edges is provided, we're building a + // When idu_only_edges is provided, we're building a // graph of server-side-versioned API dependencies only. - if let Some(localhost_edges) = localhost_only_edges { + if let Some(idu_edges) = idu_only_edges { let api = self .api_metadata .client_pkgname_lookup(client_pkg) @@ -533,10 +533,10 @@ impl SystemApis { continue; } - // Skip edges that represent localhost-only communication - // (communication within a deployment unit that doesn't - // cross deployment unit boundaries). - if localhost_edges + // Skip edges that represent intra-deployment-unit-only + // communication (communication within one instance of + // one deployment unit). + if idu_edges .contains(&(server_pkg.clone(), client_pkg.clone())) { continue; @@ -636,7 +636,7 @@ impl SystemApis { /// 3. The server and API producer are in the same deployment unit. /// /// Returns a set of (server, client) pairs. - fn compute_required_localhost_edges( + fn compute_required_idu_edges( &self, ) -> Result> { let filter = ApiDependencyFilter::Default; @@ -669,7 +669,7 @@ impl SystemApis { if server_unit == producer_unit { // This edge would create an intra-unit dependency for // a server-versioned API, so it must be in the - // localhost-only list. + // intra-deployment-unit-only list. required.insert((server.clone(), client.clone())); break; } @@ -680,18 +680,19 @@ impl SystemApis { Ok(required) } - /// Validates that the configured localhost_only_edges exactly match the - /// required set of edges that must be excluded. + /// Validates that the configured idu_only_edges exactly match the required + /// set of edges that must be excluded. /// - /// Returns the validated set of edges for use by make_deployment_unit_graph. - fn validate_localhost_only_edges( + /// Returns the validated set of edges for use by + /// make_deployment_unit_graph. + fn validate_idu_only_edges( &self, ) -> Result> { - let required = self.compute_required_localhost_edges()?; + let required = self.compute_required_idu_edges()?; // Build the configured set from the manifest. let mut configured = BTreeSet::new(); - for edge in self.api_metadata.localhost_only_edges() { + for edge in self.api_metadata.intra_deployment_unit_only_edges() { let ClientMatcher::Specific(client) = &edge.client; configured.insert((edge.server.clone(), client.clone())); } @@ -702,11 +703,15 @@ impl SystemApis { if !missing.is_empty() || !extra.is_empty() { let mut msg = String::from( - "localhost_only_edges configuration does not match required edges:\n", + "intra_deployment_unit_only_edges configuration does not \ + match required edges:\n", ); if !missing.is_empty() { - msg.push_str("\nMissing entries (these edges exist and need localhost-only exclusion):\n"); + msg.push_str( + "\nMissing entries (these edges exist and need \ + intra_deployment_unit_only exclusion):\n", + ); for (server, client) in &missing { msg.push_str(&format!( " - server = {:?}, client = {:?}\n", @@ -716,7 +721,10 @@ impl SystemApis { } if !extra.is_empty() { - msg.push_str("\nExtra entries (these edges don't exist or don't need exclusion):\n"); + msg.push_str( + "\nExtra entries (these edges don't exist or don't need \ + exclusion):\n", + ); for (server, client) in &extra { msg.push_str(&format!( " - server = {:?}, client = {:?}\n", @@ -785,9 +793,9 @@ impl SystemApis { // can't be part of a cycle. let filter = ApiDependencyFilter::Default; - // Validate that all configured localhost_only_edges are correct and - // match the required set exactly. - let localhost_only_edges = self.validate_localhost_only_edges()?; + // Validate that all configured intra_deployment_unit_only_edges are + // correct and match the required set exactly. + let idu_only_edges = self.validate_idu_only_edges()?; // Construct a graph where: // @@ -808,8 +816,8 @@ impl SystemApis { } // Do the same with a graph of deployment units. - let (graph, nodes) = self - .make_deployment_unit_graph(filter, Some(&localhost_only_edges))?; + let (graph, nodes) = + self.make_deployment_unit_graph(filter, Some(&idu_only_edges))?; let reverse_nodes: BTreeMap<_, _> = nodes.iter().map(|(d_u, node)| (node, d_u)).collect(); if let Err(error) = petgraph::algo::toposort(&graph, None) { From 91034321bbb3713ccdb9c76e8f46bb101119f7a2 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 5 Feb 2026 14:07:58 -0800 Subject: [PATCH 10/19] tool could be less wordy --- dev-tools/ls-apis/src/bin/ls-apis.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dev-tools/ls-apis/src/bin/ls-apis.rs b/dev-tools/ls-apis/src/bin/ls-apis.rs index b2c7844218e..7768c988e16 100644 --- a/dev-tools/ls-apis/src/bin/ls-apis.rs +++ b/dev-tools/ls-apis/src/bin/ls-apis.rs @@ -252,10 +252,10 @@ fn print_server_components<'a>( ); } for (c, path) in apis.component_apis_consumed(s, filter)? { - if let Some(note) = apis.idu_only_edge_note(s, c) { + if apis.idu_only_edge_note(s, c).is_some() { println!( - "{} consumes: {} (intra-deployment-unit-only: {})", - prefix, c, note + "{} consumes: {} (intra-deployment-unit-only)", + prefix, c ); } else { println!("{} consumes: {}", prefix, c); From 591a2055561ed3ea4826fdb5b69e8165c0aaadb4 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 5 Feb 2026 14:18:48 -0800 Subject: [PATCH 11/19] remove ClientMatcher --- dev-tools/ls-apis/src/api_metadata.rs | 39 +++------------------------ dev-tools/ls-apis/src/system_apis.rs | 4 +-- 2 files changed, 4 insertions(+), 39 deletions(-) diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index 44b353a61b7..731e1bf0cd9 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -214,7 +214,7 @@ impl TryFrom for AllApiMetadata { edge.server ); } - let client_name = edge.client.as_specific(); + let client_name = &edge.client; if !apis.contains_key(client_name) { bail!( "intra_deployment_unit_only_edges: @@ -449,39 +449,6 @@ pub enum Evaluation { Dag, } -/// Specifies which client to match in an IDU-only edge rule. -#[derive(Clone, Debug, Eq, PartialEq)] -pub enum ClientMatcher { - /// Match a specific client package. - Specific(ClientPackageName), -} - -impl<'de> Deserialize<'de> for ClientMatcher { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - let s = String::deserialize(deserializer)?; - Ok(ClientMatcher::Specific(ClientPackageName::from(s))) - } -} - -impl ClientMatcher { - /// Returns true if this matcher matches the given client package. - pub fn matches(&self, client: &ClientPackageName) -> bool { - match self { - ClientMatcher::Specific(name) => name == client, - } - } - - /// Returns the specific client name. - pub fn as_specific(&self) -> &ClientPackageName { - match self { - ClientMatcher::Specific(name) => name, - } - } -} - /// An edge that should be excluded from the deployment unit dependency graph /// because it represents communication that only happens locally within a /// single instance of a single deployment unit. @@ -491,7 +458,7 @@ pub struct IntraDeploymentUnitOnlyEdge { /// The server component that consumes the API. pub server: ServerComponentName, /// The client package consumed. - pub client: ClientMatcher, + pub client: ClientPackageName, /// Explanation of why this edge is intra-deployment-unit-only. pub note: String, } @@ -503,6 +470,6 @@ impl IntraDeploymentUnitOnlyEdge { server: &ServerComponentName, client: &ClientPackageName, ) -> bool { - self.server == *server && self.client.matches(client) + self.server == *server && self.client == *client } } diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index 1e4c184fb7d..1d5ad9f34f4 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -15,7 +15,6 @@ use crate::api_metadata::ApiConsumerStatus; use crate::api_metadata::ApiExpectedConsumer; use crate::api_metadata::ApiExpectedConsumers; use crate::api_metadata::ApiMetadata; -use crate::api_metadata::ClientMatcher; use crate::api_metadata::Evaluation; use crate::api_metadata::VersionedHow; use crate::cargo::DepPath; @@ -693,8 +692,7 @@ impl SystemApis { // Build the configured set from the manifest. let mut configured = BTreeSet::new(); for edge in self.api_metadata.intra_deployment_unit_only_edges() { - let ClientMatcher::Specific(client) = &edge.client; - configured.insert((edge.server.clone(), client.clone())); + configured.insert((edge.server.clone(), edge.client.clone())); } // Compare the two sets. From 66a2c187efed7e9ddd0c1c82aa78ba5bad232a2e Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 5 Feb 2026 14:28:06 -0800 Subject: [PATCH 12/19] use explicit enum instead of Option-with-semantics --- dev-tools/ls-apis/src/system_apis.rs | 35 ++++++++++++++++++---------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index 1d5ad9f34f4..dac636dbb4d 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -487,7 +487,8 @@ impl SystemApis { /// Returns a string that can be passed to `dot(1)` to render a graph of /// API dependencies among deployment units pub fn dot_by_unit(&self, filter: ApiDependencyFilter) -> Result { - let (graph, _) = self.make_deployment_unit_graph(filter, None)?; + let (graph, _) = + self.make_deployment_unit_graph(filter, EdgeFilter::All)?; Ok(Dot::new(&graph).to_string()) } @@ -497,9 +498,7 @@ impl SystemApis { fn make_deployment_unit_graph( &self, dependency_filter: ApiDependencyFilter, - idu_only_edges: Option< - &BTreeSet<(ServerComponentName, ClientPackageName)>, - >, + edge_filter: EdgeFilter<'_>, ) -> Result<( petgraph::graph::Graph<&DeploymentUnitName, &ClientPackageName>, BTreeMap<&DeploymentUnitName, NodeIndex>, @@ -521,20 +520,25 @@ impl SystemApis { for (client_pkg, _) in self.component_apis_consumed(server_pkg, dependency_filter)? { - // When idu_only_edges is provided, we're building a - // graph of server-side-versioned API dependencies only. - if let Some(idu_edges) = idu_only_edges { + if let EdgeFilter::DagOnly(idu_edges) = edge_filter { let api = self .api_metadata .client_pkgname_lookup(client_pkg) .unwrap(); + // Filtering DAG-only edges means ignoring everything + // that's not server-side-versioned. if api.versioned_how != VersionedHow::Server { continue; } - // Skip edges that represent intra-deployment-unit-only - // communication (communication within one instance of - // one deployment unit). + // When filtering DAG-only edges, also skip edges that + // represent intra-deployment-unit-only communication + // (communication within one instance of one deployment + // unit). That's because intra-deployment-unit edges + // would look like a cycle in the dependency graph, but + // aren't one that we care about since they're always + // referring to the same *instance* of the same + // deployment unit. if idu_edges .contains(&(server_pkg.clone(), client_pkg.clone())) { @@ -814,8 +818,10 @@ impl SystemApis { } // Do the same with a graph of deployment units. - let (graph, nodes) = - self.make_deployment_unit_graph(filter, Some(&idu_only_edges))?; + let (graph, nodes) = self.make_deployment_unit_graph( + filter, + EdgeFilter::DagOnly(&idu_only_edges), + )?; let reverse_nodes: BTreeMap<_, _> = nodes.iter().map(|(d_u, node)| (node, d_u)).collect(); if let Err(error) = petgraph::algo::toposort(&graph, None) { @@ -991,6 +997,11 @@ impl SystemApis { } } +enum EdgeFilter<'a> { + All, + DagOnly(&'a BTreeSet<(ServerComponentName, ClientPackageName)>), +} + /// Describes proposals for assigning how APIs should be versioned, based on /// heuristics applied while checking the DAG pub struct DagCheck<'a> { From 58e20d316e7864b441033a7208e46c9771ad6e7e Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 5 Feb 2026 14:58:15 -0800 Subject: [PATCH 13/19] clean up docs and user messaging around IDU validation --- dev-tools/ls-apis/api-manifest.toml | 1 + dev-tools/ls-apis/src/api_metadata.rs | 2 + dev-tools/ls-apis/src/system_apis.rs | 112 +++++++++++++------------- 3 files changed, 61 insertions(+), 54 deletions(-) diff --git a/dev-tools/ls-apis/api-manifest.toml b/dev-tools/ls-apis/api-manifest.toml index 8e1d8685f5c..340e1c43275 100644 --- a/dev-tools/ls-apis/api-manifest.toml +++ b/dev-tools/ls-apis/api-manifest.toml @@ -608,6 +608,7 @@ sled agent. # deployment unit (the host OS), but Sled Agent's use of MGS crosses different # instances of the deployment unit (i.e., different sleds). Those don't belong # in this category. +# XXX-dap need to update and then validate these ################################################################################ [[intra_deployment_unit_only_edges]] diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index 731e1bf0cd9..310cb98cdac 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -204,6 +204,8 @@ impl TryFrom for AllApiMetadata { } // Validate IDU-only edges reference known server components and APIs. + // XXX-dap validate that the client and server are in the same + // deployment unit let known_components: BTreeSet<_> = deployment_units.values().flat_map(|u| u.packages.iter()).collect(); for edge in &raw.intra_deployment_unit_only_edges { diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index dac636dbb4d..140832c6555 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -628,46 +628,34 @@ impl SystemApis { Ok((graph, nodes)) } - /// Computes the set of (server, client) edges that must be excluded from - /// the deployment unit dependency graph because they represent intra-unit - /// communication for server-side-versioned APIs. + /// Computes the set of (server, client) edges for server-side-only- + /// versioned APIs where the server and client are in the same deployment + /// unit. /// - /// These are edges where all of the below are true: - /// - /// 1. The server consumes the client. - /// 2. The API is server-side-versioned. - /// 3. The server and API producer are in the same deployment unit. - /// - /// Returns a set of (server, client) pairs. + /// See the caller for more on this. fn compute_required_idu_edges( &self, ) -> Result> { let filter = ApiDependencyFilter::Default; let mut required = BTreeSet::new(); - for server in self.server_component_units.keys() { - let Some(server_unit) = self.server_component_units.get(server) - else { - continue; - }; - + for (server, server_unit) in &self.server_component_units { for (client, _) in self.component_apis_consumed(server, filter)? { // Only consider server-side-versioned APIs. - let Some(api) = self.api_metadata.client_pkgname_lookup(client) - else { - continue; - }; + let api = self + .api_metadata + .client_pkgname_lookup(client) + .expect("consumed API must have metadata"); if api.versioned_how != VersionedHow::Server { continue; } // Check if any producer is in the same deployment unit. for producer in self.api_producers(client) { - let Some(producer_unit) = - self.server_component_units.get(producer) - else { - continue; - }; + let producer_unit = self + .server_component_units + .get(producer) + .expect("API producer must be in some deployment unit"); if server_unit == producer_unit { // This edge would create an intra-unit dependency for @@ -683,8 +671,28 @@ impl SystemApis { Ok(required) } - /// Validates that the configured idu_only_edges exactly match the required - /// set of edges that must be excluded. + /// Validates that these two sets of (server, client) edges match: + /// + /// - API dependencies found by this tool *within* a deployment unit for a + /// server-side-versioned API + /// - API dependencies annotated in the metadata as being + /// intra-deployment-unit + /// + /// Why? Recall that a server-side-only-versioned API means that the server + /// is always updated before its clients. Further, the update system does + /// not (and cannot) guarantee anything about the ordering of updates for a + /// particular kind of deployment unit. (Example: for host OS, the update + /// system does not say that any particular sleds are updated before any + /// others.) Thus, if you have a server-side-only-versioned API with a + /// client in the same deployment unit, that's only allowable if the client + /// is always talking to an instance of the server in the same *instance* of + /// the same deployment unit. A dependency from Sled Agent to Propolis in + /// the *same* host OS is okay. A dependency from Sled Agent to a Propolis + /// on a different sled is not. + /// + /// If we found a same-deployment-unit edge that's not labeled in the + /// manifest as intra-deployment-unit, that means we've identified either a + /// manifest bug or an update bug waiting to happen. /// /// Returns the validated set of edges for use by /// make_deployment_unit_graph. @@ -704,35 +712,31 @@ impl SystemApis { let extra: BTreeSet<_> = configured.difference(&required).collect(); if !missing.is_empty() || !extra.is_empty() { - let mut msg = String::from( - "intra_deployment_unit_only_edges configuration does not \ - match required edges:\n", - ); - - if !missing.is_empty() { - msg.push_str( - "\nMissing entries (these edges exist and need \ - intra_deployment_unit_only exclusion):\n", - ); - for (server, client) in &missing { - msg.push_str(&format!( - " - server = {:?}, client = {:?}\n", - server, client - )); - } + let mut msg = String::new(); + for (server, client) in missing { + msg.push_str(&format!( + "The following API dependendency exists between two \ + components in the same deployment unit, but is not \ + present in `intra_deployment_unit_only_edges`: \ + server {:?} client {:?}\n\ + If this client only ever uses this API with a server \ + in the same *instance* of the same deployment unit, \ + then add it to `intra_deployment_unit_only_edges`. \ + Otherwise, this relationship is incompatible with \ + automated upgrade.\n", + server, client, + )); } - if !extra.is_empty() { - msg.push_str( - "\nExtra entries (these edges don't exist or don't need \ - exclusion):\n", - ); - for (server, client) in &extra { - msg.push_str(&format!( - " - server = {:?}, client = {:?}\n", - server, client - )); - } + for (server, client) in extra { + msg.push_str(&format!( + "`intra_deployment_unit_only_edges` contains an edge \ + between server {:?} and client {:?}, but either \ + this API dependency was not found, or the API is not \ + server-side-only-versioned, or this client and server \ + are not in the same deployment unit.\n", + server, client, + )); } bail!("{}", msg); From 6d11b142e2a54745c820793e67013e734d042e6b Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 5 Feb 2026 16:07:19 -0800 Subject: [PATCH 14/19] validation --- Cargo.lock | 1 + dev-tools/ls-apis/Cargo.toml | 1 + dev-tools/ls-apis/src/api_metadata.rs | 10 +++--- dev-tools/ls-apis/src/system_apis.rs | 46 +++++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9637a9a9263..ea1c2ee705f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8335,6 +8335,7 @@ dependencies = [ "expectorate", "iddqd", "indent_write", + "itertools 0.14.0", "newtype_derive", "omicron-test-utils", "omicron-workspace-hack", diff --git a/dev-tools/ls-apis/Cargo.toml b/dev-tools/ls-apis/Cargo.toml index 4b24b7d1b7d..b1942608144 100644 --- a/dev-tools/ls-apis/Cargo.toml +++ b/dev-tools/ls-apis/Cargo.toml @@ -14,6 +14,7 @@ cargo_metadata.workspace = true clap.workspace = true iddqd.workspace = true indent_write.workspace = true +itertools.workspace = true newtype_derive.workspace = true parse-display.workspace = true petgraph.workspace = true diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index 310cb98cdac..33bdb3e536f 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -204,8 +204,6 @@ impl TryFrom for AllApiMetadata { } // Validate IDU-only edges reference known server components and APIs. - // XXX-dap validate that the client and server are in the same - // deployment unit let known_components: BTreeSet<_> = deployment_units.values().flat_map(|u| u.packages.iter()).collect(); for edge in &raw.intra_deployment_unit_only_edges { @@ -216,12 +214,12 @@ impl TryFrom for AllApiMetadata { edge.server ); } - let client_name = &edge.client; - if !apis.contains_key(client_name) { + + if !apis.contains_key(&edge.client) { bail!( - "intra_deployment_unit_only_edges: + "intra_deployment_unit_only_edges: \ unknown client {:?}", - client_name + edge.client, ); } } diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index 140832c6555..be90f938d35 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -27,6 +27,7 @@ use cargo_metadata::Package; use iddqd::IdOrdItem; use iddqd::IdOrdMap; use iddqd::id_upcast; +use itertools::Itertools; use parse_display::{Display, FromStr}; use petgraph::dot::Dot; use petgraph::graph::NodeIndex; @@ -286,6 +287,51 @@ impl SystemApis { } } + // Validate that the IDU-only edges' components belong to the same + // deployment unit. + for edge in api_metadata.intra_deployment_unit_only_edges() { + let server = &edge.server; + let Some(server_unit) = server_component_units.get(server) else { + // This was validated earlier, but there's not an easy way to + // express this in the type system, so we just handle it + // gracefully. + bail!( + "internal error: intra_deployment_unit_only specifies \ + server {:?} that does not exist in server components", + server, + ); + }; + + let client = &edge.client; + let Some(producers) = api_producers.get(client) else { + // This was validated earlier, but there's not an easy way to + // express this in the type system, so we just handle it + // gracefully. + bail!( + "internal error: intra_deployment_unit_only specifies \ + client {:?} that does not correspond to a known API", + client, + ); + }; + + if !producers.iter().any(|(p, _)| { + server_component_units + .get(p) + .map(|producer_unit| producer_unit == server_unit) + .unwrap_or(false) + }) { + bail!( + "error: intra_deployment_unit_only specifies server \ + {:?} in deployment unit {:?}, but none of the producers \ + of client {:?} are in that deployment_unit: {}", + server, + server_unit, + client, + producers.iter().map(|(p, _)| p.as_str()).join(", "), + ); + } + } + Ok(SystemApis { server_component_units, unit_server_components, From af29995b53bc4462139188d5958c1031c1b3cdad Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 5 Feb 2026 16:55:31 -0800 Subject: [PATCH 15/19] comment nit --- dev-tools/ls-apis/src/api_metadata.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index 33bdb3e536f..4a6e3246457 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -203,7 +203,8 @@ impl TryFrom for AllApiMetadata { } } - // Validate IDU-only edges reference known server components and APIs. + // Validate that IDU-only edges reference only known server components + // and APIs. let known_components: BTreeSet<_> = deployment_units.values().flat_map(|u| u.packages.iter()).collect(); for edge in &raw.intra_deployment_unit_only_edges { From b742226e52fdff9c266adc7dfc9c948edbadd7f7 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 5 Feb 2026 17:02:17 -0800 Subject: [PATCH 16/19] update metadata to reflect updates made before winding the clock back --- dev-tools/ls-apis/api-manifest.toml | 149 +++++++++++++++++++++----- dev-tools/ls-apis/src/api_metadata.rs | 2 + 2 files changed, 125 insertions(+), 26 deletions(-) diff --git a/dev-tools/ls-apis/api-manifest.toml b/dev-tools/ls-apis/api-manifest.toml index 340e1c43275..926217f3063 100644 --- a/dev-tools/ls-apis/api-manifest.toml +++ b/dev-tools/ls-apis/api-manifest.toml @@ -608,89 +608,186 @@ sled agent. # deployment unit (the host OS), but Sled Agent's use of MGS crosses different # instances of the deployment unit (i.e., different sleds). Those don't belong # in this category. -# XXX-dap need to update and then validate these ################################################################################ [[intra_deployment_unit_only_edges]] server = "ddmd" client = "dpd-client" -note = "verified: sled-agent configures ddmd with dpd_host=[::1] (services.rs:3127)" +note = """ +ddmd runs in both the global zone and the switch zone. The switch zone is +configured with dpd_host=[::1] (services.rs:3127). The global zone configures +"dendrite" to false, meaning it does not expose a dendrite server +(ddm/manifest.xml:25). +""" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/services.rs#L3127", + "https://github.com/oxidecomputer/maghemite/blob/396bb3c/smf/ddm/manifest.xml#L25", +] [[intra_deployment_unit_only_edges]] server = "dpd" client = "gateway-client" -note = "verified: dpd defaults to [::1]:MGS_PORT (dendrite switch_identifiers.rs:60)" +note = """ +dpd defaults to [::1]:MGS_PORT (switch_identifiers.rs:60), and the SMF start +script doesn't pass in --mgs-address. +""" +permalinks = [ + "https://github.com/oxidecomputer/dendrite/blob/606c0be/dpd/src/switch_identifiers.rs#L60", + "https://github.com/oxidecomputer/dendrite/blob/606c0be/dpd/misc/dpd.xml", + "https://github.com/oxidecomputer/dendrite/blob/606c0be/tools/svc-dpd", +] [[intra_deployment_unit_only_edges]] server = "lldpd" client = "dpd-client" -note = "verified: lldpd defaults to localhost for dpd (lldp dendrite.rs:194)" +note = """ +lldpd defaults to localhost for dpd (dendrite.rs:194), and the SMF start +script doesn't pass in --host. +""" +permalinks = [ + "https://github.com/oxidecomputer/lldp/blob/61479b6/lldpd/src/dendrite.rs#L194", + "https://github.com/oxidecomputer/lldp/blob/61479b6/lldpd/misc/lldpd.xml", + "https://github.com/oxidecomputer/lldp/blob/61479b6/lldpd/misc/svc-lldpd", +] [[intra_deployment_unit_only_edges]] server = "mgd" client = "ddm-admin-client" -note = "verified: mg-lower hardcodes localhost:8000 (mg-lower ddm.rs:110)" +note = "mg-lower hardcodes localhost:8000." +permalinks = [ + "https://github.com/oxidecomputer/maghemite/blob/396bb3c/mg-lower/src/ddm.rs#L110", +] [[intra_deployment_unit_only_edges]] server = "mgd" client = "dpd-client" -note = "verified: mg-lower hardcodes localhost (mg-lower dendrite.rs:491)" +note = "mg-lower hardcodes localhost." +permalinks = [ + "https://github.com/oxidecomputer/maghemite/blob/396bb3c/mg-lower/src/dendrite.rs#L491", +] [[intra_deployment_unit_only_edges]] server = "mgd" client = "gateway-client" -note = "verified: mgd defaults to [::1]:12225 (mgd main.rs:93)" - -# NOTE: The following sled-agent edges are BUGS - they use underlay IPs, not -# localhost. They are kept here to avoid breaking the build, but need to be -# fixed, either through client-side versioning or by some other means. +note = """ +mgd defaults to [::1]:12225 (mgd main.rs:93), and the SMF start script +doesn't set --mgs-addr. +""" +permalinks = [ + "https://github.com/oxidecomputer/maghemite/blob/396bb3c/mgd/src/main.rs#L93", + "https://github.com/oxidecomputer/maghemite/blob/396bb3c/smf/mgd/manifest.xml", + "https://github.com/oxidecomputer/maghemite/blob/396bb3c/smf/mgd_method_script.sh", +] [[intra_deployment_unit_only_edges]] server = "omicron-sled-agent" -client = "gateway-client" -note = "BUG: uses underlay IP, not localhost (early_networking.rs:376)" +client = "ddm-admin-client" +note = "Sled Agent uses Client::localhost() to connect to ddm." +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/ddm_reconciler.rs#L56", +] [[intra_deployment_unit_only_edges]] server = "omicron-sled-agent" -client = "ddm-admin-client" -note = "verified: uses Client::localhost() (ddm_reconciler.rs:56)" +client = "mg-admin-client" +note = """ +Sled Agent only queries the switch zone on the same sled, +which is within the same deployment unit as the global +zone. +""" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/bootstrap/early_networking.rs#L483", +] [[intra_deployment_unit_only_edges]] server = "omicron-sled-agent" -client = "dpd-client" -note = "BUG: uses underlay IP, not localhost (services.rs:1090)" +client = "propolis-client" +note = "Unsure: it looks like this only queries the Propolis server out of the same deployment unit?" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/instance.rs#L2283", +] + +# NOTE: The following sled-agent edges are bugs: they cross deployment units. +# They are kept here to avoid breaking the build, but need to be fixed, either +# through client-side versioning or by some other means. [[intra_deployment_unit_only_edges]] server = "omicron-sled-agent" -client = "mg-admin-client" -note = "BUG: uses underlay IP, not localhost (early_networking.rs:483)" +client = "gateway-client" +note = """ +BUG: Sled Agent creates two MGS clients. One of them +(early_networking.rs:376) queries the switch zone on +the same sled, which is within the same deployment unit +as the global zone, so this is okay. + +The other one (early_networking.rs:277) queries both +switch zones, so it crosses deployment units. This needs +to be fixed. +""" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/bootstrap/early_networking.rs#L376", + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/bootstrap/early_networking.rs#L277-L280", +] [[intra_deployment_unit_only_edges]] server = "omicron-sled-agent" -client = "propolis-client" -note = "BUG: uses zone IP, not localhost (instance.rs:2283)" +client = "dpd-client" +note = """ +BUG: Sled Agent reaches out to both switch zones, which crosses deployment units. +""" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/services.rs#L1090", +] [[intra_deployment_unit_only_edges]] server = "tfportd" client = "dpd-client" -note = "verified: configured with dpd_host=[::1] (services.rs:2852)" +note = """ +The tfportd service has the dpd_host SMF property (tfport.xml:52) set to [::1] +(services.rs:2852). tfportd has a --dpd-host option (main.rs:81) but the SMF +start script (svc-tfportd:12) does not use it. +""" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/services.rs#L2852", + "https://github.com/oxidecomputer/dendrite/blob/606c0be/tfportd/misc/tfport.xml#L52-L53", + "https://github.com/oxidecomputer/dendrite/blob/cf31be9/tfportd/src/main.rs#L81-L83", + "https://github.com/oxidecomputer/dendrite/blob/606c0be/tools/svc-tfportd#L12", +] [[intra_deployment_unit_only_edges]] server = "tfportd" client = "lldpd-client" -note = "verified: hardcodes localhost (tfportd tfport.rs:88)" +note = "tfportd hardcodes localhost for its lldpd-client (tfport.rs:88)." +permalinks = [ + "https://github.com/oxidecomputer/dendrite/blob/606c0be/tfportd/src/tfport.rs#L88", +] [[intra_deployment_unit_only_edges]] server = "wicketd" client = "ddm-admin-client" -note = "verified: uses DdmAdminClient::localhost() (bootstrap_addrs.rs:162)" +note = "wicketd uses DdmAdminClient::localhost() (bootstrap_addrs.rs:162)." +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/wicketd/src/bootstrap_addrs.rs#L162", +] [[intra_deployment_unit_only_edges]] server = "wicketd" client = "dpd-client" -note = "verified: hardcodes [::1]:DENDRITE_PORT (preflight_check/uplink.rs:83)" +note = "wicketd hardcodes [::1] (uplink.rs:83)." +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/wicketd/src/preflight_check/uplink.rs#L83", +] [[intra_deployment_unit_only_edges]] server = "wicketd" client = "gateway-client" -note = "verified: --mgs-address CLI expects localhost (bin/wicketd.rs:35)" +note = """ +wicketd's mgs-address config (wicketd.rs:35) is always set to [::1] +(services.rs:2586). This config is passed into wicketd at startup +(manifest.xml:20). +""" +permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/services.rs#L2685-L2689", + "https://github.com/oxidecomputer/omicron/blob/6cef874/wicketd/src/bin/wicketd.rs#L35", + "https://github.com/oxidecomputer/omicron/blob/6cef874/smf/wicketd/manifest.xml#L20", +] diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index 4a6e3246457..0b26f19e750 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -462,6 +462,8 @@ pub struct IntraDeploymentUnitOnlyEdge { pub client: ClientPackageName, /// Explanation of why this edge is intra-deployment-unit-only. pub note: String, + /// Permalinks to source code referenced by `note` + pub permalinks: Vec, } impl IntraDeploymentUnitOnlyEdge { From 5916ad63c6a8dedf6e88af28061efe00d27e94bf Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 5 Feb 2026 17:12:25 -0800 Subject: [PATCH 17/19] pull forward another change that was wound back --- dev-tools/ls-apis/src/system_apis.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index be90f938d35..6cb8e51e007 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -30,6 +30,7 @@ use iddqd::id_upcast; use itertools::Itertools; use parse_display::{Display, FromStr}; use petgraph::dot::Dot; +use petgraph::graph::Graph; use petgraph::graph::NodeIndex; use std::collections::BTreeMap; use std::collections::BTreeSet; @@ -546,10 +547,10 @@ impl SystemApis { dependency_filter: ApiDependencyFilter, edge_filter: EdgeFilter<'_>, ) -> Result<( - petgraph::graph::Graph<&DeploymentUnitName, &ClientPackageName>, + Graph<&DeploymentUnitName, &ClientPackageName>, BTreeMap<&DeploymentUnitName, NodeIndex>, )> { - let mut graph = petgraph::graph::Graph::new(); + let mut graph = Graph::new(); let nodes: BTreeMap<_, _> = self .deployment_units() .map(|name| (name, graph.add_node(name))) @@ -634,10 +635,10 @@ impl SystemApis { dependency_filter: ApiDependencyFilter, versioned_on_server_only: bool, ) -> Result<( - petgraph::graph::Graph<&ServerComponentName, &ClientPackageName>, + Graph<&ServerComponentName, &ClientPackageName>, BTreeMap<&ServerComponentName, NodeIndex>, )> { - let mut graph = petgraph::graph::Graph::new(); + let mut graph = Graph::new(); let nodes: BTreeMap<_, _> = self .server_component_units .keys() From a25117ada9b6e24c325fbfb27eb9484b4dd178d0 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 6 Feb 2026 19:39:20 -0800 Subject: [PATCH 18/19] review the actual metadata --- dev-tools/ls-apis/api-manifest.toml | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/dev-tools/ls-apis/api-manifest.toml b/dev-tools/ls-apis/api-manifest.toml index 926217f3063..3b93bc60c46 100644 --- a/dev-tools/ls-apis/api-manifest.toml +++ b/dev-tools/ls-apis/api-manifest.toml @@ -629,7 +629,8 @@ server = "dpd" client = "gateway-client" note = """ dpd defaults to [::1]:MGS_PORT (switch_identifiers.rs:60), and the SMF start -script doesn't pass in --mgs-address. +script doesn't pass in --mgs-address. The comment also explains that it +explicitly wants the local MGS and this can only be overridden for testing. """ permalinks = [ "https://github.com/oxidecomputer/dendrite/blob/606c0be/dpd/src/switch_identifiers.rs#L60", @@ -685,6 +686,9 @@ client = "ddm-admin-client" note = "Sled Agent uses Client::localhost() to connect to ddm." permalinks = [ "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/ddm_reconciler.rs#L56", + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/bootstrap/bootstore_setup.rs#L92", + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/rack_setup/service.rs#L1086", + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/sled_agent.rs#L1378", ] [[intra_deployment_unit_only_edges]] @@ -702,7 +706,10 @@ permalinks = [ [[intra_deployment_unit_only_edges]] server = "omicron-sled-agent" client = "propolis-client" -note = "Unsure: it looks like this only queries the Propolis server out of the same deployment unit?" +note = """ +Only queries the Propolis server on the same sled, so Propolis is in the +same deployment unit. +""" permalinks = [ "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/instance.rs#L2283", ] @@ -733,9 +740,15 @@ permalinks = [ server = "omicron-sled-agent" client = "dpd-client" note = """ -BUG: Sled Agent reaches out to both switch zones, which crosses deployment units. +BUG: Sled Agent creates two DPD clients. One of them (early_networking.rs:419) +is always to the switch zone on the same sled, which is in the same deployment +unit. + +The other one (services.rs:1090) queries both switch zones, so it crosses +deployment units. This needs to be fixed. """ permalinks = [ + "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/bootstrap/early_networking.rs#L419", "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/services.rs#L1090", ] From d4dae9c5621be36e506f16e9da100b42fb994a08 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 6 Feb 2026 20:01:30 -0800 Subject: [PATCH 19/19] add issue links; fix clippy --- dev-tools/ls-apis/api-manifest.toml | 4 ++++ dev-tools/ls-apis/src/system_apis.rs | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/dev-tools/ls-apis/api-manifest.toml b/dev-tools/ls-apis/api-manifest.toml index 3b93bc60c46..cd9cf4c997a 100644 --- a/dev-tools/ls-apis/api-manifest.toml +++ b/dev-tools/ls-apis/api-manifest.toml @@ -730,6 +730,8 @@ as the global zone, so this is okay. The other one (early_networking.rs:277) queries both switch zones, so it crosses deployment units. This needs to be fixed. + +Reference: https://github.com/oxidecomputer/omicron/issues/9708 """ permalinks = [ "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/bootstrap/early_networking.rs#L376", @@ -746,6 +748,8 @@ unit. The other one (services.rs:1090) queries both switch zones, so it crosses deployment units. This needs to be fixed. + +Reference: https://github.com/oxidecomputer/omicron/issues/9708 """ permalinks = [ "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/bootstrap/early_networking.rs#L419", diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index 6cb8e51e007..dcc43c4b80c 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -328,7 +328,7 @@ impl SystemApis { server, server_unit, client, - producers.iter().map(|(p, _)| p.as_str()).join(", "), + producers.keys().map(|p| p.as_str()).join(", "), ); } }