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

From: Xufeng Liu <[email-masked]>
Date: Wed, 15 Mar 2017 21:31:31 +0000

Hi Radek,

Thanks for the thorough review. We have submitted an updated revision https://datatracker.ietf.org/doc/draft-ietf-rtgwg-yang-vrrp/ to address the issues.

Regards,
- Xufeng

> -----Original Message-----
> From: Radek Krejčí [mailto:[email-masked]]
> Sent: Thursday, December 22, 2016 9:27 AM
> To: [email-masked]
> Cc: [email-masked]; Benoit Claise (bclaise) <[email-masked]>
> Subject: YANG Doctors review for draft-ietf-rtgwg-yang-vrrp
> 
> 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.

[Xufeng] We have added Section 2 to describe the model, and Appendix A to show a configuration data example.

> 
> - 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

[Xufeng] 
Fixed.

> 
> - 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)
[Xufeng] Fixed.
> 
> - unify the new-master-reason-type enums' names: (master-)preempted vs
> master-no-response
[Xufeng] Is there any particular suggestions?
> 
> - the local module prefix SHOULD be used instead of no prefix in all path
> expressions (e.g. vrid-v4, vrid-v6)
[Xufeng] Fixed.
> 
> - 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
[Xufeng] Introduced " identity vrrp-version".
> 
> - 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.
[Xufeng] You are right. The vrrp-v3-attributes grouping is not necessary. We have removed this grouping and re-organized vrrp-ipv4-attributes and vrrp-ipv4-attributes groupings for better clarity.

> 
> - 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.
[Xufeng] Renamed the key to "prefix", and fixed the description.
> 
> - 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)
[Xufeng] Fixed.
> 
> - the previous 2 notes also apply to vrrp-ipv6-attributes grouping
[Xufeng] Fixed.
> 
> - 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
[Xufeng] Changed to  up-datetime.
> 
> - 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.
[Xufeng] Introduced an identity vrrp-event-type, which covers the event types.
> Radek

---

From: Radek Krejčí <[email-masked]>
Date: Thu, 16 Mar 2017 16:00:11 +0100

Hi Xufeng,
comments are inline

> [Xufeng] We have added Section 2 to describe the model, and Appendix A to show a configuration data example.

It is better now, however, few notes:

- mixing the tree format with columns to show borders between the modules in section 2.2 seems weird. If you want to use the tree format, use prefixes as pyang or yanglint do.

- in section 2.5, use the notification names to clearly connect the text here with the specific notification

- I don't thing that it is necessary to print the complete tree in the section 3. Instead, show the currently hidden nodes in sections 2.x (and add the notifications part of the tree into section 2.5)

>> is then also used in path expression for vrid-v4

- in vrrp-virtual-router-error-event notification, I believe that vrid-v4 and vrid-v6 are mutually exclusive, and also you may want to have one of them to be always present, right? If so, put them into the choice:

choice vrid {
  mandatory true; /* if one of the leafs always expected */
  leaf vrid-v4 { ... }
  leaf vrid-v6 { ... }
}

> [Xufeng] Is there any particular suggestions?

personally I prefer shorter names, so 'not-master', 'priority', 'preempted' and 'no-response'

>> or track-networks instead of networks)

still the same issue in {grouping}[vrrp-ipv4-attributes]/track/networks/network (and the same for ipv6 grouping)

Radek