Last Call Review of draft-ietf-isis-yang-isis-cfg-24
review-ietf-isis-yang-isis-cfg-24-yangdoctors-lc-aries-2018-10-22-00

1 module in this draft:
- ietf-isis@2018-08-09.yang

No YANG compiler errors or warnings (from pyang 1.7.5 and yanglint 0.16.54)

"ietf-isis@2018-08-09" module is compatible with the NMDA architecture.

Module ietf-isis@2018-08-09.yang:
- Both the description and the draft name reference that this module is
  specific to configuration but contains operational state nodes in addition
  to RPCs and notifications.  Any wording suggesting this is only
  configuration should be changed
- Module description must contain most recent copyright notice per
  https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.1
- Module description reads "common across all of the vendor implementations".
  I don't think this needs to be called out as such as that is the overall
  intention of *all* IETF models
- This module contains '22' features (and the respective OSPF module currently
  contains '26').  While it is understood the purpose of these features in the
  module, take precaution as to complexity for clients if they need to
  understand >= quantity of features per module in use on a
  network-element.  We are going to end up w/ feature explosion to convey
  *all* possible features of each network-element leading to divergence back
  towards native models at the end of the day.  A large amount of these
  feature names could be defined within a more global namespace (e.g. nsr) but
  this gives us a granular yet cumbersome approach (e.g. feature isis:nsr,
  ospf:nsr, etc..)
- RPC 'clear-adjacency' does not have any input leaf that covers clearing a
  specific neighbor/adjacency (See comments below as well regarding RPC
  alignment w/ the OSPF model)
- RPC 'clear-adjacency' has an input node of 'interface' however this is just
  a string type.  Is there any reason this is not a leafref/if:interface-ref
  (much like in the OSPF model)
- Child nodes within a container or list SHOULD NOT replicate the parent
  identifier per
  https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-4.3.1.

  A case in point is the list /afs/af that has a leaf of 'af'
      <afs>
        <af>
          <af>ipv4</af>
          <enable>true</enable>
        </af>
      </afs>

  Not only is this replication, but we should likely not abbreviate 'afs' if
  we are using the expanded 'address-family' in other IETF models such as
  ietf-i2rs-rib


General comments on the draft + nits:
- Since YANG tree diagrams are used, please include an informative reference
  per https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.4
- Section 1.1 does not need to exist since this would be covered by the
  reference mentioned above
- Reference to NMDA compliance should be contained within Section 1 (vs.
  Section 2) per
  https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.5
- Section 2: It seems reference should be given to the location of where the
  ietf-routing module is defined (As well as reference to NMDA RFC in the
  above reference)
- Section 2.1: "Additional modules may be created this to support..." needs
  slight rewording adjustment
- Section 3: The RPC operations are named 'clear-adjacency' and
  'clear-database' rather w/ reliance off namespacing for uniqueness.  This
  section refers to 'clear-isis-database' and 'clear-isis-adjacency'
- Section 4: Notification name mismatch in this section from actual naming
  within the module (e.g. 'adjacency-change' should rather be
  'adjacency-state-change')
- Section 7: Security Considerations will need updating to be patterned after
  the latest version of the template at
  https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines per
  https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.7
- Section 12: All modules imported within this module MUST be referenced
  within this section per
  https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.9.
  There are quite a few missing from this section right now
- Appendix A: Some of the XML elements are off in alignment
- Appendix A: Examples must be validated.  The example given has the following
  issues:
  - /routing[name='SLI'] and /routing/description are invalid data nodes and
    do not exist.  I'm not sure why they are in the XML example here
  - The example is meant to reference configuration however
    /routing/interfaces is a r/o container
  - The control-plane-protocol 'type' needs to be qualified - e.g.
    <type xmlns:isis="urn:ietf:params:xml:ns:yang:ietf-isis">isis:isis</type>
  - The area-address does not validate against the pattern regex and must end
    with a '.'  e.g.
    <area-address>49.0001.0000.</area-address>
  - metric-type/value is set to 'wide' which is invalid.  This should rather
    be 'wide-only'
  - isis/afs/af/af is set to 'ipv4-unicast' which is invalid.  This should
    rather be 'ipv4' per iana-routing-types
  - /interfaces/interface/type must be populated and is invalid.  This should
    rather be qualified as such:
    <type xmlns:ianaift="urn:ietf:params:xml:ns:yang:iana-if-type">ianaift:softwareLoopback</type>
    <type xmlns:ianaift="urn:ietf:params:xml:ns:yang:iana-if-type">ianaift:ethernetCsmacd</type>
  - /interfaces/interface/link-up-down-trap-enable must have a value
    associated as such:
    <link-up-down-trap-enable>enabled</link-up-down-trap-enable>
  - NP container 'priority' has a must statement checking if an interface-type
    is set to 'broadcast' however if you take the XML example from this
    section, it will fail to validate even if <priority> is not defined
    underneath an interface-type of 'point-to-point'.  It seems to me that
    this logic may need to be readjusted or not exist at all (priority can
    still be set on implementations on loopback interfaces - which would
    default to 'broadcast' in the example here).  Could you not solve this
    with use of 'when' vs. 'must' as such:

          when '../interface-type = "broadcast"' {
              description "Priority can only be set for broadcast interfaces.";
          }

    https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-4.18.2.

  - /interfaces/interface/ipv4/mtu must contain a valid value (and likely not
    need to be defined for Loopback0)
  - 'isis/mpls-te/ipv4-router-id' is invalid and should rather be
    'isis/mpls/te-rid/ipv4-router-id'
  - 'isis/afs/af/enabled' is invalid and should rather be 'isis/afs/af/enable'
  - Examples should use IPv6 addresses where appropriate per
    https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.12.
- Looking at the latest revision of ietf-ospf (draft-ietf-ospf-yang-17) and
  some comments performed earlier this year, I would suggest that for where
  there are similarities that descriptions should be aligned both within the
  same module and across modules

  Examples:

  # ietf-isis
  container node-tags {
    if-feature node-tag;
    list node-tag {
      leaf tag {
        type uint32;
        description "Node tag value.";
      }
      description "List of tags.";
    }
    description "Node Tag container";
  }
  ...
  container node-tags {
    if-feature node-tag;
    list node-tag {
      key tag;
      leaf tag {
        type uint32;
        description
          "Node tag value.";
      }
      description
        "List of tags.";
    }
    description
      "Container for node tags.";
  }

  # ietf-ospf
  container node-tags {
    if-feature node-tag;
    list node-tag {
      key tag;
      leaf tag {
        type uint32;
          description
            "Node tag value.";
      }
      description
        "List of tags.";
    }
    description
      "Container for node admin tags.";
  }

  * Note: This is seen for other like leafs as well (e.g. short-delay,
    long-delay, etc..)
- Suggest alignment of attributes across RPCs from ietf-isis and ietf-ospf
  (e.g. ietf-ospf currently defines generic actions with a
  'routing-protocol-name' as an argument vs. protocol specific with
  'routing-protocol-instance-name')