Add `allow_ddm_traffic` parameter for link creation by andrewjstone · Pull Request #346 · oxidecomputer/dendrite

andrewjstone · 2026-08-18T21:36:10Z

Plumb this through such front ports can run ddm for multirack.

taspelund

Looks pretty straightforward to me. Just one typo and a couple questions about struct naming + which struct owns the new param.

cfzimmerman

Lgtm!

I appreciate this is time sensitive, so feel free to merge. However I'm curious about validation precedent.

Given more time, how might we test this? What do you think is warranted validation for a change like this? Here are some ideas:

A basic dpd-client test could apply these settings and verify that uplink == true on the resulting link. This is pretty low value, but I guess it would help something like a messy git resolution or big refactor.
Do we have any existing regression tests on the omicron/dendrite API? Guessing these would live on the omicron side?
I feel that we should eventually have an automated CI test somewhere for multirack networking. Since it combines (at least) omicron, maghemite, and dendrite systems, would we orchestrate this on a4x2/voxel? Do we have precedent/infra for this kind of automated testing?

cfzimmerman · 2026-08-19T14:52:13Z

+    /**
+     * Apply port settings atomically.
+     *
+     * These settings will be applied holistically, and to the extent possible
+     * atomically to a given port. In the event of a failure a rollback is
+     * attempted. If the rollback fails there will be inconsistent state. This
+     * failure mode returns the error code "rollback failure". For more details see
+     * the docs on the [`PortSettings`] type.
+     */


Is there a convention for when we use /** ... */ versus /// doc comments? This module has both. Aesthetically, sticking to one is nice. Feel like I don't see /** */ in the wild very often.

But you don't need to change this. Just curious.

That said, the contents of the comment are great. When thinking of future design, it would be nice to never need "inconsistent state" in documentation 😅

This comment was part of the original docs and just copied verbatim to the new version of the API. I personally think we should use /// everywhere. But in this case it didn't make sense for me to modify it.

andrewjstone · 2026-08-19T15:40:19Z

Lgtm!

I appreciate this is time sensitive, so feel free to merge. However I'm curious about validation precedent.

Given more time, how might we test this? What do you think is warranted validation for a change like this? Here are some ideas:

* A basic dpd-client test could apply these settings and verify that `uplink == true` on the resulting link. This is pretty low value, but I guess it would help something like a messy git resolution or big refactor.

* Do we have any existing regression tests on the omicron/dendrite API? Guessing these would live on the omicron side?

* I feel that we should eventually have an automated CI test somewhere for multirack networking. Since it combines (at least) omicron, maghemite, and dendrite systems, would we orchestrate this on a4x2/voxel? Do we have precedent/infra for this kind of automated testing?

Thanks for taking a look Cory. I'm in the process of testing this in voxel, although it requires a few more changes in omicron to do so. I'm not going to merge until all the related PRs are ready to go.

Besides that I have no idea how to test this. I don't know what tests do or don't exist and what people normally do for changes like this.

cfzimmerman · 2026-08-19T16:06:22Z

Besides that I have no idea how to test this. I don't know what tests do or don't exist and what people normally do for changes like this.

Sgtm, thx for clarifying. Figuring this out and adding any documentation or tooling to standardize dendrite PR validation is on my to-do list.

nicolaskagami

Found a few problems that need looking into:

Easy: new LinkSpec field but no modify_link change.
Easy: I think we want to pass it through in api_server.rs, no?
Annoying: I thought it was wrong to set it to false for rear ports, but as I mentioned on another comment I think the problem is that the documented semantics don't match the implementation. It currently only seems to apply for Qsfp, more specifically just setting Ipv6 and disabling uplink. At the very least we need to communicate this better for maintainability.

nicolaskagami · 2026-08-19T16:42:21Z

                        kr: true,
                        lane: Some(dpd_types::link::LinkId(0)),
                        tx_eq: None,
+                        allow_ddm_traffic: false,


Shouldn't DDM run on rear ports?

nicolaskagami · 2026-08-19T16:46:29Z

                autoneg: l.config.autoneg,
                kr: l.config.kr,
                tx_eq: l.tx_eq,
+                allow_ddm_traffic: false,


Shouldn't we pass it through here?

Suggested change

allow_ddm_traffic: false,

allow_ddm_traffic: l.config.allow_ddm_traffic,

nicolaskagami · 2026-08-19T17:00:55Z

+    /// Whether DDM traffic is allowed on this link.
+    ///
+    /// This defaults to `false`.
+    #[serde(default)]
+    pub allow_ddm_traffic: bool,


I think the semantics are not matching the behavior. allow_ddm_traffic: false still allows DDM traffic is it isn't a Qsfp port. We should either change the description (and name) to match the current behavior or the other way around.

nicolaskagami · 2026-08-19T17:06:18Z

    pub ipv4: BTreeSet<Ipv4Addr>,
    pub ipv6: BTreeSet<Ipv6Addr>,
    pub tx_eq: Option<TxEq>,
+    pub allow_ddm_traffic: bool,


We're adding a new field here but the modify_link wasn't changed to apply it. It would definitely benefit from a bit of a refactor, or at least a destructuring pattern, so we'd get compilation errors next time this happens.

nicolaskagami · 2026-08-19T17:11:31Z

+        //
+        // For multirack ddm traffic on the front ports we must also enable
+        // ipv6.
+        let ipv6_enabled =
+            !matches!(port_id, PortId::Qsfp(_)) || params.allow_ddm_traffic;
        // By default we expect external-facing links to be used as uplinks and
        // internal-facing links for backplane traffic.
-        let uplink = matches!(port_id, PortId::Qsfp(_));
+        //
+        // We need to disable uplinks for multirack ddm traffic on the front
+        // ports.
+        let uplink =
+            matches!(port_id, PortId::Qsfp(_)) && !params.allow_ddm_traffic;


Is this the only place where allow_ddm_traffic effects change? If so, it seems to only affect Qsfp ports, which is different semantics than what it's described as.

nicolaskagami · 2026-08-19T21:30:31Z

Applied the two "easy" fixes (after talking to @andrewjstone). There's a bunch around that could be improved long-term. Particularly:

There's some duplication of business logic going on.
The whole Modify<_> could be simplified and made more robust.

andrewjstone · 2026-08-19T23:37:36Z

Thanks for the fixes @nicolaskagami. Much appreciated.

andrewjstone · 2026-08-19T23:47:13Z

@nicolaskagami I think I updated all the relevant comments to reflect reality.

nicolaskagami

Thanks for bearing with me!

taspelund · 2026-08-20T21:18:22Z

@@ -721,6 +723,25 @@ pub trait DpdApi {
        params: TypedBody<latest::link::LinkCreate>,
    ) -> Result<HttpResponseCreated<latest::link::LinkId>, HttpError>;


I think the guidance in RFD 619 is to always use fully qualified type versions in the dropshot trait, rather than latest. For the endpoints you're bumping, I would also change the signature to use the specific version in the path i.e. v13::link::LinkCreate

We only use specif versions for prior prior versions of the API. The latest version should use a floating identifier .

FWIW, the claude skill in omicron handled this correctly.

andrewjstone · 2026-08-21T00:20:58Z

Good news, this appears to work correctly. With a small change to voxel, which brings up an uplink on qsfp0/0 and interconnect port on qsfp2/0, we see the settings correctly set in dpd.

Here's the relevant voxel patch in voxel/src/rss_request.rs:

@@ -132,6 +133,7 @@ fn uplink_port(p: &UplinkPort, mode: RouterMode) -> Result<PortConfig> {
         autoneg: false,
         lldp: Some(lldp(&p.switch, &p.lldp)),
         tx_eq: None,
+        allow_ddm_traffic: false,
     })
 }

@@ -152,6 +154,7 @@ fn interconnect_port(switch: &str, port: &str) -> Result<PortConfig> {
         autoneg: false,
         lldp: Some(lldp(switch, &format!("interconnect-{port}"))),
         tx_eq: None,
+        allow_ddm_traffic: true,
     })
 }

That triggers the PortConfig in the RackNetworkConfig for those ports to be send to RSS which puts them in the bootstore. The dpd-reconciler reads them and calls port_settiings_apply on the dpd client.

Looking at the output after RSS we see:

root@oxz_switch:~# swadm link get-prop qsfp0/0 uplink
true
root@oxz_switch:~# swadm link get-prop qsfp0/0 ipv6-enabled
false
root@oxz_switch:~# swadm link get-prop qsfp2/0 uplink
false
root@oxz_switch:~# swadm link get-prop qsfp2/0 ipv6-enabled
true

Add allow_ddm_traffic parameter for link creation

fcf5fa7

Plumb this through such front ports can run ddm for multirack.

andrewjstone requested review from cfzimmerman, nicolaskagami and taspelund August 18, 2026 21:36

taspelund reviewed Aug 18, 2026

View reviewed changes

Comment thread dpd-types/versions/src/allow_ddm_traffic/link.rs

taspelund reviewed Aug 18, 2026

View reviewed changes

Comment thread dpd-types/versions/src/allow_ddm_traffic/link.rs

taspelund reviewed Aug 18, 2026

View reviewed changes

Comment thread dpd/src/link.rs Outdated

taspelund approved these changes Aug 18, 2026

View reviewed changes

andrewjstone added 3 commits August 18, 2026 22:23

Merge branch 'main' into ajs/ddm-link-settings

386f762

fix types after merge

5bac560

fix typo

d47b431

cfzimmerman approved these changes Aug 19, 2026

View reviewed changes

nicolaskagami requested changes Aug 19, 2026

View reviewed changes

nicolaskagami added 2 commits August 19, 2026 17:01

pass allow_ddm_traffic through for LinkSettings

66b6b20

impose allod_ddm_traffic logic on modify_link

7da3486

fix docs

1cab7c7

more doc fixes

67635f6

nicolaskagami approved these changes Aug 20, 2026

View reviewed changes

taspelund reviewed Aug 20, 2026

View reviewed changes

andrewjstone merged commit 2df9101 into main Aug 21, 2026
6 checks passed

andrewjstone deleted the ajs/ddm-link-settings branch August 21, 2026 15:53

	allow_ddm_traffic: false,
	allow_ddm_traffic: l.config.allow_ddm_traffic,

		@@ -721,6 +723,25 @@ pub trait DpdApi {
		params: TypedBody<latest::link::LinkCreate>,
		) -> Result<HttpResponseCreated<latest::link::LinkId>, HttpError>;

Uh oh!

Conversation

andrewjstone commented Aug 18, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taspelund left a comment

Choose a reason for hiding this comment

Uh oh!

cfzimmerman left a comment • 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!

cfzimmerman Aug 19, 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!

cfzimmerman Aug 19, 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!

andrewjstone commented Aug 19, 2026

Uh oh!

cfzimmerman commented Aug 19, 2026

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!

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 commented Aug 19, 2026

Uh oh!

andrewjstone commented Aug 19, 2026

Uh oh!

andrewjstone commented Aug 19, 2026

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!

Choose a reason for hiding this comment

Uh oh!

andrewjstone commented Aug 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cfzimmerman left a comment •

edited

Loading

cfzimmerman Aug 19, 2026 •

edited

Loading

cfzimmerman Aug 19, 2026 •

edited

Loading