I have only very limited comprehension of YANG from more than 10 years ago. Therefore, my review does not include "9. YANG Specification". Security considerations are always a headache to me as well, and I skipped "10. Security Considerations". Many of the following questions/comments are because of my fuzzy YANG understanding - perhaps some clarfication in the email or in the document will clear them. [Ed Note: Some of the topics in the above list are to be addressed/ extended in a later revision of this document]. Shouldn't the above be removed? For manually provisioned Multipoint LSPs, we represent its associated FEC as (root-address, opaque-element) tuple. The following table lists various type of opaque elements with their keys, as later used in the configuration and state model: Why "manually provisioned" only? Shouldn't it apply to both provisioned ones (on leaves) and signaled ones (on roots/transit nodes)? In fact, the last sentence does say "in the ... state model". Perhaps "manually provisioned"? augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol: +--rw mpls-ldp +--rw global | +--rw capability | | +--rw mldp:mldp | | +--rw mldp:p2mp | | | +--rw mldp:enabled? boolean | | +--rw mldp:mp2mp | | | +--rw mldp:enabled? boolean Shouldn't all the "enabled" be "supported"? A router may "support" (or not) a capability, and a supported capability nay be "enabled" (or not) by provisioning as in the following (notice the "mldp:enabled"): | +--rw mldp:mldp | +--rw mldp:enabled? boolean | +--rw mldp:address-families | +--rw mldp:ipv4! | | +--rw mldp:enabled? boolean | | +--ro mldp:roots | | | +--ro mldp:root* [root-address] For the following parameters in the "capability" portion, | | | +--rw mldp:switchover-delay? uint16 | | | +--rw mldp:timeout? uint16 ... | | +--rw mldp-ext:targeted-session-teardown-delay? uint16 Shouldn't they be the under the following? | +--rw mldp:mldp | +--rw mldp:enabled? boolean If the "capability" is really about whether a feature is enabled on this node or not, then I don't think we should call them out as "capabitlity" - they'd just be like if mldp is enabled/configured or not. I noticed that the global/peers part uses "capable" - should we make it consistent? +--ro mldp:peer* [direction peer advertisement-type] | +--ro mldp:peer -> ../../../../../../../../../../../ldp:peers/peer/lsr-id | +--ro mldp:direction ldp:downstream-upstream | +--ro mldp:advertisement-type ldp:advertised-received | +--ro mldp:label? rt-types:mpls-label | +--ro mldp:mbb-status? lsp-mbb-role | +--ro mldp-ext:mofrr-status? lsp-mofrr-role I noticed the a similar mldp-ext:peer (compared to mldp:peer) +--ro mldp-ext:peer* [direction peer advertisement-type] +--ro mldp-ext:peer -> ../../../../../../../../../../../../ldp:peers/peer/lsr-id +--ro mldp-ext:direction ldp:downstream-upstream +--ro mldp-ext:advertisement-type ldp:advertised-received +--ro mldp-ext:label? rt-types:mpls-label +--ro mldp-ext:mbb-status? lsp-mbb-role +--ro mldp-ext:mofrr-status? lsp-mofrr-role Can they not share a same defintition (is that called "grouping")? The key is [direction peer advertisement-type], but the peer is listed before direction. Does that matter? +--ro mldp:bindings +--ro mldp:opaque-element-lspid | +--ro mldp-ext:recursive-fec* [recur-root-addr | +--ro mldp-ext:recur-root-address inet:ip-address | +--ro mldp-ext:recur-rd rt-types:route-distinguisher | +--ro mldp-ext:multipoint-type? mldp:multipoint-type +--ro mldp-ext:opaque-element-transit +--ro mldp-ext:opaque-element-bidir Why is mldp-ext:recursive-fec nested under mldp:opaque-element-lspid instead of parallel to all bindings? and where is the recursive part of the fec (besides the recur-root-address/rd)? +--rw mldp:configured-leaf-lsps | +--rw mldp:opaque-element-lspid | | +--rw mldp:fec-label* [root-address lsp-id] | | +--rw mldp:root-address inet:ipv4-address | | +--rw mldp:lsp-id uint32 | | +--rw mldp:multipoint-type? multipoint-type | | +--rw mldp-ext:recursive-fec* [recur-root-address recur-rd] | | +--rw mldp-ext:recur-root-address inet:ip-address | | +--rw mldp-ext:recur-rd rt-types:route-distinguisher | | +--rw mldp-ext:multipoint-type? mldp:multipoint-type Same question for the configured leaf lsp, and notifications. Looking at the other fec types, I wonder if the sub-fields of mldp-ext:recursive-fec should be pulled out, and the extra multipoint-type be removed. Or, use the mldp-ext:recursive-fec for all (though the multipoint-type seems to be extra). I noticed that the configured leaf lsps and the notifications are very similar. Could they share the same grouping (or whatever is the proper yang terminology)? * mLDP enablement: To enable mLDP under a (VRF) routing instance, mldp is enabled in the mldp container under LDP. It is to remind that the LDP modules reside under a network-instance and the scope of any configuration defined under this tree is network-instance (VRF) scoped. Given that mLDP requires LDP signaling, it is not sensible to allow disabling the LDP control plane under a (VRF) network-instance while requiring mLDP to be enabled for the same. However, if a user wants to only allow signaling for multipoint FECs on an LDP/mLDP enabled VRF instance, he/she can use LDP label-policies to disable unicast FECs under the VRF. Alternatively, the same can be achieved by advertising the capabilities and the procedures defined in [RFC7473]. For the two alternatives (policy or RFC7473), how is it refected in the YANG configuration/state? In most common deployments, it is desirable to disable mLDP (capabilities announcements) on a targeted-only LDP peering, where targeted-only peer is the one whose discovery sources are the targeted type only. Why is that? It there a reference?