Early Review of draft-ietf-roll-mpl-yang-01
review-ietf-roll-mpl-yang-01-yangdoctors-early-krejci-2018-04-24-00

Request Review of draft-ietf-roll-mpl-yang-01
Requested rev. 01 (document currently at 02)
Type Early Review
Team YANG Doctors (yangdoctors)
Deadline 2018-04-28
Requested 2018-04-03
Requested by Peter Van der Stok
Other Reviews
Review State Completed
Reviewer Radek Krejčí
Review review-ietf-roll-mpl-yang-01-yangdoctors-early-krejci-2018-04-24
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/uiFKWnM_NAHO-Nl46GoVs1Ewmac
Reviewed rev. 01 (document currently at 02)
Review result Not Ready
Draft last updated 2018-04-24
Review completed: 2018-04-24

Review
review-ietf-roll-mpl-yang-01-yangdoctors-early-krejci-2018-04-24

This document specifies 4 YANG modules for Multicast Protocol for Low power and lossy Networks (MPL) implementations. The document is not NMDA compliant and the separated modules are not correctly linked together (not even via leafrefs, but I suggest to redesign them as augments).

document

- all the modules are YANG 1.1, but you refer to RFC 6020 which defines YANG 1.0, fix the references to RFC 7950
- there is no note about NMDA, the document is NOT NMDA compliant because of the separated status data which are actually connected with the specific mpl domains (see below)

Section 1.1

- there is no note about SID term, you should refer draft-ietf-core-sid

Section 1.1.1

- instead of text description, refer to the Tree diagram specification (follow rfc6087bis sec. 3.4)

Section 2

- consider to split description of the modules to their tree diagrams, the intro of the section could be just about motivation to split all the data into a separated modules

Section 3

- as mentioned for section 1.1, there is no explanation of SID term, no reference, nothing

Section 4

OLD 
  This section describes four yang modules.  The model is based
NEW
  This section describes four yang modules.  The models are based

- RFC 7223 was obsoleted by 8343

- division between the schemas is not clear to me. Isn't the mpl-domain kind of core module, while the others extend it? In mpl-ops, I see mpl-parameter list connected with domainID from mpl-domain. Despite it should be at least leafref, I believe that it should actually augment the domains list from mpl-domain. Similarly, you can have the mpl-domain's top level domain (or better rename it to something like mpl) container as a top-level container holding all the mpl-related data together. This is also the reason why the document is NOT NMDA compliant - instead of augmenting domains list, the status data are held in a separate data tree.

- please fix indentation in the models. It makes reading of the model very difficult. If statement (mainly descriptions) carry into a subsequent line, please indent also those consequent lines. Also indent opening { of a node definition (e.g. domainID{ -> domainID {)

- update copyright notice (year) 

- you mix several naming conventions: domainID, SEED_SET_ENTRY_LIFETIME, life-time - please follow rfc6087bis sec 4.3.1

Section 5

- draft must register schema modules, so there are definitely IANA considerations. Besides this, I believe that also SID numbers are supposed to be registered via IANA.

Section 7

- update and maintain the changelog! The current information are taken from draft-vanderstok-roll-mpl-yang.



ietf-yang-mpl-domain@2018-03-29.yang

/domain/single
  - calling the whole switch as one of its case does not seem descriptive

/domain/single/mpl-domain/mpl-domain/addresses/interfaces
  - where, in RFC 6206, is interface name defined? Probably should be RFC 8343
  - why not use leafref to ietf-interfaces?


ietf-yang-mpl-ops@2018-03-29.yang

/mpl-ops/SEED_SET_ENTRY_LIFETIME (and several others)
  - use default statement
  - consider using units statement

/mpl-ops/mpl-parameter/domainID
  - should be leafref, but actually the whole mpl-parameter should be an augment of domains list in mpl-domain

/mpl-ops/mpl-parameter/CONTOL_MESSAGE_I* (and possibly others)
  - aren't there some limitations? Such as min < max? Then there should be must statement with the appropriate constraint specification.


ietf-yang-mpl-seeds@2018-03-29.yang

/mpl-seeds/local
  - fix upper cases in description or better rewrite the description as a sentence

/mpl-seeds/buffered-messages/t I
  - there is no SE-LIFETIME in the modules nor draft


ietf-yang-mpl-statistics@2018-03-29.yang

/mpl-statistics/nr-of-copies-forwarded
  - there is no number-of-copies-received in the modules nor draft

/mpl-statistics/reset-statistics
  - according to the location of the action, it is supposed to reset just the statistics for the specific buffer set (instance of the mpl-statistics list), right? Maybe it should be clarified in description, now there is "all statistics" which can be confusing according to the place of the action.