-
Notifications
You must be signed in to change notification settings - Fork 5
Add bestpath fanout config capabilities #524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Adds an API endpoint for /bestpath/fanout and methods to get/set the fanout value. This defines the maximum number of ECMP paths that can be selected by the RIB during bestpath calculation. Fixes: #400 Signed-off-by: Trey Aspelund <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Trey. Overall looks good. Just a few follow ups.
/// Update the fanout setting. | ||
Update { | ||
/// Maximum number of equal-cost paths for ECMP forwarding | ||
fanout: NonZeroU8, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have a practical limit of something like 8 in Dendrite. Is that right @Nieuwejaar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought behind a u8 was that it would be a large enough value to give us a lot of headroom for a while. Unless our switches will be doing 32 uplinks * 8 VLANs per uplink, we wouldn't realistically hit the u8 max.
I agree that the Oxide system should impose limits based on what the switch can handle.
Does that limit belong in maghemite, dpd, or in Nexus?
I think it would make sense for a hard limit to live in dpd (just as a defensive practice) and for Nexus to restrict the upper bounds to dpd's limit at the API level.
This is not a hill I'm interested in dying on though, so if you feel strongly about having a check in maghemite as well then I'm happy to add one
#[derive(Debug, Deserialize, Serialize, JsonSchema)] | ||
pub struct BestpathFanoutRequest { | ||
/// Maximum number of equal-cost paths for ECMP forwarding | ||
pub fanout: NonZeroU8, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment for this and the NonZeroU8
from mgadm
on practical limits for fanout.
Signed-off-by: Trey Aspelund <[email protected]>
I realized during this last round of testing that the handler for the "update" API should trigger a re-run of bestpath.
I'll write a method that walks the RIB and triggers a re-run of bestpath for each route, perhaps taking a closure to allow the caller to optimize which routes need to be kicked. |
Signed-off-by: Trey Aspelund <[email protected]>
New behavior observed w/ the bestpath helper invoked by the fanout update handler:
Currently I'm just having the API handler pass a closure that always evaluates to I'm sure at a large enough RIB scale we wouldn't want to run bestpath for all prefixes, but as an initial implementation I think it's probably fine. Maybe something worth revisiting once we start doing scale testing |
Signed-off-by: Trey Aspelund <[email protected]>
Adds an API endpoint for /bestpath/fanout and methods to get/set the fanout value. This defines the maximum number of ECMP paths that can be selected by the RIB during bestpath calculation.
Fixes: #400