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
>
> +1-424-254-5300 (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
> OPSAWG@ietf.org
> https://www.ietf.org/mailman/listinfo/opsawg
>
_______________________________________________
OPSAWG mailing list
OPSAWG@ietf.org
https://www.ietf.org/mailman/listinfo/opsawg
