Reviewer: Jan Lindblad Review result: Almost Ready I had a read through of the draft RFC and looked at the YANG model in particular. Generally speaking, I think the YANG looks good. I don't know much about MPLS, however, so I can't judge the usefulness of the model. I'm bringing up a few points for discussion below. 1) IETF, OpenConfig and NMDA Early on in the draft, there is this section: For the configuration and state data, this model follows the similar approach described in [I-D.openconfig-netmod-opstate] to represent the configuration (intended state) and operational (applied and derived) state. This means that for every configuration (rw) item, there is an associated (ro) item under "state" container to represent the applied state. Furthermore, protocol derived state is also kept under "state" tree corresponding to the protocol area (discovery, peer etc.). [Ed note: This document will be (re-)aligned with [I-D.openconfig-netmod-opstate] once that specification is adopted as a WG document]. This alignment is sorely needed. I don't want to open up a new chapter in the IETF vs. OpenConfig style modeling debate, but I will simply note that the current model does not fit nicely into the surrounding MPLS, routing and interface models. Since consistency is a primary factor for ease of use, the model isn't very easy to use in its current form. 2) Lack of default/mandatory/description As has become a standing item in my reviews, there are a number of leafs that have no default, or mandatory statement, and no leaf name or description making it obvious to the user what would happen if this leaf is not set. This is particularly problematic with the "leaf enable {type boolean;} leafs. I get "enabled false" and "enabled true" even without any description. But what if enabled isn't specified? At the minimum level, the description should describe this case. Better if there was a default making this clear. Or if a default really isn't appropriate, so make it mandatory. Here are a list of leafs/line numbers where I think this is a problem. There are many others that have no default/mandatory/description, but where I suspect people who understand LDP might have an opinion about reasonable behavior if not set. ietf-mpls-ldp: 251 leaf hello-holdtime { 261 leaf hello-interval { 335 leaf enable { 346 leaf hello-holdtime { 357 leaf hello-interval { 380 leaf enable { 385 leaf reconnect-time { 395 leaf recovery-time { 406 leaf forwarding-holdtime { 424 leaf enable { 429 leaf reconnect-time { 439 leaf recovery-time { 462 leaf lsr-id { 533 leaf session-ka-holdtime { 544 leaf session-ka-interval { 777 leaf enable { 892 leaf enable { 1011 leaf enable { 1131 leaf enable { 1261 leaf lsr-id { 1331 leaf lsr-id { ietf-mpls-ldp-extended: 183 leaf transport-address { 193 leaf transport-address { 204 leaf key-chain { 218 leaf enable { 228 leaf enable { 238 leaf enable { 249 leaf igp-synchronization-delay { 300 leaf ldp-disable { 325 leaf helper-enable { 336 leaf transport-address { 354 leaf transport-address { 375 leaf igp-synchronization-delay { 473 leaf admin-down { 500 leaf enable { 505 leaf peer-list { 537 leaf enable { 620 leaf enable { 723 leaf enable { 728 leaf local-address { 781 leaf interface { 3) Odd naming convention for keys Many list keys are named by the same (or similar) name as the list. Repetitions of the same symbol makes it harder for users/programmers to get their code right, and even discuss the code. A common practice when there is no obvious identifier to use with the key is to use "name" or "id" for the key. list peer { key "peer advertisement-type"; The peer is keyed by "peer"? and advertisement-type. Looking at the "peer", it's a leafref to an lsr-id. So maybe that's a good name? list peer { key "lsr-id advertisement-type"; ietf-mpls-ldp: 296 list peer { key "peer advertisement-type"; --> "lsr-id advertisement-type"? 932 list address { key "address"; --> ipv4? 944 list fec-label { key "fec"; --> prefix? 979 list interface { key "interface"; --> name? ietf-mpls-ldp-extended: 279 list interface { key "interface"; --> name? 288 list address-family { key "afi"; --> name? 577 list address { key "address"; --> ipv6? 589 list fec-label { key "fec"; --> prefix? 4) Password handling The session auth password is commented out in the current model for some reason. Password handling is somewhat of a wart in an inconvenient place in YANG today, causing a whole array of interop issues of varying severity. One of the solutions working best with the current YANG spec (could perhaps be fixed in future YANG versions) is to not include passwords in the configuration, but only have an operational element with the encrypted value or last changed timestamp and and rpc/action to set the password. /* leaf session-authentication-md5-password { type string { length "1..80"; } description "Assigns an encrypted MD5 password to an LDP peer"; } // md5-password */ There may be other possibilities as well at modeling this in a friendlier way. 5) Neighbor/prefix/peer list references The ietf-mpls-ldp-extended module defined three reference types as strings. Are these free form strings, or is there any guidance that can be provided to users? Where would I go to find out what values are possible? I don't suppose any of these could be a leafref. typedef neighbor-list-ref { type string; description "A type for a reference to a neighbor list."; } typedef prefix-list-ref { type string; description "A type for a reference to a prefix list."; } typedef peer-list-ref { type string; description "A type for a reference to a peer list."; 6) Some short/pointless descriptions leaf prefix { type inet:ip-prefix; description "FEC."; } leaf lsr-id { type yang:dotted-quad; description "Router ID."; } Descriptions like these aren't helpful. Explain how to arrive at a good value, and what it means if the value is absent. I stumbled over a copy-paste description at line 235. Presumably hello-dropped should have a different description. leaf hello-received { type yang:counter64; description "The number of hello messages received."; } leaf hello-dropped { type yang:counter64; description "The number of hello messages received."; } 7) Captialized enum The YANG convention is that all enums, all symbols actually, are in all lower case. ietf-mpls-ldp: 915 enum Ordered { ietf-mpls-ldp-extended: 560 enum Ordered { That's it, thank you. /jan