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

[Yingzhen Qu <[e-mail]>]

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]> 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/
> 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/
>
>
>
> ----------------------------------------------------------------------
> 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.


> 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) 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.