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

[Xufeng Liu <[e-mail]>]

Hi Mahesh,
Thanks for the good observations. We have updated the model according to your suggestions, with an updated revision https://tools.ietf.org/html/draft-ietf-pim-yang-15.

Best regards,
- Xufeng

> -----Original Message-----
> From: Mahesh Jethanandani [mailto:[e-mail]]
> Sent: Tuesday, January 2, 2018 4:22 PM
> To: Juergen Schoenwaelder <[e-mail]>
> Cc: YANG Doctors <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [yang-doctors] Yangdoctors last call review of draft-ietf-pim-yang-
> 12
> 
> 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 <j.schoenwaelder@jacobs-
> university.de> 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]