Subject: Re: [mpls] MPLS-RT review of draft-raza-mpls-ldp-mldp-yang-03

From: "Tarek Saad (tsaad)" <[email-masked]>
Date: Sat, 18 Jun 2016 17:30:18 +0000

Hi co-authors,

I have reviewed the version of draft “draft-raza-mpls-ldp-mldp-yang-03”. This draft is useful as it describes a data model that allows operating and managing widely deployed MPLS LDP and MLDP protocols. I think it is at a good starting point to be adopted by the WG.
My comments are below and can wait until the document is a working group document and the working group has the revision control.

General comments:

·         You use of if-feature checks in this model. One feedback from the operators was to possibly split a model into a “base” and “extended” parts- with the “base” part to contain mandatory data nodes that all vendors are expected to support and hence would not have any if-feature check. The “extended” model would contain additional features that can be if-feature checked. I’ll leave it up to you to consider this approach.

·         You are defining MLDP and LDP in same model. You may want to consider having MLDP model (either in a submodule that can included in the ldp module) or a separate module that imports LDP and augments it.

·         Since the model is hanging down from mpls, it is ok to drop the “mpls” from all leaf names (e.g. s/mpls-ldp/ldp)

Section 3.1:
>>   o  Read-Write parameters for configuration (Discussed in Section 3.2)
[TS]: the model in I-D.openconfig-netmod-opstate suggests having read-only leaves for applied configuration items hanging down the same branch

Section 3.2:
>> augment /rt:routing/rt:routing-instance/mpls:mpls:
[TS]: the above path has changed in draft-ietf-netmod-routing-cfg-21 and rt:routing-instance is not longer there. You’ll need to make the necessary changes

>> The "interfaces" is a container to configure parameters related to  VRF interfaces.
[TS]: you seem to be creating a separate “interfaces” container and list (and adding checks for example to ensure ipv4 is enabled). To ensure MPLS is enabled, you’d need to do the same. Or alternatively, you could augment the MPLS enabled interfaces listed in MPLS-BASE model?

>> typedef address-family
[TS]: rfc6991 defines “ip-version” data-type- which is similar to the type you’re defining. You may want to use that instead.

>> You are defining several data-types as enums. You may want to consider identities for types you think may be extended by other modules.

>> oper-status-event-type
Is this specific to events? not applicable to state?

>> use of defaults (e.g. hello-interval, etc.) – unless dictated by standard may not be favorable.. You may want to consider leaving it flexible and allow vendor backend to apply their internal default.

>> use of “presence” keyword to enable functions. From feedback, having explicit “enable” leaf was more favorable (avoids issues

>>      +--ro label?                uint32
[TS]: please consider using mpls:mpls-label instead in similar occurrence

Regards,
Tarek

From: Ross Callon <[email-masked]>
Date: Thursday, May 26, 2016 at 12:36 PM
To: "'Bert (IETF) Wijnen'" <[email-masked]>, Mach Chen <[email-masked]>, Tarek Saad <[email-masked]>, Minto Jeyananth <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, "Kamran Raza (skraza)" <[email-masked]>, "Reshad Rahman (rrahman)" <[email-masked]>, "Rajiv Asati (rajiva)" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, Santosh Esale <[email-masked]>, "[email-masked]" <[email-masked]>, Loa Andersson <[email-masked]>, "[email-masked]" <[email-masked]>, "Bocci, Matthew (Matthew)" <[email-masked]>, "[email-masked]" <[email-masked]>, "Sowmya Krishnaswamy (sowkrish)" <[email-masked]>, "Danial Johari (dajohari)" <[email-masked]>
Subject: MPLS-RT review of draft-raza-mpls-ldp-mldp-yang-03

Bert, Mach, Tarek, Minto;

You have be selected as MPLS review team reviewers for draft-raza-mpls-ldp-mldp-yang-03.

Note to authors: You have been CC'd on this email so that you can know
that this review is going on. However, please do not review your own
document.

Reviews should comment on whether the document is coherent, is it
useful (ie, is it likely to be actually useful in operational networks), and is
the document technically sound?  We are interested in knowing whether
the document is ready to be considered for WG adoption (ie, it doesn't
have to be perfect at this point, but should be a good start).

Reviews should be sent to the document authors, WG co-chairs and WG
secretary, and CC'd to the MPLS WG email list. If necessary, comments
may be sent privately to only the WG chairs.

If you have technical comments you should try to be explicit about what
needs to be resolved before adopting it as a working group document, and
what can wait until the document is a working group document and the
working group has the revision control.

Because of the size of the document we will increase the review time to three
weeks. Are you able to review this draft by Friday June 17, 2016? Please respond
in a timely fashion.

Thanks, Ross
(as MPLS WG chair)

---

From: "Kamran Raza (skraza)" <[email-masked]>
Date: Mon, 27 Jun 2016 00:01:43 +0000

Hi Tarek,

Thanks for your comments. Please see inline [skraza]:

From: "Tarek Saad (tsaad)" <[email-masked]<mailto:[email-masked]>>
Date: Saturday, June 18, 2016 at 1:30 PM
To: Syed Kamran Raza <[email-masked]<mailto:[email-masked]>>, "Reshad Rahman (rrahman)" <[email-masked]<mailto:[email-masked]>>, "Rajiv Asati (rajiva)" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, Santosh Esale <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, Loa Andersson <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "Bocci, Matthew (Matthew)" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "Sowmya Krishnaswamy (sowkrish)" <[email-masked]<mailto:[email-masked]>>, "Danial Johari (dajohari)" <[email-masked]<mailto:[email-masked]>>
Cc: "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "Tarek Saad (tsaad)" <[email-masked]<mailto:[email-masked]>>
Subject: Re: MPLS-RT review of draft-raza-mpls-ldp-mldp-yang-03

Hi co-authors,

General comments:

[skraza]: We discussed this in our weekly meeting and we will revisit this in later rev.

[skraza]: mLDP is tightly coupled with LDP and this decision (of having a joint model) was made in one of our earliest arch/design meetings. We had also sought WG direction on this during our 1st presentation and WG had also advised to have a common/combined model.

[skraza]: Sure.

[TS]: the model in I-D.openconfig-netmod-opstate suggests having read-only leaves for applied configuration items hanging down the same branch
[skraza]: Yes, we are waiting for open cfg opstate model/discussions to settle in netmod/rtg area and will align our model later accordingly.

[TS]: the above path has changed in draft-ietf-netmod-routing-cfg-21 and rt:routing-instance is not longer there. You’ll need to make the necessary changes
[skraza]: Yes, we are aware and this is on our TODO list for next rev.

[TS]: you seem to be creating a separate “interfaces” container and list (and adding checks for example to ensure ipv4 is enabled). To ensure MPLS is enabled, you’d need to do the same. Or alternatively, you could augment the MPLS enabled interfaces listed in MPLS-BASE model?
[skraza]: Right, we had indicated in our Yokohama presentation that we will align with mpls-base. We missed it in our prev rev but will fix it in next. Thx.

[TS]: rfc6991 defines “ip-version” data-type- which is similar to the type you’re defining. You may want to use that instead.
 [skraza]: We got the same comment from other reviewer and will address it.

>> You are defining several data-types as enums. You may want to consider identities for types you think may be extended by other modules.
 [skraza]: Ok, we will evaluate for later rev.

[skraza]: Yes.

>> use of defaults (e.g. hello-interval, etc.) – unless dictated by standard may not be favorable.. You may want to consider leaving it flexible and allow vendor backend to apply their internal defau…
[skraza]: Agree. The team had discussed the default a bit but we have not fully closed on all. Will update in later rev.

>> use of “presence” keyword to enable functions. From feedback, having explicit “enable” leaf was more favorable (avoids issues
[skraza]: Ok, will rework.

 [skraza]:  Sure.

Regards,
Tarek

From: Ross Callon <[email-masked]<mailto:[email-masked]>>
Date: Thursday, May 26, 2016 at 12:36 PM
To: "'Bert (IETF) Wijnen'" <[email-masked]<mailto:[email-masked]>>, Mach Chen <[email-masked]<mailto:[email-masked]>>, Tarek Saad <[email-masked]<mailto:[email-masked]>>, Minto Jeyananth <[email-masked]<mailto:[email-masked]>>
Cc: "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "Kamran Raza (skraza)" <[email-masked]<mailto:[email-masked]>>, "Reshad Rahman (rrahman)" <[email-masked]<mailto:[email-masked]>>, "Rajiv Asati (rajiva)" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, Santosh Esale <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, Loa Andersson <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "Bocci, Matthew (Matthew)" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "Sowmya Krishnaswamy (sowkrish)" <[email-masked]<mailto:[email-masked]>>, "Danial Johari (dajohari)" <[email-masked]<mailto:[email-masked]>>
Subject: MPLS-RT review of draft-raza-mpls-ldp-mldp-yang-03

Thanks, Ross
(as MPLS WG chair)