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

[Fioccola Giuseppe <[e-mail]>]

Hi Jan, Adrian,
First of all, Thank you Jan for your good opinion of the document (in particular for the YANG :)).
Your review is very detailed and I really appreciate it.
I will check all your comments and address them in a new revision.
If there are doubts, as usual, I will share them with you and the L2SM WG.

Best Regards,

Giuseppe

Da: Adrian Farrel [mailto:[e-mail]]
Inviato: mercoledì 31 gennaio 2018 15:31
A: [e-mail]
Cc: [e-mail]; 'Jan Lindblad'
Oggetto: FW: YANG + XML review of draft-ietf-l2sm-l2vpn-service-model-05

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]<mailto:[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-port-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]<mailto:[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/ 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, [e-mail]<mailto:[e-mail]>, [phone]
Solutions Architect, Business Development, Tail-f
Tail-f is now a part of Cisco


Questo messaggio e i suoi allegati sono indirizzati esclusivamente alle persone indicate. La diffusione, copia o qualsiasi altra azione derivante dalla conoscenza di queste informazioni sono rigorosamente vietate. Qualora abbiate ricevuto questo documento per errore siete cortesemente pregati di darne immediata comunicazione al mittente e di provvedere alla sua distruzione, Grazie. 

This e-mail and any attachments is confidential and may contain privileged information intended for the addressee(s) only. Dissemination, copying, printing or use by anybody else is unauthorised. If you are not the intended recipient, please delete this message and any attachments and advise the sender by return e-mail, Thanks. 

Rispetta l'ambiente. Non stampare questa mail se non è necessario.