Early Review of draft-ietf-mpls-mldp-yang-11
review-ietf-mpls-mldp-yang-11-yangdoctors-early-clarke-2024-07-29-00

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

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.

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.

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.";
...
}

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"?