Show example cycle when ls-apis errors about them#10235
Open
Conversation
`cargo xtask ls-apis check` will report an error if the software component graph contains a cycle, as well as if the deployment units form a cycle, but prints only the name of a node in the graph that was in the cycle. If you've added a cycle to either of these graphs, the error and node can tell you about the what that is wrong. Going a bit further and printing an example cycle including the troublesome node can help with the how you got there. With this change, the first approach at #10231 (which introduced a cycle in, but all in one deployment unit: sled-agent -> propolis-server -> sled-agent) errors with a more clear message about propolis-server and sled-agent being the problems: ``` Error: graph of server-managed API dependencies between components has a cycle (includes node: "propolis-server", example cycle: propolis-server, omicron-sled-agent) ```
davepacheco
reviewed
Apr 9, 2026
| let reverse_nodes: BTreeMap<_, _> = | ||
| nodes.iter().map(|(s_c, node)| (node, s_c)).collect(); | ||
| if let Err(error) = petgraph::algo::toposort(&graph, None) { | ||
| let example_cycle = |
Collaborator
There was a problem hiding this comment.
I think we're only constructing reverse_nodes and calling toposort to look for a cycle. Rather than use toposort to detect a cycle and then tarjan_scc to print more information, how about just using tarjan_scc to look for cycles and construct a printed summary if it finds one?
I think it's worth a one-line mention of the relationship between cycles and SCCs -- I didn't remember the connection off the top of my head. (If anybody else forgot, too: a node participates in a cycle if it (1) has a self-loop, or (2) is in an SCC with at least one other node.)
Comment on lines
+137
to
+139
| let Some(first_cycle) = sccs.get(0) else { | ||
| return None; | ||
| }; |
Collaborator
There was a problem hiding this comment.
I definitely forgot all of this, but: doesn't every node appear in at least one SCC?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cargo xtask ls-apis checkwill report an error if the software component graph contains a cycle, as well as if the deployment units form a cycle, but prints only the name of a node in the graph that was in the cycle. If you've added a cycle to either of these graphs, the error and node can tell you about the what that is wrong. Going a bit further and printing an example cycle including the troublesome node can help with the how you got there.With this change, the first approach at #10231 (which introduced a cycle
in, but all in one deployment unit: sled-agent -> propolis-server ->
sled-agent) errors with a more clear message about propolis-server and sled-agent being the problems:
(this is the PR that @jgallagher alluded to me putting together, in #10232 )