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

[[e-mail]]

Hi Adrian, 

Many thanks for the careful review. 

You can track the changes to address your comments at: https://tinyurl.com/l2nm-latest. 

Please see inline for more context. 

Cheers,
Med

> -----Message d'origine-----
> De : OPSAWG <[e-mail]> De la part de Adrian Farrel
> Envoyé : jeudi 18 novembre 2021 19:25
> À : [e-mail]
> Cc : [e-mail]
> Objet : [OPSAWG] Shepherd review of draft-ietf-opsawg-l2nm
> 
> 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.
> 

[Med] Thanks. I prefer to leave the current wording. 

> ---
> 
> Abstract doesn't need "(VPN)"
> 

[Med] OK.

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

[Med] Fixed. 

> ---
> 
> 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
> 

[Med] This is better. thanks. 

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

[Med] Fixed. Thanks. 

> ---
> 
> 2. Service orchestrator
> 
> s/responsible of the CE-PE/responsible for the CE-PE/
> 

[Med] ACK.

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

[Med] Done. 

> ---
> 
> 2. Layer 2 VPN Service Network Model (L2NM)
> 
> s/providers network/provider's network/
> 

[Med] OK

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

[Med] You are right. AAA was brought from Figure 3 of RFC8466. I prefer to delete it rather than adding text. 

> 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.
> 

[Med] We do already have "The attachment circuits (bearers) between Customer Edges (CEs) and Provider Edges (PEs) are terminated in the VPN network access. "

Added "Similar to Section 3 of [RFC8466] CE to PE attachment is achieved through a bearer with a Layer 2 connection on top. The bearer refers to properties of the attachment that are below Layer 2, while the connection refers to Layer 2 protocol-oriented properties."

> ---
> 
> 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.
> 

[Med] The introduction has already the following:

   Section 8.3 defines a YANG
   module to manage Ethernet Segments (ESes) that are required for
   instantiating EVPNs.


Updated the abstract. Thanks. 

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

[Med] For this one, this is exactly the output we get from pyang.  

> 
> 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.
> 

[Med] :-) It does not have ... but idnits won't be happy. 

This is how trees are adjusted when run with "--tree-line-length 69"

> ---
> 
> Section 5
> 
> s/is meant to manage L2VPN services/is used to manage L2VPN services/

[Med] OK

> 
> ---
> 
> 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?
> 

[Med] Changed to "The description of the data nodes depicted in Figure 3 is as follows:"

> ---
> 
> Section 6
> 
> 'name'   s/with a service/within a service/

[Med] Fixed.

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

[Med] The OLD one is OK because we are trying to be consistent with the use in rfc7432. Thanks.

> 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.
> 

[Med] Updated the description to cite 'auto-ethernet-segment-identifier', 'esi-pool-name', and 'ethernet-segment-identifier'. 

> ---
> 
> Section 7
> 
> s/is meant to manage L2VPNs/is used to manage L2VPNs/
> 

[Med] ACK. 

> ---
> 
> Section 7.2
> 
>  'forwarding-profile-identifier':
>       Such policies may consist, for example, at applying Access
>       Control Lists (ACLs).
> 
> Can't parse this.

[Med] You can't. Only those like me having English being their forth language can parse this!


 Maybe "...may consist, for example, of applying..."
> 

[Med] Fixed. Thank you/

> ---
> 
> 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'/
> 

[Med] Fixed. 

> ---
> 
> 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,/
> 

[Med] ACK

> ---
> 
> Section 7.3
> 
>                   Table 1: Valid Signaling Options per VPN
>                                 Service Type
> 
> Maybe delete "Valid"
> 

[Med] OK

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

[Med] :-)

Updated to "Priority Code Point (PCP)"

> 
> ---
> 
> Section 7.5.1
> 
> s/'bgp-auto-discovery' container/The 'bgp-auto-discovery' container/
> 

[Med] Fixed. 

> ---
> 
> Section 7.5.2.1
> 
> s/this is VLAN-based/a this is VLAN-based/

[Med] Changed to "this is a 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.

[Med] Added. Thanks.

> 
> ---
> 
> Section 7.5.2.2
> 
> s/include a an/include an/

[Med] Fixed.  


> 
> ---
> 
> 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.
> 

[Med] Agree. Fixed. 

> ---
> 
> 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?

[Med] I fully agree. No need to have that sentence. This is the kind of text you borrow since early days of a draft. 

> 
> ---
> 
> Section 7.6
> 
> s/' global-parameters-profile'/'global-parameters-profile'/
> 

[Med] ACK. 

> ---
> 
> Section 7.6.1
> 
> s/OAM 802.3 ah/OAM 802.3ah/
> 

[Med] Fixed. 

> ---
> 
> Section 7.6.4
> 
> 'svc-pe-to-ce-bandwidth' and 'svc-ce-to-pe-bandwidth' are missing colons
> 

[Med] Please note that we have "'svc-pe-to-ce-bandwidth' and 'svc-ce-to-pe-bandwidth':" to factorize the description for both directions. No colons are used for 'svc-pe-to-ce-bandwidth' and 'svc-ce-to-pe-bandwidth' because these are not in the same level as the previous bullets. Rearranged the text to avoid this confusion. Sorry.

> ---
> 
> 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

[Med] :-)


> Instead of "is designed to echo" say "echoes"
> 

[Med] Fixed. Thanks.

> ---
> 
> Section 8.1
> 
>    Appendix B lists the
>    initial values included in the "iana-bgp-l2-encaps" YANG module.
> 
> Initial values of what?

[Med] ... of the registry. This is a  living module if you will. It will be automatically updated by IANA when new values are assigned. 


 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.
> 

[Med] A pointer to the IANA registry would be sufficient, but idnits won't be happy as all reference statements have to be called outside the YANG modules. These tables solves this issue. 

> ---
> 
> 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].
> 

[Med] Agree. Unlike 8.3 and 8.4, we don't have such statements in 8.1/8.2 because we don't have any imports. 

> ---
> 
> 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)
> 

[Med] You are right, but the one in the draft is what is used by IANA for IANA-maintained YANG modules. 

> ---
> 
> 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?
> 

[Med] That's what is currently in the registry. We are not modifying it or fixing any issue we may find. 

> ---
> 
> 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?
> 

[Med] ... because we failed to find what is "[[ATM]]" in the IANA registry (please see Appendix C). Please note that we have a reference statement in 8.1 for atm-aal5. 

Fixed atm-vp-virtual-trunk by adding a pointer to MFA. 

> ---
> 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.
> 

[Med] Joe raised a similar comment in the past. We added this general pointer rather that redefining all the inherited item in the L2NM: 

   See also Section 5.10.2.1 of [RFC8466].

That section says the following:

==
   A
   "color-id" will be assigned to a service frame to identify its QoS
   profile conformance.  A service frame is "green" if it is conformant
   with the "committed" rate of the bandwidth profile.  A service frame
   is "yellow" if it exceeds the "committed" rate but is conformant with
   the "excess" rate of the bandwidth profile.  Finally, a service frame
   is "red" if it is conformant with neither the "committed" rate nor
   the "excess" rate of the bandwidth profile.
==

As I expect this to pop-up in other reviews, I updated the yang description with more text. 

> ---
> 
> 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

[Med] Added one. 

 and measurement-interval do not.
[Med] Will check if there is a default to mention.

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

[Med] Will check. 

> 
> 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?
> 

[Med] Added some more, but will further check. 

> ---
> 
> 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/
> 

[Med] Fixed.

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

[Med] this is not correct. Updated as follows: 

                "An identifier of the network element where
                 the VPN node is deployed. This identifier
                 uniquely identifies the network element within
                 an administrative domain."
> 
> 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.

[Med] Indeed. 

> 
> 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?

[Med] to the remote peer. Changed to:

               "Includes a human-readable description of 
                the interface. This may be used when 
                communicating with a remote peer." 

FWIW, this echoes this part from RFC4447:

      This arbitrary, and OPTIONAL, interface description string is used
      to send a human-readable administrative string describing the
      interface to the remote.

> 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.
> 

[Med] The text says ".. such as a layer 2 sub-interface" (for l2-termination-point) and ".. A reference may be a local bridge domain" (for local-bridge-reference). 

For bearer-reference, I made this change: 

OLD: 
   VPN network access:  Is an abstraction that represents the network
      interfaces that are associated to a given VPN node.  Traffic
      coming from the VPN network access belongs to the VPN.  The
      attachment circuits (bearers) between Customer Edges (CEs) and
      Provider Edges (PEs) are terminated in the VPN network access.

NEW:
   VPN network access:  Is an abstraction that represents the network
      interfaces that are associated to a given VPN node.  Traffic
      coming from the VPN network access belongs to the VPN.  The
      attachment circuits (bearers) between Customer Edges (CEs) and
      Provider Edges (PEs) are terminated in the VPN network access.
      A reference to the bearer is
      maintained to allow keeping the link between L3SM and L3NM when
      both models are used in a given deployment.  

> ---
> 
> Section 8.4
>                              leaf port-speed {
>                                type uint32;
>                                description
>                                  "Port speed.";
>                              }
> 
> This is missing a units clause.

[Med] Good catch. Added "units "mbps"

 And I would be slightly worried that
> uint32 might not be future-proof with some possible units.
> 

[Med] I think we are safe with the proposed unit. 
                         
> ---
> 
> 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?
> 

[Med] Fair. Changed to "Identifies the CE within the VPN."

> ---
> 
> 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/] ./]./
> 

[Med] Fixed. 

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

[Med] This is already mentioned in 10.1

==
         name: iana-bgp-l2-encaps
         namespace: urn:ietf:params:xml:ns:yang:iana-bgp-l2-encaps
         maintained by IANA: Y <=========
         prefix: iana-bgp-l2-encaps
         reference: RFC XXXX

         name: iana-pseudowire-types
         namespace: urn:ietf:params:xml:ns:yang:iana-pseudowire-types
         maintained by IANA: Y <=========
         prefix: iana-pw-types
         reference: RFC XXXX
==

and repeated in 10.2/3: 
  "This document defines the initial version of the IANA-maintained.."

> ---
> 
> Thanks for all the examples. They are helpful.
> 

[Med] Thanks. I hope we can have more integrated tools to check the json with the module structure. 

> ---
> 
> A.1
> 
> s/used to configured/used to configure/
> 

[Med] Fixed. 

> ---
> 
> A.4
> 
> s/depictes/depicts/
> 

[Med] Fixed.

> ---
> 
> 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.
> 

[Med] Thanks for catching this. Fixed. 

This should be fixed in Figure 3 of RFC8214 as well. 

> ---
> 
> A.6
> 
> s/In such as configuration/In such a configuration/
> 

[Med] Fixed. 

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

[Med] This is used as a simple way to cite all the references outside the module. If not, idnits won't he happy.

_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.