Including the rtgwg list.
Regards, B.
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
.
_______________________________________________
rtgwg mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/rtgwg
