RE: Yangdoctors last call review of draft-ietf-softwire-yang-06

[<[e-mail]>]

Hi Martin,

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Martin Bjorklund [mailto:[e-mail]]
> Envoyé : mercredi 24 octobre 2018 08:32
> À : BOUCADAIR Mohamed TGI/OLN
> Cc : [e-mail]; [e-mail]; draft-ietf-softwire-
> [e-mail]; [e-mail]
> Objet : Re: Yangdoctors last call review of draft-ietf-softwire-yang-06
> 
> Hi,
> 
> <[e-mail]> 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:[e-mail]]
> > > Envoyé : mardi 23 octobre 2018 15:30
> > > À : BOUCADAIR Mohamed TGI/OLN
> > > Cc : [e-mail]; [e-mail]; draft-ietf-softwire-
> > > [e-mail]; [e-mail]
> > > 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
> > >
> > >
> > >
> > > <[e-mail]> wrote:
> > > > Re-,
> > > >
> > > > Please see inline.
> > > >
> > > > Cheers,
> > > > Med
> > > >
> > > > > -----Message d'origine-----
> > > > > De : Martin Bjorklund [mailto:[e-mail]]
> > > > > Envoyé : mardi 23 octobre 2018 10:05
> > > > > À : BOUCADAIR Mohamed TGI/OLN
> > > > > Cc : [e-mail]; [e-mail]; draft-ietf-softwire-
> > > > > [e-mail]; [e-mail]
> > > > > Objet : Re: Yangdoctors last call review of
> > > > > draft-ietf-softwire-yang-06
> > > > >
> > > > > Hi,
> > > > >
> > > > > <[e-mail]> 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:[e-mail]]
> > > > > > > Envoyé : lundi 15 octobre 2018 11:00
> > > > > > > À : [e-mail]
> > > > > > > Cc : [e-mail]; [e-mail];
> > > > > [e-mail]
> > > > > > > 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
> > > >
> >