HI Reshad,
Thanks for the review. I've uploaded version -20 to address your comments. Details below inline.
Thanks,
Yingzhen
On Sat, Jan 13, 2024 at 4:24 PM Reshad Rahman via Datatracker <noreply@xxxxxxxx> wrote:
Reviewer: Reshad Rahman
Review result: Ready with Issues
Hi all,
This is my YANG Doctor review of -19. Thanks to the authors for making the
effort to align with draft-ietf-ospf-yang (as previously requested).
Comments
========
Section 3 (YANG Tree)
- There are a few instances of perfix-sid-flags, should bd prefix-sid-flags
[Yingzhen]: fixed.
- For the list below, can there be overlaps between different entries? If not,
this should be documented (ideally should have been documented in RFC9020)
+--rw protocol-srgb {sr-mpls:protocol-srgb}?
+--rw srgb* [lower-bound upper-bound]
+--rw lower-bound uint32
+--rw upper-bound uint32
[Yingzhen]: There should not be overlaps. Agreed with you, this should have been documented in RFC 9020.
YANG Model
- The identities such as r-bit, n-bit etc should all have a reference (not just
the document but also the section)
[Yingzhen]: I added reference to each base identity.
- All those identities are called x-bit but the description says flag. Which
terminology is typically used in IS-IS?
[Yingzhen]: changed to -flag.
- Leaf Sid, how do we know whether it’s a 32-bit SID or a 20-bit label (not
sure if we need to know)?
[Yingzhen]: Same as ospf-sr-mpls.yang, added length, and updated sid description.
- For global-blocks and local-blocks, a reference would help.
[Yingzhen]: Done.
- In grouping sid-sub-tlv, is the container sid-sub-tlv needed?
[Yingzhen]: A container would help to identify the boundary of a TLV. In an ISIS LSP, there are multiple TLVs and sub-tlvs.
- In grouping srlb , can there be an overlap of the advertised local blocks?
Need some enhanced description here iMO. Same question for sr-capability and
global blocks.
[Yingzhen]: please see the above answers.
- In list global-block, why not add a key? I’m assuming the sid would be
unique? Same comment for local-block.
[Yingzhen]: SRGB is defined in RFC 9020, which is common for both OSPF and ISIS. Here, we're defining protocol specific SRGB, and the key is defined in the grouping in the ietf-segment-routing-common.yang. SRLB is defined in RFC 9020, which applies to all protocols.
- In grouping srms-preference, leaf preference, no need for the range 0..255
since it is a uint8.
[Yingzhen]: fixed.
- Nit: a few instances of “This group …”, it should be “This grouping …”
[Yingzhen]: fixed.
- Leaf ‘af’, nit/suggestion: I would rename to address-family.
[Yingzhen] : Done.
- In grouping segment-routing-binding-tlv, leaf range, is that description
accurate?
[Yingzhen]: Thanks for catching this. I've updated the description.
- Augment with neighbor-system-id, should the leaf node be mandatory?
[Yingzhen]: added "mandatory true". Also did the same for ospf.
- Container selection-tie-breakers, can both protection types be enabled
simultaneously?
[Yingzhen]: yes, it's possible to enable both of them, one or none.
There should be an appendix with a fairly complete config example and also an
operational state example.
[Yingzhen]: Operational state is not easy to do with the IGP database since we don't have an implementation yet.
Regards,
Reshad.
-- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
