Hi Med, [Just to also close this thread/review.]
I think that you are good to go. Thanks for accommodating my comments. Regards, Rob > -----Original Message----- > From: mohamed.boucad...@orange.com > <mohamed.boucad...@orange.com> > Sent: 13 July 2021 16:17 > To: Rob Wilton (rwilton) <rwil...@cisco.com>; draft-ietf-opsawg-vpn- > common....@ietf.org > Cc: opsawg@ietf.org > Subject: RE: AD review of draft-ietf-opsawg-vpn-common-08 > > Re-, > > I updated the file to take into account your feedback: > > * Module: https://github.com/IETF-OPSAWG- > WG/lxnm/commit/fcc9f6c1e79d5074469fe454d95770fe9f1becdb (in addition > to the changes already reported as part of the L3NM review). > * I-D: https://tinyurl.com/vpn-common-latest > > Unless you have further comments, I will contact the secretariat to publish > this version. Thanks. > > Please see inline. > > Cheers, > Med > > > -----Message d'origine----- > > De : Rob Wilton (rwilton) [mailto:rwil...@cisco.com] > > Envoyé : mardi 13 juillet 2021 16:25 > > À : BOUCADAIR Mohamed INNOV/NET > <mohamed.boucad...@orange.com>; > > draft-ietf-opsawg-vpn-common....@ietf.org > > Cc : opsawg@ietf.org > > Objet : RE: AD review of draft-ietf-opsawg-vpn-common-08 > > > > Hi Med, > > > > Please see inline, just a couple of points to resolve ... > > > > > -----Original Message----- > > > From: mohamed.boucad...@orange.com > > > <mohamed.boucad...@orange.com> > > > Sent: 12 July 2021 18:35 > > > To: Rob Wilton (rwilton) <rwil...@cisco.com>; draft-ietf-opsawg- > > vpn- > > > common....@ietf.org > > > Cc: opsawg@ietf.org > > > Subject: RE: AD review of draft-ietf-opsawg-vpn-common-08 > > > > > > 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. > > > > > > Okay. > > > > > > > > > 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. > > > > Okay. > > > > > > > > > > > > > > > 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. > > > > Okay. > > > > > > > > > > > > > 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. > > > > Okay, then perhaps give a brief description of what QinAny means, > > i.e., in this context I think that frames have a pair of VLAN Ids, > > where the outer VLAN Id is fixed, but the inner VLAN Id can take any > > valid VLAN Id value. > > [Med] Added a note to the module. > > > > > > > > > > > > 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 > > > > Okay. > > > > > > > > > > > > > 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. > > > > Okay, makes sense. > > > > > > > > > > > > > > > 7. > > > > As mentioned in the other document, would "feature > > carrierscarrier" > > > > be better as "feature carriers-carrier"? > > > > > > [Med] No problem. Fixed. > > > > Okay. > > > > > > > > > > > > > 8. > > > > "Indicates the support of" => "Indicates support for" > > > > "Indicates support of" -> "Indicates support for" > > > > > > [Med] Fixed. > > > > Okay. > > > > > > > > > > > > > 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. > > > > Okay. > > > > > > > > > > > > > > > 10. Status leaves: > > > > Would UP, DOWN, UNKNWON be better as Up, Down, Unknown? > > > > > > [Med] Fixed. > > > > Okay. > > > > > > > > > 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). > > > > I'm not sure that I find that as a compelling reason. :-) > > [Med] That's may laziness then. > > > > > I presume that you aware of Kent's RFC and script for > > folding/unfolding instance data examples? > > [Med] I can't say no as we use it in the L2NM :-) > > The readability is still superior when we don't use the folding. > > > > > > > > > > > > > > > > > > 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. > > > > Okay. > > > > > > > > > > > > > > > 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. > > > > Okay. > > > > > > > > > 12. > > > > For the signalling identity descriptions: > > > > "Layer 2 VPNs using BGP signalling"; > > > > "Targeted LDP signalling."; > > > > "L2TP signalling."; > > > > > > [Med] Fixed. > > > > Okay. > > > > > > > > > > > > > 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; " > > > > Okay. > > > > > > > > > > > > > > > 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. > > > > > > > Okay, but at least for L2VPN services, a P2P connection (VPWS) is > > also quite common, no? > > [Med] Yeah, but that is an intrinsic characteristic of VPWS. Setting > "vpn-type" > to "vpws" is sufficient in such case. > > > > > > > > > > > 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. > > > > Okay. > > > > > > > > > > > > > 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. > > > > I think that it is useful to include those as informative > > references, or perhaps otherwise leave out these two identities. It > > feels strange to include them if they have no defined meaning. > > [Med] They are already listed as informative references. As I may not > advance the laziness argument again, I added the I-D references to the > module :-) > > > > > > > > > > > > > > 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. > > > > Okay. > > > > > > > > > > > > > > > 19. > > > > identity encapsulation-type { > > > > > > [Med] this one is about the encapsulation type. > > > > > > > vs identity tag-type > > > > Okay. > > > > > > > > [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 > > > > Okay. > > > > > > > > > 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). > > > > Okay. > > > > > > > > > > > > > 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. > > > > Okay. > > > > > > > > > > > > > > > > > 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". > > > > E.g., my comment was in the context of when vpn-id is used at the > > data type for port-id in the L3VPN model. > > [Med] OK, thanks. I think this is now fixed with the changes reported in the > L3NM. > > > > > > > > > > > > > > > > > > > > > > 23. > > > > What's the difference between service-timestamp > > > > > > [Med] This is about operational status. Updated to > > > oper-service-timestamp > > > > > > > Okay. > > > > > and service-status? > > > > > > [Med] This covers both admin and oper status. > > > > Okay. > > > > > > > > > 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". > > > > Okay. > > > > > > > > > > > > > 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. > > > > Okay. > > > > > > > > > > > > > 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. > > > > Okay. > > > > > > > > > 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. > > > > Thanks, > > Rob > > > > > > > > > > > > > > > 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
