Last Call Review of draft-ietf-netmod-entity-07
review-ietf-netmod-entity-07-yangdoctors-lc-krejci-2018-01-04-00

Request Review of draft-ietf-netmod-entity
Requested rev. no specific revision
Type Last Call Review
Team YANG Doctors (yangdoctors)
Deadline 2018-01-15
Requested 2017-12-18
Requested by Lou Berger
Other Reviews Genart Telechat review of -07 by Meral Shirazipour
Secdir Telechat review of -07 by Shawn Emery
Review State Completed
Reviewer Radek Krejčí
Review review-ietf-netmod-entity-07-yangdoctors-lc-krejci-2018-01-04
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/sOUKQS0aq__FQv-ml13wqULvXEE
Reviewed rev. 07
Review result Ready with Issues
Draft last updated 2018-01-04
Review completed: 2018-01-04

Review
review-ietf-netmod-entity-07-yangdoctors-lc-krejci-2018-01-04

The document itself and normative parts seem fine to me, the only issue I see is with the ietf-hardware-state module in non-normative appendix A. It seems to me that this temporary non-NMDA module does not conform to its purpose as described in RFC6087bis. According to guidelines, such a module is intended to provide state (config false) data in case the server does not implement NMDA (to bridge the time period until NMDA is implemented). So such a server is IMHO intended to implement both modules, foo and foo-state. The foo-state contains "the top-level config-false data nodes that would have been defined in a legacy YANG module" - so it's only the ro mirror of data nodes. But ietf-hardware-state contains notifications, which are not the data nodes as defined in RFC7950. I understand why the notifications were placed also into the ietf-hardware-state - the module's description states that "If a server that implements this module but doesn't support NMDA also supports configuration of hardware components, it SHOULD also implement the module 'ietf-hardware' ...", so it allows its standalone usage in case the server does not support hw configuration. But in such a case, the server can implement ietf-hardware with deviations on the config=true nodes. The same way it had to implement the legacy YANG module (before NMDA).

So I think that the notifications should be removed from ietf-hardware-state and the module's description should change this way:

OLD

  If a server that implements this module but doesn't support NMDA
  also supports configuration of hardware components, it SHOULD
  also implement the module 'ietf-hardware' in the configuration
  datastores. The corresponding state data is found in the
  '/hw-state:hardware' subtree.

NEW

  If a server that implements this module but doesn't support NMDA,
  it MUST also implement the module 'ietf-hardware' in the
  configuration datastores. The corresponding state data is found
  in the '/hw-state:hardware' subtree.