Subject: [VRRP] MIB Dr. Review of draft-ietf-vrrp-unified-mib-05.txt

From: <[email-masked]>
Date: Thu, 22 Jun 2006 16:10:54 -0400

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.

_______________________________________________
vrrp mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/vrrp

---

From: <[email-masked]>
Date: Mon, 26 Jun 2006 11:50:56 -0700

hi Joan,
Thanks for the detailed review. 
I will get back to you by tomorrow with any questions/comments I have.
Thanks
-kalyan

-----Original Message-----
From: ext [email-masked] [mailto:[email-masked]] 
Sent: Thursday, June 22, 2006 1:11 PM
To: Romascanu, Dan (Dan); Tata Kalyan (Nokia-ES/MtView)
Cc: [email-masked]; Bill Fenner; [email-masked]
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
----------------------

This needs to be fixed.

3) Warnings:

I have addressed these warnings within the MIB comments below.

smiLint Compiler Issues
-----------------------
Line Severity   Problem

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.

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)

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.

            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.

7)  Please use SMI comments, i.e. -- for RFC editor notes in the body of
the MIB Module.

Currently:
            DESCRIPTION

Please give some detail on what
revisions were made.  For an example, please see the REVISION clause in
rfc2515.

As one example:

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.

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.

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)

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)

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

The above description refers to vrrpOperState but there isn't an object
with this name.  Please fix.

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.

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?

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.

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?

DESCRIPTION:

21)            INDEX    { vrrpOperationsInetAddrType,
vrrpOperationsVrId,
                      ifIndex, vrrpAssociatedIpAddr }

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.

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.

Same questions/concerns about StorageType here as in the previous table.

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.

The index is the same as the vrrpOperationsTable so have a question as
to why this table does not use AUGMENTS?

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,

(Also, please spell out some of these names, such as Priority/Pri,
Advertisement/Adv).

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)    --

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).

32) Please change the order of the conformance objects to match the
objects in the MIB.

s/SETable/Settable

35) some objects are missing under the ReadOnlyCompliance Please check
this:  vrrpOperationsVersion, vrrpOperationsPreemptMode
vrrpOperationsAcceptMode and maybe others.

Please list the objects in the vrrpOperationsTable.  This section needs
to be very explicit.