Skip to content

[nexus] Remove duplicated networking types from omicron-common / external API#10344

Open
jgallagher wants to merge 8 commits intomainfrom
john/move-more-networking-types
Open

[nexus] Remove duplicated networking types from omicron-common / external API#10344
jgallagher wants to merge 8 commits intomainfrom
john/move-more-networking-types

Conversation

@jgallagher
Copy link
Copy Markdown
Contributor

This fixes #10293. 2/3 of these were straightforward; removing the duplicate SwitchPortGeometry2 and TxEqConfig2 types required removing those from common and moving any other types still in common that used them out into nexus-types-versions; those are done in the first two commits on this PR.

Removing the duplicate SwitchInterfaceKind2 was more involved because the two types didn't have the same structure despite having the same name. This is split into two commits: 71f7db2 renames one of them to SwitchInterfaceKindNoVlanDetails and moves it to nexus-types-versions (where it lives alongside SwitchInterfaceKind). Then 9db5a0cad8694f40e1c572f172794b2144ce4bdb makes an API change to SwitchInterfaceConfig (changing the kind to the SwitchInterfaceKind that includes the vlan ID) and SwitchPortSettings.

The SwitchPortSettings change needs the most scrutiny. My understanding is that on main, SwitchPortSettings has these two fields:

    /// Layer 3 interface settings.
    pub interfaces: Vec<SwitchInterfaceConfig>,

    /// Vlan interface settings.
    pub vlan_interfaces: Vec<SwitchVlanInterfaceConfig>,

where each item in interfaces has a kind of either Primary, Loopback, or Vlan. If the kind is Vlan, then there will be an item in vlan_interfaces containing just the interface ID and the vlan ID:

pub struct SwitchVlanInterfaceConfig {
    pub interface_config_id: Uuid,
    pub vlan_id: u16,
}

This is based on both the comments and the path that inserts these into the db, where we only add items to vlan_interfaces if it's coming from an interface with the Vlan kind:

for i in &params.interfaces {
let ifx_config = SwitchInterfaceConfig::new(
psid,
i.link_name.clone().into(),
i.v6_enabled,
i.kind.into(),
);
interface_config.push(ifx_config.clone());
if let networking::SwitchInterfaceKind::Vlan(vlan_if) = i.kind {
vlan_interface_config.push(SwitchVlanInterfaceConfig::new(
ifx_config.id,
vlan_if.vid,
));
}
}

I assume the expectation here is that clients would have to reassemble these two vecs to glue together the vlan ID of any interface with a vlan by taking the interface ID from interfaces and finding it in vlan_interfaces. The change I made on this PR is to drop the vlan_interfaces field entirely, change the kind of SwitchInterfaceConfig to include the vlan ID, then do the reassembly ourselves when we load this object from the db:

// We expect the db to hold exactly one `vlan_interface` row for any
// `interface` that has kind `Vlan`. Build up a map of all the vlan
// interface rows; we'll then prune through that map as we loop over all
// the interfaces and check that it's empty when we're done.
//
// We expect to find exactly 1 entry for every `Vlan` interface and
// nothing extra; anything else is some kind of internal inconsistency.
let mut vlan_by_interface = BTreeMap::new();
for vlan_iface in result.vlan_interfaces {
let SwitchVlanInterfaceConfig { interface_config_id, vid } =
vlan_iface;
vlan_by_interface.insert(interface_config_id, vid);
}
let mut interfaces = Vec::with_capacity(result.interfaces.len());
for iface in result.interfaces {
let kind = match iface.kind {
DbSwitchInterfaceKind::Primary => {
networking::SwitchInterfaceKind::Primary
}
DbSwitchInterfaceKind::Loopback => {
networking::SwitchInterfaceKind::Loopback
}
DbSwitchInterfaceKind::Vlan => {
let Some(vid) = vlan_by_interface.remove(&iface.id) else {
return Err(external::Error::internal_error(format!(
"switch port settings has inconsistent data: \
interface {iface:?} has type vlan, but \
vlan_interfaces does not have matching interface: \
{vlan_by_interface:?}"
)));
};
networking::SwitchInterfaceKind::Vlan(
networking::SwitchVlanInterface { vid: *vid },
)
}
};
interfaces.push(networking::SwitchInterfaceConfig {
port_settings_id: iface.port_settings_id,
id: iface.id,
interface_name: iface.interface_name.into(),
v6_enabled: iface.v6_enabled,
kind,
});
}
if !vlan_by_interface.is_empty() {
return Err(external::Error::internal_error(format!(
"switch port settings has inconsistent data: \
`vlan_interfaces` references interface IDs that do not have \
a matching vlan interface: leftover vlan values: \
{vlan_by_interface:?}, assembled interfaces: {interfaces:?}"
)));
}

But please confirm that (a) I understood what these fields contained and (b) the reassembly I'm doing there is correct!

@rcgoodfellow rcgoodfellow added the networking Related to the networking. label Apr 30, 2026
@internet-diglett
Copy link
Copy Markdown
Contributor

Starting to look at this today, thank you for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

networking Related to the networking.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

get rid of types with duplicate names in the API

3 participants