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

["Ryoo, Jeong-dong " <[e-mail]>]

Joan, thanks.


We are looking forward to hearing from you soon.


Jeong-dong







________________________________
보낸 사람 : "Joan Cucchiara" <[e-mail]>
보낸 날짜 : 2016-05-26 23:09:46 ( +09:00 )
받는 사람 : 류정동 <[e-mail]>
참조 : Joan Cucchiara <[e-mail]>, Loa Andersson <[e-mail]>, [e-mail] <[e-mail]>, [e-mail] <[e-mail]>, [e-mail] <[e-mail]>, [e-mail] <[e-mail]>
제목 : Re: [mpls] 회신: Review of draft-ietf-mpls-tp-linear-protection-mib-07



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]<mailto:[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]<mailto:[e-mail]>]



보낸 날짜: 2016년 4월 5일 화요일 오전 12:10



받는 사람: Loa Andersson; [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>



참조: [e-mail]<mailto:[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]<mailto:[e-mail]>



https://www.ietf.org/mailman/listinfo/mpls