Skip to content

nexus-db-queries: replace BgpPeerConfig with BgpPeerFromDb#10086

Open
jgallagher wants to merge 1 commit intomainfrom
john/bgp-db-cleanup-1
Open

nexus-db-queries: replace BgpPeerConfig with BgpPeerFromDb#10086
jgallagher wants to merge 1 commit intomainfrom
john/bgp-db-cleanup-1

Conversation

@jgallagher
Copy link
Contributor

The BgpPeerConfig exported by nexus-db-queries was a type that contained mostly database-flavored types (SqlU16 and friends) but had most of the same fields as the external API's BgpPeer type (while having no relation to the external API's BgpPeerConfig). This change replaces the datastore BgpPeerConfig with BgpPeerFromDb, which wraps an external API BgpPeer and adds the two extra fields we need in the sync_switch_configuration background task.

The rest of the changes are fallout from that; mostly we get to get rid of a lot of .map(|x| x.0) or .map(From::from) conversions that the bg task had to do to go from "database-flavored types" to "API-flavored types".

Part of #9832.

`BgpPeerConfig` was a type that contained mostly database-flavored types
(`SqlU16` and friends) but had most of the same fields as the external
API's `BgpPeer` type (while having no relation to the external API's
`BgpPeerConfig`). This change replaces the datastore `BgpPeerConfig`
with `BgpPeerFromDb`, which wraps an external API `BgpPeer` and adds the
two extra fields we need in the `sync_switch_configuration` background
task.

The rest of the changes are fallout from that; mostly we get to get rid
of a lot of `.map(|x| x.0)` or `.map(From::from)` conversions that the
bg task had to do to go from "database-flavored types" to "API-flavored
types".

Part of #9832.
// we can compare the rest of the struct at once, since
// `db_peer.inner.bgp_config` is always populated with an ID.
peer.bgp_config = NameOrId::Id(db_peer.bgp_config_id);
assert_eq!(peer, db_peer.inner);
Copy link
Contributor Author

@jgallagher jgallagher Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the test failing in CI is because these two disagree on the value of interface_name:

interface_name: Name("qsfp0")
interface_name: Name("phy0")

I don't think I changed the behavior here - it looks like the test as written before omitted checking this field. Was that intentional, and there's a reason they're different? Or is the fact that they're different a bug that's just now surfacing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this mismatch is also present on main. SwitchPortBgpPeerConfig takes an explicit interface_name: Name argument:

impl SwitchPortBgpPeerConfig {
pub fn new(
port_settings_id: Uuid,
bgp_config_id: Uuid,
interface_name: Name,
p: &networking_types::BgpPeer,
) -> Self {

and it ignores p.interface_name (the interface name inside the networking_types::BgpPeer it's given). When we call this from do_switch_port_settings_create(), we pass the link_name, not the interface_name:

bgp_peer_config.push(SwitchPortBgpPeerConfig::new(
psid,
bgp_config_id,
peer_config.link_name.clone().into(),
p,
));

That explains why when we query this BGP config back after inserting it, we see an interface name of phy0 (the link name), not qsfp0 (the interface name inside the BgpPeer struct, which is ignored AFAICT).

I don't know what to make of this, though. Bug? Not bug just surprising behavior? More complex than either of those?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant