Subject: Re: Yangdoctors last call review of draft-ietf-bfd-yang-09

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Sun, 4 Mar 2018 14:12:30 +0000

Hi Jurgen,

We have made the changes in revs 10 and 11 to address your comments . The exception is module ietf-bfd-types which did not get renamed per reason below.

Regards,
Reshad.

On 2018-02-25, 11:28 AM, "Reshad Rahman (rrahman)" <[email-masked]> wrote:

    Hi,
    
    Regarding the following, making the change is easy but ietf-bfd-types is used by other modules so this will impact these other modules. E.g. the RIP draft (https://datatracker.ietf.org/doc/draft-ietf-rtgwg-yang-rip/) is currently in the RFC Editor queue, I presume it's too late then to make this name change?
    
    Regards,
    Reshad.
    
            * BFD types YANG Module
            
              - <snip> In fact, ietf-bfd-types is kind of a
                misnomer; perhaps this should be ietf-bfd-common (the -common was
                used in RFC 8194 but I am biased here).
        <RR> Will rename to -common

    On 2018-02-23, 11:53 AM, "Reshad Rahman (rrahman)" <[email-masked]> wrote:
    
        Hi Jürgen,
        
        Thank you for the review, response inline.
        
        On 2018-02-15, 4:35 AM, "Jürgen Schönwälder" <[email-masked]> wrote:
        
            Reviewer: Jürgen Schönwälder
            Review result: Not Ready
            
            Review of draft-ietf-bfd-yang-09.txt.
            
            * General comments
            
              - Having requirements language below the abstract looks like a novel
                idea, I assume the RFC editor will edit this. Also note that
                nowadays authors are usually expected to cite RFC 8174 as well
                with the extended boilerplate text.
        <RR> Will make the changes.
            
              - Update 2017 to 2018 in copyright statements etc.
        <RR> Will make the changes.
        
              - References to RFC 7223, RFC 7277, RFC 8022 should be updated to
                references to the I-Ds replacing them (sitting in the RFC editor
                queue). This may also involve changes in the YANG model.
        <RR> Will make the changes. I don't think this will involve changes in the YANG model, but will pay attention to this.
            
              - State whether the model is NMDA compliant (which it likely should
                be), see also previous item.
        <RR> This is mentioned in section 2. I will look into moving this to the intro and changing the text to clarify.
            
              - I am not sure why you want to cite I-D.dsdt-nmda-guidelines. Would
                it not make more sense to cite the NMDA specification?
        <RR> Will make the change. By NMDA specification, you're referring to draft-ietf-netmod-revised-datastores?
            
              - There are some YANG validation errors that should be addressed (see
                the link on the datatracker).
        <RR> Those were in 8022bis I believe, gone now.
            
              - References YANG modules must be in the references and there must
                be citations in the text, hence there is the common phrase "This
                YANG module imports <bla> [RFCwxyz] and ...."
            
              - We generally prefer
            
                  reference "RFC 6991: Common YANG Data Types"
            
                over
            
                  reference "RFC 6991"
            
                since not everybody remembers all the RFC numbers (add the RFC
                title after the RFC number, separated by a colon). In some places
                you actually use the syntactic format but you do not use the RFC
                title. Please make this consistent, following the usual
                conventions.
        <RR> Will make the change. 
            
              - I have raised a question on yang-doctors concerning the pattern
            
                  import ietf-inet-types {
                    prefix "inet";
                    reference "RFC 6991";
                  }
            
                and whether this should perhaps be
            
                  import ietf-inet-types {
                    prefix inet;
                    reference "RFC 6991: Common YANG Data Types
                               (at the time of this writing)";
                  }
         <RR> Saw the discussion, will make the change once there is closure.
        
            * Design of the Data Model
            
              - Do I always have to use schema mount to use these YANG models? If
                so, one might consider I-D.ietf-netmod-schema-mount a normative
                reference. Are you not augmenting the routing model?
          <RR> I will clarify in the text. There was a follow-up email discussion on this, we are augmenting the routing model and schema mount can be used (but not mandatory).
          
              - I do not understand the explanations how the groupings solve the
                problem that IGPs are moving targets (they come and go). How do
                the groupings help the operator to configure BFD parameters for
                peers they do not know about yet?
        <RR> I will update the text. Basically as IGPs discover peers/neighbors, they create BFD sessions "internally" (i.e. via API, not via config).
            
              - How does a client know which choices of the "min-interval", used
                for both transmit and receive intervals, and "desired-min-tx-
                interval" and "required-min-rx-interval" are supported by an
                implementation?
         <RR> We'll think about this. We'll either add feature or make both mandatory.
           
              - The phrase 'operational model' probably means 'operational state
                model' and 'operational items' probably means 'operational state
                data'.
        <RR> Will make the change. 
            
              - You have summary information and detailed BFD session information.
                What would an implementation report if say access to some BFD
                sessions is restricted by access control? Would information about
                them still leak through the summary information? I assume so that
                this may be practically the way to do things but perhaps this
                needs to be mentioned in the security considerations.
        <RR> The summary information contains everything. I don't see what kind of security consideration is required if we know e.g. that we have so many sessions up and down.
            
              - In 2.3, you use 'clients of BFD' but I think this is very
                different from 'BFD clients'. Please clarify the terminology.
        <RR> Will use 'BFD clients' everywhere.
            
              - s/operational data/operational state data/g
        <RR> Yes
            
              - The *-count leafs seem to be gauge32, should yang-types:gauge32 be
                used?
        <RR> Yes
            
              - There are also real counters that should probably use
                yang-types:counter32 and yang-types:counter64 instead of uint32
                and uint64.
        <RR> Yes
            
              - Is [I-D.ietf-mpls-base-yang] not a normative reference? See text
                in 2.11.3.
        <RR> Yes
            
              - Is [I-D.ietf-teas-yang-te] not a normative reference? See text in
                2.11.4.
        <RR> Yes
            
            * IANA BFD YANG Module
            
              - Fix the phrase "a collection of YANG data types considered defined
                by IANA".
            
              - Does IANA understand how the typedefs diagnostic and auth-type is
                to be managed?  Does this relate to an existing registry? Or is
                this establishing a diagnostic registry? Should the last paragraph
                in more clearly spell out that any changes to the existing
                registry must lead to an update of the YANG module and that
                updates to the YANG module are not allowed without an update to
                the other registries? The current wording "intended to reflect"
                seems vague. Should the description text for the typedefs make an
                explicit reference to the IANA registry for these number spaces?
        <RR> This is for an existing registry. I will see how to modify the text.
            
            * BFD types YANG Module
            
              - The statement "This module contains a collection of BFD specific
                YANG data type definitions" seems wrong since you define way more
                things that just datatypes. In fact, ietf-bfd-types is kind of a
                misnomer; perhaps this should be ietf-bfd-common (the -common was
                used in RFC 8194 but I am biased here).
        <RR> Will rename to -common
            
              - s/Two interval values or 1 value/Two interval values or one value/
        <RR> Yes
            
              - I think this is unclear (for me):
            
                  leaf down-count {
                     type uint32;
                     description "Session Down Count.";
                   }
            
                Is this a counter counting how many times the BFD session was
                down? The terse description does not tell. If this is a counter,
                then make it clear by using yang:counter32:
            
                  leaf down-count {
                     type yang:counter32;
                     description
                       "The number of times a BFD session transitioned into
                        the down state.";
                   }
            
                Please describe clearly what is being counted. Perhaps my
                interpretation is wrong, then please insert the correct statement.
                Note that this comment also applies to several of the subsequent
                definitions.
        <RR> Yes it is for number of times the session has gone down. I will look at the subsequent descriptions also and will add clarifications.
            
              - Clarify counter relationships. Right now, I assume that
                admin-down-count is included in down-count (but my interpretation
                may also be wrong). Same for received/send and received/send bad
                packets.
        <RR>  admin-down-count is not included in down-count. For packet counters, yes the bad packet count is included in total packet count. Will clarify.
            
              - You have a container session-statistics and a grouping
                session-statistics and it seems they count very different things,
                the first seems to have statistics of stuff happening within a
                session (per session statistics) and the later seems to have
                statistics across all sessions. This is a bit unfortunate, if you
                search of session statistics you find stuff that leaves you
                puzzled. Please avoid this name clash and also make sure the
                description of the leafs makes it clear whether it is a per
                session leaf or a leaf for all sessions.
        <RR> Correct. I'll rename the container which is "across sessions" to summary or session-summary.
            
              - The *count leafs in "session-statistics" (ha) seem to be of
                type yang:gauge32, i.e., I would write something along these
                lines:
            
                   leaf sessions {
                     type yang:gauge32;
                     description "Number of BFD sessions.";
                   }
            
                   leaf sessions-up {
                     type yang:gauge32;
                     description "Number of BFD sessions that are up.";
                   }
            
                   [...]
        <RR> Yes the "count" in session-statistics are actually gauges and should be changed (and -count taken out from the name).
         
            * BFD IP multihop YANG Module
            
              - This is indented differently. Well the RFC editor will fix I
                guess.
        <RR> Will fix.
            
            * Examples
            
              - I have not validated the examples. I do not know whether the IETF
                tooling is meanwhile able to do this - likely not. Did the authors
                confirm that they automatically validate the examples? Well,
                looking at the namespaces, likely not (the augmentations do not
                live in the ietf-routing namespace). So these examples need to be
                validated and fixed. I have used yanglint for this, works better
                for me than the pyang solutions, but the authors should figure out
                what works for them.
        <RR> I didn't verify them.. I was having issues with the verification tools. The examples will be verified.
            
              - There are special IP address blocks for examples; the IPv6 address
                you show seems to belong to APNIC...
        <RR> I'll use IP addresses from the example address block.
            
            * Security Considerations
            
              - Needs to be updated to the latest boilerplate.
        <RR> Ack.
            
              - You should discuss security properties of objects, there is more
                work to do here.
        <RR> Will look into this.
            
            * Appendix A
            
              - I do not understand what is going on here, I think this needs a
                bit more explanatory text. Why is the informal description of the
                two parameters more detailed than the description in the example
                YANG module? I suggest to have a proper description in the YANG
                module only.
            <RR> Will add text. Goal was to add an example of how echo could be configured.
        
            * Appendix B
            
              - What is an area-id? The description is not helpful.
            
              - list interface [...] description "List of interfaces" is not
                really useful.
        <RR> Considering removing this appendix.

        

Subject: I-D Action: draft-ietf-bfd-yang-10.txt
From: <[email-masked]>
To: <[email-masked]>
Cc: <[email-masked]>
Date: Thu, 1 Mar 2018 18:52:26 -0800
List-Id: "RTG Area: Bidirectional Forwarding Detection DT" <rtg-bfd.ietf.org>

A New Internet-Draft is available from the on-line Internet-Drafts directories.
This draft is a work item of the Bidirectional Forwarding Detection WG of the IETF.

        Title           : YANG Data Model for Bidirectional Forwarding Detection (BFD)
        Authors         : Reshad Rahman
                          Lianshu Zheng
                          Mahesh Jethanandani
                          Santosh Pallagatti
                          Greg Mirsky
	Filename        : draft-ietf-bfd-yang-10.txt
	Pages           : 69
	Date            : 2018-03-01

Abstract:
   This document defines a YANG data model that can be used to configure
   and manage Bidirectional Forwarding Detection (BFD).

   The YANG modules in this document conform to the Network Management
   Datastore Architecture (NMDA).

The IETF datatracker status page for this draft is:
https://datatracker.ietf.org/doc/draft-ietf-bfd-yang/

There are also htmlized versions available at:
https://tools.ietf.org/html/draft-ietf-bfd-yang-10
https://datatracker.ietf.org/doc/html/draft-ietf-bfd-yang-10

A diff from the previous version is available at:
https://www.ietf.org/rfcdiff?url2=draft-ietf-bfd-yang-10

Please note that it may take a couple of minutes from the time of submission
until the htmlized version and diff are available at tools.ietf.org.

Internet-Drafts are also available by anonymous FTP at:
ftp://ftp.ietf.org/internet-drafts/

Subject: I-D Action: draft-ietf-bfd-yang-11.txt
From: <[email-masked]>
To: <[email-masked]>
Cc: <[email-masked]>
Date: Sun, 4 Mar 2018 06:09:38 -0800
List-Id: "RTG Area: Bidirectional Forwarding Detection DT" <rtg-bfd.ietf.org>

A New Internet-Draft is available from the on-line Internet-Drafts directories.
This draft is a work item of the Bidirectional Forwarding Detection WG of the IETF.

        Title           : YANG Data Model for Bidirectional Forwarding Detection (BFD)
        Authors         : Reshad Rahman
                          Lianshu Zheng
                          Mahesh Jethanandani
                          Santosh Pallagatti
                          Greg Mirsky
	Filename        : draft-ietf-bfd-yang-11.txt
	Pages           : 73
	Date            : 2018-03-04

Abstract:
   This document defines a YANG data model that can be used to configure
   and manage Bidirectional Forwarding Detection (BFD).

   The YANG modules in this document conform to the Network Management
   Datastore Architecture (NMDA).

The IETF datatracker status page for this draft is:
https://datatracker.ietf.org/doc/draft-ietf-bfd-yang/

There are also htmlized versions available at:
https://tools.ietf.org/html/draft-ietf-bfd-yang-11
https://datatracker.ietf.org/doc/html/draft-ietf-bfd-yang-11

A diff from the previous version is available at:
https://www.ietf.org/rfcdiff?url2=draft-ietf-bfd-yang-11

Please note that it may take a couple of minutes from the time of submission
until the htmlized version and diff are available at tools.ietf.org.

Internet-Drafts are also available by anonymous FTP at:
ftp://ftp.ietf.org/internet-drafts/

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Tue, 13 Mar 2018 15:58:44 +0100

Hi,

here is my re-review of draft-ietf-bfd-yang. I think the document has
significantly improved since the -09 version, the authors have done an
excellent job to improve the document quality.

I have mostly a few minor mostly editorial issues left, except the
first one, which concerns the schema mount use case.

- Thanks for clarifying that the modules can be used on standalone
  devices. The new text is helpful.

  For the LNE and NI use cases, does it make sense to detail the mount
  points that are used? My understanding is that schema mount requires
  that mount points are identified with a "mount-point" extension
  statement, i.e., you can't mount at arbitrary places in the
  hierarchy but only at places that have been designated as mount
  points.

  That all said, since your YANG modules are basically augmenting
  other YANG modules that may be mounted, you do not seem to need a
  separate schema mount. If my understanding is correct, then here is
  a starting point for making this clearer:

  OLD

    When used at the network device level, the BFD YANG model is used
    "as-is".  When the BFD model is to be used in a Logical Network
    Element or in a Network Instance, the approach taken is to do a
    schema-mount (see Schema Mount [I-D.ietf-netmod-schema-mount]) of the
    BFD model in the appropriate location.  For example, if an
    implementation supports BFD IP multihop in network instances, the
    implementation would do schema-mount of the BFD IP multihop model in
    a mount-point which resides in a network instance.

  NEW

    When used at the network device level, the BFD YANG model are used
    "as-is".  When the BFD YANG model is used in a Logical Network
    Element or in a Network Instance, then the BFD YANG model augments
    the mounted routing model for the Logical Network Element or the
    Network Instance.

  Note that with this change, you also do not need a reference to
  schema mount.
  
- Since the different use cases (device, LNE, NI) are discussed right
  at the beginning of Section 2, it seems the following statements in
  Sections 2.5, 2.6, 2.7, 2.8, 2.9 are not really needed:

                                   The "bfd" node under control-plane-
   protocol can be used in a network device (top-level), or mounted in
   an LNE or in a network instance.

                                            The "ip-sh" node can be used
   in a network device (top-level), or mounted in an LNE or in a network
   instance.

                                                            The "ip-mh"
   node can be used in a network device (top-level), or mounted in an
   LNE or in a network instance.

                              The "lag" node can be used in a network
   device (top-level), or mounted in an LNE or in a network instance.

                                                             The "mpls"
   node can be used in a network device (top-level), or mounted in an
   LNE or in a network instance.

- The text at the beginning of Section 2.13 should also mention RFC
  8177 since you are importing it.

- It might be useful to give more explicit instructions to IANA. I
  assume you want IANA to update the iana-bfd-types module whenever
  changes are made to the "BFD Diagnostic Codes" registry and "BFD
  Authentication Types" registries. Giving clear instructions what
  IANA is expected to do and when is better than a soft statement such
  as "intended to reflect". But IANA is going to ask questions about
  this anyway during their review I assume.

- The feature definitions in ietf-bfd-types have text of the form "as
  defined in RFC 5880" and perhaps it makes sense to add reference
  statements to these feature definitions. There are also a number of
  identities that say "as per RFC 588X" where perhaps reference
  statements should be added.

- The text at the beginning of Section 2.13 should also	mention	RFC
  6991 since you are importing it. And you are also importing from
  RFC XXXX (the routing model).

- The text at the beginning of Section 2.16 should also mention
  that you import from RFC XXXX (the routing model).

- The text at the beginning of Section 2.17 should also mention that
  you import from RFC 6991 and from RFC XXXX (the routing model).

- The text at the beginning of Section 2.18 should also mention that
  you import from RFC XXXX (the routing model).

- The text at the beginning of Section 2.19 should also mention that
  you import from RFC XXXX (the routing model).

- I have not validated the examples - I hope the authors have done so.
  They look more plausible than in the previous version I reviewed.

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
Fax:   +49 421 200 3103         <https://www.jacobs-university.de/>

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Tue, 20 Mar 2018 13:06:27 +0000

Hi Juergen,

Thanks again for the excellent review. We've just published rev12 to address your latest comments.

Please see inline.

Regards,
Reshad.

On 2018-03-13, 10:58 AM, "Juergen Schoenwaelder" <[email-masked]> wrote:

    Hi,

    

    
      OLD

      NEW

<RR> Done.

    
    
                                                                The "ip-mh"

                                                                 The "mpls"
<RR> Done
    
<RR> Done

<RR> Updated 5.1
    
<RR> Added reference sections to the feature definitions and identities.
    
    - The text at the beginning of Section 2.13 should also	mention	RFC
      6991 since you are importing it. And you are also importing from
      RFC XXXX (the routing model).
<RR> 2.13 already mentions RFC 6991 but it was missing from 2.15 and 2.17 (it's been added). 2.13 already has mention of 8022bis (routing model). 8022bis is now rfc8349.

    - The text at the beginning of Section 2.16 should also mention
      that you import from RFC XXXX (the routing model).
<RR> We now mention rfc8349 (8022bis).
    
    - The text at the beginning of Section 2.17 should also mention that
      you import from RFC 6991 and from RFC XXXX (the routing model).
<RR> Added mention of RFC6991.
    
    - The text at the beginning of Section 2.18 should also mention that
      you import from RFC XXXX (the routing model).
<RR> We now mention rfc8349  (8022bis).
    
    - The text at the beginning of Section 2.19 should also mention that
      you import from RFC XXXX (the routing model).
<RR> We now mention rfc8349   (8022bis).
    
    - I have not validated the examples - I hope the authors have done so.
      They look more plausible than in the previous version I reviewed.
<RR> Yes we have validated them using yanglint.
    
    /js

Subject: New Version Notification for draft-ietf-bfd-yang-12.txt
From: <[email-masked]>
To: Mahesh Jethanandani <[email-masked]>, Reshad Rahman <[email-masked]>, Juniper Networks <[email-masked]>, "Gregory Mirsky" <[email-masked]>, Greg Mirsky <[email-masked]>, "Santosh Pallagatti" <[email-masked]>, Lianshu Zheng <[email-masked]>
Date: Tue, 20 Mar 2018 06:05:36 -0700

A new version of I-D, draft-ietf-bfd-yang-12.txt
has been successfully submitted by Reshad Rahman and posted to the
IETF repository.

Name:		draft-ietf-bfd-yang
Revision:	12
Title:		YANG Data Model for Bidirectional Forwarding Detection (BFD)
Document date:	2018-03-20
Group:		bfd
Pages:		74
URL:            https://www.ietf.org/internet-drafts/draft-ietf-bfd-yang-12.txt
Status:         https://datatracker.ietf.org/doc/draft-ietf-bfd-yang/
Htmlized:       https://tools.ietf.org/html/draft-ietf-bfd-yang-12
Htmlized:       https://datatracker.ietf.org/doc/html/draft-ietf-bfd-yang
Diff:           https://www.ietf.org/rfcdiff?url2=draft-ietf-bfd-yang-12

Abstract:

The IETF Secretariat

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Wed, 21 Mar 2018 09:51:14 +0100

Reshad,

this looks good. You proabably also want to update references to point
to the RFCs that just recently appeared:

- replace [I-D.ietf-netmod-yang-tree-diagrams] with [RFC8340]
- replace [I-D.ietf-netmod-revised-datastores] with [RFC8342]
- replace [I-D.ietf-netmod-rfc7277bis] with [RFC8344]

/js

> From: [email-masked]
> To: Mahesh Jethanandani <[email-masked]>, Reshad Rahman
>  <[email-masked]>, Juniper Networks <[email-masked]>,
>  Gregory Mirsky <[email-masked]>, Greg Mirsky
>  <[email-masked]>, Santosh Pallagatti
>  <[email-masked]>, Lianshu Zheng <[email-masked]>
> Message-ID: <[email-masked]>
> 
>

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Wed, 21 Mar 2018 09:19:57 +0000

Thank you Juergen, will do. I had updated references for the new RFC8343 and 8349 but missed these docs.

Regards,
Reshad.

On 2018-03-21, 8:51 AM, "Juergen Schoenwaelder" <[email-masked]> wrote:

    Reshad,

    
    /js