Early Review of draft-ietf-trill-yang-oam-05
review-ietf-trill-yang-oam-05-yangdoctors-early-lindblad-2017-06-16-00

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