[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
FW: MIB Dr. Review of draft-ietf-vrrp-unified-mib-05.txt
FYI, Joan's review of draft-ietf-vrrp-unified-mib-05.txt.
-----Original Message-----
From: jcucchiara@mindspring.com [mailto:jcucchiara@mindspring.com]
Sent: Thursday, June 22, 2006 11:11 PM
To: Romascanu, Dan (Dan); kalyan.tata@nokia.com
Cc: vrrp@ietf.org; Bill Fenner; jcucchiara@mindspring.com
Subject: MIB Dr. Review of draft-ietf-vrrp-unified-mib-05.txt
Hello Kalyan,
Below are comments on the unified MIB.
I listed errors and warnings first, but these are also discussed in
detail within the comments.
Thanks,
Joan
smicng Compiler Issues
----------------------
1) Did not strip out the MIB module
correctly using "mstrip".
(I am not sure if this is really an
issue or not???)
2) E: f(VRRP-MIB), (151,20) Item "vrrpOperationsVersion" in
sequence "VrrpOperationsEntry" has conflicting syntax specified
This needs to be fixed.
3) Warnings:
I have addressed these warnings within the MIB comments below.
W: f(VRRP-MIB), (423,8) Row "vrrpAssociatedIpAddrEntry" has
indexing that may create variables with more than 128 sub-ids
W: f(VRRP-MIB), (132,23) Row "vrrpOperationsEntry" does not have a
consistent indexing scheme - index items from current table must
come after index items from other tables
W: f(VRRP-MIB), (439,23) Row "vrrpAssociatedIpAddrEntry" does not
have a consistent indexing scheme - cannot specify an index
item from additional "base row" vrrpOperationsEntry, since can
have only one "base row" which is ifEntry
W: f(VRRP-MIB), (563,23) Row "vrrpRouterStatisticsEntry" does not
have a consistent indexing scheme - cannot specify an index item
from additional "base row" vrrpOperationsEntry, since can
have only one "base row" which is ifEntry
smiLint Compiler Issues
-----------------------
Line Severity Problem
423 5 warning: index of row `vrrpAssociatedIpAddrEntry'
can exceed OID size limit by 142 subidentifier(s)
(flagged by smicng also)
Comments on document
-----------------------
1) Please explain what the term "unified" means in this document. It is
used in the title in and some object DESCRIPTIONS but nowhere do you
specify exactly what this term means. This term does not appear in
other VRRP documents, so think it needs to be clarified.
2) Please remove the term "trap" and use notification.
3) Use of terminology obsoleted and deprecated.
Abstract states:
... This memo obsoletes RFC 2787.
Section 4. Relationship to RFC 2787
RFC2787 [RFC2787] defines managed objects for VRRP over IPv4 and has
been deprecated by this memo.
When referring to RFC2787 document,
please consistently use the term "obsoleted". As in the document is now
"obsoleted" by this new RFC.
When referring to specific MIB objects, please use the term deprecate.
MIB MODULE
=================================================================
4) RFC4181 (MIB Author Guidelines)
suggests using the following for the start of the OID tree:
xxxMIB
|
+-- xxxNotifications(0)
+-- xxxObjects(1)
+-- xxxConformance(2)
|
+-- xxxCompliances(1)
+-- xxxGroups(2)
Currently, the OID tree is:
vrrpOperations OBJECT IDENTIFIER ::= { vrrpMIB 1 }
vrrpStatistics OBJECT IDENTIFIER ::= { vrrpMIB 2 }
vrrpConformance OBJECT IDENTIFIER ::= { vrrpMIB 3 }
vrrpNotifications OBJECT IDENTIFIER ::= { vrrpMIB 0 }
vrrpMIBCompliances OBJECT IDENTIFIER ::= { vrrpConformance 1 }
vrrpMIBGroups OBJECT IDENTIFIER ::= { vrrpConformance 2 }
You would need to create a vrrpObjects(1) branc and then have
vrrpOperations and vrrpStatistics off of this.
5) I consulted with other MIB Doctors and the rough consensus was to
leave deprected objects where they are in the MIB, even though they are
deprecated but to make it very obvious that they are deprected.
Also, Guidelines state to add the reason why they have been deprecated
to the DESCRIPTION.
vrrpNodeVersion OBJECT-TYPE -- **** DEPRECATED OBJECT ****
SYNTAX Integer32
MAX-ACCESS read-only
STATUS deprecated
DESCRIPTION
"This value identifies the particular version of the VRRP
supported by this node.
This object is deprecated in the IP Version Independent
MIB.
This object has been deprecated and replaced by
the vrrpOperationsVersion object in the
vrrpOperationsTable."
::= { vrrpOperations 1 }
Also, please add to the DESCRIPTION clause why the object has been
deprecated/obsoleted.
6) REVISION "200605020000Z" -- 23 Apr 2006
Comment for date is incorrect
7) Please use SMI comments, i.e. -- for RFC editor notes in the body of
the MIB Module.
Currently:
DESCRIPTION
"IP version neutral revision as published in xxxx (RFC-
editor this needs to be corrected with the assigned RFC
number)."
Please specify RFC Ed.note as:
--
-- RFC Ed.: In the following text, please replace XXX
-- with actual RFC number and remove this note
DESCRIPTION
Also, suggested text of this DESCRIPTION:
"Revised version, published as RFC XXXX.
This revision provides MIB support for
the Virtual Router Redundancy Protocol (VRRP)
in both IPv4 and IPv6 environments."
Please give some detail on what
revisions were made. For an example, please see the REVISION clause in
rfc2515.
As one example:
- 'vrrpNodeVersion' scalar objects has been deprecated and
is replaced by the vrrpOperationsVersion object
in the vrrpOperationsTable.
8) Could notifications be generated or not, using the
the notification MIB and target MIB (RFC2583)?
I don't see a need for this object, but please let me
know the answer to the above question.
9) vrrpOperationsTable DESCRIPTION clause is
not particularly helpful. You are describing that a
table consists of rows.
10) vrrpOperationsEntry DESCRIPTION clause
Note that rows in this table can be distinguished on a
multi-stacked device running VRRP over IPv4 and
IPv6 on the same interface.
Are you saying that this is on the same physical interface?
If so, please add the word "physical".
Again, with regard to this DESCRIPTION clause:
Rows in the table cannot be modified unless the value
of 'vrrpOperationsState' has transitioned to
'initialize'"
Why was the ability to change the OperationsState removed?
In other words, the original table had an AdminState object, but this
table does not. Please explain. Typically, there is an AdminState type
object so that an operator is able to force a transition to a different
state.
11) INDEX { vrrpOperationsInetAddrType,
vrrpOperationsVrId, ifIndex }
W: f(VRRP-MIB), (132,23) Row "vrrpOperationsEntry" does not have a
consistent indexing scheme - index items from current table must
come after index items from other tables
Wrt to the above warning, did want to ask about the indexing used here?
In the original MIB ifIndex appears first, so why is this changed here.
Also, there is a blurb in the document itself which says that ifIndex
represents the physical interface, is that still the case with this
ifIndex? If so, please mention this someplace in the Table or Entry
DESCRIPTION clause.
12) vrrpOperationsPriority OBJECT-TYPE
SYNTAX Integer32 (0..255)
Please change this type to Unsigned32.
What kind of error would be returned if an attempt is made to set this
object to 0?
Would think this is an inconsistentValue error. Please add to the
description the type of error is an object is set to 0.
13)
vrrpOperationsVersion OBJECT-TYPE
SYNTAX INTEGER {
vrrpv2 (1),
vrrpv3 (2)
}
MAX-ACCESS read-create
STATUS current
DESCRIPTION
"This object contains the VRRP version on which this
VRRP instance is running."
::= { vrrpOperationsEntry 6 }
Wanted to check and make sure that the version of the VRRP protocol is
settable (object is read-create).
Did you intend for this to be read-only or read-create?
The DESCRIPTION makes the object sound like a read-only object.
Also, would the value of vrrpOperationsInetAddrType dictate the version?
Not sure why this object is necessary.
14)
vrrpOperationsAddrCount OBJECT-TYPE
SYNTAX Integer32 (0..255)
Could you explain why there would only be a maximum of 255? Also, this
should probably be Gauged32(0..255)
Would also change the name of the object since it does not really seem
to be a counter. Maybe something like vrrpOperationsCurrentAddrs
15) vrrpOperationsPrimaryIpAddr OBJECT-TYPE
Does this object have a default value?
The original object does, so wanted to check on that.
16) vrrpOperationsAcceptMode OBJECT-TYPE
Does this object have any meaning for IPv4?
Please add to the DESCRIPTION.
17) vrrpOperationsUpTime OBJECT-TYPE
SYNTAX TimeStamp
MAX-ACCESS read-only
STATUS current
DESCRIPTION
"This is the value of the `sysUpTime' object when this
virtual router (i.e., the `vrrpOperState') transitioned
out of `initialized'."
REFERENCE " RFC 3768 section 6.1"
::= { vrrpOperationsEntry 13 }
The above description refers to vrrpOperState but there isn't an object
with this name. Please fix.
18) vrrpOperationsStorageType OBJECT-TYPE
The description is confusing. I would think that most implementations
would want to support nonVolatile(3) or something even more stable.
Please explain the choice of volatile(2)? Also, please consider adding
a DEFVAL for this object.
19) vrrpOperationsRowStatus OBJECT-TYPE
Please consistently use the term RowStatus in the DESCRIPTION clause (it
appears now as "row status" and rowstatus).
Please explain how "the vrrpOperationsTable is constrained by the
operational state of the corresponding virtual router." In other words,
are you telling us something about the value of the vrrpOperationsState
object?
Please change the 'C' to 'c' in "Corresponding instand of".
notInService(2) should be used to administratively bring the row down,
does this have an impact on the vrrpOperationsState object?
(I think this gets back to my point of having an Admin object, the
notInService refers to the row not being available for a manager, is
this what you mean, or are you trying to say that the vrrp is
notInService ??
Adding a Row:
Please list again the objects which need to have valid values for #2.
2. Populate the vrrpOperationsEntry with at least
minimal elements.
Are steps 2 and 3 out of order?
Deleting a row:
Can the same row in the vrrpAssociatedIpAddrTable be used by more than
one row in the vrrpOperationsTable?
Also, if you set the vrrpAssociatedIpAddrTable row to notInService(2),
do you need to go back and delete it, after you delete the corresponding
row in the vrrpOperationsTable?
20) vrrpAssociatedIpAddrTable OBJECT-TYPE
DESCRIPTION:
"associated with a virtual router."
21) INDEX { vrrpOperationsInetAddrType,
vrrpOperationsVrId,
ifIndex, vrrpAssociatedIpAddr }
W: f(VRRP-MIB), (423,8) Row "vrrpAssociatedIpAddrEntry" has
indexing that may create variables with more than 128 sub-ids
The issue here is that the Address (InetAddress) is 0..255 octets, but
really only need either 4 octets (IPv4 address) or
16 octets (IPv6 address). Please limit the size of this using
SIZE(0|4|16) (as an example).
DIFFSERV-MIB (and others) give an
example of how to limit the size in the conformance (and also limit the
address type). This needs to be added to the conformance section.
W: f(VRRP-MIB), (439,23) Row "vrrpAssociatedIpAddrEntry" does not
have a consistent indexing scheme - cannot specify an index
item from additional "base row" vrrpOperationsEntry, since can
have only one "base row" which is ifEntry
Again, would like to ask why the change from the original MIB wrt to the
order of the indexes? Also, it is better to try and have InetAddrType
and Addr objects next to each other.
Also, please specify which ifIndex in either the Table/Entry DESCRIPTION
clause.
22) vrrpAssociatedStorageType
Same questions/concerns about StorageType here as in the previous table.
23) vrrpAssociatedIpAddrRowStatus
Again, it seems that RowStatus is taking on some aspects of what an
AdminStatus object would do.
This will become more clear when I see your responses from the previous
table regarding that RowStatus object.
24) vrrpRouterStatisticsTable
The index is the same as the vrrpOperationsTable so have a question as
to why this table does not use AUGMENTS?
W: f(VRRP-MIB), (563,23) Row "vrrpRouterStatisticsEntry" does not
have a consistent indexing scheme - cannot specify an index item
from additional "base row" vrrpOperationsEntry, since can
have only one "base row" which is ifEntry
Also, as discussed above, have a question as to why ifIndex is not used
first?
25) naming of objects in vrrpRouterStatisticsTable: Counters usually
end in "s". Some of these counters do, so please try to do this, for
example: vrrpStatisticsBecomeMaster could be
vrrpStatisticsMasterTransitions,
vrrpStatisticsAdvertiseRcvd could be vrrpStatisticsRcvdAdvertisements
and so forth.
vrrpStatisticsInvldTypePktsRcvd could be
vrrpStatisticsRcvdInvalidTypePkts
(Also, please spell out some of these names, such as Priority/Pri,
Advertisement/Adv).
26) vrrpStatisticsRefreshRate
Not sure that I fully understand the benefit of this object.
Could you please explain why this is needed for vrrp?
27)vrrpStatisticsDiscontinuityTime
Please make this the last object in the entry. In other words, please
make sure that all the counter objects appear prior to this and
RefreshRate.
28) Use of the term trap: please use the term notification.
The exception will be the deprecated traps which were named prior to
this draft.
29) Concern on the use of accessible-for-notify: it might be a good
idea to make these objects read-only, along with a timestamp type of
object. Notifications are unreliable and so, could be lost. Also, some
customers do not like to enable a lot of notifications and prefer
polling.
Would you consider making these objects read-only?
30) --
-- deprecated objects follow.
--
As mentioned previously, please leave the original objects in place and
add to the DESCRIPTION why they were deprecated.
For the table objects, should be fine to add to the Table's DESCRIPTION
(rather than each individual object).
31) Please rename vrrpMIBCompliance2 to vrrpModuleFullCompliance
32) Please change the order of the conformance objects to match the
objects in the MIB.
33) DESCRIPTION "SETable values are from 1 to 255."
s/SETable/Settable
34) name change for vrrpMIBReadOnlyCompliance to
vrrpModuleReadOnlyCompliance
35) some objects are missing under the ReadOnlyCompliance Please check
this: vrrpOperationsVersion, vrrpOperationsPreemptMode
vrrpOperationsAcceptMode and maybe others.
36) 11. Security Consideration
"A number of objects in the vrrpOperationsTable and
vrrpAssociatedIpAddrTable possess the read-create attribute.
Manipulation of these objects is capable of affecting the operation
of a virtual router."
Please list the objects in the vrrpOperationsTable. This section needs
to be very explicit.