Hi Rob, Thank you for the follow-up.
Please see inline. Cheers, Med > -----Message d'origine----- > De : Rob Wilton (rwilton) <[email protected]> > Envoyé : mercredi 27 avril 2022 14:38 > À : BOUCADAIR Mohamed INNOV/NET <[email protected]>; > [email protected] > Cc : [email protected] > Objet : RE: AD review of draft-ietf-opsawg-l2nm-12 (2n Part) > > 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. [Med] These are defined for a specific service, not reused between services. > 2) Each VPN service can activate a subset of those global > profiles, overriding parameters if they wish (need to check > defaults). [Med] Yes but with s/VPN service/vpn-node. > 3) Each vpn-node may reference one of the active "global- > parameters-profiles". [Med] Yes but with s/vpn-node/vpn-network-access. > > 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. [Med] Yes, that is the intent. > > 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- > [Med] Looks good to me. I went with a version with very minor edits. Thanks. > (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? > [Med] Made the change under the vpn-network-access. I hope the naming will be less confusing. > > > > > > > > (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. [Med] Looks good to me. Fixed. 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? > [Med] This text is confusing. The initial intent was to draw the behavior at the service site. This text is better suited by the a service model, not L2NM. Removed this. > > > > > > > > > (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. > > [Med] This is about sending an alarm to an internal server (syslog, for example). That warning may trigger a notification to the customer using a variety of means that are out of scope. Went with your proposed wording. > > > > > > > > (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. Med: Yes, you are right. > > 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. > Med: Went with the description approach. Thanks. > > > > > > > > > > > (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/2Lqt2OvVOAVbP3awcl13q6aU > 8_U/ > > bess: > > > https://mailarchive.ietf.org/arch/msg/bess/yv3iC3qRxcxbSkbqbQD9DWP > DW > > NA/ (from Joe) + > > > https://mailarchive.ietf.org/arch/msg/bess/fR6O4zSOWhIKMxlJEiawcrm > wAU > > U/ (from Adrian) > > pals: > > > https://mailarchive.ietf.org/arch/msg/pals/jD82yF5AWwMCgEySMofZPva > KCa > > 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
