From e9516524393805a937a2126eec8cf1278e9494b8 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 20 Nov 2024 21:23:40 -0800 Subject: [PATCH] fix `ls-apis` breakage (#7122) --- dev-tools/ls-apis/api-manifest.toml | 8 +++- dev-tools/ls-apis/src/api_metadata.rs | 13 +++++++ dev-tools/ls-apis/src/system_apis.rs | 53 ++++++++++++++++++++++----- 3 files changed, 62 insertions(+), 12 deletions(-) diff --git a/dev-tools/ls-apis/api-manifest.toml b/dev-tools/ls-apis/api-manifest.toml index e3dbeed604..766708f478 100644 --- a/dev-tools/ls-apis/api-manifest.toml +++ b/dev-tools/ls-apis/api-manifest.toml @@ -96,11 +96,14 @@ label = "Crucible Pantry" packages = [ "crucible-pantry" ] [[deployment_units]] -label = "Cockroach Admin" +label = "Cockroach" packages = [ "omicron-cockroach-admin" ] +# These are really three distinct deployment units, but they behave the same for +# our purposes, and the tooling doesn't support multiple deployment units +# that each expose a particular service. [[deployment_units]] -label = "Clickhouse Admin" +label = "Clickhouse (single-node) / Clickhouse Server (multi-node) / Clickhouse Keeper (multi-node)" packages = [ "omicron-clickhouse-admin" ] [[deployment_units]] @@ -312,6 +315,7 @@ Exposed by Crucible upstairs for debugging via the `cmon` debugging tool. client_package_name = "dsc-client" label = "Downstairs Controller (debugging only)" server_package_name = "dsc" +dev_only = true notes = """ `dsc` is a control program for spinning up and controlling instances of Crucible downstairs for testing. You can use the same program to control a running `dsc` diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index d7a1c6c05b..ac9130c43c 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -199,6 +199,19 @@ pub struct ApiMetadata { pub server_package_name: ServerPackageName, /// human-readable notes about this API pub notes: Option, + /// If `dev_only` is true, then this API's server is not deployed in a + /// production system. It's only used in development environments. The + /// default (if unspecified and this comes in as `None`) is that APIs *are* + /// deployed (equivalent to `dev_only = Some(false)`). + dev_only: Option, +} + +impl ApiMetadata { + /// Returns whether this API's server component gets deployed on real + /// systems + pub fn deployed(&self) -> bool { + !(self.dev_only.unwrap_or(false)) + } } /// Describes a unit that combines one or more servers that get deployed diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index c67db1f182..5d4a84160a 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -80,12 +80,15 @@ impl SystemApis { ); } - // Create an index of server package names, mapping each one to the API - // that it corresponds to. - let server_packages: BTreeMap<_, _> = api_metadata - .apis() - .map(|api| (api.server_package_name.clone(), api)) - .collect(); + // Create an index of server package names, mapping each one to the list + // of APIs that it produces. + let mut server_packages = BTreeMap::new(); + for api in api_metadata.apis() { + server_packages + .entry(api.server_package_name.clone()) + .or_insert_with(Vec::new) + .push(api); + } // Walk the deployment units, then walk each one's list of packages, and // then walk all of its dependencies. Along the way, record whenever we @@ -160,6 +163,30 @@ impl SystemApis { let (apis_consumed, api_consumers) = (deps_tracker.apis_consumed, deps_tracker.api_consumers); + // Make sure that each API is produced by at least one producer. + for api in api_metadata.apis() { + let found_producer = api_producers.get(&api.client_package_name); + if api.deployed() { + if found_producer.is_none() { + bail!( + "error: found no producer for API with client package \ + name {:?} in any deployment unit (should have been \ + one that contains server package {:?})", + api.client_package_name, + api.server_package_name, + ); + } + } else if let Some(found) = found_producer { + bail!( + "error: metadata says there should be no deployed \ + producer for API with client package name {:?}, but found \ + one: {:?}", + api.client_package_name, + found + ); + } + } + Ok(SystemApis { server_component_units, unit_server_components, @@ -375,7 +402,8 @@ impl SystemApis { /// See `SystemApis::load()` for how this is used. struct ServerComponentsTracker<'a> { // inputs - known_server_packages: &'a BTreeMap, + known_server_packages: + &'a BTreeMap>, // outputs (structures that we're building up) errors: Vec, @@ -387,7 +415,10 @@ struct ServerComponentsTracker<'a> { impl<'a> ServerComponentsTracker<'a> { pub fn new( - known_server_packages: &'a BTreeMap, + known_server_packages: &'a BTreeMap< + ServerPackageName, + Vec<&'a ApiMetadata>, + >, ) -> ServerComponentsTracker<'a> { ServerComponentsTracker { known_server_packages, @@ -470,11 +501,13 @@ impl<'a> ServerComponentsTracker<'a> { pkgname: &str, dep_path: &DepPath, ) { - let Some(api) = self.known_server_packages.get(pkgname) else { + let Some(apis) = self.known_server_packages.get(pkgname) else { return; }; - self.found_api_producer(api, dunit_pkgname, dep_path); + for api in apis { + self.found_api_producer(api, dunit_pkgname, dep_path); + } } /// Record that the given package is one of the deployment unit's top-level