Re-, Please see inline. Cheers, Med > -----Message d'origine----- > De : Martin Bjorklund [mailto:mbj@xxxxxxxxxx] > Envoyé : mardi 23 octobre 2018 10:05 > À : BOUCADAIR Mohamed TGI/OLN > Cc : yang-doctors@xxxxxxxx; softwires@xxxxxxxx; draft-ietf-softwire- > yang.all@xxxxxxxx; ietf@xxxxxxxx > Objet : Re: Yangdoctors last call review of draft-ietf-softwire-yang-06 > > Hi, > > <mohamed.boucadair@xxxxxxxxxx> wrote: > > Hi Martin, > > > > Thank you for the review. > > > > We released a new revision -08 to address your comments. We will > > double check the formatting and issue another iteration if needed. > > Thank's for addressing my comments. See inline and at the end for > some new comments on -08. > > > > Please see inline. > > > > Cheers, > > Med > > > > > -----Message d'origine----- > > > De : Martin Björklund [mailto:mbj@xxxxxxxxxx] > > > Envoyé : lundi 15 octobre 2018 11:00 > > > À : yang-doctors@xxxxxxxx > > > Cc : softwires@xxxxxxxx; draft-ietf-softwire-yang.all@xxxxxxxx; > ietf@xxxxxxxx > > > Objet : Yangdoctors last call review of draft-ietf-softwire-yang-06 > > > > > > Reviewer: Martin Björklund > > > Review result: Ready with Issues > > > > > > This is my YANG doctor review of draft-ietf-softwire-yang-06. The > > > review focuses on YANG-specifics only, since I am not familiar with > > > the technology that is modelled. > > > > > > o I would like to see all Tom Petch's comments in his three replies > > > to the IETF LC for this document addressed. Especially his comment > > > about using ianatf:tunnel. > > > > > > > [Med] This is fixed in -07. A new tunnel-type parameter is defined > > to handle this. > > I think that the addition of identities for various tunnel types that > derive from ift:tunnel is the right thing to do. [Med] OK, thanks > > However, since these identities derive from ift:tunnel, the > augmentation of ietf-interfaces in ietf-interface-tunnel is not > needed. [Med] ietf-interface-tunnel tries to preserve a similar structure as in https://www.iana.org/assignments/ianaiftype-mib/ianaiftype-mib, but you are right we can get rid of it. Instead, the new identities should be used as if:type > directly. For example, instead of: > > <interface xmlns:ianaift="urn:ietf:params:xml:ns:yang:iana-if-type"> > <name>lw4o6-wan</name> > <type>ianaift:tunnel</type> > <tunnel-type > xmlns:iana-tunnel-type="urn:ietf:params:xml:ns:yang:iana-tunnel-type"> > iana-tunnel-type:aplusp > </tunnel-type> > ... > </interface> > > you should do: > > <interface> > xmlns:iana-tunnel-type="urn:ietf:params:xml:ns:yang:iana-tunnel-type"> > <name>lw4o6-wan</name> > <type>iana-tunnel-type:aplusp</type> > ... > </interface> > > So, I think you should remove the ietf-tunnel-type module. > [Med] OK. > > An additional comment on the identities in iana-tunnel-type; for each > identity, I think you should add a "reference" statement that points > to the RFC(s) that define the tunnel. (available in the IANA registry > at > https://www.iana.org/assignments/smi-numbers/smi-numbers.xhtml#smi-numbers-5) > [Med] I guess you meant https://www.iana.org/assignments/smi-numbers/smi-numbers.xhtml#smi-numbers-6. Fixed in my local copy, except form IPHTTPS for which we don't have an RFC. > > > > > o The term "instance" is used to mean - I think - the "device". > > > > [Med] It is used to mean function rather than device. A device may > > enable multiple instances of the same function. > > But you have for example in the description of "binding-mode": > > This feature indicates that the instance functions as a binding > based softwire instance. > > And in container algo-instances you have: > > The instances advertise the MAP-E/MAP-T > feature through the capability exchange mechanism > when a NETCONF session is established." > > Unless your intentation is that one "instance" == one "function" == > one NETCONF server, then this text is not correct. > > So I am a bit confused - if the device advertises the feature > "binding-mode" it means that "it functions as a binding based softwire > instance". Maybe you mean something along the lines of > > This feature indicates that the network element can function as > one or more binding based softwire instances. > [Med] This is it. Updates when appropriate. > (I don't know if you want to use the term "network element" or > something else.) > [Med] Network element is fine. > > Also there is similar text for the features map-e and map-t. > > [Med] Yes. > Anyway, if this meaning of the word "instance" is defined somewhere, > I suggest you add a reference to that other doc; or else explain this > meaning in 1.1. > [Med] added to the terminology section: A network element may support one or multiple instances of a softwire mechanism; each of these instances may have its own configuration and parameters. > > > I > > > didn't find this term in the RFCs 7596, 7597 or 7599. I suggest > > > you use some other term, since "instance" is quite generic, and is > > > often used to refer to instances of YANG data nodes. > > > > > > Also, does the term "instance" mean the same thing in > > > "algo-instance"? And in "br-instances"? "bind-instance"? > > > > > > > > [Med] algo-instance means an instance of type > > algorithm. br-instances denotes a set of br instances, and > > bind-instance means an instance of type binding. > > I could guess that. I think the issue is when the word "instance" is > used unqualified. [Med] Updated to avoid unqualified "instances" > > > > o In ietf-softwire-common: > > > > > > grouping algorithm-instance { > > > description > > > "Indicates that the instance supports the MAP-E and MAP-T > > > function. The instance advertises the MAP-E/MAP-T feature > > > through the capability exchange mechanism when a NETCONF > > > session is established."; > > > > > > This description does not seem right. A grouping can't indicate > > > anything. Also, what is "the MAP-E/MAP-T feature"? > > > > > > > [Med] Fixed. > > > > > > > > o In ietf-softwire-ce: > > > > > > A similar description is found here: > > > > > > container algo-instances { > > > description > > > "Indicates that the instances supports the MAP-E and MAP-T > > > function. The instances advertise the MAP-E/MAP-T > > > feature through the capability exchange mechanism > > > when a NETCONF session is established."; > > > > > > same comments apply. > > > > > > > [Med] Fixed. > > This text is still present, with a minor change: > > container algo-instances { > description > "Indicates that the instances supports the MAP-E and/or MAP-T > function. The instances advertise the MAP-E/MAP-T > feature through the capability exchange mechanism > when a NETCONF session is established."; > > But since the container "algo-instances" is a non-presence container, > it can't "indicate" anything. This text needs to be rephrased. > [Med] Updated accordingly. > > > > o In ietf-softwire-common: > > > > > > container algo-versioning { > > > description "algorithm's version"; > > > leaf version { > > > type uint64; > > > description "Incremental version number for the algorithm"; > > > } > > > leaf date { > > > type yang:date-and-time; > > > description "Timestamp to the algorithm"; > > > } > > > } > > > > > > Maybe these descriptions are crystal clear to someone who knows the > > > technology. If so, perhaps add a reference to help the rest of us? > > > > > > > [Med] This is used for logging purposes. A reference to RFC7422 is added. > > Ok. I still don't really understand how it is supposed to be used. > > When you write "version number for the algorithm", do you mean > "version number for this algo-instance"? [Med] What is meant is: "Incremental version number for the mapping algorithm rules provided to the algorithm instance"; An algorithm instance may be provided with mapping rules that may change in time (for example, increase the size of the port set). When an abuse party presents an external IP address/port, the version of the algorithm is important because depending on the version, a distinct customer may be identified. The timestamp is used as a key to find the appropriate algorithm that was put into effect when an abuse occurred. Updated the description among these lines. > > These are config true leafs; should they be config false and > internally managed by the server? [Med] This can be generated by the server or set by an operator. If not, I suppose an operator can > set them to any suitable values? If so, what does "incremental > version number" really mean? Is the server supposed to reject a value > that is not "incremental"? [Med] What is important is to have a unique version number, how is set is not important. Incremental seems to be straightforward, but one may envisage other ways to manage versions. I deleted "incremental". > > [...] > > > > Also, it seems each "instance" has a numerical id (the key), but > > > also a name (a string). Maybe there are protocol-reasons to do > > > this, but if not, did you consider using the "name" as key instead? > > > > > > > [Med] id/name is inspired from how NAT instances are managed (see > > RFC7659). The name is optional. > > Well, in MIBs instances are normally identified with integers b/c of > how the protocol (SNMP) works. In YANG modules, we usually use a > "name" that is a string to identify instances. With a string, the > operator can choose meaningful names, and use them in other leafrefs, > instead of having to remember how the names are mapped to integers. > > (Compare ifIndex (MIB) w/ interface/name (YANG)) > > So I suggest you use "name" as key. [Med] If you think this is better, I'm fine with that. > > > > o I also note that you have changed the name of some lists in -08, > e.g., list "bind-instance" is now list "bind-instances" > (plural). Another example is: > > +--rw algo-instances > +--rw algo-instances* [id] > > I think you should change these back to singluar; that's what YANG > modules typically use. > > [Med] Actually, this was a comment from Tom. Perhaps, we misunderstood it. OK to change it back if this is the recommended practice. > > > /martin
