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

["Joe Clarke (jclarke)" <[e-mail]>]

Thank you for taking on these drafts, Adrian and for a thorough review
of L2NM.

Joe

On 11/18/21 13:25, Adrian Farrel wrote:
> 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.
>
> _______________________________________________
> OPSAWG mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/opsawg
>