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
<[email protected]> wrote:
> Re-,
>
> Please see inline.
>
> Cheers,
> Med
>
> > -----Message d'origine-----
> > De : Martin Bjorklund [mailto:[email protected]]
> > Envoyé : mardi 23 octobre 2018 10:05
> > À : BOUCADAIR Mohamed TGI/OLN
> > Cc : [email protected]; [email protected]; draft-ietf-softwire-
> > [email protected]; [email protected]
> > Objet : Re: Yangdoctors last call review of draft-ietf-softwire-yang-06
> >
> > Hi,
> >
> > <[email protected]> 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:[email protected]]
> > > > Envoyé : lundi 15 octobre 2018 11:00
> > > > À : [email protected]
> > > > Cc : [email protected]; [email protected];
> > [email protected]
> > > > 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
>
_______________________________________________
Softwires mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/softwires
