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

["Joan Cucchiara" <[e-mail]>]

Venkat,

Thank you for your quick response.  Would like to close on some 
questions/comments, see below.
I have cc'd MIB Drs. and MPLS WGs also.   I think these WGs should be 
included as part of the
ongoing review.

Thanks,
  -Joan


>
> ----- Original Message ----- 
> From: venkatesan mahalingam
> To: [e-mail]
> Cc: Sam Aldrin ; Kannan Sampath ; Thomas Nadeau
> Sent: Friday, April 19, 2013 10:10 PM
> Subject: Fwd: [mpls] MIB Dr. review of draft-ietf-mpls-tp-te-mib-05
>
<snip>
>
>
>
> ---------- Forwarded message ----------
> From: Joan Cucchiara <[e-mail]>
> Date: Fri, Apr 12, 2013 at 6:34 PM
> Subject: [mpls] MIB Dr. review of draft-ietf-mpls-tp-te-mib-05
> To: [e-mail], "MIB Doctors (E-mail)" <[e-mail]>, Loa
> Andersson <[e-mail]>, Thomas Nadeau <[e-mail]>, Venkatesan
> Mahalingam <[e-mail]>, Kannan Sampath
> <[e-mail]>
>
>
> 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.
>
>
> VM> Not fixed yet, how to get this warnings in smilint compiler, what's 
> the
> severity level to be used? Do you have any suggestion to fix this kind of
> issue?
>

This is a problem with compliance (see below).     Additionally, the 
DESCRIPTION
clause is information I would expect to see in the object's DESCRIPTION, 
rather than as part of
the Compliance Statement.

>
>
> 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.
>
>
> VM>Edited.

NIT:   The value of zero is reserved, so more appropriate specification 
would be (0|1..4294967295), so NIT, but
would serve to enforce the DESCRIPTION clause.


>
> 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.
>
>
> VM>Edited.
>
>
> *) 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.
>
>
> VM>Edited.
>
>
> Specific Comments
> =====================
>
> *) Introduction
>
> "This MIB module should be used...."
>
> Which MIB module are you referring to?   (Do you mean "These MIB 
> modules"??)
>
>
> VM>Edited.
>
>
> *) 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.
>
>
> VM>Edited.
>
>
> *) 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)
>
>
> VM>Edited.
>
>
> 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.
>
>
> VM>Edited.
>
>
> 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.
>
>
> VM>Edited.
>
>
> 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.
>
>
> VM>Edited.
>
>
> 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.
>
>
> VM>Edited.
>
>
> 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?
>
> VM> NodeId is defined within the scope of GlobalId. Yes, NodeId can be set
> to different value if the GlobalId has the valid value already.
>

The impact of these scalars need to be explained to a user.   How would 
changing this value after
establishing a TP Tunnel  impact the network (for example)?



>
> *) 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.
>
>
> VM>Edited.
>
>
> *) 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?
>
>
> VM> These scalars will be used for MPLS-TP PWE3 extension also. So, it is
> better to be in the separate MIB module.
>

I kind of have a problem with this.   If there is another MIB Module that 
depends on these scalars, shouldn't that
other MIB Module be reviewed as part of this process?     What is the state 
of this other MIB Module?

Additionally, these TCs and associated scalars are dependent on a draft in 
the MPLS WG.  So, these could change
in theory.


>
> *) Error:  FullCompliance and ReadOnlyCompliance is EXACTLY the same.
>
>
> VM> OK.
>
> 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.
>

This is the cause of the SMICNG warning.   Should be fixed as this adds 
nothing and is redundant.


> *) 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."
>
>
> VM>Edited.
>
>
> 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.
>
>
> VM>Edited.
>
>
> mplsTunnelExtNodeConfigNodeId  OBJECT-TYPE
>        SYNTAX        MplsNodeId
>        MAX-ACCESS    read-create
>        STATUS        current
>        DESCRIPTION
>           "This object indicates the Node_ID within the operator.
>
> *) Sentence is awkward.
>
> VM> Corrected.
>
> mplsTunnelExtNodeConfigIccId
> Similar comment to the above.  Additionally, how can an OCTET STRING
> of size (1..6) have a zero value?
>
> VM> Corrected.
>
> *) 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?
>
> VM> NodeConfig table is a separate table used to map the Global_Node_ID 
> and
> CC::ICC identifiers with the local-id.
> So, I don't see 2 Storage types.


Sorry, my error.   You are correct.



>
> *) mplsTunnelExtIngressLSRLocalIdValid and
> mplsTunnelExtEgressLSRLocalIdValid
> Please fix the DESCRIPTION and REFERENCE.
> (Looks like the DESCRIPTION clause has been continued after the
> REF clause.)
>
>
> VM> I don't understand this comment. Do you expect the reference clause 
> for
> these two objects?
>

When I look at this, I see:

 DESCRIPTION
          "This object denotes whether the mplsTunnelIngressLSRId
           contains the local value, which is used to reference
           the complete Ingress Global_ID::Node_ID or ICC from
           the mplsTunnelExtNodeConfigTable."
         REFERENCE
          "MPLS-TE-STD-MIB [RFC3812], Section 11. mplsTunnelIngressLSRId
           object in mplsTunnelTable.

           If this object is set to FALSE, mplsTunnelExtNodeConfigTable
           will not contain an entry to reference local identifier with
           Global_ID::Node_ID or ICC value.

           This object is set to FALSE for legacy implementations like
           MPLS TE tunnels where mplsTunnelIngressId itself provides
           complete Ingress LSRId."

So, if you look at the DESCRIPTION clause and REFERENCE clause, you can see 
that
some of the text in the REFERENCE clause is more descriptive, specifically:
          ".....mplsTunnelIngressLSRId
           object in mplsTunnelTable.

           If this object is set to FALSE, mplsTunnelExtNodeConfigTable
           will not contain an entry to reference local identifier with
           Global_ID::Node_ID or ICC value.

           This object is set to FALSE for legacy implementations like
           MPLS TE tunnels where mplsTunnelIngressId itself provides
           complete Ingress LSRId."

I would not expect to find the above text in a REFERENCE clause.   Does my 
original comment make sense now?

Thanks,
  -Joan


>
> *) 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?
>
> VM> Yes. Corrected.
>
> *) 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.
>
>
> VM> Removed.
>
> *) Compliance Section
> ReadOnly compliance is the same as the Full Compliance.
> In other words, readonly compliance does not really exist.
> Please fix this.
>
>
> VM> Updated.
>
>
> 14. Security Considerations
> ============================
> Should specify which objects would jeopardize security.
>
>
> VM> Edited.
>
> 15. IANA Considerations
> ========================
>
> Where is it?  This section needs to be done.
>
>
> VM> Edited.
>
> -- the end --
>
> _______________________________________________
> mpls mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/mpls
>