Hi Reshad, Thanks for the review. You raised some very good points and especially the point about the OSPFv3 MPLS encoding being missing. We may delay the document until these are added. However, we will address your comments in the next revision of the document.
> On Apr 3, 2023, at 23:18, Reshad Rahman <[email protected]> > wrote: > > Line-breaks (hopefully) fixed below. > > On Monday, April 3, 2023, 11:08:26 PM EDT, Reshad Rahman via Datatracker > <[email protected]> wrote: > > > 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. We’ll add these. > > - The document needs a least 1,probably more, examples. Yes. > > - The abstract mentions OSPF, the overview mentions > OSPFv2 only and the YANG module has OSPFv2 and OSPFv3. The authors will need to discuss this - it is only complete for OSPFv2 due to the OSPFv3 dependence on the extended LSA YANG model (https://datatracker.ietf.org/doc/draft-ietf-lsr-ospfv3-extended-lsa-yang/) which didn’t exist when this document was first written. We may want to add and delay publication. > > - sr-algorithm should be a leafref to an algorithm somewhere, right now it's > a uint8. I changed this to sr-cmn:prefix-sr-algorithm. > > - No need to redefine uint24, it's already define in RFC8294. Fixed. > > - Leaf preference, no need for range 0..255 since it's a uint8 Fixed. > > - 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. This leaf would be ignored if “enabled" is false (which is the default) in the same grouping. If “enabled” is “true”, then it would be applicable. > > 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. > See discussion above on OSPFv3. > - Is uint16 sufficient for range-size? Yes. See section 3.2 or RFC 8665. > > - I see augment ospf:ospfv2 when rt:type = ospfv2, is the when-stmt needed? > The ospfv2 > container should only exist for ospfv2? Yes - the encoding is completely different for OSPFv3. > > - 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. It is not always a label - see section 5 of RFC 8665. > > - List lan-adj-sid-sub-tlv in container lan-adj-sid-sub-tlvs, no need for a > key? It is not read-only and there can be one for each neighbor on the LAN. I believe we need a key. Thanks, Acee > > Regards, > Reshad. > > > > _______________________________________________ > yang-doctors mailing list > [email protected] <mailto:[email protected]> > https://www.ietf.org/mailman/listinfo/yang-doctors > _______________________________________________ > Lsr mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/lsr
_______________________________________________ Lsr mailing list [email protected] https://www.ietf.org/mailman/listinfo/lsr
