| Hi Jan,
Thank you for the review, really appreciate. Please see my answers below inline.
Version -14 has been submitted including all the suggested changes.
Thanks, Yingzhen On Aug 17, 2022, at 8:49 AM, Jan Lindblad via Datatracker < noreply@xxxxxxxx> wrote:
Reviewer: Jan Lindblad
Review result: Ready with Issues
This is the YANG Doctor last call review of draft-ietf-isis-sr-yang-13. I think
this module is written in a nice, straight forward way. There are a few things
that have to be fixed before this can be published, however, so I will call it
ready with issues.
#1: Dependency on unpublished module
The ietf-isis-sr module under discussion here imports module ietf-isis. That
module is still not published as an RFC. For the review here I used the
ietf-isis.yang as defined in draft-ietf-isis-yang-isis-cfg-42. If the
ietf-isis.yang module changes before it is published, that could potentially
affect ietf-isis-sr.
A closely related observation is that the import statement lacks an import
reference. Maybe you could add a placeholder reference until you know what RFC
number to reference?
import ietf-isis {
prefix "isis";
}
[Yingzhen]: You’re right about the dependency here. We’ve been waiting for the publication Now they’re close to be published, so we requested the review of this document to get it ready. I added a place holder for the reference as you suggested.
#2: Inappropriate when expressions
The following when _expression_ appears 13 times in the module. There are two
problems with it.
when "/rt:routing/rt:control-plane-protocols/"+
"rt:control-plane-protocol/rt:type = 'isis:isis'" {
Direct equality comparison with identities is generally not recommended, as
that removes the possibility to "subclass" isis:isis into variants in the
future. The recommended approach is to use the XPath function
derived-from-or-self() instead. See below.
The other problem with this when _expression_ is worse. Since an unqualified
absolute path is used, the _expression_ becomes true for all
routing-plane-protocol instances as soon as there is at least one of isis type.
This enables the isis-sr extensions inside all routing-plane-protocol instances
as soon as there is at least one isis instance in the system. I don't believe
this was what the authors intended.
There are several possible ways to remedy the situation. The one I would
recommend is to use relative paths in the when _expression_ so that the path
never leaves the current control-plane instance. Since the relative path is a
little different depending on where it is used, I'll provide two examples. Just
let me know once you've updated all 13 of them, and I'll review.
// Line 503
augment "/rt:routing/" +
"rt:control-plane-protocols/rt:control-plane-protocol"+
"/isis:isis" {
//OLD when "/rt:routing/rt:control-plane-protocols/"+
//OLD "rt:control-plane-protocol/rt:type = 'isis:isis'" {
when "derived-from-or-self(../rt:type, 'isis:isis')" {
// Line 587
augment "/rt:routing/" +
"rt:control-plane-protocols/rt:control-plane-protocol"+
"/isis:isis/isis:interfaces/isis:interface" +
"/isis:adjacencies/isis:adjacency" {
//OLD when "/rt:routing/rt:control-plane-protocols/"+
//OLD "rt:control-plane-protocol/rt:type = 'isis:isis'" {
when "derived-from-or-self(../../../../../rt:type, 'isis:isis')" {
Basically, you add one ../ for each name in the augment path, starting at the
tail end working towards the beginning, until you hit the name
rt:control-plane-protocol (because it is the parent node of rt:type).
If you prefer, there are solutions using absolute paths too. In that case you
need to add filters ("predicates") for the control-plane-protocol keys to the
when _expression_ path. But IMO the method I explained above is probably easier
to follow.
[Yingzhen]: Thanks for pointing this out. I’ve changed the model to use relative path. Please kindly let me know if you see any issues.
#3: No mandatory, no default, no description
Some leafs in the module are not mandatory, have no default and no text in the
description that would explain how to interpret this ambiguous situation.
container ti-lfa {
if-feature ti-lfa;
leaf enable {
type boolean;
description
"Enables TI-LFA computation.";
}
...
leaf use-segment-routing-path {
if-feature remote-lfa-sr;
type boolean;
description
"force remote LFA to use segment routing
path instead of LDP path.";
}
The client may configure leaf enable to 'true' or 'false', in which case the
meaning of the configuration is clear. The client may also omit giving these
leafs any value. What happens then? You could explain what happens in the
description, e.g. "When left without a value, the system will bla bla...". Or
you could add a default statement, resolving any ambiguity, e.g. default false.
Or you could declare the leaf mandatory, in which case the client would be
forced to make a choice, i.e., mandatory true;
There are also many operational leafs that have this property, which makes it
easier for server implementors to get away with implementations that omit
returning some of these leafs. So if you think some of them are particularly
important for clients, consider marking them mandatory, so that servers
understand it's not ok to omit them.
[Yingzhen]: I added default value for these two leaves. #4: Indentation
The indentation of the module is apparently done by hand and a tad sloppy. By
running the following pyang command, you get a file with the indentation
cleaned up. It also removes unnecessary quotes and a few other things. In case
you want full control over your own module, you can just diff the two to find
places where the indentation might not be done properly.
pyang -f yang ietf-isis-sr\@2022-08-09.yang >
ietf-isis-sr\@2022-08-09-indented.yang
[Yingzhen]: fixed. #5: List keys not going first
The module contains four lists with keys. In three of them, the definitions of
the key element does not go first in the list. Here, for example, there is a
leaf af before leaf value (the key). This is perfectly valid YANG, but server
implementors MUST return the key first in the NETCONF payload. Therefore it is
customary to define the key(s) first inside the list (in the same order as in
the key statement), to adhere to the principle of least surprise.
list adjacency-sid {
key value;
config false;
leaf af {
type iana-rt-types:address-family;
description
"Address-family associated with the
segment ID";
}
leaf value {
…
[Yingzhen]: fixed as suggested. #6: Keyless lists
There are two keyless lists in the module. Keyless lists are allowed in YANG
for operational data. But that they are allowed does not necessarily mean they
are a good idea.
list global-block {
description "Segment Routing Global Block.";
leaf range-size {
type uint32;
description "The SID range.";
}
uses sid-sub-tlv;
}
...
list local-block {
description "Segment Routing Local Block.";
leaf range-size {
type uint32;
description "The SID range.";
}
uses sid-sub-tlv;
}
If there is no natural key for these lists and there aren't too many elements
in them in a typical system, then maybe leaving the list as keyless may be
fine. But if you can have hundreds or thousands of these list elements, keyless
lists are problematic. It's basically impossible for interested clients to read
anything else than the entire list every time when there is no key. There is no
way to identify list entries that have been added, removed or changed, so all
this data has to be read every time. If the data might be large, adding a key
(natural or synthetic) would improve performance.
[Yingzhen]: Thanks for the suggestion. However for these two specific lists, the number of elements are expected to be a few, or only one or two, so I suppose keyless is ok. #7: Unclear+open ended string format
The leaf fec has a string type and a pretty vague description. What is the
valid format for this leaf? Different implementors will probably allow
different formats here, rendering the module non-interoperable, despite efforts
to standardize.
leaf fec {
type string;
description
"IP (v4 or v6) range to be bound to SIDs.";
}
Worse, since this is a key, a client might specify multiple list entries with
different strings that map to the same underlaying range. For example, the YANG
model currently allows a client to configure all of these as distinct list
entries:
"2001:0000::47/50"
"2001:0::47/50"
"2001::47/50"
"::1.2.3.4"
"::1:2:3:4"
"0::1:2:3:4"
Sorry if I got the format wrong, I have to guess here :-) Hopefully you get my
point anyway. Changing to a more precise type would certainly be a good idea,
and particularly so for a key leaf.
[Yingzhen]: I looked at RFC 8667 and changed this to ip-prefix. Thanks for catching this. |
