Mohamed Boucadair has entered the following ballot position for draft-ietf-ospf-sr-yang-34: Discuss
When responding, please keep the subject line intact and reply to all email addresses included in the To and CC lines. (Feel free to cut this introductory paragraph, however.) Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ for more information about how to handle DISCUSS and COMMENT positions. The document, along with other ballot positions, can be found here: https://datatracker.ietf.org/doc/draft-ietf-ospf-sr-yang/ ---------------------------------------------------------------------- DISCUSS: ---------------------------------------------------------------------- Hi Yingzhen, Acee, Jeffrey, and Helen, Thank you for the effort put into this specification, especially for your perseverance to progress the spec for almost ten years. I hope we could produce device YANG modules quicker. The augmentations are overall adhering to the base specifications defined in RFC8665/RFC8666. There are minor issues (but many, though) that can be easily fixed. I found it unfortunate that we are mixing message format vs. what can be managed (e.g., data nodes are mirroring TLVs, etc.). I will be definitely balloting “Yes” for this specification, but I’d like to DISCUSS some few points. # Lack of narrative text I would expect a minimum of narrative text to explain the set of management function supported by the model. For example, I can’t tell whether we can store the algo capabilities of adj SR routers, etc. Please remember that RFC8407 says the following: 3.5. Narrative Sections The narrative part MUST include an overview section that describes the scope and field of application of the module(s) defined by the specification and that specifies the relationship (if any) of these modules to other standards, particularly to standards containing other YANG modules. The narrative part SHOULD include one or more sections to briefly describe the structure of the modules defined in the specification. # Implicit SID types CURRENT: container sid-sub-tlv { description "Used to advertise the SID/Label associated with a prefix or adjacency."; leaf length { type uint16; description "Length of the SID value. YANG model specification is necessary since it dictates the semantics of the SID."; } leaf sid { type uint32; description "Segment Identifier (SID) - A 20 bit label or 32 bit SID. If the length is set to 3, then the 20 rightmost bits represent a label. If the length is set to 4, then the value represents a 32-bit SID."; It is unfortunate to mix the wire encoding vs manageability function. Why a type is not used here instead of implicitly inferring it form a length? BTW, should that type (or length in the current version) be mandatory? especially that the description says "is necessary"? # Ordering control of SID/Label How to control the order of SID/Label to follow this part from RFC8665? * The originating router decides the order in which the set of SID/ Label Range TLVs are advertised inside the Router Information Opaque LSA. # Overlapping prefixes RFC8665 states that: The originating router MUST NOT advertise overlapping ranges. How the model enforces this check on SID/Label Range? # SR capabilities It seems that this is defined only for OSPFv3 in the module, but rfc8665#section-3 says the following: These SR capabilities are advertised in the Router Information Opaque LSA (defined in [RFC7770]). The TLVs defined below are applicable to both OSPFv2 and OSPFv3; see also [RFC8666]. Maybe I’m missing something. ---------------------------------------------------------------------- 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]. # 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. ## 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. # 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]. # 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! ## 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"; ## 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"; ## 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."; ## 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: 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"; ## No need to repeat the parent node prefix. There are many occurrences of this in the module. 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. ## 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"; ## 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 # 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. # Appendix ## Redundant examples Do we really need to have the xml example given that the JSON encoding for the same example is provided? ## Broken JSON example Please validate the example using yanglint, etc. 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
