Re: [i2rs] Robert Wilton's Discuss on draft-ietf-i2rs-yang-l2-network-topology-14: (with DISCUSS and COMMENT)

[Qin Wu <[e-mail]>]

Hi, Rob:
I have incorporated your comments in v-15 and split auto-nego into two leafs and address all the comments you highlighted.
Also becos we need to support both Ethernet and legacy technology, therefore we think iana-interface-type in RFC7224 is a good basis for us to derive from it,
This change was based on comments raised by WGLC.
https://tools.ietf.org/html/draft-ietf-i2rs-yang-l2-network-topology-15

-Qin
-----邮件原件-----
发件人: Robert Wilton via Datatracker [mailto:[e-mail]] 
发送时间: 2020年7月9日 20:13
收件人: The IESG <[e-mail]>
抄送: [e-mail]; [e-mail]; [e-mail]
主题: Robert Wilton's Discuss on draft-ietf-i2rs-yang-l2-network-topology-14: (with DISCUSS and COMMENT)

Robert Wilton has entered the following ballot position for
draft-ietf-i2rs-yang-l2-network-topology-14: Discuss

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/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

I support Magnus's discuss regarding IEEE 802.1Q WG review.

I also feel that the YANG model could benefit from another editorial pass:
 - In many places the descriptions are very terse, and references are missing.
 - The way that auto-neg is defined doesn't really match the 802.3  specification, probably splitting it into two separate leaves (one for whether  auto-neg is on/off, and separate one for the duplex setting would be better).
 - The use of terminology for VLAN vs QinQ might not be acceptable to IEEE. 
 Finding names that are more closely aligned with the terms in 802.1Q may be  helpful (although if I understand it correctly, 802.1Q bridges don't directly  expose double VLAN tags).  Possibly some of the terminology/description from  draft-ietf-netmod-sub-intf-vlan-model (which has been reviewed by IEEE 802.1Q
 WG) may be helpful here.


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

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.";
         }
       }
     }