Update dpd-api's use of address tags by cfzimmerman · Pull Request #383 · oxidecomputer/dendrite

cfzimmerman · 2026-09-30T01:33:36Z

Motivation

DPD already accepts tags for link/loopback address creation. It should probably also accept tags for deletion. Motivated by this PR comment: #370 (comment)

Changes

Since there's already an API version change, this seems like a good opportunity for a wider address tag cleanup. I can scale this down if scope seems excessive.

Added an optional tag in link_ipv*_delete and loopback_ipv*_delete. Supplying None uses the existing behavior of deleting regardless of tag. loopback_ipv*_delete now has the same NoSuchAddr error case as link_ipv*_delete. This change seems fine because the only consumer is swadm afaict.
port_settings_apply and port_settings_get now require a tag. This avoids hacky unwrap_or_default() tags in dpd.
Removed link_ipv*_reset entirely. Afaict we're not using it anywhere, and it totally undercuts the intention behind address tags: https://github.com/search?q=repo%3Aoxidecomputer%2Fdendrite+%2Flink_ipv%5B46%5D%2B_reset%2F&type=code
Removed the tag parameter from port_settings_clear. We were not using it, and it's confusing.

Removed return values from port_settings_clear and port_settings_apply. Clear is an easier sell because the return value on success is always empty by definition. Apply is suspicious because the command is only supposed to succeed if settings the caller already knows are successfully applied. This functionality was only used in chaos tests, which were pretty easy to migrate. Removing this addressed a TODO (from Nils?) and is conceptually simpler.

dendrite/dpd/src/port_settings.rs

Lines 726 to 728 in a2d1589

    
           // operation to complete.  Arguably, we should simply stop 
        
           // returning anything other than success or failure on these 
        
           // operations.

Test, swadm, and tfportd updates to use the new API.

Validation

Working on a home setup for voxel. Will test this before merge. Will also need API upgrades in sled-agent, wicket, and maybe maghemite. All cosmetic, though.

cfzimmerman · 2026-09-30T22:22:50Z

    async fn link_ipv4_delete(
        rqctx: RequestContext<Self::Context>,
        path: Path<latest::link::LinkIpv4Path>,
+        tag: Query<latest::misc::MaybeTagged<()>>,


Using unit here is kind of weird, but dropshot kindly washes it out of the client type signature anyway. Keeping it as a generic will hopefully reduce type duplication if/as this pattern spreads elsewhere in the API.

cfzimmerman · 2026-09-30T22:27:20Z

+    /// Returns an error if the address does not currently
+    /// exist (under this tag).


This is a behavioral change. Same for loopback_ipv6_delete. But it feels strange using different deletion semantics for link_ipv*_delete and loopback_ipv*_delete. This shifts loopback to how we currently handle link. Afaict loopback_ipv*_delete is only used within Dendrite, so I think this is safe to change. Please lmk if anyone disagrees.

https://github.com/search?q=org%3Aoxidecomputer+%2Floopback_ipv%5B46%5D%2B_delete%2F&type=code

cfzimmerman

🤖 This message was written by CLU

The dpd side of this is sound. Tag-scoped deletes are checked and cleared under the AddrMap write lock, so there is no TOCTOU. A missing address returns 404 whatever the tag, and a wrong tag or owner returns 409, as the delete docs now say. port_settings_apply and port_settings_clear return 204. The v1/v2 shims keep the old 200 bodies for old clients. omicron at d9554ae pins a pre-v14 client and ignores both response bodies. It never calls the removed link_ipv*_reset (now 410), and it never calls the delete endpoints.

Verdict now: ready. Every thread is addressed:

simport syncs only IPv6 link-locals, under its own tag, and creates the link only when it is missing. So it no longer collides with sled-agent on softnpu.
ensure_address_match deletes with None again, so it self-heals.
The delete docs, the 404 tests and the stale get_port_settings_locked comment are all done.

The chaos sweeps still check something real. The serial sweep now polls port_settings_get until it matches the target, which is stronger than comparing against the response the server built from the same request. The parallel sweep ends with a final apply-then-get check.

Minor, not worth threads:

port_settings_apply_v2 now echoes the request body back, where it used to return dpd's view (lane filled in, this tag's addresses as stored). No known consumer reads the body. A sentence in the shim's doc saying it echoes would keep a future reader from trusting it.
simport still continues when illumos has no link-local, before the own-tag delete pass, so a stale tfportd fe80:: can linger in dpd. That predates this PR. With deletes now tag-scoped, the guard could safely move after the delete pass.
simport's link_ipv6_create returns 409 on every poll if another tag or link already holds the illumos link-local. It's only log noise.
integration_tests/loopback.rs:95-97 and :114-116 still use a bare expect_err on the second delete. The chaos tests pin the 404.
chaos_tests/port_settings.rs:1339 calls .into_inner() on a () response. The final checks in test_port_settings_txn_par_sweep retry with Duration::ZERO, where RETRY_INTERVAL would be gentler.
Dropping v4 and non-link-local v6 mirroring from simport is a behavior change. I checked softnpu-zone, a4x2 and maghemite's --sync-only ddm tests, but couldn't rule out out-of-tree setups.

Second opinion: the design skeptic

What problem does this solve? The original problem is real and small: a cooperating client should be able to delete its own address without stepping on another's (the #370 comment). The optional-tag deletes do exactly that. The other changes are the same kind of cleanup, done while the version bump is already being paid for:

The required tag on port_settings_{apply,get} removes the implicit "" tag nobody chose.
clear loses its dead tag parameter.
apply and clear stop returning bodies.

Does the value warrant the cost? Yes. The newest commit pays for itself. Returning settings from apply was what forced get_port_settings_locked's ignore_deleting workaround: it pretended async link deletion had already finished, so that the response matched. Dropping the body deletes that special case. It also makes the tests check real server state instead of an echo. simport has been pulled back to owning the one address only it can know about. That follows the PR's thesis that a tag narrows what a caller touches.

What is the simplest robust alternative from here? simport and ports.rs::ensure_address_match are still the same algorithm written twice: reconcile tfportd's link-local between illumos and dpd. Your point in the ports.rs thread is the cleaner end state: make the IPv6 link-local an untagged, single-valued property of the link. That would collapse both loops and the 409 noise above. It's a follow-up, not a reason to hold this PR.

Not verified: I didn't run an a4x2 or softnpu-zone deployment, and I'm relying on green CI rather than a local build.

Findings: 1 demonstrable bug, 2 possible bug, 1 conceptual error, 1 test gap, 2 nit

Revised 2026-10-01 at 92ec938587ce.

cfzimmerman · 2026-10-01T21:13:36Z

-            Ok(mut returned) => {
-                sort_addrs(&mut returned);
-                // Verify that what the server attempted to configure matches
-                // what we asked them to configure.
-                assert_eq!(target, returned.into_inner());
-
-                // While attempting to apply our requested config on the server
-                // side, some operations will be async - as will cleanup after
-                // any errors.  We retry the test operation for a few seconds
-                // waiting for that to happen.
-                retry::retry_op(RETRY_INTERVAL, RETRY_MAX, || async {
-                    match current_port_settings(&client, &port).await {
-                        Err(e) => Err(retry::ReturnCode::Fatal(e.to_string())),
-                        Ok(new) => {
-                            if new == target {
-                                Ok(())
-                            } else {
-                                Err(retry::ReturnCode::Retry(format!(
-				"desired settings: {target:#?}\ncurrent settings: {new:#?}"
-			    )))
-                            }
-                        }
-                    }
-                })
-                .await?;


The retry loop effectively validates the same thing as the assert_eq, so I think it's fine to test the new API using just the retry loop.

cfzimmerman · 2026-10-01T21:16:02Z

            let client = new_dpd_client(4705);
            let target = random_port_settings();

-            match client


This is a slight regression in test coverage, but the new check at the end at least verifies the last reconciler standing sees correct behavior. I think that's what we care about most.

cfzimmerman · 2026-10-01T21:20:02Z

        poll_max: Duration,
        mut op: impl AsyncFnMut() -> Result<(), ReturnCode>,
    ) -> anyhow::Result<()> {
-        let poll_start = Instant::now();


In the old version, an indefinite op function could hang retry_op indefinitely. Now we wrap the provided fn in a tokio timeout.

cfzimmerman · 2026-10-01T21:23:16Z

-                    // operation to complete.  Arguably, we should simply stop
-                    // returning anything other than success or failure on these
-                    // operations.


I agree with this argument :)
I'm taking this comment as the mandate for my return value changes.

cfzimmerman · 2026-10-01T21:28:24Z

+/// Reconciles each port's illumos IPv6 link local address with
+/// the link config in DPD.
 async fn simnet_process(g: &Global) -> anyhow::Result<()> {


This is less than it did previously, but I think this is what we want it to do? Otherwise, if an illumos link gains an address belonging to some other service like sled agent and tfportd registers that in dpd first, then sled agent would see a 409 tag conflict every time it runs port_settings_apply with that address. This wasn't an issue before tag enforcement.

taspelund · 2026-10-05T23:19:29Z

+                if let IpAddr::V6(v6) = addr
+                    && v6.is_unicast_link_local()
+                    && name.starts_with(p)


Since we're already updating the openapi spec and adding a new API version, it may be a good time to consider using oxnet::UnicastLinkLocalIpv6Addr in tfportd... at least for the places that expect/enforce IPv6 addresses to be link-local only.

Sgtm. Will follow up on this after #374 (comment)

TODO: use oxnet type for UnicastLinkLocalIpv6Addr in tfportd. Both simport and real port.

taspelund · 2026-10-05T23:21:30Z

+
    if let Some(addr) = link.dpd_link_local
        && link.dpd_link_local != link.tfport_link_local
    {


Similar feedback here regarding IPv6 link-local types using oxnet::UnicastLinkLocalIpv6Address

taspelund · 2026-10-05T23:24:12Z

+        match attempt {
+            Err(_) => Err(anyhow::anyhow!("Timed out. Most recent err: {err}")),
+            Ok(Ok(())) => Ok(()),
+            Ok(Err(e)) => Err(e),
        }


My thoughts here are similar to feedback I left in the PR below this one in the stack:
#374 (comment)

Sgtm.

TODO: accumulate errors. And put all retry mod diffs in the base PR so they're not scattered.

taspelund · 2026-10-05T23:25:46Z

+        let Err(e) = client.port_settings_apply(&port, "chaos", &target).await
+        else {
+            // While attempting to apply our requested config on the server
+            // side, some operations will be async - as will cleanup after


possible typo? should "as" be "and"?

This is copy-paste from Nils, but here's how I interpret it:

port_settings_apply to delete a link can return success immediately. However, the reconciler actually cleans up the link asynchronously, so dpd queries may reflect the old state until the reconciler successfully finishes "cleanup". That mandates a retry loop to ensure complete success. I believe this is the mirror of this comment in dpd:

dendrite/dpd/src/port_settings.rs

Lines 723 to 726 in 5f37818

// will succeed - as indeed it almost certainly will. This

// optimism is primarily to allow the chaos tests to pass,

// since they have no mechanism to wait for this async

// operation to complete. Arguably, we should simply stop

Another possible wording is this, but I'm hesitant to rewrite Nils' comments based on my interpretation:

port_settings_apply may return success before the reconciler actually deletes the link, so we must poll until confirming that reconciled state matches what was just commanded.

taspelund · 2026-10-05T23:27:38Z

+                rollback_fail.fetch_add(1, Ordering::Relaxed);
+                return;
            }
+            //expect_random_chaos!(e);


Is this something we need to keep?

Probably not. I also wondered why it was commented out and then left as is. But it's not very useful if nobody knows why it's there.

TODO: remove this comment

taspelund · 2026-10-05T23:28:51Z

    }

+    // After all that, the link should still behave properly and accept new settings.
+    let client = new_dpd_client(4705);


magic number without a comment. Should this be a const?

This number only appears in tests where multiple clients must reach the same dpd instance. I believe this is just an arbitrary number pointing all clients at a dpd listening on this port. Although I agree that it's a bit mysterious.

TODO: investigate if we can use the ordinary harness and client.clone() instead of a hard coded port. If I do this, I'll probably put it in the bottom PR so I can verify that it still catches bugs.

taspelund · 2026-10-05T23:32:21Z

+/// Ignoring seems like the best option for now. There's not
+/// an obvious alternative short of making tag ownership a
+/// property of the link itself.


Since you mentioned tag ownership being a property of the link, why didn't you go with this approach? I'm curious to understand why we decided against that option.

Really just scope and complexity. Already at ~3k loc in this PR stack. I think it's very likely we someday make ownership a first class link property, especially if we need fine grained CRUD auth. But for now, I'm just trying to filter this project down to what solves an existing problem that a customer or other engineer might really care about. If we have an immediate answer to that, I'm totally happy to look into it.

taspelund · 2026-10-05T23:34:20Z

    #[endpoint {
        method = DELETE,
        path = "/ports/{port_id}/links/{link_id}/ipv4",
+        versions = ..VERSION_ADDRESS_TAGS
    }]
    async fn link_ipv4_reset(
        rqctx: RequestContext<Self::Context>,
        path: Path<latest::link::LinkPath>,
-    ) -> Result<HttpResponseUpdatedNoContent, HttpError>;
+    ) -> Result<HttpResponseUpdatedNoContent, HttpError> {
+        let _ = rqctx;
+        let _ = path;
+
+        Err(HttpError {
+            status_code: ErrorStatusCode::GONE,
+            error_code: None,
+            external_message: ADDR_RESET_EMSG.to_string(),
+            internal_message: ADDR_RESET_EMSG.to_string(),
+            headers: None,
+        })
+    }


Is this how other codebases with versioned dropshot APIs are deprecating old versions of endpoints? This is a scenario I haven't brushed up against, so I'm not sure what the SOP is here

This is what Zeeshan did for multicast. Didn't look much further than that.

dendrite/dpd/src/api_server.rs

Lines 2352 to 2367 in 5f37818

#[allow(unused_variables)]

async fn multicast_reset_untagged(

_rqctx: RequestContext<Arc<Switch>>,

) -> Result<HttpResponseDeleted, HttpError> {

// All groups now have default tags, making this endpoint obsolete.

// Groups are cleaned up via multicast_reset_by_tag using the tag

// returned from group creation.

Err(HttpError::for_client_error(

None,

ClientErrorStatusCode::GONE,

"multicast_reset_untagged is deprecated; all groups now have \

default tags. Use multicast_reset_by_tag with the tag returned \

from group creation."

.to_string(),

))

}

I think RFD 532 has a bit to say about this: https://rfd.shared.oxide.computer/rfd/0532#:~:text=Given%20that%20an,pretty%20much%20it.

And I was frankly less concerned here because this is used nowhere afaict: https://github.com/search?q=org%3Aoxidecomputer+link_ipv4_reset&type=code

TODO: check if we can use HttpError::for_client_error here.

taspelund · 2026-10-05T23:41:56Z

Why are we deprecating some endpoints and versioning others?

I'm deprecating the endpoints that nobody uses (according to github org:oxidecomputer). That lets me delete corresponding code from dpd.

link_ipv4_reset: https://github.com/search?q=org%3Aoxidecomputer+link_ipv4_reset&type=code

link_ipv6_reset: https://github.com/search?q=org%3Aoxidecomputer+link_ipv6_reset&type=code

If we keep link_ipv*_reset, I feel like it should support tags. But why do that work if nobody uses it? So I deprecated the endpoint and deleted the dpd code instead.

The other endpoints are broadly useful, so I'm versioning and updating them.

Does that answer your question?

taspelund · 2026-10-05T23:45:07Z

+/// to that tag.
+///
+/// Returns error if the address does not currently exist (under that tag).
+pub fn delete_loopback(


I don't like the new name for this method. delete_loopback implies we're deleting the loopback interface itself (which is valid on other network platforms) rather than deleting an address. clear_loopback makes it seem like we're clearing state from the loopback, which is better, but is arguably still vague.

Interesting, sounds good. I had renamed it to comply with the delete/clear convention described here: #374 (comment)

TODO: Rename to delete_loopback_addr

taspelund · 2026-10-05T23:46:14Z

        settings: PortSettings,
        tag: &str,
-    ) -> DpdResult<PortSettings> {
+    ) -> DpdResult<()> {


would it be helpful to return the old value here? pre-mutation

Currently every client outside of chaos tests just discards the return value of port_settings_apply. So it could be useful down the road, but I'm disinclined to add extra dpd code and docs to solve a problem we don't yet have.

The sacred texts seem to endorse returning unit 🤷‍♂️

dendrite/dpd/src/port_settings.rs

Lines 726 to 728 in 5f37818

// operation to complete. Arguably, we should simply stop

// returning anything other than success or failure on these

// operations.

taspelund

LGTM overall, just with some naming questions. The main thing that I'd like to understand before merging is the deprecation process for versioned dropshot API endpoints. I'm not sure how omicron or other components handle this (we haven't deprecated any in maghemite yet), but I'd like us to more or less follow suit so we're roughly aligned.

Require tag in port_settings_apply Require tag in port_settings_get Remove tag from port_settings_clear Optional tag in link_ipv4_delete Optional tag in link_ipv6_delete Optional tag in loopback_ipv4_delete Optional tag in loopback_ipv6_delete Remove link_ipv*_reset endpoints They're not used anywhere and interact awkwardly with tags. Test for loopback tag isolation Various changes based on review - Add HTTP codes to *_ipv*_delete docs. - Add tests for those cases - Use untagged deletion in tfportd to recover if another IPv6 addr somehow sneaks onto the link. Update tfportd simport to cooperate with tags Return nothing after port_settings_apply success

cfzimmerman added this pull request to stack #382 September 30, 2026 03:35

cfzimmerman changed the title ~~Require tag in port_settings_apply~~ Update dpd-api's use of address tags Sep 30, 2026

cfzimmerman force-pushed the cory/require-tags branch 5 times, most recently from 2d87d18 to ef3b444 Compare September 30, 2026 21:56

cfzimmerman commented Sep 30, 2026

View reviewed changes

Comment thread tfportd/src/simport.rs Resolved

cfzimmerman force-pushed the cory/require-tags branch 2 times, most recently from 9933050 to fe6262f Compare October 1, 2026 00:07

cfzimmerman mentioned this pull request Oct 1, 2026

Respect address tags in port_settings_* #370

Open

cfzimmerman commented Oct 1, 2026

View reviewed changes

cfzimmerman force-pushed the cory/require-tags branch 5 times, most recently from a69bc6a to 48789f2 Compare October 1, 2026 04:11

cfzimmerman marked this pull request as ready for review October 1, 2026 04:13

cfzimmerman commented Oct 1, 2026

View reviewed changes

taspelund reviewed Oct 5, 2026

View reviewed changes

cfzimmerman force-pushed the cory/eventual-reconciliation branch from 7925362 to 52f2d28 Compare November 6, 2026 15:33

cfzimmerman removed this pull request from stack #382 November 6, 2026 15:33

cfzimmerman force-pushed the cory/eventual-reconciliation branch from 14a8ff8 to 0edf400 Compare November 7, 2026 02:12

cfzimmerman added 2 commits November 7, 2026 04:18

new response codes: bad delete = 404, bad create = 409

2636995

cfzimmerman force-pushed the cory/require-tags branch from 92ec938 to 2636995 Compare November 7, 2026 06:04

	// operation to complete. Arguably, we should simply stop
	// returning anything other than success or failure on these
	// operations.

		/// Returns an error if the address does not currently
		/// exist (under this tag).

	// will succeed - as indeed it almost certainly will. This
	// optimism is primarily to allow the chaos tests to pass,
	// since they have no mechanism to wait for this async
	// operation to complete. Arguably, we should simply stop

	#[allow(unused_variables)]
	async fn multicast_reset_untagged(
	_rqctx: RequestContext<Arc<Switch>>,
	) -> Result<HttpResponseDeleted, HttpError> {
	// All groups now have default tags, making this endpoint obsolete.
	// Groups are cleaned up via multicast_reset_by_tag using the tag
	// returned from group creation.
	Err(HttpError::for_client_error(
	None,
	ClientErrorStatusCode::GONE,
	"multicast_reset_untagged is deprecated; all groups now have \
	default tags. Use multicast_reset_by_tag with the tag returned \
	from group creation."
	.to_string(),
	))
	}

Conversation

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

Uh oh!

Motivation

Changes

Validation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Second opinion: the design skeptic

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!

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!

cfzimmerman Oct 6, 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!

cfzimmerman Oct 6, 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!

cfzimmerman Oct 6, 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!

cfzimmerman commented Sep 30, 2026 •

edited

Loading

cfzimmerman left a comment •

edited

Loading

cfzimmerman Oct 6, 2026 •

edited

Loading

cfzimmerman Oct 6, 2026 •

edited

Loading

cfzimmerman Oct 6, 2026 •

edited

Loading

cfzimmerman Oct 6, 2026 •

edited

Loading