port_settings_apply clobbers link-local addresses added via link_ipv6_create

When port_settings_apply is run, it grabs a copy of dpd's internal state for each port/link, diffs it against the state requested by the API client, and then reconciles the difference.

The problem we have here is that omicron is the main (practically only) consumer of port_settings_apply, and it doesn't include any link-local IPv6 addresses in its request... since these are handled by tfportd and illumos.

tfportd learns about IPv6 link-local addresses in illumos from its reconciler, and synchronizes them into dpd via link_ipv6_create.

Both port_settings_apply and link_ipv6_create modify the same internal state: Link.ipv6

#[derive(Debug)]
pub struct Link {
    pub port_id: PortId,
    pub link_id: LinkId,
    pub port_hdl: PortHdl,
    pub asic_port_id: AsicId,
    pub updated: i64,
    pub presence: bool,
    pub ipv6_enabled: bool,
    pub tx_eq: Option<TxEq>,
    pub fsm_state: asic::PortFsmState,
    pub link_state: LinkState,
    pub media: PortMedia,

    /// A list of IPv4 addresses assigned to this link.
    pub ipv4: BTreeSet<Ipv4Entry>,

    /// A list of IPv6 addresses assigned to this link.
    pub ipv6: BTreeSet<Ipv6Entry>,                    <<<<<<<<<<<

    pub linkup_tracker: LinkUpTracker,
    pub autoneg_tracker: AutonegTracker,

    /// Configuration requested by the user/sled-agent.
    pub config: LinkConfig,

    /// State that actually exists in the ASIC layer.
    pub(crate) plumbed: LinkPlumbed,
}

When port_settings_apply is run, the internal dpd state includes all items in Link.ipv6 (including the link-locals added by tfportd) and thus the diff always indicates that the link-local is old state that should be removed.

This results in the omicron reconciler (via port_settings_apply) and the tfport reconciler (via link_ipv6_create) battling each other and the IPv6 link-local addresses being removed from the ASIC anytime omicron's reconciler runs after the tfport reconciler... which means that periodically, the ASIC will stop punting traffic destined for that link-local address. This would affect NDP, BGP unnumbered, etc. and could potentially cause service disruptions if the blackholing were to last long enough.

@cfzimmerman

When IPv6 is enabled on a link, switch port settings apply should not be clearing out link locals from the ASIC table. If this is happening we should capture it with a regression test and fix. cc: @cfzimmerman

I spent some time with claude on a potential fix for this that involved making the "owner" tags actually functional for addresses (effectively a BTreeSet acting as a refcount for each addr), but I didn't move forward with it because I wasn't a big fan of the implementation.

That said, I do have some test cases for this that could be used as a red/green indicator for a fix. Here's the branch I was working from:
https://github.com/oxidecomputer/dendrite/tree/trey/multi-owner-ip

Thanks for the info. Looking into it!

~~Still need to validate beyond just cargo test, but this branch reproduces the issue and contains a candidate fix: #370~~ See below

@jgallagher

This issue motivates a broader conversation about resource ownership in DPD. I'd particularly appreciate design input from @jgallagher for sled agent and @rcgoodfellow for networking.

Scope

DPD uses "tag" strings to track resources owned by different controllers. So far I see distinct tag domains for addresses, routes, multicast, and loopback. There are probably more. For this issue, I'm only considering the ip address registration tags as seen in port_settings_* functions.

Summary

The current address tagging implementation is missing enough pieces to feel incomplete and motivate this issue. For example, port_settings_apply can delete addresses from other tags, and port_settings_get can return addresses from other tags. I'm considering just removing address registration tags.

Detail

I see various ways to resolve the issue:

Fix the code just enough to make the current API work for current usage.
Redesign resource ownership in DPD to make tags a first class concept (at least for link settings).
Remove address tags altogether from DPD. No concept of address ownership.
Leave the existing code as-is.
[Edit] Remove address tags. Separate the link local and routable addresses on a link, and manage them via separate APIs. port_settings_* only touches routable addresses.
Open to suggestions.

(4) Feels undesirable because there are bugs/footguns in the current code that are easily demonstrated in tests: 9df3b85

(2) I explored this, but it's a giant project. Trey mentions the same above. I think this is not a reasonable solution to the open issue, but it is perhaps worth a longer term conversation.

(1) Here's a PR for this: #370. However it still has gaps. For example, the API presents address tags as a top level concept in port_settings_*, but they only affect link addresses. A tagged port_settings_apply can still delete a link that was created under another tag. Concretely, if client_a creates a link and client_b registers all the addresses on it, either client can delete the link and all addresses via tagged port_settings_apply or port_settings_clear. Doing much else leads into (2). This does not seem like an intuitive API for address tagging. Keeping this API requires controllers to have at least some awareness of each other.

(3) This is now my favorite approach. Afaik sled-agent, tfportd, wicket, and swadm are the only real address tag owners in DPD. Maghemite uses route and loopback tags, and multicast tags are totally separate. In both (1) and (4), tfportd and sled-agent need to have some awareness of eachother. And since tfportd and sled-agent register addresses from disjoint domains (link local vs routable), the fix in sled agent is to be aware of tfportd link local addresses and vice versa. I believe tfportd is already compatible with this, and sled-agent has some awareness of the issue. Although I think the implementation is incomplete: https://github.com/oxidecomputer/omicron/blob/7ff4a2103d771b3f9c66a3778def67c61d17f780/sled-agent/scrimlet-reconcilers/src/dpd\_reconciler/port\_reconciler.rs#L466-L474

Address tagging implies DPD hiding some state from the controller based on tag, which adds conceptual complexity and bug surface area. Both controllers are internal apps with at least some developer awareness of the other. I'm inclined to take the current sled-agent workaround, fix it, call it necessary, and just remove address tags from the DPD API. A caller of port_settings_* sees everything and is responsible for inferring by context what it wants to reconcile to. Otherwise we're adding complexity to one of DPD's most complex modules so that sled-agent and tfportd don't need to be aware of eachother's easily-detectable address registrations.

I believe tfportd is already compatible with this, and sled-agent has some awareness of the issue. Although I think the implementation is incomplete:

Can you expand on what sled-agent should be doing instead? I'm also curious if this is something I broke in moving reconciliation out of Nexus and into sled-agent. The specific bit you linked was introduced to Nexus (the sync_switch_configuration bg task, specifically) in oxidecomputer/omicron#9570, and I think I copied that faithfully over. Were we assuming that dendrite behaved differently somehow? (Maybe that it also didn't consider link-local addrs when computing the diff on its side?)

In both (1) and (4), tfportd and sled-agent need to have some awareness of eachother. And since tfportd and sled-agent register addresses from disjoint domains (link local vs routable), the fix in sled agent is to be aware of tfportd link local addresses and vice versa.

I'm not entirely following this - in the proposal for (3), do tfportd and sled-agent also have to be aware of each other?

A caller of port_settings_* sees everything and is responsible for inferring by context what it wants to reconcile to.

Sorry if this is missing the whole point, but: If the caller only knows what addresses should exist (in its "domain" for lack of a better word), and port_settings_get() returns everything, how does the caller know which ones are from someone else and need to be carried forward, and which ones are from an older instance of itself and need to be omitted?

Can you expand on what sled-agent should be doing instead?

My wording was kind of loose. I don't think you broke anything. Sled agent's code is reasonable if you assume dpd scopes addresses in port_settings_apply only to your tag. But that's not currently the case. And this filter is only needed because port_settings_get doesn't respect tags, so you're seeing addresses from tfportd: https://github.com/oxidecomputer/omicron/blob/7ff4a2103d771b3f9c66a3778def67c61d17f780/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler.rs#L466-L474

When sled agent sends port_settings_apply, it currently wipes the link local address from the link. tfportd then (re)adds the address, and there's no reconciliation war because tfportd only reads/writes the single link local address. Because sled-agent filters out link local addresses, the addition doesn't cause another port_settings_apply. So it converges to the correct state, but there's no link local address for a tfportd reconciler period.

If tags disappeared, then sled agent would take responsibility for the entire state of the link when it writes port_settings_apply. It would know that tfportd adds link local addresses and that it needs to include those in the applied settings. This increases the declared scope of the sled-agent reconciler, but it's the de-facto contract right now anyway.

One implementation option in sled-agent could be removing link locals after port_settings_get, diffing against target settings, and then adding the link locals back in before calling port_settings_apply. Read/Modify/Write race conditions would converge due to tfportd's reconciler as they do now.

do tfportd and sled-agent also have to be aware of each other?

Yes. In (3), tfportd needs to know that a link may contain routable addresses registered by other services. This is already handled. More importantly, sled-agent needs to know that tfportd's link local addresses from port_settings_get should be included in port_settings_apply. This is awkward coupling, but my suggestion is that it's a lesser evil than the complexity of a complete link state namespacing regime in dpd. (That conclusion is open to discussion)

Sorry if this is missing the whole point, but: If the caller only knows what addresses should exist (in its "domain" for lack of a better word), and port_settings_get() returns everything, how does the caller know which ones are from someone else and need to be carried forward, and which ones are from an older instance of itself and need to be omitted?

Yes, this question is exactly the point. It's only possible under the condition that tfportd exclusively manages link local addresses, and sled-agent exclusively manages routable addresses. If we assert this, then sled-agent and tfportd can diff their own changes within their respective domains, and the complexity of tags can be avoided. If not, then that motivates the complexity and effort of (1) or (2).

do tfportd and sled-agent also have to be aware of each other?

Yes. In (3), tfportd needs to know that a link may contain routable addresses registered by other services. This is already handled. More importantly, sled-agent needs to know that tfportd's link local addresses from port_settings_get should be included in port_settings_apply. This is awkward coupling, but my suggestion is that it's a lesser evil than the complexity of a complete link state namespacing regime in dpd. (That conclusion is open to discussion)

Ahhh ok thank you, I think I'm completely following now. Is this right?

Both tfportd and sled-agent manage port settings via port_settings_get -> diff locally against desired config -> port_settings_apply.
port_settings_get returns the union of addresses managed by both clients.
Each client is responsible for ignoring the other client's addresses when computing the diff, but including them when calling port_settings_apply.

I do think item 3 is pretty gross and somewhat fragile for a few reasons (some of this you already noted explicitly or hinted at, but just restating for clarity):

Correct behavior now depends on cross-service and cross-repo coordination. This is mostly theoretical, I think, because these bits don't seem likely to change all that frequently, but it's sort of the smelliest bit of the proposal to me. We need comments in both places that are referring to the other.
It might be hard to notice if we break one? I guess we could add "simulate what tfportd does" in some or all of the sled-agent tests, and vice versa, and that's probably worth doing if we go this route. If we managed to break one side anyway, we still might not notice due to the eventual consistency thing we have (which is the state we're in today, I gather?).
The whole "get -> diff -> apply" reconciliation pattern is only safe from TOCTOU races if it's only a single client doing it; as you noted, with two clients we can race. Maybe this is fine-ish since, also as you noted, it should resolve itself on the next reconciliation pass, but it's pretty unsatisfying.

All that said: I agree that going this route is a pretty manageable piece of work, and I certainly believe believe that any of the other proposals that require dpd rework are both more work and (presumably) riskier. But in terms of this bug, it seems like it only makes it less likely, but doesn't actually fix it, right? (Because of the TOCTOU race between the services. The window on that race might be small in practice most of the time, so maybe it makes it a lot less likely?) I think if we go this route, we should keep an issue open and consider this tech debt; if we ever need to do dpd rework for some more urgent reason, it would be nice to clean this up.

A naive question: if we wanted to give sled-agent and tfport APIs that only present their respective domains, tagging is one option. Would it be any easier from a dpd point of view if we offered separate endpoints entirely? There's a bunch of stuff in port_settings_apply beyond just addresses; I assume sled-agent is responsible for the other settings (tx_eq, FEC, etc.) and tfportd only adds/removes addresses? So either something like:

sled-agent keeps using port_settings_{get,apply} as they are today, but part of the API contract is that the addresses only include routable addrs
tfportd uses some new API that only allows for adding/removing link-local addrs to ports

or:

one endpoint pair for managing non-address link settings (used by sled-agent)
one endpoint pair for managing routeable addrs on a port (used by sled-agent)
one endpoint pair for managing link local addrs on a port (used by tfportd)

Sweet, we're basically on the same page. Thank you for the input. I will think about this a bit more and follow up later with a full response. One quick clarification in the meantime, though:

tfportd uses a bespoke get function from dpd, so it doesn't see addresses registered by sled-agent. It only diffs the optional link local address present on the link. sled-agent see the link local address registered by tfportd, and tfportd sees when that link local address is inadvertently deleted by sled-agent.

tfportd:

dendrite/tfportd/src/ports.rs

Lines 110 to 117 in 2475028

    
           // Fetch all links that `dpd` knows about. 
        
           let dpd_data = g 
        
               .client 
        
               .tfport_data() 
        
               .await 
        
               .context("failed to list all links")? 
        
               .into_inner() 
        
               .into_iter();

dpd:

dendrite/dpd/src/link.rs

Lines 286 to 297 in 2475028

    
           impl From<&Link> for TfportData { 
        
               fn from(m: &Link) -> Self { 
        
                   Self { 
        
                       port_id: m.port_id, 
        
                       link_id: m.link_id, 
        
                       asic_id: m.asic_port_id, 
        
                       mac: m.config.mac, 
        
                       ipv6_enabled: m.ipv6_enabled, 
        
                       link_local: m.link_local(), 
        
                   } 
        
               } 
        
           }

Your response gives an interesting (5).

Remove link address tags from the API.
Make the link local address its own link property. Take it out of the global address set and diffable settings.
port_settings_get and port_settings_apply don't see the link local address, which is how they already want to operate.
tfportd only interacts with the link local address.

That evades the generic complexity of using tags to synchronize tfportd and sled-agent. And I think it requires no substantial changes in either client.

I'm going to try prototyping this rn.

Discussed this with Ry. A few factors point back to (1):

In the future, we may need more robust access controls in dpd. That points towards design (2), in which case keeping tags in the API prevents churn.
Tags were originally added to dpd so that swadm commands survive the omicron reconciler loop. Designs (5) and (3) are a regression from that perspective.
(5) Implies substantial API churn, while the complexity of (1) is mostly internal to dpd.

btw this (possibly out of control) numbering scheme comes from the list here.

Navigation Menu

Repository navigation

port_settings_apply clobbers link-local addresses added via link_ipv6_create #342

Scope

Summary

Detail

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	// Fetch all links that `dpd` knows about.
	let dpd_data = g
	.client
	.tfport_data()
	.await
	.context("failed to list all links")?
	.into_inner()
	.into_iter();

	impl From<&Link> for TfportData {
	fn from(m: &Link) -> Self {
	Self {
	port_id: m.port_id,
	link_id: m.link_id,
	asic_id: m.asic_port_id,
	mac: m.config.mac,
	ipv6_enabled: m.ipv6_enabled,
	link_local: m.link_local(),
	}
	}
	}

port_settings_apply clobbers link-local addresses added via link_ipv6_create #342

Description

Activity

rcgoodfellow commented on Aug 25, 2026

taspelund commented on Aug 25, 2026

cfzimmerman commented on Aug 25, 2026

cfzimmerman commented on Sep 4, 2026

cfzimmerman commented on Sep 9, 2026

Scope

Summary

Detail

jgallagher commented on Sep 9, 2026

cfzimmerman commented on Sep 9, 2026

jgallagher commented on Sep 9, 2026

cfzimmerman commented on Sep 9, 2026

cfzimmerman commented on Sep 9, 2026

cfzimmerman commented on Sep 10, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions