Hi Martin, FWIW, a new revision which implements the last edits we discussed is available online: https://tools.ietf.org/html/draft-ietf-softwire-yang-12. Also, this version fixes the "id" thing. Cheers, Med > -----Message d'origine----- > De : mohamed.boucadair@xxxxxxxxxx [mailto:mohamed.boucadair@xxxxxxxxxx] > Envoyé : mercredi 24 octobre 2018 08:51 > À : Martin Bjorklund > 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 Martin, > > Please see inline. > > Cheers, > Med > > > -----Message d'origine----- > > De : Martin Bjorklund [mailto:mbj@xxxxxxxxxx] > > Envoyé : mercredi 24 octobre 2018 08:32 > > À : 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: > > > Re-, > > > > > > Fixed the first two ones in my local copy. > > > > > > id is optional. I'm maintaining it because it is already used by some > > > implementations. > > > > Ok, but these implementations propably need to change anyway since the > > "id" is no longer the key. > > [Med] Yes. > > > > > I will just point out that having one key and another integer based > > identifier that doesn't serve any purpose looks a bit odd. But if the > > WG thinks that it is needed then that's fine (although in this case I > > suggest you add some text to the descriptions of these leafs that > > explain what the purpose is). > > > > > > I had a closer look at the iana-tunnel-type module and the > > instructions to IANA, and I think that you could make some minor > > clarificiations: > > > > In the module description, you have: > > > > This module contains a collection of YANG data types defined > > by IANA and used for tunnel types. > > > > perhaps write: > > > > This module contains a collection of YANG identities defined > > by IANA and used as interface types for tunnel interfaces. > > > > [Med] Works for me. > > > And in the IANA Considerations section you have: > > > > > > "base": Contains the value of the tunnel type in lowercase. > > > > maybe instead > > > > "base": Contains the string "ift:tunnel". > > > > > > > > [Med] That text is referring to the table in > https://www.iana.org/assignments/smi-numbers/smi-numbers.xhtml#smi-numbers-6. > > We can change the text for better clarity to: > > "base": Contains the name assigned to the tunnel type, in lowercase. > > > > > /martin > > > > > > > > > > > > Thank you again for the review. Much appreciated! > > > > > > Cheers, > > > Med > > > > > > > -----Message d'origine----- > > > > De : Martin Bjorklund [mailto:mbj@xxxxxxxxxx] > > > > Envoyé : mardi 23 octobre 2018 15:30 > > > > À : 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, > > > > > > > > Thanks for the quick update! > > > > > > > > I have looked at -11, and have just a few minor comments. > > > > > > > > o Section 5.1 > > > > > > > > Maybe the tree diagram needs to be re-generated; at least: > > > > > > > > | +--rw bind-instance* [id] > > > > > > > > should be > > > > > > > > | +--rw bind-instance* [name] > > > > > > > > > > > > > > > > o Section 8 > > > > > > > > > > > > leaf softwire-num-max { > > > > type uint32; > > > > must ". >= 1"; > > > > > > > > This should be: > > > > > > > > leaf softwire-num-max { > > > > type uint32 { > > > > range "1..max"; > > > > } > > > > > > > > > > > > o Section 8 > > > > > > > > Since you now have "name" as key in the lists, is the leaf "id" > > > > still needed? > > > > > > > > > > > > > > > > > > > > /martin > > > > > > > > > > > > > > > > <mohamed.boucadair@xxxxxxxxxx> wrote: > > > > > 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 > > > > > > > >
