Skip to content

Commit

Permalink
fix ls-apis breakage (#7122)
Browse files Browse the repository at this point in the history
  • Loading branch information
davepacheco authored Nov 21, 2024
1 parent dc6b440 commit e951652
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 12 deletions.
8 changes: 6 additions & 2 deletions dev-tools/ls-apis/api-manifest.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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]]
Expand Down Expand Up @@ -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`
Expand Down
13 changes: 13 additions & 0 deletions dev-tools/ls-apis/src/api_metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,19 @@ pub struct ApiMetadata {
pub server_package_name: ServerPackageName,
/// human-readable notes about this API
pub notes: Option<String>,
/// 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<bool>,
}

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
Expand Down
53 changes: 43 additions & 10 deletions dev-tools/ls-apis/src/system_apis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -375,7 +402,8 @@ impl SystemApis {
/// See `SystemApis::load()` for how this is used.
struct ServerComponentsTracker<'a> {
// inputs
known_server_packages: &'a BTreeMap<ServerPackageName, &'a ApiMetadata>,
known_server_packages:
&'a BTreeMap<ServerPackageName, Vec<&'a ApiMetadata>>,

// outputs (structures that we're building up)
errors: Vec<anyhow::Error>,
Expand All @@ -387,7 +415,10 @@ struct ServerComponentsTracker<'a> {

impl<'a> ServerComponentsTracker<'a> {
pub fn new(
known_server_packages: &'a BTreeMap<ServerPackageName, &'a ApiMetadata>,
known_server_packages: &'a BTreeMap<
ServerPackageName,
Vec<&'a ApiMetadata>,
>,
) -> ServerComponentsTracker<'a> {
ServerComponentsTracker {
known_server_packages,
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit e951652

Please sign in to comment.