Hi Renato, Thanks for the feedback. Please see my response below inline.
Thanks, Yingzhen On Mon, Apr 21, 2025 at 12:07 PM Renato Westphal <[email protected]> wrote: > Hi Yingzhen and Acee, > > I'm glad I could be of help, and thanks for incorporating my suggestions :) > > I've updated Holo to implement the latest version of the SR-MPLS > modules. Here are some more examples of JSON data for IS-IS and OSPF: > * IS-IS: https://textbin.net/raw/jfpbj1axgm > * OSPFv2: https://textbin.net/raw/ksuukedldr > * OSPFv3: https://textbin.net/raw/sbj5pgaxf1 > > [Yingzhen]: these examples actually have LS database. I'm kind of wondering whether we should include them in an appendix. Thoughts? While updating the code, I noticed a few minor inconsistencies that I > thought were worth sharing: > * In the OSPF module, SIDs are always represented using a > "label-value"/"index-value" choice. However, in some cases there's an > additional "length" leaf, and in others, there isn't. > * In the IS-IS module, SIDs are represented either as a simple "sid" > leaf or as a "label-value"/"index-value" choice combined with a > "length" field. > [Yingzhen]: Thanks for catching this. I looked at RFC8667 and fixed the is-is model to be consistent with OSPF. > * In the IS-IS module, the "adjacency" list is augmented with a list > of local Adjacency-SIDs ("adjacency-sid"). In the OSPF module, the > "neighbor" list doesn't have a similar augmentation. Adjacency-SID > information is only available in the LSDB. > [Yingzhen]: do you mean the following in IS-IS? augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol/isis:isis/isis:interfaces/isis:interface/isis:adjacencies/isis:adjacency: +--ro adjacency-sid* [] +--ro value? uint32 +--ro address-family? iana-rt-types:address-family +--ro weight? uint8 +--ro protection-requested? boolean This is an augmentation of operational state. You're right, we don't have this in OSPF. I looked at RFC9129, this can be added to ospf:interface/ospf:neighbors/ospf:neighbor. Unfortunately we have different names in RFC9129 and 9130, neighbors vs. adjacencies. > > Best Regards, > Renato. > > Em sex., 18 de abr. de 2025 às 09:15, Acee Lindem > <[email protected]> escreveu: > > > > Hi Renato, > > > > Thanks so much for your implementation. All the reviews are also useful > but this is invaluable. > > > > Yingzhen has incorporated your comments in -28. > > > > > On Apr 16, 2025, at 8:17 PM, Renato Westphal <[email protected]> > wrote: > > > > > > Hi Acee, > > > > > > Thank you for your quick response. > > > > > > Em seg., 14 de abr. de 2025 às 12:06, Acee Lindem > > > <[email protected]> escreveu: > > >> > > >> Hi Renato, > > >> > > >>> On Apr 13, 2025, at 11:50 AM, Renato Westphal < > [email protected]> wrote: > > >>> > > >>> Hi all, > > >>> > > >>> I'm implementing this draft and would like to share some feedback. > > >>> > > >>> There's an augmentation to the "router-capabilities" container that > is > > >>> wrong. Instead, it is the "router-capability" list within that > > >>> container that should be augmented. > > >> > > >> Mahesh noted this as well and it is fixed in GitHub. > > > > > > That might have been a different issue. I just checked the latest -27 > > > version and the augmentation to "router-capabilities" is still > > > incorrect. > > > > Right - that was a different problem with a redundant level of > hierarchy. This is in -28. > > > > > > > > As I progress with my SR implementation, I came across another issue > > > related to augmentations. The module is currently augmenting the > > > "extended-is-neighbor" and "mt-is-neighbor" containers, instead of > > > augmenting the "instance" list within those containers. I believe it > > > would be better to have the Adj-SID sub-TLVs directly associated with > > > their respective IS reachability entries, rather than grouping all > > > Adj-SID sub-TLVs for each "neighbor-id" together (in the case of > > > parallel links, there might be multiple IS reachability entries for > > > the same neighbor). > > > > I agree - this is where all the feature specific neighbor extended > attributes belong. This is in -28. > > > > > > > > > > I also suggest renaming the "sid-list" list to "adj-sid-sub-tlv" for > > > consistency with the existing "prefix-sid-sub-tlv" list and to align > > > with the OSPF SR module. It might also be a good idea to group all > > > Adj-SID sub-TLVs inside an "adj-sid-sub-tlvs" container for > > > consistency as well. > > > > > > Good suggestion - this is in -28. > > > > > > > > Other than these minor issues, I think the module looks excellent. > > > > > > FWIW, I'm sharing [1] an example data file populated from a virtual > > > IS-IS SR topology using the Holo routing stack. It's a partial > > > implementation of this module, including fixes for the aforementioned > > > issues. > > > > > > [1] > https://gist.githubusercontent.com/rwestphal/aa85a402e7ed12f6da131b1a0e172c93/raw/dfe4cdbbac4abd21828e5dc9a9a60e5a5d51f9c1/ietf-isis-sr-mpls.json > > > > > > Cool!!! > > > > Thanks, > > Acee > > > > > > > > > > > > > > Best Regards, > > > -- > > > Renato Westphal > > > > > -- > Renato Westphal > > _______________________________________________ > Lsr mailing list -- [email protected] > To unsubscribe send an email to [email protected] >
_______________________________________________ Lsr mailing list -- [email protected] To unsubscribe send an email to [email protected]
