Re: [VRRP] AD review of draft-ietf-vrrp-unified-mib

[Mukesh Gupta <[e-mail]>]

Kalyan,

Thanks a lot.  Please go ahead and address the comments and publish
the next rev. We all want this draft done asap :)

- Mukesh

Sent from my iPhone

On Jan 10, 2011, at 2:12 PM, "Kalyan (Srinivas)Tata" <[e-mail]
 > wrote:

> Thanks Adrian for the review. I will update the draft (Once WG
> chairs confirm). I will getback if I need any clarifications.
>
> Thanks
> Kalyan
>
> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]] On Behalf
> Of Adrian Farrel
> Sent: Saturday, January 08, 2011 11:37 AM
> To: [e-mail]
> Cc: [e-mail]; [e-mail]
> Subject: [VRRP] AD review of draft-ietf-vrrp-unified-mib
>
> Hi,
>
> Don't panic!
>
> I have performed my AD review of your draft. The purpose of the
> review is to catch any nits or issues before the document goes
> forward to IETF last call and IESG review. By getting these issues
> out at this stage we can hope for a higher quality review and a
> smoother passage through the process.
>
> There are a good number of small issues that I believe can be fixed
> really easily.
>
> I appreciate that a number of these issues are inherited from RFC
> 2787, but this is an ideal chance to clean up.
>
> You will need a new revision to address these points, and I'd ask
> the WG chairs to evaluate whether the changes are large enough to
> warrant a further WG last call.
>
> I have moved the draft into "AD-review:Revised-ID-needed" state in
> the datatracker, and I look forward to seeing the new revision which
> I can put forward for IETF last call.
>
> Thanks for all your work with this draft,
>
> Adrian
>
> ---
>
> Document header
>
> OLD
>   Document: draft-ietf-vrrp-unified-mib-08.txt               July 2010
>   Intended Status: Proposed Standard
> NEW
>   Document: draft-ietf-vrrp-unified-mib-08.txt               July 2010
>   Obsoletes: 2787 (if approved)
>   Intended Status: Proposed Standard
> END
>
> ---
>
> Abstract
>
> OLD
>   This specification defines a Management Information Base (MIB) for
> NEW
>   This specification defines a portion of the Management Information
>   Base (MIB) for
> END
>
> ---
>
> Section 2
>
> OLD
>   This specification defines a Management Information Base (MIB) for
> NEW
>   This specification defines a portion of the Management Information
>   Base (MIB) for
> END
>
> ---
>
> Section 2 etc.
>
> Is it necessary to introduce the term "IPvX"? It is only used a
> couple of times, and the time it is used in the MIB module is a
> problem because the definition of the term is outside the module.
> (Typically, MIB modules are extracted from RFCs and have to survive
> as standalone
> text.)
>
> Can you:
> - remove "(IPvX)" form section 2
> - remove the definition for section 3
> - say "IPv4 and IPv6" in the two uses in section 6 and section 9
>
> ---
>
> Section 4
>
> You need to add text to describe what has changed from 2787. Not a
> lot of details - perhaps a series of bullet points.
>
> --
>
> Section 6 etc.
>
> You need to say "MIB module" not "MIB" because there is only one
> MIB, and you are making just a module in the MIB.
>
> s/This MIB is designed/This MIB module is designed/
>
>
> ---
>
>
> Section 7
>
>   Tables in the MIB include the following:
>
> In fact, there are exactly these three tables, so how about...
>
>   This MIB module contains three tables:
>
> ---
>
> Section 8
>
> Trivial, but...
>
>           -----   MIB Tables For VRRP Router "VR 1":   -----
>
> ...should read "VR1"
> Similarly for VR2 later in the section.
>
> ---
>
> IMPORTS
>
> It is helpful, but mandatory, to show the RFC numbers from which
> things are imported. Thus...
>
> OLD
>       IMPORTS
>           MODULE-IDENTITY, OBJECT-TYPE,
>           NOTIFICATION-TYPE, Counter32,
>           Integer32, mib-2, Unsigned32        FROM SNMPv2-SMI
>
>           TEXTUAL-CONVENTION, RowStatus,
>           MacAddress, TruthValue, TimeStamp,
>           TimeInterval                        FROM SNMPv2-TC
>
>           MODULE-COMPLIANCE, OBJECT-GROUP,
>           NOTIFICATION-GROUP                  FROM SNMPv2-CONF
>           ifIndex                             FROM IF-MIB
>           InetAddressType, InetAddress        FROM INET-ADDRESS-MIB;
> NEW
>       IMPORTS
>           MODULE-IDENTITY, OBJECT-TYPE,
>           NOTIFICATION-TYPE, Counter32,
>
>           Integer32, mib-2, Unsigned32
>               FROM SNMPv2-SMI                               -- RFC2578
>
>           TEXTUAL-CONVENTION, RowStatus,
>           MacAddress, TruthValue, TimeStamp,
>           TimeInterval
>               FROM SNMPv2-TC                                -- RFC2579
>
>           MODULE-COMPLIANCE, OBJECT-GROUP,
>           NOTIFICATION-GROUP
>               FROM SNMPv2-CONF                              -- RFC2580
>
>           ifIndex
>               FROM IF-MIB                                   -- RFC2863
>
>
>           InetAddressType, InetAddress
>               FROM INET-ADDRESS-MIB;                        -- RFC3291
> END
>
> ---
>
> Section 9
>
> In order to ensure that references can be provided, it is customary
> to begin Section 9 (i.e. before the module BEGIN statement) with
> some text such as:
>
>   This MIB module makes reference to the following documents
> [RFC2578],
>   [RFC2579], [RFC2580], [RFC2863], [RFC3291], and [RFC4001].
>
> ---
>
> Vrrpv3VrIdTC
>
> Typos
>               (ifIndex)and IP version, serves to uniquely identify a
> Missing space.
>
>           REFERENCE " RFC 5798 (Sections 3 and 5.2.3"
> Missing close brace.
>
> ---
>
> vrrpv3OperationsTable
>
>               "Unified Operations table for a VRRP router which
>                consists of a sequence (i.e., one or more conceptual
>                rows) of 'vrrpv3OperationsEntry' items which describe
>                the operational characteristics of a virtual router."
>
> I think "which describe" should read "each of which describes"
>
> ---
>
> vrrpv3OperationsEntry
>
>                Rows in the table cannot be modified unless the value
>                of 'vrrpv3OperStatus' has transitioned to
>                'initialize' state.
>
> I think a little more precision would help...
>
>                A rows in this table cannot be modified unless the
> value
>                of 'vrrpv3OperStatus' in the row has transitioned to
>                'initialize' state.
>
> ---
>
> vrrpv3OperationsInetAddrType
>
> As far as I can tell, you only support two values: ipv4(1) and ipv6
> (2).
> Other values of the InetAddressType textual conventions are, I
> think, not supported.
>
> You should add this fact as a note to the DESCRIPTION clause.
>
> ---
>
> vrrpv3OperationsPrimaryIpAddr
>
>               "In the case where there are more than one IP
>
> s/are/is/
>
> ---
>
> vrrpv3OperationsVirtualMacAddr
>
>           REFERENCE "STD 58 RFC 2578"
>
> I am not clear why this reference is cited. MacAddress is defined in
> RFC 2579, but you don't need to provide a reference because that is
> implicit in the IMPORTS clause. Perhaps you mean to give a reference
> to where the mapping from VRID to MAC address is defined?
>
> ---
>
> vrpv3OperStatus
>
> This object is incongruously named.
> To fit with the naming convention for the table you need one of:
> - vrpv3OperationsStatus
> - vrpv3OperationsOperStatus
>
> Note that there are many references to this object throughout the
> document.
>
> ---
>
> vrrpv3OperationsAcceptMode
>
>               "Controls whether a virtual router in Master state
>               will accept packets addressed to the address owner's
>               IPv6 address as its own if it is not the IPv6 address
>               owner.  Default is False.
>               This object is not relevant for rows representing VRRP
>               over IPv4 and should be set to false."
>
> Should read...
>
>   Default is false(2)
>
> ...and...
>
>   should be set to false(2)."
>
> ---
>
> vrrpv3OperationsUpTime
>           SYNTAX       TimeStamp
>           MAX-ACCESS   read-only
>           STATUS       current
>           DESCRIPTION
>               "This is the value of the `sysUpTime' object when this
>               virtual router (i.e., the `vrrpv3OperStatus')
>               transitioned out of `initialized'."
>
> I am not saying that you MUST change this, but I wonder how useful
> it is, because to make sense of it, a management station must also
> read the current value of sysUpTime.
>
>
> An alternative is to supply the up time in timer ticks. That means
> that the management agent has to perform a computation each the row
> is read. If you did this you would have...
>
>           SYNTAX       TimeTicks
>           MAX-ACCESS   read-only
>           STATUS        current
>           DESCRIPTION
>               "This value represents the amount of time since this
>               virtual router (i.e., the `vrrpv3OperStatus')
>               transitioned out of `initialize'."
>
> I just need you to think about which you prefer (there is a trade-
> off) and make a decision. No need to tell me which you chose, or why.
>
> Note also s/initialized/initialize/
>
> ---
>
> Creation and deletion of a vrrpv3OperationsTable row
>
> I'm slightly confused :-(
>
> We have...
>
>       vrrpv3OperationsEntry
>
>                Rows in the table cannot be modified unless the value
>
>                of 'vrrpv3OperStatus' has transitioned to
>                'initialize' state.
>
> and
>
>       vrrpv3OperationsRowStatus
>
>               When `vrrpv3OperationsRowStatus' is set to
>               active(1), no other objects in the conceptual row can
>               be modified.
>
> In general, the instructions in the description of the rowStatus are
> good an detailed. But I have some difficulty with row creation. What
> I think is missing is a statement that the row must be created with
> operStatus set to initialize(1) and cannot transition to backup(2) or
> master(3) until rowStatus is transitioned to active(1).
>
> Similarly, there is an issue with row deletion. In order to delete,
> the row I must first set rowStatus to notInService(2), and later to
> delete(6). But, according to the description of operStatus, I cannot
> modify the row (including the rowStatus) until operStatus has gone
> to initialize(1). How do I get that to happen?
>
> I suspect this can be fixed by allowing rowStatus to be changed
> regardless of the value of operStatus.
>
> ---
>
> vrrpv3AssociatedIpAddrTable
>
>               "The table of addresses associated with this virtual
>                router."
>
> I think that there is just one table, and it contains the addresses
> of all virtual routers. What about...
>
>               "The table of addresses associated with each virtual
>                router."
>
> ---
>
> vrrpv3AssociatedIpAddrEntry
>
> OLD
>               Rows in the table cannot be modified unless the value
>               of `vrrpv3OperStatus' has transitioned to
>               `initialize'.
> NEW
>               Rows in the table cannot be modified unless the value
>               of `vrrpv3OperStatus' for the corresponding entry in the
>               vrrpv3OperationsTable has transitioned to initialize(1).
> END
>
> ---
>
> vrrpv3AssociatedIpAddr
>
> This object's name is odd given the convention for naming objects
> within their tables. You probably need:
>
> vrrpv3AssociatedIpAddrAddress
>
> ---
>
> vrrpv3AssociatedIpAddr
>
> You should add a statement the description that says that the
> content of the object is to be interpreted in the context of the
> setting of vrrpv3OperationsInetAddrType in the index of this row.
>
> ---
>
> VRRP Router Statistics
>
> Do you need a discontinuity timer for the three global objects:
> - vrrpv3RouterChecksumErrors
> - vrrpv3RouterVersionErrors
> - vrrpv3RouterVrIdErrors
>
> ---
>
> VRRP Router Statistics
>
> In the presence of an attack or a broken router or host nearby, is
> it possible that Countr32 will not be large enough for the up-time
> of this router?
>
> You can choose to use Counter64 or describe wrapping conditions.
>
> ---
>
> vrrpv3RouterVrIdErrors
>
>               "The total number of VRRP packets received with an
>               invalid VRID for this virtual router."
>
> This object is global, so it is wider than the scope of a single VR.
> I think you need:
>
>               "The total number of VRRP packets received with a
>               VRID that is not valid for any virtual router on this
>               router."
>
> ---
>
> vrrpv3StatisticsAdvIntervalErrors
>
>               "The total number of VRRP advertisement packets
>               received for which the advertisement interval is
>               different than the one configured for the local virtual
>               router.
>
> Can you add a reference to vrrpv3OperationsAdvInterval
>
> ---
>
> vrrpv3ProtoError
>
> Don't you think this is a *really* dangerous notification?
>
> If a VR is under attack or receiving packets from a faulty speaker,
> it will spew notifications.
>
> You should probably either add some thresholding objects (which is a
> fair bit of work) or a single object to turn notifications on and
> off (with the default being "off").
>
> ---
>
> Notifications
>
> As currently specified, both of the notifications will come from the
> management agent which will identify the physical router that
> sourced the notification, but not the VR.
>
> Don't you need to add some index values to the notifications as well?
>
> ---
>
> Section 11
>
> Since you are obsoleting RFC 2787, is it your intention to ask IANA
> to deprecate {mib-2 68} ?
>
> ---
>
> Would you please consider adding
>
> ---
>
> Section 12
>
> A reference to RFC 4001 needs to be added as it shows up in some
> REFERENCE clauses.
>
> ---
>
> Would you please consider adding a short section on migrating from
> VRRP-MIB to VRRPV3-MIB?
>
> _______________________________________________
> vrrp mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/vrrp
>
> Scanned by Check Point Total Security Gateway.
> _______________________________________________
> vrrp mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/vrrp