> 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:
vrrp-bounces <at> ietf.org [mailto:
vrrp-bounces <at> ietf.org] On Behalf
> Of Adrian Farrel
> Sent: Saturday, January 08, 2011 11:37 AM
> To:
draft-ietf-vrrp-unified-mib <at> tools.ietf.org
> Cc:
vrrp-chairs <at> tools.ietf.org;
vrrp <at> ietf.org
> 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
>
vrrp <at> ietf.org
>
https://www.ietf.org/mailman/listinfo/vrrp
>
> Scanned by Check Point Total Security Gateway.
> _______________________________________________
> vrrp mailing list
>
vrrp <at> ietf.org
>
https://www.ietf.org/mailman/listinfo/vrrp
_______________________________________________
vrrp mailing list
vrrp <at> ietf.orghttps://www.ietf.org/mailman/listinfo/vrrp
RSS Feed