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

From: Xufeng Liu <[email-masked]>
Date: Wed, 26 Apr 2017 20:22:30 +0000

Hi Radek,

Thanks for checking again.
Please review the attached changes to see if they are ok.

Thanks,
- Xufeng

> -----Original Message-----
> From: Radek Krejčí [mailto:[email-masked]]
> Sent: Thursday, March 16, 2017 11:00 AM
> To: Xufeng Liu <[email-masked]>; draft-ietf-rtgwg-yang-
> [email-masked]
> Cc: [email-masked]; Benoit Claise (bclaise) <[email-masked]>
> Subject: Re: YANG Doctors review for draft-ietf-rtgwg-yang-vrrp
> 
> Hi Xufeng,
> comments are inline
> 
> Dne 15.3.2017 v 22:31 Xufeng Liu napsal(a):
> > 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 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.
[Xufeng] Changed to the format generated by pyang.

> 
> - in section 2.5, use the notification names to clearly connect the text here with
> the specific notification
[Xufeng] Updated. Also included the tree structure as mentioned below.

> 
> - 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)
[Xufeng] Removed this section, and put the notification tree to 2.5. Using the idea of RFC8022, include the complete tree structure in the Apendex A, so that a reader can use it as a reference if necessary.

> 
> >
> >> - 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.
> 
> - 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] Thanks for noticing this. It is right to allow one and only one of the two options. Please check if the change has made the representation more accurate and clear.

> 
> >
> >> - unify the new-master-reason-type enums' names: (master-)preempted
> >> vs master-no-response
> > [Xufeng] Is there any particular suggestions?
> 
> personally I prefer shorter names, so 'not-master', 'priority', 'preempted' and
> 'no-response'
[Xufeng] Agree. Changed accordingly.

> 
> >
> >> - 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.
> 
> still the same issue in {grouping}[vrrp-ipv4-attributes]/track/networks/network
> (and the same for ipv6 grouping)
[Xufeng] Sorry. I am not very sure which description lines. Can you check if the revised descriptions are what you were referring to?

> 
> >> - the previous 2 notes also apply to vrrp-ipv6-attributes grouping
> > [Xufeng] Fixed.
> 
> Radek

---

From: Martin Bjorklund <[email-masked]>
Date: Thu, 27 Apr 2017 09:56:03 +0200 (CEST)

Hi,

I haven't reviewed this draft, but I happened to find this:

             leaf interface {
               type if:interface-ref;
               must "/if:interfaces/if:interface[if:name=.]/ip:ipv6" {

Note that this must expression will always evaluate to false.  You
probably meant:

               must "/if:interfaces/if:interface[if:name=current()]/ip:ipv6" {

current() evaluates to the initial context node, but "." to the
context node for which the predicate expression is evaluated.

In YANG 1.1 this can also be written as:

               must "deref(.)/../ip:ipv6";

/martin

---

From: Xufeng Liu <[email-masked]>
Date: Thu, 27 Apr 2017 16:26:28 +0000

Hi Martin,

Thanks for catching the error. Will correct it.

Regards,
- Xufeng

> From: Martin Bjorklund [mailto:[email-masked]]
> Sent: Thursday, April 27, 2017 3:56 AM
> To: Xufeng Liu <[email-masked]>
> Cc: [email-masked]; [email-masked]; yang-
> [email-masked]
> Subject: Re: [yang-doctors] YANG Doctors review for draft-ietf-rtgwg-yang-vrrp
> 
> Hi,
> 
> Note that this must expression will always evaluate to false.  You probably
> meant:
> 
> current() evaluates to the initial context node, but "." to the context node for
> which the predicate expression is evaluated.
> 
> > > https://datatracker.ietf.org/doc/draft-ietf-rtgwg-yang-vrrp/ to
> > > address the issues.
> > > >
> > > > Regards,
> > > > - Xufeng
> > > >
> > > >> 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
> > > >>
> > > >> 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.
> > > - 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
> > > Instead, show the currently hidden nodes in sections 2.x (and add
> > > the notifications part of the tree into section 2.5)
> > [Xufeng] Removed this section, and put the notification tree to 2.5. Using the
> idea of RFC8022, include the complete tree structure in the Apendex A, so that a
> reader can use it as a reference if necessary.
> >
> > >
> > > >
> > > >> - 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.
> > > - 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 {
> > [Xufeng] Thanks for noticing this. It is right to allow one and only one of the
> two options. Please check if the change has made the representation more
> accurate and clear.
> >
> > >
> > > >
> > > >> - unify the new-master-reason-type enums' names:
> > > >> (master-)preempted vs master-no-response
> > > personally I prefer shorter names, so 'not-master', 'priority',
> > > 'preempted' and 'no-response'
> > > still the same issue in
> > > {grouping}[vrrp-ipv4-attributes]/track/networks/network
> > [Xufeng] Sorry. I am not very sure which description lines. Can you check if the
> revised descriptions are what you were referring to?
> >
> > >