-
Notifications
You must be signed in to change notification settings - Fork 657
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
Addition of BFD sessions subtree under interface in openconfig-bfd.yang #1137
base: master
Are you sure you want to change the base?
Conversation
Creation of a BFD single-hop session subtree at interface level, so that BFD sessions can be created besides routing protocols, as stated in RFC-9127 (https://datatracker.ietf.org/doc/rfc9127/)
Fix typo "single-hope" to "single-hop"
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 for the well-formed submission. Generally, looks reasonable to me, assuming I understand the context (question below).
There's one implementation listed here -- are there others that allow static non-protocol-related single hop sessions to be configured?
We should probably clarify what happens if both the per-protocol BFD enabled leaves are used as well as this configuration.
Update description of contatiner interface/sessions to specify that these are statically configured single-hop sessions. Also changed session/config/enabled leaf to session/config/admin-down to be more especific.
Q1: yes, after some search I've found a CLI configuration example for Huawei system at this link in section Configuring Single-Hop BFD. Q2: Probably system checks for two concurrent sessions if source and dest addresses are the same and if so, prioritize the session that was firstly established. |
/gcbrun |
No major YANG version changes in commit 72c371e |
Fix issues in openconfig-bfd.yang sessions subtree under interfaces and add revision date.
Update reference number for revision date
/gcbrun |
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.
Couple of small changes, and then LGTM.
Leaving to @dplore to ensure last-call process.
@@ -26,7 +26,14 @@ module openconfig-bfd { | |||
"An OpenConfig model of Bi-Directional Forwarding Detection (BFD) | |||
configuration and operational state."; | |||
|
|||
oc-ext:openconfig-version "0.3.0"; | |||
oc-ext:openconfig-version "0.3.1"; |
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.
Since we're adding a feature here, let's make this be a minor release - i.e., this should be 0.4.0
.
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.
@Pull-eckermann please update version to 0.4.0. Thanks!
the single-hop BFD session specified."; | ||
} | ||
|
||
leaf admin-down { |
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.
We've generally used enabled
as this leaf's name -- and have zero called admin-down
, so let's go for some consistency here and use enabled
.
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.
This was also raised some time ago whether or not this should have been corrected for those that veered but current stats look like:
$ find . -name "*.yang" | xargs grep -E "leaf enable(d?) {" | awk '{print $3}' | sort | uniq -c
15 enable
51 enabled
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.
@Pull-eckermann please rename this leaf to enabled
} | ||
description | ||
"A reference to the remote-address for the BFD single-hop | ||
session"; |
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.
session"; | |
session."; |
} | ||
description | ||
"A reference to the local-address for the BFD single-hop | ||
session"; |
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.
session"; | |
session."; |
@Pull-eckermann could you please elaborate on the original problem statement?
I would disagree with that interpretation of the RFC9127 and BFD protocol behavior for a number of reasons. Firstly, the protocol behavior is defined in RFCs 5880-5882. RFC5880, section 3.1:
RFC5881 tells us that the session must be associated with a protocol:
RFC5882 tells us that the protocol is expected to bootstrap the session:
In addition,
Also, probably you want to use RFC9314 instead of 9127 as a reference. To summarize, while technically the rfc9127 yang model does not prevent the creation of the config as you describe, I don't believe it is designed for the purpose you stated. To me, it seems that you're trying to "standardize" a non-standard implementation that requires a fixed provisioning of the destination IP via this pull request. I disagree with the proposal in its current form. |
@Pull-eckermann it seems @LimeHat has a point regarding the intent of the RFC. However, OC doesn't need to strictly follow this. If there is operational need and implementation precedent for configuration of BFD session IP addresses, (and not just built-in, device protocol based discovery auto session IP address assignment), then this could be added to openconfig. I see one reference to ipInfusion configuration of BFD session ip addresses . Can you find at least one more implementation that supports this kind of configuration? |
Creation of a BFD single-hop session subtree at interface level, so that BFD sessions can be created besides routing protocols, as stated in RFC-9127.
Change Scope
Platform Implementations