Early Review of draft-ietf-rtgwg-yang-vrrp-01
review-ietf-rtgwg-yang-vrrp-01-yangdoctors-early-krejci-2017-03-17-00

Request Review of draft-ietf-rtgwg-yang-vrrp-01
Requested rev. 01 (document currently at 09)
Type Early Review
Team YANG Doctors (yangdoctors)
Deadline 2017-03-17
Requested 2017-03-17
Requested by Mehmet Ersue
Other Reviews Rtgdir Early review of -02 by Henning Rogge (diff)
Genart Last Call review of -07 by Linda Dunbar (diff)
Secdir Last Call review of -07 by Rich Salz (diff)
Opsdir Last Call review of -08 by Zitao Wang (diff)
Genart Telechat review of -08 by Linda Dunbar (diff)
Review State Completed
Reviewer Radek Krejčí
Review review-ietf-rtgwg-yang-vrrp-01-yangdoctors-early-krejci-2017-03-17
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/ewxIQW1Qq5TP3bD3LBLZ0BExL-0
Reviewed rev. 01 (document currently at 09)
Review result Ready with Issues
Draft last updated 2017-03-17
Review completed: 2017-03-17

Review
review-ietf-rtgwg-yang-vrrp-01-yangdoctors-early-krejci-2017-03-17

Hi,

I was asked to review the draft-ietf-rtgwg-yang-vrrp document, here are 
my comments:

- overall, the module is in a good shape, the common compilers are able 
to parse it and do not complain about anything

- the I-D text is very brief - some more detailed text about the scope, 
relationship to other (augmented) modules and use cases for the module 
would be welcome. Similarly, I would appreciate more detailed 
description of the specific sections of the module. The use cases can be 
demonstrated on configuration data examples which are now completely 
missing.

- it seems to me that at least some of the leafs in notifications are 
supposed to be mandatory, maybe not only in notifications, but I'm not 
expert in this area.
   - e.g. the interface leaf in vrrp-virtual-router-error-event 
notification, the leaf is then also used in path expression for vrid-v4 
(vrid-v6) leafref

- module imports ietf-interfaces and ietf-ip, thus it MUST contain 
normative references to RFC 7223 and RFC RFC 7277 - in I-D's reference 
section as well as in the module (as other imported modules are already 
referenced)

- unify the new-master-reason-type enums' names: (master-)preempted vs 
master-no-response

- the local module prefix SHOULD be used instead of no prefix in all 
path expressions (e.g. vrid-v4, vrid-v6)

- consider changing type of the version leaf in vrrp-common-attributes 
grouping to identityref - that way it would be possible only to augment 
the current module for any future version of the protocol, enumeration 
is limiting

- vrrp-v3-attributes grouping seems as an addition to the 
vrrp-common-attributes (at least it is always used together with 
vrrp-common-attributes). Do you see (e.g. in some other module) a use 
case to instantiate vrrp-common-attributes without vrrp-v3-attributes? 
If not, the vrrp-v3-attributes should be placed into vrrp-v3-attributes 
grouping. It is also question if it needs in that case a separate 
grouping, the accept-mode leaf could be placed directly into the common 
grouping just with the when condition.

- having a key of the "network" list named "network" (in 
vrrp-ipv4-attributes/track) seems as a bad design, try to rename the 
list or its key, also the descriptions of the network list and its 
container networks are not very clear.

- the names in the comments behind the closing brackets inside the 
vrrp-ipv4-attributes/track container are wrong (e.g. track-interfaces 
instead of interfaces or track-networks instead of networks)

- the previous 2 notes also apply to vrrp-ipv6-attributes grouping

- vrrp-state-attributes/up-time - uptime is usually interpreted as a 
time duration, but here it is used as a moment in time, so if it is not 
defined this way in VRRP protocol, consider renaming

- vrrp-state-attributes/last-event - are the events really so generic 
that the type here must be a string? Maybe having identities for them 
could help readability and understandability.


Radek