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

[Yingzhen Qu <[e-mail]>]

Hi Mahesh,

I've published version -12 and removed the "mandatory true". Please let me
know if you have any other comments.

Thanks,
Yingzhen

On Fri, Jul 5, 2024 at 11:22 AM Mahesh Jethanandani <[e-mail]>
wrote:

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