Hi Rob, Many thanks for the review. A candidate updated version can be seen at: https://tinyurl.com/vpn-common-latest
Please see inline. Cheers, Med > -----Message d'origine----- > De : Rob Wilton (rwilton) [mailto:rwil...@cisco.com] > Envoyé : lundi 12 juillet 2021 17:15 > À : draft-ietf-opsawg-vpn-common....@ietf.org > Cc : opsawg@ietf.org > Objet : AD review of draft-ietf-opsawg-vpn-common-08 > > Hi, > > This is my AD review of draft-ietf-opsawg-vpn-common-08. > > Thank you for this document. Again, just minor > comments/suggestions. > > > > 1. > In section 3. Description of the VPN Common YANG Module > "Encapsulation features such as" -> "Encapsulation features. Such > as" > "Routing features such as" -> "Routing features. Such as" > [Med] Fixed. > 2. > As a very minor comment. Where you have lists (i.e., encapsulation > features, routing features, service type) and particularly because > they have references, it may be slightly easier to read if the list > was indented. Also does it make sense for this list to just be > examples, or are they actually normative lists of service types that > are supported? [Med] Sure. These are only examples. We cite them as we need to list include in the main text any reference quoted in the module. > > E.g., > 'service-type': Used to identify the VPN service type. > Examples of supported service types are: > > o L3VPN, > > o Virtual Private LAN Service (VPLS) using BGP [RFC4761], > > o VPLS using Label Distribution Protocol (LDP) [RFC4762], > > o Virtual Private Wire Service (VPWS) [RFC8214], > > o BGP MPLS-Based Ethernet VPN [RFC7432], > > o Ethernet VPN (EVPN) [RFC8365], and > > o Provider Backbone Bridging Combined with Ethernet > VPN (PBB-EVPN) [RFC7623]. > > > In the Yang Module: > > 3. OPSA => OPSAWG > Please can you check if this needs to be fixed for the L3NM YANG > model as well. [Med] Fixed. > > 4. > feature qinany { > description > "Indicates the support of the QinAny encapsulation."; > } > > Is there a reference, or perhaps a more detailed description that > you can use here? > [Med] This was also a comment raised in the WGLC, but we don't have any acceptable authoritative reference to cite for it. > 5. > Very minor: > feature ipv4, feature ipv6, is it worth adding references to the > RFCs? I appreciate that they are obvious, but since you have > references for everything else, it seems like it might be worth > using adding them? [Med] OK > > 6. > feature rtg-ospf-sham-link { > description > "Indicates support of OSPF sham links."; > > Does this mean that feature rtg-ospf excldues this support? This > feature seems very specific relative to the other features in this > YANG module. [Med] Yes. We are barrowing this one for RFC8299, fyi. > > 7. > As mentioned in the other document, would "feature carrierscarrier" > be better as "feature carriers-carrier"? [Med] No problem. Fixed. > > 8. > "Indicates the support of" => "Indicates support for" > "Indicates support of" -> "Indicates support for" [Med] Fixed. > > 9. > This model defines a lot of features, and I wasn't sure how helpful > that will really be in practice. Is the intention here for an SP to > use features to customize the model to their needs? I wonder if the > heavy use of features won't work so well if both L2VPN and L3VPN's > are being modelled and support different protocols/etc. Will having > the common features act as a limitation? E.g., an alternative might > be to express the features in the L2NM and L3NM models directly > allowing them to enabled/disable different features. [Med] The "common" set of features was initially included to rationalize the LxSM and LxNM. I don't know if/and to what extent this would have limitations when both L2xM and L3xM. > > 10. Status leaves: > Would UP, DOWN, UNKNWON be better as Up, Down, Unknown? [Med] Fixed. > Would "admin-enabled" be better than "admin-up" and "admin-disabled" > be better than "admin-down". [Med] I prefer the OLD as it is short + not problematic when including examples and make no folding is used (please trust me, that's a nightmare). > > For all the non-base identities, I would suggest removing the > "Identity for" prefix in the descriptions. It is self-evident that > the descriptions are for identities, and the extra words probably > would not help a GUI rendering of the description strings. [Med] Good suggestion. > > 11. For all the "service type" identities, I would suggest ending > all the descriptions with "service". > E.g., > "L3VPN service."; > "Provider Backbone Bridging (PBB) EVPNs service."; > "Virtual Private LAN Service (VPLS)."; > "Point-to-point Virtual Private Wire Service (VPWS)."; > "Provider Backbone Bridging (PBB) EVPN service."; > "MPLS based EVPN service."; > "VXLAN based EVPN service."; > [Med] OK. > 12. > For the signalling identity descriptions: > "Layer 2 VPNs using BGP signalling"; > "Targeted LDP signalling."; > "L2TP signalling."; [Med] Fixed. > > 13. > For the bgp-signalling identity, does it make sense for the > description to be specific to L2VPNs? Couldn't bgp-signalling also > be used for L3VPNs? [Med] RFC4364 says: " - DOES NOT require that there be any explicit setup of the tunnels, either via signaling or via manual configuration; - DOES NOT require that there be any tunnel-specific signaling; " > > 14. > For the routing-protocol-type generic identities, would it make > sense to add a "-routing" suffic to them, e.g., "direct-routing", > "any-routing"? Otherwise some of the identity names look fairly > generic, but perhaps that is okay. > [Med] OK to add the "-routing" suffix. > 15. > vpn-topology: > Is No P2P topology identity required? [Med] The VPN topology is more related to how the site can communicate with each other. We are using the traditional roles: hub, spoke, etc. > > 16. > For identity qos-profile-direction: > Rather than "site-to-wan" and "wan-to-site" identity names, would > it be better to use "vpn" or "service" instead of wan. E.g., "site- > to-vpn", or "site-to-service"? [Med] I prefer the old name, as sites can be considered by some as "part" of the VPN. > > 17. > identity enhanced-vpn > identity ietf-network-slice > > Would it be helpful to add RFC or draft references for these > identities? E.g., [I-D.ietf-teas-enhanced-vpn], or an IETF network > slice service [I-D.ietf-teas-ietf-network-slices] [Med] We don't cite those as they are not "stable" pointers yet. > > 18. > identity protocol-type { > description > "Base identity for Protocol Type."; > } > > identity unknown { > base protocol-type; > description > "Not known protocol type."; > } > > Is this identity required/useful? Wouldn't it be better to just > leave the leaf not populated, but in the general case, shouldn't > this always be specified? [Med] that's OK for the write operations. What we had in mind is more read operations to report that the underlying transport is not known to the controller. > > 19. > identity encapsulation-type { [Med] this one is about the encapsulation type. > vs identity tag-type [Med] this is more about the tag type (c, s, etc.) > - These seem to both effectively convey similar information. > - c-s-vlan, should probably be s-c-vlan, since it is normal to > specify encapsulation from outermost inner. > [Med] We used c-s to follow the order in which c-* and s-* identities are listed. I don't have a preference. Changed to s-c-vlan > 20. > identity tf-type > There is no type for unicast. Is it not required? [Med] We are not defining it here as the main case we have for this is the so called "bum" (Broadcast, Unknown Unicast, or Multicast). > > 21. > grouping vpn-description { > > Which, if any, of these fields are expected to uniquely identify the > VPN? > E.g., what, if any, uniqueness requirements are there for vpn-name? [Med] It is the vpn-id. Update to make this clear in the module: "A VPN identifier that uniquely identifies a VPN" > > > leaf vpn-id { > type vpn-id; > description > "VPN identifier. > This identifier has a local meaning."; > } > > What is meant by "local meaning", could this be clarified? [Med] Updated to: This identifier has a local meaning, e.g., within a service provider network. > > > 22. The vpn-id type seems to be used very generically for quite a [Med] Yes. > few different things, and I was wondering whether having more > specific subtypes of vpn-id might be helpful? [Med] Not sure this is useful as we don't associate any specific "structure". > > > 23. > What's the difference between service-timestamp [Med] This is about operational status. Updated to oper-service-timestamp and service-status? [Med] This covers both admin and oper status. > Both include timestamps and status. Perhaps slightly different > names for these groupings might make them more consistent (e.g., > oper-service-status, admin-oper-service-status). > > 24. > last-updated is okay, but note that ietf-interfaces.yang uses last- > changed instead. Given the description mentions change, last-change > might be better? [Med] Went for "last-change". > > 25. > I'm not sure that the tree diagram examples in Appendix A is > actually that useful, given that they do not represent what the > model is now, just how it used to be. I would suggest keeping the > text that justifies the approach taken but remove the trees. [Med] That's a good input. > > I also annotated part of the YANG model (just the grouping > descriptions) with comments inline. Please see suggestions on > (#RW:) inline in the attached file. It is up to you whether and how > you want to incorporate these and I don't need to see your response. > [Med] Thanks. I incorporated almost all your suggestions. > Grammar Warnings (by automated tool): > Section: 3, draft text: > For example, diversity or redundancy constraints can be applied on a > per group basis. > > Warning: In this context, per-group forms an adjective and is > spelled with a hyphen. > Suggested change: "per-group" [Med] Fixed. > > Regards, > Rob _________________________________________________________________________________________________________________________ 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. _______________________________________________ OPSAWG mailing list OPSAWG@ietf.org https://www.ietf.org/mailman/listinfo/opsawg
