From cd0a92a6c45fdd29e4cdac23f5c4ff04b7904275 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 18 Nov 2025 00:51:13 +0000 Subject: [PATCH 1/2] [spr] initial version Created using spr 1.3.6-beta.1 --- Cargo.lock | 1 + dev-tools/ls-apis/Cargo.toml | 1 + dev-tools/ls-apis/api-manifest.toml | 6 + dev-tools/ls-apis/src/api_metadata.rs | 111 +++++++++ dev-tools/ls-apis/src/bin/ls-apis.rs | 114 +++++++-- dev-tools/ls-apis/src/lib.rs | 8 +- dev-tools/ls-apis/src/plural.rs | 13 + dev-tools/ls-apis/src/system_apis.rs | 243 ++++++++++++++++--- dev-tools/ls-apis/tests/api_check.out | 45 ++++ dev-tools/ls-apis/tests/api_dependencies.out | 1 + dev-tools/ls-apis/tests/test_dependencies.rs | 11 + 11 files changed, 508 insertions(+), 46 deletions(-) create mode 100644 dev-tools/ls-apis/src/plural.rs create mode 100644 dev-tools/ls-apis/tests/api_check.out diff --git a/Cargo.lock b/Cargo.lock index 16c9c9ce22e..479e1b993c3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8205,6 +8205,7 @@ dependencies = [ "cargo_metadata 0.21.0", "clap", "expectorate", + "iddqd", "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 7f34eef3fce..c0cf7246029 100644 --- a/dev-tools/ls-apis/Cargo.toml +++ b/dev-tools/ls-apis/Cargo.toml @@ -12,6 +12,7 @@ anyhow.workspace = true camino.workspace = true cargo_metadata.workspace = true clap.workspace = true +iddqd.workspace = true newtype_derive.workspace = true parse-display.workspace = true petgraph.workspace = true diff --git a/dev-tools/ls-apis/api-manifest.toml b/dev-tools/ls-apis/api-manifest.toml index 45984bb053d..3143acd0897 100644 --- a/dev-tools/ls-apis/api-manifest.toml +++ b/dev-tools/ls-apis/api-manifest.toml @@ -347,6 +347,12 @@ server_package_name = "nexus-lockstep-api" # dependency with sled-agent-client, which is server-versioned. versioned_how = "client" versioned_how_reason = "Circular dependencies between Nexus and other services" +# Exactly these consumers (Rust packages specified in one or more of the +# `packages` arrays in the deployment_units table above) are allowed to use this +# API. ls-apis check will error out if the list of consumers is incorrect. +consumers = [ + { name = "omicron-sled-agent", reason = "RSS uses it, which isn't part of upgrade" }, +] [[apis]] client_package_name = "oxide-client" diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index fc83942d707..8ba068c3523 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -228,6 +228,13 @@ pub struct ApiMetadata { pub label: String, /// package name of the server that provides the corresponding API pub server_package_name: ServerPackageName, + /// expected consumers (Rust packages) that use this API + /// + /// By default, we don't make any assertions about expected consumers. But + /// some APIs must have a fixed list of consumers, and we assert on that + /// via this array. + #[serde(default)] + pub consumers: ApiExpectedConsumers, /// human-readable notes about this API pub notes: Option, /// describes how we've decided this API will be versioned @@ -248,6 +255,110 @@ impl ApiMetadata { } } +/// Expected consumers (Rust packages) for an API. +#[derive(Debug, Default)] +pub enum ApiExpectedConsumers { + /// No assertions are made about consumers. + #[default] + Any, + /// Exactly these consumers are allowed. + Exactly(Vec), +} + +impl ApiExpectedConsumers { + pub fn status( + &self, + server_pkgname: &ServerComponentName, + ) -> ApiConsumerStatus { + match self { + ApiExpectedConsumers::Any => ApiConsumerStatus::NoAssertion, + ApiExpectedConsumers::Exactly(consumers) => { + if let Some(consumer) = + consumers.iter().find(|c| c.name == *server_pkgname) + { + ApiConsumerStatus::Expected { + reason: consumer.reason.clone(), + } + } else { + ApiConsumerStatus::Unexpected + } + } + } + } +} + +impl<'de> Deserialize<'de> for ApiExpectedConsumers { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + use serde::de::Error; + + struct ApiExpectedConsumersVisitor; + + impl<'de> serde::de::Visitor<'de> for ApiExpectedConsumersVisitor { + type Value = ApiExpectedConsumers; + + fn expecting( + &self, + formatter: &mut std::fmt::Formatter, + ) -> std::fmt::Result { + formatter.write_str( + "null (for no assertions) or a list of Rust package names", + ) + } + + fn visit_unit(self) -> Result + where + E: Error, + { + Ok(ApiExpectedConsumers::Any) + } + + fn visit_none(self) -> Result + where + E: Error, + { + Ok(ApiExpectedConsumers::Any) + } + + fn visit_seq(self, seq: A) -> Result + where + A: serde::de::SeqAccess<'de>, + { + let consumers = Vec::::deserialize( + serde::de::value::SeqAccessDeserializer::new(seq), + )?; + Ok(ApiExpectedConsumers::Exactly(consumers)) + } + } + + deserializer.deserialize_any(ApiExpectedConsumersVisitor) + } +} + +/// Describes a single allowed consumer for an API. +#[derive(Deserialize, Debug)] +#[serde(deny_unknown_fields)] +pub struct ApiExpectedConsumer { + /// The name of the Rust package. + pub name: ServerComponentName, + /// The reason this consumer is allowed. + pub reason: String, +} + +/// The status of an API consumer that was discovered by walking the Cargo +/// metadata graph. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum ApiConsumerStatus { + /// No assertions were made about this API consumer. + NoAssertion, + /// The API consumer is expected to be used. + Expected { reason: String }, + /// The API consumer was not expected. This is an error case. + Unexpected, +} + /// Describes a unit that combines one or more servers that get deployed /// together #[derive(Deserialize)] diff --git a/dev-tools/ls-apis/src/bin/ls-apis.rs b/dev-tools/ls-apis/src/bin/ls-apis.rs index 0bac6a48ebd..939339a297b 100644 --- a/dev-tools/ls-apis/src/bin/ls-apis.rs +++ b/dev-tools/ls-apis/src/bin/ls-apis.rs @@ -8,8 +8,9 @@ use anyhow::{Context, Result, bail}; use camino::Utf8PathBuf; use clap::{Args, Parser, Subcommand}; use omicron_ls_apis::{ - AllApiMetadata, ApiDependencyFilter, ApiMetadata, LoadArgs, - ServerComponentName, SystemApis, VersionedHow, + AllApiMetadata, ApiConsumerStatus, ApiDependencyFilter, ApiMetadata, + FailedConsumerCheck, LoadArgs, ServerComponentName, SystemApis, + VersionedHow, plural, }; use parse_display::{Display, FromStr}; @@ -119,11 +120,11 @@ fn run_adoc(apis: &SystemApis) -> Result<()> { println!("|{}", apis.adoc_label(&api.client_package_name)?); println!("|"); - for (c, _) in apis.api_consumers( + for consumer in apis.api_consumers( &api.client_package_name, ApiDependencyFilter::default(), )? { - println!("* {}", apis.adoc_label(c)?); + println!("* {}", apis.adoc_label(&consumer.server_pkgname)?); } match &api.versioned_how { @@ -145,24 +146,25 @@ fn run_adoc(apis: &SystemApis) -> Result<()> { } fn run_apis(apis: &SystemApis, args: ShowDepsArgs) -> Result<()> { + let mut unexpected_count = 0; + let metadata = apis.api_metadata(); for api in metadata.apis() { println!("{} (client: {})", api.label, api.client_package_name); - for (s, dep_paths) in - apis.api_consumers(&api.client_package_name, args.filter)? - { - let (repo_name, package_path) = apis.package_label(s)?; + for c in apis.api_consumers(&api.client_package_name, args.filter)? { + let (repo_name, package_path) = + apis.package_label(c.server_pkgname)?; println!( " consumed by: {} ({}/{}) via {} path{}", - s, + c.server_pkgname, repo_name, package_path, - dep_paths.len(), - if dep_paths.len() == 1 { "" } else { "s" }, + c.dep_paths.len(), + if c.dep_paths.len() == 1 { "" } else { "s" }, ); if args.show_deps { - for (i, dep_path) in dep_paths.iter().enumerate() { - let label = if dep_paths.len() > 1 { + for (i, dep_path) in c.dep_paths.iter().enumerate() { + let label = if c.dep_paths.len() > 1 { format!(" path {}", i + 1) } else { String::new() @@ -173,9 +175,28 @@ fn run_apis(apis: &SystemApis, args: ShowDepsArgs) -> Result<()> { } } } + match c.status { + ApiConsumerStatus::NoAssertion => { + // We don't know whether it's okay for this API to be + // present. + } + ApiConsumerStatus::Expected { reason } => { + println!(" status: expected, reason: {reason}"); + } + ApiConsumerStatus::Unexpected => { + println!(" status: UNEXPECTED"); + unexpected_count += 1; + } + } } println!(""); } + if unexpected_count > 0 { + bail!( + "{unexpected_count} unexpected {} reported (see above)", + plural::consumers_str(unexpected_count) + ); + } Ok(()) } @@ -311,6 +332,40 @@ fn run_check(apis: &SystemApis) -> Result<()> { println!(")"); } + fn print_failed_consumer_check( + api: &ApiMetadata, + check: &FailedConsumerCheck, + apis: &SystemApis, + ) { + println!(" {} ({}):", api.label, api.client_package_name); + for consumer in &check.unexpected { + let deployment_unit = + apis.server_component_unit(consumer).unwrap_or_else(|| { + panic!( + "consumer {consumer} doesn't have an associated \ + deployment unit (this is checked at load time)" + ); + }); + println!( + " unexpected dependency on {consumer} (part of {deployment_unit})" + ); + } + for (consumer, reason) in &check.missing { + let deployment_unit = + apis.server_component_unit(consumer).unwrap_or_else(|| { + panic!( + "consumer {consumer} doesn't have an associated \ + deployment unit (this is checked at load time)" + ); + }); + println!( + " missing expected dependency on {consumer} \ + (part of {deployment_unit})" + ); + println!(" reason this consumer is expected: {reason}"); + } + } + println!("\n"); println!("Server-managed APIs:\n"); for api in apis @@ -330,6 +385,8 @@ fn run_check(apis: &SystemApis) -> Result<()> { } } + let mut error_count = 0; + println!("\n"); print!("APIs with unknown version management:"); let unknown: Vec<_> = apis @@ -343,8 +400,37 @@ fn run_check(apis: &SystemApis) -> Result<()> { println!("\n"); for api in unknown { print_api_and_producers(api, apis); + error_count += 1; + } + } + + println!("\n"); + print!("APIs that failed consumer checks:"); + if dag_check.failed_consumers().is_empty() { + println!(" none"); + } else { + println!("\n"); + for c in dag_check.failed_consumers() { + let api = apis + .api_metadata() + .client_pkgname_lookup(c.client_pkgname) + .unwrap_or_else(|| { + panic!( + "client package name {} not found in API metadata", + c.client_pkgname + ) + }); + print_failed_consumer_check(api, c, apis); + error_count += 1; } - bail!("at least one API has unknown version strategy (see above)"); + } + + if error_count > 0 { + println!("\n"); + bail!( + "{error_count} {} reported (see above)", + plural::errors_str(error_count) + ); } Ok(()) diff --git a/dev-tools/ls-apis/src/lib.rs b/dev-tools/ls-apis/src/lib.rs index 403f7324e64..62449be95c9 100644 --- a/dev-tools/ls-apis/src/lib.rs +++ b/dev-tools/ls-apis/src/lib.rs @@ -6,13 +6,17 @@ mod api_metadata; mod cargo; +pub mod plural; mod system_apis; mod workspaces; pub use api_metadata::AllApiMetadata; +pub use api_metadata::ApiConsumerStatus; +pub use api_metadata::ApiExpectedConsumer; pub use api_metadata::ApiMetadata; pub use api_metadata::VersionedHow; pub use system_apis::ApiDependencyFilter; +pub use system_apis::FailedConsumerCheck; pub use system_apis::SystemApis; use anyhow::{Context, Result}; @@ -25,7 +29,7 @@ use std::borrow::Borrow; #[macro_use] extern crate newtype_derive; -#[derive(Clone, Deserialize, Ord, PartialOrd, Eq, PartialEq)] +#[derive(Clone, Deserialize, Hash, Ord, PartialOrd, Eq, PartialEq)] #[serde(transparent)] pub struct ClientPackageName(String); NewtypeDebug! { () pub struct ClientPackageName(String); } @@ -62,7 +66,7 @@ impl Borrow for ServerPackageName { } } -#[derive(Clone, Deserialize, Ord, PartialOrd, Eq, PartialEq)] +#[derive(Clone, Deserialize, Hash, Ord, PartialOrd, Eq, PartialEq)] #[serde(transparent)] pub struct ServerComponentName(String); NewtypeDebug! { () pub struct ServerComponentName(String); } diff --git a/dev-tools/ls-apis/src/plural.rs b/dev-tools/ls-apis/src/plural.rs new file mode 100644 index 00000000000..20d01e8d651 --- /dev/null +++ b/dev-tools/ls-apis/src/plural.rs @@ -0,0 +1,13 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Helpers for pluralization of text. + +pub fn errors_str(count: usize) -> &'static str { + if count == 1 { "error" } else { "errors" } +} + +pub fn consumers_str(count: usize) -> &'static str { + if count == 1 { "consumer" } else { "consumers" } +} diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index 7dec541cd6c..ef5f39b38d8 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -11,6 +11,9 @@ use crate::LoadArgs; use crate::ServerComponentName; use crate::ServerPackageName; use crate::api_metadata::AllApiMetadata; +use crate::api_metadata::ApiConsumerStatus; +use crate::api_metadata::ApiExpectedConsumer; +use crate::api_metadata::ApiExpectedConsumers; use crate::api_metadata::ApiMetadata; use crate::api_metadata::Evaluation; use crate::api_metadata::VersionedHow; @@ -21,6 +24,9 @@ use anyhow::Result; use anyhow::{Context, anyhow, bail}; use camino::Utf8PathBuf; use cargo_metadata::Package; +use iddqd::IdOrdItem; +use iddqd::IdOrdMap; +use iddqd::id_upcast; use parse_display::{Display, FromStr}; use petgraph::dot::Dot; use petgraph::graph::NodeIndex; @@ -46,10 +52,7 @@ pub struct SystemApis { /// maps an API name (using the client package name as primary key) to the /// list of server components that use it /// (reverse of `apis_consumed`) - api_consumers: BTreeMap< - ClientPackageName, - BTreeMap>, - >, + api_consumers: BTreeMap>, /// maps an API name to the server component(s) that expose that API api_producers: BTreeMap, @@ -62,6 +65,54 @@ pub struct SystemApis { type ApiProducerMap = BTreeMap>; +#[derive(Debug)] +struct ApiConsumer { + server_pkgname: ServerComponentName, + dep_paths: Vec, + status: ApiConsumerStatus, +} + +impl ApiConsumer { + fn new(name: ServerComponentName, status: ApiConsumerStatus) -> Self { + Self { server_pkgname: name, dep_paths: Vec::new(), status } + } + + fn add_path(&mut self, path: DepPath) { + self.dep_paths.push(path); + } +} + +impl IdOrdItem for ApiConsumer { + type Key<'a> = &'a ServerComponentName; + fn key(&self) -> Self::Key<'_> { + &self.server_pkgname + } + id_upcast!(); +} + +/// Public form of an API consumer. +#[derive(Debug)] +pub struct ApiConsumerRef<'a> { + /// The name of the consumer. + pub server_pkgname: &'a ServerComponentName, + /// The list of paths through which this consumer depends on the client, + /// after filters have been applied. + pub dep_paths: Vec<&'a DepPath>, + /// The status of the consumer, such as whether it is expected to be present. + pub status: &'a ApiConsumerStatus, +} + +impl<'a> IdOrdItem for ApiConsumerRef<'a> { + type Key<'b> + = &'b ServerComponentName + where + Self: 'b; + fn key(&self) -> Self::Key<'_> { + self.server_pkgname + } + id_upcast!(); +} + impl SystemApis { /// Load information about APIs in the system based on both developer- /// maintained metadata and Cargo-provided metadata @@ -130,6 +181,28 @@ impl SystemApis { tracker.api_producers, ); + // Ensure that if allowed_consumers is defined, all consumers listed are + // specified by at least one deployment unit. + for api in api_metadata.apis() { + match &api.consumers { + ApiExpectedConsumers::Any => {} + ApiExpectedConsumers::Exactly(consumers) => { + for consumer in consumers { + if !server_component_units.contains_key(&consumer.name) + { + bail!( + "api {} specifies unknown consumer: {} \ + (with expected reason: {})", + api.client_package_name, + consumer.name, + consumer.reason, + ); + } + } + } + } + } + // Now that we've figured out what servers are where, walk dependencies // of each server component and assemble structures to find which APIs // are produced and consumed by which components. @@ -202,6 +275,14 @@ impl SystemApis { self.unit_server_components.keys() } + /// Get the deployment unit associated with a server component + pub fn server_component_unit( + &self, + server_component: &ServerComponentName, + ) -> Option<&DeploymentUnitName> { + self.server_component_units.get(server_component) + } + /// For one deployment unit, iterate over the servers contained in it pub fn deployment_unit_servers( &self, @@ -274,18 +355,16 @@ impl SystemApis { &self, client: &ClientPackageName, filter: ApiDependencyFilter, - ) -> Result< - impl Iterator)> + '_ + use<'_>, - > { - let mut rv = Vec::new(); + ) -> Result>> { + let mut rv = IdOrdMap::new(); let Some(api_consumers) = self.api_consumers.get(client) else { - return Ok(rv.into_iter()); + return Ok(rv); }; - for (server_pkgname, dep_paths) in api_consumers { + for api_consumer in api_consumers { let mut include = Vec::new(); - for p in dep_paths { + for p in &api_consumer.dep_paths { if filter.should_include( &self.api_metadata, &self.workspaces, @@ -297,11 +376,16 @@ impl SystemApis { } if !include.is_empty() { - rv.push((server_pkgname, include)) + rv.insert_unique(ApiConsumerRef { + server_pkgname: &api_consumer.server_pkgname, + dep_paths: include, + status: &api_consumer.status, + }) + .expect("api_consumers is uniquely indexed by server_pkgname"); } } - Ok(rv.into_iter()) + Ok(rv) } /// Given the client package name for an API and the name of a server @@ -555,16 +639,49 @@ impl SystemApis { } continue; } + + for consumer in + self.api_consumers(&api.client_package_name, filter)? + { + // Are there any unexpected consumers? + match consumer.status { + ApiConsumerStatus::NoAssertion + | ApiConsumerStatus::Expected { .. } => {} + ApiConsumerStatus::Unexpected => { + dag_check.report_unexpected_consumer( + &api.client_package_name, + consumer.server_pkgname, + ); + } + } + } + + // Are there any missing consumers? + match &api.consumers { + ApiExpectedConsumers::Any => {} + ApiExpectedConsumers::Exactly(consumers) => { + for expected in consumers { + if !self + .api_consumers(&api.client_package_name, filter)? + .contains_key(&expected.name) + { + dag_check.report_missing_consumer( + &api.client_package_name, + expected, + ); + } + } + } + } + for producer in self.api_producers(&api.client_package_name) { let apis_consumed: BTreeSet<_> = self .component_apis_consumed(producer, filter)? .map(|(client_pkgname, _dep_path)| client_pkgname) .collect(); - let dependents: BTreeSet<_> = self + let consumers = self .api_consumers(&api.client_package_name, filter) - .unwrap() - .map(|(dependent, _dep_path)| dependent) - .collect(); + .unwrap(); if api.versioned_how == VersionedHow::Unknown { // If we haven't determined how to manage versioning on this @@ -590,7 +707,7 @@ impl SystemApis { &api.client_package_name, String::from("depends on itself"), ); - } else if dependents.is_empty() { + } else if consumers.is_empty() { // If something has no consumers in deployed components, it // can be server-managed. (These are generally debug APIs.) dag_check.propose_server( @@ -616,9 +733,9 @@ impl SystemApis { // produced by component C1, which uses API A2 produced by C2, which // also uses A1). In such cases, either A1 or A2 must be // client-managed (or both). - for other_pkgname in dependents { + for c in consumers { if let Some(dependency_clientpkg) = - dependencies.get(other_pkgname) + dependencies.get(c.server_pkgname) { let dependency_api = self .api_metadata @@ -682,6 +799,8 @@ pub struct DagCheck<'a> { /// most once in this structure. proposed_upick: BTreeMap<&'a ClientPackageName, BTreeSet<&'a ClientPackageName>>, + /// clients that failed assertions about consumers + failed_consumers: IdOrdMap>, } impl<'a> DagCheck<'a> { @@ -690,6 +809,7 @@ impl<'a> DagCheck<'a> { proposed_server_managed: BTreeMap::new(), proposed_client_managed: BTreeMap::new(), proposed_upick: BTreeMap::new(), + failed_consumers: IdOrdMap::new(), } } @@ -747,6 +867,30 @@ impl<'a> DagCheck<'a> { .insert(client_pkgname2); } + fn report_unexpected_consumer( + &mut self, + client_pkgname: &'a ClientPackageName, + consumer_name: &'a ServerComponentName, + ) { + self.failed_consumers + .entry(client_pkgname) + .or_insert_with(|| FailedConsumerCheck::new(client_pkgname)) + .unexpected + .insert(consumer_name); + } + + fn report_missing_consumer( + &mut self, + client_pkgname: &'a ClientPackageName, + expected: &'a ApiExpectedConsumer, + ) { + self.failed_consumers + .entry(client_pkgname) + .or_insert_with(|| FailedConsumerCheck::new(client_pkgname)) + .missing + .insert(&expected.name, &expected.reason); + } + /// Returns a list of APIs (identified by client package name) that look /// like they could use server-side versioning, along with reasons pub fn proposed_server_managed( @@ -776,6 +920,47 @@ impl<'a> DagCheck<'a> { .iter() .flat_map(|(c, others)| others.iter().map(|o| (*c, *o))) } + + /// Returns the map of all client package names and consumers that failed + /// consumers checks. + pub fn failed_consumers(&self) -> &IdOrdMap> { + &self.failed_consumers + } +} + +/// Information about an API that failed consumers checks. +#[derive(Clone, Debug)] +pub struct FailedConsumerCheck<'a> { + /// The API's client package name. + pub client_pkgname: &'a ClientPackageName, + /// Components that actually consume this API but that were not present in + /// the list of expected consumers. + pub unexpected: BTreeSet<&'a ServerComponentName>, + /// Components that were expected to consume this API but that were not + /// present in the list of actual consumers, along with the reason they + /// should be present. + pub missing: BTreeMap<&'a ServerComponentName, &'a str>, +} + +impl<'a> IdOrdItem for FailedConsumerCheck<'a> { + type Key<'b> + = &'a ClientPackageName + where + Self: 'b; + fn key(&self) -> Self::Key<'_> { + self.client_pkgname + } + id_upcast!(); +} + +impl<'a> FailedConsumerCheck<'a> { + pub fn new(client_pkgname: &'a ClientPackageName) -> Self { + Self { + client_pkgname, + unexpected: BTreeSet::new(), + missing: BTreeMap::new(), + } + } } /// Helper for building structures to index which deployment units contain which @@ -924,10 +1109,7 @@ struct ClientDependenciesTracker<'a> { ServerComponentName, BTreeMap>, >, - api_consumers: BTreeMap< - ClientPackageName, - BTreeMap>, - >, + api_consumers: BTreeMap>, } impl<'a> ClientDependenciesTracker<'a> { @@ -951,18 +1133,19 @@ impl<'a> ClientDependenciesTracker<'a> { pkgname: &str, dep_path: &DepPath, ) { - if self.api_metadata.client_pkgname_lookup(pkgname).is_none() { + let Some(api) = self.api_metadata.client_pkgname_lookup(pkgname) else { return; - } + }; // This is the name of a known client package. Record it. + let status = api.consumers.status(server_pkgname); let client_pkgname = ClientPackageName::from(pkgname.to_owned()); self.api_consumers .entry(client_pkgname.clone()) - .or_insert_with(BTreeMap::new) - .entry(server_pkgname.clone()) - .or_insert_with(Vec::new) - .push(dep_path.clone()); + .or_insert_with(IdOrdMap::new) + .entry(&server_pkgname) + .or_insert_with(|| ApiConsumer::new(server_pkgname.clone(), status)) + .add_path(dep_path.clone()); self.apis_consumed .entry(server_pkgname.clone()) .or_insert_with(BTreeMap::new) diff --git a/dev-tools/ls-apis/tests/api_check.out b/dev-tools/ls-apis/tests/api_check.out new file mode 100644 index 00000000000..cb06720164f --- /dev/null +++ b/dev-tools/ls-apis/tests/api_check.out @@ -0,0 +1,45 @@ + + +Server-managed APIs: + + Clickhouse Cluster Admin for Keepers (clickhouse-admin-keeper-client, exposed by omicron-clickhouse-admin) + Clickhouse Cluster Admin for Servers (clickhouse-admin-server-client, exposed by omicron-clickhouse-admin) + Clickhouse Single-Node Cluster Admin (clickhouse-admin-single-client, exposed by omicron-clickhouse-admin) + CockroachDB Cluster Admin (cockroach-admin-client, exposed by omicron-cockroach-admin) + Crucible Agent (crucible-agent-client, exposed by crucible-agent) + Crucible Control (for testing only) (crucible-control-client, exposed by propolis-server) + Crucible Pantry (crucible-pantry-client, exposed by crucible-pantry) + Maghemite DDM Admin (ddm-admin-client, exposed by ddmd) + DNS Server (dns-service-client, exposed by dns-server) + Dendrite DPD (dpd-client, exposed by dpd) + Management Gateway Service (gateway-client, exposed by omicron-gateway) + LLDP daemon (lldpd-client, exposed by lldpd) + Maghemite MG Admin (mg-admin-client, exposed by mgd) + NTP Admin (ntp-admin-client, exposed by omicron-ntp-admin) + External API (oxide-client, exposed by omicron-nexus) + Oximeter (oximeter-client, exposed by oximeter-collector) + Propolis (propolis-client, exposed by propolis-server) + Sled Agent (sled-agent-client, exposed by omicron-sled-agent) + Wicketd (wicketd-client, exposed by wicketd) + + +Client-managed API: + + Bootstrap Agent (bootstrap-agent-client, exposed by omicron-sled-agent) + reason: depends on itself (i.e., instances call each other) + Wicketd Installinator (installinator-client, exposed by wicketd) + reason: client is provided implicitly by the operator + Nexus Internal API (nexus-client, exposed by omicron-nexus) + reason: Circular dependencies between Nexus and other services + Nexus Internal Lockstep API (nexus-lockstep-client, exposed by omicron-nexus) + reason: Circular dependencies between Nexus and other services + Crucible Repair (repair-client, exposed by crucible-downstairs) + reason: depends on itself (i.e., instances call each other) + Repo Depot API (repo-depot-client, exposed by omicron-sled-agent) + reason: depends on itself (i.e., instances call each other) + + +APIs with unknown version management: none + + +APIs that failed consumer checks: none diff --git a/dev-tools/ls-apis/tests/api_dependencies.out b/dev-tools/ls-apis/tests/api_dependencies.out index ac6ae2d8b9e..94a582d809d 100644 --- a/dev-tools/ls-apis/tests/api_dependencies.out +++ b/dev-tools/ls-apis/tests/api_dependencies.out @@ -70,6 +70,7 @@ Nexus Internal API (client: nexus-client) Nexus Internal Lockstep API (client: nexus-lockstep-client) consumed by: omicron-sled-agent (omicron/sled-agent) via 1 path + status: expected, reason: RSS uses it, which isn't part of upgrade NTP Admin (client: ntp-admin-client) consumed by: omicron-nexus (omicron/nexus) via 2 paths diff --git a/dev-tools/ls-apis/tests/test_dependencies.rs b/dev-tools/ls-apis/tests/test_dependencies.rs index 3497e7b807d..3c6a4486e82 100644 --- a/dev-tools/ls-apis/tests/test_dependencies.rs +++ b/dev-tools/ls-apis/tests/test_dependencies.rs @@ -27,3 +27,14 @@ fn test_api_dependencies() { println!("stderr:\n------\n{}\n-----", stderr_text); expectorate::assert_contents("tests/api_dependencies.out", &stdout_text); } + +#[test] +fn test_api_check() { + let cmd_path = path_to_executable(CMD_LS_APIS); + let exec = subprocess::Exec::cmd(cmd_path).arg("check"); + let (exit_status, stdout_text, stderr_text) = run_command(exec); + assert_exit_code(exit_status, EXIT_SUCCESS, &stderr_text); + + println!("stderr:\n------\n{}\n-----", stderr_text); + expectorate::assert_contents("tests/api_check.out", &stdout_text); +} From ff1b60251b0159a4bd02c43352f4a3bc4e11b981 Mon Sep 17 00:00:00 2001 From: Rain Date: Wed, 19 Nov 2025 17:45:44 -0800 Subject: [PATCH 2/2] expectorate Created using spr 1.3.6-beta.1 --- dev-tools/ls-apis/tests/api_dependencies.out | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-tools/ls-apis/tests/api_dependencies.out b/dev-tools/ls-apis/tests/api_dependencies.out index 94a582d809d..94197641c23 100644 --- a/dev-tools/ls-apis/tests/api_dependencies.out +++ b/dev-tools/ls-apis/tests/api_dependencies.out @@ -70,7 +70,7 @@ Nexus Internal API (client: nexus-client) Nexus Internal Lockstep API (client: nexus-lockstep-client) consumed by: omicron-sled-agent (omicron/sled-agent) via 1 path - status: expected, reason: RSS uses it, which isn't part of upgrade + status: expected, reason: Only RSS uses this API and it is not part of upgrade NTP Admin (client: ntp-admin-client) consumed by: omicron-nexus (omicron/nexus) via 2 paths