Re: [mpls] Yangdoctors early partial review of draft-ietf-mpls-ldp-yang-01

["Rajiv Asati (rajiva)" <[e-mail]>]

Hi Dean,

Thank you so much for your review and feedback. Please see inline,

>   In general, I like you design with base and extended model. Think it is a very
>   good approach in modeling and maybe I as author should have taken a similar
>    approach when modeling ACLs.
 
Thanks for pointing it out. We co-authors discussed quite a bit on this before settling on it.

 
>   Why are IPv4 and v6 separated? Why is IPv6 missing from the base model? The RFC
>    5036 is specifying both AF is the document. And you are mentioning that RFC
>    5036 is used for base model. Wy having same hierarchy but different namespaces?

Good point. I somewhat agree though there are 2 points to consider – 
a) capabilities are covered mostly in Extended model (and LDP IPv6 requires a dual-stack capability)
b) LDP IPv6 is mostly specified in rfc7552, beyond rfc5036

Nonethless, co-authors could discuss and revert, given the v6-only may become a norm at some point.


>    4.1.1. Base
>  
>   1. should 'discovery' be under 'global'?

It should be. 

>    Especially the 'interfaces' section. 'Interfaces' are directly attached to the
>    device. Compared to peers which is the remote end (either that be directly
>    connected peers, or peers that are more than 1 hop away). If peers are not
>    under global, it seems that 'discovery' and especially 'discovery/interfaces'
>    should not be under 'global'.
 
Peers are under ‘global’. 

Perhaps, you allude to making ‘discovery’ and ‘peers’ consistent – either under global or global config.  Is that it?
   
>       leaf peer-ldp-id {
>         type yang:dotted-quad;
>         description "Peer LDP ID.";
>       }
>    Nit: "peer-" prefix needed?  

No. 

However, LDP ID should not be a dotted-quad, since it is a 6-octet value. 
We will fix it in the next version.

>    Is this value different from peer LSR ID?
    
Yes. LSR ID is the first 4 octets of LDP ID.
 
    

-- 
Cheers,
Rajiv Asati
Distinguished Engineer, Cisco

-----Original Message-----
From: Dean Bogdanovic <[e-mail]>
Date: Friday, September 8, 2017 at 9:10 AM
To: "[e-mail]" <[e-mail]>
Cc: "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, IETF Discussion <[e-mail]>
Subject: Yangdoctors early partial review of draft-ietf-mpls-ldp-yang-01
Resent-From: <[e-mail]>
Resent-To: <[e-mail]>, Rajiv Asati <[e-mail]>, <[e-mail]>, <[e-mail]>, <[e-mail]>, Himanshu Shah <[e-mail]>, <[e-mail]>, <[e-mail]>, "[e-mail]" <[e-mail]>, Loa Andersson <[e-mail]>, <[e-mail]>, DEBORAH BRUNGARD <[e-mail]>, <[e-mail]>
Resent-Date: Friday, September 8, 2017 at 9:10 AM

    
    Review is partially done. Another review request has been registered for
    completing it.
    
    Reviewer: Dean Bogdanovic
    Review result: On the Right Track
    
    In general, I like you design with base and extended model. Think it is a very
    good approach in modeling and maybe I as author should have taken a similar
    approach when modeling ACLs.
    
    But there is a bit of confusion for me
    
    Why are IPv4 and v6 separated? Why is IPv6 missing from the base model? The RFC
    5036 is specifying both AF is the document. And you are mentioning that RFC
    5036 is used for base model. Wy having same hierarchy but different namespaces?
    
    4.1.1. Base
    
    1. should 'discovery' be under 'global'?
    
    Especially the 'interfaces' section. 'Interfaces' are directly attached to the
    device. Compared to peers which is the remote end (either that be directly
    connected peers, or peers that are more than 1 hop away). If peers are not
    under global, it seems that 'discovery' and especially 'discovery/interfaces'
    should not be under 'global'.
    
    This applies to 'discovery/targeted' as well.
    
    module: ietf-mpls-ldp
    augment /rt:routing/rt:control-plane-protocols:
       +--rw mpls-ldp!
          +--rw global
          |  +--rw config
          |  |  +--rw capability
          |  |  +--rw graceful-restart
          |  |  |  +--rw enable?                boolean
          |  |  |  +--rw reconnect-time?        uint16
          |  |  |  +--rw recovery-time?         uint16
          |  |  |  +--rw forwarding-holdtime?   uint16
          |  |  +--rw lsr-id?             yang:dotted-quad
          |  +--rw address-families
          |  |  +--rw ipv4
          |  |     +--rw config
          |  |        +--rw enable?         boolean
          |  |        +--rw label-policy
          |  |           +--rw advertise
          |  |              +--rw egress-explicit-null
          |  |                 +--rw enable?   boolean
          |  +--rw discovery
          |     +--rw interfaces
          |     |  +--rw config
          |     |  |  +--rw hello-holdtime?   uint16
          |     |  |  +--rw hello-interval?   uint16
          |     |  +--rw interface* [interface]
          |     |     +--rw interface           mpls-interface-ref
          |     |     +--rw address-families
          |     |        +--rw ipv4
          |     |           +--rw config
          |     |              +--rw enable?   boolean
    
    9.1 Base
    
       leaf peer-ldp-id {
         type yang:dotted-quad;
         description "Peer LDP ID.";
       }
    
    Nit: "peer-" prefix needed?
    
    Is this value different from peer LSR ID?
    
    In general, I believe you are on the right path, but I don’t have deep LDP
    knowledge, so can’t understand some of the nuances that you might have wanted
    to address