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

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
<jcucchiara@mindspring.com>wrote:

> 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: jcucchiara@mindspring.com
>> 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 <jcucchiara@mindspring.com>
>> Date: Fri, Apr 12, 2013 at 6:34 PM
>> Subject: [mpls] MIB Dr. review of draft-ietf-mpls-tp-te-mib-05
>> To: mpls@ietf.org, "MIB Doctors (E-mail)" <mib-doctors@ietf.org>, Loa
>> Andersson <loa@pi.nu>, Thomas Nadeau <tnadeau@juniper.net>, Venkatesan
>> Mahalingam <venkat.mahalingams@gmail.com>**, Kannan Sampath
>> <kannankvs@gmail.com>
>>
>>
>> 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.
>
> VM1> Edited.
>
>>
>>
>> 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.
>
> VM1> Edited.
>
>
>> 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)?
>
> VM1> Edited.
>
>
>
>> *) 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.
>
> 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.
>
>
>> *) 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.
>
> VM1> Edited.

>
>
>  *) 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.
>>
>> *) mplsTunnelExtNodeConfigStorage**Type
>>
>> 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.
>
> VM1> OK, No problem.
>
>
>
>> *) mplsTunnelExtIngressLSRLocalId**Valid and
>> mplsTunnelExtEgressLSRLocalIdV**alid
>> 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?
>
> VM1> Yes, edited.

> 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
>> mpls@ietf.org
>> https://www.ietf.org/mailman/**listinfo/mpls<https://www.ietf.org/mailman/listinfo/mpls>
>>
>>
>