Subject: AD review of draft-ietf-rtgwg-yang-vrrp-04

From: Alia Atlas <[email-masked]>
Date: Wed, 20 Sep 2017 12:35:52 -0400

As is customary, I have done my AD review of draft-ietf-rtgwg-yang-vrrp-04.
First, I would like to thank the authors, Xufeng, Athanasios, Ravi,
Acee,and Mingui, as well as the WG for their work on this draft.  It is
clear and well-written.

My one issue is that it does not conform to the NMDA guidelines. I know
that the transformation can be done largely programmatically - and Acee &
Xufeng are quite familiar with the details.  I've also cc'd Rob Wilton who
has some tooling to potentially help.

>From the shepherd's report, I understand that there is an implementation.
That implies that the existing model should be in the appendix.

I would be delighted to forward this draft to IETF Last Call (and my
apologies for the long delay in review) after it has been updated.

Thanks,
Alia

---

From: Robert Wilton <[email-masked]>
Date: Thu, 21 Sep 2017 15:02:37 +0100

Hi Alia, authors,

OK, so the tooling is still a bit rough and ready, but I've tweaked it a 
bit and run it on the VRRP module.

The tooling itself:
(1) Alpha/beta level quality, generally it seems to work OK, but the 
results need to be carefully checked (e.g. by diffing the old and new 
modules to see what has changed).

(2) Is just a fork of pyang with some extra modules (it is sync'ed with 
Martin's latest pyang github repository today).

(3) Current code is available at: https://github.com/rgwilton/pyang
- Feel free to clone, extend, pull requests, etc.

(4) How does it work?:
- It looks through the parallel config/state trees finding nodes with 
the same names on equivalent paths.
- It that match then it checks description and reference and merges them 
together.  If it merges it flags it with a "FIX ME" comment.
- It also tries to handle augmentations and groupings, by heuristically 
checking for "-state" in the grouping name.

(5) Stuff that is known not to work:
- It should, but currently doesn't, validate that the same type is used 
between the config and state leaves!
- I've not tried it with submodules, I'm not sure how it will fare.

(6) Generically it is used like this (from the directory that contains 
the split YANG module):
- clone the github repository above, and ensure pyang's normal 
dependencies are available.
- run these commands:
cd <pyang directory>
source env.sh
cd <yang module directory>
"pyang -f yang --max-line-length 69 --ietf-split-to-combined-tree 
--remove-state-nodes <src.yang> > <tgt.yang>"

(7) Actual example steps on VRRP (draft-ietf-rtgwg-yang-vrrp-04), module 
extracted by rcfstrip:

# Generate a 'clean' tree for more informative diff (or else many 
whitespace changes are reported).
pyang -f yang --ietf --max-line-length 69 [email-masked] > 
[email-masked]
pyang -f yang --ietf-split-to-combined-tree --remove-state-nodes  
--max-line-length 69 [email-masked] > 
[email-masked]
diff [email-masked] [email-masked] > 
vrrp-nmda.diff

# Checking the tree output is also sensible.
pyang -f tree [email-masked] > [email-masked]
pyang -f tree --ietf [email-masked] > 
[email-masked]
diff [email-masked] [email-masked] > 
vrrp-nmda-tree.diff

The new VRRP YANG module, New tree output, model diff, and tree diff are 
all attached.

Requires manual fixing:
- revision date/history
- a few "FIX ME" comments to merge config/state description strings.
- check that leaf types match for any config/state nodes on equivalent 
paths where the state leaf has been removed.

Thanks,
Rob

On 20/09/2017 17:35, Alia Atlas wrote:
> As is customary, I have done my AD review of 
> draft-ietf-rtgwg-yang-vrrp-04. First, I would like to thank the 
> authors, Xufeng, Athanasios, Ravi, Acee,and Mingui, as well as the WG 
> for their work on this draft.  It is clear and well-written.
>
> My one issue is that it does not conform to the NMDA guidelines. I 
> know that the transformation can be done largely programmatically - 
> and Acee & Xufeng are quite familiar with the details.  I've also cc'd 
> Rob Wilton who has some tooling to potentially help.
>
> From the shepherd's report, I understand that there is an 
> implementation. That implies that the existing model should be in the 
> appendix.
>

625a626
>       config false;
630a632
>       config false;
635a638
>       config false;
640a644
>       config false;
647a652
>       config false;
654a660
>       config false;
662a669
>       config false;
667a675
>       config false;
672a681
>       config false;
676a686
>         config false;
686a697
>         config false;
692a704
>         config false;
698a711
>         config false;
705a719
>         config false;
713a728
>         config false;
719a735
>         config false;
725a742
>         config false;
732a750
>         config false;
739a758
>         config false;
751a771
>       config false;
756a777
>       config false;
760a782
>       config false;
764a787
>         config false;
776a800
>         config false;
783a808
>         config false;
790a816
>         config false;
797a824
>         config false;
809c836,839
<       "Augment IPv4 interface.";
---
>       "Augment IPv4 interface.
> 
>        FROM STATE TREE (FIX ME):
>        Augment IPv4 interface state.";
813c843,847
<          version 2 or version 3 for IPv4.";
---
>          version 2 or version 3 for IPv4.
> 
>          FROM STATE TREE (FIX ME):
>          State information for Virtual Router Redundancy Protocol
>          (VRRP) version 2 for IPv4.";
817a852,855
>            identifier (VRID), within IPv4 address space.
> 
>            FROM STATE TREE (FIX ME):
>            States of a virtual router, identified by a virtual router
819a858
>         uses vrrp-state-attributes;
825c864,867
<       "Augment IPv6 interface.";
---
>       "Augment IPv6 interface.
> 
>        FROM STATE TREE (FIX ME):
>        Augment IPv6 interface state.";
829c871,875
<          version 3 for IPv6.";
---
>          version 3 for IPv6.
> 
>          FROM STATE TREE (FIX ME):
>          State information of the Virtual Router Redundancy Protocol
>          (VRRP) version 2 or version 3 for IPv6.";
838,870c884,887
<            identifier (VRID), within IPv6 address space.";
<         uses vrrp-ipv6-attributes;
<       }
<     }
<   }
<   augment "/if:interfaces-state/if:interface/ip:ipv4" {
<     description
<       "Augment IPv4 interface state.";
<     container vrrp {
<       description
<         "State information for Virtual Router Redundancy Protocol
<          (VRRP) version 2 for IPv4.";
<       list vrrp-instance {
<         key "vrid";
<         description
<           "States of a virtual router, identified by a virtual router
<            identifier (VRID), within IPv4 address space.";
<         uses vrrp-ipv4-attributes;
<         uses vrrp-state-attributes;
<       }
<     }
<   }
<   augment "/if:interfaces-state/if:interface/ip:ipv6" {
<     description
<       "Augment IPv6 interface state.";
<     container vrrp {
<       description
<         "State information of the Virtual Router Redundancy Protocol
<          (VRRP) version 2 or version 3 for IPv6.";
<       list vrrp-instance {
<         key "vrid";
<         description
<           "States of a virtual router, identified by a virtual router
---
>            identifier (VRID), within IPv6 address space.
> 
>            FROM STATE TREE (FIX ME):
>            States of a virtual router, identified by a virtual router
877c894
<   augment "/if:interfaces-state" {
---
>   augment "/if:interfaces" {
29,30c29,49
<              +--rw virtual-ipv4-address* [ipv4-address]
<                 +--rw ipv4-address    inet:ipv4-address
---
>           |  +--rw virtual-ipv4-address* [ipv4-address]
>           |     +--rw ipv4-address    inet:ipv4-address
>           +--ro state?                          identityref
>           +--ro is-owner?                       boolean
>           +--ro last-adv-source?                inet:ip-address
>           +--ro up-datetime?                    yang:date-and-time
>           +--ro master-down-interval?           uint32
>           +--ro skew-time?                      uint32
>           +--ro last-event?                     identityref
>           +--ro new-master-reason?              new-master-reason-type
>           +--ro statistics
>              +--ro discontinuity-datetime?    yang:date-and-time
>              +--ro master-transitions?        yang:counter32
>              +--ro advertisement-recv?        yang:counter64
>              +--ro advertisement-sent?        yang:counter64
>              +--ro interval-errors?           yang:counter64 {validate-interval-errors}?
>              +--ro priority-zero-pkts-rcvd?   yang:counter64
>              +--ro priority-zero-pkts-sent?   yang:counter64
>              +--ro invalid-type-pkts-rcvd?    yang:counter64
>              +--ro address-list-errors?       yang:counter64 {validate-address-list-errors}?
>              +--ro packet-length-errors?      yang:counter64
53,125c72,73
<              +--rw virtual-ipv6-address* [ipv6-address]
<                 +--rw ipv6-address    inet:ipv6-address
<   augment /if:interfaces-state/if:interface/ip:ipv4:
<     +--ro vrrp
<        +--ro vrrp-instance* [vrid]
<           +--ro vrid                            uint8
<           +--ro version                         identityref
<           +--ro log-state-change?               boolean
<           +--ro preempt
<           |  +--ro enabled?     boolean
<           |  +--ro hold-time?   uint16
<           +--ro priority?                       uint8
<           +--ro accept-mode?                    boolean
<           +--ro (advertise-interval-choice)?
<           |  +--:(v2)
<           |  |  +--ro advertise-interval-sec?         uint8
<           |  +--:(v3)
<           |     +--ro advertise-interval-centi-sec?   uint16
<           +--ro track
<           |  +--ro interfaces
<           |  |  +--ro interface* [interface]
<           |  |     +--ro interface             if:interface-ref
<           |  |     +--ro priority-decrement?   uint8
<           |  +--ro networks
<           |     +--ro network* [prefix]
<           |        +--ro prefix                inet:ipv4-prefix
<           |        +--ro priority-decrement?   uint8
<           +--ro virtual-ipv4-addresses
<           |  +--ro virtual-ipv4-address* [ipv4-address]
<           |     +--ro ipv4-address    inet:ipv4-address
<           +--ro state?                          identityref
<           +--ro is-owner?                       boolean
<           +--ro last-adv-source?                inet:ip-address
<           +--ro up-datetime?                    yang:date-and-time
<           +--ro master-down-interval?           uint32
<           +--ro skew-time?                      uint32
<           +--ro last-event?                     identityref
<           +--ro new-master-reason?              new-master-reason-type
<           +--ro statistics
<              +--ro discontinuity-datetime?    yang:date-and-time
<              +--ro master-transitions?        yang:counter32
<              +--ro advertisement-recv?        yang:counter64
<              +--ro advertisement-sent?        yang:counter64
<              +--ro interval-errors?           yang:counter64 {validate-interval-errors}?
<              +--ro priority-zero-pkts-rcvd?   yang:counter64
<              +--ro priority-zero-pkts-sent?   yang:counter64
<              +--ro invalid-type-pkts-rcvd?    yang:counter64
<              +--ro address-list-errors?       yang:counter64 {validate-address-list-errors}?
<              +--ro packet-length-errors?      yang:counter64
<   augment /if:interfaces-state/if:interface/ip:ipv6:
<     +--ro vrrp
<        +--ro vrrp-instance* [vrid]
<           +--ro vrid                            uint8
<           +--ro version                         identityref
<           +--ro log-state-change?               boolean
<           +--ro preempt
<           |  +--ro enabled?     boolean
<           |  +--ro hold-time?   uint16
<           +--ro priority?                       uint8
<           +--ro accept-mode?                    boolean
<           +--ro advertise-interval-centi-sec?   uint16
<           +--ro track
<           |  +--ro interfaces
<           |  |  +--ro interface* [interface]
<           |  |     +--ro interface             if:interface-ref
<           |  |     +--ro priority-decrement?   uint8
<           |  +--ro networks
<           |     +--ro network* [prefix]
<           |        +--ro prefix                inet:ipv6-prefix
<           |        +--ro priority-decrement?   uint8
<           +--ro virtual-ipv6-addresses
<           |  +--ro virtual-ipv6-address* [ipv6-address]
<           |     +--ro ipv6-address    inet:ipv6-address
---
>           |  +--rw virtual-ipv6-address* [ipv6-address]
>           |     +--rw ipv6-address    inet:ipv6-address
145,146c93,94
<   augment /if:interfaces-state:
<     +--ro vrrp-global
---
>   augment /if:interfaces:
>     +--rw vrrp-global

---

From: Alia Atlas <[email-masked]>
Date: Thu, 21 Sep 2017 10:17:27 -0400

Rob,

Thank you very much!

Regards,
Alia

On Thu, Sep 21, 2017 at 10:02 AM, Robert Wilton <[email-masked]> wrote:

> - It looks through the parallel config/state trees finding nodes with the
> same names on equivalent paths.
> (6) Generically it is used like this (from the directory that contains the
> split YANG module):
>> draft-ietf-rtgwg-yang-vrrp-04. First, I would like to thank the authors,
>> Xufeng, Athanasios, Ravi, Acee,and Mingui, as well as the WG for their work
>> on this draft.  It is clear and well-written.
>>

---

From: Robert Wilton <[email-masked]>
Date: Thu, 21 Sep 2017 15:23:48 +0100

Hi Alia, authors,

Separately when doing the NMDA conversion on the VRRP module, I noted 
that it is directly augmenting the "/interfaces-state" container (rather 
than "/interfaces-state/interface" directly with "VRRP-global" 
container, which looked a bit odd to me (and broke my conversion tool ;-).

E.g.

   augment /if:interfaces-state:
     +--ro vrrp-global
        +--ro virtual-routers?   uint32
        +--ro interfaces?        uint32
        +--ro statistics
           +--ro discontinuity-datetime? yang:date-and-time
           +--ro checksum-errors?          yang:counter64
           +--ro version-errors?           yang:counter64
           +--ro vrid-errors?              yang:counter64
           +--ro ip-ttl-errors?            yang:counter64

This naively seems like the wrong place to me, and I think that it would 
be better to place this either as a top level "vrrp" container, or 
perhaps put under the routing tree (e.g. 
/routing/control-plane-protocols/vrrp).

I would have thought that putting this directly under 
the/interfaces-state container would mean that the /interfaces-state 
container could hold an interleaved mix of interface list entries and 
the vrrp-global container!?!

E.g. I think that with the model the existing design then this following 
XML would be allowed - cc Martin in case I am wrong :-)

        <interfaces-state
            xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces"
            xmlns:ianaift="urn:ietf:params:xml:ns:yang:iana-if-type">

          <interface>
            <name>eth0</name>
            <type>ianaift:ethernetCsmacd</type>
            <admin-status>down</admin-status>
            <oper-status>down</oper-status>
            ...
          </interface>

          <vrrp-global>
             ....
          </vrrp-global>

          <interface>
            <name>eth1</name>
            <type>ianaift:ethernetCsmacd</type>
            <admin-status>up</admin-status>
            <oper-status>up</oper-status>
            ....
          </interface>

          <interface>
            <name>eth1.10</name>
            <type>ianaift:l2vlan</type>
            <admin-status>up</admin-status>
            <oper-status>up</oper-status>
            ....
          </interface>
      </interfaces-state>

Thanks,
Rob

---

From: Jeff Tantsura <[email-masked]>
Date: Thu, 21 Sep 2017 17:00:05 -0700

Thanks Rob!

Dear authors,

please publish the updated draft ASAP.

Thanks! 

Jeff

From: rtgwg <[email-masked]> on behalf of Robert Wilton <[email-masked]>
Date: Thursday, September 21, 2017 at 07:23
To: Alia Atlas <[email-masked]>, "[email-masked]" <[email-masked]>, <[email-masked]>, Martin Bjorklund <[email-masked]>
Subject: Re: AD review of draft-ietf-rtgwg-yang-vrrp-04

Hi Alia, authors,

Separately when doing the NMDA conversion on the VRRP module, I noted that it is directly augmenting the "/interfaces-state" container (rather than "/interfaces-state/interface" directly with "VRRP-global" container, which looked a bit odd to me (and broke my conversion tool ;-).

E.g. 

This naively seems like the wrong place to me, and I think that it would be better to place this either as a top level "vrrp" container, or perhaps put under the routing tree (e.g. /routing/control-plane-protocols/vrrp).

I would have thought that putting this directly under the /interfaces-state container would mean that the /interfaces-state container could hold an interleaved mix of interface list entries and the vrrp-global container!?!

E.g. I think that with the model the existing design then this following XML would be allowed - cc Martin in case I am wrong :-) 
       <interfaces-state
           xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces"
           xmlns:ianaift="urn:ietf:params:xml:ns:yang:iana-if-type">
 
         <interface>
           <name>eth0</name>
 
         <vrrp-global>
            ....
         </vrrp-global>
 
         <interface>
           <name>eth1</name>
 
         <interface>
           <name>eth1.10</name>

As is customary, I have done my AD review of draft-ietf-rtgwg-yang-vrrp-04. First, I would like to thank the authors, Xufeng, Athanasios, Ravi, Acee,and Mingui, as well as the WG for their work on this draft.  It is clear and well-written. 

My one issue is that it does not conform to the NMDA guidelines. I know that the transformation can be done largely programmatically - and Acee & Xufeng are quite familiar with the details.  I've also cc'd Rob Wilton who has some tooling to potentially help.

>From the shepherd's report, I understand that there is an implementation. That implies that the existing model should be in the appendix.

I would be delighted to forward this draft to IETF Last Call (and my apologies for the long delay in review) after it has been updated.

Thanks,

Alia

_______________________________________________ rtgwg mailing list [email-masked] https://www.ietf.org/mailman/listinfo/rtgwg

---

From: Xufeng Liu <[email-masked]>
Date: Fri, 22 Sep 2017 12:43:39 +0000

Thanks to Alia for the review, and Rob for the comments.

We will update the model soon.

As for the vrrp-global container, I think that we will move it to a different location, since if:interfaces-state is deprecated in the NMDA compatible model.

Thanks,
- Xufeng

From: Jeff Tantsura [mailto:[email-masked]]
Sent: Thursday, September 21, 2017 8:00 PM
To: Robert Wilton <[email-masked]>; Alia Atlas <[email-masked]>; [email-masked]; [email-masked]; Martin Bjorklund <[email-masked]>
Subject: Re: AD review of draft-ietf-rtgwg-yang-vrrp-04

Thanks Rob!

Dear authors,

Thanks!
Jeff
From: rtgwg <[email-masked]<mailto:[email-masked]>> on behalf of Robert Wilton <[email-masked]<mailto:[email-masked]>>
Date: Thursday, September 21, 2017 at 07:23
To: Alia Atlas <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, <[email-masked]<mailto:[email-masked]>>, Martin Bjorklund <[email-masked]<mailto:[email-masked]>>
Subject: Re: AD review of draft-ietf-rtgwg-yang-vrrp-04

Hi Alia, authors,

E.g.

       <interfaces-state

         <interface>

           <name>eth0</name>

           ...

         </interface>

         <vrrp-global>

            ....

         </vrrp-global>

         <interface>

           <name>eth1</name>

           ....

         </interface>

         <interface>

           <name>eth1.10</name>

           ....

         </interface>

     </interfaces-state>

Thanks,
Rob

Thanks,
Alia

_______________________________________________ rtgwg mailing list [email-masked]<mailto:[email-masked]> https://www.ietf.org/mailman/listinfo/rtgwg

---

From: Alia Atlas <[email-masked]>
Date: Fri, 22 Sep 2017 09:03:09 -0400

Xufeng,

Thank you very much. I'd like to get this through before next IETF - which
means around a 3 week cycle,
with IETF Last Call for 2 weeks & then needing to be timed for the telechat.

Regards,
Alia

On Fri, Sep 22, 2017 at 8:43 AM, Xufeng Liu <[email-masked]> wrote:

> As for the vrrp-global container, I think that we will move it to a
> different location, since if:interfaces-state is deprecated in the NMDA
> compatible model.
>
>
>
> Thanks,
>
> - Xufeng
>
>
>
> *From:* Jeff Tantsura [mailto:[email-masked]]
> *Sent:* Thursday, September 21, 2017 8:00 PM
> *To:* Robert Wilton <[email-masked]>; Alia Atlas <[email-masked]>;
> [email-masked]; [email-masked]; Martin Bjorklund <
> [email-masked]>
>
> *Subject:* Re: AD review of draft-ietf-rtgwg-yang-vrrp-04
>
>
>
> Thanks Rob!
>
>
>
> Dear authors,
>
> *From: *rtgwg <[email-masked]> on behalf of Robert Wilton <
> [email-masked]>
> *Date: *Thursday, September 21, 2017 at 07:23
> *To: *Alia Atlas <[email-masked]>, "[email-masked]" <[email-masked]>, <
> [email-masked]>, Martin Bjorklund <[email-masked]>
> *Subject: *Re: AD review of draft-ietf-rtgwg-yang-vrrp-04
>
>
>
> Hi Alia, authors,
>
> Separately when doing the NMDA conversion on the VRRP module, I noted that
> it is directly augmenting the "/interfaces-state" container (rather than "
> /interfaces-state/interface" directly with "VRRP-global" container, which
> looked a bit odd to me (and broke my conversion tool ;-).
>
> E.g.
>
> be better to place this either as a top level "vrrp" container, or perhaps
> put under the routing tree (e.g. /routing/control-plane-protocols/vrrp).
>
> I would have thought that putting this directly under the
> /interfaces-state container would mean that the /interfaces-state
> container could hold an interleaved mix of interface list entries and the
> vrrp-global container!?!
>
> _______________________________________________ rtgwg mailing list
> [email-masked] https://www.ietf.org/mailman/listinfo/rtgwg
>

---

From: Xufeng Liu <[email-masked]>
Date: Fri, 22 Sep 2017 13:08:06 +0000

Hi Alia,

Thanks. We will try to complete the update within a week.

Best,
- Xufeng

From: Alia Atlas [mailto:[email-masked]]
Sent: Friday, September 22, 2017 9:03 AM
To: Xufeng Liu <[email-masked]>
Cc: Jeff Tantsura <[email-masked]>; Robert Wilton <[email-masked]>; [email-masked]; [email-masked]; Martin Bjorklund <[email-masked]>
Subject: Re: AD review of draft-ietf-rtgwg-yang-vrrp-04

Xufeng,

Thank you very much. I'd like to get this through before next IETF - which means around a 3 week cycle,
with IETF Last Call for 2 weeks & then needing to be timed for the telechat.

Regards,
Alia

On Fri, Sep 22, 2017 at 8:43 AM, Xufeng Liu <[email-masked]<mailto:[email-masked]>> wrote:
Thanks to Alia for the review, and Rob for the comments.

Thanks,
- Xufeng

From: Jeff Tantsura [mailto:[email-masked]<mailto:[email-masked]>]
Sent: Thursday, September 21, 2017 8:00 PM
To: Robert Wilton <[email-masked]<mailto:[email-masked]>>; Alia Atlas <[email-masked]<mailto:[email-masked]>>; [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>; Martin Bjorklund <[email-masked]<mailto:[email-masked]>>

Thanks Rob!

Dear authors,

Thanks!
Jeff

Hi Alia, authors,

E.g.

       <interfaces-state

         <interface>

           <name>eth0</name>

           ...

         </interface>

         <vrrp-global>

            ....

         </vrrp-global>

         <interface>

           <name>eth1</name>

           ....

         </interface>

         <interface>

           <name>eth1.10</name>

           ....

         </interface>

     </interfaces-state>

Thanks,
Rob

Thanks,
Alia