scrimlet-reconcilers: Add dpd port reconciliation#10449
Conversation
|
Created oxidecomputer/dendrite#272 for testing |
| .addresses | ||
| .iter() | ||
| .filter_map(|a| { | ||
| // TODO we're discarding any vlan_id - is that okay? |
There was a problem hiding this comment.
I think the vlan configuration is handled by uplinkd
| // TODO We're discarding the `ip_net.prefix()` here | ||
| // and only using the IP address; at some point we | ||
| // probably need to give the full CIDR to dendrite? | ||
| Some(ip_net.addr()) |
There was a problem hiding this comment.
I think all of the mechanisms that rely on the net mask / full CIDR are also handled via uplinkd.
| // Workaround for dpd limitation: the speed and fec of a link | ||
| // require us to remove it first. Any other change can be | ||
| // applied in place. TODO: link to dendrite issue / PR | ||
| if leaf.before().speed != leaf.after().speed | ||
| || leaf.before().fec != leaf.after().fec | ||
| { | ||
| to_clear.insert(port_id.clone()); | ||
| } |
There was a problem hiding this comment.
We can remove this workaround now
There was a problem hiding this comment.
👍 Will remove this once #10497 merges and I can pull it into this branch.
There was a problem hiding this comment.
Approved, although it looks to do the same update that I've already done here :D
| let are_port_settings_clear = { | ||
| let DpdPortSettings { links } = &settings; | ||
| links.is_empty() | ||
| }; |
There was a problem hiding this comment.
Just something I was curious about, is there a reason we are doing this instead of something like:
| let are_port_settings_clear = { | |
| let DpdPortSettings { links } = &settings; | |
| links.is_empty() | |
| }; | |
| let are_port_settings_clear = settings.links.is_empty(); |
There was a problem hiding this comment.
I've started being pretty aggressive with adding full destructurings like this to be defensive against fields being added in the future. Suppose a new version of DpdPortSettings adds another field (interfaces or whatever). If we had this:
let are_port_settings_clear = settings.links.is_empty();that will still compile, and we have to rely on memory / tests / whatever to know whether it's okay that we're ignoring a new field. But this:
let DpdPortSettings { links } = &settings;will fail to compile because it doesn't include the new field, which guarantees a person will look at it and decide what to do about the new field.
There was a problem hiding this comment.
Got it. That makes a lot of sense and that's a pattern I should also keep in mind for use in the future!
2 participants
This PR looks big but over 60% of it is tests; the production code that needs careful review is the new
sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler.rsmodule. It contains a port of whatsync_switch_reconcilerdoes to configure QSFP ports, with several changes:RackNetworkConfiginstead of the dbdaftdpdissue where ports that change speed or FEC have to be removed before reapplying. @internet-diglett is working on adpdchange that would make this unnecessary, but for now it's easy enough to work around on our side.