Hi Reshad, Thanks for the follow-up review. > On Jan 13, 2024, at 15:30, Reshad Rahman via Datatracker <noreply@xxxxxxxx> wrote: > > Reviewer: Reshad Rahman > Review result: Ready with Issues > > Hi all, > > This is my YANG Doctor review of -28, I had previously reviewed -20. Thanks to > the authors for addressing my previous comments. There is 1 comment in my > initial review which concerns RFC9020, I am not convinced yet and may send > another email (or errata). > > Comments > ======== > > Should the title explicitly call out OSPFv2 and OSPFv3? The reason I’m asking > is because OSPF may imply v2 only, e.g. RFC8665 says “OSPF Extensions for > Segment Routing” but then the abstract says OSPFv2. While we haven’t been consistent, the base model (RFC 9129) uses simply OSPF to refer to both OSPFv2 and OSPFv3. > > Section 2. “OSPF base model[RFC9129]”, nit: add a space before the reference Sure. > > In the following, can there be overlaps? If not, this should be documented > (ideally should have been documented in RFC9020) > > +--rw srgb > | +--rw srgb* [lower-bound upper-bound] > | +--rw lower-bound uint32 > | +--rw upper-bound uint32 No overlaps but we this is a RFC 9020 issue. > > Section 2.1 (the YANG module) > > - In grouping ospfv2-prefix-sid-sub-tlvs, leaf-list flags should have a > reference? Same for v3. I have a reference at the grouping level. It doesn’t change for the flags. I’m hesitant to repeat it. > > - The grouping ospfv2-extended-prefix-range-tlvs has an ‘af’ address family > leaf which is a uint8, why not use address-family from RFC8294 with the > appropriate restrictions. But since this is OSPFv2 specific, is address family > still needed? For v3, I believe the af leaf is needed, although I’d rename it > to address-family and would use address-family enum from RFC8294. I’ll use the enum from RFC 8294. It shouldn’t be omitted for OSPFv2 since it is included in the ecodings. > > - The grouping ospfv2-extended-prefix-range-tlvs: should there be a range for > prefix-length? Same question, but but different range needed, for OSPFv3. No - this is not supported. I was never a big fan of the range functionality in the IGPs. > > - In list local-block-tlv, description of leaf range-size has “…The return of a > zero value”. Nit: change to “A value of zero…” Sure. > > - In container srms-preference-tlv, leaf preference. Nit: “with with 255”. Fixed. > > - Should leaf neighbor-id be mandatory? If not, what happens when it’s not set, > does it need a default value? I believe the description needs to indicate what > happens when it is set or not set. If you specify an unknown neighbor-id including invalid ID, it won’t be used. Specification is optional. > > - In ti-lfa container: the enable flag is not mandatory and does not have a > default value, you should add a default value or make it mandatory. Other > choice is a presence container. Ok - I defaulted it to false like the other LFA features in ietf-ospf.yang. I also changed it to “enabled” Consistent with ietf-ospf.yang. > > - In the selection-tie-breakers container, can both tie-breakers be enabled > simultaneously? Yes. I’ve updated the description to indicate this but am not going to attempt to describe the TI-LFA selection algorithm in the description. > > - For leaf use-segment-routing-path, the description has “…is in effect only > when remote-lfa is enabled”. I did not see any remote-lfa leaf node, not sure > if this is referring to a feature. I think the description needs to be modified > and a reference would be very helpful here. The reference would be the base mode container which this is augmenting. I don’t know that adding a reference makes sense unless you’re going to add a reference to every augmentation. > > Appendix A. There is only 1 (simple) example and it covers v2 only. Please add > a v3 example also, ideally with more config data than the current example e.g. > with the neighbor-id (since that augment is in this document). Having an > operational state example for OSPFv2 and OSPFv3 would also really be helpful. I > realize examples can be painful... We’ll take this under advisement but it won’t be -29. Examples are easier if you have implementations. Thanks, Acee > > Regards, > > Reshad. > > > -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
