Add BGP Monitoring Protocol (BMP) support#1283
Conversation
| "This module defines configuration and operational state data for | ||
| BGP Monitoring Protocol (BMP) as defined in RFC 7854."; | ||
|
|
||
| oc-ext:openconfig-version "9.9.0"; |
There was a problem hiding this comment.
Is a new module proposed here (which is not a submodule) thus start the versioning at 0.1.0
|
|
||
| leaf idle-time { | ||
| type uint16; | ||
| description |
| leaf probe-interval { | ||
| type uint16; | ||
| description | ||
| "The time in seconds between TCP keepalive probes"; |
| } | ||
| } | ||
|
|
||
| grouping bmp-tcp-keepalive-state { |
There was a problem hiding this comment.
The same grouping can be reused for config/state (reflection) vs. redefining each node again
| description | ||
| "Operational state parameters for BMP monitoring of specific AFI-SAFIs"; | ||
|
|
||
| leaf afi-safi-name { |
There was a problem hiding this comment.
Same comment as above re: duplication.
|
|
||
| leaf uptime { | ||
| type uint64; | ||
| description |
There was a problem hiding this comment.
Recommend using this type for all the timestamp leaves:
type oc-types:timeticks64;
| } | ||
|
|
||
| leaf flap-count { | ||
| type uint32; |
There was a problem hiding this comment.
| type uint32; | |
| type uint32; | |
| type oc-yang:counter32; |
Likely colocate under a counters container
There was a problem hiding this comment.
+1 please use a state/counters container whenever there are counters present. For example:
+--rw stations
| +--rw station* [name]
| +--rw name string
| +--ro state
| | +--ro counters
| | | +--ro total? oc-yang:counter64
| | | +--ro statistics? oc-yang:counter64
| | | +--ro route-monitoring? oc-yang:counter64
| | | +--ro peer-monitoring? oc-yang:counter64
| | | +--ro route-mirroring? oc-yang:counter64
| +--rw afi-safis
| +--rw afi-safi* [afi-safi-name]
| +--rw afi-safi-name -> ../config/afi-safi-name
| +--rw config
| | +--rw afi-safi-name? identityref
| | +--rw enabled? boolean
| +--ro state
| +--ro afi-safi-name? identityref
| +--ro enabled? boolean
| +--ro counters
| +--ro route-monitoring-messages-sent? uint64
| "Total number of BMP messages sent to this station"; | ||
| } | ||
|
|
||
| leaf statistics { |
There was a problem hiding this comment.
Suggest better name as this indicates "sent" messages
| at the next higher level in the hierarchy."; | ||
|
|
||
| oc-ext:openconfig-version "9.8.0"; | ||
| oc-ext:openconfig-version "9.9.0"; |
There was a problem hiding this comment.
This model doesn't appear to need any edits at all - no version increment is needed
earies
left a comment
There was a problem hiding this comment.
After minor adjustments, the tree view looks as follows:
module: openconfig-bgp-bmp
augment /oc-netinst:network-instances/oc-netinst:network-instance/oc-netinst:protocols/oc-netinst:protocol/oc-netinst:bgp/oc-netinst:global:
+--rw bmp
+--rw config
| +--rw enabled? boolean
| +--rw connection-mode? connection-mode-type
| +--rw local-address? oc-inet:ip-address
| +--rw statistics-timeout? uint16
| +--rw idle-time? uint16
| +--rw probe-count? uint8
| +--rw probe-interval? uint16
+--ro state
| +--ro enabled? boolean
| +--ro connection-mode? connection-mode-type
| +--ro local-address? oc-inet:ip-address
| +--ro statistics-timeout? uint16
| +--ro idle-time? uint16
| +--ro probe-count? uint8
| +--ro probe-interval? uint16
+--rw tcp-keepalive
| +--rw config
| | +--rw idle-time? uint16
| | +--rw probe-count? uint8
| | +--rw probe-interval? uint16
| +--ro state
| +--ro idle-time? uint16
| +--ro probe-count? uint8
| +--ro probe-interval? uint16
+--rw stations
| +--rw station* [name]
| +--rw name string
| +--rw config
| | +--rw address oc-inet:ip-address
| | +--rw port? oc-inet:port-number
| | +--rw policy-type? route-monitoring-policy-type
| | +--rw exclude-non-eligible? boolean
| +--ro state
| | +--ro address? oc-inet:ip-address
| | +--ro port? oc-inet:port-number
| | +--ro connection-status? connection-status-type
| | +--ro uptime? uint64
| | +--ro flap-count? uint32
| | +--ro message-counters
| | | +--ro total? oc-yang:counter64
| | | +--ro statistics? oc-yang:counter64
| | | +--ro route-monitoring? oc-yang:counter64
| | | +--ro peer-monitoring? oc-yang:counter64
| | | +--ro route-mirroring? oc-yang:counter64
| | +--ro policy-type? route-monitoring-policy-type
| | +--ro exclude-non-eligible? boolean
| +--rw afi-safis
| +--rw afi-safi* [afi-safi-name]
| +--rw afi-safi-name -> ../config/afi-safi-name
| +--rw config
| | +--rw afi-safi-name? identityref
| | +--rw enabled? boolean
| +--ro state
| +--ro afi-safi-name? identityref
| +--ro enabled? boolean
| +--ro route-monitoring-messages-sent? uint64
+--rw station-groups
| +--rw station-group* [name]
| +--rw name string
| +--rw config
| | +--rw station-names* string
| +--ro state
| +--ro station-names* string
+--rw afi-safis
+--rw afi-safi* [afi-safi-name]
+--rw afi-safi-name -> ../config/afi-safi-name
+--rw config
| +--rw afi-safi-name? identityref
| +--rw enabled? boolean
+--ro state
+--ro afi-safi-name? identityref
+--ro enabled? boolean
+--ro route-monitoring-messages-sent? uint64
augment /oc-netinst:network-instances/oc-netinst:network-instance/oc-netinst:protocols/oc-netinst:protocol/oc-netinst:bgp/oc-netinst:neighbors/oc-netinst:neighbor:
+--rw bmp
+--rw config
| +--rw enabled? boolean
+--ro state
| +--ro enabled? boolean
+--rw afi-safis
+--rw afi-safi* [afi-safi-name]
+--rw afi-safi-name -> ../config/afi-safi-name
+--rw config
| +--rw afi-safi-name? identityref
| +--rw enabled? boolean
+--ro state
+--ro afi-safi-name? identityref
+--ro enabled? boolean
+--ro route-monitoring-messages-sent? uint64
augment /oc-netinst:network-instances/oc-netinst:network-instance/oc-netinst:protocols/oc-netinst:protocol/oc-netinst:bgp/oc-netinst:peer-groups/oc-netinst:peer-group:
+--rw bmp
+--rw config
| +--rw enabled? boolean
+--ro state
| +--ro enabled? boolean
+--rw afi-safis
+--rw afi-safi* [afi-safi-name]
+--rw afi-safi-name -> ../config/afi-safi-name
+--rw config
| +--rw afi-safi-name? identityref
| +--rw enabled? boolean
+--ro state
+--ro afi-safi-name? identityref
+--ro enabled? boolean
+--ro route-monitoring-messages-sent? uint64
Some overall comments:
- You are anchoring global BMP config/state inside a BGP protocol instance within a network-instance. I would question the association to a "protocol instance" here if this is necessary and also the NI anchor point vs. NI reference as seen in other "service" definitions (e.g. dial-out similar technologies)
- List keys must be a leafref to subsequent
../config/or../state/nodes (e.g.stations/station/name) - Am I missing where
station-groupsis of use? I believe I only see a definition but no reference/use
|
|
||
| prefix "oc-bgp-bmp"; | ||
|
|
||
| import openconfig-bgp { prefix oc-bgp; } |
|
|
||
| leaf-list station-names { | ||
| type leafref { | ||
| path "../../../stations/station/config/name"; |
|
|
||
| leaf-list station-names { | ||
| type leafref { | ||
| path "../../../stations/station/config/name"; |
There was a problem hiding this comment.
leafref does not resolve (do not duplicate groupings)
| uses bmp-route-monitoring-state; | ||
| } | ||
|
|
||
| grouping bmp-station-group-config { |
There was a problem hiding this comment.
You are defining a relative path leafref within a grouping. If the grouping is intended for reuse then it is subject to dependency hiearchies that sit at the same level anchor points
|
|
||
| leaf uptime { | ||
| type uint64; | ||
| description |
There was a problem hiding this comment.
Recommend using this type for all the timestamp leaves:
type oc-types:timeticks64;
| } | ||
|
|
||
| leaf flap-count { | ||
| type uint32; |
There was a problem hiding this comment.
+1 please use a state/counters container whenever there are counters present. For example:
+--rw stations
| +--rw station* [name]
| +--rw name string
| +--ro state
| | +--ro counters
| | | +--ro total? oc-yang:counter64
| | | +--ro statistics? oc-yang:counter64
| | | +--ro route-monitoring? oc-yang:counter64
| | | +--ro peer-monitoring? oc-yang:counter64
| | | +--ro route-mirroring? oc-yang:counter64
| +--rw afi-safis
| +--rw afi-safi* [afi-safi-name]
| +--rw afi-safi-name -> ../config/afi-safi-name
| +--rw config
| | +--rw afi-safi-name? identityref
| | +--rw enabled? boolean
| +--ro state
| +--ro afi-safi-name? identityref
| +--ro enabled? boolean
| +--ro counters
| +--ro route-monitoring-messages-sent? uint64
| to BMP servers or passively listens for connections"; | ||
| } | ||
|
|
||
| leaf local-address { |
There was a problem hiding this comment.
Minor nit: In most OC models, we use 'source-address' for this leaf.
| } | ||
| } | ||
|
|
||
| grouping bmp-afi-safi-config { |
There was a problem hiding this comment.
I don't see examples in the referenced implementations of being able to enable/disable BMP per afi/safi. Is this really needed? We should only model OC for the features we are actually using (or have an open, active feature request to use).
| } | ||
| } | ||
|
|
||
| grouping bmp-station-config { |
There was a problem hiding this comment.
I think we probably need to have some support for the BMP station to be in a different network-instance than the BGP protocol. For example, we might have BGP in the DEFAULT network instance and a BMP station in a management network instance. This could be modeled by adding a 'network-instance' leaf to this grouping.
| } | ||
| } | ||
|
|
||
| container station-groups { |
There was a problem hiding this comment.
In concept this seems ok, but is this really needed? For example, are we using this in production already? I don't see any example of this in the quoted implementation references?
| uses bmp-tcp-keepalive-config; | ||
| } | ||
|
|
||
| grouping bmp-state { |
There was a problem hiding this comment.
as noted elsewhere, you can reuse the bmp-config grouping instead of defining an identical bmp-state grouping.
| augment "/oc-netinst:network-instances/oc-netinst:network-instance/oc-netinst:protocols/oc-netinst:protocol/oc-netinst:bgp/oc-netinst:neighbors/oc-netinst:neighbor" { | ||
| description | ||
| "Augments BGP neighbor configuration with BMP parameters"; | ||
| uses bmp-neighbor-top; | ||
| } | ||
|
|
||
| augment "/oc-netinst:network-instances/oc-netinst:network-instance/oc-netinst:protocols/oc-netinst:protocol/oc-netinst:bgp/oc-netinst:peer-groups/oc-netinst:peer-group" { | ||
| description | ||
| "Augments BGP peer-group configuration with BMP parameters"; | ||
| uses bmp-neighbor-top; | ||
| } |
There was a problem hiding this comment.
I don't see peer-group and neighbor defined in the referenced implementations. How important is this?
|
/gcbrun |
|
@ihebboubaker reviewed in May 13, 2025 OC operators meeting. Looking forward to your responses to the comments here. |
|
No major YANG version changes in commit 4c4f1f6 |
|
obsoleted by #1343 |
Add BGP Monitoring Protocol (BMP) Model
Change Scope
• This PR adds a new YANG model for BGP Monitoring Protocol (BMP) as defined in RFC 7854. The model provides configuration and operational state for BMP at global, neighbor, and peer-group levels with support for AFI-SAFI granularity.
• This change is backwards compatible as it only adds new nodes through augmentation of the existing BGP model without modifying any existing structures.
Platform Implementations
Implementation output:
Implementation output:
Additional Information
The BMP model includes support for: