Re: [yang-doctors] [Lsr] Yangdoctors last call review of draft-ietf-isis-sr-yang-19

 Hi Yingzhen,
Please see inline.
    On Thursday, January 18, 2024, 04:49:28 PM EST, Yingzhen Qu <yingzhen.ietf@gmail.com> wrote:  

 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@ietf.org> 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.<Reshad> I looked at the changes in -20. That grouping is now gone and the (mt-)sid-binding-tlvs lists have no key, is that the intent?Also container mt-sid-binding-tlvs should be renamed to mt-sid-binding-tlv.

- 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. <Reshad> I agree that it's not easy but that's the case for most YANG documents at IETF. I've found yanglint to be a very useful tool for this. Up to the WG to decide.
Regards,Reshad.

Regards,
Reshad.

_______________________________________________
Lsr mailing list
Lsr@ietf.org
https://www.ietf.org/mailman/listinfo/lsr