Fix indefinite port_settings_* failures by cfzimmerman · Pull Request #374 · oxidecomputer/dendrite

cfzimmerman · 2026-09-14T18:53:54Z

Prefix parlance

"create" fails if the target entry already exists. Transition from nothing to something.
"delete" fails if the target entry does not exist. Transition from something to nothing.
"set" succeeds as long as the target entry ends up in the collection. No issue if the target already exists.
"clear" succeeds as long as the collection ends without the entry. No issue if the entry already does not exist.

In my view, set and clear are generally more useful APIs for DPD's reconciler.

Summary

Table operations previously used create/delete, which caused unrecoverable failures in sequences of the following form. I propose using set/clear instead to absorb these failures in subsequent reconciliation attempts.
- Example 1: reconcile_link, which doesn't use txn/rollback:
  - Attempt 1:
    - delete asset 1. Success.
    - delete asset 2. Fails. Return error.
  - Every subsequent attempt:
    - delete asset 1. Fails because asset 1 doesn't exist.
- Example 2: port_settings_apply, which uses txn/rollback:
  - Attempt 1:
    - create asset 1. Success.
    - create asset 2. Fails.
    - Roll back asset 1. Fails. Unlikely but possible. This happens in the txn chaos test.
  - Every subsequent attempt:
    - create asset 1. Fails because asset 1 already exists.
Changing that raised an issue with how addresses are globally synchronized. We're currently using AsicError::Exists from the SDE to prevent one link from stealing an address belonging to another. set/clear is actually a regression in that it ignores previous ownership. However, afaict DPD already carries a grave distrust of the SDE, and using it for synchronization seems questionable. Since link/loopback addresses share global switch tables, this PR proposes tracking these addresses in a global table and not relying on the SDE for synchronization.
There was substantial code duplication in the link and port_settings modules for IPv4 and IPv6 operations. The new global table uses generics, which helped deduplicate some code.

Fixes

Running only the first commit yields the following test failures. test_port_settings_txn_sweep fails roughly 1/10 runs, which motivated this PR in the first place. The same failure case is reproduced in settings_eventually_reconcile.

failures:
    chaos_tests::port_settings::deletion_doesnt_leak_table_entries
    chaos_tests::port_settings::link_addrs_are_isolated
    chaos_tests::port_settings::link_init_recovers
    chaos_tests::port_settings::loopback_addrs_are_isolated
    chaos_tests::port_settings::loopback_cannot_steal_addrs
    chaos_tests::port_settings::partial_failures_are_recoverable
    chaos_tests::port_settings::settings_eventually_reconcile

test result: FAILED. 18 passed; 7 failed; 0 ignored; 0 measured; 0 filtered out; finished in 102.20s

cfzimmerman · 2026-09-17T23:40:47Z

This has a lot of new tests. But they're hand written, and I think they'll be useful at catching regressions. I don't take a political position on TDD, but I did kind of use TDD in this PR when isolating bugs.

taspelund · 2026-10-05T21:32:34Z

+
+    let port1: PortId = "qsfp0".parse()?;
+    let port2: PortId = "qsfp1".parse()?;
+    let mut rng = IpRng::new(1404);


This value is also used as a seed for two different tests

copy paste 😞

Per your suggestion above, I'll make a generator.

taspelund · 2026-10-05T21:37:51Z

+/// This tests the order where link registration wins.
+#[tokio::test]
+async fn link_addrs_are_isolated() -> anyhow::Result<()> {
+    let no_failures = AsicConfig::uniform_set(TESTING_RADIX, 0.);


nit: I think it would raise fewer eyebrows to use 0.0 instead of 0.. I didn't realize that was even valid syntax (obviously it passed CI so it seemed to be) until I looked into this. I know it's annoying to nit pick valid syntax, but I think it causes distraction where it's not needed.

this is repeated a few times in this file

Sgtm. Updated to add trailing zeros everywhere I did this. Was just muscle memory that ironically I think I picked up from ocaml.

taspelund · 2026-10-05T22:02:03Z

+/// Dropping a link without deleting the corresponding table entries
+/// is a leak. So DPD shouldn't do that.
+/// If a table entry cannot be deleted, then DPD should not allow
+/// that entry's link to be deleted.


I'm having a hard time tracking what is happening in this test, since it seems like a lot of the behaviors we're testing are implicit or handled within macros / test fixtures.

It seems like we are basically relying on table_chaos!() to guarantee that deletion of IPv4 port addrs fails, then we assert that the attempt to remove the IPv4 port addr returns an error. Maybe adding a couple comments inline might help make it more obvious to dendrite noobs like me.

Correct. Haha good point, I was deep in the sauce by the time this test came around. Updated the comment to provide some more context.

taspelund · 2026-10-05T22:06:19Z

+    std::mem::swap(&mut p1.v4_entry, &mut p2.v4_entry);
+    p2.apply_addrs()
+        .await
+        .expect_err("v4 address is already registered under p1");
+    std::mem::swap(&mut p1.v4_entry, &mut p2.v4_entry);
+
+    std::mem::swap(&mut p1.v6_entry, &mut p2.v6_entry);
+    p2.apply_addrs()
+        .await
+        .expect_err("v6 address is already registered under p1");
+    std::mem::swap(&mut p1.v6_entry, &mut p2.v6_entry);
+
+    std::mem::swap(&mut loopback_v4, &mut p2.v4_entry);
+    p2.apply_addrs()
+        .await
+        .expect_err("v4 address is already registered under loopback");
+    std::mem::swap(&mut loopback_v4, &mut p2.v4_entry);
+
+    std::mem::swap(&mut loopback_v6, &mut p2.v6_entry);
+    p2.apply_addrs()
+        .await
+        .expect_err("v6 address is already registered under loopback");
+    std::mem::swap(&mut loopback_v6, &mut p2.v6_entry);


This is really hard to follow without knowing the shape of all the types in play ahead of time. I'm not opposed to using swap instead of assigning values directly, but it's not clear at a glance what swap is occurring at each step or why.

Yeah this is both confusing and ugly. Added a helper function try_steal that we now assert error on.

taspelund · 2026-10-05T22:10:18Z

+        PortSettings {
+            links: HashMap::from([(
+                link_id.to_string(),
+                LinkSettings { params: LINK_CREATE, addrs: Vec::new() },


Worth an impl Default?

Hmm. First reaction is a bit averse, but I'll think on it. LINK_CREATE is just a reflection of what Nils had presumably copy pasted around this module, and I did it just for LOC reduction. I'm not confident enough to say a 100G link without autoneg, fec, or kr is an authoritative default per-se. Is that something you can speak to?

I'm also hesitant to make a default PortSettings because the link_id input should become much more meaningful once we roll out multiple links per port. As it stands, I only feel confident in this as a helper function in tests.

taspelund · 2026-10-05T22:13:15Z

            })
            .map(|()| {
-                self.usage.occupancy -= 1;
+                self.usage.occupancy = self.usage.occupancy.saturating_sub(1);


Is this due to a bug you found, or is this proactive/preventative? just curious

More proactive. It came from a claude review. Previously, underflow was impossible because the asic operations prevented re-creating/deleting an entry. Now, if soft state somehow gets detached from asic state, a clear could succeed and then panic on counter underflow.

Although this is speculative because I don't see how such a detachment would happen. We reset switch tables on dpd init. But the change is easy enough, and I generally prefer safe arithmetic ops anyway.

taspelund · 2026-10-05T22:19:27Z

+    #[error("Address {addr} is already in use by {owner}")]
+    AddrOwnerConflict { addr: IpAddr, owner: std::borrow::Cow<'static, str> },


This variant name is a bit confusing. AddrTagConflict says that tag is what currently owns the address, so how is that any different from AddrOwnerConflict?

It would be nice for the name to be a little more self-describing, i.e. we don't use the term Owner explicitly here and implicitly for Tag at the same time.

Maybe we could call this AddrLinkConflict or something to that effect so it's clear that the conflict stems from the address already existing on another link/loopback rather than being owned by a different entity.

In general, this PR assumes these defintions:

"owner" is an entity on the switch where addresses may be uniquely registered

"tag" is an ID for some external actor requesting operations on the switch.

But yes, this verbiage is confusing.

I was trying to avoid using Link in names to accomodate Loopback as well. However, AddrLinkConflict works here because, if there's a conflict, one of them is necessarily a link.

Update: It seems every possible word in this domain is overloaded, so I'll just pick my favorite. I renamed it to AddrIfaceConflict. I'm thinking to use the word "interface" for the category containing both "link" and "loopback". Any concerns about that?

taspelund · 2026-10-05T22:31:30Z

-    /// IPv4 addresses assigned to this link.
-    /// Each is associated with its creator's ID tag.
-    pub ipv4: BTreeMap<Ipv4Addr, String>,
-    /// IPv6 addresses assigned to this link.
-    pub ipv6: BTreeMap<Ipv6Addr, String>,


This is a nice cleanup. If we could also move ipv6_enabled out, that'd make Link purely focused on Layer1/2, which I think would be a nice place to draw boundaries around the abstraction.

Agreed from a design perspective. Perhaps allow_ddm_traffic could join it. However, I don't see a better spot to put ipv6_enabled right now. Okay to leave as-is in this PR?

Yeah, totally fine for this PR. I was just sharing thoughts

taspelund · 2026-10-05T22:38:48Z

+                Ipv6Addr::is_unicast_link_local(addr).then_some(*addr)
+            },
+        )
    }


Why keep this as a method of Link? It seems like it would be more natural to make this a method of SyncAddrMap.
e.g.

impl SyncAddrMap { pub fn first_link_local(&self, owner: AsicAddrOwner) -> Option<Ipv6Addr> { self.read().unwrap().iter_by_owner(owner).find_map( |(addr, _tag)| { Ipv6Addr::is_unicast_link_local(addr).then_some(*addr) }, ) } }

I also think a better name for the method would be first_link_local() since (afaik) there's nothing preventing multiple from showing up in the addr map, and the name would convey how the IP is selected when there are multiple

I'll implement this and then circle back. I think it will resolve both: #370 (comment)

taspelund · 2026-10-05T22:41:13Z

+    /// Removes addresses of type `A` from asic and link tables.
+    ///
+    /// If `Some(tag)` is provided, only those addresses belonging
+    /// to the named tag are deleted.
+    ///
+    /// Returns [`DpdError::AddrClear`] if any addresses could not be deleted.
+    fn clear_link_addresses<A: IpAddrLike>(
+        &self,
+        switch: &Switch,
+        tag: Option<&str>,
+    ) -> DpdResult<()> {
+        switch.addrs.write().unwrap().try_retain_by_owner::<A>(
+            switch,
+            self.asic_addr_id(),
+            |_addr, entry_tag| tag.is_some_and(|t| t != entry_tag),
+        )
+    }
 }


Similar feedback here. This feels like an awkward place for this method to live now that Link doesn't own IPs anymore

Moved this to SyncAddrMap.

taspelund · 2026-10-05T22:48:45Z

+
+impl crate::link::Link {
+    /// Constructs an address ownership ID for this link.
+    pub fn asic_addr_id(&self) -> AsicAddrOwner {


This method name doesn't match the return type (id != owner). I would probably align them to reduce possible confusion

Related to #374 (comment)

Renamed to pub fn asic_addr_iface(&self) -> AsicAddrIface.

taspelund · 2026-10-05T22:50:27Z

+///
+/// This upholds the following DPD address invariants:
+/// - Unicast addresses have exactly one owner across loopback and all links.
+/// - Such an address is in this soft state IFF it's in an asic table.


What do you mean by "soft state" here? Why make the distinction of "soft"? Just curious if the wording has significance or if it's just a random detail that I happened to notice and latch onto :D

For my language in this PR, DPD is concerned about three layers of state:

What entries exist in actual tofino asic tables. I'm calling this asic state.

What entries exist in tofino sde tables on the scrimlet side. I'm calling this sde state.

What entries exist in rust data structures within dpd. I'm calling this soft state.

Afaict, in an ideal world, we would only have (1) and (3) because the difference between (2) and (3) is mostly language and author. I'm happy changing the keywords here if something makes more sense, but I believe I picked up "soft state" from discussion with Ry.

taspelund · 2026-10-05T22:51:59Z

+/// # Synchronization
+///
+/// Any method with a `switch` parameter will lock asic tables.
+/// Never hold an asic table lock while calling such a method.


This makes it sound like we could be making better use of the type system to ensure callers can't make this mistake. Not needed for this PR, but something to think about while working on dendrite

Yes! When time allows, I would love to revisit the synchronization model used in dpd.

taspelund · 2026-10-05T22:54:09Z

+// ### Design
+//
+// - A single `HashMap<IpAddr, ...>` is sufficient for
+//   correctness and better for simplicity. However, most common
+//   operations are scoped to a single owner. Caching a reverse
+//   `mirror` table avoids a full map scan in those cases.
+// - IPv4 and IPv6 addresses have different tables in the switch, so
+//   we could parallelize better by splitting IPv4 and IPv6 soft state
+//   behind separate locks. I skipped this to avoid additional
+//   complexity, but it's certainly possible if needed.


You might consider iddqd::BiHashMap to centralize this into a single type.
The actual value could be a simple struct (or I think even a tuple) wrapper for the address/owner, with addr/owner both acting as keys.

Yeah, you would get all your invariants upheld by the type system basically for free with an iddqd::BiHashMap

Reading further down the file, it seems like AddrSpec would be an excellent candidate for this: iddqd::BiHashMap<AddrSpec>

Wow yes, that looks perfect. Lol now I remember the podcast episode on this. Thx for the suggestion.

Update: iddqd might not be a great fit.

Preface: As of writing, I'm thinking to rename "owner" to "interface". Ik it's overloaded, but in this case I'm using "interface" to mean either (PortId, LinkId) or Loopback.

Here's what the current data structure enforces:

An address belongs to one interface.

Each interface may have many addresses.

A client can iterate the addrs on an interface without iterating all addrs on the switch. And dropshot pagination mandates iterating stably-ordered segments.

I think the one to many relation between interface and address is what makes BiHashMap a poor fit, and stable iteration wants the addresses in a BTree* collection at some point as well. Lmk if I'm missing something, but I believe two HashMaps here gets us something a bijective map cannot.

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

+    /// Returns err if the provided tag or owner do not match
+    /// the current spec for this address.
+    ///
+    /// Do not mutate an entry if this fails.
+    fn compare(
+        addr: IpAddr,
+        spec: &AddrSpec,
+        owner: AsicAddrOwner,
+        tag_filter: Option<&str>,
+    ) -> DpdResult<()> {
+        if AddrOwner::from(owner) != spec.owner {


This seems like it could be an impl PartialEq for AddrSpec?

compare returns a unique error identifying the cause of inequality. I think PartialEq could say whether to mutate an entry, but I don't think it would easily say why.

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

+pub enum AsicAddrOwner {
+    Loopback,
+    Link { port_id: PortId, link_id: LinkId, asic_id: AsicId },
+}


Is AsicId not relevant for Loopbacks? If it is, maybe this would be better as a struct?
e.g.

pub struct AsicAddrOwner { asic_id: AsicId, owner: AddrOwner, }

Correct. Afaict asic_id is just the tofino port number. There's no asic id for loopback because there is no physical port.

That comes from pre-existing table management fns:

Loopback:

dendrite/dpd/src/table/port_ip.rs

Lines 94 to 98 in 5f37818

pub fn loopback_ipv4_add(s: &Switch, ipv4: Ipv4Addr) -> DpdResult<()> {

let claim_key = Ipv4MatchKey {

dst_addr: ipv4,

in_port: MatchMask { val: 0u16.into(), mask: 0u16.into() },

};

Link:

dendrite/dpd/src/table/port_ip.rs

Lines 168 to 169 in 5f37818

fn ipv4_add_work(s: &Switch, port: u16, ipv4: Ipv4Addr) -> DpdResult<()> {

let (claim_key, drop_key) = match_keys_ipv4(ipv4, port);

taspelund

This is good work overall, thanks @cfzimmerman!

I do have a bit of feedback that I think would be good to address before merge though.

Particularly, some of the naming I think could use some work (e.g. AddrOwnerConflict and Link.asic_addr_id()) and I think AddrMap would be greatly simplified by using iddqd::BiHashMap<AddrSpec>

- Tagged port_settings_apply can delete addresses from another tag. - Tagged port_settings_get returns addresses belonging to other tags. - Tagged port_settings_clear can delete addresses from another tag. Not changing this, just observing it. - Tagged port_settings_apply steals tagged addresses in a way create_ipv*_addr does not.

Scope link address CRUD to user-provided tags as advertised in the dropshot API. This avoids some tempting breaking changes in favor of just fixing what is broken for now. If we want breaking changes, the two highest priority are (1) removing tags from port_settings_clear, which does not use them and (2) adding tags to link_ipv*_delete.

- Reproduce port settings reconciler failure from txn test. - Reproduce other reconciler wedge cases. - Test isolation between loopback and link addresses. - Reproduce link deletion table entry leak. - Test isolation between link and loopback addresses.

- Use itempotent set/clear in table modifications. - Only remove an address from soft state once the asic operation succeeds. - Track IP address ownership in a global table. - Use generics to deduplicate IPv4/IPv6 code.

cfzimmerman

🤖 This message was written by CLU

Fresh pass at 14a8ff8. The PR now targets main and carries #370's commits.

The core of the change holds up. table_entry_set/clear plus the AddrMap guard replaces the SDE's accidental Exists check with an explicit one. The two maps stay paired on every mutation path I traced, including the refcounted tags, the restore on a failed clear_asic, and the link-local slot. Lock order is links → Link → AddrMap → Table everywhere. The tag-selector commit (OptTag) is mechanically equivalent to the Option<&str> it replaces.

Two things should change before merge:

The new address validation rejects ::, which wicketd's uplink preflight sends for addrconf uplinks.
Link address pagination drops per-tag rows. This is new, because this PR is what lets one address carry several tags.

The rest are test-strength and wording.

On the PR body. "Running only the first commit yields the following test failures" was written when this PR sat on #370. The first commit in the range is now #370's 2713924 ("Add repro tests for address isolation issues"). The tests listed there (link_init_recovers, loopback_cannot_steal_addrs, ...) arrive in a07f1b1, the third. The body also does not mention several API-visible behaviour changes a reviewer or release note would want:

port_settings_apply, link_ipv*_create and loopback_ipv*_create now 400 on unspecified, loopback, multicast, broadcast and IPv4-mapped addresses, and port_settings_apply also 400s on link-locals.
A second link-local on a link is a 409.
One address can carry several tags.
The list endpoints return one row per (address, tag).

Checked and left alone. I did not re-raise these. The read-path expect/assert! under panic = "abort" is a deliberate choice, and I found no mutation path that desyncs the maps. The add-before-clear ordering across links of one port is latent, since every consumer uses one link per port. One interface per IP is stricter than main for IPv4: correct, and already discussed. tfportd's softnpu simport adds before it deletes, so a changed link-local would 409 for one loop period. Link-locals are MAC-derived and never change, so I left that out.

Second opinion: the design skeptic

This PR is drifting toward the over-building ditch, but only in one place.

The set/clear conversion and the global AddrMap remove a concept: "the SDE's Exists is what stops two interfaces sharing an address" was an invariant nobody could see, and v4 never had it at all. Refcounted tags were asked for by a reviewer with a concrete operator scenario (swadm vs sled-agent). Both earn their size.

RouterIfaceIpAddr is a different project. It is input validation across three endpoint families. It changes what the API accepts without a version bump, and it is the one piece of the PR that breaks a consumer today. What #342 actually needs from validation is narrow: keep link-locals out of port_settings_*. TableIp already classifies link-local vs everything else. My suggestion is to scope this PR's validation to that one rule. The full RouterIfaceIpAddr predicate can then land alongside the wicketd change that stops sending ::, which is also where omicron could start sharing the type, as its doc comment hopes.

The same lens applies to pagination. The simplest correct fix needs no page-token change: never end a page between two tags of one address. Truncate to whole addresses, so a page may come back slightly short. A short page is legal in dropshot, which still emits a token. That removes the per-tag cursor, and its dead tag_bounds branch, instead of adding an API version to carry it.

Findings: 2 demonstrable bug, 1 unclear assumption, 2 test gap, 1 style/convention, 2 nit

cfzimmerman · 2026-11-07T01:15:44Z

+        for addr in l.addrs.iter().copied().map(RouterIfaceIpAddr::try_new) {
+            addrs.entry(addr?).or_default().insert(tag.to_string());
+        }


🤖 This message was written by CLU

Category: Demonstrable bug

RouterIfaceIpAddr::try_new rejects the unspecified address (common/src/addr.rs:124), and wicketd's uplink preflight sends exactly that for addrconf uplinks.

The path in omicron (checked at b140914b7):

wicketd/src/preflight_check/uplink.rs:856, in build_port_settings, maps every uplink address through ip_squashing_addrconf_to_unspecified().

That turns UplinkAddress::AddrConf into :: (sled-agent/types/versions/src/impls/early_networking.rs:216-219).

The result goes to port_settings_apply(port, Some("omicron"), ..) at uplink.rs:204, and any error there becomes UplinkPreflightTerminalError::ConfigurePort.

Failure scenario: an operator config with a BGP-unnumbered (addrconf) uplink. After this PR, the preflight fails terminally with a 400 Conversion failed to RouterIfaceIpAddrErr: Unspecified(::). On main, :: was accepted. It programs two dead claim rows, but the preflight passed.

The steady-state sled-agent reconciler is unaffected, because it maps AddrConf => None (port_reconciler.rs ~L394). So this is wicketd only, which makes it easy to miss in a racklette run.

Either:

tolerate :: here (skip it, as a claim of nothing), or

land the wicketd change first and gate this rejection on it.

cfzimmerman · 2026-11-07T01:15:44Z

+        let (addr_bounds, tag_bounds) = if let Some((addr, tag)) = last_entry {
+            start_addr = Some(addr);
+            let addr = (Bound::Excluded(addr), Bound::Unbounded);
+            let tag = (Bound::Excluded(tag), Bound::Unbounded);
+            (addr, tag)
        } else {
-            Err(DpdError::NoSuchAddress {
-                port_id: link.port_id,
-                link_id: link.link_id,
-                address: address.into(),
-            })
-        }
-    }
+            let addr = (Bound::Unbounded, Bound::Unbounded);
+            let tag = (Bound::Unbounded, Bound::Unbounded);
+            (addr, tag)
+        };

-    /// Delete one IPv6 address on the specified link.
-    pub fn delete_ipv6_address(
-        &self,
-        port_id: PortId,
-        link_id: LinkId,
-        address: Ipv6Addr,
-    ) -> DpdResult<()> {
-        self.link_update(port_id, link_id, |link| {
-            self.delete_ipv6_address_locked(link, address)
+        self.link_fetch(port_id, link_id, |link| {
+            self.addrs
+                .get()
+                .read()
+                .unwrap()
+                .iface_addr_range(link.asic_addr_iface(), addr_bounds)
+                .flat_map(move |(addr, tags)| {
+                    let range = if start_addr.is_some_and(|a| a == addr) {
+                        tag_bounds
+                    } else {
+                        all_tags
+                    };
+                    tags.range::<str, _>(range).map(move |t| (addr, t.as_str()))


🤖 This message was written by CLU

Category: Demonstrable bug

Pagination drops entries once an address carries more than one tag. The per-tag resume branch is also unreachable.

The address bound is Bound::Excluded(addr) (L1092), so iface_addr_range never yields the cursor address again. start_addr.is_some_and(|a| a == addr) at L1108 is never true, and tag_bounds is dead.

Failure scenario: address A has tags {chaos1, chaos2}.

link_ipv4_list(limit = 1) returns page 1 as [(A, chaos1)] with token A.

Page 2 starts strictly after A, so (A, chaos2) is never returned.

link_ipv4_list_stream therefore loses rows silently.

On main, each address had exactly one entry, so the address-only token was exact. This regression arrives with the multi-tag change.

Impact is limited:

It takes more than one page.

Only the extra tags of an address go missing, so address-only readers (mg-lower, tfportd simport) are unaffected.

Tag-aware readers such as swadm are affected.

The TODO at api_server.rs:1133 is not sufficient on its own. Carrying the tag in the token also requires Included(addr) here. Switching to Included while the token still carries "" would instead re-emit every tag of A.

A fix that needs no token change: end each page on an address boundary, i.e. truncate to whole addresses and allow a short page.

The test that would catch this is commented out in basic_multiple_tag_crud (chaos_tests/port_settings.rs:1440-1462). fetch_all_addrs reads a single page, so nothing else exercises paging.

cfzimmerman · 2026-11-07T01:15:44Z

+    fn addr_set(
        ctx: &mut Context<'_>,
        link: &mut Link,
        rb: &mut Rollback,
-        addr: Ipv4Addr,
+        addr: TableIp,
+        tag: String,
    ) -> DpdResult<()> {
-        trace!(ctx.log, "ipv4 add {addr}");
+        trace!(ctx.log, "ip add ({addr:?}: {tag})");
        // Create address on ASIC first.
-        let entry =
-            Ipv4Entry { tag: ctx.tag.clone().unwrap_or("".into()), addr };
        let switch = ctx.switch;
-        switch.create_ipv4_address_locked(link, entry)?;
+        switch.set_ip_address_locked(link, addr, tag.clone())?;

        let link_id = link.link_id;
        rb.wind(move |ctx: &mut Context<'_>| -> DpdResult<()> {
            let switch = ctx.switch;
            let link_lock = ctx.link(link_id)?;
-            let mut link = link_lock.lock().unwrap();
-            switch.delete_ipv4_address_locked(&mut link, addr)
-        });
-        Ok(())
-    }
-
-    fn addr_del_v4(
-        ctx: &mut Context<'_>,
-        link: &mut Link,
-        rb: &mut Rollback,
-        addr: Ipv4Addr,
-    ) -> DpdResult<()> {
-        trace!(ctx.log, "ipv4 del {addr}");
-        let entry =
-            Ipv4Entry { tag: ctx.tag.clone().unwrap_or("".into()), addr };
-        let switch = ctx.switch;
-        let link_id = link.link_id;
-        switch.delete_ipv4_address_locked(link, addr)?;
-
-        rb.wind(move |ctx: &mut Context<'_>| -> DpdResult<()> {
-            let switch = ctx.switch;
-            let link_lock = ctx.link(link_id)?;
-            let mut link = link_lock.lock().unwrap();
-            switch.create_ipv4_address_locked(&mut link, entry)
-        });
-        Ok(())
-    }
-
-    fn addr_add_v6(
-        ctx: &mut Context<'_>,
-        link: &mut Link,
-        rb: &mut Rollback,
-        addr: Ipv6Addr,
-    ) -> DpdResult<()> {
-        trace!(ctx.log, "ipv6 add {addr}");
-        // Create address on ASIC first.
-        let entry =
-            Ipv6Entry { tag: ctx.tag.clone().unwrap_or("".into()), addr };
-        let switch = ctx.switch;
-        let link_id = link.link_id;
-        switch.create_ipv6_address_locked(link, entry)?;
-
-        rb.wind(move |ctx: &mut Context<'_>| -> DpdResult<()> {
-            let switch = ctx.switch;
-            let link_lock = ctx.link(link_id)?;
-            let mut link = link_lock.lock().unwrap();
-            switch.delete_ipv6_address_locked(&mut link, addr)
+            let link = link_lock.lock().unwrap();
+            switch.addrs.clear_addr(
+                switch,
+                link.asic_addr_iface(),
+                addr,
+                &OptTag::exact(&tag),
+            )?;
+            Ok(())
        });
        Ok(())
    }

-    fn addr_del_v6(
+    fn addr_clear(
        ctx: &mut Context<'_>,
        link: &mut Link,
        rb: &mut Rollback,
-        addr: Ipv6Addr,
+        addr: TableIp,
+        tag: String,
    ) -> DpdResult<()> {
-        trace!(ctx.log, "ipv6 del {addr}");
+        trace!(ctx.log, "ip clear ({addr}: {tag})");
        let switch = ctx.switch;
        let link_id = link.link_id;
-        switch.delete_ipv6_address_locked(link, addr)?;
+        switch.addrs.clear_addr(
+            switch,
+            link.asic_addr_iface(),
+            addr,
+            &OptTag::exact(&tag),
+        )?;

        rb.wind(move |ctx: &mut Context<'_>| -> DpdResult<()> {
-            let entry =
-                Ipv6Entry { tag: ctx.tag.clone().unwrap_or("".into()), addr };
            let switch = ctx.switch;
            let link_lock = ctx.link(link_id)?;
            let mut link = link_lock.lock().unwrap();
-            switch.create_ipv6_address_locked(&mut link, entry)
+            switch.set_ip_address_locked(&mut link, addr, tag)?;
+            Ok(())
        });
        Ok(())
    }


🤖 This message was written by CLU

Category: Unclear assumption

addr_set and addr_clear each wind an inverse undo unconditionally:

addr_set discards the bool from set_ip_address_locked (L540).

addr_clear discards the bool from clear_addr (L568).

If either forward op were a no-op, the unwind would remove a tag claim this transaction never added, or add one that never existed. Removing the last tag would also clear the ASIC rows.

This is unreachable today, for reasons that are not stated here:

modify_link only touches (addr, tag) pairs that differ from addrs_before.

addrs_before is read from AddrMap under the same switch.links guard.

add_link starts from a slot that delete_link purged.

Winding only on Ok(true) makes it locally obvious and costs two ifs.

cfzimmerman · 2026-11-07T01:15:44Z

+    self::try_steal(&mut p2, p1.v4_entry.addr.into(), TAG1.to_string())
+        .await
+        .expect_err("v4 address is already registered under p1");
+
+    self::try_steal(&mut p2, p1.v6_entry.addr.into(), TAG1.to_string())
+        .await
+        .expect_err("v6 address is already registered under p1");
+
+    self::try_steal(&mut p2, loopback_v4.addr.into(), TAG1.to_string())
+        .await
+        .expect_err("v4 address is already registered under loopback");
+
+    self::try_steal(&mut p2, loopback_v6.addr.into(), TAG1.to_string())
+        .await
+        .expect_err("v6 address is already registered under loopback");
+
+    client
+        .loopback_ipv4_create(&p1.v4_entry)
+        .await
+        .expect_err("v4 address is already registered under p1");
+    client
+        .loopback_ipv6_create(&p1.v6_entry)
+        .await
+        .expect_err("v6 address is already registered under p1");


🤖 This message was written by CLU

Category: Test gap

All six theft attempts use a bare expect_err, so any failure passes, including a 400, a 500, or a chaos Synthetic.

The v4 cases matter most. AddrMap is the only thing that detects a v4 cross-interface collision, because the v4 drop keys differ by port and neither the SDE nor the chaos table notices the duplicate. These should assert StatusCode::CONFLICT, and ideally match the AddrIfaceConflict message. Every address conflict is a 409, so the status alone does not say which one fired.

The same pattern appears in:

link_addrs_are_isolated (L1136, L1141)

loopback_addrs_are_isolated (L1167-1173)

port_settings_disallows_link_local (L1646 should be 400, L1658 should be 409)

cfzimmerman · 2026-11-07T01:15:44Z

+    client.port_settings_clear(&cxt.port_id, None).await?;
+    assert!(client.tfport_data().await?.is_empty());


🤖 This message was written by CLU

Category: Test gap

This assertion races the reconciler.

port_settings_clear only marks the link delete_me.

remove_link clears router addresses, not the link-local.

The link, and its link-local, leave the map only when the async reconciler runs LinkMap::delete_link.

port_tfport_data (link.rs:694-710) does not filter delete_me.

So tfport_data().is_empty() holds only if the reconciler wins. Other tests in this file wrap the same wait in retry::retry_op. I did not reproduce a failure; the race is from reading the code.

cfzimmerman · 2026-11-07T01:15:44Z

+    /// Return Ok(true) if this cleared any config from the switch.
+    /// Returns Ok(false) if this entry didn't exist.


🤖 This message was written by CLU

Category: Nit

"Return Ok(true) if this cleared any config from the switch" doesn't match L329-331. Those lines return Ok(true) when one of several tags was removed and the ASIC was not touched. Something like "Ok(true) if this tag's claim was removed" matches what callers (delete_ip_address) actually rely on.

cfzimmerman · 2026-11-07T01:15:44Z

+impl convert::From<common::addr::RouterIfaceIpAddrErr> for DpdError {
+    fn from(err: common::addr::RouterIfaceIpAddrErr) -> Self {
+        DpdError::Invalid(format!(
+            "Conversion failed to RouterIfaceIpAddrErr: {err:?}"
+        ))
+    }
+}


🤖 This message was written by CLU

Category: Nit

Two problems with the message:

It names the error type rather than the target type.

It Debug-formats the error.

Operators will see this one, e.g. via wicketd: Invalid("Conversion failed to RouterIfaceIpAddrErr: Unspecified(::)"). format!("address not permitted on a switch interface: {err}") would use the thiserror text that already exists.

cfzimmerman · 2026-11-07T01:15:44Z

+    //
+    // TODO::cory: Enforce port RouterIfaceIpAddr constraint
+    // across the port_settings_* api.


🤖 This message was written by CLU

Category: Style/convention

This TODO::cory reads as though the constraint is not enforced yet, but try_from_settings (L150) already enforces it.

The repo's convention is also TODO-<kind> (TODO-cleanup, TODO-robustness, TODO-correctness), not a personal marker. The same applies to api_server.rs:1133 and :1213, and to the two in chaos_tests/port_settings.rs (L1440, L1465).

To be breadcrumbed

cfzimmerman marked this pull request as ready for review September 14, 2026 18:55

cfzimmerman changed the base branch from main to cory/ipwars2 September 14, 2026 18:55

cfzimmerman marked this pull request as draft September 14, 2026 18:55

cfzimmerman force-pushed the cory/eventual-reconciliation branch from 1d73d15 to 9275cc4 Compare September 15, 2026 13:13

cfzimmerman linked an issue Sep 15, 2026 that may be closed by this pull request

DPD partial failures can be fatal #354

Open

cfzimmerman force-pushed the cory/eventual-reconciliation branch 4 times, most recently from 2c46cfe to c0d84b5 Compare September 16, 2026 16:55

cfzimmerman changed the title ~~Fix indefinite port_settings_* failure cases~~ Fix indefinite port_settings_* failures Sep 17, 2026

cfzimmerman mentioned this pull request Sep 17, 2026

Respect address tags in port_settings_* #370

Open

cfzimmerman force-pushed the cory/eventual-reconciliation branch 5 times, most recently from cddb1fc to 05ff476 Compare September 17, 2026 23:33

cfzimmerman commented Sep 17, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

cfzimmerman commented Sep 18, 2026

View reviewed changes

Comment thread dpd/src/addr.rs Resolved

cfzimmerman force-pushed the cory/eventual-reconciliation branch 2 times, most recently from dd8ad41 to f186051 Compare September 24, 2026 22:57

cfzimmerman changed the base branch from cory/ipwars2 to cory/link-state September 25, 2026 23:49

cfzimmerman added this pull request to stack #379 September 25, 2026 23:49

cfzimmerman force-pushed the cory/eventual-reconciliation branch 2 times, most recently from da167a2 to fd532cb Compare September 28, 2026 15:15

cfzimmerman force-pushed the cory/link-state branch from e57f270 to bd0c5d6 Compare September 28, 2026 16:10

cfzimmerman force-pushed the cory/eventual-reconciliation branch from fd532cb to 837482e Compare September 28, 2026 16:12

cfzimmerman force-pushed the cory/link-state branch from bd0c5d6 to e74d84c Compare September 28, 2026 16:21

cfzimmerman force-pushed the cory/eventual-reconciliation branch 2 times, most recently from 38e1252 to 5b7bdc5 Compare September 28, 2026 18:27

taspelund reviewed Oct 5, 2026

View reviewed changes

taspelund mentioned this pull request Oct 5, 2026

Update dpd-api's use of address tags #383

Open

cfzimmerman mentioned this pull request Oct 23, 2026

[route] per-endpoint routing tables (RFD 662) #386

Open

cfzimmerman mentioned this pull request Nov 2, 2026

[dpd,dpd-api,dpd-types] make external multicast constraints self-enforcing #396

Open

cfzimmerman added 4 commits November 6, 2026 15:26

Fix failures isolated in tests

0b49cf9

- Use itempotent set/clear in table modifications. - Only remove an address from soft state once the asic operation succeeds. - Track IP address ownership in a global table. - Use generics to deduplicate IPv4/IPv6 code.

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 changed the base branch from cory/ipwars2 to main November 6, 2026 15:34

cfzimmerman commented Nov 7, 2026

View reviewed changes

Omnibus PR iteration

0edf400

To be breadcrumbed

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

		#[error("Address {addr} is already in use by {owner}")]
		AddrOwnerConflict { addr: IpAddr, owner: std::borrow::Cow<'static, str> },

	pub fn loopback_ipv4_add(s: &Switch, ipv4: Ipv4Addr) -> DpdResult<()> {
	let claim_key = Ipv4MatchKey {
	dst_addr: ipv4,
	in_port: MatchMask { val: 0u16.into(), mask: 0u16.into() },
	};

	fn ipv4_add_work(s: &Switch, port: u16, ipv4: Ipv4Addr) -> DpdResult<()> {
	let (claim_key, drop_key) = match_keys_ipv4(ipv4, port);

		client.port_settings_clear(&cxt.port_id, None).await?;
		assert!(client.tfport_data().await?.is_empty());

		/// Return Ok(true) if this cleared any config from the switch.
		/// Returns Ok(false) if this entry didn't exist.

Conversation

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

Uh oh!

Prefix parlance

Summary

Fixes

Uh oh!

cfzimmerman Sep 17, 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!

This comment was marked as resolved.

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!

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!

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!

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!

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!

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!

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!

cfzimmerman commented Sep 14, 2026 •

edited

Loading

cfzimmerman Sep 17, 2026 •

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

cfzimmerman Oct 6, 2026 •

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