[3/n] [ls-apis] track sled-agent-rack-setup as a separate subcomponent by sunshowers · Pull Request #10459 · oxidecomputer/omicron

sunshowers · 2026-05-18T23:29:55Z

RSS has very different semantics from the rest of Sled Agent, and the recent split to put it in another crate (#10340) gives us a change to handle this divergence in a principled manner.

Introduce two new notions to ls-apis:

subcomponents: components that are part of a deployment unit but should be treated separately. Currently, RSS is the only subcomponent.
lifecycle: when a server component's code runs -- "steady-state" by default, but "rack-init" for RSS

This separation also gives us a chance to do a lot of cleanup within the API manifest:

the not-deployed evaluation for RSS goes away, since that knowledge can be derived from the rack-init lifecycle
there's no need for RSS to declare IDU-only edges, since it doesn't participate in the upgrade DAG

Depends on:

[2/n] [ls-apis] return walked dependencies instead of using a callback #10458

Created using spr 1.3.6-beta.1

sunshowers · 2026-05-19T00:18:09Z

+# upgrade DAG, so that sled-agent-rack-setup doesn't impose any ordering
+# constraints on the upgrade DAG.
+subcomponents = [
+    { name = "sled-agent-rack-setup", inside = "omicron-sled-agent", lifecycle = "rack-init" },


Looking for feedback on the inside thing. I tried autodiscovering the inside by looking at the dependency graph but honestly that was a complete mess that was hard to understand both in the code and in this manifest. I feel like having an explicit inside makes both layers easier to understand.

I really like this tbh. Having it explicitly set helped me understand what was going on. Coming to this PR with very little knowledge of how ls-apis worked, I can say that not having it would have made the functionality much harder to grasp.

Yeah I agree, making it explicit is much more approachable. ls-apis is necessarily quite complex and it's good to try and keep the complexity budget down.

Created using spr 1.3.6-beta.1

karencfv

I didn't have time to finish reviewing, but wanted to give you my initial questions. A lot of it is just me wanting to understand the ls-apis tool 😄

karencfv · 2026-05-27T04:06:41Z

+==== Subcomponents
+
+Sometimes one binary contains a body of code whose API dependencies are best tracked separately from the rest of it.  The motivating case is the Rack Setup Service (RSS): it lives in the `sled-agent-rack-setup` library crate, which is linked into the `omicron-sled-agent` binary, but it runs only once (at rack initialization) and has different semantics from the rest of the Sled Agent.
+
+A _subcomponent_ is such a library, tracked as an API consumer in its own right:


When I think of the concept of a "subcomponent", what comes to mind is a piece of a unit, sort of what packages does. I don't really think of a piece of a unit that should be treated differently. This terminology caused a bit of confusion for me and made it harder to understand what was going on here. At first glance it gave me the impression you should list all API dependencies or something like that.

Perhaps we could use another term? Something like tracked_subcomponentswhere it's clear that they're something that should be tracked separately? Or perhaps auxiliary_subcomponents?

Hmm, I see what you mean. What about "library component" to indicate that this is a library that should be reasoned about separately? Or is that too general for you?

hm, it still feels a little vague? What about independent_component?

karencfv · 2026-05-27T06:01:51Z

+]
+```
+
+`inside` names the top-level package (in the same deployment unit) that links the subcomponent.  When ls-apis walks that package's dependencies, the subcomponent's own crate is skipped, so any API dependency reachable only through the subcomponent is attributed to the subcomponent instead of to the containing binary.  (A dependency the binary also reaches by another path is attributed to both.)  The subcomponent is then walked separately, as its own consumer.


Optional nit. What about parent or parent-package instead of inside?

Well it's not necessarily a direct parent, so it would have to be called ancestor. I guess that might work, though I felt that ancestor is a little overloaded.

Fair, I can't think of a better name either lol. Whichever one is good

karencfv · 2026-05-27T06:06:36Z

+# upgrade DAG, so that sled-agent-rack-setup doesn't impose any ordering
+# constraints on the upgrade DAG.
+subcomponents = [
+    { name = "sled-agent-rack-setup", inside = "omicron-sled-agent", lifecycle = "rack-init" },


I really like this tbh. Having it explicitly set helped me understand what was going on. Coming to this PR with very little knowledge of how ls-apis worked, I can say that not having it would have made the functionality much harder to grasp.

karencfv · 2026-05-27T06:20:15Z

+            for pkg in &raw_unit.packages {
+                component_names.push(pkg.clone());
+                register_server_component(
+                    &mut server_components,
+                    ServerComponent {
+                        name: pkg.clone(),
+                        deployment_unit: raw_unit.name.clone(),
+                        lifecycle: Lifecycle::SteadyState,
+                        kind: ServerComponentKind::TopLevel,
+                    },
+                )?;
+            }


Is it possible in the future to have a package in a Lifecycle::RackInit lifecycle? If so, what do you envision happening?

Certainly possible in principle, yeah, though I'm a little doubtful in practice. (I could be wrong, though!)

At that point we'd update the tooling to let you specify that.

#10458) We're going to return more data from this function (the set of omitted nodes that were seen) in an upcoming PR. I tried out a few different approaches: * two callbacks * a trait * a hybrid approach where we keep the current callback and return the additional data as a value Based on the prototyping I feel like this ends up being the cleanest approach. There are no functional changes in this PR. Best reviewed in conjunction with its dependent PR: * #10459

karencfv · 2026-05-28T08:06:08Z

+==== Subcomponents
+
+Sometimes one binary contains a body of code whose API dependencies are best tracked separately from the rest of it.  The motivating case is the Rack Setup Service (RSS): it lives in the `sled-agent-rack-setup` library crate, which is linked into the `omicron-sled-agent` binary, but it runs only once (at rack initialization) and has different semantics from the rest of the Sled Agent.
+
+A _subcomponent_ is such a library, tracked as an API consumer in its own right:


hm, it still feels a little vague? What about independent_component?

karencfv · 2026-05-28T08:12:33Z

+]
+```
+
+`inside` names the top-level package (in the same deployment unit) that links the subcomponent.  When ls-apis walks that package's dependencies, the subcomponent's own crate is skipped, so any API dependency reachable only through the subcomponent is attributed to the subcomponent instead of to the containing binary.  (A dependency the binary also reaches by another path is attributed to both.)  The subcomponent is then walked separately, as its own consumer.


Fair, I can't think of a better name either lol. Whichever one is good

karencfv · 2026-05-28T08:43:03Z

+                                "subcomponent {:?} (in deployment unit \
+                                 {:?}) is not a package in workspace {:?} \
+                                 (the workspace of its `inside` package \
+                                 {:?}); check for a typo in `name`, or that \
+                                 the package actually exists",
+                                component.name(),
+                                component.deployment_unit(),
+                                workspace.name(),
+                                inside,


I was a little confused by this message. How about something like

anyhow!( "subcomponent {:?} not found in its expected workspace\n \ expected workspace: {:?} (the workspace of its `inside` package {:?})\n \ deployment unit: {:?}\n \ hint: check `name` for typos, or that the package exists in that workspace", component.name(), workspace.name(), inside, component.deployment_unit(), )

karencfv · 2026-05-28T08:49:45Z

+                            bail!(
+                                "subcomponent {:?} (in deployment unit \
+                                 {:?}) is declared with `inside = {:?}`, \
+                                 but {:?} has no normal or build Cargo \
+                                 dependency on it; remove the stale \
+                                 `subcomponents` entry, correct its \
+                                 `inside` field, or restore the dependency \
+                                 if it was removed or made dev-only",
+                                sub.name(),
+                                sub.deployment_unit(),
+                                component_name,
+                                component_name,
+                            );
+                        }


How about

bail!( "subcomponent {:?} is declared `inside` a package that doesn't depend on it\n \ deployment unit: {:?}\n \ `inside` package: {:?} (no normal or build Cargo dependency on this subcomponent)\n \ hint: remove the stale `subcomponents` entry, fix its `inside` field, \ or restore the dependency if it was removed or made dev-only", sub.name(), sub.deployment_unit(), component_name, )

karencfv · 2026-05-28T08:57:04Z

+                        if !self.component_in_upgrade_dag(server_pkg) {
+                            continue;
+                        }


Should this not be the first condition? If the component is not the upgrade DAG at all why would we check how the API is versioned first?

karencfv · 2026-05-28T08:57:16Z

+                    if !self.component_in_upgrade_dag(server_component) {
+                        continue;
+                    }


[spr] initial version

34caf60

Created using spr 1.3.6-beta.1

sunshowers commented May 19, 2026

View reviewed changes

sunshowers requested a review from davepacheco May 19, 2026 00:18

sunshowers mentioned this pull request May 19, 2026

[2/n] [ls-apis] return walked dependencies instead of using a callback #10458

Merged

karencfv self-requested a review May 26, 2026 20:35

rebase on main

91a304a

Created using spr 1.3.6-beta.1

karencfv reviewed May 27, 2026

View reviewed changes

karencfv reviewed May 28, 2026

View reviewed changes

Conversation

sunshowers commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karencfv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sunshowers commented May 18, 2026 •

edited

Loading