Re: [Yang-multicast] Proposed updates to draft-ietf-pim-yang

[Xufeng Liu <[e-mail]>]

The YANG files and draft files are checked into Github: https://github.com/mcallisterjp/pim-yang

Thanks,
- Xufeng

From: Xufeng Liu
Sent: Friday, February 16, 2018 11:59 PM
To: '[e-mail]' <[e-mail]>
Subject: Proposed updates to draft-ietf-pim-yang

Attached please find the proposed updates to address YANG doctor’s review comments.

Authors and contributors,
It would be appreciated if you can review and respond within a week. ADs and shepherd have been requesting, and I promised the update next week.

The followings are the proposed replies.

Thanks,
- Xufeng

=========================================

Wed 12/20/2017 4:22 PM
Jürgen Schönwälder [e-mail]<mailto:[e-mail]>
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,”
Because such an implementation is not common, the consensus was to leave it to the vendor augmentation if needed.

* 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 some 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]: Added.
  - 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 route 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, will double check with the working group. The periods of these messages are usually at the order of seconds per interface. The counters do not seem to need 64 bit. Can we change 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.