Subject: Yangdoctors early partial review of draft-ietf-mpls-ldp-yang-01

From: Dean Bogdanovic <[email-masked]>
Date: Fri, 08 Sep 2017 06:10:52 -0700

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

---

From: "Rajiv Asati (rajiva)" <[email-masked]>
Date: Fri, 8 Sep 2017 19:25:35 +0000

Hi Dean,

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

>    approach when modeling ACLs.
 
Thanks for pointing it out. We co-authors discussed quite a bit on this before settling on it.

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

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

It should be. 

>    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?
   
>    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 <[email-masked]>
Date: Friday, September 8, 2017 at 9:10 AM
To: "[email-masked]" <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, IETF Discussion <[email-masked]>
Subject: Yangdoctors early partial review of draft-ietf-mpls-ldp-yang-01
Resent-From: <[email-masked]>
Resent-To: <[email-masked]>, Rajiv Asati <[email-masked]>, <[email-masked]>, <[email-masked]>, <[email-masked]>, Himanshu Shah <[email-masked]>, <[email-masked]>, <[email-masked]>, "[email-masked]" <[email-masked]>, Loa Andersson <[email-masked]>, <[email-masked]>, DEBORAH BRUNGARD <[email-masked]>, <[email-masked]>
Resent-Date: Friday, September 8, 2017 at 9:10 AM

    

    
    4.1.1. Base

    
    
    module: ietf-mpls-ldp
    
    9.1 Base
    
       leaf peer-ldp-id {
         type yang:dotted-quad;

---

From: "Kamran Raza (skraza)" <[email-masked]>
Date: Fri, 15 Sep 2017 03:21:09 +0000

Thanks for your review and comments. 

We further discussed the comments in our bi-weekly meeting amongst the authors.
Please see some clarifications/response inline tagged as [skraza].

On 2017-09-08, 3:25 PM, "Rajiv Asati (rajiva)" <[email-masked]> wrote:

    Hi Dean,

     

    
    
[skraza]: We had discussed this matter at length in our meetings when doing the base/extended categorization.
In our opinion, “ipv6” LDP falls under extended given our definition of a “base” – see quote below from our draft:
“The "base" category contains the basic and fundamental features that
   are covered in LDP base specification [RFC5036] and constitute the
   minimum requirements for a typical base LDP deployment”
Note that LDP base RFC 5036 did not cover IPv6 fully/properly, and hence LDPv6 was not deployable until we came up with its own RFC 7552.

    
    It should be. 

    
 
[skraza]: Infact, discovery, peer, and global should be on the same level. Looks like discovery got moved under “global” by mistake.
  We will fix it in our next major rev.

    No. 

[skraza]: We will define ldp-id type as { lsr-id:label-space-id}  and use it for peer-ldp-id.

Thx.
--
Kamran

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

        

        
        4.1.1. Base

        
        
        module: ietf-mpls-ldp
        
        9.1 Base
        
           leaf peer-ldp-id {
             type yang:dotted-quad;