Review of draft-ietf-bfd-yang-09.txt. * General comments - Having requirements language below the abstract looks like a novel idea, I assume the RFC editor will edit this. Also note that nowadays authors are usually expected to cite RFC 8174 as well with the extended boilerplate text. - Update 2017 to 2018 in copyright statements etc. - References to RFC 7223, RFC 7277, RFC 8022 should be updated to references to the I-Ds replacing them (sitting in the RFC editor queue). This may also involve changes in the YANG model. - State whether the model is NMDA compliant (which it likely should be), see also previous item. - I am not sure why you want to cite I-D.dsdt-nmda-guidelines. Would it not make more sense to cite the NMDA specification? - There are some YANG validation errors that should be addressed (see the link on the datatracker). - References YANG modules must be in the references and there must be citations in the text, hence there is the common phrase "This YANG module imports [RFCwxyz] and ...." - We generally prefer reference "RFC 6991: Common YANG Data Types" over reference "RFC 6991" since not everybody remembers all the RFC numbers (add the RFC title after the RFC number, separated by a colon). In some places you actually use the syntactic format but you do not use the RFC title. Please make this consistent, following the usual conventions. - I have raised a question on yang-doctors concerning the pattern import ietf-inet-types { prefix "inet"; reference "RFC 6991"; } and whether this should perhaps be import ietf-inet-types { prefix inet; reference "RFC 6991: Common YANG Data Types (at the time of this writing)"; } * Design of the Data Model - Do I always have to use schema mount to use these YANG models? If so, one might consider I-D.ietf-netmod-schema-mount a normative reference. Are you not augmenting the routing model? - I do not understand the explanations how the groupings solve the problem that IGPs are moving targets (they come and go). How do the groupings help the operator to configure BFD parameters for peers they do not know about yet? - How does a client know which choices of the "min-interval", used for both transmit and receive intervals, and "desired-min-tx- interval" and "required-min-rx-interval" are supported by an implementation? - The phrase 'operational model' probably means 'operational state model' and 'operational items' probably means 'operational state data'. - You have summary information and detailed BFD session information. What would an implementation report if say access to some BFD sessions is restricted by access control? Would information about them still leak through the summary information? I assume so that this may be practically the way to do things but perhaps this needs to be mentioned in the security considerations. - In 2.3, you use 'clients of BFD' but I think this is very different from 'BFD clients'. Please clarify the terminology. - s/operational data/operational state data/g - The *-count leafs seem to be gauge32, should yang-types:gauge32 be used? - There are also real counters that should probably use yang-types:counter32 and yang-types:counter64 instead of uint32 and uint64. - Is [I-D.ietf-mpls-base-yang] not a normative reference? See text in 2.11.3. - Is [I-D.ietf-teas-yang-te] not a normative reference? See text in 2.11.4. * IANA BFD YANG Module - Fix the phrase "a collection of YANG data types considered defined by IANA". - Does IANA understand how the typedefs diagnostic and auth-type is to be managed? Does this relate to an existing registry? Or is this establishing a diagnostic registry? Should the last paragraph in more clearly spell out that any changes to the existing registry must lead to an update of the YANG module and that updates to the YANG module are not allowed without an update to the other registries? The current wording "intended to reflect" seems vague. Should the description text for the typedefs make an explicit reference to the IANA registry for these number spaces? * BFD types YANG Module - The statement "This module contains a collection of BFD specific YANG data type definitions" seems wrong since you define way more things that just datatypes. In fact, ietf-bfd-types is kind of a misnomer; perhaps this should be ietf-bfd-common (the -common was used in RFC 8194 but I am biased here). - s/Two interval values or 1 value/Two interval values or one value/ - I think this is unclear (for me): leaf down-count { type uint32; description "Session Down Count."; } Is this a counter counting how many times the BFD session was down? The terse description does not tell. If this is a counter, then make it clear by using yang:counter32: leaf down-count { type yang:counter32; description "The number of times a BFD session transitioned into the down state."; } Please describe clearly what is being counted. Perhaps my interpretation is wrong, then please insert the correct statement. Note that this comment also applies to several of the subsequent definitions. - Clarify counter relationships. Right now, I assume that admin-down-count is included in down-count (but my interpretation may also be wrong). Same for received/send and received/send bad packets. - You have a container session-statistics and a grouping session-statistics and it seems they count very different things, the first seems to have statistics of stuff happening within a session (per session statistics) and the later seems to have statistics across all sessions. This is a bit unfortunate, if you search of session statistics you find stuff that leaves you puzzled. Please avoid this name clash and also make sure the description of the leafs makes it clear whether it is a per session leaf or a leaf for all sessions. - The *count leafs in "session-statistics" (ha) seem to be of type yang:gauge32, i.e., I would write something along these lines: leaf sessions { type yang:gauge32; description "Number of BFD sessions."; } leaf sessions-up { type yang:gauge32; description "Number of BFD sessions that are up."; } [...] * BFD IP multihop YANG Module - This is indented differently. Well the RFC editor will fix I guess. * Examples - I have not validated the examples. I do not know whether the IETF tooling is meanwhile able to do this - likely not. Did the authors confirm that they automatically validate the examples? Well, looking at the namespaces, likely not (the augmentations do not live in the ietf-routing namespace). So these examples need to be validated and fixed. I have used yanglint for this, works better for me than the pyang solutions, but the authors should figure out what works for them. - There are special IP address blocks for examples; the IPv6 address you show seems to belong to APNIC... * Security Considerations - Needs to be updated to the latest boilerplate. - You should discuss security properties of objects, there is more work to do here. * Appendix A - I do not understand what is going on here, I think this needs a bit more explanatory text. Why is the informal description of the two parameters more detailed than the description in the example YANG module? I suggest to have a proper description in the YANG module only. * Appendix B - What is an area-id? The description is not helpful. - list interface [...] description "List of interfaces" is not really useful.