RIB Priority Changes#359
Merged
Merged
Conversation
1. Adds local_pref to rdb::BgpPathProperties, since it's a BGP attribute 2. Updates bestpaths() to compare BgpPathProperties::local_pref instead of protocol-agnostic Path::local_pref. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
27a6e13 to
726dbf8
Compare
local_pref is a well-known BGP attribute, so let's not overload the term for something that's unrelated to BGP (protocol priorities in the RIB). Fixes: #343 Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Enable comparison of rib_priority in the bestpath selection algorithm. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Deliberately prefer static routes over bgp routes, even when the RIB priority matches. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
dc77a80 to
bb45625
Compare
| nexthop: nexthop.into(), | ||
| shutdown: update.graceful_shutdown(), | ||
| local_pref: update.local_pref(), | ||
| rib_priority: DEFAULT_RIB_PRIORITY_BGP, |
Collaborator
There was a problem hiding this comment.
Can we create a follow on issue to make this configurable? It seems to me that we'd want this to be configurable at the router level. Perhaps it could be useful as a per-session option. But for the purposes of this PR, a static value is fine.
rcgoodfellow
approved these changes
Sep 19, 2024
rcgoodfellow
left a comment
Collaborator
There was a problem hiding this comment.
Just one comment on rib_priority configurability for BGP learned routes. Otherwise LGTM.
This was referenced Sep 27, 2024
Closed
ahl
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Sep 27, 2024
This is kind of gross. Omicron currently is out of sync with maghemite, see #6693 and the two maghemite pushes that require synchronization: oxidecomputer/maghemite#359 and oxidecomputer/maghemite#360. I'd like to make forward progress with the hyper v1 migration, and that's blocked on updating the omicron dependency of maghemite which pulls in old hyper (reqwest, progenitor, etc). This gets pull into other repos via omicron-common. It's worth noting that this circular arrangement seems lousy. The "vassal crates" (crucible, propolis, maghemite, (and dendrite to a lesser degree)) depend on omicron-common, but omicron-common pulls in maghemite's `mg-admin-client` which in turn pulls in... lots... including progenitor, hyper, reqwest, etc. My goal here is to have a temporary dependency on a branch of maghemite (oxidecomputer/maghemite#378); once #6693 is integrated, we can then pin the dependency on maghemite's HEAD rev.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This primarily makes 5 changes:
local_prefintoBgpPathProperties(this is a BGP-specific attribute, let's store it with the other BGP-specific attributes)local_preftorib_priority(since this is used by the RIB to prioritize certain protocols/paths over others)