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

[Joan Cucchiara <[e-mail]>]

Jeong-dong,

Thank you for the responses to my comments.   I will take a look at the
updated draft soon.

-Joan

On Thu, May 19, 2016 at 9:24 AM, 류정동 <[e-mail]> wrote:

> 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 ---
>
>
> _______________________________________________
> mpls mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/mpls
>