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

[Jürgen Schönwälder <[e-mail]>]

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.