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

From: Alia Atlas <[email-masked]>
Date: Wed, 20 Sep 2017 13:27:09 -0400

As is customary, I have done my AD review of draft-ietf-rtgwg-yang-rip-04.
First, I would like to thank the authors, Xufeng, Prateek, and Vikram, as
well as the WG for their work on this document.

My one major issue is that this does not conform to the NMDA guidelines -
where augmenting -state models is not preferred.  It is quite acceptable to
have that in an appendix, if there are implementations. I do see the
shepherd's write-up indicates a partial implementation exists.
There is some tooling to help convert a model to conform to NMDA; I've cc'd
Rob Wilton, who was working on that.

I also have some questions.

1) For the prefix-set-ref, I don't see any information about what the
string should contain.

2) For the route-policy-ref, I don't see any information about what the
string should contain.

Nits:
a) p.26:"choice auth-type-selection {
                 description
                   "Specify the authentication scheme.
                    The use of the key-chain reference here is
                    designed to align with other proposed protocol
                    models.";"
   Since the key-chain model is approved for RFC publication, the
description can be updated.

Once the model conforms to the NMDA guidelines, I will be happy to advance
this draft to IETF Last Call.

Thanks,
Alia

---

From: Robert Wilton <[email-masked]>
Date: Thu, 21 Sep 2017 16:00:44 +0100

So the conversion tool has worked OK on the RIP model as well, but I 
spotted a few areas where manual conversion is required (because the 
types/structure between config and state differ):

So along with the revision date, and a few FIX ME comments, the 
following few places also need to be manually tweaked/fixed:

rwilton@rwilton-lnx:~/ietf-models-to-combined/draft_modules$ pyang -f 
tree --ietf [email-masked] > 
[email-masked]
[email-masked]:1: warning: unexpected modulename 
"ietf-rip" in [email-masked], should be ietf-rip-nmda
[email-masked]:1: warning: unexpected latest revision 
"2017-06-05" in [email-masked], should be 2017-09-21
[email-masked]:740: error: unexpected keyword "type"
<- Means that config and state type differ.

[email-masked]:761: error: unexpected keyword "type"
<- Means that config and state type differ.

[email-masked]:818: error: there is already a child node 
to "interface" at [email-masked]:636 with the name 
"originate-default-route" defined at [email-masked]:731 
(at [email-masked]:141)
<- Trying to merge an "originate-default-route" leaf from the state tree 
with the "originate-default-route" container in the equivalent config tree.

RIB YANG model converted to NMDA structure attached.

Thanks,
Rob

On 20/09/2017 18:27, Alia Atlas wrote:
> As is customary, I have done my AD review of 
> draft-ietf-rtgwg-yang-rip-04. First, I would like to thank the 
> authors, Xufeng, Prateek, and Vikram, as well as the WG for their work 
> on this document.
>
> My one major issue is that this does not conform to the NMDA 
> guidelines - where augmenting -state models is not preferred.  It is 
> quite acceptable to have that in an appendix, if there are 
> implementations. I do see the shepherd's write-up indicates a partial 
> implementation exists.
> There is some tooling to help convert a model to conform to NMDA; I've 
> cc'd Rob Wilton, who was working on that.
>
> 2) For the route-policy-ref, I don't see any information about what 
> the string should contain.
>
> Nits:
> Once the model conforms to the NMDA guidelines, I will be happy to 
> advance this draft to IETF Last Call.
>
> Thanks,
> Alia

612c612,616
<          of RIP (type 'ripv2' or 'ripng').";
---
>          of RIP (type 'ripv2' or 'ripng').
> 
>          FROM STATE TREE (FIX ME):
>          This augment is only valid for a routing protocol instance
>          of type 'ripv2' or 'ripng'.";
615c619,622
<       "RIP augmentation.";
---
>       "RIP augmentation.
> 
>        FROM STATE TREE (FIX ME):
>        RIP state.";
618c625,628
<         "RIP configuration data.";
---
>         "RIP configuration data.
> 
>          FROM STATE TREE (FIX ME):
>          RIP operational state.";
726c736,740
<                specified interface.";
---
>                specified interface.
> 
>                FROM STATE TREE (FIX ME):
>                Sending RIP packets is disabled on the interface.";
>             type boolean;
743c757,761
<                specified interface.";
---
>                specified interface.
> 
>                FROM STATE TREE (FIX ME):
>                'true' if Split Horizon processing is enabled.";
>             type boolean;
771,810d788
<         }
<       }
<     }
<   }
<   augment "/rt:routing-state/rt:control-plane-protocols/rt:control-plane-protocol" {
<     when "derived-from(rt:type, 'rip:rip')" {
<       description
<         "This augment is only valid for a routing protocol instance
<          of type 'ripv2' or 'ripng'.";
<     }
<     description
<       "RIP state.";
<     container rip {
<       description
<         "RIP operational state.";
<       uses global-attributes;
<       uses distribute-lists;
<       leaf next-triggered-update {
<         type uint32;
<         description
<           "Next triggered update.";
<       }
<       leaf num-of-routes {
<         type uint32;
<         description
<           "The number of routes.";
<       }
<       uses timers-container;
<       container interfaces {
<         description
<           "Containing a list of RIP interfaces.";
<         list interface {
<           key "interface";
<           description
<             "List of RIP interfaces.";
<           leaf interface {
<             type if:interface-ref;
<             description
<               "Enable RIP on this interface.";
<           }
822a801
>             config false;
826,833d804
<           leaf cost {
<             type uint8 {
<               range "1..16";
<             }
<             default "1";
<             description
<               "Interface cost.";
<           }
835a807
>             config false;
841a814
>             config false;
846a820
>             config false;
850,854d823
<           leaf passive {
<             type boolean;
<             description
<               "Sending RIP packets is disabled on the interface.";
<           }
856a826
>             config false;
861,865d830
<           leaf split-horizon {
<             type boolean;
<             description
<               "'true' if Split Horizon processing is enabled.";
<           }
867a833
>             config false;
871d836
<           uses timers-container;
873a839
>             config false;
912a879,890
>       leaf next-triggered-update {
>         type uint32;
>         config false;
>         description
>           "Next triggered update.";
>       }
>       leaf num-of-routes {
>         type uint32;
>         config false;
>         description
>           "The number of routes.";
>       }
917a896
>         config false;
989a969
>         config false;
1059a1040
>         config false;
65,122c65,99
<           +--rw interface* [interface]
<              +--rw interface                  if:interface-ref
<              +--rw authentication
<              |  +--rw (auth-type-selection)?
<              |     +--:(auth-key-chain)
<              |     |  +--rw key-chain?          key-chain:key-chain-ref
<              |     +--:(auth-key)
<              |        +--rw key?                string
<              |        +--rw crypto-algorithm?   identityref
<              +--rw bfd {bfd}?
<              |  +--rw enabled?                    boolean
<              |  +--rw local-multiplier?           bfd-multiplier
<              |  +--rw (interval-config-type)?
<              |     +--:(tx-rx-intervals)
<              |     |  +--rw desired-min-tx-interval     uint32
<              |     |  +--rw required-min-rx-interval    uint32
<              |     +--:(single-interval)
<              |        +--rw min-interval                uint32
<              +--rw cost?                      uint8
<              +--rw neighbors {explicit-neighbors}?
<              |  +--rw neighbor* [address]
<              |     +--rw address    inet:ip-address
<              +--rw no-listen?                 empty
<              +--rw originate-default-route!
<              |  +--rw route-policy?   route-policy-ref
<              +--rw passive?                   empty
<              +--rw split-horizon?             enumeration
<              +--rw summary-address
<              |  +--rw address?   inet:ip-prefix
<              |  +--rw metric?    uint8
<              +--rw timers
<                 +--rw update-interval?     uint16
<                 +--rw invalid-interval?    uint16
<                 +--rw holddown-interval?   uint16
<                 +--rw flush-interval?      uint16
<   augment /rt:routing-state/rt:control-plane-protocols/rt:control-plane-protocol:
<     +--ro rip
<        +--ro originate-default-route!
<        |  +--ro route-policy?   route-policy-ref
<        +--ro default-metric?               uint8
<        +--ro distance?                     uint8
<        +--ro triggered-update-threshold?   uint8
<        +--ro maximum-paths?                uint8
<        +--ro output-delay?                 uint8
<        +--ro distribute-list* [prefix-set-name direction]
<        |  +--ro prefix-set-name    prefix-set-ref
<        |  +--ro direction          enumeration
<        |  +--ro if-name?           if:interface-ref
<        +--ro next-triggered-update?        uint32
<        +--ro num-of-routes?                uint32
<        +--ro timers
<        |  +--ro update-interval?     uint16
<        |  +--ro invalid-interval?    uint16
<        |  +--ro holddown-interval?   uint16
<        |  +--ro flush-interval?      uint16
<        +--ro interfaces
<        |  +--ro interface* [interface]
<        |     +--ro interface                  if:interface-ref
---
>        |  +--rw interface* [interface]
>        |     +--rw interface                  if:interface-ref
>        |     +--rw authentication
>        |     |  +--rw (auth-type-selection)?
>        |     |     +--:(auth-key-chain)
>        |     |     |  +--rw key-chain?          key-chain:key-chain-ref
>        |     |     +--:(auth-key)
>        |     |        +--rw key?                string
>        |     |        +--rw crypto-algorithm?   identityref
>        |     +--rw bfd {bfd}?
>        |     |  +--rw enabled?                    boolean
>        |     |  +--rw local-multiplier?           bfd-multiplier
>        |     |  +--rw (interval-config-type)?
>        |     |     +--:(tx-rx-intervals)
>        |     |     |  +--rw desired-min-tx-interval     uint32
>        |     |     |  +--rw required-min-rx-interval    uint32
>        |     |     +--:(single-interval)
>        |     |        +--rw min-interval                uint32
>        |     +--rw cost?                      uint8
>        |     +--rw neighbors {explicit-neighbors}?
>        |     |  +--rw neighbor* [address]
>        |     |     +--rw address    inet:ip-address
>        |     +--rw no-listen?                 empty
>        |     +--rw originate-default-route!
>        |     |  +--rw route-policy?   route-policy-ref
>        |     +--rw passive?                   empty
>        |     +--rw split-horizon?             enumeration
>        |     +--rw summary-address
>        |     |  +--rw address?   inet:ip-prefix
>        |     |  +--rw metric?    uint8
>        |     +--rw timers
>        |     |  +--rw update-interval?     uint16
>        |     |  +--rw invalid-interval?    uint16
>        |     |  +--rw holddown-interval?   uint16
>        |     |  +--rw flush-interval?      uint16
124d100
<        |     +--ro cost?                      uint8
128d103
<        |     +--ro passive?                   boolean
130d104
<        |     +--ro split-horizon?             boolean
132,136d105
<        |     +--ro timers
<        |     |  +--ro update-interval?     uint16
<        |     |  +--ro invalid-interval?    uint16
<        |     |  +--ro holddown-interval?   uint16
<        |     |  +--ro flush-interval?      uint16
141a111,112
>        +--ro next-triggered-update?        uint32
>        +--ro num-of-routes?                uint32

---

From: Jeff Tantsura <[email-masked]>
Date: Thu, 21 Sep 2017 16:56:52 -0700

Thanks Rob!

Dear authors,
please publish the updated draft ASAP.

Thanks! 
Jeff
-----Original Message-----
From: rtgwg <[email-masked]> on behalf of Robert Wilton <[email-masked]>
Date: Thursday, September 21, 2017 at 08:01
To: Alia Atlas <[email-masked]>, "[email-masked]" <[email-masked]>, <[email-masked]>
Subject: Re: AD review of draft-ietf-rtgwg-yang-rip-04

    

    
    
    Thanks,
    Rob

    > advance this draft to IETF Last Call.
    
    _______________________________________________
    rtgwg mailing list
    [email-masked]
    https://www.ietf.org/mailman/listinfo/rtgwg

---

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

Thanks to Alia for the review the comments.
Thanks to Rob for putting the model through the conversion tool, and providing the suggestions.

We will update the model soon.

Regards,
- Xufeng

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

---

From: Alia Atlas <[email-masked]>
Date: Fri, 22 Sep 2017 09:02:45 -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

---

From: Xufeng Liu <[email-masked]>
Date: Fri, 22 Sep 2017 13:07:34 +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]
Subject: Re: AD review of draft-ietf-rtgwg-yang-rip-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:49 AM, Xufeng Liu <[email-masked]<mailto:[email-masked]>> wrote:
Thanks to Alia for the review the comments.
Thanks to Rob for putting the model through the conversion tool, and providing the suggestions.

Regards,
- Xufeng

> From: Jeff Tantsura [mailto:[email-masked]<mailto:[email-masked]>]
> 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]>
> From: rtgwg <[email-masked]<mailto:[email-masked]>> on behalf of Robert Wilton
> <[email-masked]<mailto:[email-masked]>>
> To: Alia Atlas <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>,
> <[email-masked]<mailto:[email-masked]>>
>     tree --ietf [email-masked]<mailto:[email-masked]> >
>     [email-masked]<mailto:[email-masked]>
>     [email-masked]:1<mailto:[email-masked]:1>: warning: unexpected modulename
>     "ietf-rip" in [email-masked]<mailto:[email-masked]>, should be ietf-rip-nmda
>     [email-masked]:1<mailto:[email-masked]:1>: warning: unexpected latest revision
>     "2017-06-05" in [email-masked]<mailto:[email-masked]>, should be 2017-09-21
>     [email-masked]:740<mailto:[email-masked]:740>: error: unexpected keyword "type"
>     [email-masked]:761<mailto:[email-masked]:761>: error: unexpected keyword "type"
>     [email-masked]:818<mailto:[email-masked]:818>: error: there is already a child node
>     to "interface" at [email-masked]:636<mailto:[email-masked]:636> with the name
>     "originate-default-route" defined at [email-masked]:731<mailto:[email-masked]:731>
>     (at [email-masked]:141<mailto:[email-masked]:141>)
>     [email-masked]<mailto:[email-masked]>