Re: [pim] [yang-doctors] Yangdoctors last call review of draft-ietf-pim-yang-12

[Mahesh Jethanandani <[e-mail]>]

Few other observations.

Is the YANG model description statement not supposed to carry the IETF copyright statement?

There is already a enum definition for BFD state called “state” in the BFD types file imported by the module. Why is that not being used in this model?

> On Dec 20, 2017, at 1:22 PM, Jürgen Schönwälder <[e-mail]> wrote:
> 
> 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.
> 
>  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.
> 
> 
> _______________________________________________
> yang-doctors mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/yang-doctors

Mahesh Jethanandani
[e-mail]