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

[류정동 <[e-mail]>]

Dear Joan,

We resolved all of your comments and uploaded a revsion today.

Please, see inlines starting with [Authors] in your previous email below.

The revised draft can be found in:
https://www.ietf.org/internet-drafts/draft-ietf-mpls-tp-linear-protection-mib-08.txt
  
We appreciate your help on this draft.

Best regards,

Jeong-dong 

________________________________________
보낸 사람: Joan Cucchiara [[e-mail]]
보낸 날짜: 2016년 4월 5일 화요일 오전 12:10
받는 사람: Loa Andersson; [e-mail]; [e-mail]; [e-mail]
참조: [e-mail]
제목: Review of draft-ietf-mpls-tp-linear-protection-mib-07

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

[Authors] OK, Fixed.

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

[Authors] Fixed by using the IndexIntegerNextFree object.

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.

[Authors] This is not the INDEX for this table. It is used to identify the corresponding mplsLpsConfigDomainIndex value in the other table, “mplsLpsConfigTable”. The name of this object is changed to mplsLpsMeConfigDomainIndexValue to avoid confusion.


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.

[Authors] Yes, there is only one defined in this document. Fixed

* 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.

[Authors] Fixed by using the IndexIntegerNextFree object and changing the name of mplsLpsConfigDomainIndex to mplsLpsMeConfigDomainIndexValue

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.

[Authors] OK. Added more REFERENCE Clauses.

* 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.

[Authors] OK, Fixed.

*) 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

[Authors] OK, Fixed.

*) 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?

[Authors] OK, ReadOnly Compliance is added.


Specific Comments:

Section 1. Introductions

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

[Authors] Yes, there is only one. Fixed.


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....."

[Authors] OK. Fixed.

Section 5.1 Textual Conventions

* I don't see a separate MIB Module for TCs.  Please clarify.

[Authors] Fixed.

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)?

[Authors] “mplsLpsConfigGroupName” should be “mplsLpsConfigDomainName”. It’s been corrected. 

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.

[Authors] A protection domain consists of two paths, working and protection paths, and requires two OAM MEs; one OAM ME for the working path and the other ME for the protection path. In other words, a row of “mplsLpsConfigTable” is for one protection domain, which requires two rows in “mplsOamIdMeTable”: one for the working path and the other for the protection path. Also note that an entry of “mplsOamIdMeTable” may not belong to any protection domain. The row of “mplsLpsMeConfigTable” defined in this document has a sparse relationship with that of the “mplsOAMIdMeTable” defined in RFC 7697. 


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

[Authors] Yes. Fixed.


* The  mplsLpsStatusTable

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

[Authors] Added.


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.

[Authors] Ok. Fixed.


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.

[Authors] The name of this object is changed to mplsLpsMeConfigDomainIndexValue to avoid confusion.


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?

[Authors] Yes, it has been addressed with IndexIntegerNextFree.



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.)

[Authors] Ok. Fixed.


* 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.

[Authors] No DEFAULT value is needed. The size has been changed to 0..32.


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

[Authors] REFERENCE is given. 


* 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.

[Authors] Fixed with “minutes”. The range is also corrected.


* 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.

[Authors] It can be configured like: 0, 100 ms, 200 ms, … , 10 seconds. So, the units and the description are changed using “deciseconds”. 

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

[Authors] Yes. Fixed


*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.

[Authors] There are some objects that can be changed during protocol operation, while other objects cannot be changed but their values need to be given before the operation. In the revision, we specified them consistently. 


*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.

[Authors] “mplsLpsMeConfigDomainIndex” was not intended to be INDEX, but to contain the value of the value of protection domain index. We changed it to mplsLpsMeConfigDomainIndexValue to avoid confusion 


*mplsLpsMeStatusSwitchoverSeconds
Needs a units clause for Seconds

[Authors] Fixed.


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.

[Authors] Whenever there is an increment in any of the enabled counters, network operators need to be alarmed. 


* mplsLpsEventFopTimOut Notification

Please rename this to mplsLpsEventFopTimeout

[Authors] OK. Fixed.


* Compliance/Conformance Section of the MIB Module

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

[Authors] OK. It’s been added.


--- end of comments ---