proposal for making prefix-limit general for afi-safi

[ishidawataru]

Hello,

I'm implementing prefix-limit feature for GoBGP using OpenConfig yang model and have a proposal for the model.

Now prefix-limit configuration knob is under each address family specific item.
(ipv4-unicast-group, ipv6-unicast-group etc...)

https://github.com/openconfig/public/blob/master/release/models/bgp/openconfig-bgp-multiprotocol.yang#L718-L727

When we auto-generate a configuration header file for some specific language
(for my case, golang),
current model leads to generate redundant items.

For example, when afi-safi-name is ipv4-unicast, we don't need these lines

Since prefix limit configuration is general among address families (like graceful restart), how about making it the same way as graceful restart?

I think this makes the model more simple and clean.

[aashaikh]

We have an afi-safi specific setting because the semantics of a global prefix-limit setting would be ambiguous -- e.g., does the limit apply to all families (same number of prefixes allowed for v6, v4, multicast, etc.), or is it somehow allocated to different address families. Often times you would want a different prefix-limit for one family vs. another. In other words, unlike graceful restart, it's not a feature that you might turn on or off for all address families the same way.

I would hope that for a machine-generated config, it would not be too much burden to generate them for each afi-safi. BTW, the model is designed such that you would only have to set it for the afi-safis that are enabled per the flag for each family.

[ishidawataru]

Hello @aashaikh

Thanks for the reply.

I'm not saying make prefix-limit global but general.

ishidawataru@78a2256

The commit above shows what I'm meaning.
This keeps ability to allocate different prefix-limit for each address family.

BTW, the model is designed such that you would only have to set it for the afi-safis that are enabled per the flag for each family.

Could you explain more about the flag? Is this in somewhere of the model?

[aashaikh]

hi @ishidawataru , I'm still not sure I understand what is the difference between global and general -- by general, I guess you mean a single setting that would apply to all afi-safis? You're correct that this doesn't have to be at a bgp-global level; it could also be at neighbor or peer group level.

If it's about the model structure (as your reference commit seems to indicate), we already have a common grouping that is reused across all afi-safi containers which includes the prefix-limit setting.

The enabled flag is at:
/bgp:bgp/bgp:global/bgp:afi-safis/bgp:afi-safi/bgp:config/bgp:enabled
in the schema. So an example XPATH would be
/bgp/global/afi-safis/afi-safi[afi-safi-name=ipv4-unicast]/config/enabled
This config is also available on per-neighbor or per-peer-group basis.

[ishidawataru]

I guess you mean a single setting that would apply to all afi-safis?

No.

If it's about the model structure (as your reference commit seems to indicate), we already have a common grouping that is reused across all afi-safi containers which includes the prefix-limit setting.

Yes, I'm saying about the model structure.

What about moving this all-afi-safi-common to /bgp:bgp/bgp:global/bgp:afi-safis/bgp-mp:afi-safi/bgp-mp:all-afi-safi-common like graceful-restart (which is at /bgp:bgp/bgp:global/bgp:afi-safis/bgp-mp:afi-safi/bgp-mp:graceful-restart) because all-afi-safi-common is all common among afi-safis ?

Thanks for the pointer of the enabled flag!

[robshakir]

Hi,

With the current specified set of <AFI,SAFI> one could move the prefix-limit configuration to being within the root (AFI, SAFI) container. The motivation and benefit that the current structure has is that if one were to introduce a new address family that does not support configuration of maximum prefix, then one simply omits the all-afi-safi-common grouping. Since GR is something that's supported at the session level, and then explicitly enabled/disabled at the <AFI, SAFI> level, then it doesn't seem to make sense that an implementation that supported GR would ever omit that configuration, hence the difference.

Cheers,
r.

[robshakir]

An example where all-afi-safi-common may not be used is Route Target constrain RFC4684. In this case, the peer is announcing the set of RTs that they are interested in to the peer (typically a route reflector). It is uncommon that we will have a limited number of RTs that are expected to be announced by a single PE, so therefore operationally, we would potentially exclude the all-afi-safi-common grouping for this <AFI,SAFI>.

To do this in the case that maximum-prefix was at the top level we would need to issue a module internal deviation statement, which is complex.

Cheers,
r.

[ishidawataru]

Hi @robshakir

I see. I understand your point.
But don't you think the model will be simpler if we can remove containers named ipv4-unicast, ipv6-unicast etc.. ?
Please see this tree diagram (generated by pyang using openconfig model)

            +--rw afi-safis
            |  +--rw afi-safi* [afi-safi-name]
            |     +--rw afi-safi-name              -> ../config/afi-safi-name
...(snip)...
            |     +--rw ipv4-unicast
            |     |  +--rw prefix-limit
            |     |  |  +--rw config
            |     |  |  |  +--rw max-prefixes?             uint32
            |     |  |  |  +--rw shutdown-threshold-pct?   oc-types:percentage
            |     |  |  |  +--rw restart-timer?            decimal64
            |     |  |  +--ro state
            |     |  |     +--ro max-prefixes?             uint32
            |     |  |     +--ro shutdown-threshold-pct?   oc-types:percentage
            |     |  |     +--ro restart-timer?            decimal64
            |     |  +--rw config
            |     |  |  +--rw send-default-route?   boolean
            |     |  +--ro state
            |     |     +--ro send-default-route?   boolean
            |     +--rw ipv6-unicast
            |     |  +--rw prefix-limit
            |     |  |  +--rw config
            |     |  |  |  +--rw max-prefixes?             uint32
            |     |  |  |  +--rw shutdown-threshold-pct?   oc-types:percentage
            |     |  |  |  +--rw restart-timer?            decimal64
            |     |  |  +--ro state
            |     |  |     +--ro max-prefixes?             uint32
            |     |  |     +--ro shutdown-threshold-pct?   oc-types:percentage
            |     |  |     +--ro restart-timer?            decimal64
            |     |  +--rw config
            |     |  |  +--rw send-default-route?   boolean
            |     |  +--ro state
            |     |     +--ro send-default-route?   boolean
            |     +--rw ipv4-labelled-unicast
            |     |  +--rw prefix-limit
            |     |     +--rw config
            |     |     |  +--rw max-prefixes?             uint32
            |     |     |  +--rw shutdown-threshold-pct?   oc-types:percentage
            |     |     |  +--rw restart-timer?            decimal64
            |     |     +--ro state
            |     |        +--ro max-prefixes?             uint32
            |     |        +--ro shutdown-threshold-pct?   oc-types:percentage
            |     |        +--ro restart-timer?            decimal64
            |     +--rw ipv6-labelled-unicast
            |     |  +--rw prefix-limit
            |     |     +--rw config
            |     |     |  +--rw max-prefixes?             uint32
            |     |     |  +--rw shutdown-threshold-pct?   oc-types:percentage
            |     |     |  +--rw restart-timer?            decimal64
            |     |     +--ro state
            |     |        +--ro max-prefixes?             uint32
            |     |        +--ro shutdown-threshold-pct?   oc-types:percentage
            |     |        +--ro restart-timer?            decimal64
            |     +--rw l3vpn-ipv4-unicast
            |     |  +--rw prefix-limit
            |     |     +--rw config
            |     |     |  +--rw max-prefixes?             uint32
            |     |     |  +--rw shutdown-threshold-pct?   oc-types:percentage
            |     |     |  +--rw restart-timer?            decimal64
            |     |     +--ro state
            |     |        +--ro max-prefixes?             uint32
            |     |        +--ro shutdown-threshold-pct?   oc-types:percentage
            |     |        +--ro restart-timer?            decimal64
...(snip)...

We have many redundant items (related to prefix-limit) in afi-safi container.
I know the model is using when statement here, so not all these items will be enabled on runtime.
However, XPATH of prefix-limit items still need address-family.

When afi-safi-name is ipv4-unicast, it would be

/bgp/global/afi-safis/afi-safi[afi-safi-name=ipv4-unicast]/ipv4-unicast/prefix-limit/config/max-prefixes

If we can make this

/bgp/global/afi-safis/afi-safi[afi-safi-name=ipv4-unicast]/prefix-limit/config/max-prefixes

by using when or choice (to exclude prefix-limit knob for route-target address family), it would be nice.

[missaesasaya]

In #781 (comment) there is a mention of "Impact on path targeting":

"For any API operations that target a specific path for interesting data (e.g.
Give me all interfaces with IPv4 or IPv6 addresses), the divergence in model
design for only some interfaces means that a client cannot target a single
specific path

e.g.

/interfaces/interface[name=*]/subinterfaces/subinterface/ipv4/addresses
Since the path hierarchy is different, this means a client needs to target
multiple paths for consumption or programmability of like objects."

Wouldn't that apply here also? It seems to me that it would be better to have a when statement excluding the unwanted afi/safi values than having to create a new path everytime you need a new afi/safi.

[github-actions]

This issue is stale because it has been open 180 days with no activity. If you wish to keep this issue active, please remove the stale label or add a comment, otherwise will be closed in 14 days.