Subject: Re: [yang-doctors] Yangdoctors last call review of draft-ietf-ospf-yang-09

From: "Acee Lindem (acee)" <[email-masked]>
Date: Tue, 13 Mar 2018 21:13:35 +0000

Hi Lada, 
We believe that https://datatracker.ietf.org/doc/draft-ietf-ospf-yang/ satisfies your comments. Please take a look ASAP as we intend to WG last called the ietf-ospf YANG model imminently. 

Thanks,
Co-Authors 

On 12/6/17, 6:26 AM, "Ladislav Lhotka" <[email-masked]> wrote:

    Reviewer: Ladislav Lhotka
    Review result: Ready with Issues
    
    The data model defined in this document is a massive piece of work: it
    consists of 11 YANG modules and defines around 1200 schema nodes. The
    "ietf-ospf@2017-10-30" module is compatible with the NMDA architecture.
    
    **** Comments
    
    1. Unless there is a really compelling reason not to do so, the
       "ietf-ospf" should declare YANG version 1.1. For one,
       "ietf-routing" that is being augmented by "ietf-ospf" already
       declares this version. Some of my suggestions below also assume
       version 1.1.
    
    2. The "ietf-ospf" can work only with the new NMDA-compatible
       revisions of some modules, such as "ietf-interfaces" and
       "ietf-routing". I understand it is not desirable to import such
       modules by revision, but at least it should be mentioned in a
       description attached to every such import.
    
    3. Maybe the draft could mention that implementations should supply a
       default routing domain as a system-controlled resource.
    
    4. In "when" expressions, the module uses literal strings for
       identities. This is known to be problematic, the XPath functions
       derived-from() or derived-from-or-self() should be used instead.
    
    5. Some enumerations, such as "packet-type" and "if-state-type"
       define enum identifiers with uppercase letters and/or underscores,
       for example "Database-Description" or "LONG_WAIT". RFC6087bis
       recommends that only lowercase letters, numbers and dashes. I think
       this convention should be observed despite the fact that the
       current names are traditionally used in OSPF specs. The
       "ietf-routing" module also defines "router-id" even though the
       documents use "Router ID".
    
    6. The types of LSA headers are modelled as integers. While OSPF gurus
       probably know these numbers by heart, it is not very
       reader-frienly. So at least some references to documents defining
       these numbers should be provided, but my suggestion is to consider
       implementing them with identities. It seems it might also be useful
       to define some "abstract" identities for these types. For example,
       if "opaque-lsa" is defined, then the definition of container
       "opaque" could simply use
    
         when "derived-from(../../header/type, 'ospf:opaque-lsa')";
    
       instead of
    
          when "../../header/type = 9 or "
                  + "../../header/type = 10 or "
                  + "../../header/type = 11";
    
    7. The title of sec. 2.9 should be "OSPF Notifications" rather than
       "OSPF notification".

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Tue, 13 Mar 2018 21:15:15 +0000

Hi Martin, 
We believe that https://datatracker.ietf.org/doc/draft-ietf-ospf-yang/ satisfies your comments. Please take a look.
Thanks,
Acee

On 12/6/17, 6:46 AM, "Martin Bjorklund" <[email-masked]> wrote:

    > consists of 11 YANG modules and defines around 1200 schema nodes. The
    
    11 YANG modules?  Isn't there just one?

    I would like to add two quick comments to Lada's review.
    
    o  Remove all "revision" statements except the one that says "initial
       revision".   The idea is to have one revision statement per
       published version (i.e., RFC).

    o  Many of the nodes have quite rudimentary descriptions, and the
       "reference" statement is rarely used.  For example:
    
             leaf lsa-id {
               type yang:dotted-quad;
               mandatory true;
               description "LSA ID.";
             }
    
       It seems as the description is put there just to keep the tools
       quiet.  I know that it is painful to write good descriptions, but
       it does help the consumer of the data model.

    /martin

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

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Tue, 13 Mar 2018 21:19:13 +0000

Hi Lada, 
    
    Thanks,
    Co-Authors 

        
        
        **** Comments

        

        

           instead of

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Tue, 13 Mar 2018 21:20:54 +0000

Hi Martin, 

        

        
                 leaf lsa-id {
                   type yang:dotted-quad;
                   mandatory true;
                   description "LSA ID.";
                 }

        /martin

---

From: Martin Bjorklund <[email-masked]>
Date: Wed, 14 Mar 2018 08:30:35 +0100 (CET)

Hi,

"Acee Lindem (acee)" <[email-masked]> wrote:
> Hi Martin, 
> We believe that https://datatracker.ietf.org/doc/draft-ietf-ospf-yang/
> satisfies your comments. Please take a look.
> Thanks,
> Acee
>     

This is now addressed.

>            it does help the consumer of the data model.

I don't think this was addressed; many descriptions are still on this
form (including the lsa-id).  But if the authors and the WG believe
that this is clear to the reader this is fine.

Just a few additional comments:

o  In section 3, before the module, add:

   This module augments [I-D.ietf-netmod-rfc8022bis], imports a
   grouping from [I-D.ietf-bfd-yang], and references [RFC1765],
   [RFC4552], [RFC5082], [RFC5286], [RFC5329], [RFC5443], [RFC5631],
   [RFC5714], [RFC5880], [RFC5881], [RFC6021], [RFC6860], [RFC6987],
   [RFC7490], [RFC7684], [RFC7777], [RFC8291], and
   [I-D.ietf-rtgwg-backoff-algo].

  and add these documents to the references.

  NOTE: the list above is probably not complete, please ensure that
  all references in the YANG module are covered in this list, except
  the ones that are already present in the text body of the document
  (like RFC 8177).

  Also add RFC editor notes to the "RFC XXXX" strings you have that
  refer to current I-Ds (as opposed to the current document).

o  Remove the statement

     reference "RFC XXXX";

   on the top-level (before the revision statement).

   (The revision statement references this RFC).

o  I suggest you remove the "end comments" like:

      } // list link-scope-lsas

   It is not really clear why some end brackets have them and some do
   not.  Also they are not consistent, and sometimes don't match the
   "other end" (probably half of the ones I quickly checked were not
   correct).

   If you decide to keep them, please ensure that they are consistent
   and correct.

/martin

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Fri, 16 Mar 2018 17:30:58 +0000

Hi Martin, 

On 3/14/18, 3:30 AM, "Martin Bjorklund" <[email-masked]> wrote:

    Hi,

    This is now addressed.

Note that I did beef up the descriptions and references for a number of data leaves. This is a pretty fundamental concept in OSPF and one could argument that someone who doesn’t understand it won't benefit from looking at the YANG model. However, we will expand the abbreviation in the first occurrence and indicate that LSAs are uniquely identified by the <lsa-type, lsa-id, adv-router> tuple in the various database description text. 

    

Thanks - actually I knew about these many were a result of the above done right before the IETF cut-off. I was fully expecting a comment from Tom Petch by now... We'll make sure we add all these in the next revision. 

Right - a few of these are in the AUTH48 hours and will be updated to the actual numbers in the next revision. 

    o  Remove the statement
    
         reference "RFC XXXX";

Sure. 

    

I agree - let me discuss with my co-authors. 

Thanks,
Acee 

    /martin

---

From: Rob Shakir <[email-masked]>
Date: Mon, 19 Mar 2018 09:22:13 +0000

Hi Acee,

For the descriptions, there are tools that use these to build documentation
automatically for a YANG model. We have found that such documentation is
particularly useful for engineers that are actually consuming the models to
generate config.

Going forward, if we expect YANG modelled data to be the way that we
interact with network elements, then such generated documentation is likely
to replace a bunch of CLI manuals, so it is worth ensuring the descriptions
are useful in such a context IMHO.

Just my $0.02,
r.
On Fri, Mar 16, 2018 at 17:31 Acee Lindem (acee) <[email-masked]> wrote:

>     > We believe that
> https://datatracker.ietf.org/doc/draft-ietf-ospf-yang/
>     >         > The data model defined in this document is a massive piece
> of work:
>     >         > it
> Note that I did beef up the descriptions and references for a number of
> data leaves. This is a pretty fundamental concept in OSPF and one could
> argument that someone who doesn’t understand it won't benefit from looking
> at the YANG model. However, we will expand the abbreviation in the first
> occurrence and indicate that LSAs are uniquely identified by the <lsa-type,
> lsa-id, adv-router> tuple in the various database description text.
>
>
> Thanks - actually I knew about these many were a result of the above done
> right before the IETF cut-off. I was fully expecting a comment from Tom
> Petch by now... We'll make sure we add all these in the next revision.
>
>
>     >         >    recommends that only lowercase letters, numbers and
> dashes. I
>     >         >    think
>     >         > 6. The types of LSA headers are modelled as integers.
> While OSPF
>     >         > gurus
>     >         >    implementing them with identities. It seems it might
> also be
>     >         >    useful
> Lsr mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/lsr
>

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Mon, 19 Mar 2018 13:15:48 +0000

Hi Rob,

From: Rob Shakir <[email-masked]>
Date: Monday, March 19, 2018 at 5:22 AM
To: Acee Lindem <[email-masked]>
Cc: Martin Bjorklund <[email-masked]>, "[email-masked]" <[email-masked]>, YANG Doctors <[email-masked]>, Ladislav Lhotka <[email-masked]>, "[email-masked]" <[email-masked]>
Subject: Re: [Lsr] [yang-doctors] Yangdoctors last call review of draft-ietf-ospf-yang-09

Hi Acee,

For the descriptions, there are tools that use these to build documentation automatically for a YANG model. We have found that such documentation is particularly useful for engineers that are actually consuming the models to generate config.

We are going through another revision prior to WG last call so feel free to improvements to data leafs that are lacking.

Going forward, if we expect YANG modelled data to be the way that we interact with network elements, then such generated documentation is likely to replace a bunch of CLI manuals, so it is worth ensuring the descriptions are useful in such a context IMHO.

This is direction supported by confd with CLI prompt text being either the YANG description or the text specified in te  tailf:info YANG extension dependent on the confdc compile options.

Thanks,
Acee

Just my $0.02,
r.
On Fri, Mar 16, 2018 at 17:31 Acee Lindem (acee) <[email-masked]<mailto:[email-masked]>> wrote:
Hi Martin,

On 3/14/18, 3:30 AM, "Martin Bjorklund" <[email-masked]<mailto:[email-masked]>> wrote:

    Hi,

    "Acee Lindem (acee)" <[email-masked]<mailto:[email-masked]>> wrote:
    >     On 12/6/17, 6:46 AM, "Martin Bjorklund" <[email-masked]<mailto:[email-masked]>> wrote:
    >
    >         Ladislav Lhotka <[email-masked]<mailto:[email-masked]>> wrote:

    This is now addressed.

    o  Remove the statement

         reference "RFC XXXX";

Sure.

Thanks,
Acee

    /martin

    >         > [email-masked]<mailto:[email-masked]>

_______________________________________________
Lsr mailing list
[email-masked]<mailto:[email-masked]>
https://www.ietf.org/mailman/listinfo/lsr