Hi Rob, Focusing on the first part of your review, except point (9).
The changes can be tracked at: https://github.com/IETF-OPSAWG-WG/lxnm/commit/337f65012f55e71df4105481bc28fe53ac8bb302, while the full changes made so far can be tracked at: https://tinyurl.com/l2nm-latest Please see inline for more context. Cheers, Med > -----Message d'origine----- > De : Rob Wilton (rwilton) <rwil...@cisco.com> > Envoyé : jeudi 17 mars 2022 21:53 > À : draft-ietf-opsawg-l2nm....@ietf.org > Cc : opsawg@ietf.org > Objet : AD review of draft-ietf-opsawg-l2nm-12 > > Hi, > > Apologies for the delay, but I have now managed my AD review of draft- > ietf-opsawg-l2nm-12. (Also attached in case my email is truncated ...) > > I would like to thank the authors, WG for their work on this document, > and the shepherd for his review. I found the document to be well > written and pretty straightforward to follow and understand. I also > believe that this document is a useful addition to the YANG and Network > Management Ecosystem, to thank you for that. > > Anyway, here my comments. I think that they mostly pretty minor, but > perhaps a few that my need a bit more thought. Hopefully they will help > improve the doc. > > --- > > Moderate comments: > > (1) > The VPN network access is comprised of: > > 'id': Includes an identifier of the VPN network access. > > 'description': Includes a textual description of the VPN > network > access. > > 'interface-id': Indicates the interface on which the VPN > network > access is bound. > > 'global-parameters-profile': Provides a pointer to an active > 'global-parameters-profile' at the VPN node level. > Referencing an > active 'global-parameters-profile' implies that all > associated > data nodes will be inherited by the VPN network access. > However, > some of the inherited data nodes (e.g., ACL policies) can > be > overridden at the VPN network access level. In such case, > adjusted values take precedence over inherited ones. > > It wasn't entirely clear to me how this works with the global parameters > defined at the VPN network access level and the VPN node level work. Is > this meant to be a 3 tier hierarchy, or is it always only 2 tiers? Are > you allowed to reference different global profiles both at the VPN > network access level and the VPN node level? Possibly, some slightly > expanded description here may be helpful (and/or in the YANG module). > > [Med] Isn't this covered by this text: The 'global-parameters-profile' defines reusable parameters for the same L2VPN service instance ('vpn-service'). Global parameters profile are defined at the VPN service level and then called at the VPN node and VPN network access levels. Each VPN instance profile is identified by 'profile-id'. Some of the data nodes can be adjusted at the VPN node or VPN network access levels. These adjusted values take precedence over the global ones. The subtree of 'global- parameters-profile' is depicted in Figure 7. > (2) | +--rw encapsulation > | | +--rw encap-type? identityref > | | +--rw dot1q > | | | +--rw tag-type? identityref > | | | +--rw cvlan-id? uint16 > > Did you consider adding support for ranges or sets of VLAN Ids (e.g., a > list of non-overlapping ranges) (both for the single and double tagged > cases)? > [Med] We didn't. We went for the same structure as we have in RFC9182. > > (3) | +--rw lag-interface > > I'm slightly surprised that you don't have parameters for the physical > interfaces, and I can understand your justification for this, but then > you do have configuration for LAG, including configuration for the > underlying member interfaces. This feels slightly inconsistent to me at > the level that the configuration is being provided. Understanding the > rationale a bit more here might be helpful, and I think that we should > double check that this should definitely be in this model. > [Med] We spent many cycles on this one (see the full list of related issues at https://github.com/IETF-OPSAWG-WG/lxnm/issues?q=lag). There was an agreement that LAG-related details are generic and can be defined outside the L2NM. However, we included some of this information because we need LACP configuration for the ESI auto-assignment mode based on LACP configuration (https://github.com/IETF-OPSAWG-WG/lxnm/issues/219). > > (4) +--rw svc-pe-to-ce-bandwidth > | {vpn-common:inbound-bw}? > | +--rw pe-to-ce-bandwidth* [bw-type] > > Can you specify different bandwidths for multiple CoS fields? It looks > like the list key is the bw-type, and hence you would only be able to > specify the bandwidth for a single CoS field? Is that sufficient? > [Med] It isn't :-( Updated the structure to fix this + submitted an errata for RFC8466 to report the issue. > > (5) 8.3. Ethernet Segments > > The names used here don't seem to be particularly friendly. Is giving > them more human understandable identity names an option, or are these > names directly mirroring some other registry? Or perhaps even something > like esi-type-1-lacp, esi-type-3-mac, etc? > [Med] These are taken directly form the type values in Section 5 of RFC7432, but the comment is fair. Updated the names to be more human understandable. > > (6) leaf svc-mtu { > type uint32; > description > "Service MTU, it is also known as the maximum transmission > unit or maximum frame size. When a frame is larger than > the MTU, it is fragmented to accommodate the MTU of the > network."; > } > > MTU's turn up in various places. Given that MTUs seem to mean different > things for different people, please can you check the descriptions and > given that this model is for L2VPN's be super explicit about whether > these MTUs are representing the L3 payload, or the max frame size > including the L2 header and presumably FCS sequence as well. > [Med] This is about the max frame including L2 header. Made this change: OLD: 'svc-mtu': Is the service MTU for an L2VPN service. It is also known as the maximum transmission unit or maximum frame size. When a frame is larger than the MTU, it is fragmented to accommodate the MTU of the network. NEW: 'svc-mtu': Is the service MTU for an L2VPN service (i.e., Layer 2 MTU). It is also known as the maximum transmission unit or maximum frame size. When a frame is larger than the MTU, it is fragmented to accommodate the MTU of the network. > > (7) identity mac-action { > description > "Base identity for a MAC action."; > } > > Would an VPN implementation ever want to drop and log rather than just > doing one or the other? > [Med] This is about sending a warning message. This was inherited form RFC8466: ... SPs may impose a maximum number of MAC addresses learned from the customer for a single service instance by using the "mac-addr-limit" leaf and may use the "action" leaf to specify the action taken when the upper limit is exceeded: drop the packet, flood the packet, or simply send a warning log message. I think "log" is confusing. I removed it from the L2NM. > > (8) Hierarchical groupings and defaults > > I want to check what the model/plan is regarding hierarchical config > (e.g., grouping parameters-profile) and default values. It looks like > some of the leaves in the provide have default values, which I believe > will be ambiguous when it comes to hierarchical config. I.e., normally > I would never expect that a leaf with a default value defined would fall > back to the hierarchical policy because logically the leaf always has a > value if it is in scope. One solution to this problem (that is a bit > more verbose), would be to take the defaults out of the leaves in the > grouping, and then add them back in using "refine" them using refine > statements when the global parameters-profile is used. Another choice > would be to not express these using the YANG "default" keyword at all, > and instead describe the defaults in the description. > > Generally, defining defaults where we can is probably useful, but we > need to be careful with any hierarchical config/policies. [Med] Good point. However, for the particular case of parameters-profile, the intent is that all these parameters are factorized at the service level. Values that have to be overridden at lower levels, will be explicitly called and then take precedence. > > > (9) Various comment related to handling VLAN tag rewrites: > ... > > > (10) > leaf speed { > type uint32; > units "mbps"; > default "10"; > description > "LACP speed."; > } > > 10 Mbps seems like a slow default LACP speed. Does this need a default > at all, Ethernet interfaces would generally negotiate to the fastest > supported speed. The same comment applies to the port speed. > [Med] The default was inherited from the service model (L2SM, RFC8466). > > (11) > I did wonder whether it is okay to include the BGP and PW types as part > of this draft? Personally, since they will be controlled by IANA > anyway, and this is where they are being used I think that is okay, [Med] Idem. I don' see any issue there. but > I was wondering whether IDR/BESS had been consulted on this at all, it > might be worth checking with them that they are okay. > [Med] IDR, BESS, and PALS were solicited during the WGLC, but not specifically on this part. Adrian contacted the Chairs of PALS WG about an issue on the PW types specifically. idr: https://mailarchive.ietf.org/arch/msg/idr/2Lqt2OvVOAVbP3awcl13q6aU8_U/ bess: https://mailarchive.ietf.org/arch/msg/bess/yv3iC3qRxcxbSkbqbQD9DWPDWNA/ (from Joe) + https://mailarchive.ietf.org/arch/msg/bess/fR6O4zSOWhIKMxlJEiawcrmwAUU/ (from Adrian) pals: https://mailarchive.ietf.org/arch/msg/pals/jD82yF5AWwMCgEySMofZPvaKCag/ (from Adrian) _________________________________________________________________________________________________________________________ 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
