[OPSAWG] Shepherd review of draft-ietf-opsawg-l2nm

[Adrian Farrel <[e-mail]>]

I have done my review of draft-ietf-opsawg-l2nm based on revision -10 of
the draft. Sorry it took so long, but there are a few pages of text that
needed to be read.

I have to congratulate the authors on this substantial piece of work. A
lot of time and effort must have gone into it.

It seems to be in pretty reasonable shape to me. Of course, it would be
a surprise if such a long document didn't have a few issues, but most of
what I have below is just nits.

Please feel free to ask for clarification or to refute anything I have
said.

Thanks,

Adrian

====

I see "layer 2" and "Layer 2" etc. I think in the context of L2VPN,
L2NM, etc., "Layer 2" is fine. But others "like Layer 2 connection"
are used inconsistently.

---

Abstract doesn't need "(VPN)"

---

Abstract

s/providers/provider/
s/Also, the document/Also, this document/
s/that defines/that define/

---

1.

OLD
   [RFC8466] defines an L2VPN Service Model (L2SM) YANG data model that
   can be used for Layer 2 Virtual Private Network (L2VPN) service
   ordering matters between customers and service providers.
NEW
   [RFC8466] defines an L2VPN Service Model (L2SM) YANG data model that
   can be used between customers and service providers for ordering
   Layer 2 Virtual Private Network (L2VPN) services.
END

---

1.

s/providers network/provider's network/
s/Also, the document/Also, this document/
s/rather than be limited/rather than being limited/

---

2. Service orchestrator

s/responsible of the CE-PE/responsible for the CE-PE/

---

2.

You might put "VPN node" before "VPN network access" to resolve the
reference.

---

2. Layer 2 VPN Service Network Model (L2NM)

s/providers network/provider's network/

---

Figure 1

It is unclear what +++++++
                   + AAA +
                   +++++++  means, and I don't see any text about it.

Is "Bearer Connection" identical to "Ethernet Segment"? See the
definition of "bearer" in "VPN network access" in section 2. If they are
the same, why use two terms? If they are different, please define bearer
connection.

---

Section 6 and the Ethernet Segment YANG Module comes as a bit of a
surprise.  There is just one mention of that module (in Section 5).

I think the Abstract and Introduction should at least mention the ES
module.

---

Figure 3 appears to have some rather random whitespace. Needs tidying?
Perhaps tabs were used in the original.

Figures 7, 8, 9, 11, 12, 13, 15 are pretty bad, too.

Figures 6, 14, and 22 have this a little.

Figure 17 is pretty much OK, but I don't know that "enumeration" needs
to be on a separate line each time.

---

Section 5

s/is meant to manage L2VPN services/is used to manage L2VPN services/

---

Section 6

   In reference to the structure shown in Figure 3, the following data
   nodes can be included:

Too much passive voice. Can be included:
- where?
- by whom?
- under what circumstances are they included or left out?

---

Section 6

'name'   s/with a service/within a service/

'esi-redundancy-mode'    Not sure, should you...
      s/Single-Active/'single-active'/
      s/All-Active/'all-active/

There are a number of data nodes shown in Figure 3 that don't have
explanations in the text. Is that intentional? It would seem to me that
they should be described in the same way as the other nodes. If you mean
to leave them out, perhaps say so and point at the YANG for definitive
descriptions of the other nodes.

---

Section 7

s/is meant to manage L2VPNs/is used to manage L2VPNs/

---

Section 7.2

 'forwarding-profile-identifier':
      Such policies may consist, for example, at applying Access
      Control Lists (ACLs).

Can't parse this. Maybe "...may consist, for example, of applying..."

---

Section 7.3

s/Such 'vpn-id' is/Such a 'vpn-id' is/


'signaling-type':  Indicates the signaling that is used for setting
   s/setting/setting up/

s/l2tp-signaling'/'l2tp-signaling'/

---

Section 7.3
   'underlay-transport'
      (e.g., an identifier of a VPN+ instance, a virtual network
      identifier, or a network slice name)
I don't know why you have "identifier" for the first two, but "name" for
a network slice. Wouldn't "identifier" serve better for all three cases
as...
      (e.g., an identifier of a VPN+ instance, virtual network, or
      network slice)

   'status'
   s/provision/provisioning/

   'vpn-node'
   s/various 'description' data node/various 'description' data nodes,/

---

Section 7.3

                  Table 1: Valid Signaling Options per VPN
                                Service Type

Maybe delete "Valid"

---

Section 7.4

I think PCP needs to expanded here. (Especially since you don't mean
Port Control Protocol.)

---

Section 7.5.1

s/'bgp-auto-discovery' container/The 'bgp-auto-discovery' container/

---

Section 7.5.2.1

s/this is VLAN-based/a this is VLAN-based/

---

Section 7.5.2.1

   The value of the 'pw-
   encapsulation-type' are taken from the IANA-maintained "iana-bgp-
   l2-encaps" module.

A reference to Section 8.1 would be good.

---

Section 7.5.2.2

s/include a an/include an/

---

Section 7.5.2.3

   The PW type ('pseudowire-type') value is taken from the
   IANA-maintained "iana-pseudowire-types" module.

A reference to Section 8.2 would be good.

---

7.6

   As such, every 'vpn-network-access' MUST belong to
   one and only one 'vpn-node'.

I'm not sure how to interpret this. The vpn-network-access part of the
tree is anchored under vpn-node. So it cannot be in two places at once.

Maybe this is saying that the value of the 'id' node must not appear
twice across the whole instantiation of the data model?

---

Section 7.6

s/' global-parameters-profile'/'global-parameters-profile'/

---

Section 7.6.1

s/OAM 802.3 ah/OAM 802.3ah/

---

Section 7.6.4

'svc-pe-to-ce-bandwidth' and 'svc-ce-to-pe-bandwidth' are missing colons

---

Section 8.1

   The "iana-bgp-l2-encaps" YANG module (Section 8.1) is designed to
   echo the registry available at [IANA-BGP-L2].

This *is* Section 8.1
Instead of "is designed to echo" say "echoes"

---

Section 8.1

   Appendix B lists the
   initial values included in the "iana-bgp-l2-encaps" YANG module.

Initial values of what? I think you are saying that the YANG model is
populated according to the state of the registry at the time of
publication of this document (and that new entries to the registry
should be matched with additional nodes in this model).

So, I would be inclined to leave this out.

And actually, I'd omit Appendix B which doesn't add anything.

---

8.1 and other subsections

It is useful to help resolve references and import clauses by placing
some text before the <CODE BEGINS> to say something like...

   This module makes imports from [RFCaaaa], and makes references to
   [RFCbbbb] and [RFCcccc].

---

Section 8.1

I believe the correct postal details are:

   Internet Assigned Numbers Authority (IANA)
   12025 Waterfront Drive, Suite 300
   Los Angeles CA 90094
   USA

   [phone] (phone)

---

8.1 and 8.2 
     identity layer-2-transport {
       base foo;
       description
         "IP Layer 2 Transport.";
       reference
         "RFC 3032: MPLS Label Stack Encoding";
     }

I don't quite get this. Is the reference wrong? Is the name of the
identity and the description wrong? 

---

Section 8.2

The comments about the introductory paragraphs of 8.1 apply to 8.2.
Also the comment about the contact address.

---

Section 8.2

Why don't atm-aal5 and atm-vp-virtual-trunk have reference clauses?

---
Section 8.4 

I don't find any explanation or reference for the color-type identities
or the color-type leaf. Maybe a description for Figure 20, or a 
description clause for the leaf or the base identity.

---

Section 8.4

I'm puzzled as to which leaf nodes qualify for a default clause and
which don't.

For example, ccm-interval and ccm-holdtime have default values. That
seems reasonable. But message-period and measurement-interval do not.

Similarly, mac-loop-prevention and frequency have defaults, retry-timer
does not.

Can you say how you decided which leaf nodes don't merit a default
value? Can you check that you have assigned a default everywhere one is
needed?

---

Section 8.4

       leaf ce-vlan-preservation {
         type boolean;
         description
           "Preserve the CE-VLAN ID from ingress to egress,i.e.,
            CE-VLAN tag of the egress frame are identical to
            those of the ingress frame that yielded this egress
            service frame. If All-to-One bundling within a site

s/,i.e.,/, i.e.,/
s/are identical/is identical/
s/those/that/

---

Section 8.4

               leaf ne-id {
                 type string;
                 description
                   "Indicates the node's IP address.";
               }

But 7.5 had
   'ne-id':  Includes a unique identifier of the network element where
      the VPN node is deployed.
...and 8.3 had
           leaf ne-id {
             type string;
             description
               "Unique identifier of the network element where the ES
                is configured with a service provider network.";

Now, I know that a node's IP address is usually used as the unique 
identifier of the node, but 7.5 and 8.3 make it look like you could use
something else so long as it is unique.

Additionally, "unique" is probably stronger than you mean, since you are
probably happy with a certain domain of uniqueness.

Compare and contrast with

               leaf router-id {
                 type rt-types:router-id;
                 description
                   "A 32-bit number in the dotted-quad format that is
                    used to uniquely identify a node within an
                    autonomous system (AS). ";
               }

---

Section 8.4
                         leaf pw-description {
                           type string;
                           description
                             "Includes an interface description used to
                              send a human-readable administrative
                              string describing the interface to the
                              remote.";

To the remote what? 
Actually, to be completely clear, are you saying that the interface
description is used to send a string to the remote? Or is this garbled?

Could be...
Includes a human-readable description of the interface. This may be used
when communicating with remote administrative processes to identify the
interface.

---

Section 8.4

There is a set of three members of the container 'connection' that are 
of type string : l2-termination-point, local-bridge-reference, and
bearer-reference. The description classes are clear up to a point, but
"a reference to" doesn't really tell me what I should put in these
objects. 

---

Section 8.4
                             leaf port-speed {
                               type uint32;
                               description
                                 "Port speed.";
                             }

This is missing a units clause. And I would be slightly worried that
uint32 might not be future-proof with some possible units.

---

Section 8.4

                         leaf ce-id {
                           type uint16;
                           description
                             "The PE must know the set of virtual
                              circuits connecting it to the CE and a
                              CE ID identifying the CE within the VPN.";

The first part of the description ("set of VCs connecting to the CE") is
true, but is it relevant to this leaf?

---

Section 9

   The YANG modules specified in this document defines schema for data
   that is designed to be accessed via network management protocols such
   as NETCONF [RFC6241] or RESTCONF [RFC8040] . The lowest NETCONF layer

s/defines schema/define schemas/
s/that is/that are/
s/] ./]./

---

I think 10.2 and 10.3 should make it clearer that IANA is requested to
maintain the two YANG models.

---

Thanks for all the examples. They are helpful.

---

A.1

s/used to configured/used to configure/

---

A.4

s/depictes/depicts/

---

A.4

In Figure 29 is it intentional that the left-hand end of the Emulated
Service is at the edge of the CE, but the right-hand end is in the 
middle of the CE.

---

A.6

s/In such as configuration/In such a configuration/

---

I'm unclear of the purpose of Appendixes B and C. They seem to repeat
information that is found in the relevant IANA registries.