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

From: "Joan Cucchiara" <[email-masked]>
Date: Mon, 4 Jun 2012 02:17:58 -0400

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.

*) There were some warning generated by smiLint that should probably
be fixed (please see individual MIB Modules discussion below.)

*) MIB should be capitalized consistently.  Sometimes mib is used, so
please use capitals.

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

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

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

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.

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.

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 ?

*) These TCs should have references.  They are
originally defined elsewhere, so why aren't their references?

*) MplsNodeId

DESCRIPTION
...LSR's /32 IPv4 loopback address

What is meant by /32 ?

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

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.

*) Is mplsXCExtTunnelPointer supposed to be read-only?
The DESCRIPTION seems to indicate read-create?
(leads to following question regarding compliance stmts....)

*) The FullCompliance and ReadOnlyCompliance
are confusing.  Why does the mplsXCExtTunnelPointer object
appear in both?

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.

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

---

From: Sam Aldrin <[email-masked]>
Date: Tue, 12 Jun 2012 21:01:08 -0700

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'
> and mplsNodeConfigLocalId are all 32-bit values,  the following 2 TCs are not semantically the same:
> 
> 
> Therefore, I believe this is a semantic redefinition and could potentially cause
%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.
> be fixed (please see individual MIB Modules discussion below.)
%Sam - nod.
> *) MIB should be capitalized consistently.  Sometimes mib is used, so
%sam - nod
> authors feel strongly about using mplsStdMIB, then please let me
%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
%sam - nod
> at Appendix C of the MIB Guidelines (rfc4181).
%sam - nod.
> Section 4. Motivations
%sam - nod.
> ..        -  MPLS-TE-STD-MIB [RFC3812] contains references to objects in
%sam - it is a mistake. will fix it.
> MPLS-TC-EXT-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. 
> originally defined elsewhere, so why aren't their references?
%sam - will add.
> ...LSR's /32 IPv4 loopback address
%sam - :-). will fix it.
> 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
%sam - same reasons given above.
> (leads to following question regarding compliance stmts....)
%sam - will fix it. Initially we had it as read create.
> are confusing.  Why does the mplsXCExtTunnelPointer object
%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
> 
> 
%sam - nod
> One example is, MPLS-TP-TE-EXT-MIB.
%sam - same reasons given above.