Last Call Review of draft-ietf-pim-yang-12
review-ietf-pim-yang-12-yangdoctors-lc-schoenwaelder-2017-12-20-00
Request | Review of | draft-ietf-pim-yang-11 |
---|---|---|
Requested revision | 11 (document currently at 17) | |
Type | Last Call Review | |
Team | YANG Doctors (yangdoctors) | |
Deadline | 2017-12-15 | |
Requested | 2017-11-27 | |
Requested by | Stig Venaas | |
Authors | Xufeng Liu , Pete McAllister , Anish Peter , Mahesh Sivakumar , Yisong Liu , fangwei hu | |
I-D last updated | 2017-12-20 | |
Completed reviews |
Yangdoctors Early review of -00
by Dean Bogdanović
(diff)
Yangdoctors Last Call review of -12 by Jürgen Schönwälder (diff) Rtgdir Telechat review of -12 by Adrian Farrel (diff) Opsdir Last Call review of -12 by Jürgen Schönwälder (diff) Genart Last Call review of -12 by Roni Even (diff) Secdir Last Call review of -12 by Melinda Shore (diff) |
|
Assignment | Reviewer | Jürgen Schönwälder |
State | Completed | |
Request | Last Call review on draft-ietf-pim-yang by YANG Doctors Assigned | |
Reviewed revision | 12 (document currently at 17) | |
Result | Almost ready | |
Completed | 2017-12-20 |
review-ietf-pim-yang-12-yangdoctors-lc-schoenwaelder-2017-12-20-00
* General YANG Review Remarks This document depends on a number of other I-Ds. Is it safe to process this document while the other documents this document depends on are not yet through the process? Who is tracking things and making sure that any changes in other documents that may impact the PIM document are detected and handled appropriately before publication? It is generally good style to write complete sentences in description statements. Some of the description statements are just fractions of a sentence. I think we do not recommend anymore to list WG chairs in the contact statement. It is sometimes not clear why you define groupings that are only used once (and sometimes are likely not reusable since they contain relative paths in must expressions). * Introduction - Is there a reason why you refer to RFC 6020 and to RFC 7950 in sections 1 and 1.2? Why do you need the reference to RFC 6020? - What are 'wider' management interfaces? If you mention NETCONF, why not mention RESTCONF? - s/Protocol-Independent Multicast (PIM) devices /devices supporting Protocol-Independent Multicast (PIM)/ - So which YANG terminology applies, RFC 6020 or RFC 7950? I personally think using YANG 1.1 is pretty safe these days. * Design of Data Model - I did not really understand Section 2.5. It seems you are duplicating objects for different address families but on some systems these duplicate objects must have the same value. If so, where would the must expression go and how does an implementation add such a must expression? How many of such must expressions would there have to be? Did you consider having address family independent objects and optionally (controlled by a feature) per address family objects that overwrite the independent settings? * Module Structure - s/is included/is imported/ - The tree diagrams are rather long. It would likely help readers if the diagram would be split into meaningful units and additional text added to describe the units. Are lists of the form | +--rw ipv4-rp* [ipv4-address] | | +--rw ipv4-address inet:ipv4-address designed for exensibility? Otherwise, this may be collapsed into a simple leaf-list. - Since I am not familiar with details of PIM, I can't comment on the question whether the model makes sense or not. * PIM base Module - YANG modules compile cleanly according to the tracker page. - As said above, consider using YANG 1.1. The ietf-routing module actually uses YANG 1.1 so you will need a YANG 1.1 compiler anyway. - Consider adding reference statements to the feature definitions in case a feature is described in a protocol specification. - The value of timer-value is not really clear, you could have used rt-types:timer-value-seconds16 directly. - Why is graceful-restart/duration using an ad-hoc type for 16-bit seconds and not timer-value? Is it because infinity and not-set does not apply? - Does a bfd/hello-interval of 'infinity' or 'not-set' make sense? - More explicit description of bfd/hello-holdtime? Is a choice really appropriate (hello-holdtime-or-multiplier)? Can I not have both holdtime and multiplier? Perhaps I am just not clear what holdtime does... - Does a bfd/jp-interval of 'infinity' or 'not-set' make sense? - More explicit description of bfd/jp-holdtime? Is a choice really appropriate (jp-holdtime-or-multiplier)? Can I not have both holdtime and multiplier? Perhaps I am just not clear what holdtime does... - Please provide more meaningful descriptions: description "Propagation description"; description "Override interval"; - What is the meaning of the interface augmentation 'oper-status' relative to 'oper-status' defined by ietf-interfaces? Is this just a duplicate with fewer states? Or is this somehow more specific to multicast or PIM packets? In the later case, I think this deserves to the be explained in the description clause. - How do the ip4 and ipv6 addresses relate to ip addresses assigned to an interface in ietf-ip? - What is the meaning of hello-expiration 'not-set'? - What is the meaning of expiration 'not-set'? - Is it useful to return the uptime in seconds (which is changing on every get that is not in the same second) or could it be an option to report the time when something transitioned into the up state (which is not changing)? Well, it could be that we are simply used to uptime like objects. Anyway, the description of up-time should make it clear what exactly is defining the state 'up'. If this says up for 5 seconds, what exactly transitioned into an 'up' state 5 seconds ago? - Is the any restriction for dr-priority or is it a full 32-bit unsigned number space? In some vendor documentation I saw 0..65535 with a default of 1. What do RFCs say? - I am not sure what the precise meaning of the error statistic counters are. What turns an received or sent messages into an error message? The description of grouping statistics-error does not explain this. Also, if I receive a hello and I later decide that this hello must have been an error, is this hello counted twice? And what about messages that could not be parsed because they were malformed, where are those counted? - Why is 'pim' a presence container? - Do comments like 'configuration data nodes' make sense if you include config false nodes in the same branch of the tree? * PIM RP Module - Does the feature bsr-election-state depend on the feature bsr? - Should there be a default bsr-candidate/priority? - Do you need the address/hash-mask-length/priority in bsr-state-attributes in an NMDA implementation? - I _assume_ the bsr-next-bootstrap value has to be interpreted relative to the time the value was obtained. What about making this an absolute timestamp instead? Well, actually I am not sure what the value represents - is it the remaining time interval until the next bootstrap will be sent? - I have no clue what to expect here: leaf group-policy { type string; description "Group policy."; } What can I expect the string to contain? - I am again uncertain how exactly to understand the value of rp-candidate-next-advertisement, see similar questions above. - What are the policy values that can go into this: leaf policy-name { type string; description "Static RP policy."; } Is the string just an arbitrary name or does it mean something? - How is this supposed to be used? leaf policy { type string; description "ACL (Access Control List) policy used to filter group addresses."; } What is the meaning of <policy>foo</policy>? * PIM SM Module - What is the meaning of a policy-name value? leaf policy-name { if-feature spt-switch-policy; type string; description "Switch policy."; } - What is the meaning of a range-policy value? leaf range-policy { type string; description "Policy used to define SSM address range."; } * PIM DM Module - I wonder, would you need an identity for dense mode? * PIM BIDIR Module - Remove /* * Typedefs */ - What is the meaning of offer-interval or backoff-interval 'not-set'? * Implementation Status It seems the trac page pointed to was used to collect information about what proprietary implementation support, i.e., it does not document to what extend the models defined in this document have been implemented. There are some notable differences. For example, it seems most counters implemented are 32-bit while most counters in the YANG models are 64-bit. How chatty is PIM, are 64-bit counters really needed and are implementors willing to move to 64-bit counters? * Security Considerations I think this needs to include a bit more details. See https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines for the latest template that YANG module authors are expected to use.