Re: [yang-doctors] Yangdoctors last call review of draft-ietf-ospf-sr-yang-28

[Reshad Rahman <[e-mail]>]

 Hi Acee,

    On Wednesday, January 17, 2024, 12:42:48 PM EST, Acee Lindem <[e-mail]> wrote:  
 
 Hi Reshad, 

Thanks for the follow-up review. 

> On Jan 13, 2024, at 15:30, Reshad Rahman via Datatracker <[e-mail]> 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. 

<RR> Ok.

> 
> 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. 

<RR> Ok. This is probably obvious to SR experts, but not to others.
> 
> 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. 

<RR> Ok.

> 
> - 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. 

<RR> Ok.

> 
> - 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. 
<RR> Ok.

> 
> - 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.
<RR> Typo in latest: neighorr

> 
> - 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.  
<RR> Ok.


> 
> - 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. 

<RR> Ok.

> 
> - 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.

<RR> I missed the fact that it was referring to the parent container it's containing. This leaf node is conditional on remote-lfa-sr feature, it is a child of remote-lfa so I don't understand the point
of the comment "…is in effect only when remote-lfa is enabled”, it actually threw me off.
> 
> 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. 
<RR> Yanglint...
Regards,Reshad.

Thanks,
Acee




> 
> Regards,
> 
> Reshad.
> 
> 
>