Re: AD review of draft-ietf-vrrp-unified-mib
Mukesh Gupta <[email protected]> Mon, 10 Jan 2011 14:56:25 -0800
| Newsgroups | gmane.ietf.vrrp |
|---|---|
| Message-ID | <[email protected]> |
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" <[email protected] > 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: [email protected] [mailto:[email protected]] On Behalf > Of Adrian Farrel > Sent: Saturday, January 08, 2011 11:37 AM > To: [email protected] > Cc: [email protected]; [email protected] > 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 > [email protected] > https://www.ietf.org/mailman/listinfo/vrrp > > Scanned by Check Point Total Security Gateway. > _______________________________________________ > vrrp mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/vrrp _______________________________________________ vrrp mailing list [email protected] https://www.ietf.org/mailman/listinfo/vrrp