[mpls] Re: Mahesh Jethanandani's No Objection on draft-ietf-mpls-msd-yang-10: (with COMMENT)

[Mahesh Jethanandani <[e-mail]>]

Hi Yingzhen,

Thanks for addressing most of the comments. Just one follow-up comment.

> On Jul 4, 2024, at 11:07 PM, Yingzhen Qu <[e-mail]> wrote:
> 
> Hi Mahesh,
> 
> Thanks for the review and comments. I've uploaded version -11 to address your comments. Please see my answers below.
> 
> Thanks,
> Yingzhen
> 
> On Thu, Jul 4, 2024 at 5:35 PM Mahesh Jethanandani via Datatracker <[e-mail] <mailto:[e-mail]>> wrote:
> Mahesh Jethanandani has entered the following ballot position for
> draft-ietf-mpls-msd-yang-10: No Objection
> 
> 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/ <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-mpls-msd-yang/ <https://datatracker.ietf.org/doc/draft-ietf-mpls-msd-yang/>
> 
> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> 
> "MPLS", paragraph 0
> >  YANG Data Model for MPLS Maximum Segment Identifier (SID) Depth (MSD)
> >                       draft-ietf-mpls-msd-yang-10
> I support Eric's DISCUSS on the naming of the draft. If the draft is defining
> identities for both MPLS and SRH, shouldn't the title reflect it?
> 
>  [Yingzhen]: changed the title to "YANG Data Model for Maximum SID Depth Types and MPLS Maximum SID Depth" .
> 
> Section 1, paragraph 1
> >    YANG [RFC7950] is a data modeling language used to define the
> >    contents of a conceptual data store that allows networked devices to
> >    be managed using NETCONF [RFC6241] or RESTCONF [RFC8040].
> 
> A redundant paragraph. Do you really need it?
> 
> [Yingzhen]: removed this.
>  
> Section 4.1, paragraph 17
> >      identity msd-base-mpls {
> >        base msd-base;
> >        description
> >          "Identity for MSD types for MPLS data plane.";
> >      }
> 
> Would it help to say in the description "Base identity of MSD types for MPLS
> data plane"?
> 
> [Yingzhen]: modified as suggested.
>  
> Section 4.1, paragraph 18
> >      identity erld-mpls {
> >        base msd-base-mpls;
> >        description
> >          "msd-erld is defined to advertise the Entropy Readable
> >           Label Depth (ERLD).";
> >        reference
> >          "RFC 8662: Entropy Label for Source Packet Routing in
> >                     Networking (SPRING) Tunnels
> >           RFC 9088: Signaling Entropy Label Capability and Entropy
> >                     Readable Label Depth Using IS-IS";
> >      }
> 
> I agree with Med that the name of the identity does not match the description.
> Can this be fixed?
> 
> [Yingzhne]: fixed.
>  
> Section 4.1, paragraph 18
> >      identity msd-base-srh {
> >        base msd-base;
> >        description
> >          "Identity for MSD types for Segment Routing Header (SRH).";
> >      }
> 
> Much like above, would it help to say in the description "Base identity of MSD
> types for SRH"?
> 
> [Yingzhen]: fixed.
>  
> Section 4.1, paragraph 18
> >      identity srh-max-sl {
> >        base msd-base-srh;
> >        description
> >          "The Maximum Segment Left MSD type.";
> >        reference
> >          "RFC 9352: IS-IS Extensions to Support Segment Routing
> >                     over the IPv6 Data Plane";
> >      }
> 
> I know that you had a discussion with Med about these identities.
> 
> My observation is slightly different. identity 'srh-max-sl' is derived from an
> identity 'msd-base-srh', which in turn is derived from 'msd-base'. You are
> suggesting that identity 'srh-max-sl' is a transitive identity of 'msd-base'.
> This would make sense if there is a need to compare an identity to 'msd-base'
> without regard to whether the identity is used in MPLS or SRH data planes. The
> naming of identities suggests otherwise. Is there such a use case?
> 
> [Yingzhen]: You're right, "msd-base" is not used by the ietf-mpls-msd module. However, it's defined as this is one single registry, and there might be cases in future that need to use it. 
>  
> Section 4.2, paragraph 12
> >      grouping msd-type-value {
> >        description
> >          "Grouping for MSD type and value.";
> >        leaf msd-type {
> >          type identityref {
> >            base iana-msd-types:msd-base-mpls;
> >          }
> >          mandatory true;
> >          description
> >            "MSD types. The MSD type is defined in IANA IGP
> >             MSD-Types registry.";
> >        }
> 
> If the nodes defined here are read-only nodes, why the 'mandatory true'
> statement? Note, if you do decide to drop the mandatory keyword, remember to
> update the tree diagram.
> 
> [Yingzhen]: If I remember right, the "mandatory true" was added during the YANG doctor review. It requires the msd-type has to have a value.

I went and checked Jan’s YANG doctors review. All he said was that if the nodes are going to be mandatory, the tree diagram needed to reflect it. Nothing about ro nodes being mandatory.

The point being, even if ‘msd-type’ and ‘msd-value’ are required parameters, who is checking, if these are ro nodes? If they are configurable parameters, the NETCONF/RESTCONF server can check for mandatory flag, but the backend system that populates ro nodes is not going to check for the flag.

>  
> Section 6.1, paragraph 2
> >       URI: urn:ietf:params:xml:ns:yang:iana-msd-types
> >       Registrant Contact: IANA.
> >       XML: N/A, the requested URI is an XML namespace.
> 
> rfc8407bis, Section 5.1 example seems to indicate that when the module is an
> IANA module, the registrant contact is still "The IESG".
> 
> [Yingzhen]: changed to "The IESG".
>  
> The IANA review of this document seems to not have concluded yet.
> 
> No reference entries found for these items, which were mentioned in the text:
> [RFC9088], and [RFC9352].
> 
> [Yingzhen]: Added.
>  
> -------------------------------------------------------------------------------
> NIT
> -------------------------------------------------------------------------------
> 
> All comments below are about very minor potential issues that you may choose to
> address in some way - or ignore - as you see fit. Some were flagged by
> automated tools (via https://github.com/larseggert/ietf-reviewtool <https://github.com/larseggert/ietf-reviewtool>), so there
> will likely be some false positives. There is no need to let me know what you
> did with these suggestions.
> 
> Section 1, paragraph 1
> >    There are two YANG modules defined in this document.  Module iana-
> >    msd-types defines the identities for Maximum SID Depth (MSD) Types as
> >    per the IANA the IGP MSD-Types registry [IANA-IGP-MSD-Types].  This
> >    document also defines module ietf-mpls-msd augmenting the IETF MPLS
> >    YANG model [RFC8960], which itself augments the routing RIB data
> >    model [RFC8349], to provide operational state for various
> >    MSDs[RFC8662].  The module augments the base MPLS model with a list
> >    of various types of node MSDs, as well as various types of MSDs on
> >    links.
> 
> s/per the IANA the IGP/per the IANA IGP/
> 
> [Yingzhen]: fixed.
>  
> Section 2.2, paragraph 2
> >    As defined in [RFC8491], MSD is the number of SIDs supported by a
> >    node or a link on a node.  The module defines lists of MSDs with
> >    different MSD Types for a node and links.  Please note that these are
> >    read-only data as per the node's hardware capability.
> 
> s/read-only data/read-only nodes/
> 
> [Yingzhen]: I think "read-only data" is fine.
>  
> Section 4.2, paragraph 11
> >        leaf msd-value {
> >          type uint8;
> >          mandatory true;
> >          description
> >            "MSD value, in the range of 0-255. 0 represents the lack
> >             of ability to support a SID statck of any depth.";
> >        }
> 
> s/statck/stack/
> 
> [Yingzhen]: fixed.
>  
> Uncited references: [RFC2119] and [RFC8174].
> 
> [Yingzhen]: removed.
>  
> Section 4.2, paragraph 13
> > RFC3688], the following registrations is requested to be made: COMMENT: URI:
> >                                       ^^
> The verb form "is" does not seem to match the subject "registrations".
> 
> [Yingzhen]: fixed.
>  
> Section 5, paragraph 6
> > he title of the document added. When a MSD type is added to the "IGP MSD-Typ
> >                                      ^
> Use "an" instead of "a" if the following word starts with a vowel sound, e.g.
> "an article", "an hour".
> 
> [Yingzhen]: fixed. 


Mahesh Jethanandani
[e-mail]