Last Call Review of draft-ietf-ospf-yang-09

Request Review of draft-ietf-ospf-yang
Requested rev. no specific revision (document currently at 29)
Type Last Call Review
Team YANG Doctors (yangdoctors)
Deadline 2017-12-14
Requested 2017-11-08
Requested by Acee Lindem
Authors Derek Yeung, Yingzhen Qu, Zhaohui Zhang, Ing-Wher Chen, Acee Lindem
Draft last updated 2017-12-06
Completed reviews Yangdoctors Last Call review of -09 by Ladislav Lhotka (diff)
Yangdoctors Last Call review of -23 by Ladislav Lhotka (diff)
Rtgdir Last Call review of -23 by Ravi Singh (diff)
Genart Last Call review of -23 by Erik Kline (diff)
Secdir Last Call review of -23 by Stefan Santesson (diff)
Assignment Reviewer Ladislav Lhotka 
State Completed
Review review-ietf-ospf-yang-09-yangdoctors-lc-lhotka-2017-12-06
Reviewed rev. 09 (document currently at 29)
Review result Ready with Issues
Review completed: 2017-12-06


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