[2/n] [reconfigurator] test that Reconfigurator updates are in server-side topological order by sunshowers · Pull Request #10099 · oxidecomputer/omicron

sunshowers · 2026-03-19T05:51:24Z

Add a test which makes sure that when Reconfigurator runs an update, it is in topological order as defined by omicron-ls-apis (only considering server-side versioned APIs): for a particular producer/consumer pair, all instances of producer must be updated before the first instance of consumer.

The point of coordination between the two is a new file, dev-tools/ls-apis/tests/output/deployment-unit-dag.toml.

The ls-apis tool generates this file as a condensed form of api-manifest.toml with just the edges (and there's an expectorate test for it).
The test in nexus-reconfigurator-planning reads this file, simulates an update and builds a trace, then asserts that the update happened in topo order (and various other related properties).

This exercise found three violations:

Host OS (including omicron-sled-agent) is updated before NTP, but omicron-sled-agent is a consumer of NTP. Addressed this by turning NTP into a client-side-versioned API (as discussed in an update watercooler).
omicron-sled-agent depends on cockroach-admin-client during RSS. At RSS time, the system is known to be at a single, consistent version, and we mark this fact through a new rss-only dependency filter rule. (rss-only is similar to non-dag, except it doesn't check that versioned_how is client; there is a larger discussion to be had about how we may wish to define client-side versioning as applying to edges rather than nodes, but that is outside the scope of this work.)
omicron-sled-agent depends on dns-service-client during RSS. We treat this the same as 2.

Depends on:

Created using spr 1.3.6-beta.1

…n to inventory (#10112) Prerequisite for #10099.

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

Previously, the DNS server depended on its own client (dns-service-client) for two things: * `TransientServer` for a transient in-memory server. * The `dnsadm` binary. This isn't a real dependency in the sense that the DNS server doesn't use the client to call other instances of itself in normal use, so there was an exclusion for this in `api-manifest.toml`. This exclusion was causing issues for #10099. Move both of these out into their own crates, and drop the exclusion from `api-manifest.toml`. There is a small, benign change to the output of `ls-apis apis` (captured by `dev-tools/ls-apis/tests/api_dependencies.out`). With `--show-deps`: ``` DNS Server (client: dns-service-client) [...] consumed by: omicron-sled-agent (omicron/sled-agent) via 2 paths via path 1: path+file:///home/rain/dev/oxide/omicron/sled-agent#omicron-sled-agent@0.1.0 via path 2: path+file:///home/rain/dev/oxide/omicron/dns-server/transient#transient-dns-server@0.1.0 via path 2: path+file:///home/rain/dev/oxide/omicron/sled-agent#omicron-sled-agent@0.1.0 ```

Created using spr 1.3.6-beta.1

davepacheco · 2026-04-01T23:14:48Z

The NTP issue is covered by #8769, which I believe will be resolved with this change.

davepacheco

Nice! I've reviewed everything except the new test itself here.

davepacheco · 2026-04-01T23:17:33Z

dev-tools/dropshot-apis/src/main.rs

+        // The NTP Admin API is client-side-versioned and currently frozen. We
+        // allow trivial changes to go through. If we did not, we would need to
+        // unfreeze the API and bump the version number for trivial changes.
+        .allow_trivial_changes_for_latest(),


Does this mean that:

we allow the user to update the OpenAPI document for a blessed version as long as it's trivial, or

we allow the generated OpenAPI document to diverge from the corresponding blessed one as long as the changes are trivial?

The first one seems better for the reasons we've previously discussed not wanting to accumulate delta (it makes it harder for future people to see what their changes were) but I guess either is okay right now.

davepacheco · 2026-04-01T23:19:40Z

ntp-admin/api/src/lib.rs

+    // Do not create new versions of this client-side versioned API.
+    // https://github.com/oxidecomputer/omicron/issues/9290


I do feel like we discussed this comment in the context of some other API and settled on the text you've got here. Is that right?

I don't personally love it because it's not quite true that you can't create new versions. If we need a new endpoint for example we do have options like adding a new version and not using it until the next release or tolerate the endpoint failing until the next release, etc.

davepacheco · 2026-04-01T23:24:38Z

nexus/inventory/src/builder.rs

    }

-    /// Record metrics from a CockroachDB node
+    /// Record metrics from a CockroachDB node.


This is not important but this change seems superfluous. It makes it differ from the others here in terms of style. (At some point, I picked up the pattern of not using punctuation on fragments used as the one-line summary. I don't know what we do more often in Omicron.)

I wouldn't even mention it but it's the only change to this file.

davepacheco · 2026-04-01T23:26:04Z

dev-tools/ls-apis-shared/src/lib.rs

+//! Types shared between `omicron-ls-apis` and consumers of its output
+//! (e.g. the reconfigurator planner tests).


What about giving this a more meaningful name, like omicron-deployment-graph?

davepacheco · 2026-04-01T23:27:40Z

dev-tools/ls-apis/src/bin/ls-apis.rs

+        .context("serializing DAG edges as TOML")?;
+    print!(
+        "# BEGIN @generated server-side deployment unit DAG edges.\n\
+         # To regenerate, run `EXPECTORATE=overwrite cargo nextest run -p omicron-ls-apis`.\n\


What do you think of:

Suggested change

# To regenerate, run `EXPECTORATE=overwrite cargo nextest run -p omicron-ls-apis`.\n\

# To regenerate, run `EXPECTORATE=overwrite cargo nextest run -p omicron-ls-apis`.\n\

# YOU SHOULD STILL REVIEW CHANGES TO THIS FILE FOR CORRECTNESS.

davepacheco · 2026-04-01T23:29:20Z

dev-tools/ls-apis/api-manifest.toml

+Sled Agent only uses the DNS service client during RSS, which we don't care
+about for the purpose of upgrade.
+"""
+


Should the comment about Non-DAG dependencies have gotten preserved here?

davepacheco · 2026-04-01T23:30:13Z

dev-tools/ls-apis/api-manifest.toml

+[[dependency_filter_rules]]
+ancestor = "omicron-sled-agent"
+client = "cockroach-admin-client"
+evaluation = "rss-only"
+note = """
+Sled Agent only uses the Cockroach Admin client during RSS, which we don't care
+about for the purpose of upgrade.
+"""
+
+[[dependency_filter_rules]]
+ancestor = "omicron-sled-agent"
+client = "dns-service-client"
+evaluation = "rss-only"
+note = """
+Sled Agent only uses the DNS service client during RSS, which we don't care
+about for the purpose of upgrade.
+"""


We also discussed putting RSS into its own package so that we could eliminate the risk that some other part of sled agent grows these dependencies. Do you have any idea how much work that is? Seems at least worth filing an issue.

davepacheco · 2026-04-01T23:32:34Z

dev-tools/ls-apis/api-manifest.toml

 # The host OS includes Sled Agent, Propolis, and all the components that get
 # bundled into the switch zone.
 [[deployment_units]]
+id = "host_os"


I think adding a second field here makes it pretty likely people will copy/paste and forget to change either the id or label.

Out of curiosity, why not use the label for this (or use this as the label)?

If we want to preserve both as distinct things:

Should we make deployment_units a map instead of an array? (I wonder if we could make it an iddqd map with id as the key?)

Should we add a check that the ids and labels are both unique?

davepacheco · 2026-04-01T23:36:19Z

dev-tools/ls-apis/src/api_metadata.rs

+    /// the update DAG. Unlike `NonDag`,
+    /// this does not require that the target API uses client-side versioning.


spurious line break here

davepacheco · 2026-04-01T23:39:02Z

nexus/reconfigurator/planning/tests/integration_tests/planner.rs

 }
+
+/// Maps a `ZoneKind` to its omicron-ls-apis deployment unit ID.
+fn zone_kind_to_deployment_unit(kind: ZoneKind) -> DeploymentUnitId {


Hmm. This feels brittle but I'm not sure what to do about it.

I guess we could make an enum of deployment unit ids in ls-apis-shared and expect that the API manifest uses one of those?

[spr] initial version

5245858

Created using spr 1.3.6-beta.1

sunshowers mentioned this pull request Mar 20, 2026

[reconfigurator] example system now publishes ntp and crdb information to inventory #10112

Merged

sunshowers added a commit that referenced this pull request Mar 20, 2026

[reconfigurator] example system now publishes ntp and crdb informatio…

8d7f973

…n to inventory (#10112) Prerequisite for #10099.

sunshowers mentioned this pull request Mar 30, 2026

[1/n] make dns-server -> dns-service-client dependency dev-only #10193

Merged

sunshowers added 2 commits March 30, 2026 17:54

[spr] changes introduced through rebase

a69d102

Created using spr 1.3.6-beta.1 [skip ci]

rebase on 10193, fixups

64f2717

Created using spr 1.3.6-beta.1

sunshowers changed the base branch from main to sunshowers/spr/main.reconfigurator-test-that-reconfigurator-updates-are-in-server-side-topological-order March 31, 2026 00:54

sunshowers changed the title ~~[reconfigurator] test that Reconfigurator updates are in server-side topological order~~ [2/n] [reconfigurator] test that Reconfigurator updates are in server-side topological order Mar 31, 2026

sunshowers changed the base branch from sunshowers/spr/main.reconfigurator-test-that-reconfigurator-updates-are-in-server-side-topological-order to main March 31, 2026 17:14

rebase on main

36a7124

Created using spr 1.3.6-beta.1

sunshowers marked this pull request as ready for review March 31, 2026 19:44

sunshowers requested review from davepacheco and jgallagher March 31, 2026 19:44

davepacheco reviewed Apr 1, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2/n] [reconfigurator] test that Reconfigurator updates are in server-side topological order#10099

[2/n] [reconfigurator] test that Reconfigurator updates are in server-side topological order#10099
sunshowers wants to merge 4 commits intomainfrom
sunshowers/spr/reconfigurator-test-that-reconfigurator-updates-are-in-server-side-topological-order

sunshowers commented Mar 19, 2026 •

edited

Loading

Uh oh!

davepacheco commented Apr 1, 2026

Uh oh!

davepacheco left a comment

Uh oh!

davepacheco Apr 1, 2026

Uh oh!

davepacheco Apr 1, 2026

Uh oh!

davepacheco Apr 1, 2026

Uh oh!

davepacheco Apr 1, 2026

Uh oh!

davepacheco Apr 1, 2026

Uh oh!

davepacheco Apr 1, 2026

Uh oh!

davepacheco Apr 1, 2026

Uh oh!

davepacheco Apr 1, 2026

Uh oh!

davepacheco Apr 1, 2026

Uh oh!

davepacheco Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// Do not create new versions of this client-side versioned API.
		// https://github.com/oxidecomputer/omicron/issues/9290

		//! Types shared between `omicron-ls-apis` and consumers of its output
		//! (e.g. the reconfigurator planner tests).

	# To regenerate, run `EXPECTORATE=overwrite cargo nextest run -p omicron-ls-apis`.\n\
	# To regenerate, run `EXPECTORATE=overwrite cargo nextest run -p omicron-ls-apis`.\n\
	# YOU SHOULD STILL REVIEW CHANGES TO THIS FILE FOR CORRECTNESS.

		/// the update DAG. Unlike `NonDag`,
		/// this does not require that the target API uses client-side versioning.

Conversation

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

Uh oh!

Uh oh!

davepacheco commented Apr 1, 2026

Uh oh!

davepacheco 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!

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 Mar 19, 2026 •

edited

Loading