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

[Yingzhen Qu <[e-mail]>]

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 <
[e-mail]> 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.
>
>
>
>