[L2sm] FW: YANG + XML review of draft-ietf-l2sm-l2vpn-service-model-05

["Adrian Farrel" <[e-mail]>]

Hi authors,
 
Jan Lindblad has done a great review for us.
 
On the positive side he thinks you have done a really good job with the module,
but he has nevertheless found a pile of small issues.
 
It looks to me (what do I know ;-) that the majority of these should be really
easy to fix, so I'd like to lean on you a bit to get a revision "soon." Can you
let us know what the plan is?
 
At the same time, I will do an English language review.
 
Thanks,
Adrian
 
From: Jan Lindblad [mailto:[e-mail]] 
Sent: 31 January 2018 13:48
To: Adrian Farrel
Cc: [e-mail]; Qin Wu
Subject: YANG + XML review of draft-ietf-l2sm-l2vpn-service-model-05
 
Adrian, shepherds,
 
I've now made a YANG review of the l2vpn YANG model and examples. Since I'm not
listed as the formal reviewer, I'll just write it in a mail like this. Let me
know how you'd like me to post this if this mail to you is not what you had in
mind.
 
That's fabulous!
 
We are looking to do WG last call in early February. Of course, incorporating
your comments before then would be best, but that would mean completing your
review by the end of January.
 
The back-up plan would be to do your review before the end of last call so
targeting 20th Feb.
 
So I read the first 35 pages of the RFC before I realized I'm not going to
understand this well enough to have any use of the plaintext when looking at the
YANG anyway. So then I jumped right at the YANG and examples instead. I must say
it's a clean and simple approach and one of the more well written YANGs I have
seen; I could not find any serious issues with it. Since it's written in a very
down to earth fashion without much in terms of fancy constructs, it stays clear
of some of the pitfalls we've seen in the past. When it comes to the examples,
they all omit things (which may or may not be acceptable, you decide) and almost
all the examples are broken. In other words, business as usual.
 
On the detailed level I have a long list of comments. Mostly the usual stuff. So
here goes:
 
General:
-- indentation is broken
 
General:
  feature dot1q{
-- missing space between name and open brace in lots of places
 
General:
  identity opaque {
    base bw-type;
    description
      "Opaque";
-- descriptions like this have negative value, no information but take up space.
I would have hoped to understand how/when to use "opaque", etc.
 
46:  feature L2CP-control { 
443:  identity all2one-Bundling {
-- should be all lowercase
 
80:  feature signaling-options {
    description
      "Enable the support of signalling option.";
3001:      leaf signalling-type {
-- make spelling consistent; either "signaling" or "signalling". Our own XML
examples below stumble over this as well
 
365:  identity site-vpn-flavor-single {
371:  identity site-vpn-flavor-multi {
381:  identity site-vpn-flavor-nni {
601:  identity co-managed {
606:  identity customer-managed {
611:  identity provider-managed {
616:  identity address-family {
-- remove the word "Base" in the description, these are identities, but not base
identities
 
454:  identity color-id--cvlan { 
468:  identity cos-id--dscp {
-- use single dash
 
906:  identity bidirection {
-- should be "bidirectional" ?
 
1024:        choice list-flavor {
1064:    leaf site-vpn-flavor {
1252:        leaf delivery-mode {
1452:    leaf bundling-type {
1459:    leaf default-ce-vlan-id {
1469:   grouping cfm-802-grouping { (one leaf has a default)
1538:  grouping y-1731 {
1650:   grouping lacp-grouping {
1672:       leaf system-priority {
1830:        leaf enable {
1838:      leaf uni-loop-prevention {
1888:        leaf tag-type {
1895:        leaf cvlan-id {
1909:         leaf tag-type {
1925:        leaf tag-type {
1932:        leaf svlan-id {
1937:        leaf cvlan-id {
1951:        leaf tag-type {
1958:        leaf svlan-id {
1972:        leaf vni-id {
1977:        leaf peer-mode {
2005:      leaf encapsulation-type {
2012:      leaf eth-inf-type {
2049:  grouping svc-preservation-grouping {
2073: grouping site-mac-addr-limit {
2105:      leaf access-priority {
2139:  grouping l2cp-grouping {
2199:  grouping site-bum {
2275:  grouping site-mac-loop-prevention {
2301:  grouping ethernet-svc-oam-grouping {
2327:  grouping fate-sharing-group {
2452:      choice request-type-choice {
2542:      leaf attachment-device-id {
2577:          leaf vpn-id {
2648:        leaf vpn-id {
2653:        leaf cir {
2661:        leaf cbs {
2855:                leaf direction {
2863:                leaf policing {
2879:                  choice flavor {
2907:                  choice flavor {
3001:      leaf signalling-type {
3135:        leaf svc-type {
3147:        leaf svc-topo {
3203:            leaf remote-carrier-name {
-- no default, no mandatory, no description what happens if not set. Groupings
included above when every leaf inside is lacking default/mandatory/description
 
1171:             list filter {
-- only one filter of each type (and only one standard type). Will there ever be
a filter that you'd want to have several of?
 
1211:               mandatory true;
-- redundant, this is already a key, hence mandatory
 
1349:          leaf group-id {
1718:            leaf profile-name {
1779:      leaf flow-control {
1818:      leaf flow-control {
2334:      leaf group-color {
2341:        leaf group-id {
2362:        leaf group-id {
2399:                  leaf group-id {
2455:            leaf physical-if {
2472:          leaf physical-if {
2477:          leaf circuit-id {
2499:        leaf requested-type {
2542:      leaf attachment-device-id {
3203:            leaf remote-carrier-name {
-- These use type string, is that really the right type? How about leafref,
enumeration, ...? What are the valid values here? What happens if an "incorrect"
value is given?
 
1563:       leaf message-period {
1569:      leaf measurement-interval {
1662:      leaf lacp-speed {
1667:      leaf mini-link {
1695:        leaf bfd-interval {
1700:        leaf bfd-hold-timer {
1748:          leaf port-speed {
1803:      leaf port-speed {
2245:      leaf bum-overall-rate {
2259:        leaf rate {
2277:      leaf frequency {
2935:                  leaf fr-loss-rate {
-- add units. In some cases unit is specified in the description, but better in
units stmt.
 
1107:          leaf management-transport {
1650:   grouping lacp-grouping {
1652:      leaf lacp-state {
1657:      leaf lacp-mode {
1662:      leaf lacp-speed {
1681:        leaf micro-bfd-on-off {
1695:        leaf bfd-interval {
1700:        leaf bfd-hold-timer {
1710:        leaf bfd-hold-timer {
1857:        leaf lag-ifindex {
1875:      leaf tagged-inf-type {
2075:      leaf mac-num-limit {
2776:      container qos-classification-policy {
2824:      container qos-profile {
2935:                  leaf fr-loss-rate {
3060:    leaf site-network-access-type {
-- remove redundant part of the name. Inside container bfd, leaf:s should not be
called bfd-foo, but just foo
 
1652:      leaf lacp-state {
-- wouldn't "enable" be a better name than "state" for an on/off switch?
 
1657:      leaf lacp-mode {
-- what does values true and false mean here? How is this different from leaf
lacp-state?
 
1740:      container member-link-list {
-- should probably be "member-links"
 
1798:      leaf ifindex {
-- how do I know what ifindex value to use? Is this the same ifIndex as in snmp?
 
1853:    container lag-interface {
-- should probably be lag-interfaces
 
2112:          leaf single-active {
2121:          leaf all-active {
-- I think you mean type empty?
 
2308:      leaf md-level {
2792:               leaf match-phy-port {
-- how do I know what is a good value for this leaf?
 
2460:            leaf vlan-id {
-- add a range?
 
2499:        leaf requested-type {
-- same name as parent container, confusing
 
Finally, there are seven leafrefs with absolute paths. In the past reviews there
have been issues when an absolute path was used but only a relative path had the
right semantics. In this case, I don't understand well enough what the author's
intent is, so let me verify for each one that the YANG matches the author's
intent:
 
1015:         leaf cloud-identifier {
-- points to ANY cloud, regardless of profile
 
1033:            leaf-list permit-site {
1042:            leaf-list deny-site {
-- points to ANY site, regardless of which customer
 
1206:            leaf vpn-id {
1273:      leaf svc-id {
2577:          leaf vpn-id {
-- points to ANY vpn, regardless of which customer
 
2832:            leaf profile {
-- points to ANY qos-profile
 
 
 
Then we have the XML examples. I refer to them by page number in the spec, with
"a" and "b" suffix when there is more than one on a single page.
 
p24a, p24b, p25:
The
/l2vpn-svc/vpn-services/vpn-service["123456487"]/frame-delivery/multicast-gp-por
t-mapping is mandatory and not set by the example. After working a bit with the
model, I'd suggest removing mandatory here and add a default instead. It's
really a bit annoying to have to care about multicast properties as soon as I
define a vpn-service.
 
p26:
Example broken in a couple of places:
 
1.
          <vpn-service-topology>hub-spoke</vpn-service-topology>
==>
          <svc-topo>hub-spoke</svc-topo>
 
2.
          <vpn-service-topology>any-to-any</vpn-service-topology>
==>
          <svc-topo>any-to-any</svc-topo>
 
3.
                  <site-role>spoke-role</site-role>
==>
                  <local-sites-role>spoke-role</local-sites-role>
 
p39:
Example broken in a couple of places:
 
1.
         <site-network-access-id>LA1</site-network-access-id>
==>
         <network-access-id>LA1</network-access-id>
2.
         <site-network-access-id>LA2</site-network-access-id>
==>
         <network-access-id>LA2</network-access-id>

And a few things are omitted:

/l2vpn-svc/sites/site["SITE1"]/service/svc-mtu is mandatory and not included in
the example. I'd suggest adding a default instead, the MTU is perhaps not the
first thing you think about when defining a new service. Now it has to be given
immediately.
 
/l2vpn-svc/sites/site["SITE1"]/management/type is mandatory and not included in
the example. As soon as a type is set, a number of other things become mandatory
too. Which ones depends on what type you select.
 
/l2vpn-svc/sites/site["SITE1"]/site-network-accesses/site-network-access["LA1"]/
vpn-attachment/vpn-id refers to a VPN that is not defined, but I guess that's
acceptable.
 
p41:
Example broken:
 
         <site-network-access-id>LA1</site-network-access-id>
==>
         <network-access-id>LA1</network-access-id>
 
Same things omitted as on p39.
 
p42:
Example broken in a couple of places:
 
1.
               <filter>
                 <lan-tag>LAN1</lan-tag>
==>
               <filter>
                 <type>lan</type>
                 <lan-tag>17</lan-tag>
 
2.
               <filter>
                 <lan-tag>LAN2</lan-tag>
==>
               <filter>
                 <type>lan</type>
                 <lan-tag>18</lan-tag>
 
3.
            <site-network-access-id>LA1</site-network-access-id>
==>
            <network-access-id>LA1</network-access-id>
Omits service/svc-mtu and management/type like on p39.
 
p60:
Example broken in a number of places:
 
1.
            <location>
              <city>NY</city>
              <country-code>US</country-code>
           </location>
==>
          <locations>
            <location>
              <location-id>NY1</location-id>
              <city>NY</city>
              <country-code>US</country-code>
           </location>
         </locations>
2.
               <site-network-access-id>CSP_A_VN1</site-network-access-id>
==>
               <network-access-id>CSP_A_VN1</network-access-id-->
3.
                       <eth-inf-type>tagged<eth-inf-type>
==>
                       <eth-inf-type>tagged</eth-inf-type>
4.
                         <tagged-inf-type>dot1q-vlan</tagged-inf-type>
==>
                         <tagged-inf-type>dot1q</tagged-inf-type>
5.
                          <vlan-id>17</vlan-id>
==>
                          <cvlan-id>17</cvlan-id>
 
6.
Need to move <service> out of <site-network-accesses/>
 
7.
Missing end tag
                           </bandwidth>
 
8.
Not making up our mind on how to spell signaling/signalling bites us:
 
                             <signaling-type>bgp</signaling-type>
==>
                             <signalling-type>bgp</signalling-type>
Same omissions as on p39.
 
p68a:
Example broken:
 
Some unwanted whitespace crept in:
                     <svc-type>vpws</svc -type >
==>
                     <svc-type>vpws</svc-type>
 
/l2vpn-svc/vpn-services/vpn-service["12456487" and
"12456488"]/frame-delivery/multicast-gp-port-mapping is mandatory and not
configured
 
p68b:
Example broken in a number of places:
 
1.
                <location>
                   <city>NY</city>
                   <country-code>US</country-code>
                </location>
==>
                <locations>
                <location>
                  <location-id>NY1</location-id>
                   <city>NY</city>
                   <country-code>US</country-code>
                </location>
                </locations>
 
2.
Double start tags
                    <access-diversity>
==>
                    </access-diversity>
3.
I'm guessing the author means "vlan" when saying "dot1q" ?
                     <encapsulation-type>dot1q</encapsulation-type>
==>
                     <encapsulation-type>vlan</encapsulation-type>
 
4.
Wrong end tag
                      </untagged-interface>
==>
                      </tagged-interface>
5.
Needs to be lowercase
                        <stp-rstp-mstp>TUNNEL</stp-rstp-mstp>
==>
                        <stp-rstp-mstp>tunnel</stp-rstp-mstp>
6.
                        <lldp>TRUE</lldp>
==>
                        <lldp>true</lldp>
7.
Need to move <service> out of <site-network-accesses/>
 
8.
                       <signaling-type>bgp</signaling-type>
==>
                       <signalling-type>bgp</signalling-type>
 
Same omissions as on p39.
 
 
 
Obviously, these XML fixes assume an unchanged YANG model. As they YANG is
fixed/changed, the examples should follow suit.
 
That's all I found.
 
Thanks!
/jan
 
 
 
From: Jan Lindblad [mailto:[e-mail]] 
Sent: 17 January 2018 11:29
To: Adrian Farrel
Cc: [e-mail]; Qin Wu
Subject: Re: Cheeky request
 
Adrian,
 
You did such a good job with the L3SMbis draft that it should come as no
surprise that we are all queueing up to have you as a reviewer.

 <https://datatracker.ietf.org/doc/draft-ietf-l2sm-l2vpn-service-model/>
https://datatracker.ietf.org/doc/draft-ietf-l2sm-l2vpn-service-model/ is now
very close to WG last call and we just made a request for "early" YANG Doctor
review.

But if you were able to look at it as well, that would be rather good. Of
course, you are allowed to be tired/bored/busy :-)
 
Flattery is often effective ;-) Thank you for asking. I would certainly be
interested to have a good look at all IETF service models, and perhaps better to
get into the habit of doing that before the publication rather than after :-/ I
guess it's primarily going to be a question of when you need the answer. By when
would you need it?
 
Best Regards,
/jan
--
Jan Lindblad,  <mailto:[e-mail]> [e-mail], [phone]
Solutions Architect, Business Development, Tail-f
Tail-f is now a part of Cisco