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

[Benoit Claise <[e-mail]>]

Hi Jürgen,
> 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?
I do look at the YANG module dependencies, thanks to tooling.
Let's look at the draft-ietf-pim-yang-12 dependencies:

    https://www.yangcatalog.org/yang-search/impact_analysis.php?modules[]=[e-mail]&modules[]=[e-mail]&modules[]=[e-mail]&modules[]=[e-mail]&modules[]=[e-mail]&recurse=2&rfcs=1&show_subm=1&show_dir=dependencies

[I-D.ietf-rtgwg-routing-types] is RFC 8294. We will update the tool.
[I-D.ietf-netmod-rfc7223bis] is on the Jan 11th IESG telechat.
[I-D.ietf-netmod-rfc8022bis] is in IETF LC now and on the Jan 25th IESG 
telechat
The only unknown to me is the state of [I-D.ietf-bfd-yang]. Copying Alia 
and the BFD chairs.

In terms of I-D normative references, 
https://tools.ietf.org/html/draft-ietf-pim-yang-12#section-9.1, there is 
an extra related one:
     [I-D.ietf-netmod-revised-datastores] is on the Jan 11th IESG telechat

So we're getting there...

Regards, Benoit
>
>    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