Hi Med, Catching up with email, sorry for the delay, please see further comments inline ...
> -----Original Message----- > From: [email protected] > <[email protected]> > Sent: 05 April 2022 11:40 > To: Rob Wilton (rwilton) <[email protected]>; draft-ietf-opsawg- > [email protected] > Cc: [email protected] > Subject: RE: AD review of draft-ietf-opsawg-l2nm-12 (2n Part) > > 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) <[email protected]> > > Envoyé : jeudi 17 mars 2022 21:53 > > À : [email protected] > > Cc : [email protected] > > 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. I think that have figured this out. My understanding is: 1) 1 or more profiles can be defined globally with particular parameters. 2) Each VPN service can activate a subset of those global profiles, overriding parameters if they wish (need to check defaults). 3) Each vpn-node may reference one of the active "global-parameters-profiles". Is this correct? I will note that the model doesn't allow a single global profile to create two slightly different vpn-node profiles based on the same global profile (since the names match at all levels). Possibly this is a good thing to avoid any possible confusion, but I wanted to ensure that you had noted it. I still think that clarifying some of this text might be helpful in a couple of ways: (i) In 7.4, possibly tweak the text something like: OLD 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 values. The subtree of 'global- PROPOSED: The 'global-parameters-profile' defines reusable parameters for the same L2VPN service instance ('vpn-service'). Global parameters profiles are defined at the VPN service level, activated and at the VPN node level, and then an activated VPN profile may be used at the VPN network access level. 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 values. The subtree of 'global- (ii) Within the vpn-node, I was initially confused because I thought that "global-parameter-profiles" was a leafref to a global parameter, not an activated parameter. I wonder whether changing this name to "active-vpn-node-profile" would make the hierarchical relationship clearer? > > > > (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. Okay. > > > > > (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). Okay. > > > > > (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. Okay. > > > > > (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. Okay. > > > > > (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. I still think that this could be more explicit (I remember some implementations getting this wrong when Ethernet PWs were first implemented - they would signal and negotiate the L3 MTU not the L2 one - even for an L2 service ...) : 'svc-mtu': Is the service MTU for an L2VPN service (i.e., layer 2 MTU including L2 frame header/tail). It is also known as the maximum transmission unit or maximum frame size. When a frame is larger than the service MTU, it is fragmented to accommodate the MTU of the network. Also, it is always the case that it will be fragmented? I thought that often frames would just be dropped at the PE if they exceeded the L2 MTU for the service? > > > > > (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. Where is the warning message sent? Is this just a syslog message to the service provider, or is this signalled to the customer? If it the first then "Log a warning message as the MAC action" may ne clearer. > > > > > (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. At a YANG language level, I'm not convinced this works. To take an example, if you look at mac-policies: | +--rw mac-policies | | +--rw mac-addr-limit | | | +--rw limit-number? uint16 | | | +--rw time-interval? uint32 | | | +--rw action? identityref | | +--rw mac-loop-prevention | | +--rw window? uint32 | | +--rw frequency? uint32 | | +--rw retry-timer? uint32 | | +--rw protection-type? identityref Then the leaf mac-policies/mac-addr-limit/limit-number is always in scope and has a default value assigned. So, even if the global policy "foo" sets limit-number to 200, and this "foo" policy is activated for a given VPN Node then even if limit-number isn't explicitly set under active-global-parameters-profiles the default value for limit-number is in scope and hence would always override the value in the global scope. I think that the best solution here is to not have any defaults in any the leaves under the parameters-profile grouping. Where that grouping is used to define the top level global parameters then you can make use of refine statements under the "uses parameters-profile" to add default values back in only at the top level. The alternative choice is to take the "default" statements out, and put the default behaviour in the description instead, making it clear that the defaults only apply at the top level profiles. > > > > > > > (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). Okay. I'm not sure the last time I saw an interface running at 10 Mbps. :-) > > > > > (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. Okay. Thanks, Rob > > idr: > https://mailarchive.ietf.org/arch/msg/idr/2Lqt2OvVOAVbP3awcl13q6aU8_U/ > bess: > https://mailarchive.ietf.org/arch/msg/bess/yv3iC3qRxcxbSkbqbQD9DWPDW > NA/ (from Joe) + > https://mailarchive.ietf.org/arch/msg/bess/fR6O4zSOWhIKMxlJEiawcrmwAU > U/ (from Adrian) > pals: > https://mailarchive.ietf.org/arch/msg/pals/jD82yF5AWwMCgEySMofZPvaKCa > g/ (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 [email protected] https://www.ietf.org/mailman/listinfo/opsawg
