Skip to content

[1/n] [sync-switch] build bootstore bgp peers from already-loaded port settings#10642

Open
sunshowers wants to merge 2 commits into
mainfrom
sunshowers/spr/1n-sync-switch-build-bootstore-bgp-peers-from-already-loaded-port-settings
Open

[1/n] [sync-switch] build bootstore bgp peers from already-loaded port settings#10642
sunshowers wants to merge 2 commits into
mainfrom
sunshowers/spr/1n-sync-switch-build-bootstore-bgp-peers-from-already-loaded-port-settings

Conversation

@sunshowers

Copy link
Copy Markdown
Contributor

Previously, the sync_switch_configuration task built the bootstore RackNetworkConfig's BGP peers by issuing, for every port:

  • a bgp_peer_configs query; and,
  • three queries for each peer (communities, allowed-import, allowed-export).

But all of that data is already present in the SwitchPortSettingsCombinedResult the task fetched for the port, and is in fact loaded transactionally as part of that. Switch to using that via info.bgp_peers instead.

(Previously, we also hardcoded the interface as phy0, so a peer on any other link silently got empty communities and no-filtering policies pushed to the bootstore. As a natural consequence, this PR fixes that.)

This drastically simplifies the Nexus-side fix I had in mind for #10640, so I'd like to get this in as a preliminary fix.

Note that the mgd-apply path has the same redundant reads but (a) already uses the correct interface, (b) doesn't interact with #10640, and (c) is currently undergoing churn, so I left it alone for now.

How did we get here?

As part of fixing this I tried digging into the history of these redundant reads.

  • In 2f65249 (oxpop #5649) these fetches were introduced. There were two fetches here -- bootstore-config, which passed in the port name (wrong), and the mgd-apply path, which passed in the interface name (correct). (Even putting the mixup aside, doing fetches twice like this is dodgy in its own right -- we should generally minimize fetches from the db.)
  • In ecb6214 (Various networking API & DB fixes #5701) the same data was read transactionally and by the correct interface name, but the bg task was never updated to use this data, and instead kept doing the broken re-fetch.
  • In e4bcfee (fix query selector for bgp filters and communities #6072) the bug was worked around by hardcoding phy0 as the interface name. This was a workaround, though not a proper fix since it didn't use the real interface name.

This PR effectively closes the loop by using the correctly-fetched data. (But it doesn't resolve #3062, breakout support, fully yet.)

The root cause of the mixup is the lack of newtypes around the port and interface names (they're both Names). In the future, we should introduce newtype wrappers so they can't be confused like this. But I've deferred that work for now.

Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
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.

Handle breakout ports

1 participant