Re: [mpls] MIB Dr. review of draft-ietf-mpls-tp-te-mib-05

[Sam Aldrin <[e-mail]>]

Hi Joan,

Thank you for your detailed review.
Will address all the comments you have raised.

An updated version with appropriate changes will be sent for your review, prior to publishing the new version.

thanks again.

-sam
On Apr 12, 2013, at 6:34 PM, Joan Cucchiara <[e-mail]> wrote:

> Sam,
> 
> I am sorry for such a long delay....here is the MIB Dr. Review.   Lots of progress on the draft!
> 
> Please see the comments below.
> 
> Thanks,
> -Joan
> 
> 
> smicng OUTPUT
> --------------
> 
> MPLS-LSR-EXT-STD-MIB
> ======================
> W: f(MPLS-LSR-EXT-STD-MIB.my), (216,18) For "mplsXCExtTunnelPointer", syntax is identical
> W: f(MPLS-LSR-EXT-STD-MIB.my), (215,18) MIN-ACCESS value identical to access specified for "mplsXCExtTunnelPointer"
> W: f(MPLS-LSR-EXT-STD-MIB.my), (223,18) For "mplsXCExtOppositeDirXCPtr", syntax is identical
> 
> *) The DESCRIPTION clauses related to these objects
> should be updated to include the information in this
> conformance section.
> 
> 
> 
> 
> MPLS-TE-EXT-STD-MIB
> =====================
> E: f(MPLS-TE-EXT-STD-MIB.my), (284,17) Index item "mplsTunnelExtNodeIpMapNodeId" must be defined with syntax that includes a range
> 
> I see the following in MPLS-TC-EXT-STD-MIB
>  MplsNodeId ::= TEXTUAL-CONVENTION
>  ....
>     SYNTAX  Unsigned32  -- the default range: (0..4294967295)
> 
> my suggestion is to include the range as part of the SYNTAX in the TC.
> 
> 
> smilint
> -----------
> MIBs compile cleanly.
> 
> 
> GENERAL COMMENTS
> ----------------
> *) The terms augments/sparse augments/extensions are all used to
> describe relationships with tables in this document and
> tables in rfc3812 and rfc3813.  For example,
> a table in this document says "sparse augments" but then has
> a value of noSuchInstance returned if the counter is not applicable
> for TP.
> 
> Another part of the document says that a table in this document
> augments a table from elsewhere, but that does not seem to be the
> case when looking at the MIB module.
> 
> The relationships beween tables in this document and
> the tables in rfc3812 and rfc3813 need to be specified clearly.
> I would ask that the authors please check these relationships between
> the tables.   This may be (simply?) using the term
> 'sparse augments' consistently.
> 
> 
> 
> *) Appendix D of rfc4181 (MIB guidelines) suggests:
> 
> 
>        xxxMIB
>        |
>        +-- xxxNotifications(0)
>        +-- xxxObjects(1)
>        +-- xxxConformance(2)
>            |
>            +-- xxxCompliances(1)
>            +-- xxxGroups(2)
> 
> The MIB Modules in this doc:
> 
>  \-2 Conformance
>    \-v-1 Groups
> 
>      \-2 Compliances
> 
> So Groups and Compliances are not in the suggested order.
> 
> 
> Specific Comments
> =====================
> 
> *) Introduction
> 
> "This MIB module should be used...."
> 
> Which MIB module are you referring to?   (Do you mean "These MIB modules"??)
> 
> *) Same Section (Intro)
> "...for MPLS based traffic engineering configuration and management."
> 
> I don't understand completely, but seems like MPLS TP is probably what is meant.
> 
> 
> *) 4. Motivations
> 
> s/used in non-IP environment./used in non-IP environments.
> 
> awkward sentence:  This MIB also defines three other MIB modules within this document.
> Maybe:  This document defines 4 MIB modules: (and then list all 4)
> 
> 
> 5. Feature List
> Could you refer to the MIB module by name?  So, instead of
> "MPLS transport profile MIB module" use the MPLS-TE-EXT-STD-MIB
> and so forth.
> 
> 
> 6. Brief description of MIB Objects
> This section focuses on MPLS-TE-EXT-STD-MIB, so maybe rename the section
> or also include the other MIB modules in this document.
> 
> 6.5 mplsTunnelExtReversePerfTable
> 
> "This table augments the mplsTunnelTable..."
> 
> If it augments, then why is AUGMENTS not used in the
> MPLS-TE-EXT-STD-MIB?   I think this relationship is a
> sparce augments, not an actual augments?   Please
> explain.
> 
> 
> MPLS-TC-EXT-MIB
> ==================
> *)  Why is the CC ID not represented in this MIB Module?
> 
> Basically, the question has to do with the following from
> draft-ietf-mpls-tp-itu-t-identifiers, specifically:
> 
> "Together, the CC and the ICC form the ICC_Operator_ID as:
> 
>     CC::ICC
> 
>  The ICC_Operator_ID is used as a replacement for the Global_ID as
>  specified in [RFC6370], i.e. its purpose is to provide a globally
>  unique context for other MPLS-TP identifiers."
> 
> ICC_Operator_ID appears to be the counterpart to GLOBAL_ID.  In other
> words, these both uniquely identify an operator. Whereas, ICC by itself
> does not.
> 
> 
> MPLS-ID-STD-MIB
> =================
> 
> *)What is the relationship between mplsIdGlobalId and mplsIdNodeId?
> Specifically, if mplsIdGlobalId has been set, can mplsIdNodeId be
> set to a different value?
> 
> 
> *) NIT:  Could the order reflect the same order as in MPLS-TC-EXT-STD-MIB?
> The reason is that GLobal_ID and NODE_ID are related so think would be
> logical to have them near each other.
> 
> *) Also, I'm not really sure why you want a MIB module with only 3 scalars
> in it?  As far as I can see these scalars are only used in the
> MPLS-TE-EXT-STD-MIB, so why separate them?
> 
> *) Error:  FullCompliance and ReadOnlyCompliance is EXACTLY the same.
> 
> MPLS-LSR-EXT-STD-MIB
> =====================
> 
> Compliance:
>    OBJECT      mplsXCExtTunnelPointer
>    SYNTAX      RowPointer
>    MIN-ACCESS  read-only
>    DESCRIPTION
>       "The only valid value for Tunnel Pointer is
>        mplsTunnelTable entry."
> 
> *) This object is read-only, so no need for a read-only compliance
> since object is already read-only.
> 
> *) The above DESCRIPTION needs to be included in the
> object's DESCRIPTION.
> 
> 
>    OBJECT      mplsXCExtOppositeDirXCPtr
>    SYNTAX      RowPointer
>    MIN-ACCESS  read-only
>    DESCRIPTION
>       "The only valid value for XC Pointer is
>        mplsXCTable entry."
> 
>    ::= { mplsLsrExtCompliances 2 }
> 
> *) The above DESCRIPTION needs to be included in the
> object's DESCRIPTION.
> 
> *) There is nothing here that specifies the readOnly compliance.
> Would expect something like:
> 
> OBJECT nameOfObject
> MIN-ACCESS  read-only
> DESCRIPTION "Write access is not required."
> 
> 
> MPLS-TE-EXT-STD-MIB
> ===================
> 
> *)          DESCRIPTION
>          "This object indicates the Global Operator Identifier.
>           This object value should be zero when
>           mplsTunnelExtNodeConfigIccId is configured with non-null
>           value."
> 
> DESCRIPTION should state that the object has no meaning when
> mplsTunnelExtNodeConfigIccId is valid.  Same comment for
> mplsTunnelExtNodeConfigId.
> 
> In other words, maybe an additional object is needed to indicate
> Global or ICC?  What if a 3rd TP id is created, then what happens?
> 
> Better to be explicite I think, than to try and overload these
> objects with values that they shouldn't have.
> 
> mplsTunnelExtNodeConfigNodeId  OBJECT-TYPE
>        SYNTAX        MplsNodeId
>        MAX-ACCESS    read-create
>        STATUS        current
>        DESCRIPTION
>           "This object indicates the Node_ID within the operator.
> 
> *) Sentence is awkward.
> 
> mplsTunnelExtNodeConfigIccId
> Similar comment to the above.  Additionally, how can an OCTET STRING
> of size (1..6) have a zero value?
> 
> *) mplsTunnelExtNodeConfigStorageType
> 
> Not sure I see the advantage of having this as it is
> already in the MplsTunnelTable.  Why have 2 StorageType objects
> for the same row?
> 
> *) mplsTunnelExtIngressLSRLocalIdValid and
> mplsTunnelExtEgressLSRLocalIdValid
> Please fix the DESCRIPTION and REFERENCE.
> (Looks like the DESCRIPTION clause has been continued after the
> REF clause.)
> 
> 
> 
> *) mplsTunnelExtReversePerfTable
> 
> I would like to understand why noSuchInstance would be needed.
> This table is a sparse augments,
> so either the entry would be there or it wouldn't, right?
> 
> *) Probably could remove the following (comment applies to other
> MIB Modules also.)
> 
>    -- Notifications
>    mplsTeExtNotifications OBJECT IDENTIFIER
>                                     ::= { mplsTeExtStdMIB 0 }
> 
>    -- Notifications.
>    -- Notification objects need to be added here.
>    -- End of notifications.
> 
> 
> *) Compliance Section
> ReadOnly compliance is the same as the Full Compliance.
> In other words, readonly compliance does not really exist.
> Please fix this.
> 
> 
> 14. Security Considerations
> ============================
> Should specify which objects would jeopardize security.
> 
> 15. IANA Considerations
> ========================
> 
> Where is it?  This section needs to be done.
> 
> 
> -- the end --
>