Hi Jürgen, Thank you much for the review and comments. We have posted the updated draft https://tools.ietf.org/html/draft-ietf-pim-yang-14 to address the comments. Thanks, - Xufeng > -----Original Message----- > From: Jürgen Schönwälder [mailto:j.schoenwaelder@xxxxxxxxxxxxxxxxxxxx] > Sent: Wednesday, December 20, 2017 4:22 PM > To: yang-doctors@xxxxxxxx > Cc: draft-ietf-pim-yang.all@xxxxxxxx; ietf@xxxxxxxx; pim@xxxxxxxx > Subject: Yangdoctors last call review of draft-ietf-pim-yang-12 > > Reviewer: Jürgen Schönwälder > Review result: Almost Ready > > * 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. [Xufeng] Removed > > 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). [Xufeng] Removed the unnecessary groupings. > > * 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? [Xufeng] As suggested, moved the YANG version to 1.1, so that we have removed reference to RFC6020. > > - What are 'wider' management interfaces? If you mention NETCONF, > why not mention RESTCONF? [Xufeng] “RESTCONF” is mentioned before this sentence: “using network management protocols such as NETCONF [RFC6241] or RESTCONF [RFC8040]”. > > - s/Protocol-Independent Multicast (PIM) devices > /devices supporting Protocol-Independent Multicast (PIM)/ [Xufeng] Changed. > > - So which YANG terminology applies, RFC 6020 or RFC 7950? I > personally think using YANG 1.1 is pretty safe these days. [Xufeng] As suggested, moved the YANG version to 1.1, so that we have removed reference to RFC6020. > > * 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? [Xufeng] Clarified the deviation statement with clear location and detailed example. > How many of such must expressions > would there have to be? [Xufeng] Depending on the implementation, there can be five such configurable attributes: dr-priority, hello-interval, jp-interval, propagation-delay, and override-interval. > Did you consider having address family > independent objects and optionally (controlled by a feature) per > address family objects that overwrite the independent settings? [Xufeng] Yes. It is listed as one of the options, i.e. “For these implementations, the restriction that interface configuration must be address-family independent may either be expressed as a vendor augmentation of an address-family-independent parameter above the address-family level,” > > * Module Structure > > - s/is included/is imported/ [Xufeng] Fixed. > > - 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. [Xufeng] Section 3 now describes the tree in smaller units. According to the mailing list discussion in NETMOD WG, we have also included the un-altered full tree in Section 4 for reference. > > 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. [Xufeng] Yes for extensibility. Other modules, such as pim-sm and pim-bidir augment this list item to add more attributes. > > - 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. [Xufeng] As suggested, moved the YANG version to 1.1. It is a good point that ietf-routing already uses YANG 1.1. > > - Consider adding reference statements to the feature definitions > in case a feature is described in a protocol specification. [Xufeng] Added references. > > - The value of timer-value is not really clear, you could have used > rt-types:timer-value-seconds16 directly. [Xufeng] Changed as suggested. > > - 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? [Xufeng] Right. > > - Does a bfd/hello-interval of 'infinity' or 'not-set' make sense? [Xufeng] Yes. If 'infinity' or 'not-set' is used, no periodic Hello messages are sent. Clarified in the description. > > - 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... [Xufeng] We cannot have both holdtime and multiplier. Holdtime is timer value to time out the neighbor state when the timer expires. The holdtime value can be specified either by the given holdtime value or by the calculation of the hello-interval multiplied by the given value of the multiplier. Clarified in the description. > > - Does a bfd/jp-interval of 'infinity' or 'not-set' make sense? [Xufeng] Yes. If 'infinity' or 'not-set' is used, no periodic Join/Prune messages are sent. Clarified in the description. > > - 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... [Xufeng] We cannot have both holdtime and multiplier. Holdtime is timer value to time out the Join/Prune state when the timer expires. The holdtime value can be specified either by the given holdtime value or by the calculation of the jp-interval multiplied by the given value of the multiplier. Clarified in the description. > > - Please provide more meaningful descriptions: > > description "Propagation description"; > description "Override interval"; [Xufeng] Clarified. > > - 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. [Xufeng] It is the latter case. This is more specific to multicast or PIM packets. Added more clarifications in the description. > > - How do the ip4 and ipv6 addresses relate to ip addresses assigned > to an interface in ietf-ip? [Xufeng] The addresses are from those assigned to the interface (operational state), but they are the addresses on which PIM is operating. Clarified in the description. > > - What is the meaning of hello-expiration 'not-set'? [Xufeng] ‘not-set’ means that the timer is not running. In this case, it is effectively the same as 'infinity', which means that the timer is running but never expires. If such a timer value is defined in the message format, ‘not-set’ will cause the implementation not to put the timer object into the message, while ‘infinity’ will cause the implementation to put 0xffff as the timer value. > > - What is the meaning of expiration 'not-set'? [Xufeng] As above. > > - 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? [Xufeng] Right. It is the case that people are too used to the uptime. We have updated the description. > > - 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? [Xufeng] RFC7761 Sec 4.3.1 and 4.3.2 describe it as a 32-bit unsigned number, so we will keep the full range. Added the default value 1 to the model. > > - 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? [Xufeng] Added description to the error container. Right. if I receive a hello and I later decide that this hello must have been an error, is this hello counted twice: both in error counter and in received counter. You have raised a good point about the format error, which was missing in the model and has been added. > > - Why is 'pim' a presence container? [Xufeng] The presence container enables the PIM protocol. Clarified in the statement description. > > - Do comments like 'configuration data nodes' make sense if you > include config false nodes in the same branch of the tree? [Xufeng] Removed the term “configuration” here. We used to have “configuration” and “operational”, but when we removed the “operational” branch, we forgot to update the comments. > > * PIM RP Module > > - Does the feature bsr-election-state depend on the feature bsr? [Xufeng] Yes. The container that uses the feature bsr-election-state is a sub-container of bsr (that uses the feature bsr). Is the arrangement good enough? To clarify, we have added “if-feature bsr” to the feature bsr-election-state. > > - Should there be a default bsr-candidate/priority? [Xufeng] Yes. Added a default value 64, and removed the mandatory requirement. > > - Do you need the address/hash-mask-length/priority in > bsr-state-attributes in an NMDA implementation? [Xufeng] We feel yes. There are two distinctive concepts here: candidate-BSR and elected BSR. The candidate-bsr container specifies if this router wants to be a candidate along with used parameters. The bsr container shows the election result, providing the information of elected bsr (which may be this route, if this router wants to be a candidate, or one of other remote candidate routers). > > - 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? [Xufeng] You are right. It is the remaining time interval until the next bootstrap will be sent. It is a timer with short periodic interval like 60 sec. Operator usually checks its state to get a sense of the protocol’s health. It is possible to use an absolute timestamp instead, but it may not bring more convenience, because the operator needs to do the conversion and it is not obvious to use the absolute timestamp to indicate the protocol healthiness. > > - I have no clue what to expect here: > > leaf group-policy { > type string; > description "Group policy."; > } > > What can I expect the string to contain? [Xufeng] Clarified the string value with the following: "The string value is the name to uniquely identify a policy that contains one or more policy rules used to accept or reject certain multicast groups. If a policy is not specified, the entire multicast address space is accepted. The definition of such a policy is outside the scope of this document."; > > - I am again uncertain how exactly to understand the value of > rp-candidate-next-advertisement, see similar questions above. [Xufeng] Clarified with "The remaining time interval in seconds until the next RP candidate advertisement will be sent."; > > - 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? [Xufeng] Clarified with the following: "The string value is the name to uniquely identify a policy that contains one or more policy rules used to determine which multicast group addresses are mapped to this statically configured RP address. If a policy is not specified, the entire multicast address space is mapped. The definition of such a policy is outside the scope of this document."; > > - 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>? [Xufeng] Clarified with the following: "The string value is the name to uniquely identify a policy that contains one or more policy rules used to accept or reject certain multicast groups. If a policy is not specified, the entire multicast address space is accepted. The definition of such a policy is outside the scope of this document."; > > * 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."; > } [Xufeng] Clarified with the following: "The string value is the name to uniquely identify a policy that contains one or more policy rules used to accept or reject certain multicast groups. The groups accepted by this policy have the SPT switchover threshold set to infinity, meaning that they will stay on the shared tree forever. If a policy is not specified, the entire multicast address space is accepted. The definition of such a policy is outside the scope of this document."; > > - What is the meaning of a range-policy value? > > leaf range-policy { > type string; > description > "Policy used to define SSM address range."; > } [Xufeng] Clarified with the following: "The string value is the name to uniquely identify a policy that contains one or more policy rules used to accept or reject certain multicast groups. The groups accepted by this policy define the multicast group rang used by SSM. If a policy is not specified, the default SSM multicast group rang is used. The default SSM multicast group range is 232.0.0.0/8 for IPv4 and ff3x::/96 for IPv6 where x reprents any valid scope identifier. The definition of such a policy is outside the scope of this document."; > > * PIM DM Module > > - I wonder, would you need an identity for dense mode? [Xufeng] I think that you mean an identity based on “identity rp-mode” in ietf-pim-rp.yang. No, we don’t need. PIM dense mode does not use RP, so that rp-model is not applicable. > > * PIM BIDIR Module > > - Remove > > /* > * Typedefs > */ [Xufeng] Done. > > - What is the meaning of offer-interval or backoff-interval > 'not-set'? [Xufeng] It is not valid to use “not-set” or “infinity” for these two parameters. Changed them to uint16. > > * 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? [Xufeng] The understanding is correct. We were requested to add this section to provide some background information to the reviewers to help the reviewing process. As for the counter sizes, we have discussed again and checked with the implementers. The periods of these messages are usually at the order of seconds per interface. The counters do not seem to need 64-bit. We have changed them to 32-bit. > > * 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. [Xufeng] Updated. >
