Reviewer: Jan Lindblad Review result: On the right track I have reviewed draft-ietf-trill-yang-oam-05, and in particular the YANG model within it. Due to their coupling, I also got in contact with draft-ietf-lime-yang-oam-model-10 and the YANG model defined there. The draft document and YANG module are easily read (nit: except when it comes to the text layout; the YANG text is a bit garbled here and there). I note that there are no configuration/usage examples given, which I think makes correct application of this document harder. #1) Module does not compile The augment path is missing a name in two places, which prevents it from being properly compiled. yang/ietf-trill-oam.yang:279: error: cannot augment a case into the output node 'output' yang/ietf-trill-oam.yang:312: error: cannot augment a case into the list node 'response' These errors are easily fixed: augment "/goam:continuity-verification/goam:output" { Should be: augment "/goam:continuity-verification/goam:output/goam:monitor-stats" { And: augment "/goam:traceroute/goam:output/goam:response" { Should be: augment "/goam:traceroute/goam:output/goam:response/goam:monitor-stats" { #2) Case missing leafs I suspect that one of the augments of a choice is meant to contribute one new case with one uses and two leafs. As written now, it's actually contributing three different cases. One with the uses, then two more cases which consist of a single leaf each (that appear to make little sense in isolation). In case I'm right, this: case monitor-stats-resp { uses monitor-stats-trill; } leaf upstream-rbridge { type tril-rb-nickname; description "Trill Rbridge nickname."; } leaf-list next-hop-rbridge { type tril-rb-nickname; description "nickname of the next hop RBRdige"; } Should be: case monitor-stats-resp { uses monitor-stats-trill; leaf upstream-rbridge { type tril-rb-nickname; description "Trill Rbridge nickname."; } leaf-list next-hop-rbridge { type tril-rb-nickname; description "nickname of the next hop RBRdige"; } } // End case here instead #3) Bad when expression The when expression ensuring that mep-address is only configurable on trill technology domains is likely not doing what the modeler had intended. The original form expression looks like this: augment "/goam:domains/goam:domain/goam:MAs/goam:MA/goam:MEP/goam:mep-address" { case mep-address-trill { leaf mep-address-trill { when "/goam:domains/goam:domain/goam:technology='trill'" { description "Technology TRILL"; This allows setting a mep-address-trill on ANY domain regardless of technology in that domain as soon as there exists at least one domain with technology trill. Probably not what was intended. If instead the mep-address-trill should appear on the domains where technology trill have been configured, the when expression needs to be: when "../../../../goam:technology='trilloam:trill'" { In YANG 1.0, the XPATH string representation of identityrefs was not clearly defined, which has led to some interoperability issues in the field. In YANG 1.1 this has been clarified so that the identityref should always include the namespace prefix (per above). In order to make identityref even more interoperable and even allow "subclassing" (which was one of the original intents with the YANG identity concept), the YANG modeling recommendation is to test identityrefs not using equality, but using the new YANG 1.1 XPATH function derived-from-or-self. Like this: module ietf-trill-oam { yang-version 1.1; ... when "derived-from-or-self(../../../../goam:technology, 'trilloam:trill')" { This is what I recommend. #4) No defaults There is a single "default" statement in the entire module, and since all leafs are being augmented into a main module they are precluded from having mandatory true. There are little in terms of explanations what it would mean when an optional leaf is not present in the configuration. In some cases the reader might guess what it means, but in many cases at least I am unsure. E.g. what should the system do if a MEP has no mep-address-trill? I would suggest going through every leaf in the module and add to the description what it means if this leaf is not set. Alternatively, add a default statement which explains the value the system would use if the operator doesn't say. #5) vlan type The module defines a VLAN type like this: typedef vlan { type uint16 { range "0..4095"; Other models defining VLAN types usually have a range of 1..4094. Is the range here the intended one? #6) flow-entropy-trill type In section 4.2 Flow Entropy the document says: In TRILL, flow-entropy is defined as a 96 octet field. [GENYANGOAM] In the YANG the following typedef is given: typedef flow-entropy-trill { type binary { length "1..96"; To me, the two definitions seem not to match up perfectly. The text says 96 bytes. The YANG says up to 96 bytes. Which one is right? #7) ietf-conn-oam This is not a review of the ietf-conn-oam YANG model, but since the current module builds on that one, I could not help but notice that there are some issues in the ietf-conn-oam module that I would recommend looking into before letting either model reach RFC status. Things I noticed while scanning quickly: - When expressions with unqualified equality comparisions to identities are not likely to be interoperable - The module has many leaf and container names with capitals. This goes against the IETF YANG modeling style guide. Fixing this would impact all augmenting models, so sooner is better than later. Best Regards, /jan