Ho Rob, I updated the files to take into account your feedback:
* Fix input/output in the common I-D (diff): https://github.com/IETF-OPSAWG-WG/lxnm/commit/ff456448a9398311457eb51c1d78adc8210fd8f8 * L3NM (diff): https://github.com/IETF-OPSAWG-WG/lxnm/commit/ed48b938990e6e7e4be7e329d986592ebcdc0c37 * I-D (diff): https://tinyurl.com/l3nm-latest Please see inline for the context. Cheers, Med > -----Message d'origine----- > De : Rob Wilton (rwilton) [mailto:rwil...@cisco.com] > Envoyé : mardi 13 juillet 2021 14:52 > À : BOUCADAIR Mohamed INNOV/NET <mohamed.boucad...@orange.com>; > draft-ietf-opsawg-l3sm-l3nm....@ietf.org > Cc : opsawg@ietf.org > Objet : RE: AD review of draft-ietf-opsawg-l3sm-l3nm-09 > > Hi Med, > > Thanks for the quick reply. > > A few comments inline ... > > > > -----Original Message----- > > From: mohamed.boucad...@orange.com > > <mohamed.boucad...@orange.com> > > Sent: 12 July 2021 18:57 > > To: Rob Wilton (rwilton) <rwil...@cisco.com>; draft-ietf-opsawg- > l3sm- > > l3nm....@ietf.org > > Cc: opsawg@ietf.org > > Subject: RE: AD review of draft-ietf-opsawg-l3sm-l3nm-09 > > > > Re-, > > > > Many thanks for the review. A candidate version can be tracked at: > > https://tinyurl.com/l3nm-latest. > > > > Please see inline. > > > > Cheers, > > Med > > > > > -----Message d'origine----- > > > De : Rob Wilton (rwilton) [mailto:rwil...@cisco.com] Envoyé : > lundi > > > 12 juillet 2021 14:13 À : draft-ietf-opsawg-l3sm- > l3nm....@ietf.org > > > Cc : opsawg@ietf.org > > > Objet : AD review of draft-ietf-opsawg-l3sm-l3nm-09 > > > > > > Hi, > > > > > > Sorry for the delay in the review (it is a long draft). The > review > > > of the common l2vpn module will follow later today. > > > > > > I believe that this work will be really helpful for SPs > modelling > > > their networks and hence I would like to thank the authors, > > > shepherd, and WG for the effort that they have put into this > > > document. > > > > > > Most of my comments on this draft are either minor, or editorial > in > > > nature. I've listed the minor questions first, for which a > response > > > would be useful, and then the editorial/grammar suggestions are > at > > > the end, and in the attached copy of the draft (labelled with > #RW:). > > > > > > > > > 4. L3NM Reference Architecture > > > > > > The terminology from [RFC8309] is introduced to show the > > > distinction > > > between the customer service model, the service delivery > model, > > > the > > > network configuration model, and the device configuration > model. > > > In > > > that context, the "Domain Orchestration" and "Config > Manager" > > > roles > > > may be performed by "Controllers". > > > > > > 1. Service delivery model doesn't seem to be included in figure > 1, > > > > [Med] This is supposed to be covered by "network model" in the > figure. > > Updated the figure with an explicit mention. > > Ok. > > > > > > and doesn't appear to be referenced further. Does it need to be > > > mentioned at all? > > > > > > > > > 7.3. VPN Services > > > > > > The 'vpn-service' is the data structure that abstracts a > VPN > > > service > > > in the service provider network. Each 'vpn-service' is > uniquely > > > identified by an identifier: 'vpn-id'. Such 'vpn-id' is > only > > > meaningful locally within the network controller. The > subtree > > > of the > > > > > > 2. Why limit the vpn-id to the network controller? Presumably, > an > > > implementation could allow these identifiers to be unique within > the > > > SP's management network (e.g., perhaps by using a network > controller > > > specific prefix)? > > > > [Med] Agree. Made this change: s/meaningful locally (e.g., within > the > > network controller) > > Ok. > > > > > > > > > > > > 7.4. VPN Instance Profiles > > > > > > | | | +--rw vpn-policies > > > | | | +--rw import-policy? string > > > | | | +--rw export-policy? string > > > > > > 3. Is it right that import-policy and export-policy are plain > > > strings? > > > > [Med] That usage is correct as we define vpn-id in the common > draft as > > a generic identifier. That is why we mention "service identifier" > as an example. > > We can make that more clearer in the common I-D. > > Yes, making it clearer in the common I-D, will be helpful. > > > > > > > > > > > > > 7.6. VPN Network Access > > > > > > +--rw vpn-network-access* [id] > > > +--rw id vpn- > > > common:vpn-id > > > +--rw port-id? vpn- > > > common:vpn-id > > > > > > 4. I'm surprised that port-id is of type vpn-common:vpn:id > > > > [Med] We don't require any structure for the identification of > the > > port; hence the use of the string (vpn-common:vpn-id) type. > > Okay, my point is that the model isn't just saying that it is a > string (since it is using the type vpn-id) but is using a type with > more meaning assigned to it. I.e., I would expect that a vpn-id > should be the identifier for a VPN. If isn't one of these, then I > don't think that it should be a vpn-id. > [Med] Went with your proposal. Only maintained in few places where I think it makes sense. > > > > > > Also, is > > > the name port-id better than interface-id? E.g., if the > > > connectivity was not via a physical port? I think that the > > > description also defines this as an interface rather than a > port. > > > > [Med] The details about logical interfaces is covered in the > > connection and IP connection (e.g., lx-termination-point). > > Okay, if this always identifies a physical port, then the > description should presumably describe it in terms of port rather > than interface. > [Med] After thinking about this, I changed it to "interface-id' as we use it also to manage loopbacks. I updated the description to mention this is a physical or logical interface. I also checked the text we have about sub-interfaces. > > > > > > > > > > > > > 'multipoint': Represents a broadcast connection between > the > > > endpoints. The controller must keep the association > > > between a > > > logical or physical interface on the device with the > 'id' > > > of > > > the 'vpn-network-access'. > > > > > > 5. Is broadcast the right description here? > > > > [Med] Updated to: "Represents a multipoint connection between the > > customer site and the PEs". > > Okay. > > > > > > > > > > > > > YANG Model: > > > > > > identity port-id { > > > base bearer-inf-type; > > > description > > > "Identity for the priority-tagged interface."; > > > } > > > > > > 6. Is this description right? Is port-id automatically > priority- > > > tagged? > > > > [Med] What is meant here is defining an identity for priority- > tagged interface. > > Please note that we removed the bearer-inf-type as it is not used > in > > the module. > > Does this mean that this port-id identity has also been removed, or > just the base identity? > [Med] Yes, port-id was removed. > > > > > > > > > > > > identity lag-id { > > > base bearer-inf-type; > > > description > > > "Identity for the lag-tagged interface."; > > > } > > > > > > 7. Is "lag-tagged" right, should this be the just "Identity for > LAG > > > interface"? > > > > [Med] Yes, that's better. Please note that we removed the > > bearer-inf-type as it is not used in the module. > > > > > > > > > > > typedef area-address { > > > type string { > > > pattern '[0-9A-Fa-f]{2}(\.[0-9A-Fa-f]{4}){0,6}'; > > > } > > > description > > > "This type defines the area address format."; > > > } > > > > > > 8. This looks like hex can be entered in case insensitive way. > > > Is this value ever compared? Perhaps define a canonical form? > > > > [Med] Actually, we aligned with the device module > > draft-ietf-isis-yang-isis-cfg (currently in the RFC Editor Queue). > > Ack. Makes sense. > > > > > > > > > > > > > grouping vpn-instance-profile { > > > ... > > > leaf address-family { > > > type identityref { > > > base vpn-common:address-family; > > > } > > > description > > > "Indicates the address family (IPv4 or IPv6)."; > > > > > > 9. Should this be (IPv4 and/or IPv6)? I.e., is 'dual-stack' an > > > allowed identity here? > > > > [Med] It is. This is used in some examples: > > Ok. > > > > > == > > "address-family": [ > > { > > "address-family": "vpn-common:dual- > stack", == > > > > Updated to "(IPv4 and/or IPv6)". > > > > > > > > > > > leaf tag-type { > > > type identityref { > > > base vpn-common:tag-type; > > > } > > > default "vpn-common:c-s-vlan"; > > > description > > > "Tag type. By default, the tag type is > > > 'c-s-vlan'."; > > > > > > 10. What is meant by c-s-vlan? > > > Does this mean an outer S-VLAN with an inner C-VLAN? I would > > > normally expect this to be described as s-c-vlan (i.e., starting > > > with the outermost tag first). > > > > [Med] Yes. Updated to align with the candidate common module. > > Okay. > > > > > > > > > > container pseudowire { > > > description > > > "Includes pseudowire termination > > > parameters."; > > > leaf vcid { > > > type uint32; > > > description > > > "Indicates a PW or VC identifier."; > > > } > > > leaf far-end { > > > type union { > > > type uint32; > > > type inet:ip-address; > > > } > > > description > > > "Neighbor reference."; > > > > > > 11. What does it mean when a uint32 is used for the far end? > Should > > > the description cover this? > > > > [Med] That covers the case in > > https://datatracker.ietf.org/doc/html/rfc4447#section-5.2 > > Okay. Please can you update the description to make this clear, and > possibly add a reference statement. > [Med] Done. Thanks. > > > > > > > > > > > leaf address-allocation-type { > > > type identityref { > > > base address-allocation-type; > > > } > > > must "not(derived-from-or-self(current(), > " > > > + "'slaac') or derived-from-or- > > > self(current()," > > > + " 'provider-dhcp-slaac'))" { > > > error-message > > > "SLAAC is only applicable to IPv6."; > > > } > > > description > > > "Defines how addresses are allocated to > the > > > peer site. > > > > > > If there is no value for the address > > > allocation type, then IPv4 addressing > is > > > not > > > enabled."; > > > } > > > > > > 12. An alternative, possible cleaner approach, could have been > to > > > make both the ipv4 and ipv6 containers to have presence, and > then > > > make the address-allocation-type leaf to be mandatory. > > > > > > > [Med] Agree. We prefer to use this one for consistency with other > > checks in the module. > > Okay. > > > > > > > > > description > > > "Choice based on the DHCP service > > > type."; > > > case relay { > > > when "./dhcp-service-type = > 'relay'"; > > > > > > 13. It is slightly strange to have a case statement with a when > > > statement, normally I would expect to see a model use either of > > > these approaches rather than both. Perhaps the choice and case > > > statements are not required? > > > > [Med] We used a choice as it more directive about which data nodes > to > > include as a function of the type. > > But the when statement already achieves that without also needing > the choice and case statements. [Med] Fair. Fixed this one (and similar ones for consistency). > > > > > > > > > > > > > list address-pool { > > > key "pool-id"; > > > description > > > "Describes IP addresses to > be > > > allocated by DHCP. > > > > > > When only start-address > or > > > only > > > end-address is present, > it > > > represents a single > address. > > > > > > When both start-address > and > > > end-address are > specified, it > > > implies a range inclusive > of > > > both > > > addresses."; > > > > > > 14. Why not make start-address mandatory, and require that is > always > > > specified, e.g., for a single address. > > > > [Med] Agree. Fixed. > > Okay. > > > > > > > > > > > > > list address { > > > key "address-id"; > > > description > > > "Lists the IPv4 addresses that are > > > used."; > > > leaf address-id { > > > type string; > > > description > > > "An identifier of the static IPv4 > > > address."; > > > } > > > leaf customer-address { > > > type inet:ipv4-address; > > > description > > > "IPv4 address at the customer > side."; > > > } > > > > > > 15. I was slightly surprised that the customer-address isn't the > > > list key, that would allow the address-id to be optional extra > > > information rather than a list key. > > > > [Med] "id" is used to ease referring to an address in other places > > when appropriate (e.g.., acls). > > Okay. > > > > > > > > > > > > > leaf next-hop { > > > type union { > > > type inet:ip-address; > > > type predefined-next-hop; > > > } > > > description > > > "The next-hop that is to be used > for > > > the > > > static route. This may be > specified > > > as > > > an IP address, an interface, or a > > > pre-defined next-hop type (e.g., > > > discard or local-link)."; > > > } > > > > > > 16. The description states that an interface can be specified as > the > > > next hop. Is this correct? > > > > [Med] Good catch. Fixed. > > Okay. > > > > > > > > > > > > > leaf prepend-global-as { > > > type boolean; > > > default "false"; > > > description > > > "In some situations, the ASN that is > > > provided at the VPN node level may be > > > distinct from the one configured at > the > > > VPN network access level. When set to > > > 'true', this parameter prevents that > > > the ASN provided at the VPN node > > > level is also prepended to the BGP > > > route updates for this access."; > > > } > > > 17. Is this description correct? I.e., I don't understand the > > > "prevent that ..." part. > > > > [Med] Changed to: > > > > "In some situations, the ASN that is > > provided at the VPN node level may be > > distinct from the one configured at the > > VPN network access level. When such > > ASNs are provided, they are both > > prepended to the BGP route updates > > for this access. To disable that > > behavior, the prepend-global-as > > must be set to 'true'. In such a case, > > the ASN that is provided at > > the VPN node level is not prepended to > > the BGP route updates for this > access."; > > > > Better? > > I'm still confused by the fact that the leaf is called prepend- > global-as, and if you set it to true then "VPN node level is not > prepended" > [Med] Argh.. s/true/false. Always good to have fresh eyes. Thanks. > > > > > > > > > > > > leaf violate-action { > > > type enumeration { > > > enum warning { > > > description > > > "Only a warning message is sent > to > > > the peer when the limit is > > > exceeded."; > > > } > > > enum discard-extra-paths { > > > description > > > "Discards extra paths when the > > > limit is exceeded."; > > > } > > > enum restart { > > > description > > > "Restarts after a time > interval."; > > > > > > 18. What is meant by restarts? Should this have a more detailed > > > description? > > > > [Med] Updated to: "The BGP session restarts after a time > interval." > > Okay. > > > > > > > > > > > > > leaf inbound-rate-limit { > > > type decimal64 { > > > fraction-digits 5; > > > range "0..100"; > > > } > > > units "percent"; > > > description > > > "Specifies whether/how to rate-limit > the > > > inbound traffic matching this QoS > > > policy. > > > It is expressed as a percent of the > > > value > > > that is indicated in 'input- > > > bandwidth'."; > > > > > > 19. I note that inbound and outbound are used here, but > input/output > > > bandwidth were used previously. Is input and inbound applying > to > > > traffic flowing in the same direction? > > > > [Med] Yeah. Joe raised a similar comment, but we maintain "input" > to > > ease the mapping with the L3NM. This would ease building L3SM-bis > to > > make use of the common module if such bis is to be worked out in > the future. > > I would argue that it would be better for this model to be > internally consistent. > > Could this inbound-bandwidth and outbound-bandwidth, and in the > comment make it clear that they have the reverse sense to the L3SM > model? E.g., perhaps in a future version of the L3SM model the > existing names could be deprecated and use new ones with reverse > sense? > [Med] Fair. Fixed this in the L3NM and the common I-D. > > > > > > > > > > > container carrierscarrier { > > > > > > 20. Is carrierscarrier as one word normal, would carriers- > carrier, > > > be better as a leaf name? > > > > [Med] Agree. Fixed. > > Okay. > > > > > > > > > > > > 21. In the examples > > > "address-allocation-type": "static-address > > > "address-allocation-type": "ietf-l3vpn-ntw:static- > > > address Does static address need a namespace qualifier? Both > are > > > valid, but I would suggest using the simple form (and being > > > consistent in the examples). > > > > > > > [Med] Both are allowed as per RFC7951: > > > > "Otherwise, both the simple and namespace- > > qualified forms are permitted." > > > > But I agree it is better to be consistent. > > Okay. > > > > > > > > > 22. I note that you have some references in the YANG model, but > > > potentially, it might be worth looking to see if there are any > extra > > > references that could be added, although I will leave this to > your > > > discretion. > > > > > > > > > Finally, I have a few forms of mostly editorial suggestions. I > > > don't need to know whether you choose to incorporate these: > > > > > > (i) I have attached an annotated a version of the draft with > some > > > suggested editorial rewording of some descriptions in the YANG > model > > > (the questions above are also included and can be ignored. You > can > > > see these in the attached file with "#RW:". > > > > [Med] Considered almost all your embedded comments. > > Thanks. > > > > > > > > > (ii) I have also run a spelling/grammar tool over the draft > (which > > > doesn't currently check the YANG module). The warnings that it > > > flagged up are below (they may not all be correct). > > > > > > > [Med] Thanks. Addressed almost all of them. > > Thanks. > > 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
