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

	Hi Joan,


	First, thanks for taking the time to go over the document again. The  
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?

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

_______________________________________________
MIB-DOCTORS mailing list
MIB-DOCTORS@ietf.org
https://www.ietf.org/mailman/listinfo/mib-doctors