`ls-apis` CI test didn't fail for new cyclic dependency

[jordanhendricks]

oxidecomputer/propolis#1091 is adding a cyclic dependency between propolis and sled-agent (propolis now hits the RoT sled-agent attestation endpoints #9739). I have been testing this change by bumping the propolis dep in omicron (example) and I realized I should've seen a CI failure, presumably from here:

omicron/.github/buildomat/build-and-test.sh

Lines 67 to 75 in ad395b0

	banner ls-apis
	(
	source ./tools/include/force-git-over-https.sh;
	ptime -m cargo xtask ls-apis apis &&
	ptime -m cargo xtask ls-apis deployment-units &&
	ptime -m cargo xtask ls-apis servers &&
	ptime -m cargo xtask ls-apis check && \
	ptime -m cargo xtask ls-apis adoc > /dev/null
	)

for introducing this cyclic dependency, but I have never seen that happen.

[davepacheco]

I may well be missing something but it looks to me like the commit you referenced there (e59bf3c) does not yet reflect a circular dependency.

• Omicron commit e59bf3 references Propolis commit 50c24ff8af2c5dff68bc7f3a6b635f55a4ff0d0f
• Propolis commit 50c24ff8af2c5dff68bc7f3a6b635f55a4ff0d0f has sled-agent-client in the top-level Cargo.toml, but that looks like a workspace Cargo.toml. I don't see sled-agent-client or sled_agent_client in any other place in the Propolis repo.

ls-apis looks at Rust dependencies to infer API dependencies. From what it can see, the propolis-server package doesn't depend on sled-agent-client, so Propolis cannot be using the sled agent API. Is there some other way that it's calling out to sled agent?

[davepacheco]

I was missing something -- it's not a direct dependency, but rather goes through a package that's not in the Propolis repo.

With this diff (which is a workaround, not sure yet if it's a great fix) against @jordanhendricks's branch:

diff --git a/dev-tools/ls-apis/src/workspaces.rs b/dev-tools/ls-apis/src/workspaces.rs
index f3cb89b19..0af6a184e 100644
--- a/dev-tools/ls-apis/src/workspaces.rs
+++ b/dev-tools/ls-apis/src/workspaces.rs
@@ -67,7 +67,7 @@ impl Workspaces {
             ("dendrite", "dpd-client", None),
             (
                 "propolis",
-                "propolis-client",
+                "propolis-mock-server",
                 // The artifacts shipped from the Propolis repo (particularly,
                 // `propolis-server`) are built with the `omicron-build`
                 // feature, which is not enabled by default.  Enable this

The tool does the right thing, which is to report:

$ cargo xtask ls-apis check
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.57s
     Running `target/debug/xtask ls-apis check`
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.85s
     Running `target/debug/ls-apis check`
loading metadata for workspace omicron from current workspace
loading metadata for workspace lldp from /home/dap/.cargo/git/checkouts/lldp-d47de417041f191b/d22509d/Cargo.toml
loading metadata for workspace maghemite from /home/dap/.cargo/git/checkouts/maghemite-c0236f0fd3d582b6/b603c9f/Cargo.toml
loading metadata for workspace propolis from /home/dap/.cargo/git/checkouts/propolis-d68c8bd1bc59c9bd/50c24ff/Cargo.toml
loading metadata for workspace crucible from /home/dap/.cargo/git/checkouts/crucible-0a48bd218bc2bbbc/7103cd3/Cargo.toml
loading metadata for workspace dendrite from /home/dap/.cargo/git/checkouts/dendrite-ae9f1715c17fc765/f20f786/Cargo.toml
loading metadata for workspace dendrite from /home/dap/.cargo/git/checkouts/dendrite-ae9f1715c17fc765/3799229/Cargo.toml
note: ignoring Cargo dependency from crucible-pantry -> ... -> crucible-control-client
note: ignoring Cargo dependency from omicron-sled-agent -> dns-server
Error: The following API dependendency exists between two components in the same deployment unit, but is not present in `intra_deployment_unit_only_edges`: server "propolis-server" client "propolis-client"
If this client only ever uses this API with a server in the same *instance* of the same deployment unit, then add it to `intra_deployment_unit_only_edges`. Otherwise, this relationship is incompatible with automated upgrade.
The following API dependendency exists between two components in the same deployment unit, but is not present in `intra_deployment_unit_only_edges`: server "propolis-server" client "sled-agent-client"
If this client only ever uses this API with a server in the same *instance* of the same deployment unit, then add it to `intra_deployment_unit_only_edges`. Otherwise, this relationship is incompatible with automated upgrade.

Actually there's an extra message there -- more on that below.

The tool also shows the expected dependency from propolis-server on sled-agent-client:

$ cargo xtask ls-apis servers --show-deps
...
propolis-server (propolis/bin/propolis-server)
    exposes: Crucible Control (for testing only) (client = crucible-control-client)
    exposes: Propolis (client = propolis-client)
    consumes: nexus-client
        via: path+file:///home/dap/.cargo/git/checkouts/propolis-d68c8bd1bc59c9bd/50c24ff/bin/propolis-server#0.1.0
    consumes: propolis-client
        via: git+https://github.com/oxidecomputer/omicron?rev=becbbb616f5f18b59cc42e511c148734c2ba3831#sled-agent-client@0.1.0
        via: git+https://github.com/oxidecomputer/dice-util?rev=1d3084b514389847e8e0f5d966d2be4f18d02d32#dice-verifier@0.3.0-pre0
        via: git+https://github.com/oxidecomputer/vm-attest?rev=2cdd17580a4fc6c871d24797016af8dbaac9421d#0.1.0
        via: path+file:///home/dap/.cargo/git/checkouts/propolis-d68c8bd1bc59c9bd/50c24ff/bin/propolis-server#0.1.0
    consumes: sled-agent-client
        via: git+https://github.com/oxidecomputer/dice-util?rev=1d3084b514389847e8e0f5d966d2be4f18d02d32#dice-verifier@0.3.0-pre0
        via: git+https://github.com/oxidecomputer/vm-attest?rev=2cdd17580a4fc6c871d24797016af8dbaac9421d#0.1.0
        via: path+file:///home/dap/.cargo/git/checkouts/propolis-d68c8bd1bc59c9bd/50c24ff/bin/propolis-server#0.1.0
...

We see that it goes through vm-attest and dice-util, which I gather is expected.

There's also a path from propolis-server to propolis-client, which I initially thought might relate to live migration, but no -- that was introduced here as well because sled-agent-client depends on propolis-client, apparently to re-export some types. I imagine we will want to separate that into a propolis-api-types crate and then this won't report the problem of propolis-server depending on propolis-client.

---

So, why's this happening? First, some background: ls-apis needs a copy of the propolis repo to understand dependencies. More than that: it wants a copy that it can run cargo commands on (like cargo metadata). How does it find one? Easy: cargo necessarily has one if it's ever initialized this Omicron clone and it'll definitely be the right one. So ls-apis wants to use that. But how does it find that? It uses cargo metadata in the Omicron clone to look up a package that it knows is in the Propolis repo: propolis-client. We do this for several repos.

The problem here is that there are multiple references to propolis-client within Omicron right now:

• the one we care about here (the one being used by sled-agent). This one has the circular dependency that we expect ls-apis to catch.
• the one coming from falcon_runner inside omicron-test-utils. This one is tied to a commit that predates the circular dependency.

For no good reason, ls-apis winds up picking the second of these (it assumes if it finds more than one that they'll be the same):

omicron/dev-tools/ls-apis/src/workspaces.rs

Lines 225 to 228 in ad395b0

	// `Workspace` doesn't let us look up a non-workspace package by name
	// because there may be many of them. So list all the pkgids and take any
	// one of them -- any of them should work for our purpoes.
	let pkgid = workspace.pkgids(pkgname).next().ok_or_else(|| {

The diff above causes ls-apis to use a different package in the Propolis repo, propolis-mock-server, instead. This works because that one happens to be unique, though it only works at all because Omicron happens to depend on that package, too. That's why I don't think it's a great fix, though I'm not yet sure if there's a better option. It'd be nice if there were some way for it to identify the right one, but it's not obvious to me yet how to do that. I believe this is all happening before we process the filter rules so we can't easily use those.

[davepacheco]

I confirmed that if I check out Jordan's branch and apply c1e92fa (from #10217), I get the expected error.