[mpls] Review of draft-ietf-mpls-tp-linear-protection-mib-07

["Joan Cucchiara" <[e-mail]>]

Comments for draft-ietf-mpls-tp-linear-protection-mib-07.txt

Authors,

Lots of work went into this draft.  The early MIB Doctor review 
comments have been incorporated, so thank you for that.   These 
comments are arranged in 3 sections:  MIB compiler outputs, 
General Comments which are observations that apply to several 
places in the MIB and should be checked for throughout the MIB.  

The last section is for specific comments.

Please take these comments as part of the last call.

Thanks,
-Joan

Compiles cleanly with smilint

smicng flagged some errors
Output from smicng
E: f(MPLS-LSP-MIB.my), (370,4) Row "mplsLpsConfigEntry" may not 
have columns with MAX-ACCESS of read-write if any column is read-create
E: f(MPLS-LSP-MIB.my), (378,15) Index item 
"mplsLpsConfigDomainIndex" must be defined with syntax that 
includes a range
E: f(MPLS-LSP-MIB.my), (907,4) Item "mplsLpsMeConfigDomainIndex" 
has invalid value for MAX-ACCESS


When looking at the MIB, I see that there do appear to be some potential
errors:
   mplsLpsConfigCommand OBJECT-TYPE
      SYNTAX      MplsLpsCommand
      MAX-ACCESS  read-write    <---- should be read-create 

because row created using RowStatus
      STATUS      current

In general, indices should specify a range so this is why it was 
flagged by compiler.  Looking at this specific index would like to 
understand how the value is supposed to be assigned?   If this is 
assigned by an operator, perhaps there should be a mechanism for 
the operator to choose a value (for example, by using a 
IndexIntegerNextFree object)?  Please clarify.

   mplsLpsConfigDomainIndex OBJECT-TYPE
      SYNTAX        Unsigned32

mplsLpsMeConfigDomainIndex <--- name implies that this is an index 
but it is NOT included in the INDEX clause for this table.   

Please clarify what is intended.


General Comments:
=====================

* There are mentions of there being two MIB Modules in this 
document, but there is only one MIB Module. I have tried to note 
these statements under specific comments, but please check for 
such statements.   If the intention is to create two MIB Modules, 
that is fine, but currently, there is only one.

* The relationships of these Tables is not clear.  
MplsLpsConfigTable has an INDEX but how the operator 
is supposed to choose a value for this index is 
not specified.  The MplsLpsMeConfigTable indexing is confusing.  
Although the document states that this table is an extension 
of the MPLSOamIdMeTable, the name of the object, 
mplsLpsMeConfigDomainIndex is confusing because it suggests
this is an INDEX (as does the status of not-accessible).   Please clarify.  

Since the indexing for these Tables is confusing to me, then 
please realize that this MIB may have additional comments
during the next review once the indexing is clarified. 

* In general more REFERENCE Clauses could/should be added throughout MIB.

Objects such as mplsLpsMeConfigDomainIndex, mplsLpsMeStatusCurrent,
mplsLpsConfigMode, mplsLpsConfigProtectionType, etc.   This was also 
mentioned during the early MIB Dr. review.

* Some of the objects use Integer32 but they probably should 
use Unsigned32.   In other words, if the objects can only take on values 0
and above, then 
they should use Unsigned32.

e.g.     mplsLpsConfigSdBadSeconds, mplsLpsConfigSdGoodSeconds,
mplsLpsConfigWaitToRestore, mplsLpsConfigHoldOff, etc.  Please 
check all the Integer32 objects to see if they should be Unsigned32.


*) Date is same a previous version.  This should be updated for 
every revision of the document.  Please update.
      LAST-UPDATED  and REVISION clauses
    "201512060000Z"  -- December 06, 2015


*) Only FullCompliance is done for this MIB Module.  As you 
probably are aware, not all operators want to configure
using SNMP, if there is not a ReadOnly Compliance available, then 
they will not be compliant with the MIB.  I think a ReadOnly Compliance 
for a MIB is useful and would like to understand why this MIB doesn't have
one.
Could the authors please clarify?


Specific Comments:   

Section 1. Introductions

Mentions multiple MIB Modules but there is only one.  Please 
clarify the text:  "However, since the MIB modules ..." <-- plural


Section 4.

As mentioned before there is only 1 MIB module.  Please update.

   "This document specifies a MIB module 
    for the Label Edge Router (LER)
    that supports MPLS TP Linear protection and a MIB 
    module that defines textual conventions....."


Section 5.1 Textual Conventions
 
* I don't see a separate MIB Module for TCs.  Please clarify.

Section 5.4 The Table Structure

 * The mplsLpsConfigTable

"The protection domain is identified by mplsLpsConfigGroupName."   
This statement does not seem to be entirely accurate given the MIB 
design for 2 reasons, 1.  there doesn't seem to be an object
mplsLpsConfigGroupName 
and 2. the INDEX is mplsLpsConfigDomainIndex Unsigned32 (which also appears
in the 
mplsLpsMeConfigEntry with a status of not-accessible 
(and I think you intend for it to be an object)?

As a reviewer, this is confusing because the relationship with 
these tables is unclear and so it is very difficult to 
review the MIB Module.  Please clarify the relationship with 
these tables and to the mplsOamIdMeTable in the 
MPLS-OAM-ID-STD-MIB.


"The other attributes in this table", do you mean objects?

* The  mplsLpsStatusTable

There is no mention that the mplsLpsStatusTable's Entries have an 
AUGMENTS relationship with the mplsLpsConfigTable Entries.  Please add.

6.1  Relationship to the MPLS OAM maintenance identifier MIB Module

The title needs to be capitalized correctly, Relationship to the 
MPLS OAM Maintenance Identifier MIB Module

Please update this section to use RFC7697 (and in Informative References
also) 
instead of draft-ietf-mpls-tp-oam-id-mib.   

As mentioned above, the mplsLpsMeConfigTable has an object 
mplsLpsMeConfigDomainIndex which is (not-accessible).
Is this supposed to be an INDEX, or is this an object?   I am 
confused by what is intended.  

7.  Example of Protection switching configuration for 

MPLS-TP TE tunnel (Please change title:  Example of Protection Switching
Configuration)

* I am unclear how mplsLspConfigEntry is actually configured for 
use in this example.   Is an operator supposed to randomly choose an INDEX
value?

Would an IndexNext object be useful to use in conjunction with this INDEX?

MIB Module
------------

(general comment:  the DESCRIPTION clauses could be more readable 
if consistency was used.  Sometimes the
value is listed on the side and the description follows on the 
same line and sometimes the value is listed
on a single line and the description follows a couple of lines 
after.   Please be consistent.)

* mplsLpsConfigDomainName  -- Is there a DEFAULT value for this 
object?   The string size is 1..32 with no
option of 0 length string, so wanted to check about a default 
value?  Under what circumstances can this value
be modified?   Please give a REFERENCE.

* mplsLpsConfigMode - Needs REFERENCE (and please try to be 
specific).  Under what circumstances can this be modified?

* mplsLpsConfigWaitToRestore
Why is this not in minutes?  If someone configures this to be 30 
seconds is that valid?  Doesn't seem so based on the DESCRIPTION.  Please
clarify.

* mplsLpsConfigHoldOff What is meant by "Can be configured in 
steps of 100?"   Is this 100 milliseconds?  If so then maybe a better unit
choice would be 
centiseconds.   Please clarify.

*mplsLpsConfigCommand is read-write.  Is this supposed to be 
read-create?

*mplsLpsConfigRowStatus --  I think there is some conflicting 
advice given to the operator.  Several objects say that it is fine to change
the 
value of the object when RowStatus is active, but this is not specified
consistently.  Limiting the 
values of RowStatus in the Conformance Section
may be the way to go.  Please clarify.

*mplsLpsMeConfigState is a read-create. This is probably okay, but 
again, that depends on if mplsLpsMeConfigDomainIndex
is an INDEX for this table given that it has a status of not-accessible,
etc.


*mplsLpsMeStatusSwitchoverSeconds
Needs a units clause for Seconds


Notifications

There are a couple Notifications that are send when values of certain 
counters increment.  Maybe this is valid, but it seems suspect.  
If a management stations needs information on counters,
why can't it just retrieve them at that point?   I don't see any 
counter discontinuity objects, so was wondering about that too.  

* mplsLpsEventFopTimOut Notification

Please rename this to mplsLpsEventFopTimeout

* Compliance/Conformance Section of the MIB Module

Currently, there is only FullCompliance.   Why is there no
ReadOnlyCompliance?

--- end of comments ---