Enable scrimlet reconcilers in tests by internet-diglett · Pull Request #11233 · oxidecomputer/omicron

internet-diglett · 2026-09-03T16:23:26Z

This is another "this wasn't working but we should make it work" PR.
It now works, but any feedback to make it nice is appreciated.

jgallagher

Thank you very much for picking this up; having these work in tests will be awesome.

I took a quick-ish first pass focused on just the changes to sled-agent; several suggestions below. Happy to give the rest of it a full review later too.

jgallagher · 2026-09-03T18:15:57Z

+    /// The address the mgd bgp-dispatcher is listening on, used to set the
+    /// correct BGP port when creating routers and numbered neighbors. `None`
+    /// means use the standard BGP port 179.
+    bgp_dispatcher_addr: Option<SocketAddr>,


I think we're asking too much from Option<SocketAddr> here. IIUC:

None means we should configure MGD routers to listen on [::]:179, and any BGP numbered peers should be assumed to be on port 179.

Some(addr) means we should configure MGD routers to listen on addr, and any BGP numbered peers should take their IP from their config and their port from addr.

Is that right? If so, some followup questions:

Should this affect the config for unnumbered peers?

Is there a scenario where we could want the MGD router listen addr and the BGP numbered peers to be on different ports? (The "steal the port out of addr and attach to peers" is the weirdest bit of this, to me.)

The use of this is attached to a TODO The values here should come from Nexus... comment. Is this such a case, where eventually we might want either the listen addr or the peer port to be coming from Nexus?

I think we probably want a custom type to hold this info, particularly so we can hide some of the details here behind specific method names with comments, but the exact name/shape of that type kinda depends on the answers to those questions.

Apologies for the delay in responding to this comment, it had a few things I needed to think through / double check.

Should this affect the config for unnumbered peers?

The bgp dispatcher addr being configurable at launch is primarily for enabling us to let multiple mgd instances listen for BGP sessions on different loopback interfaces (in the test context the only real way we can test bgp peering in all environments is via loopback interfaces). I'm not sure we could get two loopback interfaces with ipv6 link local / addrconf addresses to communicate with each other, so it's quite possible that unnumbered peering simply won't work in these kinds of tests.

Is there a scenario where we could want the MGD router listen addr and the BGP numbered peers to be on different ports? (The "steal the port out of addr and attach to peers" is the weirdest bit of this, to me.)

As of today, I don't think so. From what I know, customers haven't asked us for the ability to use different ports for BGP. The main issue we're working around here is the fact that BGP normally listens on port 179 in production, but binding to that port requires root / priv escalation, so we use a different port in the test context.

The use of this is attached to a TODO The values here should come from Nexus... comment. Is this such a case, where eventually we might want either the listen addr or the peer port to be coming from Nexus?

Maybe. It seems that most commercial routers use port 179 and do not allow you to change the global listening port for BGP. As for configuring listen addrs, [in production] we currently listen on all available addresses today, but I could see there being a world where a customer wants to have more control over that.

I see, thanks. I think that votes for "keep this pretty simple" then; maybe something like a BgpSocketConfig (names are hard - feel free to pick something better, because it'd be easy to pick a name here that collides with the existing BgpPortConfig types...) struct that internally holds an Option<SocketAddr> (for the listening address) and an Option<u16> (for the peer port) and methods that return non-optional values for both (which default to the values we want in production)?

Mostly I'm trying to move away from "the producers and consumers of this value both have to remember what None / Some(addr) mean" into something that's more obvious in both spots.

I went with a slightly different representation of your example, but I think it accomplishes the same thing you were wanting, let me know what you think!

jgallagher · 2026-09-03T18:18:54Z

    /// configuration to emulate the sled agent's hardware
    pub hardware: ConfigHardware,
+    /// whether this sled is a scrimlet (connected to a switch)
+    pub is_scrimlet: bool,


Can we use the SledRole enum for this instead of bool? (The variant name for "non-scrimlet" being Gimlet is janky now that we have more kinds of sleds, but I think we use SledRole for this info pretty broadly so should just rename that at some point.)

jgallagher · 2026-09-03T18:26:11Z

                            sa_address: sa_address.to_string(),
                            repo_depot_port,
-                            role: NexusTypes::SledRole::Scrimlet,
+                            role: if config.is_scrimlet {


Ah yeah I think with https://github.com/oxidecomputer/omicron/pull/11233/changes#r3927414946 this can become something like role: config.role.

jgallagher · 2026-09-03T18:28:25Z

+            *config = EarlyNetworkConfigEnvelope::from(&body.body)
+                .serialize_to_bootstore_with_generation(body.generation);
+        }
+        sa.notify_network_config_changed();


I don't think we want to have to remember to call an explicit notification method; my gut feeling is that sa.bootstore_network_config should be a watch channel instead of a mutex so the recipient automatically gets notified on changes? Will think about this when I get to that point.

Ok after looking at the implementation more, I think I'd propose this:

Change sa.bootstore_network_config to be a watch channel instead of a mutex. All these HTTP handlers can become something like sa.bootstore_network_config.send_modify(|c| *c = /* ... */);.

When creating the sim-sled-agent, spawn a task that loops forever and is solely responsible for forwarding changes made to the bootstore_network_config channel into the network_config_tx channel. This is exactly what real sled-agent does to handle the type differences between the bootstore and the scrimlet-reconcilers.

As a part of this, I think it should be fine to make network_config_tx non-optional and present on all sim-sled-agents. Only scrimlets will subscribe to it, but that's fine; this is also consistent with real sled agent.

jgallagher · 2026-09-03T18:51:20Z

+        #[cfg(feature = "testing")]
+        let network_config_tx = if config.is_scrimlet {
+            let (tx, _) = tokio::sync::watch::channel(SystemNetworkingConfig {
+                rack_network_config: RackNetworkConfig {


I don't think we want to duplicate all of this from where we create bootstore_network_config above. Can we create the SystemNetworkingConfig once ahead of both, and then give it to both fields in their respective types?

jgallagher · 2026-09-03T18:56:20Z

+    /// addresses. Must only be called once and only on scrimlet sleds.
+    ///
+    /// Only available under `cfg(feature = "testing")` because it uses
+    /// [`sled_agent_scrimlet_reconcilers::ScrimletReconcilersMode::Test`].


I don't think we want put anything in sim-sled-agent behind a testing feature. sim-sled-agent is already meant for testing, so it seems weird to have a "regular sim-sled-agent" and a "sim-sled-agent with extra testing features". A couple options off the top of my head:

Remove the testing feature from the scrimlet-reconcilers crate, and just make ScrimletReconcilersMode::Test always available. I don't love this but it's probably fine?

Split sim-sled-agent out to a separate crate from sled-agent so that it can enable the testing feature(s) of downstream crates without "polluting" sled-agent proper. This feels nice from a "break sled-agent up into smaller pieces" point of view but might be more work than we'd like.

If you want to look into 2 and see how painful it is, great (if it turns out to not be painful, I'd definitely open that as a separate PR). If you don't want to or it's super painful, I think 1 would be okay.

Okay. I didn't like polluting the space with the testing feature but I wasn't sure if it was okay to make ::Test available in prod binaries, but since we're open to these approaches I'll give them both a look.

Yeah totally, I have both of those same feelings (polluting this with testing is not great, and making ::Test available in prod is not great). I think there are just more downsides to the former than the latter in practice, hence my "option 1 is okay". 🤷

jgallagher · 2026-09-03T19:02:52Z

+
+        // Store to keep the reconcilers alive. Ignore the error: if called
+        // twice it is a programmer error and we just silently drop the second
+        // set (the first set is already running).


If this is a programmer error we should probably assert / .expect() it instead of silently ignoring it? (Especially true for sim-sled-agent where the stakes are low.)

jgallagher · 2026-09-03T19:03:36Z

+    #[cfg(feature = "testing")]
+    pub fn start_scrimlet_reconcilers(
+        &self,
+        mgs_addr: std::net::SocketAddr,


Could this take a ScrimletReconcilersMode instead of several SocketAddrs that the caller has to order correctly?

jgallagher · 2026-09-03T19:07:41Z

+    }
+
+    /// Returns the current status of the scrimlet reconcilers, or `None` if
+    /// `start_scrimlet_reconcilers()` has not yet been called.


I think it would be fine to:

Always create a ScrimletReconcilers when creating a sim-sled-agent

Only call set_sled_agent_networking_info_once() / set_scrimlet_status(Scrimlet) when relevant. (The scrimlet reconcilers don't start doing anything until both of those are called.)

and then this method wouldn't need to return an option. Real non-scrimlet sled-agents still have a status, even if that status is something like WaitingForSledAgentNetworkingInfo.

internet-diglett · 2026-09-03T20:24:28Z

@jgallagher many thanks for the early feedback! I will work on this and look forward to your deeper review later.

internet-diglett · 2026-09-10T16:23:07Z

@jgallagher I believe I've addressed all of the items from your initial review, let me know what you think!

jgallagher · 2026-09-11T15:09:05Z

+/// used to avoid requiring elevated privileges.
+#[derive(Debug, Clone, Copy)]
+pub struct BgpSocketConfig {
+    /// Address mgd's BGP dispatcher listens on. `None` → `[::]:179`.


There's no Option here anymore, so I don't think we need this bit:

Suggested change

/// Address mgd's BGP dispatcher listens on. `None` → `[::]:179`.

/// Address mgd's BGP dispatcher listens on.

But it may be worth adding a comment that we also assume the peer is listening on the same port as this address. That still seems a little weird to me - in tests, if we have MGD bind to some ephemeral port instead of 179, we tell it to talk to peers on that same port?

I guess the distinction is that in the test suite each mgd will be listening on its own unique loopback interface address for BGP, so they won't have a reason to be using a different port. We use separate loopback interface addresses for each BGP peer because BGP peers must have unique addresses, and therefore we cannot have multiple BGP daemons listening and peering on the same loopback address with different port numbers.

jgallagher · 2026-09-11T15:09:58Z

+    }
+
+    /// Returns the router listen address string for mgd configuration.
+    pub(crate) fn router_listen_addr(&self) -> String {


I think this should still return a SocketAddr, and let the caller convert it to a string if/when needed.

jgallagher · 2026-09-11T15:11:14Z

 #[derive(Debug, Clone, Copy)]
 pub enum ScrimletReconcilersMode {
    SwitchZone(ThisSledSwitchZoneUnderlayIpAddr),
-    #[cfg(any(test, feature = "testing"))]


Can we remove the testing feature from this crate's Cargo.toml entirely now? I think this was the only use of it.

jgallagher · 2026-09-11T15:17:38Z

                            sa_address: sa_address.to_string(),
                            repo_depot_port,
-                            role: NexusTypes::SledRole::Scrimlet,
+                            role: match config.sled_role {


Hmm this really is the same type. Can we add another replace directive in the nexus-lockstep client for SledRole here:

omicron/clients/nexus-client/src/lib.rs

Lines 28 to 34 in 6661057

replace = {

Generation = omicron_generation_kinds::Generation,

MacAddr = omicron_common::api::external::MacAddr,

Name = omicron_common::api::external::Name,

NetworkInterface = sled_agent_types::inventory::NetworkInterface,

NetworkInterfaceKind = sled_agent_types::inventory::NetworkInterfaceKind,

},

so we can avoid having to do this conversion?

jgallagher · 2026-09-11T15:38:00Z

+                        if matches!(
+                            last.activation_reason,
+                            ReconcilerActivationReason::SystemNetworkingConfigChanged
+                        ) {
+                            Ok(())


I think this check is technically racy - on an overloaded CI system, we could have this order:

this check runs, doesn't find the status it wants, returns NotYet, goes to sleep

reconcilers run and record SystemNetworkingConfigChanged

reconcilers run again and record "periodic timer" as the activation reason

this check runs again, doesn't find the status it wants, and never will

I think the thing we actually care about is that the changes we initiated via Nexus are propagated out to MGD - can we wait on seeing MGD report those changes itself instead?

jgallagher · 2026-09-11T15:40:52Z

+/// Tests the full pipeline:
+///   Nexus (sync_switch_configuration)
+///     → sled-agent bootstore updated
+///     → `notify_network_config_changed()` called


notify_network_config_changed() is mentioned a couple times but doesn't exist anymore (and arguably would be an internal sim-sled-agent detail not really relevant to the test here). I think maybe I'd phrase this in terms of the larger pieces instead of specific functions or watch channels; something like

- Nexus pushes changes to the bootstore - sled-agent reconciles those changes against the relevant networking services

jgallagher · 2026-09-11T15:43:39Z

    local_storage_error_count: AtomicU32,
-    pub bootstore_network_config: Mutex<bootstore::NetworkConfig>,
+    pub bootstore_network_config:
+        tokio::sync::watch::Sender<bootstore::NetworkConfig>,


This should be private, I think. If we need read access for tests, we could add a bootstore_network_config_subscribe() -> watch::Receiver<_> method?

jgallagher · 2026-09-11T15:49:06Z

+    network_config_tx: tokio::sync::watch::Sender<
+        sled_agent_types::system_networking::SystemNetworkingConfig,
+    >,


SystemNetworkingConfig is already imported; could we also import tokio::sync::watch so this could become

Suggested change

network_config_tx: tokio::sync::watch::Sender<

sled_agent_types::system_networking::SystemNetworkingConfig,

>,

network_config_tx: watch::Sender<SystemNetworkingConfig>,

jgallagher · 2026-09-11T15:50:59Z

+    scrimlet_reconcilers:
+        std::sync::Arc<sled_agent_scrimlet_reconcilers::ScrimletReconcilers>,


I don't think we need the Arc here. Could we also import ScrimletReconcilers, shortening this to

Suggested change

scrimlet_reconcilers:

std::sync::Arc<sled_agent_scrimlet_reconcilers::ScrimletReconcilers>,

scrimlet_reconcilers: ScrimletReconcilers,

?

jgallagher · 2026-09-11T15:53:21Z

@jgallagher I believe I've addressed all of the items from your initial review, let me know what you think!

Looks great! I left a bunch of nitpicks with only one serious concern (the might-be-flaky test).

fixup comment Co-authored-by: John Gallagher <john@oxidecomputer.com>

Enable scrimlet reconcilers for test context

41b64b8

internet-diglett force-pushed the enable-scrimlet-reconcilers-in-tests branch from 3fb955f to 41b64b8 Compare September 3, 2026 16:28

EXPECTORATE

b3215b6

jgallagher reviewed Sep 3, 2026

View reviewed changes

internet-diglett added 6 commits September 4, 2026 21:02

use SledRole instead of bool for designating sim sled-agent as scrimlet

56db20f

use ScrimletReconcilersMode instead of a bag of addrs

2933876

Remove testing feature requirement

a2c8b77

Remove need to notify reconciler on config update

ca3cff8

deduplicate system network config creation

51d8a7c

use a clearer representation for bgp socket configuration

99c0fed

internet-diglett requested a review from jgallagher September 10, 2026 16:23

jgallagher reviewed Sep 11, 2026

View reviewed changes

internet-diglett and others added 5 commits September 12, 2026 15:31

Update sled-agent/scrimlet-reconcilers/src/handle.rs

b9482bb

fixup comment Co-authored-by: John Gallagher <john@oxidecomputer.com>

Update sled-agent/scrimlet-reconcilers/src/handle.rs

caf16cf

fixup comment Co-authored-by: John Gallagher <john@oxidecomputer.com>

return SocketAddr instead of String

cd7acaa

remove unneeded testing feature

38b7dd8

use replace on SledRole

4e2431f

	/// Address mgd's BGP dispatcher listens on. `None` → `[::]:179`.
	/// Address mgd's BGP dispatcher listens on.

	replace = {
	Generation = omicron_generation_kinds::Generation,
	MacAddr = omicron_common::api::external::MacAddr,
	Name = omicron_common::api::external::Name,
	NetworkInterface = sled_agent_types::inventory::NetworkInterface,
	NetworkInterfaceKind = sled_agent_types::inventory::NetworkInterfaceKind,
	},

		scrimlet_reconcilers:
		std::sync::Arc<sled_agent_scrimlet_reconcilers::ScrimletReconcilers>,

	scrimlet_reconcilers:
	std::sync::Arc<sled_agent_scrimlet_reconcilers::ScrimletReconcilers>,
	scrimlet_reconcilers: ScrimletReconcilers,

Uh oh!

Conversation

internet-diglett commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgallagher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

internet-diglett Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

internet-diglett commented Sep 3, 2026

Uh oh!

internet-diglett commented Sep 10, 2026

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgallagher commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

internet-diglett commented Sep 3, 2026 •

edited

Loading

internet-diglett Sep 4, 2026 •

edited

Loading