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

From: Jürgen Schönwälder <[email-masked]>
Date: Thu, 15 Feb 2018 01:35:15 -0800

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.

  - Update 2017 to 2018 in copyright statements etc.

  - 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.

  - State whether the model is NMDA compliant (which it likely should
    be), see also previous item.

  - 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?

  - There are some YANG validation errors that should be addressed (see
    the link on the datatracker).

  - 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.

  - 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)";
      }

* 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?

  - 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?

  - 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?

  - The phrase 'operational model' probably means 'operational state
    model' and 'operational items' probably means 'operational state
    data'.

  - 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.

  - In 2.3, you use 'clients of BFD' but I think this is very
    different from 'BFD clients'. Please clarify the terminology.

  - s/operational data/operational state data/g

  - The *-count leafs seem to be gauge32, should yang-types:gauge32 be
    used?

  - There are also real counters that should probably use
    yang-types:counter32 and yang-types:counter64 instead of uint32
    and uint64.

  - Is [I-D.ietf-mpls-base-yang] not a normative reference? See text
    in 2.11.3.

  - Is [I-D.ietf-teas-yang-te] not a normative reference? See text in
    2.11.4.

* 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?

* 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).

  - s/Two interval values or 1 value/Two interval values or one value/

  - 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.

  - 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.

  - 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.

  - 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.";
       }

       [...]

* BFD IP multihop YANG Module

  - This is indented differently. Well the RFC editor will fix I
    guess.

* 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.

  - There are special IP address blocks for examples; the IPv6 address
    you show seems to belong to APNIC...

* Security Considerations

  - Needs to be updated to the latest boilerplate.

  - You should discuss security properties of objects, there is more
    work to do here.

* 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.

* Appendix B

  - What is an area-id? The description is not helpful.

  - list interface [...] description "List of interfaces" is not
    really useful.

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Fri, 16 Feb 2018 23:11:28 +0000

Hi Jurgen, 

See one inline.

On 2/15/18, 4:35 AM, "yang-doctors on behalf of Jürgen Schönwälder" <[email-masked] on behalf of [email-masked]> wrote:

    
    * General comments

    

    

      - We generally prefer

        over
    
          reference "RFC 6991"

    
          import ietf-inet-types {
            prefix "inet";
            reference "RFC 6991";
          }

          import ietf-inet-types {
            prefix inet;

This Is definitely not the case. This model will augment RFC8022BIS. The question on how to do is being discussion on the YANG doctors list. 

Thanks,
Acee 

    

    

    

    
    * IANA BFD YANG Module

    
    * BFD types YANG Module

    
    
          leaf down-count {
             type uint32;

          leaf down-count {
             type yang:counter32;
             description

    

           leaf sessions {
             type yang:gauge32;
    
           leaf sessions-up {
             type yang:gauge32;
    
           [...]

    
    * Examples

    

    
    * Appendix A

    * Appendix B

    
    _______________________________________________
    yang-doctors mailing list
    [email-masked]
    https://www.ietf.org/mailman/listinfo/yang-doctors

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Sat, 17 Feb 2018 09:55:51 +0100

> This Is definitely not the case. This model will augment RFC8022BIS. The question on how to do is being discussion on the YANG doctors list. 

This is what I thought but the text is kind of misleading:

   BFD can operate in the following contexts:

   [...]

   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
   locations.

/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: Sat, 17 Feb 2018 18:24:43 +0000

Right, schema-mount can be used in some cases (logical device or in a VRF) but doesn’t have to be used in other cases (e.g. network device which doesn't support VRFs). We will clarify the text, at a certain time we incorrectly thought that schema mount had to be used in all cases.

Regards,
Reshad.

On 2018-02-17, 3:56 AM, "Juergen Schoenwaelder" <[email-masked]> wrote:

    

       [...]

    /js

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Sat, 17 Feb 2018 18:31:13 +0000

I thought about that after I replied. I guess you are saying that it is conceivable for a network device to support ietf-bfd but not support routing (ietf-routing)? 

Thanks,
Acee 

On 2/17/18, 1:24 PM, "Reshad Rahman (rrahman)" <[email-masked]> wrote:

    Regards,
    Reshad.

        

        
           [...]

        /js

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Sat, 17 Feb 2018 22:26:44 +0000

Ietf-bfd augments the ietf-routing model, that's not conditional. How the ietf-bfd model is used may vary:
1) It may be used "directly" in a device (i.e no schema mount)
2) It may be schema mounted for use in an LNE
3) It may be schema mounted for use in a VRF

I thought this was the case for all routing protocols.

Regards,
Reshad.

On 2018-02-17, 1:31 PM, "Acee Lindem (acee)" <[email-masked]> wrote:

    Thanks,
    Acee 

        
        Regards,
        Reshad.

            

            
               [...]

            /js

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Sun, 18 Feb 2018 00:46:44 +0000

Hi Reshad, 
Are you saying that BFD would never augment the list of control place protocols in ietf-routing and always be at the root of the tree (whether it be the device, LNE, or NI)? I guess it doesn't have to be in the list since it will never install routes in the routing table. 

Thanks,
Acee

On 2/17/18, 5:26 PM, "Reshad Rahman (rrahman)" <[email-masked]> wrote:

    
    Regards,
    Reshad.

        
        Thanks,
        Acee 

            
            Regards,
            Reshad.

                

                
                   [...]

                /js

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Sun, 18 Feb 2018 03:28:13 +0000

Hi Acee,

What I meant is that BFD always augments the control palne protocols in ietf-routing model. And the BFD augmentation can be used as-is or mounted in LNE or in NI.

Regards,
Reshad.

On 2018-02-17, 7:46 PM, "Acee Lindem (acee)" <[email-masked]> wrote:

    Hi Reshad, 
    
    Thanks,
    Acee

        
        
        Regards,
        Reshad.

            
            Thanks,
            Acee 

                
                Regards,
                Reshad.

                    

                    
                       [...]

                    /js

---

From: Jeffrey Haas <[email-masked]>
Date: Wed, 21 Feb 2018 09:57:21 -0500

> What I meant is that BFD always augments the control palne protocols in ietf-routing model. And the BFD augmentation can be used as-is or mounted in LNE or in NI.

Basically:
Whereever ietf-routing is, BFD may also be.

BFD may not be everywhere that ietf-routing is (e.g. it may or may not be in
an instance, depending on device capabilities).

-- Jeff