[Lsr] Yangdoctors last call review of draft-ietf-isis-sr-yang-19
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
- 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-bounduint32
+--rw upper-bounduint32
YANG Model
- The identities such as r-bit, n-bit etc should all have a reference (not just
the document but also the section)
- All those identities are called x-bit but the description says flag. Which
terminology is typically used in IS-IS?
- 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)?
- For global-blocks and local-blocks, a reference would help.
- In grouping sid-sub-tlv, is the container sid-sub-tlv needed?
- 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.
- In list global-block, why not add a key? I’m assuming the sid would be
unique? Same comment for local-block.
- In grouping srms-preference, leaf preference, no need for the range 0..255
since it is a uint8.
- Nit: a few instances of “This group …”, it should be “This grouping …”
- Leaf ‘af’, nit/suggestion: I would rename to address-family.
- In grouping segment-routing-binding-tlv, leaf range, is that description
accurate?
- Augment with neighbor-system-id, should the leaf node be mandatory?
- Container selection-tie-breakers, can both protection types be enabled
simultaneously?
There should be an appendix with a fairly complete config example and also an
operational state example.
Regards,
Reshad.
___
Lsr mailing list
Lsr@ietf.org
https://www.ietf.org/mailman/listinfo/lsr
[Lsr] Yangdoctors last call review of draft-ietf-ospf-sr-yang-28
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. Section 2. “OSPF base model[RFC9129]”, nit: add a space before the reference 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-bounduint32 | +--rw upper-bounduint32 Section 2.1 (the YANG module) - In grouping ospfv2-prefix-sid-sub-tlvs, leaf-list flags should have a reference? Same for v3. - 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. - The grouping ospfv2-extended-prefix-range-tlvs: should there be a range for prefix-length? Same question, but but different range needed, for OSPFv3. - In list local-block-tlv, description of leaf range-size has “…The return of a zero value”. Nit: change to “A value of zero…” - In container srms-preference-tlv, leaf preference. Nit: “with with 255”. - 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. - 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. - In the selection-tie-breakers container, can both tie-breakers be enabled simultaneously? - 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. 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... Regards, Reshad. ___ Lsr mailing list Lsr@ietf.org https://www.ietf.org/mailman/listinfo/lsr
[Lsr] Yangdoctors early review of draft-ietf-ospf-sr-yang-20
Reviewer: Reshad Rahman Review result: Ready with Issues Main comments = - Please add lots of references in the YANG. There are many (most/all?) nodes e.g. m-bit, protocol-srgb etc etc, and probably types, which need references to the relevant sections in corresponding RFCs. - The document needs a least 1, probably more, examples. - The abstract mentions OSPF, the overview mentions OSPFv2 only and the YANG module has OSPFv2 and OSPFv3. - sr-algorithm should be a leafref to an algorithm somewhere, right now it's a uint8. - No need to redefine uint24, it's already define in RFC8294. - Leaf preference, no need for range 0..255 since it's a uint8 - Looking at RFC9020, I see that container segment-routing, in grouping sr-control-plane, is non-presence and leaf receive has default value true, meaning receive is true by default even if the container hasn’t been created. Is that the intention? And is it the desired behaviour in OSPF? If no, you can add a refine statement. My guess though is that is a mistake in RFC9020. Questions = - extended-prefix-range-tlvs is used only for ospfv2. Is that on purpose. Since there's an "af" node in the grouping, I assumed it'd be used for ospfv3 also. BTW 'af' is defined as uint8, there's an address-family type in RFC6991 and that should be used instead. - Is uint16 sufficient for range-size? - I see augment ospf:ospfv2 when rt:type = ospfv2, is the when-stmt needed? The ospfv2 container should only exist for ospfv2? - Leaf sid is a uint32. In SR models, is the Sid always represented as a uint32 or is there a type defined. And should it be a choice for 20-bit label v/s 32-bit SID. - List lan-adj-sid-sub-tlv in container lan-adj-sid-sub-tlvs, no need for a key? Regards, Reshad. ___ Lsr mailing list Lsr@ietf.org https://www.ietf.org/mailman/listinfo/lsr
