Subject: [VRRP] AD Review of draft-ietf-vrrp-unified-mib

From: Bill Fenner <[email-masked]>
Date: Fri, 10 Feb 2006 15:43:49 -0800

I've performed my AD Review of the unified MIB, with a copy of RFC 4181
open and my MIB hat on.  (Don't ask to see it, I have to close the door
to my office when I am wearing it.)

Here are my comments.  Despite the number of comments, I think each one is
reasonably easy to resolve.  I'm happy to discuss any point, or to learn
that there's information that I didn't consider (e.g., compatability
issues, etc.) in my suggestions.

Thanks,
  Bill

MIB:

1. The REVISION clause from RFC 2338 needs to be retained.
The more recent REVISION clause should be last and should describe
the changes.

2. Every deprecated object needs an explanation in the DESCRIPTION
saying why it's deprecated (RFC 2578, section 10.2 (3)).  Text like
"This object is deprecated in favor of the IP Version Independent object,
vrrpFoobarObject" seems appropriate.  Similarly for the deprecated Groups.

3. The DESCRIPTION for vrrpOperationsMasterIpAddr seems to have had an
editing error (I realize that it's copied from vrrpOperMasterIpAddr).
The current wording implies that it could be set to the source of a
packet from a backup.  The DESCRIPTION also doesn't say that it's set
to my address when I'm the master, but I think that's what happens.

4. In the vrrpAssociatedIpAddrTable, I think that we should
delete the vrrpAssociatedInetAddrType and replace it in the
INDEX with vrrpOperationsInetAddrType.  (RFC 4181, section 4.6.4;
vrrpAssociatedIpAddrTable is an "expansion table")

5. The DESCRIPTION of vrrpAssociatedIpAddrRowStatus says that setting
it to createAndGo(4) results in administratively bringing down the row.
That's an interesting implication of createAndGo ;-)  Did you mean
notInService(2)?

6. The relationship between vrrpOperationsTable and
vrrpAssociatedIpAddrTable seems complex when creating rows.  I think
one sensible order of operations is:
  1. Create a row in vrrpOperationsTable with createAndWait(5).
  2. Create one or more corresponding rows in vrrpAssociatedIpAddrTable
  3. set vrrpOperationsRowStatus to active(1).
Is this what's intended?

7. Nothing says what happens when you delete all the rows from the
vrrpAssociatedIpAddrTable that correspond to a given row in the
vrrpOperationsTable.  My guess is that vrrpAssociatedIpAddrRowStatus
should say that you cannot delete the only row associated with an active
row in the vrrpOperationsTable.

8. vrrpAssociatedIpAddrTable and vrrpOperationsTable need either
  a) to specify in the Entry DESCRIPTION what happens to dynamically
     created rows after an agent restart, or
  b) to gain a StorageType column
(RFC4181 section 4.6.4)

9. vrrpAssociatedIpAddrRowStatus needs to say whether or not you can
modify the values when it's active(1).  (RFC 4181 section 4.6.4)

10. Please go through and add REFERENCE clauses where possible, so that
it's easy for an implementor to find the section of the spec that applies
to a given object.  (RFC 4181, section 4.6.2)

11. From the text, of the DESCRIPTION, it's hard to tell the difference
between vrrpMIBCompliance2 and vrrpMIBReadOnlyCompliance.  See, for
example, diffServMIBReadOnlyCompliance in RFC3289 for a more complete
DESCRIPTION.

12. In the security considerations, I think the vrrpOperationsIpAddrTable
also needs to be listed with read-create.  Since you don't permit changing
any columns while a row is active, it's true that the RowStatus column
is the only one that needs to be listed, but it might be nice to say
something like "While there are other columns that, if changed, could
disrupt operations, they can not be changed without first changing the
RowStatus object".  (Also, you say "RowState" and not "RowStatus" when
referring to the column.)

Formatting:

1. Needs the description of The Internet-Standard Management Framework
from http://www.ops.ietf.org/mib-boilerplate.html

2. Title of section 3 should probably be "Relationship to RFC 2787"

3. Should the introduction to the scenario in section 8 include something
saying that A, B and C are IPv4 addresses and X, Y and Z are IPv6?

4. Is it feasible to the deprecated objects to the end (or, rather,
middle, just before the groups) to make it more readable for someone
who wants to come and see the current stuff?

5. The comment above vrrpTrapAuthFailure refers to it as vrrpAuthFailureTrap.

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

---

From: [email-masked]
Date: Mon, 13 Feb 2006 11:25:22 -0800

HI Bill,
Thanks For taking time to review the draft. I think the MIB hat looks
great on you :). These are excellent comments.  

I will get back to you if I need more clarifications, All the comments
look valid to me.

Thanks
Kalyan

-----Original Message-----
From: ext Bill Fenner [mailto:[email-masked]] 
Sent: Friday, February 10, 2006 3:44 PM
To: [email-masked]
Cc: Tata Kalyan (Nokia-ES/MtView)
Subject: AD Review of draft-ietf-vrrp-unified-mib

Here are my comments.  Despite the number of comments, I think each one
is reasonably easy to resolve.  I'm happy to discuss any point, or to
learn that there's information that I didn't consider (e.g.,
compatability issues, etc.) in my suggestions.

Thanks,
  Bill

MIB:

1. The REVISION clause from RFC 2338 needs to be retained.
The more recent REVISION clause should be last and should describe the
changes.

2. Every deprecated object needs an explanation in the DESCRIPTION
saying why it's deprecated (RFC 2578, section 10.2 (3)).  Text like
"This object is deprecated in favor of the IP Version Independent
object, vrrpFoobarObject" seems appropriate.  Similarly for the
deprecated Groups.

3. The DESCRIPTION for vrrpOperationsMasterIpAddr seems to have had an
editing error (I realize that it's copied from vrrpOperMasterIpAddr).
The current wording implies that it could be set to the source of a
packet from a backup.  The DESCRIPTION also doesn't say that it's set to
my address when I'm the master, but I think that's what happens.

4. In the vrrpAssociatedIpAddrTable, I think that we should delete the
vrrpAssociatedInetAddrType and replace it in the INDEX with
vrrpOperationsInetAddrType.  (RFC 4181, section 4.6.4;
vrrpAssociatedIpAddrTable is an "expansion table")

5. The DESCRIPTION of vrrpAssociatedIpAddrRowStatus says that setting it
to createAndGo(4) results in administratively bringing down the row.
That's an interesting implication of createAndGo ;-)  Did you mean
notInService(2)?

6. The relationship between vrrpOperationsTable and
vrrpAssociatedIpAddrTable seems complex when creating rows.  I think one
sensible order of operations is:
  1. Create a row in vrrpOperationsTable with createAndWait(5).
  2. Create one or more corresponding rows in vrrpAssociatedIpAddrTable
  3. set vrrpOperationsRowStatus to active(1).
Is this what's intended?

10. Please go through and add REFERENCE clauses where possible, so that
it's easy for an implementor to find the section of the spec that
applies to a given object.  (RFC 4181, section 4.6.2)

12. In the security considerations, I think the
vrrpOperationsIpAddrTable also needs to be listed with read-create.
Since you don't permit changing any columns while a row is active, it's
true that the RowStatus column is the only one that needs to be listed,
but it might be nice to say something like "While there are other
columns that, if changed, could disrupt operations, they can not be
changed without first changing the RowStatus object".  (Also, you say
"RowState" and not "RowStatus" when referring to the column.)

Formatting:

3. Should the introduction to the scenario in section 8 include
something saying that A, B and C are IPv4 addresses and X, Y and Z are
IPv6?

4. Is it feasible to the deprecated objects to the end (or, rather,
middle, just before the groups) to make it more readable for someone who
wants to come and see the current stuff?

5. The comment above vrrpTrapAuthFailure refers to it as
vrrpAuthFailureTrap.