dynamic external ddm peers by rcgoodfellow · Pull Request #911 · oxidecomputer/maghemite

rcgoodfellow · 2026-09-11T08:32:51Z

Background

This PR adds support for dynamic external peers. Up until now ddm peers have been created for every backplane port on an Oxide rack. That meant that at startup one peer state machine is spun up for each physical port and lasts the lifetime of the ddmd process. Additional peer state machines are not created beyond that point, and existing state machines are not torn down.

With the introduction of regional cluster underlay networks, there is a need for DDM peerings to take place over external ports to extend the underlay network across racks. The lifetime of an external peering is a function of operator configuration and not of rack construction like the internal backplane peerings.

PR #877 was a first swing at accomplishing dynamic external peers. That PR is build around a design with a centralized overseer held behind a synchronous lock where 1) then endpoint that modifies peers takes an overseer write lock, 2) many other DDM admin API operations require a read lock on the overseer, and 3) release of the overseer lock depends on asynchronous runtime tasks making progress (1, 2). This presents the opportunity for ddm to become deadlocked.

This PR takes a bunch of cues from #877, but takes a more decentralized approach to external peers. The admin API handler is an orchestrator rather than a resource owner. External peer threads are launched by admin API handlers. Shutdown is done through the event bus. Peers also announce themselves to their siblings through the event bus. No new global lock is required for this approach. However it does require careful ordering of operations for things like peer expiration to close race windows with outstanding threads that are shutting down like the initial pull thread of the exchange state.

What's Included

New API Endpoints

Addition of symmetric get_external_peers and set_external_peers API endpoints. These endpoints provide an interface for the control plane to manage external peerings. The set_external_peers endpoint is a total specification. Any active peerings not present in a request will be removed and any peerings present in the specification that do not exist in ddmd will be created.

Implementation

Adding dynamic external peers has some basic underlying machinery needs.

In-memory bookkeeping to keep track of what external peers are currently running. This is not maintained persistently by ddmd as the control plane should reconciling the single persistent source of truth to the daemon.
A mechanism to start and stop external peers sessions.
Ensuring that dynamic peers are included in stats collection.
Dynamic peers are specifically for transit-to-transit router sessions, so we need path vector loop breaking.
Transit-to-transit redistribution.
Peer state machines that can deal with sibling peer state machines disappearing.

Starting New Peers

The start mechanism for dynamic peers mirrors how static internal peers are started. A dynamic peer is started with default parameters on a specified illumos address object. We may want to extend the external peers API to allow for session parameterization, but that will not be included in this MVP PR.

The set of event channels for communicating with other peer sessions is cloned from the handler context and the new peers' event channel transmitter is added to the handler context event channel. When the peer reaches the established state it broadcasts a AdminEvent::NewExternalPeer(Sender<Event>) message to all sibling peers. This is a new admin event introduced in this PR. It's a way for a new dynamic peer to announce itself to its siblings and provide them with an event transmitter that can be used to send it peering events.

When a resident peer receives a NewExternalPeer(Sender<Event>) event, they attempt to redistribute their routes to the new peer through the sender provided. If that is successful they add the sender to their list of event channels. The only way this can fail is if the new external peer has been destroyed by the time the resident peer attempts to redistribute through it (this can happen if the peer is destroyed shortly after creation which is very possible under automation). In this situation, the new peer's event sender is not added to the resident peer's sibling peer event channel set.

Removing Peers

Removing peers takes place through event channels. When a set_external_peers request comes in that removes a peer, the API handler which maintains an event sender for each peer sends a AdminEvent::Shutdown event to the running state machine for the peer. That causes the peer to halt the state machine and return. If the peer is in the established state, it uses the existing expire_peer mechanism to notify existing peers that its session is going down and that the appropriate withdraws need to be made in sibling sessions.

The API handler also removes the peer form the in-memory database and handler context.

Transit-transit redistribution

Until now transit-transit redistribution has been stubbed out. We were notably missing path breaking which has been added in this PR.

Testing

A new zone-based falcon test environment has been added to tests/src/ddm.rs called test_external_peer_sextet(). This is a two sidecar/scrimlet four sled testing environment. A number of changes needed to be made to the common machinery for setting up zone tests to accommodate two sidecar/scrimlet nodes as much of the infrastructure assumed a single sidecar/scrimlet node.

The new tests themselves are run in run_sextet_tests. They set up the two-switch/four-sled topology with a set of prefix announcements coming from the sled routers. The test then proceed to add and remove external peers, making sure that the observable result through the API is what we expect in terms of how many peerings each router has, and what prefixes each router has. This is done initially for a few specific cases. Then a test suite runs that iterates through 100 random peer changes and validates that the result is correct according to expectation functions computed over the set of external peers that was just given to set_external_peers.

The exchange endpoints all talk to dendrite over tokio, and they all take a context lock. If we hold the context lock across the tokio ops if we get enough simultaneous requests we'll stop tokio from being able to run while at the same time depending on tokio to make progress for the blocking lock to release the dam.

nicolaskagami

Doing a first pass

Shouldn't there be a tests/conf/dpd-ports-sextet.toml?

nicolaskagami · 2026-09-16T14:36:37Z

+    async fn set_external_peers(
+        ctx: RequestContext<Self::Context>,
+        request: TypedBody<ExternalPeers>,
+    ) -> Result<HttpResponseUpdatedNoContent, HttpError> {


Since set_external_peers doesn't wait for all the SMs to expire their peers (which was one of the links in the deadlock on #877), what is preventing a race condition for a quick succession of set_external_peers where the first one deletes and the second recreates?

I'm not sure exactly what we're achieving with our current context lock. I take it we want to guarantee that each API call is serialized but in this case that does not extend to their ramifications (e.g. on the state machines). I think that, for it to be consistent/useful, we'd have to actually wait for the API intent to be fully applied, which has its own difficulties as we've seen on #877.

I think the long term fix would have to be a refactor where we'd move from high-level C-style mutexes to the idiomatic Rust style of guarding mutability wherever it's neeeded. Changes should be much easier to reason about after the initial time investment.

A more immediate fix (for this race) would be to either lock SMs by their address object or associate the Routes with the owning SM so they don't clash.

It may also be that the race is exceedingly rare and we may not care about it at this moment. We could probably adapt the sextet test to exercise this.

what is preventing a race condition for a quick succession of set_external_peers where the first one deletes and the second recreates?

The second will be stuck in discovery until it can bind to the unicast listening address. The only way it can bind is if the first lets go of its unicast socket and the operating system's TIME_WAIT expires.

I suggest that we only free the listening address after expire_peers then.

rcgoodfellow · 2026-09-16T18:21:10Z

Shouldn't there be a tests/conf/dpd-ports-sextet.toml?

No. That's an artifact of how dpd/sofnpu used to be set up. I can get rid of that across the board for all test topologies across the board I believe.

now that these are uuids, this is the ohly way

andrewjstone

Looks great. Does the right thing via oxidecomputer/omicron#11143.

andrewjstone · 2026-09-21T16:51:58Z

 }

+impl HandlerContext {
+    pub fn event_channels(&self) -> impl Iterator<Item = &Sender<Event>> {


This is a great change, along with removing the redundant vec.

From talking to Ry in person, it sounds like this is really meant to represent an event bus.
Maybe we could change the name to reflect that?

andrewjstone · 2026-09-21T17:02:48Z

+                }
+            }
+        }
+        // remove peers back to front so we don't shift the order our from under


Suggested change

// remove peers back to front so we don't shift the order our from under

// remove peers back to front so we don't shift the order out from under

I'm not really sure why this is needed. What am I missing here?

We remove peers by an index we calculated above. So we need to remove them from highest index first. Otherwise if we remove the peer at index 1, all peers higher than index one will shift down one position and a subsequent removal of the peer at index 3 will be for the peer that was previously at 4 when we calculated the index, or possibly an index that is beyond the current length of the vector.

andrewjstone · 2026-09-21T17:25:32Z

                self.ctx.event_channels.len()
            );

+            // Only send withdraws for expirations that result in a total loss


This would be a nice invariant to check with a property based test some day.

andrewjstone · 2026-09-21T17:32:27Z

+    }
+    impl Drop for DropDump {
+        fn drop(&mut self) {
+            // Async just loves to make things difficult, it's taken over all


nicolaskagami

Overall looks good to me 👍

I would definitely like us to further guarantee that the race condition doesn't happen, as I suggested below, especially if it isn't too difficult.

nicolaskagami · 2026-09-28T16:39:21Z

+                        remove_idx.insert(i);
+                        info!(
+                            ctx.log,
+                            "removing external peeer for address object {aobj}"


Suggested change

"removing external peeer for address object {aobj}"

"removing external peer for address object {aobj}"

nicolaskagami · 2026-09-28T16:49:47Z

+    async fn set_external_peers(
+        ctx: RequestContext<Self::Context>,
+        request: TypedBody<ExternalPeers>,
+    ) -> Result<HttpResponseUpdatedNoContent, HttpError> {


I suggest that we only free the listening address after expire_peers then.

andrewjstone · 2026-10-01T20:07:14Z

Is this code safe to merge? I'd like to get it in so once oxidecomputer/omicron#11143 is ready I can setup the references.

taspelund · 2026-10-01T22:23:38Z

+            withdraw: self
+                .withdraw
+                .iter()
+                .filter(|x| !x.path.contains(hostname))


It makes me nervous to filter routes from a withdrawal message... seems like an area where we need to tread carefully in case there's ever a situation where a route is advertised (acceptable attributes), modified (gains unacceptable attribute), then withdrawn -- an unacceptable attribute in the withdrawal causes it to be ignored.

Yeah. The goal here is to prevent loops. And we cannot create a loop through withdraws. Changed to not filter withdraws.

taspelund · 2026-10-01T22:26:51Z

    pub event_channels: Vec<Sender<Event>>,
    pub rt: Arc<tokio::runtime::Handle>,
-    pub hostname: String,
+    pub router_id: String,


Do we ever expect this to change? e.g. when SMF refresh happens and the hostname changes, should this also be updated? (when not explicitly configured via CLI) If so, do we have logic to ensure route state is also kept in sync? (i.e. routes are re-advertised with the new router_id the path vector, a Pull is sent to peers to re-learn routes with a router_id we no longer own that may have filtered, the local RIB is updated)

No. This property is not smf-refreshable. See this comment for more details. I believe what we really want for a router id change is a router restart.

taspelund · 2026-10-01T22:32:02Z

+            for (i, p) in ctx.peers.iter().enumerate() {
+                if p.iface.external {
+                    if &p.config.aobj_name == aobj {
+                        let _ = p.tx.send(Event::Admin(AdminEvent::Shutdown));


This whole rigmarole smells like we're using the wrong data structure for this. It seems like this would all be immediately simplified by using a HashMap keyed on aobj_name

This is meant to be an event bus for all the peers, right? Maybe even switching over to an mpmc channel would simplify things even further and we wouldn't even need to iterate over all the peers when updating things?

It could potentially be a hash map with some sort of stable key, but i'd like to defer that to a follow up PR unless there is some reason we think reverse iteration over indexes for deletion is unsound.

mpmc is not in stable rust. It also means that messages would need destination addresses and receivers would need to filter.

taspelund · 2026-10-01T22:32:45Z

+pub const SOLICIT_INTERVAL: Duration = Duration::from_millis(2000);
+pub const EXPIRE_THRESHOLD: Duration = Duration::from_millis(5000);
+pub const DISCOVERY_READ_TIMEOUT: Duration = Duration::from_millis(1000);
+pub const IP_ADDR_WAIT: Duration = Duration::from_millis(1000);
+pub const EXCHANGE_TIMEOUT: Duration = Duration::from_millis(3000);


I would probably lean towards making these associated constants of struct Tunables, but that's not a blocker IMO

I guess the reason for splitting these out is because we want to use them as clap args in ddmadm? If clap lets you flatten out the args, maybe we could just put Tunables as part of the arg structure and flatten it out (so we don't break CLI compatibility)

taspelund · 2026-10-01T22:33:33Z

        // TODO this is gross, use link type properties rather than futzing
        // around with strings.


Is there a reason we aren't just converting this over to using client.link_uplink_get()?

It was not needed to unblock multirack and can be done in it's own PR.

taspelund

I think it's okay to merge this now, but I would like to see a couple of the other items I noted addressed in a follow-up. I can submit something after we hit merge on this

rcgoodfellow added 21 commits September 11, 2026 08:30

a sketch of dynamic external ddm peers

4cc6222

linux chores

bad4ba1

plumb ddmadm

6743700

dynamic peer shutdown logic

671c90b

test environment for external peers, various fixes

d91e8be

add tests, fix things uncovered by tests

7a25f46

add combinatorial tests and fix what that surfaced

747d7ba

add GET endpoint for external peers

d1bf043

plumb stats so they work with dynamic external peers

bf29b37

add redistribution tests and fix issues they uncovered

43a8b77

allow routes on qsfp through to dpd

9eea2ce

limit ext peer removal to ext interfaces, plumb tunables

b900076

check for new peers in exchange/init

2ea011a

remove dupe event channels vec at handler ctx level

3d29fe5

more tunable plumbing

e9478de

linux bs

ddb1989

plumb dendrite tunable

20cfcc3

reject external peers api calls for non-transit routers

ea4f02d

extend reach tests to include transit routers

257b0e9

test that all nexthops are peers

9be4cdb

rcgoodfellow marked this pull request as ready for review September 15, 2026 09:07

close possible shutdown race with pull threads

4ba4c1f

nicolaskagami reviewed Sep 16, 2026

View reviewed changes

nicolas' feedback

999fdd6

rcgoodfellow mentioned this pull request Sep 17, 2026

External peer address objects could have a better type than string #914

Open

andrewjstone reviewed Sep 17, 2026

View reviewed changes

Comment thread ddm-api/src/lib.rs Outdated

andrewjstone mentioned this pull request Sep 17, 2026

Multirack join service part 4 oxidecomputer/omicron#11143

Open

log loop breaking, PUSH->PUT, use sled id as router id

886ee97

andrewjstone mentioned this pull request Sep 17, 2026

Add API endpoints for mg-ddm to configure frontports for routing #793

Open

ddmadm: path element per line

26fcf26

now that these are uuids, this is the ohly way

jgallagher mentioned this pull request Sep 18, 2026

ddmd's handling of SMF refreshes is incomplete #918

Open

rcgoodfellow added 2 commits September 19, 2026 06:06

add an explicit router-id parameter to ddmd

182a8df

🤦

23b7c04

rcgoodfellow mentioned this pull request Sep 19, 2026

derive switch zone ddm router id from baseboard id oxidecomputer/omicron#11326

Merged

andrewjstone approved these changes Sep 21, 2026

View reviewed changes

ajs feeback

e34fe56

nicolaskagami approved these changes Sep 28, 2026

View reviewed changes

taspelund reviewed Oct 1, 2026

View reviewed changes

trey's feedback

d2bfae3

taspelund approved these changes Oct 2, 2026

View reviewed changes

rcgoodfellow merged commit a070c58 into main Oct 2, 2026
21 checks passed

rcgoodfellow deleted the ry/external-peers branch October 2, 2026 18:59

	// remove peers back to front so we don't shift the order our from under
	// remove peers back to front so we don't shift the order out from under

	"removing external peeer for address object {aobj}"
	"removing external peer for address object {aobj}"

		// TODO this is gross, use link type properties rather than futzing
		// around with strings.

Conversation

rcgoodfellow commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

What's Included

New API Endpoints

Implementation

Starting New Peers

Removing Peers

Transit-transit redistribution

Testing

Uh oh!

nicolaskagami left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

rcgoodfellow commented Sep 16, 2026

Uh oh!

Uh oh!

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

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

andrewjstone commented Oct 1, 2026

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!

rcgoodfellow Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

rcgoodfellow commented Sep 11, 2026 •

edited

Loading

rcgoodfellow Oct 2, 2026 •

edited

Loading