Re: [mpls] MIB Dr. Review: draft-ietf-mpls-tp-te-mib-03.txt

[Sam Aldrin <[e-mail]>]

Hi Joan,

Thank you for the detailed review. Appreciate that very much.
Please find my comments inline with %sam.

-sam
On Jun 3, 2012, at 11:17 PM, Joan Cucchiara wrote:

> 
> Hello,
> 
> This document is clearly written and the examples are great!  Thanks for that!
> 
> Review is divided into sections.  General Comments are followed by specific sections.
> The MIB Modules were compiled by smiLint and results are in each MIB Modules'
> section.   There are some warnings, but think these should be fixed.
> 
> My major concern is contained in the General Comments section below.
> 
> Thanks,
> -Joan
> 
> 
> General Comments
> -----------------
> 
> Major concern:
> 
> I would like to express that I can see why the MIB Modules
> are designed as they are.  Namely, so mplsTunnelTable (in rfc3812)
> can be used for MPLS-TP Tunnels also.  I do see there is likely to
> be value in doing that.  However, my concern is that
> the semantic definition of two indices for mplsTunnelTable
> (mplsTunnelIngressLSRId,  mplsTunnelEgressLSRId)
> are being semantically redefined in the MPLS-TE-EXT-STD-MIB, and this
> breaks the rules of the SMI and there is a chance that legacy
> implementations could be jeopardized.
> 
> The index being proposed is: mplsNodeConfigLocalId
> 
>        mplsNodeConfigLocalId  OBJECT-TYPE
>           SYNTAX        MplsLocalId
>           MAX-ACCESS    not-accessible
>           STATUS        current
>           DESCRIPTION
>             "This object allows the administrator to assign a unique
>              local identifier to map Global_Node_ID or ICC."
>           ::= { mplsNodeConfigEntry 1 }
> 
> Where the TC MplsLocalId is:
> 
>     MplsLocalId ::= TEXTUAL-CONVENTION
>        DISPLAY-HINT "d"
>        STATUS      current
>        DESCRIPTION
>          "This textual convention is used in accommodating the bigger
>           size Global_Node_ID and/or ICC with lower size LSR
>           identifier in order to index the mplsTunnelTable.
> 
>           The Local Identifier is configured between 1 and 16777215,
>           as valid IP address range starts from 16777216(01.00.00.00).
>           This range is chosen to identify the mplsTunnelTable's
>           Ingress/Egress LSR-id is IP address or Local identifier,
>           if the configured range is not IP address, administrator is
>           expected to retrieve the complete information
>          (Global_Node_ID or ICC) from mplsNodeConfigTable. This way,
>           existing mplsTunnelTable is reused for bidirectional tunnel
>           extensions for MPLS based transport networks.
> 
>           This Local Identifier allows the administrator to assign
>           a unique identifier to map Global_Node_ID and/or ICC."
>        SYNTAX  Unsigned32(1..16777215)
> 
> 
> 
> The MPLS-TE-STD-MIB (rfc3812) and the MPLS-TC-STD-MIB (rfc3811)
> have described the MplsExtendedTunnelId as:
> 
>  MplsTunnelEntry ::= SEQUENCE {
> .....
>        mplsTunnelIngressLSRId       MplsExtendedTunnelId,
>        mplsTunnelEgressLSRId        MplsExtendedTunnelId,
> 
> 
> 
>  mplsTunnelIngressLSRId OBJECT-TYPE
>     SYNTAX        MplsExtendedTunnelId
>     MAX-ACCESS    not-accessible
>     STATUS        current
>     DESCRIPTION
>          "Identity of the ingress LSR associated with this
>            tunnel instance. When the MPLS signalling protocol
>            is rsvp(2) this value SHOULD be equal to the Tunnel
>            Sender Address in the Sender Template object and MAY
>            be equal to the Extended Tunnel Id field in the
>            SESSION object. When the MPLS signalling protocol is
>            crldp(3) this value SHOULD be equal to the Ingress
>            LSR Router ID field in the LSPID TLV object."
>     REFERENCE
>          "1. RSVP-TE: Extensions to RSVP for LSP Tunnels,
>            Awduche et al, RFC 3209, December 2001
>           2. Constraint-Based LSP Setup using LDP, Jamoussi
>            (Editor), RFC 3212, January 2002"
>     ::= { mplsTunnelEntry 3 }
> 
>  mplsTunnelEgressLSRId OBJECT-TYPE
>     SYNTAX        MplsExtendedTunnelId
>     MAX-ACCESS    not-accessible
>     STATUS        current
>     DESCRIPTION
>          "Identity of the egress LSR associated with this
>            tunnel instance."
>     ::= { mplsTunnelEntry 4 }
> 
> 
> The TC from MPLS-TC-STD-MIB (rfc3811) is:
>      MplsExtendedTunnelId ::= TEXTUAL-CONVENTION
>         STATUS        current
>         DESCRIPTION
>            "A unique identifier for an MPLS Tunnel.  This may
>             represent an IPv4 address of the ingress or egress
>             LSR for the tunnel.  This value is derived from the
>             Extended Tunnel Id in RSVP or the Ingress Router ID
>             for CR-LDP."
>         REFERENCE
>            "RSVP-TE: Extensions to RSVP for LSP Tunnels,
>             [RFC3209].
> 
>             Constraint-Based LSP Setup using LDP, [RFC3212]."
>         SYNTAX  Unsigned32(0..4294967295)
> 
> 
> So, while I agree that mplsTunnelIngressLSRID and mplsTunnelEgressLSRId
> and mplsNodeConfigLocalId are all 32-bit values,  the following 2 TCs are not semantically the same:
> 
> 
>      MplsExtendedTunnelId ::= TEXTUAL-CONVENTION
>         STATUS        current
>         DESCRIPTION
>            "A unique identifier for an MPLS Tunnel.  This may
>             represent an IPv4 address of the ingress or egress
>             LSR for the tunnel.  This value is derived from the
>             Extended Tunnel Id in RSVP or the Ingress Router ID
>             for CR-LDP."
>         REFERENCE
>            "RSVP-TE: Extensions to RSVP for LSP Tunnels,
>             [RFC3209].
> 
>             Constraint-Based LSP Setup using LDP, [RFC3212]."
>         SYNTAX  Unsigned32(0..4294967295)
> 
>     MplsLocalId ::= TEXTUAL-CONVENTION
>        DISPLAY-HINT "d"
>        STATUS      current
>        DESCRIPTION
>          "This textual convention is used in accommodating the bigger
>           size Global_Node_ID and/or ICC with lower size LSR
>           identifier in order to index the mplsTunnelTable.
> 
>           The Local Identifier is configured between 1 and 16777215,
>           as valid IP address range starts from 16777216(01.00.00.00).
>           This range is chosen to identify the mplsTunnelTable's
>           Ingress/Egress LSR-id is IP address or Local identifier,
>           if the configured range is not IP address, administrator is
>           expected to retrieve the complete information
>          (Global_Node_ID or ICC) from mplsNodeConfigTable. This way,
>           existing mplsTunnelTable is reused for bidirectional tunnel
>           extensions for MPLS based transport networks.
> 
>           This Local Identifier allows the administrator to assign
>           a unique identifier to map Global_Node_ID and/or ICC."
>        SYNTAX  Unsigned32(1..16777215)
> 
> Therefore, I believe this is a semantic redefinition and could potentially cause
> issues for some legacy implementations.
%sam - Very good point. Thinking more about it, we should be able to use mplsExtendedTunnelId itself, instead of mplsLocalId. As the mplsExtendedTunnelId could have IP address or LSR identifier, we should just use it as LSR identifier. Instead of specifying or mandating what the identifier should be, we should leave as implementation specific, when used for identifying Egress and Egress LSR's. 
Having said that, we could still provide text with guidance, like we did to describe what LocalId is and how it is derived to identify Egress and Ingress LSR's for TP tunnels. At the end of the day, the Egress LSR and Ingress LSR identifiers should be unique, for a TP tunnel, on a given LSR (based on GlobalID etc).

We will reword the text and make appropriate changes to reflect the same. This will ensure the semantics are not redefined and legacy implementations are nor broken.
> 
> 
> 
> 
> *) There were some warning generated by smiLint that should probably
> be fixed (please see individual MIB Modules discussion below.)
> 
%Sam - nod.
> 
> 
> 
> *) MIB should be capitalized consistently.  Sometimes mib is used, so
> please use capitals.
%sam - nod
> 
> 
> *) There are 4 MIB Modules in this document which pertain to
> MPLS-TP.  The proposal is to root them under mplsStdMIB.
> This has not been the convention for sometime.  However, if the
> authors feel strongly about using mplsStdMIB, then please let me
> know.
%sam - we went back and forth on this, amongst ourselves and gotten some feed back from WG chairs and others as well. Essentially, MPLS-TP is MPLS, so, we shouldn't be defining it as MPLS-TP. So, all the changes or extensions we are proposing are not just for MPLS-TP but MPLS in general. In the future, we should be able to use these extensions not only for TP tunnels, but other types, which we already foresee happening. This was the primary reason for not forking out MPLS-TP specific MIB's, rather re-use and hang off of existing MPLS MIB's.
Does that answer your concern?
> 
> 
> 
> *) The word "traps" appears as a MIB comment in the MIB Modules.  Please either remove
> the comment or use the notifications where appropriate.  Traps are a SNMPv1
> mechanism.
%sam - nod
> 
> 
> *) MIB Guidelines suggest naming conventions to be used
> within a MIB Module (for tables, rows and objects within a row),
> This was not followed in these MIB Modules.  As an example:
> 
> MPLS-TE-EXT-STD-MIB has the following table names:
> 
> mplsNodeConfigTable
> mplsNodeIpMapTable
> mplsNodeIccMapTable
> mplsTunnelExtTable
> 
> This is also the case in the other MIB Module(s).  Please take a look
> at Appendix C of the MIB Guidelines (rfc4181).
%sam - nod.
> 
> 
> 
> Section 4. Motivations
> ----------------------
> Awkward.  Section doesn't specify motivations, expect something
> like, This document provides a MIB Module which supports
> configuration of .... and monitoring of ....etc.
%sam - nod.
> 
> 
> Section 7. MIB Module Interdependencies
> 
> 
> How is this possible, since MPLS-TE-STD-MIB was defined so long ago?
> 
> ..        -  MPLS-TE-STD-MIB [RFC3812] contains references to objects in
>          MPLS-ID-STD-MIB.
%sam - it is a mistake. will fix it.
> 
> 
> 
> 
> MPLS-TC-EXT-STD-MIB
> ==========================
> 
> *) The name of this MIB Module is misleading.
> The TCs defined are specific to MPLS-TP so could this name
> be more specific, for example,  MPLS-TP-TC-STD-MIB ?
%sam - same reasoning given above. We want to keep MIB generic and not TP specific. This was the guidance we got, to remove 'TP', as we had 'TP' in every object as well. 
> 
> *) These TCs should have references.  They are
> originally defined elsewhere, so why aren't their references?
> 
%sam - will add.
> 
> 
> 
> *) MplsNodeId
> 
> DESCRIPTION
> ...LSR's /32 IPv4 loopback address
> 
> What is meant by /32 ?
%sam - :-). will fix it.
> 
> 
> 
> 
> 
> MPLS-ID-STD-MIB
> =========================
> 
> SMILINT
> -------
> 
> mibs/MPLS-ID-STD-MIB:70: [5] {identifier-external-case-match} warning: identifier `mplsGlobalId' differs from `MPLS-TC-EXT-STD-MIB::MplsGlobalId' only in case
> mibs/MPLS-TC-EXT-STD-MIB:61: [6] {previous-definition} info: previous definition of `MplsGlobalId'
> mibs/MPLS-ID-STD-MIB:94: [5] {identifier-external-case-match} warning: identifier `mplsNodeId' differs from `MPLS-TC-EXT-STD-MIB::MplsNodeId' only in case
> mibs/MPLS-TC-EXT-STD-MIB:88: [6] {previous-definition} info: previous definition of `MplsNodeId'
> 
> mibs/MPLS-ID-STD-MIB:4: [5] {import-unused} warning: identifier `NOTIFICATION-TYPE' imported from module `SNMPv2-SMI' is never used
> mibs/MPLS-ID-STD-MIB:6: [5] {import-unused} warning: identifier `NOTIFICATION-GROUP' imported from module `SNMPv2-CONF' is never used
> 
%sam - nod.
> 
> 
> 
> MPLS-LSR-EXT-MIB
> ===================
> 
> *) The name of this MIB Module is too broad because MIB Module
> is specific to MPLS-TP, so something like MPLS-TP-LSR-EXT-MIB
> would be more appropriate IMHO.
%sam - same reasons given above.
> 
> *) Is mplsXCExtTunnelPointer supposed to be read-only?
> The DESCRIPTION seems to indicate read-create?
> (leads to following question regarding compliance stmts....)
> 
%sam - will fix it. Initially we had it as read create.
> 
> *) The FullCompliance and ReadOnlyCompliance
> are confusing.  Why does the mplsXCExtTunnelPointer object
> appear in both?
> 
%sam - will fix it.
> 
> 
> 
> 
> MPLS-TE-EXT-MIB
> ===================
> 
> SMILINT
> mibs/MPLS-TE-EXT-STD-MIB:22: [4] {module-identity-registration} warning: uncontrolled IETF module identity registration
> mibs/MPLS-TE-EXT-STD-MIB:4: [5] {import-unused} warning: identifier `Unsigned32' imported from module `SNMPv2-SMI' is never used
> mibs/MPLS-TE-EXT-STD-MIB:5: [5] {import-unused} warning: identifier `Gauge32' imported from module `SNMPv2-SMI' is never used
> mibs/MPLS-TE-EXT-STD-MIB:5: [5] {import-unused} warning: identifier `NOTIFICATION-TYPE' imported from module `SNMPv2-SMI' is never used
> mibs/MPLS-TE-EXT-STD-MIB:9: [5] {import-unused} warning: identifier `NOTIFICATION-GROUP' imported from module `SNMPv2-CONF' is never used
> 
> 
> *) Warnings of severity [5] should be fixed.
%sam - nod
> 
> 
> *) The name of this MIB Module is too broad also. MIB is specific
> to MPLS-TP and the name of the MIB Module should reflect that.
> One example is, MPLS-TP-TE-EXT-MIB.
%sam - same reasons given above.
> 
> 
> 
>