[yang-doctors] Re: Yangdoctors early review of draft-ietf-mpls-mldp-yang-11

["Joe Clarke (jclarke)" <[e-mail]>]

Thanks for addressing the comments and providing the additional context as to decisions made.  From what I can see, you addressed my points in rev -12.

Joe

From: Kamran Raza (skraza) <[e-mail]>
Date: Thursday, December 12, 2024 at 12:02
To: Joe Clarke (jclarke) <[e-mail]>, [e-mail] <[e-mail]>
Cc: [e-mail] <[e-mail]>, [e-mail] <[e-mail]>, [e-mail] <[e-mail]>, [e-mail] <[e-mail]>
Subject: re: Yangdoctors early review of draft-ietf-mpls-mldp-yang-11
Hello Joe/YANG Doctors,

Thanks for your detailed review of the mLDP YANG model.

We have posted new rev (https://datatracker.ietf.org/doc/html/draft-ietf-mpls-mldp-yang-12 ) on Dec1.
This rev/update addresses most of your comments.
Please take a look and let us know if there is anything more to be addressed  or fixed.

Furthermore, please see blow my inline responses tagged with [skraza] to your review comments:

From: Joe Clarke via Datatracker <[e-mail]>
Date: Monday, July 29, 2024 at 4:11 PM
To: [e-mail] <[e-mail]>
Cc: [e-mail] <[e-mail]>, [e-mail] <[e-mail]>
Subject: Yangdoctors early review of draft-ietf-mpls-mldp-yang-11
Reviewer: Joe Clarke
Review result: Almost Ready

I have been tasked to review the latest rev on behalf of the YANG Doctors.
This draft defines two YANG modules for multipoint label distribution protocol
(mLDP) in support of an overall YANG data model for the technology.  The intent
is to augment the existing ietf-mpls-ldp module.

Overall, I would suggest running a spell check.  I found multiple typos in the
text (augement, minumum, hiearchy, alongwith, Alteratively, acheived, familty,
digram, avaiable, higlighting, mutipoint).
***
[skraza]: All fixed now.
***

The modules adhere to NMDA, which is stated in the Overview, but missing from
the Introduction (as mentioned in RFC8407), but I don't think that's a huge
deal.  The tree diagrams do reference RFC8340.
***
[skraza]: Updated the Introduction section as well to highlight the NDMA compliance.
***

In terms of the modules themselves, they fall victim to one of my pet peeves:
tautological descriptions.  Take for example "timeout" in the mldp-capabilities
grouping.  The description is just "Timeout in seconds."  This might be obvious
to someone versed in the technology, but it doesn't help those that might be
working more on the automation side of mLDP.  There are a scattering of
"reference" properties for mLDP, but I was hoping to see more (e.g., references
for MBB).  The extended module does a much better job than the base module in
this regard.  I would suggest revisiting descriptions and make sure they are
clear, informative, and have references to documents that describe the mLDP
protocol semantics.
***
[skraza]: Updated several descriptions and added references as applicable.
***

In terms of structure, I noticed a few groupings (e.g.,
mldp-binding-label-state-attributes) that have a relative leafref to an object
outside of the grouping itself.  In a different context, that leafref might not
resolve.  This is not a problem per se, but the grouping description must state
the context(s) in which the grouping can be used (see Section 4.6.1 of 8407).

You have presence containers (one in each module).  The description is slightly
off with what one might expect with a presence container.  According to 7950, a
presence container, when created, can enable a protocol or capability.  The
example there is with SSH.  In your case of ipv4 and ipv6, I might restate the
presence as:

ipv4 {
  presence "Enables IPv4 mLDP support.";
...
}

ipv6 {
  presence "Enables IPv6 mLDP support.";
...
}
***
[skraza]: mDLP model follows closely with LDP YANG and we had lengthy discussion during LDP YANG IESG review along the same lines.In this rev-12 of mLDP, we have now made changes to mLDP to align with what was done for LDP YANG model (now an RFC).
***

Speaking of this ipv6 container, I see that you have a child object in the
opaque-element-transit container, source-address.  This is of type
inet:ip-address vs. inet:ipv6-address (same kind of comment for group-address
and the rp and group-address leafs in opaque-element-bidir child container).
Additionally, the description of this container is "The type of opaque value
element is the transit IPv4 source".  Should that be "IPv6 source"?  The same
applies for the augmented notification in the "opaque-element-transit"
container.  It reads IPv4, but should this be "IPv4 or IPv6"?
***
[skraza]:  There are cases where IP address is mandated to follow the parent AFI tree (v4 or v6), whereas there are cases where the address field is not constrained by its parent AFI and can be either ipv4 or ipv6. This did revisit and checked the fields accordingly and updated the fields that needed to be updated. Please x-chk the -rev12 diffs. Also fixed the typos as needed.
***