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

From: "Joan Cucchiara" <[email-masked]>
Date: Fri, 12 Apr 2013 20:34:47 -0500

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 --

---

From: Sam Aldrin <[email-masked]>
Date: Sat, 13 Apr 2013 23:05:14 -0700

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 <[email-masked]> wrote:

> Sam,
> 
> I am sorry for such a long delay....here is the MIB Dr. Review.   Lots of progress on the draft!
> 
> 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
> 
> E: f(MPLS-TE-EXT-STD-MIB.my), (284,17) Index item "mplsTunnelExtNodeIpMapNodeId" must be defined with syntax that includes a range
> 
> I don't understand completely, but seems like MPLS TP is probably what is meant.
> 
> 
> *) 4. Motivations
> 
> awkward sentence:  This MIB also defines three other MIB modules within this document.

---

From: "Joan Cucchiara" <[email-masked]>
Date: Sat, 20 Apr 2013 08:53:40 -0500

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: [email-masked]
> 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 <[email-masked]>
> Date: Fri, Apr 12, 2013 at 6:34 PM
> Subject: [mpls] MIB Dr. review of draft-ietf-mpls-tp-te-mib-05
> To: [email-masked], "MIB Doctors (E-mail)" <[email-masked]>, Loa
> Andersson <[email-masked]>, Thomas Nadeau <[email-masked]>, Venkatesan
> Mahalingam <[email-masked]>, Kannan Sampath
> <[email-masked]>
>
>
> Sam,
>
> 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.

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

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

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

> VM> OK.

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

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

> 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

> VM> Yes. Corrected.
> VM> Removed.
> VM> Updated.
> VM> Edited.
> VM> Edited.
> _______________________________________________
> mpls mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/mpls
>

---

From: venkatesan mahalingam <[email-masked]>
Date: Sat, 20 Apr 2013 12:03:46 -0700

Joan,

I appreciate your quick response on my comments. Please find below my
in-lined response with the tag VM1>.

-Venkat.

On Sat, Apr 20, 2013 at 6:53 AM, Joan Cucchiara
<[email-masked]>wrote:

>> ----- Original Message ----- From: venkatesan mahalingam
>> Mahalingam <[email-masked]>**, Kannan Sampath
>> <[email-masked]>
>>
>>
>> Sam,
>>
> VM1> Edited.
> VM1> Edited.
>> draft-ietf-mpls-tp-itu-t-**identifiers, specifically:
>>
> VM1> Edited.
> other MIB Module be reviewed as part of this process?     What is the
> state of this other MIB Module?
>
> VM1> This comment alone is not fixed, I would like to discuss this further
> with co-authors and let you know you our stand on this next week.
>
>
> VM1> Edited.

>> *) mplsTunnelExtNodeConfigStorage**Type
>>
> VM1> OK, No problem.
>> *) mplsTunnelExtIngressLSRLocalId**Valid and
>> mplsTunnelExtEgressLSRLocalIdV**alid
> So, if you look at the DESCRIPTION clause and REFERENCE clause, you can
> see that
> VM1> Yes, edited.

>> ______________________________**_________________
>> mpls mailing list
>> [email-masked]
>> https://www.ietf.org/mailman/**listinfo/mpls<https://www.ietf.org/mailman/listinfo/mpls>
>>
>>
>