Hi Acee,
On Wednesday, January 17, 2024, 12:42:48 PM EST, Acee Lindem <acee.ietf@xxxxxxxxx> wrote:
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.
<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.
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.
<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.
>
> - 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.
>
> - 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.
>
> - 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.
>
>
>
Thanks,
Acee
>
> Regards,
>
> Reshad.
>
>
>
-- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
