Subject: [manet] Fw: Response to Adrian's NHDP-MIB comments

From: "Cole, Robert G CIV USARMY CERDEC (US)" <[email-masked]>
Date: Mon, 26 Mar 2012 16:32:45 +0000

Adrian,  Joe,  Stan,

This should help in your decision for next step.  

Thanks,  Bob and Ulrich 

----- Original Message -----
From: Robert G Cole [mailto:[email-masked]]
Sent: Monday, March 26, 2012 04:23 PM
To: Cole, Robert G CIV USARMY CERDEC (US)
Subject: Response to Adrian's NHDP-MIB comments

Adrian,

Thank you very much for this thorough review. This is much appreciated!

Please see our comments below:

> State changed to AD Evaluation::Revised ID Needed from Publication
> RequestedAD review...
>
> Hi,
>
> Many apologies for a delayed AD review of this document. I love MIB
> modules deeply, but I did want to clear out the other MANET documents
> from the pipe before working on this one.
>
> As usual, the purpose of my review is to catch and clean up issues that
> might show later in the process, thereby saving community and IESG
> review effort, and giving the document a smoother ride through the
> system and a greater likelihood of approval.
>
> This is a substantial document and is generally solid. The number of
> comments reflects the fact that writing MIB modules is a real back-
> breaker. I think it is important to note that I did not have any
> concerns about the structure of the module or the tables (except one
> small suggestion about moving the TCs into a separate module). Most
> of the comments are at the level of MIB nits.
>
> All comments are, of course, up for discussion and debate.
>
> I think we need a new revision at this point. Then we can go to MIB
> Doctor review and IETF last call at the same time.
>
> Many thanks,
> Adrian
>
> ---
>
> 5.1.3.1
>
>   The majority of critical events occur when NHDP is enabled on a
>   router, at which time the symmetric neighbors and two-hop neighbors
>   of the NHDP router are discovered.
>
> I think we can clarify this because there are currently two
> interpretations:
>
> - when NHDP is operational (enabled)
> - when NHDP is initiated (enabled or the first time)
>
> >From context, I think you mean...
>
>   The majority of critical events occur when NHDP is first enabled on a
>   router, at which time the symmetric neighbors and two-hop neighbors
>   of the NHDP router are discovered.
>

Yes, agreed.

>
> ---
>
> 5.1.3.1
>
>   To avoid unnecessary notifications, a router should not
>   originate expected notifications until a certain time interval has
>   elapsed, which is to be predefined by the network manager.
>
> 1. Is this "SHOULD NOT"?
>

Yes.

> 2. Is there a recommended default for such a timer value? I think you
>   should give one if you can.
>

How about this:
It is RECOMMENDED that this time interval is at least 3 x HELLO_INTERVAL,
so that symmetric neighbors are discovered.

>
> ---
>
> 5.1.3.2
>
>   Appropriate values for the window time and upper bound are to be
>   selected by the network manager and depend on the deployment of the
>   MANET.
>
> I believe you here, but I would welcome some more guidance for the
> implementer and the deployer. Is there anything you can add?
>

We added that:
If NHDP is deployed on a lossy, wireless medium, sending too many
notifications in a short time interval may lead to collisions and dropped
packets. In particular, in dense deployments of NHDP routers (i.e. where
each router has many neighbors), a change of the local topology may trigger
many notifications at the same time. [RFC4750] recommends "7 notifications
with a window time of 10 seconds" as upper bound. As NHDP is expected to be
deployed in more lossy channels than OSPF, it is RECOMMENDED to choose a
lower threshold for the number of notifications per time than that.

>
> ---
>
> 5.1.3.3
>
> OLD
>   Similar to the according mechanism in [RFC4750], only one
>   notification is sent per event.
> NEW
>   Similar to the mechanism in [RFC4750], only one notification is sent
>   per event.
> END
>

OK.

>
> ---
>
> Section 6
>
>   This section specifies the relationship of the MIB modules contained
>
> s/modules/module/
>

OK.

>
> ---
>
> You have included Section 6.3 which is perfect.
> In the light of this, there is a body of opinion amongst "MIB experts"
> that citation notation should not be used in the body of the MIB
> module. This is because the module will be ripped from the document
> for compilation and passed around as a stand-alone piece of text.
>
> Thus, just take the square brackets out form the comments in the
> IMPORT clauses.
>
> Make the same change to any Description and Reference clauses.
>

OK.

>
> ---
>
> Don't forget to make any necessary changes to the CONTACT-INFO clause.
>

I (Ulrich) have updated Stan's email address. I have agreement with Thomas and my
new employer to leave my old affiliation on this document.

>
> ---
>
> The copyright date in the MODULE-IDENTITY clause is a little out-of-
> date.
>

OK, changed to 2012.

>
> ---
>
> For sound reasons, revisions of the MIB module posted in Internet-Drafts
> are not considered as "Revisions". That is, per the boilerplate, it is
> inappropriate to use Internet-Drafts as reference material or to cite
> them other than as "work in progress.
>
> That means that the first revision of the MIB Module is the one that
> gets published in the RFC.
>
> Can you tidy the Revision clauses so that there is only one, and it
> applies to the RFC-that-will-be. The RFC Editor will fix up the date on
> publication.
>
>

Good point! Fixed.

> ---
>
> NeighborIfIndex
>
> I'm not saying what you have is wrong, but why did you decide to not
> assign this TC the Syntax InterfaceIndex?
>

We agree that this is somewhat unusual, but we keep this as is.  However, 
we did modify the TC Description to point out that this is not persistent
between re-initializations.  This is different from the semantics of the
'InterfaceIndex'.  As NHDP process discover more about it's neighborhood
it may in fact realize that what it originally thought were two interfaces
on neighbors is in fact a single interface and thus needs to merge its
understanding of the interfaces.

>
> ---
>
> For nhdpInterfaceTable I think it is necessary to say that when the
> corresponding entry with ifIndex value is deleted from the Interface
> Table, the entry in this table is automatically deleted.
>

OK, added.

>
> ---
>
> nhdpIfIndex has Syntax InterfaceIndexOrZero, but the Description says
> "The ifIndex for this interface."
>
> Note that according to RFC 2863, ifIndex has Syntax InterfaceIndex.
>
> So you need to decide to either:
> - change the Syntax of nhdpIfIndex
>

Changed to InterfaceIndex.

> or
> - add an explanation of what it means to have a zero value
>
> ---
>
> nhdpIfStatus
>
> I *think* it is more normal to write:
>
>      DEFVAL { false(2) }
>

Done.

>
> ---
>
> nhdpInitialPending
>
> Given the constraint conditions described for this object, I don't
> think it is appropriate to have a Default clause. That is, if the
> constraining objects are not defaulted for whatever reason, then this
> object cannot be allowed to default.
>
> This is correctly reflected in you making the object read-only, and we
> should note that there is no value in assigning a Default for a read-
> only object.
>

Agreed.

>
> ---
>
> Finding nhdpIfRowStatus and seeing the text talk about row creation, I
> wondered why none of the objects in the row under the control of this
> object have Max-Access read-create. What you have is legal, but it means
> that the row is created with all the defaults in place and then can be
> modified if the management application wants to.
>
> Since there is no equivalent of an adminStatus, this means that for a
> while the row (and associated protocol elements) will operate with the
> default values rather than the values the management application wanted
> to set.
>
>

Yes, in fact RFC2578 states " .. if any columnar object has 'read-create'
... then no other object (of the row) may have 'read-write' as max-access."

So we maintained the RowStatus object in the entry and changed max-access
to writable objects to 'read-create'.

> ---
>
> The second paragraph of the description of nhdpLibLocalIfSetTable begins
> "It consists of..."  This is mildly ambiguous because the previous
> paragraph ends with a sentence about nhdpIfIndex.
>

Changed "it" to "the Local Interface Set".

>
> ---
>
> nhdpLibLocalIfSetIpAddrType should contain a comment about the valid
> values since I assume you don't support the full set defined for
> InetAddressType. You can also constrain the interpretation of
> nhdpLibLocalIfSetIpAddr according to nhdpLibLocalIfSetIpAddrType.
>
> You would write...
>
>   nhdpLibLocalIfSetIpAddrType  OBJECT-TYPE
>      SYNTAX      InetAddressType
>      MAX-ACCESS  read-write
>      STATUS      current
>      DESCRIPTION
>         "The type of the nhdpLibLocalIfSetIpAddr
>          in the InetAddress MIB [RFC4001].
>
>          Only the values unknown(0), ipv4(1), and
>          ipv6(2) are supported."
>      REFERENCE
>         "[RFC6130]."
>   ::= { nhdpLibLocalIfSetEntry 1 }
>

OK, although we have included only ipv4(4) and ipv6(2) 
for these objects.

>
>   nhdpLibLocalIfSetIpAddr  OBJECT-TYPE
>      SYNTAX      InetAddress
>      MAX-ACCESS  read-write
>      STATUS      current
>      DESCRIPTION
>         "nhdpLibLocalIfSetAddr is an
>          address of an interface of
>          this router.
>
>          This object is interpretted according to
>          the setting of nhdpLibLocalIfSetIpAddrType."
>      REFERENCE
>         "[RFC6130]."
>

OK.

>
> This will also need to show up in the conformance statements. You
> write something like...
>
> OBJECT nhdpLibLocalIfSetIpAddrType
>  SYNTAX       InetAddressType { unknown(0), ipv4(1), ipv6(2) }
>  MIN-ACCESS   read-only
>  DESCRIPTION  "Only unknown(0), ipv4(1), and ipv6(2) support
>                is required."
>
> And similarly...
>
> OBJECT nhdpLibLocalIfSetIpAddr
>  SYNTAX      InetAddress (SIZE(0|4|16))
>  MIN-ACCESS  read-only
>  DESCRIPTION "An implementation is only required to support
>               address sizes of:
>                 0 for address type unknown(0)
>                 4 for address type ipv4(1)
>                 16 for address type ipv6(2)."
>

So, we scoped the allowable address types to 
'ipv4(4), ipv6(2)' and reflected this in the
Description of this object and reflected in a statement
in the Compliance Statements.

Note, this comment also applies to the following objects:
nhdpLibRemovedIfAddrSetIpAddrType and nhdpLibRemovedIfAddrSetIpAddr.
We scoped these objects as well.

>
> Lastly, is the value zero allowed for nhdpLibLocalIfSetIpAddr? If so
> you will need some text in its DESCRIPTION clause like...
>
>     If this object is set to 0, the value of
>     nhdpLibLocalIfSetIpAddrType must be set to
>     unknown(0)."
>
> You would need to do this for all uses of InetAddress[Type]
>

As mentioned above; we have precluded type 'unknown(0)'.

>
> ---
>
> You need to do a general search and replace s/MIB/MIB module/
> There is only one MIB and this MIB module forms part of it.
>

OK.

> For example...
>
> nhdpStateObjGrp    OBJECT IDENTIFIER ::= { nhdpObjects 2 }
>
>   -- Two new constructs have been defined in this MIB for
>   -- indexing into the following
>   -- tables and indexing into other tables in other MIBs.
>
> ---
>
> Question...
>
> nhdpStateObjGrp    OBJECT IDENTIFIER ::= { nhdpObjects 2 }
>
>   -- Two new constructs have been defined in this MIB for
>   -- indexing into the following
>   -- tables and indexing into other tables in other MIBs.
>
> 1. This text seems to come very late in the module. How about
>   relocating to be next to the definition of the "constructs"?
>

Yes, we have moved this text up to in-front of the TC
definitions. 

>
> 2. I think s/constructs/Textual Conventions/
>

Done.

>
> 3. If it is clear that you will use these TCs to index other
>   MIB modules, you should seriously consider putting them in
>   their own module. You can keep this new module in this
>   document, but making it a separate module makes imports from
>   other modules much more simple.
>

So, it is not clear at this time that we need or want to reuse
these constructs.  In the one case where we thought we might is in the
OLSRv2-MIB draft.  However, there we are planning to rewrite it
to rely on AUGMENTS statements to define simple extensions to the
existing associated Tables in NHDP.  Hence, we have left the TC
definitions within the NHDP-MIB module.  We hope this is OK?

>
> ---
>
> nhdpUpTime could use a display hint clause.
>

We changed the semantics of this object per your comments
below regarding the use of TimeStamp and sysUpTime.  So now,
this object holds the sysUptime at the time that the current
NHDP process was initialized.

>
> ---
>
>   nhdpInterfaceStateTable  OBJECT-TYPE
>      SYNTAX      SEQUENCE OF NhdpInterfaceStateEntry
>      MAX-ACCESS  not-accessible
>      STATUS      current
>      DESCRIPTION
>         "nhdpInterfaceStateTable lists state information
>          related to specific interfaces of this NHDP router.
>          The ifIndex is from the interfaces group
>          defined in the Interfaces Group MIB.
>
> Unless I am mistaken, you are not using ifIndex, but ndhpIfIndex.
> So you should say...
>
>          The value of ndhpIfIndex is an ifIndex from the
>          interfaces group defined in the Interfaces Group
>          MIB.
>
> Similarly nhdpInterfaceStateEntry (which is missing a REFERENCE clause.
>

OK.

>
> ---
>
> There is nothing wrong, IMHO, with using TimeTicks for nhdpUpTime and
> nhdpIfStateUpTime. However, this places a burden on the implementation
> rather than the agent.
>
> A common approach is to grab the sysUpTime value when the instance was
> initialised, and to only report that. An agent that is interested in
> the amount of time that the instance has been up, can read sysUpTime
> and do the calculation itself.
>
> If you grab sysUpTime, you use the Textual Convention TimeStamp which
> ironically has the Syntax TimeTicks. The difference is that it is frozen
> rather than your usage which is rolling.
>

As mentioned above, we have converted the symantics of this (and other)
objects to have SYNTAX TimeStamp and to hold the value of sysUpTime
at the time of initialization.  This also affacts the following objects
in the MIB module:
nhdpLibRemovedIfAddrSetIrTime and nhdpDiscNeighborNibNeighborSetUpTime .

>
> ---
>
> As I understand it, nhdpIibLinkSetLTime is a time in the future. I'm not
> sure, butI don't think you are allowed to use TimeStamp like this
> because it is supposed to report on an event that has already happened.
> This may be a bit pettifogging and we should maybe make a list of
> questions to ask the MIB Doctor.
>

We left the semantics of this object as a future time
based upon the expected sysUpTime at which these entry
objects expire.  We add to the Description text to read:

"nhdpIibLinkSetLTime specifies the sysUptime
when to expire this Tuple and in which case
MUST be removed.  Upon expiration this entry
in the 'nhdpIibLinkSetTable' should be removed."

>
> ---
>
> nhdpInterfacePerfTable etc.
>
> For you to think about depending on the interface speeds you think you
> are supporting. The requirements on the use of 32-bit and 64-bit
> counters copied from RFC 2863 are as follows:
>
>   For interfaces that operate at 20,000,000 (20 million) bits per
>   second or less, 32-bit byte and packet counters MUST be supported.
>   For interfaces that operate faster than 20,000,000 bits/second,
>   and slower than 650,000,000 bits/second, 32-bit packet counters
>   MUST be supported and 64-bit octet counters MUST be supported.
>   For interfaces that operate at 650,000,000 bits/second or faster,
>   64-bit packet counters AND 64-bit octet counters MUST be
>   supported.
>

We do not expect interface speeds in excess of 650,000,000
bits/second.  In this case we are in compliance with the
statement from RFC 2863.

>
> ---
>
>   nhdpSetNotification OBJECT-TYPE
>          SYNTAX       OCTET STRING (SIZE(4))
>          MAX-ACCESS   read-write
>          STATUS       current
>          DESCRIPTION
>             "A 4-octet string serving as a bit map for
>             the notification events defined by the NHDP
>             notifications. This object is used to enable
>             and disable specific NHDP notifications where
>             a 1 in the bit field represents enabled. The
>             right-most bit (least significant) represents
>             notification 0.
>
>             This object is persistent and when written
>             the entity SHOULD save the change to
>             non-volatile storage.
>             "
>           ::= { nhdpNotificationsControl 1 }
>
> I find the use of Octet String in this way a bit icky. It is, of course,
> possible to represent the whole MIB module using Octet Strings, but we
> don't. Did you consider and reject the BITS construct? It seems neater
> and more explicit.
>

We pulled this object from the MIB module as we felt it was unnecessary
to the operation of the module. 

>
> I also spent some time trying to work out which notification is
> "notification 0" because the first notification listed is
> { nhdpNotificationsObjects 1 }
>

These should start at '1'.

>
> ---
>
> Should the ChangeThreshold and ChangeWindow family of objects have
> DEFVALs?
>

We have added DEFVALs and associated Descriptions for each of the 
following NotificationGroup objects:
nhdpNbrStateChangeThreshold, nhdpNbrStateChangeWindow, nhdp2HopNbrStateChangeThreshold,
nhdp2HopNbrStateChangeWindow, nhdpIfRxBadPacketThreshold and
nhdpIfRxBadPacketWindow.

>
> ---
>
> nhdpNbrState, nhdp2HopNbrState, and nhdpIfState are (correctly)
> read-only. They shouldn't have DEFVALs defined because they report what
> they report.
>
> 

OK.

> You might want to add to the Description...
>
> If the state of foo is unknown, an implementation should return bar(n)
>
> Or you might want to define specific "unknown" values to handle this,
>

We addressed this comment through addition Description, e.g., for the
nhdpNbrState we have the following Description:

"NHDP neighbor states. In NHDP, note that it is not
 necessary to remove Protocol Tuples from Protocol Sets
 at the exact time indicated, only to behave as if the
 Protocol Tuples were removed at that time.  This case is
 indicated here as 'down(0)', all other cases being
 indicated as 'assymetric(1)' or 'symmetric(2)'. If down,
 the direct neighbor is also added to the
 nhdpNibLostNeighborSetTable."

Comparable text was added to the nhdp2HopNbrState and nhdpIfState
object descriptions.

>
> ---
>
> Really good job on the Security Considerations section!
>

Thanks!
Ulrich and Bob