Hi Jan
Thanks again for your thorough review and comments. We have uploaded version -03 of the draft that addresses your comments. Please see Inline
[TS] for the resolution action taken.
Please let us know if you have any further comments.
From: Jan Lindblad <janl@xxxxxxxxxx>
Date: Monday, November 5, 2018 at 4:20 AM
To: Tarek Saad <tsaad@xxxxxxxxx>
Cc: "yang-doctors@xxxxxxxx" <yang-doctors@xxxxxxxx>, "draft-ietf-teas-yang-te-types.all@xxxxxxxx" <draft-ietf-teas-yang-te-types.all@xxxxxxxx>, "ietf@xxxxxxxx" <ietf@xxxxxxxx>, "teas@xxxxxxxx" <teas@xxxxxxxx>
Subject: Re: Yangdoctors early review of draft-ietf-teas-yang-te-types-01
Resent-From: <alias-bounces@xxxxxxxx>
Resent-To: Tarek Saad <tsaad@xxxxxxxxx>, <rgandhi@xxxxxxxxx>, <xufeng.liu.ietf@xxxxxxxxx>, <vbeeram@xxxxxxxxxxx>, <igor.bryskin@xxxxxxxxxx>, <lberger@xxxxxxxx>, <mhartley.ietf@xxxxxxxxx>, <martin.vigoureux@xxxxxxxxx>, <db3546@xxxxxxx>, <aretana.ietf@xxxxxxxxx>,
Lou Berger <lberger@xxxxxxxx>
Resent-Date: Monday, November 5, 2018 at 4:19 AM
Tarek,
See my comments below [janl].
#1: There are many locations in the YANG talking about an "ERO subobject index"
(and once RRO index, record route subobject). What is this, and how is it
supposed to be used? The document is silent on this matter, and I have seen
modules with problems around numeric index leafs much like this earlier. Are
these numbers stable, i.e. remains the same forever?
[TS]: The list of explicit route objects defines a TE path (e.g. hops), where the index is used as a key to identify a specific entry in the list. A lower index implies the TE path traverses
this entry earlier. We will clarify the description and remove mention of “subobject” in the next update to the document.
[janl] Very good re description changes.
I'm a little worried that numeric indices will end up in the same quagmire+impasse where ACLs have been the last decades. Isn't there a risk that people will want to insert entries between two consecutive indices? Is "renumber" an operation
that someone might ask for on these indices?
[TS]: The list is ordered by the key (index).. We’ve added a note in the description that entries with lower index are visited first by the path. It is understandable that the list may
have to be reconfigured if an entry can not be fit in between two existing ones. Note, a change in the configured ERO list is generally handled as non-destructive event to an existing/provisioned tunnel (e.g. handled as make-before-break).
leaf index {
type uint32;
description
"Route object entry index. A lower index indicates
path traverses the hop earlier than the higher index
hop(s)";
#2: There are few leafs (5) with default values given, and none with mandatory.
Probably needs to increase before we get to last call.
[TS]: In general, the team tried to avoid setting defaults unless it is strictly dictated by another RFC/standard. The team will review again to see if any was missed and will take action.
[janl] That's all fine. Just remember to describe what happens when a leaf has no value every time there is no default or mandatory. Easy to forget, experience shows. Missing this causes non-interoperability in the standard.
[TS]: the team has opted to assign defaults to all optional leafs in the latest revision of the draft.
#3: There are several choices in the module that are meant to be augmented with
additional cases. In many instances, this is explicitly spelled out, very good.
If this is meant to happen in all choices, it would be nice to point this out
in every instance. Also, if there are any specific assumptions or
considerations to keep in mind when augmenting in a new technology, please note
that in the description as well.
[TS]: yes, we will update as necessary.
What is the format of this binary? If this is always a 4-byte binary, wouldn't
a numeric type be more user friendly, e.g. uint32?
[TS]: the standard defines admin group as 4-octet binary (9 - Administrative group (4 octets) ), and for extended-admin-group as series of those. We are also defining a union to encompass both
as admin-groups below. Do you still have concerns on this?
type extended-admin-group;
description "TE administrative group derived type";
[janl] I'm have two concerns. The first is that I don't know what the format of the type is. How do I know how to construct one of these values, or interpret an existing one? Is this a randomly chosen value? The other concern is how an
operator would enter this value. The binary type is not the most user friendly of types. A numeric representation in the management interface might make better sense.
[TS]: RFC3630 and RFC5305 defines admin-groups (aka affinity colors) as binary flags that are user specified (e.g. bit-position X is for RED, and bit-position Y is for GREEN). So, binary
type would be closest to this representation. On other hand, RFC7591 states that a binary value is represented as a JSON string -- base64 encoding (https://tools.ietf.org/html/rfc7951#section-6.6).
Let us know if there are still concerns on this.
#5: identifier in container with optional leafs
grouping te-topology-identifier {
The name suggests this is used as an identifier, but all the leafs are
optional. This is not typical. They are also in a container, precluding them
from being used as list keys. Is that as intended?
[TS]: this container is not meant to be reused for a list, rather contains ID(s) that together can be used to lookup a specific topology that the TE tunnel is using. We can clarify this in the
description
[janl] Hmm, ok. Still not sure what happens if you omit some value(s), but I guess I should wait to see your clarification.
[TS]: we have addressed this by specifying defaults for the optional leafs.
#6: Optional -id leafs again
Leafs that appear to be used as identifiers are optional
[TS]: such IDs identify a specific node, link, or address in the network but in our applications we do not to use them as keys in such lists. For example list explicit-route-objects defines a
separate index leaf to be used as key -- since, a path may contain the same node-id or address repeated multiple times (looping path)..
[janl] So what happens if you omit specifying some of them?
[TS]: This was reworked and added a mandatory to leafs that are must, and added defaults to optional leafs.
#7: binary length in bits?
It appears to me the modeler might have thought the length is given in bits.
The value of length is in bytes, however.
[TS]: good catch, thanks. We'll correct it to indicate number of octets as RFC6020 defines.
#8: Must _expression_ copy paste
must "not(../label-end/te-label/direction) or "
+ "not(te-label/direction) "
+ "or ../label-end/te-label/direction = te-label/direction" {
This must _expression_ appears to have been copied from label-start. In any case,
it always evaluates to true and has no effect.
[TS]: thanks, we can see the bug in the _expression_ as is, and this will be fixed/updated.
must "not(../label-start/te-label/direction) or "
+ "not(te-label/direction) "
+ "or ../label-start/te-label/direction = te-label/direction" {
[janl] This revised must statement is superfluous. The must statement on container label-start already covers this case.
[TS]: addressed
"When there are gaps between label-start and label-end,
this attribute is used to specify the positions
Need more information on how to interpret this leaf. Which bits map to what,
and what does the bit field values 0 and 1 indicate?
[TS]: ]: Each bit-position in the bitmap will map to an offset from the label-start. For example, if label-start=16000 and bitmap=0x11, then labels={16000, 16004} are relevant. We will clarify
this in the next update.
[janl] Very good. Since you are using type binary and refer to the contents using integers, you should also specify how you map those integer values to bit positions in the binary. E.g. big-endian?
[TS]: We have clarified this in the description of the leaf that it is big-endian representation
#10: Canonical representation
The type is based on a string with a pattern allowing hex characters and an
upper or lowercase P. Since the pattern allows multiple representations of the
same underlaying value (0x1p10 presumably means the same as 0x1p0xa and
0x1P0XA) the question comes up if there is a canonical representation of this
value, e.g. using all lowercase and all hex, or if the string must be
remembered exactly as given by the client. The description could answer this
[TS]: We will update the description to indicate more strict canonical form (all upper case).
#11: Mix of upper and lowercase
The module specifies many enumeration and identity values. Some are all
lowercase. Some are all uppercase. The principle of least astonishment suggests
to pick one and stick with it. YANG recommendations suggest to use all
typedef te-link-direction {
typedef te-label-direction {
identity LSP_METRIC_TYPE {
identity LSP_METRIC_RELATIVE {
identity LSP_METRIC_ABSOLUTE {
identity LSP_METRIC_INHERITED {
[TS]: we will update to all lower-case to align with YANG recommendations.
|
