Hi Med, Let me respond to the high level DISCUSS points first. I'll address the other comments in a subsequent Email.
> On Mar 26, 2025, at 12:13 PM, Mohamed Boucadair via Datatracker > <nore...@ietf.org> wrote: > > 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. This has proved to be a good exercise. > > # 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? For the LSAs, we've mirrored the encodings in most cases. However, I guess we could add a type. > > BTW, should that type (or length in the current version) be mandatory? > especially that the description says "is necessary"? These are read-only state data and we normally don't indicate mandatory fields in read-only data. I intend to update the model to make it much clearer as to which augmentations are read-write and which are read-only. > > # 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. This is left to the implementation. This is read-only data as well. > > # Overlapping prefixes > > RFC8665 states that: > The originating router MUST NOT advertise overlapping ranges. > > How the model enforces this check on SID/Label Range? This is read-only data as well. If you wanted, you could file an Errata on RFC 9020 to enforce non-overlapping ranges. Offhand, I can't think of a way to enforce with YANG constraints other than in the text. > > # 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]. The augmentations to the OSPFv2 RI LSA Opaque and OSPFv3 RI LSA are in the draft. For example: augment "/rt:routing/" + "rt:control-plane-protocols/rt:control-plane-protocol/" + "ospf:ospf/ospf:areas/" + "ospf:area/ospf:database/" + "ospf:area-scope-lsa-type/ospf:area-scope-lsas/" + "ospf:area-scope-lsa/ospf:version/ospf:ospfv2/" + "ospf:ospfv2/ospf:body/ospf:opaque/ospf:ri-opaque" { when "derived-from(/rt:routing/rt:control-plane-protocols/" + "rt:control-plane-protocol/rt:type, 'ospf:ospfv2')" { description "This augmentation is only valid for OSPFv2."; } description "SR-specific TLVs for OSPFv2 type 10 opaque LSA."; uses sr-algorithm-tlv; uses sid-range-tlvs; uses local-block-tlvs; uses srms-preference-tlv; } augment "/rt:routing/" + "rt:control-plane-protocols/rt:control-plane-protocol/" + "ospf:ospf/ospf:areas/" + "ospf:area/ospf:database/" + "ospf:area-scope-lsa-type/ospf:area-scope-lsas/" + "ospf:area-scope-lsa/ospf:version/ospf:ospfv3/" + "ospf:ospfv3/ospf:body/ospf:router-information" { when "derived-from(/rt:routing/rt:control-plane-protocols/" + "rt:control-plane-protocol/rt:type, 'ospf:ospfv3')" { description "This augmentation is only valid for OSPFv3."; } description "SR-specific TLVs for OSPFv3 Router Information LSA."; uses sr-algorithm-tlv; uses sid-range-tlvs; uses local-block-tlvs; uses srms-preference-tlv; } Thanks, Acee > > 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
