[OPSAWG] Yangdoctors early review of draft-ietf-opsawg-l3sm-l3nm-03

[Radek Krejčí via Datatracker <[e-mail]>]

Reviewer: Radek Krejčí
Review result: Ready with Issues

The I-D document contains a single YANG module ietf-l3vpn-ntw.

The module is quite complex, but despite I'm not an expert in the area, the
text of the draft seems very descriptive and helpful to understand all the
parts of the module tree. From my perspective, the most problematic part of the
module is the overall use of groupings and the current status of their
specification in the module.

Here are the specific issues:

- the module seems NMDA compliant, so it should be mentioned in Introduction
(or some other) section:

    The YANG data model in this document conforms to the Network
    Management Datastore Architecture (NMDA) defined in RFC 8342.

- the filename's revision part does not match the latest revision date
(2020-04-03) of the module:
  <CODE BEGINS>  file "[e-mail]"

- all the imported modules must have their document (RFC) in the normative
references section, but RFC 6991, RFC 8294, RFC 8299 and RFC 8519 are missing.

- since RFC 8277 is referenced in the module, it should appear in the normative
references section.

- additional indentation (3 spaces on each line except the first one) in the
module's contact statement seems weird and unnecessary/unintentional.

- some of the descriptions in the module use RFC 2119 key words, so the
module's description is supposed to contain the following text:

     The key words 'MUST', 'MUST NOT', 'REQUIRED', 'SHALL', 'SHALL
     NOT', 'SHOULD', 'SHOULD NOT', 'RECOMMENDED', 'NOT RECOMMENDED',
     'MAY', and 'OPTIONAL' in this document are to be interpreted as
     described in BCP 14 (RFC 2119) (RFC 8174) when, and only when,
     they appear in all capitals, as shown here.

- typedef protocol-type;
  - Are you really sure that the list of the underlying protocols is fixed
  forever in all its instances? Having it in the form of identity would allow
  later augmenting of the list of possible values.

- the whole groupings part need a cleanup and maybe some structural changes:
  - most of the groupings are instantiated only once - having them defined this
  way makes sense only in case you believe they are useful for other modules to
  use. Splitting a complex module into groupings just for keeping its parts
  separated IMO decreases the readability and usability of the module - it is
  quite challenging to find a specific path or subtree because of intensive
  skipping from grouping to another. - there are also groupings that are not
  instantiated at all, if they are intended exclusively for use by other
  modules, I would prefer to have some note about it in their descriptions. -
  there are several comment out uses statements, there is also whole /* UNUSED
  */ part

- description of the service-status grouping seems to be invalid

- descriptions of {grouping site-bearer-params}/site-bearers/bearer/bearer-id
and {grouping vpn-route-targets}/vpn-polices nodes are empty

- I'm afraid about the usability of appendix A. The status of the
implementations will tent to change quite intensively and having such
information in RFC does not make much sense to me.