Hi Med, Thanks, just ack'ing that I agree with your proposed fixes, and sorry for getting the terminology mixed up.
Thanks, Rob > -----Original Message----- > From: mohamed.boucad...@orange.com > <mohamed.boucad...@orange.com> > Sent: 28 April 2022 12:27 > To: Rob Wilton (rwilton) <rwil...@cisco.com>; draft-ietf-opsawg- > l2nm....@ietf.org > Cc: opsawg@ietf.org > Subject: RE: AD review of draft-ietf-opsawg-l2nm-12 (2n Part) > > Hi Rob, > > Thank you for the follow-up. > > Please see inline. > > Cheers, > Med > > > -----Message d'origine----- > > De : Rob Wilton (rwilton) <rwil...@cisco.com> > > Envoyé : mercredi 27 avril 2022 14:38 > > À : BOUCADAIR Mohamed INNOV/NET > <mohamed.boucad...@orange.com>; > > draft-ietf-opsawg-l2nm....@ietf.org > > Cc : opsawg@ietf.org > > 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: mohamed.boucad...@orange.com > > > <mohamed.boucad...@orange.com> > > > Sent: 05 April 2022 11:40 > > > To: Rob Wilton (rwilton) <rwil...@cisco.com>; draft-ietf-opsawg- > > > l2nm....@ietf.org > > > Cc: opsawg@ietf.org > > > 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) <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. > > > > 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 OPSAWG@ietf.org https://www.ietf.org/mailman/listinfo/opsawg
