feat: Implement BFD for BGPPeers - #438
Conversation
3400add to
37e3c78
Compare
| SrcIf string `json:"srcIf,omitempty"` | ||
|
|
||
| // BFD enablement | ||
| PeerControl string `json:"ctrl"` |
There was a problem hiding this comment.
I don't see this field at all in the YANG model. Should we delete it?
There was a problem hiding this comment.
I somehow have in my had, this was needed to activate the BFD feature.
This should be the field:
https://github.com/YangModels/yang/blob/559418adbd5f05860c36b1d2ffa590ef8ee0309e/vendor/cisco/nx/10.6-2/Cisco-NX-OS-device.yang#L48686-L48690
| } | ||
| if peer.BFD.RequiredMinimumReceive != nil { | ||
| ms := peer.BFD.RequiredMinimumReceive.Duration | ||
|
|
There was a problem hiding this comment.
weird formatting.
37e3c78 to
df40292
Compare
Signed-off-by: Sven Rosenzweig <sven.rosenzweig@sap.com>
df40292 to
5c9c1e1
Compare
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
| SrcIf string `json:"srcIf,omitempty"` | ||
|
|
||
| // BFD enablement | ||
| PeerControl PeerControlType `json:"ctrl,omitempty"` |
There was a problem hiding this comment.
Do we really want this to be omitempty? Does this not have a platform default value that is always present? If not and it's completely omitted from the json payload, than we should rather use the Option[T] type we have in this nxos package.
| if err != nil { | ||
| return err | ||
| } | ||
| pe.PeerControl = PeerControlTypeBFD |
There was a problem hiding this comment.
If this field is only conditionally present, it should rather use the Option type than be omitempty.
| return err | ||
| } | ||
| pe.PeerControl = PeerControlTypeBFD | ||
| pe.BfdMultihop = bfd |
There was a problem hiding this comment.
As this field is only set, when bfd is in the spec and never set or reset otherwise, please add a comment that this will not work with a gnmiext.Client.Patch call. The reason is that bfd might have been configured outside of the network operator and thus, the "mhbfdintvl-items" entry is present. However, as we don't set it (because the spec doesn't specify it), we would unmarshal the response and detect a drift/diff on every reconcile, making gnmi set calls over and over again to Patch the configuration. Currently this is not the case, as we are Updating/Replacing the configuration, however with a Patch we would have a problem here. So I would prefer if we document this in a little comment.
| SrcIf string `json:"srcIf,omitempty"` | ||
|
|
||
| // BFD enablement | ||
| PeerControl PeerControlType `json:"ctrl,omitempty"` |
There was a problem hiding this comment.
I don't see this field being present in the payload at all:
leaf1(config-router-neighbor)# sh run | sec '^router bgp'
router bgp 65000
neighbor 10.1.1.1
bfd multihop interval 999 min_rx 999 multiplier 10$ gnmic --skip-verify get --type config --values-only --path 'System/bgp-items/inst-items/dom-items/Dom-list[name=default]/peer-items/Peer-list[addr=10.1.1.1]'
[
[
{
"addr": "10.1.1.1",
"adminSt": "enabled",
"asnType": "none",
"bfdType": "none",
"bmpSrvId1St": "disabled",
"bmpSrvId2St": "disabled",
"capSuppr4ByteAsn": "disabled",
"dscp": "cs6",
"epe": "disabled",
"holdIntvl": 180,
"internalVpnClient": "disabled",
"kaIntvl": 60,
"logNbrChgs": "none",
"lowMemExempt": "disabled",
"mhbfdintvl-items": {
"minRxMs": 999,
"minTxMs": 999,
"multiplier": 10
},
"passwdType": "LINE",
"peerType": "fabric-internal",
"privateASctrl": "none"
}
]
]|
|
||
| pe.BfdType = BfdTypeNone | ||
| if req.BGPPeer.Spec.BFD != nil && req.BGPPeer.Spec.BFD.Enabled { | ||
| bfd, err := NewBGPNeighborBfd(req.BGPPeer.Spec.BFD) |
There was a problem hiding this comment.
You have to make sure that the bfd feature is also enabled here, otherwise the gnmi call will fail. I assume bfd was already enabled elsewhere on the device you were testing against?
You can just take the logic for enabling bfd from the EnsureInterface func.
2 participants
No description provided.