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 <[email protected]> > 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 of the base ISIS model, which has a dependency of https://datatracker.ietf.org/doc/draft-ietf-bfd-rfc9127-bis/ <https://datatracker.ietf.org/doc/draft-ietf-bfd-rfc9127-bis/>. 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. > ## > > >
_______________________________________________ Lsr mailing list [email protected] https://www.ietf.org/mailman/listinfo/lsr
