Jeff, Will echo Reshad's comment on the review. You should join the YANG doctors :-)
More inline > On Mar 24, 2017, at 7:05 PM, Reshad Rahman (rrahman) <[email protected]> > wrote: > > Thanks Jeff for review + detailed comments, please see inline. > > Co-authors, feel free to chime in. We¹ll meet to discuss in Chicago. > > >> On 2017-03-24, 12:14 PM, "Jeffrey Haas" <[email protected]> wrote: >> >> Authors, >> >> Thanks for continued work on the yang module. It does appear it's >> approaching a good level of maturity! >> >> I have a few comments from my recent read-through. I'd like to encourage >> other members of the Working Group to review the module. I'd also like to >> encourage the authors to reach out to other protocol groups to check the >> suitability of the module for interaction with their protocols. >> >> Some stream of thought comments: >> - TTL seems to be only a property of multi-hop sessions. How do you >> configure GTSM for single-hop sessions? > > <RR> TTL must be 255 as per section 5 of RFC5881? > >> - In single-hop, sessions are operationally keyed on "interface". >> However, >> there's also a "out-interface" node. How are they intended to be >> different? > > <RR> This is for the case where ³interface² is a virtual interface, > ³out-interface² would indicate the physical interface. Thinking more about > this, we should consider taking it out of the base model, vendors who want > to support this can do augment. Agree that this should be an augmentation. > >> - state-change-reason in notification is a string type. While this is >> probably the fastest path to getting some code written, it's also the >> least machine usable mechanism. Has there been any discussion among the >> authors on how that might otherwise be addressed? > > <RR> You¹re the first one to bring it up :-) So you¹d like us to e.g. look > into defining an identity for this? > >> - time-in-previous-state is oddly a string. Why not a time of some sort? > > <RR> I agree. > >> - Future-proofing observation: An mpls-fec is not required to be an IP >> address, although that is pretty much its only major manifestation at >> the >> moment. This may cause weirdness down the road. > > <RR> So are you suggesting we use a choice too cater for this? > >> - mpls-fec is of type ip-address. FECs are typicall prefixes. > > <RR> Ack. > >> - bfd-diagnostic is missing code point 9. (See IANA issues.) > > <RR> Ack. > >> - Suggest adding the range restriction 0..31 to the description for >> bfd-diagnostic. > > <RR> Ack. > >> - Naming elements such as nbor, conc, etc. are rather unnecessarily >> truncated. While I realize that yang is somewhat human readable, it's >> mostly for machine interaction. Reasonably expanded names make a module >> easier to read, especially to English as a second language parties. >> This, >> however, needs to be balanced against things like "admin" which is >> already >> present in other BFD documents. > > <RR> Ack. > >> - In the bfd-auth-replay-protection description, please include citations >> to >> the specific section of RFC 5880. > > <RR> Ack. > >> - The bfd-all-session grouping includes source-port and dest-port. This >> may >> not be relevant to all transports of BFD. What discussion have you >> given >> to restricting this, if any? > > <RR> Good point. You¹re referring to encaps such as ACH I assume? > >> - rx-ttl's description is a bit confusing to read. I think you are trying >> to suggest that the received TTL should be <= this value to be >> acceptable? > > <RR> >= from what recall. We¹ll discuss this amongst authors. > >> - potential nit on "ttl"; ipv6 calls this hop-count. Any discussion on >> the >> naming of this type among the authors? > > <RR> None but we will. > >> >> >> Features: >> Echo and demand mode are not pervasively implemented for BFD. (I've not >> actually heard of an implementation of demand mode.) I suspect these >> elements should be covered by if-feature? While I realize the elements >> are >> optionally usable in the configuration leaves, the lack of a feature might >> lead a netconf implementation with the thought that they can use this >> feature. What has been the author discussion on this point? > > <RR> We have had some discussions on this. I agree that we should add > if-feature. Authors will discuss. Agree. > >> >> IANA management of some module components: >> Several elements in this draft are things that will regularly need to be >> maintained. Examples include: >> - bfd-path-type >> - bfd-auth-replay-protection >> - bfd-diagnostic >> >> I suspect we wish to place these components in a module/modules that are >> IANA maintained. I'm a bit behind in current yang practices, but I >> suspect >> we have something like this defined now. It unfortunately has the >> headache >> that it impacts name-spacing issues for the consuming modules. > > <RR> Hadn¹t thought of that. I suspect BFD isn¹t the first module to need > thisŠ. While we need to put the module name in IANA list of modules for namespace reasons, I am not sure of individual elements. Are these values not already maintained by IANA? > >> >> Timing hints: >> Has any discussion been given to an operational module that might suggest >> what timer values are acceptable to an implementation? Note: I'm not >> suggesting this as part of the core module, but it's a common "what timers >> does your implementation support" question that typically happens. > > <RR> No discussions on this. We¹ll have to discuss. What is tricky with > this is that some implementations support different timer ranges based on > ³path-type². E.g. 5 ms mayb be supported for single-hop but for multi-hop > the minimum could be 150 ms. I seem to remember an effort to define the most commonly used timeout values. If so, we could define them in the model, leaving individual implementations to add their own preferred values. Mahesh Jethanandani [email protected] > > Regards, > Reshad. > >> >> >>> On Fri, Mar 10, 2017 at 09:29:45PM +0000, Reshad Rahman (rrahman) wrote: >>> Main changes from rev 04 are: >>> - Removed augment of network-instance, added reference to schema-mount >>> - bfd is not top-level anymore, augments control-plane-protocol from >>> RFC8022 >>> - Addeds section on Interaction with other YANG modules >>> >>> >>> >>> Regards, >>> BFD YANG authors. >>> >>> On 2017-03-10, 4:26 PM, "Rtg-bfd on behalf of [email protected]" >>> <[email protected] on behalf of [email protected]> wrote: >>> >>>> >>>> A New Internet-Draft is available from the on-line Internet-Drafts >>>> directories. >>>> This draft is a work item of the Bidirectional Forwarding Detection of >>>> the IETF. >>>> >>>> Title : Yang Data Model for Bidirectional Forwarding >>>> Detection (BFD) >>>> Authors : Reshad Rahman >>>> Lianshu Zheng >>>> Santosh Pallagatti >>>> Mahesh Jethanandani >>>> Greg Mirsky >>>> Filename : draft-ietf-bfd-yang-05.txt >>>> Pages : 53 >>>> Date : 2017-03-10 >>>> >>>> Abstract: >>>> This document defines a YANG data model that can be used to >>> configure >>>> and manage Bidirectional Forwarding Detection (BFD). >>>> >>>> >>>> >>>> The IETF datatracker status page for this draft is: >>>> https://datatracker.ietf.org/doc/draft-ietf-bfd-yang/ >>>> >>>> There's also a htmlized version available at: >>>> https://tools.ietf.org/html/draft-ietf-bfd-yang-05 >>>> >>>> A diff from the previous version is available at: >>>> https://www.ietf.org/rfcdiff?url2=draft-ietf-bfd-yang-05 >>>> >>>> >>>> Please note that it may take a couple of minutes from the time of >>>> submission >>>> until the htmlized version and diff are available at tools.ietf.org. >>>> >>>> Internet-Drafts are also available by anonymous FTP at: >>>> ftp://ftp.ietf.org/internet-drafts/ >
