Subject: [manet] [Gen-art] review: draft-ietf-manet-nhdp-mib-12

From: "Joel M. Halpern" <[email-masked]>
Date: Fri, 06 Apr 2012 11:59:06 -0400

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-manet-nhdp-mib-12
     Definition of Managed Objects for the
         Neighborhood Discovery Protocol
Reviewer: Joel M. Halpern
Review Date: 6-April-2012
IETF LC End Date: 16-April-2012
IESG Telechat date: (if known)

Summary: This document is almost ready for publication as a Proposed 
Standard.

Major issues:
     Section 5.1.3.1 on Ignoring Initial Activity is trying to do a very 
reasonable thing, namely suppress notifications for activity which is 
expected.  The text references RFC 4750 as precedent.  RFC 4750 is clear 
that the suppress window is tied to specific events (interface up and 
election as a DR.)  Section 5.1.3.1 does not specify which condition(s) 
start(s) the suppress window.  If, as seems likely, it is Interface Up 
which starts the window, please state that explicitly in the text.

     In section 5.4, in addition to describing objects which are defined 
in the MIB, the text describes, under the heading "The following objects 
return statistics related to HELLO messages:", a number of what it 
refers to as "Derived Objects".  These do not appear to be actual 
elements of the MIB.  They appear rather to be descriptions of 
calculations which the manager can perform using the information from 
the MIB.  It is not at all clear why they are here.  If I am 
understanding their role properly, and if they belong in this document, 
they belong in some other section, as they are NOT objects which return 
statistics related to HELLO messages.  They appear not to be returned by 
the managed device at all.

Minor issues:
     I can not find the object that corresponds to the setting for 
Ignoring the Initial Activity.  I presume this is my error.  The 
document would be helped if the object were named in section 5.1.3.1.

     I believe section 5.1.3.2 on Throttling Traps is intended to refer 
to the StateChange Threshold and StateChangeWindow objects.  It would be 
very helpful if these were actually named in section 5.1.3.2.

     Most MIBs I review have a description of the tables they contain, 
how the tables relate to each other, and how they are indexed, in the 
front matter that is roughly equivalent to section 5.2, 5.3, and 5.4. 
As I am not a MIB Doctor, I do not know if that is formally required, 
but I find it very helpful, and am surprised not to see it here.

     In looking at the fields in the NhdpInterfaceEntry, some of the 
field definitions include some of the constraints from RFC 6130 section 
5 in their DESCRIPTION clauses.  Some do not.  (For exampple, 
REFRESH_INTERVAL >= HELLO_INTERVAL is captured in nhdbpRefreshInterval, 
but not in nhdpHelloInterval.  The requirement that nhdpHelloInterval be 
greater than 0 is not captured anywhere.  Neither is H_HOLD_TIME >= 
REFRESH_INTERVAL captured.)  Some elements have a statement that the 
object is persistent, while others do not, but these do not seem to 
correspond to a difference in RFC 6130.  It is possible that there is a 
good reason for this apparent variation.  Is there?

     Particularly for top-level objects such as nhdpNHoldTime and 
NhdpIHoldTime I would really like to see a better description than just 
this is <named> object from section 5 of RFC 6130.  Someone who is using 
the MIB, who is looking at the description clause for assistance, really 
needs something more than the name of the field in the MIB.  (I think 
better descriptions would be a good idea through much of the MIB.)

Nits/editorial comments:
     The tracker claims this is "In WG Last Call (manet), but also seems 
to indicate that it is in IETF Last Call.  Are the two happening at the 
same time?

---

From: Ulrich Herberg <[email-masked]>
Date: Fri, 27 Apr 2012 09:56:49 -0700

Dear Joel,

thank you very much for your review and apologies for our late reply. Find
our answers below, and
please tell us if they address your comments.

 ---------- Forwarded message ----------
From: Joel M. Halpern <[email-masked]>
Date: Fri, Apr 6, 2012 at 8:59 AM
Subject: [manet] [Gen-art] review: draft-ietf-manet-nhdp-mib-12
To: [email-masked]
Cc: [email-masked], "A. Jean Mahoney" <[email-masked]>,
[email-masked]

> Summary: This document is almost ready for publication as a Proposed

<nhdp-mib-authors>
That's good :-)
</nhdp-mib-authors>

> expected.  The text references RFC 4750 as precedent.  RFC 4750 is
> clear that the suppress window is tied to specific events (interface
> up and election as a DR.)  Section 5.1.3.1 does not specify which
> condition(s) start(s) the suppress window.  If, as seems likely, it is
> Interface Up which starts the window, please state that explicitly in
> the text.

<nhdp-mib-authors>
How about adding the following sentence at the end of
the paragraph of 5.1.3.1:
"The suppression window for notifications is started when the 'nhdpIfStatus'
transitions
 from its default value of 'false' to 'true'."
</nhdp-mib-authors>

> in the MIB, the text describes, under the heading "The following
> objects return statistics related to HELLO messages:", a number of
> what it refers to as "Derived Objects".  These do not appear to be
> actual elements of the MIB.  They appear rather to be descriptions of
> understanding their role properly, and if they belong in this
> document, they belong in some other section, as they are NOT objects
> which return statistics related to HELLO messages.  They appear not to
> be returned by the managed device at all.

<nhdp-mib-authors>
Yes, you are right. We we pull these from the draft and
save the text and possibly use elsewhere in the future.
</nhdp-mib-authors>

> document would be helped if the object were named in section 5.1.3.1.

<nhdp-mib-authors>
We can change 'HELLO_INTERVAL' in Section 5.1.3.1 to 'nhdpHelloInterval'.
</nhdp-mib-authors>

> to the StateChange Threshold and StateChangeWindow objects.  It would
> be very helpful if these were actually named in section 5.1.3.2.

<nhdp-mib-authors>
How about adding the following sentence to 5.1.3.2:
The following objects are used to define the thresholds and time
windows for specific Notifications defined in the NHDP-MIB module:
nhdpNbrStateChangeThreshold,
nhdpNbrStateChangeWindow, nhdp2HopNbrStateChangeThreshold,
nhdp2HopNbrStateChangeWindow,         nhdpIfRxBadPacketThreshold,
nhdpIfRxBadPacketWindow.
</nhdp-mib-authors>

> but I find it very helpful, and am surprised not to see it here.

<nhdp-mib-authors>
We agree that this is probably a good practice to follow and will work up
text to handle this.
</nhdp-mib-authors>

> field definitions include some of the constraints from RFC 6130
> section 5 in their DESCRIPTION clauses.  Some do not.  (For exampple,
> REFRESH_INTERVAL >= HELLO_INTERVAL is captured in
> nhdbpRefreshInterval, but not in nhdpHelloInterval.  The requirement
> that nhdpHelloInterval be greater than 0 is not captured anywhere.
>  Neither is H_HOLD_TIME >= REFRESH_INTERVAL captured.)  Some elements
> have a statement that the object is persistent, while others do not,
> but these do not seem to correspond to a difference in RFC 6130.  It
> is possible that there is a good reason for this apparent variation.
>  Is there?

<nhdp-mib-authors>
That's true. We will go through all constraints from NHDP and
add them to the MIB.
</nhdp-mib-authors>

> NhdpIHoldTime I would really like to see a better description than
> just this is <named> object from section 5 of RFC 6130.  Someone who
> is using the MIB, who is looking at the description clause for
> assistance, really needs something more than the name of the field in
> the MIB.  (I think better descriptions would be a good idea through
> much of the MIB.)

<nhdp-mib-authors>
We will can look at the descriptions and copy some more text from
NHDP. However, we would like to avoid copying all NHDP into the MIB.
</nhdp-mib-authors>

> to indicate that it is in IETF Last Call.  Are the two happening at
> the same time?

<nhdp-mib-authors>
We actually don't know, and will ask the chairs about that.
</nhdp-mib-authors>

Best regards
Ulrich and Bob

---

From: "Joel M. Halpern" <[email-masked]>
Date: Fri, 27 Apr 2012 14:03:55 -0400

Pending actual text availability, the approaches described seem to 
recognize the issues and plan to address them appropriately.
I look forward to seeing the result when it is ready,
Thank you,
Joel

On 4/27/2012 12:56 PM, Ulrich Herberg wrote:
> Dear Joel,
>
> thank you very much for your review and apologies for our late reply.
> Find our answers below, and
> From: Joel M. Halpern <[email-masked] <mailto:[email-masked]>>
> To: [email-masked] <mailto:[email-masked]>
> Cc: [email-masked] <mailto:[email-masked]>, "A. Jean Mahoney"
> <[email-masked] <mailto:[email-masked]>>,
> [email-masked] <mailto:[email-masked]>
>
>
> the paragraph of 5.1.3.1 <http://5.1.3.1>:
> How about adding the following sentence to 5.1.3.2 <http://5.1.3.2>:
> We agree that this is probably a good practice to follow and will work
> up text to handle this.
> </nhdp-mib-authors>
>
>