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

Martin Bjorklund <mbj@tail-f.com> Wed, 14 March 2018 07:30 UTC

Return-Path: <mbj@tail-f.com>
X-Original-To: yang-doctors@ietfa.amsl.com
Delivered-To: yang-doctors@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 9482D1242F5; Wed, 14 Mar 2018 00:30:43 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.911
X-Spam-Level:
X-Spam-Status: No, score=-1.911 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_PASS=-0.001, T_RP_MATCHES_RCVD=-0.01] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zSN2qpGbOYxW; Wed, 14 Mar 2018 00:30:41 -0700 (PDT)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id 45FB91200C5; Wed, 14 Mar 2018 00:30:38 -0700 (PDT)
Received: from localhost (h-80-27.A165.priv.bahnhof.se [212.85.80.27]) by mail.tail-f.com (Postfix) with ESMTPSA id 929CB1AE034E; Wed, 14 Mar 2018 08:30:35 +0100 (CET)
Date: Wed, 14 Mar 2018 08:30:35 +0100 (CET)
Message-Id: <20180314.083035.1071848174448590900.mbj@tail-f.com>
To: acee@cisco.com
Cc: lhotka@nic.cz, yang-doctors@ietf.org, draft-ietf-ospf-yang.all@ietf.org, lsr@ietf.org
From: Martin Bjorklund <mbj@tail-f.com>
In-Reply-To: <ED41D824-69F8-4A01-92E7-890E908BB0CE@cisco.com>
References: <20171206.124611.1073986528289329597.mbj@tail-f.com> <1F5FF6F1-1309-4A89-9A79-4607C2423CEE@cisco.com> <ED41D824-69F8-4A01-92E7-890E908BB0CE@cisco.com>
X-Mailer: Mew version 6.7 on Emacs 24.5 / Mule 6.0 (HANACHIRUSATO)
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/5UTn8RR8a51leRVxsbZrbEgVKec>
Subject: Re: [yang-doctors] Yangdoctors last call review of draft-ietf-ospf-yang-09
X-BeenThere: yang-doctors@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: Email list of the yang-doctors directorate <yang-doctors.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/yang-doctors/>
List-Post: <mailto:yang-doctors@ietf.org>
List-Help: <mailto:yang-doctors-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 14 Mar 2018 07:30:43 -0000

Hi,

"Acee Lindem (acee)" <acee@cisco.com> wrote:
> 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" <mbj@tail-f.com> wrote:
>     
>         Ladislav Lhotka <lhotka@nic.cz> 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
>         
>         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).

This is now addressed.


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

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


>         
>         
>         /martin
>         
>         
>         > "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".
>         > 
>         > 
>         > _______________________________________________
>         > yang-doctors mailing list
>         > yang-doctors@ietf.org
>         > https://www.ietf.org/mailman/listinfo/yang-doctors
>         > 
>         
>     
>     
>