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.

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.

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

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!

internet-diglett commented Sep 3, 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