[yang-doctors] YANG Doctors review for draft-ietf-rtgwg-yang-vrrp

Radek Krejčí <rkrejci@cesnet.cz> Thu, 22 December 2016 14:27 UTC

To: draft-ietf-rtgwg-yang-vrrp@tools.ietf.org
From: Radek Krejčí <rkrejci@cesnet.cz>
Organization: CESNET, z.s.p.o.
Message-ID: <ee8f5425-6829-4025-e1a9-f84f94f8cbc5@cesnet.cz>
Date: Thu, 22 Dec 2016 15:27:02 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-2"; format="flowed"
Content-Transfer-Encoding: 7bit
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/ewxIQW1Qq5TP3bD3LBLZ0BExL-0>
Cc: yang-doctors@ietf.org
Subject: [yang-doctors] YANG Doctors review for draft-ietf-rtgwg-yang-vrrp
Precedence: list

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

Re: [yang-doctors] YANG Doctors review for draft-… Xufeng Liu
Re: [yang-doctors] YANG Doctors review for draft-… Martin Bjorklund
[yang-doctors] YANG Doctors review for draft-ietf… Radek Krejčí
Re: [yang-doctors] YANG Doctors review for draft-… Benoit Claise
Re: [yang-doctors] YANG Doctors review for draft-… Xufeng Liu
Re: [yang-doctors] YANG Doctors review for draft-… Xufeng Liu
Re: [yang-doctors] YANG Doctors review for draft-… Radek Krejčí