Hi Mohamed, I've addressed the non-DISCUSS level comments in -36. I've incorporated most but not all of your comments.
> > > > ---------------------------------------------------------------------- > COMMENT: > ---------------------------------------------------------------------- > > # Abstract: “configure” is covered by “manage” (remember the FCAPS functions) > > OLD: > This document defines a YANG data module that can be used to > configure and manage OSPF Extensions for Segment Routing for the MPLS > data plane. > > NEW : > This document defines a YANG data module that can be used to > manage OSPF Extensions for Segment Routing (SR) for the MPLS > data plane. > > # Introduction: Simplify + remove “configure” as this is covered by “manage” > > OLD: > This document defines a YANG data model [RFC7950] that can be used to > configure and manage OSPFv2 extensions for Segment Routing [RFC8665] > and OSPFv3 extensions for Segment Routing [RFC8666] for the MPLS data > plane. It is an augmentation to the OSPF YANG data model [RFC9129]. > > NEW: > This document defines a YANG data model [RFC7950] that can be used to > manage Segment Routing (SR) for OSPFv2 [RFC8665] > and OSPFv3 [RFC8666] for the MPLS data > plane. It is an augmentation to the OSPF YANG data model [RFC9129]. Fixed for both. > > # Section 2 > > ## (redundant) I would delete the following as this was stated few lines > above. > > CURRENT: > It is an augmentation of the OSPF base model. > > ## The module is not only for configuration, but for state retrieval. The > document says that it adheres to the NMDA ;-) > > Please update the following accordingly: > > CURRENT: > The OSPF SR YANG module requires support for the base segment routing > module [RFC9020], which defines the global segment routing > configuration independent of any specific routing protocol > configuration, and support of OSPF base model [RFC9129] which defines > basic OSPF configuration and state. Fixed to a separate section with narrative text. > > ## Long tree diagram > > Please move the full tree to an appendix but consider snippets to help readers > go through the model. > > Refer to Section 3.4 of 8407bis for more guidance. Done. > > # Section 2.1: Check references > > CURRENT: > [RFC2328], [RFC4750], [RFC4915], [RFC5340], [RFC5643], [RFC5838], > [RFC6991], [RFC8102], [RFC8294], [RFC8343], [RFC8476], [RFC8349], > [RFC9587], and [I-D.ietf-rtgwg-segment-routing-ti-lfa] are referenced > in the YANG data model. > > For example, I failed to find where these ones are cited [RFC4750], > [RFC5643], > [RFC5838], [RFC8343 ], [RFC8476]. Fixed. > > # Section 2.2 > > ## Please use the template at > https://datatracker.ietf.org/doc/html/draft-ietf-netmod-rfc8407bis-22#name-template-for-ietf-modules > > ## Description issues > > CURRENT: > This YANG module defines the generic configuration > and operational state for OSPF Segment Routing (SR), which > is common across all of the vendor implementations. It is > intended that the module will be extended by vendors to > define vendor-specific OSPF Segment Routing configuration > and operational parameters for the MPLS data plane. > > * How you checked this part “is common across all of the vendor > implementations”? > > * I would remove the last sentence. It is weird to make assumption about what > proprietary extensions in a STD spec! Ok. > > ## Consistent referencing style: The module include a mix of styles for the > reference statements. Please update accordingly. For example, > > OLD: "RFC 6991 - Common YANG Data Types"; > NEW: "RFC 6991: Common YANG Data Types"; Fixed > > ## Lack of references: > > For example, consider adding the following for all prefix-sid-flag > > NEW: > reference > "RFC 8665: OSPF Extensions for Segment Routing, Section 5 > RFC 8666: OSPFv3 Extensions for Segment Routing, Section 6"; This module already had more references than most but I've added more. > > ## Some description are copied from the base spec, while the statement does > not > make sense for a given type. > > For example, the following is included for an identity, but 'if set, ..' does > not make sense for an identity. This is not a flag bit. An example is provided > below (but other similar constructs are included in the module) > > OLD: > "Inter-Area flag. If set, advertisement is of > inter-area type."; > NEW: > "Inter-Area flag."; Fixed. > > ## Lack of mapping between some identities and the flag names as defined in > the > base spec. For example, the description for vi-flag should indicate that this > corresponds to the V-Flag as there is no VI-Flag in 8665/8666: This is due to pyang not being able to disambiguate the duplicate identities with different base identities. > > OLD: > "Value/Index flag."; > > NEW: > "Value/Index flag. Corresponds to the Adj-SID V-Flag."; > reference > "RFC 8665: OSPF Extensions for Segment Routing, Section 6.1 > RFC 8666: OSPFv3 Extensions for Segment Routing, Section 7.1"; > > Idem, please indicate that lo-flag corresponds to the L-Flag defined in > 8665/8666 as there is no LO-Flag in 8665/8666. > > OLD: > "Local/Global flag."; > > NEW: > "Local/Global flag. Corresponds to the Adj-SID L-Flag."; > reference > "RFC 8665: OSPF Extensions for Segment Routing, Section 6.1 > RFC 8666: OSPFv3 Extensions for Segment Routing, Section 7.1"; Fixed. > > ## No need to repeat the parent node prefix. There are many occurrences of > this > in the module. I'm going to leave these as is. In these cases, the list elements are TLVs. > > OLD: list prefix-sid-sub-tlv { > NEW: list prefix-sid { > > OLD: list extended-prefix-range-tlv { > NEW: list extended-prefix-range { > > OLD: container extended-prefix-range-flags { > NEW: container flags { > > Etc. > > ## Use Singular for list/leaf-list per rfc8407bis, for example: > > OLD: leaf-list flags { > NEW: leaf-list flag { > > There are many occurrences in the text. Fixed. > > ## Defaults: Please provide an authoritative reference where the default is > defined + indicate the meaning of this default. > > For example, it is not clear for the definition the meaning of this value: > > CURRENT: > leaf priority { > type uint8; > default "128"; In the context of the container, the default should be obvious. > > ## Restrict using range: restrict the range rather than having this in the > description. > > CURRENT: > leaf mt-id { > type uint8; > description > "Multi-topology ID. Topologies range from 0-127 and > return of any other value would indicate an error."; > > # Section 3: Security template > > Please follow the template defined in > https://datatracker.ietf.org/doc/html/draft-ietf-netmod-rfc8407bis-22#name-security-considerations-sect Fixed > > # Section 5: Indicate whether this is maintained by IANA or not > > OLD: > name: ietf-ospf-sr-mpls > namespace: urn:ietf:params:xml:ns:yang:ietf-ospf-sr-mpls > prefix: ospf-sr-mpls > reference: RFC XXXX > > NEW: > name: ietf-ospf-sr-mpls > namespace: urn:ietf:params:xml:ns:yang:ietf-ospf-sr-mpls > prefix: ospf-sr-mpls > maintained by IANA? N > reference: RFC XXXX > > # Section 6: Issues with classifying references > > RFC6241, RFC6242, RFC8040, RFC8343, RFC8446, and RFC8476 are cited as > normative, while they shouldn’t. Please move those to be listed as > Informative. Fixed. RFC8343 and RFC8476 were removed. > > # Appendix > > ## Redundant examples > > Do we really need to have the xml example given that the JSON encoding for the > same example is provided? I'd be glad to only have json. Leaving in -36 > > ## Broken JSON example > > Please validate the example using yanglint, etc. We'll discuss among the co-authors but I believe this was validated. It looks like all you did was replace the module prefixes with the module names. Thanks Acee > > OLD: > { > "routing": { > "router-id": "1.1.1.1", > "control-plane-protocols": { > "control-plane-protocol": { > "type": "ospf:ospfv2", > "name": "OSPFv2", > "ospf": { > "areas": { > "area": { > "area-id": "0.0.0.0", > "interfaces": { > "interface": { > "name": "eth0", > "segment-routing": { > "adjacency-sid": { > "adj-sids": { > "value": 3888 > } > } > } > } > } > } > }, > "segment-routing": { > "enabled": true > }, > "protocol-srgb": { > "srgb": { > "lower-bound": 4000, > "upper-bound": 5000 > } > } > } > } > } > } > } > > NEW: > { > "ietf-routing:routing": { > "router-id": "1.1.1.1", > "control-plane-protocols": { > "control-plane-protocol": { > "type": "ietf-ospf:ospfv2", > "name": "OSPFv2", > "ietf-ospf:ospf": { > "areas": { > "area": { > "area-id": "0.0.0.0", > "interfaces": { > "interface": { > "name": "eth0", > "ietf-ospf-sr-mpls:segment-routing": { > "adjacency-sid": { > "adj-sids": { > "value": 3888 > } > } > } > } > } > } > }, > "ietf-ospf-sr-mpls:segment-routing": { > "enabled": true > }, > "ietf-ospf-sr-mpls:protocol-srgb": { > "srgb": { > "lower-bound": 4000, > "upper-bound": 5000 > } > } > } > } > } > } > } > > Cheers, > Med > > > _______________________________________________ Lsr mailing list -- lsr@ietf.org To unsubscribe send an email to lsr-le...@ietf.org
