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

["Kalyan (Srinivas)Tata" <[e-mail]>]

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.