Skip to content

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

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

Merged
sunshowers merged 13 commits into from
Apr 30, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5245858
[spr] initial version
sunshowers Mar 19, 2026
a69d102
[spr] changes introduced through rebase
sunshowers Mar 31, 2026
64f2717
rebase on 10193, fixups
sunshowers Mar 31, 2026
36a7124
rebase on main
sunshowers Mar 31, 2026
471b88c
rebase on main, review feedback
sunshowers Apr 14, 2026
9a65610
shorten to ClickHouse
sunshowers Apr 15, 2026
27599f8
rustfmt
sunshowers Apr 15, 2026
6c3f2bd
[spr] changes introduced through rebase
sunshowers Apr 28, 2026
7e538bb
Update, rebase on 10338
sunshowers Apr 28, 2026
47c11d3
[spr] changes introduced through rebase
sunshowers Apr 28, 2026
c1ec51d
rebase on main, small tweak
sunshowers Apr 28, 2026
6ff3c27
hakari
sunshowers Apr 28, 2026
83253f6
rebase on main + review feedback
sunshowers Apr 30, 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
13 changes: 13 additions & 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.

3 changes: 3 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ members = [
"dev-tools/omdb",
"dev-tools/omicron-dev",
"dev-tools/omicron-dev-lib",
"dev-tools/omicron-deployment-graph",
"dev-tools/oxlog",
"dev-tools/pins",
"dev-tools/reconfigurator-cli",
Expand Down Expand Up @@ -227,6 +228,7 @@ default-members = [
"dev-tools/omdb",
"dev-tools/omicron-dev",
"dev-tools/omicron-dev-lib",
"dev-tools/omicron-deployment-graph",
"dev-tools/oxlog",
"dev-tools/pins",
"dev-tools/reconfigurator-cli",
Expand Down Expand Up @@ -592,6 +594,7 @@ linear-map = "1.2.0"
live-tests-macros = { path = "live-tests/macros" }
lldpd_client = { git = "https://github.com/oxidecomputer/lldp", package = "lldpd-client" }
lldp_protocol = { git = "https://github.com/oxidecomputer/lldp", package = "protocol" }
omicron-deployment-graph = { path = "dev-tools/omicron-deployment-graph" }
macaddr = { version = "1.0.1", features = ["serde_std"] }
maplit = "1.0.2"
newtype_derive = "0.1.6"
Expand Down
6 changes: 5 additions & 1 deletion dev-tools/dropshot-apis/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,11 @@ fn all_apis() -> anyhow::Result<ManagedApis> {
},
api_description: ntp_admin_api_mod::stub_api_description,
ident: "ntp-admin",
}),
})
// 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(),
Copy link
Copy Markdown
Collaborator

@davepacheco davepacheco Apr 1, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@sunshowers sunshowers Apr 14, 2026

Choose a reason for hiding this comment

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

It is the latter, matching the other client-side API versions. I would really like to preserve the property that blessed versions are immutable if at all possible.

One possibility here for future work is introducing something like a minor version (1.1, 1.2, etc), where the minor versions are always wire compatible with the corresponding major version (1.0, etc). Then, we clients can be instructed to send 1.0 in their api-version header rather than 1.1, etc. I haven't fully thought this through so it's possible there are issues with this approach.

ManagedApi::from(ManagedApiConfig {
title: "Oxide Oximeter API",
versions: Versions::new_versioned(
Expand Down
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 @@ -15,6 +15,7 @@ clap.workspace = true
iddqd.workspace = true
indent_write.workspace = true
itertools.workspace = true
omicron-deployment-graph.workspace = true
newtype_derive.workspace = true
parse-display.workspace = true
petgraph.workspace = true
Expand Down
9 changes: 6 additions & 3 deletions dev-tools/ls-apis/README.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,9 @@ Filter rules always represent a determination that a human has made about one or
|`non-dag`
|Any matching dependency has been flagged as "will not be part of the DAG used for online upgrade". This is primarily to help us keep track of the specific dependencies that we've looked at and made this determination for. These are currently ignored by default.

|`rss-only`
|Any matching dependency is only used during RSS, not during normal operation or upgrade. Like `non-dag`, these are excluded from the default view. Unlike `non-dag`, this does not require the API to use client-side versioning.

|`dag`
|Any matching dependency has been flagged as "we want this to be part of the DAG used for online upgrade".

Expand All @@ -218,17 +221,17 @@ In summary:
* All dependencies start as `unknown`.
* All the known false positives have been flagged as `bogus`.
* All the known dependencies from non-deployed programs inside deployed packages have been flagged as `not-deployed`.
* What remains is to evaluate the rest of the edges and determine if they're going to be `dag` or `non-dag`.
* What remains is to evaluate the rest of the edges and determine if they're going to be `dag`, `non-dag`, or `rss-only`.

It is a runtime error for two filter rules to match any dependency chain. This makes the evaluation unambiguous. i.e., you can't have one rule match a dependency chain and say it's `bogus` while another says it's `dag`.

==== Applying different filters at runtime

By default, this command shows dependencies that might be in the final graph. This includes those labeled `dag` and `unknown`. It excludes `bogus`, `non-dag`, and `not-deployed` dependencies.
By default, this command shows dependencies that might be in the final graph. This includes those labeled `dag` and `unknown`. It excludes `bogus`, `non-dag`, `rss-only`, and `not-deployed` dependencies.

You can select different subsets using the `--filter` option, which accepts:

* `include-non-dag`: show non-`bogus`, non-`not-deployed` dependencies (i.e., all dependencies that do exist in the deployed system).
* `include-non-dag`: show non-`bogus`, non-`not-deployed` dependencies (i.e., all dependencies that do exist in the deployed system). This includes `non-dag` and `rss-only` dependencies.
* `non-bogus`: show everything _except_ bogus dependencies
* `bogus`: show only the bogus dependencies (useful for seeing all the false positives)
* `all`: show everything, even bogus dependencies
62 changes: 48 additions & 14 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 @@ -67,7 +67,7 @@ ignored_non_clients = [
# The host OS includes Sled Agent, Propolis, and all the components that get
# bundled into the switch zone.
[[deployment_units]]
label = "Host OS"
name = "Host OS"
packages = [
"omicron-sled-agent",
"propolis-server",
Expand All @@ -83,44 +83,49 @@ packages = [

# Installinator gets packaged into its own host OS image.
[[deployment_units]]
label = "Installinator"
name = "Installinator"
packages = [ "installinator" ]

# The rest of these get bundled by standard control plane zone images.

[[deployment_units]]
label = "Crucible"
name = "Crucible"
packages = [ "crucible-agent", "crucible-downstairs" ]

[[deployment_units]]
label = "Crucible Pantry"
name = "Crucible Pantry"
packages = [ "crucible-pantry" ]

[[deployment_units]]
label = "Cockroach"
name = "Cockroach"
packages = [ "omicron-cockroach-admin" ]

# These are really three distinct deployment units, but they behave the same for
# our purposes, and the tooling doesn't support multiple deployment units
# that each expose a particular service.
# This is really three distinct deployment units:
#
# * single-node ClickHouse
# * multi-node ClickHouse Server
# * multi-node ClickHouse Keeper
#
# but they behave the same for our purposes, and the tooling doesn't support
# multiple deployment units that each expose a particular service.
[[deployment_units]]
label = "Clickhouse (single-node) / Clickhouse Server (multi-node) / Clickhouse Keeper (multi-node)"
name = "ClickHouse"
packages = [ "omicron-clickhouse-admin" ]

[[deployment_units]]
label = "DNS Server"
name = "DNS Server"
packages = [ "dns-server" ]

[[deployment_units]]
label = "Nexus"
name = "Nexus"
packages = [ "omicron-nexus" ]

[[deployment_units]]
label = "NTP"
name = "NTP"
packages = [ "omicron-ntp-admin" ]

[[deployment_units]]
label = "Oximeter"
name = "Oximeter"
packages = [ "oximeter-collector" ]


Expand Down Expand Up @@ -239,7 +244,11 @@ configuration and monitoring that requires the `cockroach` CLI.
client_package_name = "ntp-admin-client"
label = "NTP Admin"
server_package_name = "ntp-admin-api"
versioned_how = "server"
versioned_how = "client"
versioned_how_reason = """
Sled Agent (within the Host OS deployment unit) calls into NTP, and is updated \
before NTP. This requires that the NTP Admin API be client-side versioned.
"""
notes = """
This is the server running inside NTP zones that performs \
monitoring on 'chrony'.
Expand Down Expand Up @@ -475,9 +484,34 @@ instance. It's also used by `crutest` for testing.
# edges should be in that graph.
#

#
# RSS-only dependencies. These are real dependencies that only exist during
# RSS, not during normal operation or upgrade.
#
# At RSS time, the system is known to be at a single, consistent version.
#
[[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.
"""
Comment thread
sunshowers marked this conversation as resolved.

Comment thread
sunshowers marked this conversation as resolved.
#
# Non-DAG dependencies. See above for details.
#

[[dependency_filter_rules]]
ancestor = "oximeter-producer"
client = "nexus-client"
Expand Down
46 changes: 33 additions & 13 deletions 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 @@ -5,7 +5,6 @@
//! Developer-maintained API metadata

use crate::ClientPackageName;
use crate::DeploymentUnitName;
use crate::ServerComponentName;
use crate::ServerPackageName;
use crate::cargo::DepPath;
Expand All @@ -14,6 +13,7 @@ use anyhow::{Result, bail};
use iddqd::IdOrdItem;
use iddqd::IdOrdMap;
use iddqd::id_upcast;
use omicron_deployment_graph::DeploymentUnitName;
use serde::Deserialize;
use std::borrow::Borrow;
use std::collections::BTreeMap;
Expand All @@ -27,7 +27,7 @@ use std::collections::BTreeSet;
#[serde(try_from = "RawApiMetadata")]
pub struct AllApiMetadata {
apis: BTreeMap<ClientPackageName, ApiMetadata>,
deployment_units: BTreeMap<DeploymentUnitName, DeploymentUnitInfo>,
deployment_units: IdOrdMap<DeploymentUnitInfo>,
dependency_rules: BTreeMap<ClientPackageName, Vec<DependencyFilterRule>>,
ignored_non_clients: BTreeSet<ClientPackageName>,
intra_deployment_unit_only_edges: Vec<IntraDeploymentUnitOnlyEdge>,
Expand All @@ -42,10 +42,18 @@ impl AllApiMetadata {
/// Iterate over the deployment units defined in the metadata
pub fn deployment_units(
&self,
) -> impl Iterator<Item = (&DeploymentUnitName, &DeploymentUnitInfo)> {
) -> impl Iterator<Item = &DeploymentUnitInfo> {
self.deployment_units.iter()
}

/// Look up a deployment unit's info by its name
pub fn deployment_unit_info(
&self,
name: &DeploymentUnitName,
) -> Option<&DeploymentUnitInfo> {
self.deployment_units.get(name)
}

/// Iterate over the package names for all the APIs' clients
pub fn client_pkgnames(&self) -> impl Iterator<Item = &ClientPackageName> {
self.apis.keys()
Expand All @@ -55,7 +63,7 @@ impl AllApiMetadata {
pub fn server_components(
&self,
) -> impl Iterator<Item = &ServerComponentName> {
self.deployment_units.values().flat_map(|d| d.packages.iter())
self.deployment_units.iter().flat_map(|d| d.packages.iter())
}

/// Look up details about an API based on its client package name
Expand Down Expand Up @@ -166,14 +174,12 @@ impl TryFrom<RawApiMetadata> for AllApiMetadata {
}
}

let mut deployment_units = BTreeMap::new();
let mut deployment_units = IdOrdMap::new();
for info in raw.deployment_units {
if let Some(previous) =
deployment_units.insert(info.label.clone(), info)
{
if let Err(e) = deployment_units.insert_unique(info) {
bail!(
"duplicate deployment unit in API metadata: {}",
&previous.label,
"duplicate deployment unit name in API metadata: {}",
e.new_item().name,
);
}
}
Expand Down Expand Up @@ -206,7 +212,7 @@ impl TryFrom<RawApiMetadata> for AllApiMetadata {
// Validate that IDU-only edges reference only known server components
// and APIs.
let known_components: BTreeSet<_> =
deployment_units.values().flat_map(|u| u.packages.iter()).collect();
deployment_units.iter().flat_map(|u| u.packages.iter()).collect();
for edge in &raw.intra_deployment_unit_only_edges {
if !known_components.contains(&edge.server) {
bail!(
Expand Down Expand Up @@ -411,12 +417,21 @@ pub enum ApiConsumerStatus {
#[derive(Deserialize)]
#[serde(deny_unknown_fields)]
pub struct DeploymentUnitInfo {
/// human-readable label, also used as primary key
pub label: DeploymentUnitName,
/// human-readable name (e.g. "Nexus", "DNS Server"), also used as primary
/// key
pub name: DeploymentUnitName,
/// list of Rust packages that are shipped in this unit
pub packages: Vec<ServerComponentName>,
}

impl IdOrdItem for DeploymentUnitInfo {
type Key<'a> = &'a DeploymentUnitName;
fn key(&self) -> Self::Key<'_> {
&self.name
}
id_upcast!();
}

#[derive(Deserialize)]
#[serde(deny_unknown_fields)]
pub struct DependencyFilterRule {
Expand Down Expand Up @@ -446,6 +461,11 @@ pub enum Evaluation {
NotDeployed,
/// This dependency should not be part of the update DAG
NonDag,
/// This dependency is only used during RSS, not during normal operation or
/// upgrade. Like `NonDag`, this means the dependency should not be part of
/// the update DAG. Unlike `NonDag`, this does not require that the target
/// API uses client-side versioning.
RssOnly,
/// This dependency should be part of the update DAG
Dag,
}
Expand Down
Loading
Loading