Skip to content

ls-apis needs to detect cycles in dependency unit graph #9707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
davepacheco wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

ls-apis needs to detect cycles in dependency unit graph #9707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
dfb34d4
ls-apis needs to detect cycles in dependency unit graph
davepacheco Jan 22, 2026
172dfef
remove eprintln
davepacheco Jan 22, 2026
d133557
move to api-manifest.toml
sunshowers Jan 23, 2026
5e04994
make check exhaustive
sunshowers Jan 23, 2026
d714b7d
use the dependency_filter_rules list, and add more notes and permalinks
sunshowers Jan 26, 2026
0e08578
self-review fixes
sunshowers Jan 29, 2026
c74b429
more self-review fixes
sunshowers Jan 29, 2026
c2e885a
wind back the clock to when the new metadata was a new type of thing …
davepacheco Feb 5, 2026
b7ac647
localhost -> intra-deployment-unit
davepacheco Feb 5, 2026
9103432
tool could be less wordy
davepacheco Feb 5, 2026
591a205
remove ClientMatcher
davepacheco Feb 5, 2026
66a2c18
use explicit enum instead of Option-with-semantics
davepacheco Feb 5, 2026
58e20d3
clean up docs and user messaging around IDU validation
davepacheco Feb 5, 2026
6d11b14
validation
davepacheco Feb 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions dev-tools/ls-apis/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
101 changes: 101 additions & 0 deletions dev-tools/ls-apis/api-manifest.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,3 +593,104 @@ note = """
Sled Agent uses the Crucible Agent client types only, and only in the simulated
sled agent.
"""

################################################################################
# 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 *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.
# 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)"

[[intra_deployment_unit_only_edges]]
server = "dpd"
client = "gateway-client"
Copy link
Collaborator Author

@davepacheco davepacheco Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the referenced source: it looks like someone could override this by providing a CLI argument to dpd. What do you think about seeing if we can put that CLI option behind a dev-only feature flag so that shipping code can't do this? (I'd of course talk to Levon about this before proceeding.)

(I'd do this as a follow up and just mention it in the note for now.)

Edit: maybe a simpler solution would be to remove these from the SMF start script, if that's possible? I noticed that the mgd -> dendrite dependency looks similar (can be overridden by a CLI argument), but the method script doesn't ever set the --mgs-addr option, which makes it pretty unlikely this would get broken in production.

note = "verified: dpd defaults to [::1]:MGS_PORT (dendrite switch_identifiers.rs:60)"

[[intra_deployment_unit_only_edges]]
server = "lldpd"
client = "dpd-client"
note = "verified: lldpd defaults to localhost for dpd (lldp dendrite.rs:194)"

[[intra_deployment_unit_only_edges]]
server = "mgd"
client = "ddm-admin-client"
note = "verified: mg-lower hardcodes localhost:8000 (mg-lower ddm.rs:110)"

[[intra_deployment_unit_only_edges]]
server = "mgd"
client = "dpd-client"
note = "verified: mg-lower hardcodes localhost (mg-lower dendrite.rs:491)"

[[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.

[[intra_deployment_unit_only_edges]]
server = "omicron-sled-agent"
client = "gateway-client"
note = "BUG: uses underlay IP, not localhost (early_networking.rs:376)"

[[intra_deployment_unit_only_edges]]
server = "omicron-sled-agent"
client = "ddm-admin-client"
note = "verified: uses Client::localhost() (ddm_reconciler.rs:56)"

[[intra_deployment_unit_only_edges]]
server = "omicron-sled-agent"
client = "dpd-client"
note = "BUG: uses underlay IP, not localhost (services.rs:1090)"

[[intra_deployment_unit_only_edges]]
server = "omicron-sled-agent"
client = "mg-admin-client"
note = "BUG: uses underlay IP, not localhost (early_networking.rs:483)"

[[intra_deployment_unit_only_edges]]
server = "omicron-sled-agent"
client = "propolis-client"
note = "BUG: uses zone IP, not localhost (instance.rs:2283)"

[[intra_deployment_unit_only_edges]]
server = "tfportd"
client = "dpd-client"
note = "verified: configured with dpd_host=[::1] (services.rs:2852)"

[[intra_deployment_unit_only_edges]]
server = "tfportd"
client = "lldpd-client"
note = "verified: hardcodes localhost (tfportd tfport.rs:88)"

[[intra_deployment_unit_only_edges]]
server = "wicketd"
client = "ddm-admin-client"
note = "verified: uses DdmAdminClient::localhost() (bootstrap_addrs.rs:162)"

[[intra_deployment_unit_only_edges]]
server = "wicketd"
client = "dpd-client"
note = "verified: hardcodes [::1]:DENDRITE_PORT (preflight_check/uplink.rs:83)"

[[intra_deployment_unit_only_edges]]
server = "wicketd"
client = "gateway-client"
note = "verified: --mgs-address CLI expects localhost (bin/wicketd.rs:35)"
59 changes: 58 additions & 1 deletion dev-tools/ls-apis/src/api_metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub struct AllApiMetadata {
deployment_units: BTreeMap<DeploymentUnitName, DeploymentUnitInfo>,
dependency_rules: BTreeMap<ClientPackageName, Vec<DependencyFilterRule>>,
ignored_non_clients: BTreeSet<ClientPackageName>,
intra_deployment_unit_only_edges: Vec<IntraDeploymentUnitOnlyEdge>,
}

impl AllApiMetadata {
Expand Down Expand Up @@ -73,6 +74,13 @@ impl AllApiMetadata {
&self.ignored_non_clients
}

/// 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
pub(crate) fn evaluate_dependency(
&self,
Expand Down Expand Up @@ -138,6 +146,7 @@ struct RawApiMetadata {
deployment_units: Vec<DeploymentUnitInfo>,
dependency_filter_rules: Vec<DependencyFilterRule>,
ignored_non_clients: Vec<ClientPackageName>,
intra_deployment_unit_only_edges: Vec<IntraDeploymentUnitOnlyEdge>,
}

impl TryFrom<RawApiMetadata> for AllApiMetadata {
Expand Down Expand Up @@ -188,17 +197,40 @@ impl TryFrom<RawApiMetadata> 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 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.intra_deployment_unit_only_edges {
if !known_components.contains(&edge.server) {
bail!(
"intra_deployment_unit_only_edges: \
unknown server component {:?}",
edge.server
);
}

if !apis.contains_key(&edge.client) {
bail!(
"intra_deployment_unit_only_edges: \
unknown client {:?}",
edge.client,
);
}
}

Ok(AllApiMetadata {
apis,
deployment_units,
dependency_rules,
ignored_non_clients,
intra_deployment_unit_only_edges: raw
.intra_deployment_unit_only_edges,
})
}
}
Expand Down Expand Up @@ -416,3 +448,28 @@ pub enum Evaluation {
/// This dependency should be part of the update DAG
Dag,
}

/// 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.
#[derive(Deserialize)]
#[serde(deny_unknown_fields)]
pub struct IntraDeploymentUnitOnlyEdge {
/// The server component that consumes the API.
pub server: ServerComponentName,
/// The client package consumed.
pub client: ClientPackageName,
/// Explanation of why this edge is intra-deployment-unit-only.
pub note: String,
}

impl IntraDeploymentUnitOnlyEdge {
/// 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 == *client
}
}
9 changes: 8 additions & 1 deletion dev-tools/ls-apis/src/bin/ls-apis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,14 @@ fn print_server_components<'a>(
);
}
for (c, path) in apis.component_apis_consumed(s, filter)? {
println!("{} consumes: {}", prefix, c);
if apis.idu_only_edge_note(s, c).is_some() {
println!(
"{} consumes: {} (intra-deployment-unit-only)",
prefix, c
);
} else {
println!("{} consumes: {}", prefix, c);
}
if show_deps {
for p in path.nodes() {
println!("{} via: {}", prefix, p);
Expand Down
Loading
Loading