Subject: [i2rs] Robert Wilton's No Objection on draft-ietf-i2rs-yang-l2-network-topology-18: (with COMMENT)

From: Robert Wilton via Datatracker <[email-masked]>
Date: Thu, 17 Sep 2020 09:50:30 -0700

Robert Wilton has entered the following ballot position for
draft-ietf-i2rs-yang-l2-network-topology-18: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-i2rs-yang-l2-network-topology/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Clearing discuss held for IEEE review:

The IEEE Yangsters (members of the IEEE YANG experts) who reviewed and
contributed review comments to this draft are comfortable with version -18. 
Note, that these represent informal reviews of individuals and do not represent
a formal IEEE consensus position."

Regards,
Rob

Mostly minor/editorial comments  (###) on the YANG model, but I do think that
it would be helpful for these to be discussed and addressed as appropriate:

4.  Layer 2 Topology YANG Module

     import ietf-inet-types {
       prefix inet;
       reference
         "Section 4 of RFC 6991";
     }
     import ietf-yang-types {
       prefix yang;
       reference
         "Section 3 of RFC 6991";
     }

###
These references should probably both just be: "RFC 6991: Common YANG Data
Types"

     revision 2020-06-29 {
       description
         "Initial revision";
       reference
         "RFC XXXX: A YANG Data Model for Layer 2 Network
                    Topologies";
     }

###
I would reorder these sections to be (which will solve the forward reference
issue mentioned by Ben):
   feature-stmt
   identity-stmt
   typedef-stmt

I'm surprised that pyang didn't flag this.

     /*
      * Typedefs
      */

     typedef vni {
       type uint32 {
         range "0..16777215";
       }
       description
         "VXLAN Network Identifier or VXLAN Segment ID.
          It allows up to 16 M VXLAN segments to coexist
          within the same administrative domain.

          The use of value '0' is implementation-specific.";
       reference
         "RFC 7348: Virtual eXtensible Local Area  Network (VXLAN):
                    A Framework for Overlaying Virtualized Layer 2
                    Networks over Layer 3 Networks";
     }

     typedef l2-flag-type {
       type identityref {
         base flag-identity;
       }
       description
         "Base type for L2 flags. One example of L2 flag
          type is trill which represents trill topology
          type.";
     }

###
This isn't really a base type.  Given where this flag is used, would this be
better as an "network-flag-type", with a description more similar to the ones
below?

     typedef node-flag-type {
       type identityref {
         base flag-identity;
       }
       description
         "Node flag attributes. The physical node can be
          one example of node flag attribute.";
     }

     typedef link-flag-type {
       type identityref {
         base flag-identity;
       }
       description
         "Link flag attributes. One example of link flag
          attribute is the pseudowire.";
     }

###
Should there be identities defined for l2-flag, node-flag and link-flag that
derive from flag-identity?  That would them make these three typedefs reference
different things rather than all referencing the same base flags.

     typedef l2-network-event-type {
       type enumeration {
         enum add {
           value 0;
           description
             "A Layer 2 node or link or termination-point
              has been added.";
         }
         enum remove {
           value 1;
           description
             "A Layer 2 node or link or termination-point
              has been removed.";
         }
         enum update {
           value 2;
           description
             "A Layer 2 node or link or termination-point
              has been updated.";
         }
       }
       description
         "Layer 2 network event type for notifications.";
     }

###
Since these are events, I would suggest renaming these
"add" -> "addition", "remove" -> "removal", "update"

     typedef neg-mode {
       type enumeration {
         enum full-duplex {
           description
             "Indicates full-duplex mode.";
         }
         enum auto-neg {
           description
             "Indicates auto-negotiation mode.";
         }
         enum half-duplex {
           description
             "Indicates half-duplex mode.";
         }
       }
       description
         "Indicates the type of the negotiation mode.";
     }
###
What negotiation do you mean?  Does this mean IEEE 802.3 auto-negotation?  If
so, it would be more correct to have a separate leaf for duplex vs
auto-negotation.

     /*
      * Features
      */

     feature VLAN {
       description
         "Indicates that the system supports the
          vlan functions (also known as an IEEE 802.1Q tag).";
     }

     feature QinQ {
       description
         "Indicates that the system supports the
          qinq functions (also known as IEEE 802.1ad double tag).";
     }
###
I think that the description should be more clear on what is supported here: 2
VLAN tags, as defined by IEEE 802.1ad.  The features should also include
reference statements.

Possibly we need to find a better name for these features (IEEE will possibly
comment on this).

     feature VXLAN {
       description
         "Indicates that the device supports VXLAN functions.";
       reference
         "RFC 7348: Virtual eXtensible Local Area  Network (VXLAN):
                    A Framework for Overlaying Virtualized Layer 2
                    Networks over Layer 3 Networks";
     }

     /*
      * Identities
      */

     identity flag-identity {
       description
         "Base type for flags.";
     }
###
Probably the name should be just "flag" rather than "flag-identity"

     identity eth-encapsulation-type {
       base ift:iana-interface-type;
       description
         "Base identity from which specific Ethernet
          encapsulation types are derived.";
       reference
         "RFC 7224: IANA Interface Type YANG Module";
     }

###
I would rename the base identity to probably "l2-encapsulation-type".  It also
probably doesn't help to derive from ift:iana-interface-type.

     identity ethernet {
     ...
     identity vxlan {
       base eth-encapsulation-type;
       description
         "VXLAN MAC in UDP encapsulation.";
     }

###
Please add references for protocols defined in the identities.

     /*
      * Groupings
      */

     grouping l2-network-type {

Dong, et al.            Expires December 31, 2020              [Page 12]
Internet-Draft     Layer 2 Network Topology Data Model         June 2020

       description
         "Indicates the topology type to be L2.";
       container l2-network {
         presence "Indicates L2 Network";
         description
           "The presence of the container node indicates
            L2 Topology.";
       }
     }

     grouping l2-network-attributes {
       description
         "L2 Topology scope attributes.";
       container l2-network-attributes {
         description
           "Contains L2 network attributes.";
         leaf name {
           type string;
           description
             "Name of the L2 network.";
###
Perhaps just "Name of the network".  Are there any restrictions here on how
long the name can be, can it contain spaces, etc?

         }
         leaf-list flag {
           type l2-flag-type;
           description
             "L2 network flags.";
         }
       }
     }

###
Perhaps expand the description to "Flags that can be associated with the
network"

I would suggest changing the name of all "flag" to "flags" where they are
leaf-lists.

     grouping l2-node-attributes {
       description
         "L2 node attributes";
       container l2-node-attributes {
         description
           "Contains L2 node attributes.";
         leaf name {
           type string;
           description
             "Node name.";
         }
         leaf description {
           type string;
           description
             "Node description.";
         }
         leaf-list management-address {
           type inet:ip-address;
           description
             "System management address.";
         }
         leaf sys-mac-address {
           type yang:mac-address;
           description
             "System MAC address.";
         }
         leaf management-vid {
           if-feature "VLAN";
           type dot1q-types:vlanid;
           description
             "System management VID.";

###
Please change "management-vid" to "management-vlan-id", and fix the description.

Probably could expand on all the descriptions of the "System" properties to
explain what these system properties are and how they are used (e.g. to manage
the device).

         }
         leaf-list flag {
           type node-flag-type;
           description
             "Node operational flags.";
         }
       }
     }

     grouping l2-link-attributes {
       description
         "L2 link attributes";
       container l2-link-attributes {
         description
           "Contains L2 link attributes.";
         leaf name {
           type string;
           description
             "Link name.";
         }
         leaf-list flag {
           type link-flag-type;
           description
             "Link flags.";
         }
         leaf rate {
           type decimal64 {
             fraction-digits 2;
           }
           units "Mbps";
           description
             "Link rate.";

###
Please expand on this description.  Does this describe how the link is
operating at the physical layer, or its bandwidth?

         }
         leaf delay {
           type uint32;
           units "microseconds";

           description
             "Link delay in microseconds.";

###
Is this uni-directional delay, or bi-directional?  Please clarify.

         leaf maximum-frame-size {
           type uint32;
           description
             "Maximum L2 frame size. If L2 frame is an Ethernet
              frame, the Ethernet header should be included;
              if L2 frame is other type (e.g., PPP), the L2
              header should be included.";

###
This needs to clarify whether the 4 byte CRC is included in the frame size.

             leaf eth-encapsulation {
               type identityref {
                 base eth-encapsulation-type;
               }
###
As per comments on the identities, I think that this should be "encapsulation".

               description
                 "Encapsulation type of this
                  termination point.";
             }
             leaf lag {
               type boolean;
               default "false";
               description
                 "Defines whether lag is supported or not.";
###
Does this leaf indicate whether LAG is supported, or enabled on the link?

             }
             leaf-list member-link-tp {
               when "../lag = 'true'" {
                 description
                   "Relevant only when the lag interface is supported.";
               }
               type leafref {
                 path "/nw:networks/nw:network/nw:node/"
                    + "nt:termination-point/nt:tp-id";
               }
               description
                 "Member link termination points.";
             }
             leaf mode {
               type neg-mode;
               default "auto-neg";
               description
                 "Exposes the negotiation mode.";
             }
###
Suggest changing the name to "ethernet-auto-neg-mode", and refining the
description.

I would also suggest splitting duplex and auto-negotiation into 2 separate
leaves to more accurately represent

             leaf port-vlan-id {
               when "derived-from-or-self(../eth-encapsulation"
                  + ", 'l2t:vlan')" {
                 description
                   "Only applies when the type of the Ethernet
                    encapsulation is 'vlan'.";
               }
               if-feature "VLAN";
               type dot1q-types:vlanid;
               description
                 "Port VLAN ID is the VLAN identifier that
                  will be assigned to any untagged frames entering
                  the switch on the specific port.";
             }
###
Probably naming this as the "default-untagged-vlan" may be more helpful.

             list vlan-id-name {
               when "derived-from-or-self(../eth-encapsulation"
                  + ", 'l2t:vlan')" {
                 description
                   "Only applies when the type of the Ethernet
                    encapsulation is 'vlan'.";
               }
###
I think that I would name this list "vlans"

               leaf vlan-name {
###
I would suggest renaming this leaf to just "name"

                 "Interface configured SVLANs and CVLANs.";
               leaf svlan-id {
                 type dot1q-types:vlanid;
                 description
                   "SVLAN ID.";
               }
               leaf cvlan-id {
                 type dot1q-types:vlanid;
                 description
                   "CVLAN ID.";
               }
             }

###
Suggest "SVLAN" => "S-VLAN" and "CVLAN" => "C-VLAN"

             container vxlan {
               when "derived-from-or-self(../eth-encapsulation"
                  + ", 'l2t:vxlan')" {
                 description
                   "Only applies when the type of the Ethernet
                    encapsulation is 'vxlan'.";
               }
               if-feature "VXLAN";
               leaf vni-id {
                 type vni;
                 description
                   "VXLAN Network Identifier (VNI).";
               }
               description
                 "Vxlan encapsulation type.";
             }
           }

           //case ethernet
           case legacy {
             leaf layer-2-address {
               type yang:phys-address;
               description
                 "Interface Layer 2 address.";
             }
             leaf encapsulation {
               type identityref {
                 base ift:iana-interface-type;
               }
               description
                 "Other legacy encapsulation type of this
                  termination point.";
             }
           }
           //case legacy such as atm, ppp, hdlc, etc.
         }
         //choice termination-point-type
         leaf tp-state {
           type enumeration {
             enum in-use {
               value 1;
               description
                 "The termination point is in forwarding state.";
             }
             enum blocking {
               value 2;
               description
                 "The termination point is in blocking state.";
             }
             enum down {
               value 3;
               description
                 "The termination point is in down state.";
             }
             enum others {

###
Please rename others => other
               value 4;
               description
                 "The termination point is in other state.";
###
Please modify the description to something like "The termination point is in
another state, not listed in the enumeration."

             }
           }
           config false;
           description
             "State of the termination point.";
         }
       }
     }