Re: [mpls] MIB DR review of draft-ietf-mpls-fastreroute-mib-08.txt

Hi Tom,

>
> Hi Joan,
>
>
> First, thanks for taking the time to go over the document again. The

Your welcome!

> comments
> are minor except for the one I have a question/comment about below, so  we 
> will
> fix those in a follow on version ASAP.
>
>> MIB Dr review of  draft-ietf-mpls-fastreroute-mib-08.txt
>>
>> Tom, et al.
>>
>> Following are the review comments from the previous version
>> of the MIB.  There are a few overall comments followed by
>> comments in order of the document.
>> I have indicated that the comment has been
>> "DONE", "NOT DONE" or if comment is
>> a new comment "NEW".
>>
>> Thanks,
>> Joan
>>
>> Overall COMMENTs
>> ---------------
>>
>> *) MIB compiles cleanly with smiLint and
>> smicngPRO. (applies to version 8)
>>
>> *) First overall comment has to do with the organization
>> of the MIB module itself; a better way
>> to represent the objects would be to have separate MIB
>> modules for General, One2One and Facility. Even the scalar objects  seem 
>> dependent on
>> which method is being used so seems that separate
>> MIB modules would help to clarify this.
>> Please comment.
>> NOT DONE. (Think we need to get this comment addressed
>> since many of the other comments marked NOT DONE are
>> related to this.)
>
> Sorry. I think we left this to ask the WG because it was such a major 
> change
> and never got around to doing so. DOH!
>
> I will agree to breaking up the MIB into different modules if we can 
> agree
> that the agent conformance statements need not be overly complex in  their 
> interactions.
> That is, I see three modules called:
>
> MPLS-FRR-GENERAL-STD-MIB MPLS-FRR-ONE2ONE-STD-MIB 
> MPLS-FRR-FACILITY-STD-MIB
> In the latter two modules, each requires only the mandatory objects
> from the MPLS-FRR-GENERAL-STD-MIB. I do not want the latter two  modules 
> to
> require each other in any way (which is why we have a General module).
>
> Does this seem reasonable to you?

This seems reasonable as long as the scalars which are related to
a specific method are included in the appropriate MIB Module for that 
specific method
(Also,  the scalars need to be edited so that they apply only to their 
specific method
as described in the comments below), then yes, I think this would be the 
best approach.

Thanks,
   Joan

>
> --Tom
>
>
>
>> *) I tried to be very specific about this during the
>> commenting, but basically would ask that the names used
>> be consistent with the MPLS-TE-STD-MIB.  For example,
>> Tunnel, Index, Instance, Ingress, Egress and other words
>> are spelled out when used in an object's name, but in
>> this MIB Module these are abbreviated.  Just looking for
>> some consistency with the naming conventions used in the
>> 2 MIBs since they are tightly coupled.
>> DONE but not completely.  This needs to be checked throughout the
>> document since it is not updated consistenly, e.g.
>>
>> * Several places in the document use: mplsFrrTunARHopTable
>> and this table was renamed mplsFrrTunnelARHopTable so please make  this 
>> correction throughout entire document.
>>
>>
>>
>> COMMENTS ON DOC (as they appear in the doc)
>> ---------------------------------------------
>> *) Abstract:
>>   In particular, it describes managed objects for Multiprotocol Label
>>   Switching fast rerouting.
>> suggestion to specify the 2 fast reroute methods:
>> ...managed objects used to support two fast reroute (FRR) methods for
>> Multiprotocol Label Switching (MPLS) based traffic engineering (TE).
>> The two methods are one-to-one backup method and facility backup
>> method.
>> DONE.
>>
>> *) Date on the page headers do not match
>> date of document.
>> DONE.
>> *) TOC
>> References is off by a space.
>> Also, the subsections 1.1, 4.1, 4.2
>> (maybe others) are missing from the TOC.
>> DONE, BUT some sections have incorrect page numbers.
>> Also, some spacing of off.
>>
>>
>> *) 1. Introduction
>> Would add RFC3811 also, i.e.
>> used in conjunction with [RFC3811], [RFC3812] and
>> [RFC3813].
>> DONE.
>>
>> *) NEW:  following needs updating
>> s/mpls@uu.net/mpls@ietf.org/
>>
>>
>> *) 2. Terminology
>> Should state the title of the referenced docs
>> here.
>> So
>> This document uses terminology defined in the
>> "Multiprotocol Label Switching Architecture" [RFC3031]
>> document
>> and  the "Fast Reroute Extensions to RSVP-TE for
>> LSP Tunnels" [RFC4090] document.
>>
>> DONE.
>>
>>
>> *) 4. Brief Description of the MIB Module Objects.
>> s/Objects./Objects
>> Would retitle this section:
>> Overview of the MIB Module
>>
>> DONE.
>> *)I am confused by the term "bypass" in this section.
>> Are you referring to the one-to-one backup method
>> described in [RFC4090]?  Could the terms one-to-one
>> and facility be used consistently?
>>
>>  " The specification [RFC4090] stipulates two different  approaches    to 
>> implementing MPLS TE fast reroute: bypass and  facility
>>   backup.  As such, the MIB module defined in this document"
>>
>> s/bypass/one-to-one backup/
>>
>> NOT DONE (bypass refers to a type of tunnel, not a backup method).
>> "LSRs do not implement both facility and bypass methods
>> at the same time, the Agent Compliance section in this
>> module defined herein is divided into portions, one for
>> each method allowing any LSR to implement only the objects
>> applicable to the method they have implemneted."
>> Either correct the above statement:
>> s/Agent Compliance/Conformance
>> s/implemneted/implemented
>>
>> or reword:
>> "LSRs do not implement both one-to-one backup method and
>> facility backup method
>> at the same time, thus, the Conformance section specifies
>> conformance based on the two fast reroute methods.  This allows
>> a developer to implement only the objects applicable to
>> the fast reroute method supported."
>>
>> DONE.
>>
>>
>> *) I would like to understand what is meant by common practice?
>> Is it that vendors don't support both methods at the same time, or
>> is it that operators don't mix these 2 methods in a network?
>>
>>
>> NOT DONE. Part of my confusion is the phrase "common practice has  shown 
>> that LSRs do not implement both..."  Is the issue that
>> LSRs do not support both, or that operators choose to configure
>> only one method on an LSR?  In other words, could you be specific
>> about what the common practice is?  In my mind, this is important
>> to your justification of why the MIB is only going to allow one
>> kind of backup method on an LSR.
>> *) 4.1 mplsFrrConstTable
>> Please use the entire name Constraints, as in
>> mplsFrrConstraintsTable.
>> DONE, BUT TOC needs to be updated with new tabel name.
>>
>> *) 4.2 mplsFrrTunARHopTable
>> Please use mplsFrrTunnelARHopTable to match
>> with mplsTunnelARHopTable.
>> First sentence:
>> s/mplsTunnelARHop table/mplsTunnelARHopTable
>> DONE, BUT TOC and other places need to be updated with new table name.
>>
>> *) 4.3  mplsFrrOne2OnePlrTable
>>
>> "The mplsFrrOne2OnePlrTable is an optional table that contains
>>   lists of PLRs that initiated detour LSPs to protect tunnel
>>   instances. As such, it is only required for LSRs implementing
>>   the detour backup method. In these cases, the detour LSPs
>>   are reflected in the mplsFrrDetourTable."
>> I don't understand the above. Is this table mandatory when the
>> one-to-one backup method is used?  If so, shouldn't that be stated.
>> The definition from RFC 4090 states:
>> "PLR: Point of Local Repair.  The head-end LSR of a backup tunnel
>> or a detour LSP."
>> So the phrase "contains lists of PLRs" is confusing.
>> Also, the phrase "detour backup method" is confusing, are you
>> referring to the one-to-one backup method?
>>
>> DONE.
>>
>> 4.4 mplsFrrDetourTable
>> *) Could this be renamed to mplsFrrOne2OneDetourTable?
>> DONE, BUT TOC needs to be updated.
>>
>> *) NEW (NIT)
>>
>>  "In the mplsTunnelTable, the higher 16 bits of the tunnel  instance 
>> SHOULD be used as detour instance. Note that for the  protected..."
>>
>> s/be used as/be uses as a/
>>
>>
>> *) Is this table mandatory when the one-to-one backup method is
>> used?  If so, could that be stated?
>> "This table is optional and is only required in case 
>> mplsFrrOne2OnePlrTable is supported."
>> If you state that the table is mandatory for the backup method,
>> then think the above statement is not necessary.
>> DONE.
>> *) 4.5 mplsFrrFacilityDBTable
>>
>> s/mplsFrrDBTable/mplsFrrFacRouteDBTable
>> Could this be renamed to mplsFrrFacilty
>>
>> DONE.
>>
>> *) Could you state that this table is mandatory when the
>> facility backup method is used?
>> DONE.
>>
>>
>> *) 5. Handling IPv6 Tunnels
>>
>> draft-ietf-ccamp-gmpls-addressing-03 does not appear
>> as a reference, please add to Normative References.
>>
>> DONE.
>> *) 6  (MIB MODULE)
>> *) LAST-UPDATED and REVISION is incorrect
>>
>> DONE.
>> *) NEW.  Please remove the REVISION/DESCRIPTION clauses
>> except for the latest version of the MIB.  The REVISION
>> clause is typically used for RFC revisions (not draft
>> revisions). *) DESCRIPTION:
>> ...This MIB module is part of RFC 4327;
>> see the RFC itself for full legal notices.
>> needs to be changed to something like
>>                            This version of this MIB module is part  of 
>> RFC xxxx;
>>               See the RFC itself for full legal notices.
>> -- RFC EDITOR: please replace xxxx with actual number          --  and 
>> remove this note.
>> NOT DONE.
>>
>>
>> *) Please change to yyy
>> ::= { mplsStdMIB yyy } -- RFC-editor please fill in
>>                         -- yyy with value assigned by IANA,
>>                         -- see section 18.1 for details
>>
>> DONE.
>>
>> *) Please change
>> mplsFrrNotif to mplsFrrNotifications
>>
>> DONE.
>> *) Doesn't seem necessary to have
>>   mplsFrrScalars       OBJECT IDENTIFIER ::= { mplsFrrMIB 1 }
>>   mplsFrrObjects       OBJECT IDENTIFIER ::= { mplsFrrMIB 2 }
>> Since Scalars are objects, right?
>> So maybe just use mplsFrrObjects
>> mplsFrrObjects       OBJECT IDENTIFIER ::= { mplsFrrMIB 1 }
>> mplsFrrConformance   OBJECT IDENTIFIER ::= { mplsFrrMIB 2 }
>> NOT DONE.
>>
>>
>> *) Also, please move mplsFrrConformance under mplsFrrObjects.
>>
>> NOT DONE.
>>
>> *) As stated above, I believe this MIB should be organized in
>> 3 separate MIB Modules.  This would help to clarify what
>> actually needs to be supported when using One2One or
>> Facility.
>> The following scalars seem to be dependent on the
>> type of backup method:
>> mplsFrrDetourIncoming
>> mplsFrrDetourOutgoing
>> mplsFrrDetourOriginating
>> mplsFrrConfIfs
>> mplsFrrActProtectedIfs
>> mplsFrrConfProtectionTuns
>> mplsFrrActProtectionTuns
>> mplsFrrActProtectedLSPs
>> mplsFrrNotifsEnabled
>> mplsFrrNotifMaxRate
>> So why are these under General Objects?
>> DONE (sort of).  The scalars which apply to one-to-one backup
>> or facility backup should be co-located with their respective
>> tables.  This is just a cut-n-paste and would go a long way
>> to make the MIB Module more readable.
>>
>>
>> *) NEW the mplsFrrFacObjects name should be changed to
>> mplsFrrFacilityObjects.  The tables use the entire word
>> Facility so this would be for consistency.
>>
>> *) mplsFrrProtectionMethod Scalar
>> should probably be moved to being the first
>> scalar.  Also, I think an unknown(1) should be
>> added.  The reason is that if a device has been
>> rebooted, due to a change from one fast reroute
>> method to another, and if something were misconfigured,
>> then it might be that the fast reroute method would be
>> "unknown" until a correction was made.  Obviously,
>> "unknown" would be read-only and not settable.
>> Would suggest:
>> unknown(1),
>> onetooneBackup(2),
>> facilityBackup(3)
>> DONE.
>>
>> *) Last sentence
>> s/notified/generated/
>> DONE.
>>
>> *) NEW for mplsFrrProtectedMethod
>> Please include in the DESCRIPTION clause what
>> error should be returned if the value of unknown(1) is
>> set.
>>
>> *) NEW, most of the DESCRIPTION clauses in the other
>> objects which refer to this object (mplsFrrProtectedMethod)
>> need to be updated (this means most of the scalars).
>>
>>
>> *) mplsFrrSwitchover
>> Should this be a Gauge32?
>>
>> DONE.
>>
>>
>>
>> *) mplsFrrNotifsEnabled.
>> Why is this object needed?
>>
>> NOT DONE.  Please answer the question wrt
>> RFC3413.
>>
>> *) mplsFrrNotificationsMaxRate
>> Are there other objects which indicate
>> if events are being thrown away due to this
>> throttling?  (Would that be useful?)
>>
>> NOT DONE.  Please answer the question.  I would
>> think such an object (or objects) would be useful
>> information if throttling is done.
>>
>>
>> *) mplsFrrDetourIncoming
>> Should be put with the oneToOne objects.
>> Should this be a Counter32 or Gauge32?
>>
>> NOT DONE.
>>
>> *)Please rename to mplsFrrIncomingDetourLSPs
>> DONE.
>> *) mplsFrrDetourOutgoing
>> Should be put with the oneToOne objects.
>> Should this be a Counter32 or Gauge32?
>>
>> NOT DONE.
>>
>> *) please rename to mplsFrrOutgoingDetourLSPs
>> DONE.
>>
>>
>> *) Could mplsFrrConfIfs be
>> renamed to:
>> mplsFrrConfiguredInterfaces
>>
>> DONE.
>>
>> *)Also, this should apply only to
>> facilityBackup, so could be
>> a Gauge32 or Counter32?
>> NOT DONE.
>>
>>
>> *) mplsFrrActProtectedIfs
>> What does the Act stand for?
>> Is this Active or Actual?
>> Could this be renamed to
>> mplsFrrActiveInterfaces/mplsFrrActualInterfaces
>> DONE.
>>
>>
>> *) mplsFrrConfProtectionsTuns
>> Could this be renamed
>> mplsFrrConfiguredBypassTunnels ?
>>
>> DONE.
>>
>>
>> *) NEW could DESCRIPTION say the value must be 0
>> and should be ignored.
>>
>>
>> *) mplsFrrActProtectionTuns
>> Could this be renamed to
>> mplsFrrActiveBypassTunnels?
>>
>> DONE.
>>
>> *) NEW DESCRIPTION should say, the value must be 0 and should
>> be ignored.
>>
>> *) mplsFrrActProtectedLSPs
>> could this be renamed to:
>> mplsFrrActiveProtectedLSPs?
>> DONE.
>>
>> *) NEW DESCRIPTION should say, the value must be 0 and should
>> be ignored.
>>
>> *) mplsFrrConstTable
>> Please rename to mplsFrrConstraintsTable
>>
>> DONE.
>> *)     mplsFrrConstEntry OBJECT-TYPE
>> Please add a REFERENCE clause and add the
>> reference from the DESCRIPTION clause to the
>> REFERENCE clause.
>>
>> DONE.
>> s/speciifed/specified
>> DONE.
>>
>> "contains at a tunnel ingress."
>> Should be contains a tunnel ingress.
>> DONE.
>>
>> *) RFC 3209 Does not appear in the References.
>>  (This reference is mentioned several times)
>> DONE.
>>
>> *) mplsFrrConstIfIndex
>> please rename to mplsFrrConstIfIndexOrZero
>> DONE.
>> *) mplsFrrConstTunnelInstance
>> s/identication/identification
>>
>> DONE.
>>
>> *) NEW mplsFrrConstraintHopLimit
>> According to RFC4090 this looks to be a byte, so
>> why is there not a range on this?
>> *) mplsFrrConstBandwidth
>> s/reserved for detour/reserved for a detour
>> DONE.
>>
>> *) MplsFrrTunARHop
>> please use Tunnel
>> Also, other object names use Protection or Protected
>> so please use entire word here
>> DONE.
>>
>> *) mplsFrrOne2OnePlrTable
>> DESCRIPTION is a little bit awkward
>> "...that initiated the detour LSPs that traverse this node."
>> maybe the last "that" is not needed?
>> DONE.
>>
>>
>> *) mplsFrrOne2OnePlrEntry
>> states:
>>         "...An entry in
>>         this table is only created by an SNMP agent as instructed
>>         by an MPLS signaling protocol."
>> But there are read-create objects, so want to make sure
>> that these read-create objects are allowed to be
>> changed after a row has been created?
>>
>> DONE.
>>
>> Please spell out Tunnel, Ingress, Egress, Index and Instance in these
>> names.  This is to match the MPLS-TE-STD-MIB.
>>
>> MOST DONE.  Inst still being used.
>>
>>
>> *) mplsFrrOne2OnePlrRunIngrLSRId
>> s/identity/identify
>> *) mplsFrrOne2OnePlrId
>> Please add a REFERENCE clause
>> DONE.
>>
>>
>> *) mplsFrrOne2OnePlrAvoidNAddrType
>> mplsFrrOne2OnePlrAvoidNAddr
>> What does the N stand for?
>> DONE.
>>
>> *) Please add REFERENCE to mplsFrrOne2OnePlrAvoidNAddr
>> DONE.
>>
>> *) mplsFrrDetourTable
>> Please use Tunnel, Index, Instance
>> DONE.
>>
>> *) mplsFrrDetourActive
>> Please add to the DESCRIPTION what true(1) means.
>> DONE.
>>
>> *) mplsFrrDetourMerging
>> name suggestion: mplsFrrDetourMergedStatus
>>      SYNTAX        INTEGER { none(1),
>>                              protectedTunnel(2),
>>                              detour(3)
>>                            }
>> Could the above labels be
>> changed to:
>> notMerged(1),
>> mergedWithProtectedTunnel(2)
>> mergedWithDetour(3)
>> DONE.
>>
>> *)Also the description talks of setting this, but
>> it is a read-only.
>>
>> Please just get rid of the phrase "set to" NOT DONE.
>>
>> *) NEW, mplsFrrOne2OneDetourMergedDetourInst
>>
>> Same comment as above.  Please just the the value is, as
>> the phrase "set to" is confusing with read-only objects.
>>
>>
>> *) mplsFrrFacRouteDBTable
>> Please expand Fac to Facility, Tun to Tunnel,
>> Idx to Index, Inst to Instance, Ingr to Ingress
>> and Egr to Egress.  Also, Prot is used but other
>> places in this MIB spell out Protection, or Protected.
>>
>> DONE.
>>
>>
>> Would also take out the word Route in these
>> object names, so mplsFrrFacRouteDBTable becomes
>> mplsFrrFacilityDBTable
>> DONE.
>>
>> *) mplsFrrFacRouteDBTable
>> DESCRIPTION
>> s/mplsFrrDBTable/mplsFrrFacRouteDBTable
>> (The above occurs in other objects of this
>> table, so please check the DESCRIPTIONs.)
>> DONE.
>>
>>            The protecting tunnel is indicated by the second two
>>            indexes (mplsTunnelIndex and mplsTunnelInstance) and
>>            represents a valid mplsTunnelEntry. Note that the tunnel
>> The above sentence is confusing.  Are you saying that
>> the second two indexes in this table have the same values
>> as mplsTunnelIndex and mplsTunnelInstance?
>> DONE.
>>
>>
>> *) mplsFrrFacRouteProtIfIdx
>> s/applies/apply
>>
>> DONE.
>> *) mplsFrrFacRouteProtTunIdx
>> Please add a REFERENCE clause.  In general, if the
>> DESCRIPTION specifies a reference, then there should
>> probably be a REFERENCE clause.
>> DONE.
>>
>>
>> *) Many of the DESCRIPTIONs state:
>> "...on the specified interface as specified in the
>> mplsFrrFacRouteIfProtIdx."
>> Could this be reworded as:
>> "...on the interface specified by mplsFrrFacRouteIfProtIdx."
>> DONE.
>>
>> *) mplsFrrFacDBNumProtTunOnIf
>> Please supply the specified interface's name.
>> DONE.
>>
>>
>> *) NEW mplsFrrFacilityDBNumProtectingTunnelOnIf
>> s/speficied/specified/
>>
>> *) mplsFrrRacRouteDBNumProtLspOnIf
>> Please supply the specified interface's name.
>>
>> DONE.
>>
>> *) mplsFrrFacRouteDBProtTunStatus
>> Which protected tunnel is denoted here?
>> DONE.
>>
>>
>> *) Same question for mplsFrrFacRouteDBProtTunResvBw
>> DONE.
>>
>> *) mplsFrrFacRouteDBProtTunResvBw
>> Please specify which object (or objects) from
>> the MPLS-TE-STD-MIB are being repeated.
>> DONE.
>>
>> *) mplsFrrFacProtected
>> The name doesn't say much about this notification.
>> Could we think of a more descriptive name, e.g.
>> (suggestion only) mplsFrrFacilityInitialBkupTunnelInvoked?
>> DONE.
>>
>> s/network loading/network load
>> DONE.
>>
>> DESCRIPTION is a little confusing.
>> Could you just say that the notification needs to
>> be sent prior to forwarding data ?
>>
>> DONE.
>> Last paragraph:
>> "Note this notification only applicable..."
>> change to:
>> Note this notification is only applicable
>>
>> DONE. *) mplsFrrFacUnProtected
>> The name doesn't say much about this notification.
>> Could we think of a more descriptive name, e.g.
>> (suggestion only) mplsFrrFacilityFinalTunnelRestored?
>> DONE, okay suggestion only.
>>
>> s/network loading/network load
>> DONE.
>>
>> Last paragraph:
>> "Note this notification only applicable..."
>> change to:
>> Note this notification is only applicable
>> DONE.
>>
>>
>> Conformance
>> --------------
>> *) Full conformance
>> DESCRIPTION of the one-to-one or facilty methods
>> state:
>> "...and is optional for those which do not."
>> Why is it optional?  Is it possible to support these
>> objects in any meaningful way?
>> NOT DONE.
>>
>>
>> read-only Compliance
>> ----------------------- *) Comment is misplaced since it appears  before
>> a scalar object
>>      -- mplsFrrConstTable
>> DONE.
>>
>> *)  Missing from Read-only conformance:
>> mplsFrrNotifsEnabled
>> mplsFrrNotifMaxRate
>> mplsFrrConstSetupPrio
>> mplsFrrconstHoldingPrio
>> mplsFrrConstInclAnyAffinity
>> mplsFrrConstInclAllAffinity
>> mplsFrrConstExclAnyAffinity
>> mplsFrrConstBandwidth
>> mplsFrrOne2OnePlrSenderAddrType
>> mplsFrrOne2OnePlrSenderAddr
>> DONE.
>>
>>
>> *) 7. Security Considerations
>>       configuration and/or performanc statistics.
>>       Administrators not wishing to reveal this information should
>>       consider these objects sensitive/vulnerable and take
>>       precautions so they are not revealed.
>> s/performanc/performance
>> DONE.
>>
>> *) 9. Acknowledgments
>> Please begin this section with:
>> This document is a product of the MPLS Working Group.
>> NOT DONE.
>>
>> *) REFERENCES are not in order of RFC number
>>
>> NOT DONE.
>>
>> *) 10.1 Normative References
>>
>>   [RFC4090] Pan, P., Swallow, G., Atlas, A., "Fast Reroute
>>             Extensions to RSVP-TE for LSP Tunnels", RFC4090,
>>             May 2005.
>> and A. Atlas,
>> s/RFC4090/RFC 4090
>> DONE.
>>
>> *)    [RFC3813] Srinivasan, C., Viswanathan, A. and Nadeau, T.,
>>            "MPLS Label Switch Router Management Information Base ",
>>             RFC 3813, June 2004
>> and T. Nadeau,
>> "Multiprotocol Label Switching (MPLS) Label Switching Router (LSR)
>> Management Information Base"
>> NOT DONE.
>>
>>
>>
>> *)    [RFC3291]  Daniele, M., Haberman, B., Routhier, S., and J.
>>             Schoenwaelder, "TextualConventions for Internet Network
>>             Addresses", RFC 3291, May 2002.
>> s/TextualConventions/Textual Conventions
>> DONE (N/A).
>>
>> *) 10.2 Informative References
>> *) RFC3031 should probably be Normative, it is currently
>> listed as Informative
>> DONE.
>> -- end--
>>
>>
>>
>>
> 

_______________________________________________
mpls mailing list
mpls@ietf.org
http://www.ietf.org/mailman/listinfo/mpls