Add BGP group/neighbor level and policy support for AIGP#1451
Add BGP group/neighbor level and policy support for AIGP#1451jenna-mccampbell wants to merge 6 commits intoopenconfig:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances BGP functionality by integrating support for Accumulated IGP (AIGP) metrics. It provides network operators with the flexibility to enable AIGP at various granularities within their BGP configurations and to manipulate AIGP values through routing policies. This expansion allows for more sophisticated traffic engineering and path selection based on internal network costs, aligning the models with advanced routing capabilities found in modern network devices. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request successfully introduces BGP AIGP support at both the group/neighbor level and within routing policies. The changes include adding an enable-aigp boolean flag for address families in neighbors and peer-groups, and a set-aigp leaf for policy actions. The overall structure and integration of these new features into the existing YANG models are well-executed. The use of a dedicated grouping for enable-aigp ensures reusability and consistency.
|
/gcbrun |
|
No major YANG version changes in commit bb42bae |
dplore
left a comment
dplore
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Please consider moving the leaf into this bgp model file:
| } | ||
| } | ||
|
|
||
| grouping ni-bgp-afi-aigp-config { |
There was a problem hiding this comment.
instead of adding a new group in the network-instance.yang and augmenting, this leaf can probably be added to this grouping for a more simple addition.
There was a problem hiding this comment.
I tried this first, but it caused some additional diffs. It enabled the same leaf in the global afi-safis as well, which is outside the scope of this augment. It seems to me that AIGP enablement at the global afi-safi level already exists here: https://openconfig.net/projects/models/schemadocs/yangdoc/openconfig-network-instance.html#network-instances-network-instance-protocols-protocol-bgp-global-afi-safis-afi-safi-route-selection-options-config-enable-aigp
There was a problem hiding this comment.
I have re-analyzed grouping usage:
If we use bgp-common-mp-afi-safi-config, enable-aigp becomes available in all address families, which causes unintended scope creep beyond just the use case (the use case requires only ipv4 and ipv6 unicast).
We could instead use bgp-common-mp-ipv4-ipv6-unicast-common-config, however that configuration also causes unintended scope creep because it applies the config to all ipv4 and ipv6 unicast address families including those under the global configuration (the use case requires the configuration only at the group and neighbor level)
Although it is less clean, I think going with the original implementation here best preserves the intended scope of the change.
|
Should we cover other AFI/SAFI as well in this PR to not have to add later? |
HPE/Juniper's implementation limits the afi/safi it can be used for to ipv4/ipv6 unicast/labeled unicast. |
Adding AIGP for these additional address families is outside of the scope of the use case that motivates this change. The change here does not preclude adding this capability to additional address families should a use case for that functionality come up. For now, I don't want to boil the ocean. |
|
/gcbrun |
6 participants
Change Scope
the group and neighbor level for IPv4 and IPv6 unicast address families.
It also adds the ability to set an arbitrary AIGP value in a BGP policy.
Platform Implementations
Note: Implementation A and Implementation B align well with the paths added
here. Implementation C does not allow setting an arbitrary AIGP value in
policy. AIGP value can instead be originated and adjusted via addition,
subtraction, multiplication, or division. Implementation D supports AIGP
enablement only in the base router BGP instance (not per group or
neighbor) and does not support AIGP manipulation in route policies.
Tree View
module: openconfig-network-instance +--rw network-instances +--rw network-instance* [name] +--rw protocols | +--rw protocol* [identifier name] | +--rw bgp | | +--rw neighbors | | | +--rw neighbor* [neighbor-address] | | | +--rw afi-safis | | | | +--rw afi-safi* [afi-safi-name] | | | | +--rw ipv4-unicast | | | | | +--rw prefix-limit | | | | | +--rw prefix-limit-received | | | | | +--rw config | | | | | | +--rw send-default-route? boolean | | | | | | +--rw extended-next-hop-encoding? boolean + | | | | | | +--rw enable-aigp? boolean | | | | | +--ro state | | | | | +--ro send-default-route? boolean | | | | | +--ro extended-next-hop-encoding? boolean + | | | | | +--ro enable-aigp? boolean | | | | +--rw ipv6-unicast | | | | | +--rw prefix-limit | | | | | +--rw prefix-limit-received | | | | | +--rw config | | | | | | +--rw send-default-route? boolean + | | | | | | +--rw enable-aigp? boolean | | | | | +--ro state | | | | | +--ro send-default-route? boolean + | | | | | +--ro enable-aigp? boolean | | | | +--rw ipv4-labeled-unicast | | +--rw peer-groups | | | +--rw peer-group* [peer-group-name] | | | +--rw afi-safis | | | | +--rw afi-safi* [afi-safi-name] | | | | +--rw ipv4-unicast | | | | | +--rw prefix-limit | | | | | +--rw prefix-limit-received | | | | | +--rw config | | | | | | +--rw send-default-route? boolean | | | | | | +--rw extended-next-hop-encoding? boolean + | | | | | | +--rw enable-aigp? boolean | | | | | +--ro state | | | | | +--ro send-default-route? boolean | | | | | +--ro extended-next-hop-encoding? boolean + | | | | | +--ro enable-aigp? boolean | | | | +--rw ipv6-unicast | | | | | +--rw prefix-limit | | | | +--rw ipv4-labeled-unicast module: openconfig-routing-policy +--rw routing-policy +--rw policy-definitions +--rw policy-definition* [name] +--rw statements +--rw statement* [name] +--rw actions +--rw oc-bgp-pol:bgp-actions | +--rw oc-bgp-pol:config | | +--rw oc-bgp-pol:set-route-origin? oc-bgp-types:bgp-origin-attr-type | | +--rw oc-bgp-pol:set-local-pref? uint64 | | +--rw oc-bgp-pol:set-next-hop? bgp-next-hop-type | | +--rw oc-bgp-pol:set-med? bgp-set-med-type | | +--rw oc-bgp-pol:set-med-action? bgp-set-med-action + | | +--rw oc-bgp-pol:set-aigp? uint64 | +--ro oc-bgp-pol:state | | +--ro oc-bgp-pol:set-route-origin? oc-bgp-types:bgp-origin-attr-type | | +--ro oc-bgp-pol:set-local-pref? uint64 | | +--ro oc-bgp-pol:set-next-hop? bgp-next-hop-type | | +--ro oc-bgp-pol:set-med? bgp-set-med-type | | +--ro oc-bgp-pol:set-med-action? bgp-set-med-action + | | +--ro oc-bgp-pol:set-aigp? uint64