Comments on draft-ietf-bfd-mpls-mib-05

[Jeffrey Haas <[e-mail]>]

Note that I have neither audited the conformances nor the security section
in detail.


:   This document adopts the definitions, acronyms and mechanisms
:   described in [BFD], [BFD-1HOP], [BFD-MH], [RFC5884], [RFC6428].
:   Unless otherwise stated, the mechanisms described therein will not be
:   re-described here.

I'd suggest making the links BFD and BFD-1HOP pointer to the relevant RFC
numbers.  Otherwise continue the naming practice with 5884 and 6428.

: 5.1. Extensions to the BFD session table (bfdSessionTable)

Auditing this entry lead to the discovery of errata in RFC 7331.  The
underlying table is bfdSessTable and should be listed that way in this MIB.

This document contains two textual conventions.  The SessionMapTypeTC in
particular has properties that suggest it may be best to craft a separate
MIB module to permit IANA maintenance.  This will permit the TCs to be
maintained separately without requiring an update to the RFC.

It is unclear whether the DefectActionTC would similarly benefit from being
an IANA maintained MIB module.

If the SessionMapTypeTC is placed in a separate module, section 5.1 will
require updating.

:        Similarly BFD session would be configured on the tail-end of

"Similarly, the BFD session"...

The same correction applies in section 5.2.

Section 5.3 refers to bfdSessionPerfTable.  As noted from the errata
mentioned above, bfdSessPerfTable is the actual table name.

Section 6:

:         RowPointer,TruthValue,TEXTUAL-CONVENTION

Please apply consistent spacing after commas.

:          -- RFC Ed.: RFC-editor pls fill in xxxx

s/xxxx/XXX/  (Although I suspect the RFC editor would still catch this. :-)

:           DESCRIPTION
:             "A row in this table extends a row in bfdSessTable."

While I believe this comment is clear, it may be worth noting explicitly
that not all bfdSessEntry instances will utilize these extensions.  This
will make it clearer for MIB doctor review why you have chosen to use a
foreign index rather than augment the underlying bfdSessTable.

:       bfdMplsSessRole  OBJECT-TYPE
:           SYNTAX      INTEGER {
:                         active(1),
:                         passive(2)
:                       }

While obviously required for the BFD MPLS MIB, I'm wondering if the
active/passive state is more generally useful for the underlying BFD MIB.
While I don't believe that we should force this to go into an update for the
BFD MIB, the BFD Yang editors should probably make note of this
configuration state for their efforts.

:           REFERENCE
:               "1.RFC6428, Proactive Connectivity Verification,

The 1 here is extraneous and should be removed.

:       bfdMplsSessMapPointer OBJECT-TYPE
:           SYNTAX           RowPointer
:           MAX-ACCESS       read-create
:           STATUS           current
:           DESCRIPTION
:             "If bfdMplsSessMapType is nonTeIpv4(1) or nonTeIpv6(2),
:              then this object MUST contain zeroDotZero or point to
:              an instance of the mplsXCEntry indicating the LDP-based
:              LSP associated with this BFD session.
[...]

As noted above, I suspect that the underlying bfdMplsSessMapType TC should
be an IANA maintained TC.  That would impact the text in this section.  I'm
somewhat unclear on how the underlying dependencies of the DESCRIPTION text
for this object which belongs in this MIB vs. the bfdMplsSessMapType TC in a
separate module should be handled.  Probably a question for the MIB doctors.

[next two comments out of order from the MIB text]
:              If this object contains zeroDotZero then no valid path is
:              associated with this BFD session entry till it is
:              populated with a valid pointer consistent with
:              the value of bfdMplsSessMapType as explained above."

My reading of this is that if a BfdMplsSessEntry row is instantiated but 
bfdSessRowStatus is otherwise set to a state that would be "running" (e.g.
active, createAndGo), that action should fail.  Similarly:

:              If this object points to a conceptual row instance
:              in a table consistent with bfdMplsSessMapType but this
:              instance does not currently exist then no valid
:              path is associated with this session entry.

In the case where the entry does not currently exist, I would similarly
expect the bfdSessRowStatus would fail for running states.  I would spell
this out.

It is probably also necessary to spell out what happens when an underlying
reference is invalidated.  My expectation is that the bfdSessRowStatus would
need to transition to notInService or notReady.

The bfdMplsSessPerfTable consists only of Counter32 objects.  Practice
appears to use paired Counter64 "High Capacity" counters.  Are these being
left out for any specific reason?

One final comment is that no NOTIFICATIONs are defined for this MIB.  While
not strictly required, an operator has no indication that a given
NOTIFICATION in the underlying RFC 7331 MIB is for sessions that may change
state for reasons having to do with underlying MPLS session association.  

One possibility that comes to mind to address this is to add a
recommendation that the bfdSessUp/bfdSessDown NOTIFICATIONS receive an
additional OBJECTS entry of bfdMplsSessMapPointer.  The one obvious counter
to such a practice is this may disrupt the low/high range optimization in
the base NOTIFICATION.

-- Jeff