Early Review of draft-ietf-teas-yang-rsvp-10
review-ietf-teas-yang-rsvp-10-yangdoctors-early-aries-2019-05-03-00

Request Review of draft-ietf-teas-yang-rsvp
Requested rev. no specific revision (document currently at 11)
Type Early Review
Team YANG Doctors (yangdoctors)
Deadline 2019-04-15
Requested 2019-03-05
Requested by Lou Berger
Draft last updated 2019-05-03
Completed reviews Yangdoctors Early review of -10 by Ebben Aries (diff)
Comments
This document is basically ready for WG LC.  Would like to get an early review as part of preparing for this.
Assignment Reviewer Ebben Aries
State Completed
Review review-ietf-teas-yang-rsvp-10-yangdoctors-early-aries-2019-05-03
Reviewed rev. 10 (document currently at 11)
Review result On the Right Track
Review completed: 2019-05-03

Review
review-ietf-teas-yang-rsvp-10-yangdoctors-early-aries-2019-05-03

2 modules in this draft:
- ietf-rsvp@2019-02-18.yang
- ietf-rsvp-extended@2019-02-18.yang

No YANG compiler errors or warnings (pyang 2.0, yanglint 1.1.16, confdc 6.6.1)

Module ietf-rsvp@2019-02-18.yang:
- Remove WG Chairs from contact information per
  https://tools.ietf.org/html/rfc8407#appendix-B
- Use of 'state' containers.  It is stated in Section 2.3 that 'Derived state
  data is contained under a "state" container...'.  My only comments here are:
  a) Should use caution when using 'state' containers in NMDA compliant
  modules.  Rob put together a nice doc here that I won't reiterate:
  https://github.com/netmod-wg/FAQ/wiki/NMDA-Modelling-FAQ
  Using such nomenclature locks you into r/o nodes only.
  b) In some cases, the hierarchy is a bit redundant (statistics/state).
  Other NMDA compliant modules will not introduce another 'state' hierarchy
  for instance (see ietf-interfaces)
- leaf 'packet-len' is abbreviated while most other leafs are not
- authentication-key is of type string.  Is this leaf mean to be clear-text?
  There is nothing associated with this type that would imply special
  treatment in handling.
- crypto-algorithm: Are all identities from ietf-key-chain supported?
- Pluralization on counters:  e.g. 'in-error' vs. 'in-errors'. Maintain
  consistency with other modules (see ietf-interfaces)
- Normative references are missing for some of the modules imported.  These
  must be added per https://tools.ietf.org/html/rfc8407#section-3.9

Module ietf-rsvp-extended@2019-02-18.yang:
- Remove WG Chairs from contact information per
  https://tools.ietf.org/html/rfc8407#appendix-B
- Use of 'state' containers.  It is stated in Section 2.3 that 'Derived state
  data is contained under a "state" container...'.  My only comments here are:
  a) Should use caution when using 'state' containers in NMDA compliant
  modules.  Rob put together a nice doc here that I won't reiterate:
  https://github.com/netmod-wg/FAQ/wiki/NMDA-Modelling-FAQ
  Using such nomenclature locks you into r/o nodes only.
  b) In some cases, the hierarchy is a bit redundant (statistics/state).
  Other NMDA compliant modules will not introduce another 'state' hierarchy
  for instance (see ietf-interfaces)
- Pluralization on counters:  e.g. 'in-error' vs. 'in-errors'. Maintain
  consistency with other modules (see ietf-interfaces)
- 9 features are defined with no 'if-feature' statements.  Where are these
  feature conditions meant to be used?
- Normative references are missing for some of the modules imported.  These
  must be added per https://tools.ietf.org/html/rfc8407#section-3.9


General comments on the draft/modules:
- The state container and list key designs appear very familiar to that of
  OpenConfig modules however not consistent with IETF module design.
  Consistency is key from each producing entity (e.g. IETF in this case)
- The draft mentions RPCs and Notifications however none are defined within
  the modules
- Section 2.3: Has examples of RPCs and Notifications that are non-existant in
  the modules
- Abstract: s/RVSP/RSVP/
- Abstract: s/remote procedural/remote procedure/
- Section 2: s/extensiion/extension/
- Section 4: Update the security considerations section to adhere to
  https://tools.ietf.org/html/rfc8407#section-3.7 and
  https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines
- Various (missing) wording/pluralization cleanup throughout that I'll defer
  to the RFC Editor.  A once over proofread should solve this.