Re: [bess] MIBDoc review of draft-ietf-bess-mvpn-mib-02.txt

[Hiroshi Tsunoda <[e-mail]>]

Hi Glenn,

Thank you for your review and valuable comments.
I am going to look at your comments in detail and
revise the draft.

Best regards,

-- tsuno

2017-06-26 9:52 GMT+02:00 Glenn Mansfield Keeni <[e-mail]>:
> Hi Tsuno,
>    Thanks for waiting. I have done one pass of the draft.
> The comments are attached.
> Please note that we probably need to do some more design
> considerations on the MIB - there are some issues that
> need to be addressed before we can arrive at a reasonably
> stable version of the MIB itself.
>
> Glenn
>
>
> On 2017/06/12 21:47, Glenn Mansfield Keeni wrote:
>>
>> Hi Tsuno,
>>     Got this. I will be working on this. It is massive-
>> 40 pages. I hope to get back to you on this by the end
>> of next week.
>>
>> Cheers,
>>
>> Glenn
>> On 2017/06/07 1:22, Hiroshi Tsunoda wrote:
>>>
>>> Hi Glenn,
>>>
>>> I posted a new revision (-04) of MVPN-MIB document.
>>> In this revision, "Summary of MIB Module" has been rewritten.
>>> I hope this change improves the readability.
>>>
>>> URL:
>>> https://www.ietf.org/internet-drafts/draft-ietf-bess-mvpn-mib-04.txt
>>> Htmlized: https://tools.ietf.org/html/draft-ietf-bess-mvpn-mib-04
>>>
>>> Htmlized:https://datatracker.ietf.org/doc/html/draft-ietf-bess-mvpn-mib-04
>>> Diff:
>>> https://www.ietf.org/rfcdiff?url2=draft-ietf-bess-mvpn-mib-04
>>>
>>> Please see notes below for other changes.
>>>
>>> 2017-03-01 16:00 GMT+01:00 Hiroshi Tsunoda <[e-mail]>:
>>>>>
>>>>> 1.  Abstract:
>>>>> 1.2 "In particular, it describes managed objects to configure and/or
>>>>>      monitor Multicast in MPLS/BGP IP VPNs (MVPN) on a router."
>>>>>     Is this for any router or, a "Provider Edge" router ?
>>>>>     Please fix accordingly.
>>>>
>>>>
>>>> This point will be fixed in the next revision.
>>>
>>>
>>> Fixed. "Provide Edge" router is correct.
>>>
>>>>> 2.  Introduction
>>>>>     Are the objects "generic" to PIM-MVPN and BGP-MVPN or "common"
>>>>>     to  PIM-MVPN and BGP-MVPN ? Please change accordingly.
>>>>
>>>>
>>>> This point will be fixed in the next revision.
>>>
>>>
>>> Fixed. "common" is correct.
>>>
>>>>> 2.5 The terminology section is a bit terse. Explaining the terms
>>>>>     that are used, with reference to the protocol documents will
>>>>>     improve readability.
>>>>>     e.g.
>>>>>      - MVPN, PE, PMSI/tunnels,
>>>>>      - C-multicast routing exchange protocol (PIM or BGP),
>>>>>        C-multicast states
>>>>>      - I-PMSI, S-PMSI, provider tunnels
>>>>
>>>>
>>>> Partially fixed. I will give more detailed explanation in the
>>>> nextrevision.
>>>
>>>
>>> I have added some more explanation in this revision.
>>>
>>>>> 3.  MVPN MIB.
>>>>>     This gives the overview of the MVPN MIB.
>>>>>     The MIB module aims to provide "configuring and/or monitoring"
>>>>> 3.1 In
>>>>>      "This MIB enables configuring and/or monitoring of MVPNs on PE
>>>>>      devices: the whole multicast VPN machinery....."
>>>>>     "the whole multicast VPN machinery" is very difficult to define.
>>>>>     Please use precisely defined terms.
>>>>> 3.2 In "To represent them,...."
>>>>>     "them" seems ambiguous, please clarify.
>>>>> 3.3 The diagram needs some explanation.
>>>>>     What do the boxes represent? Tables ? The labels are meant to be
>>>>>     table names ? The table names do not match the labels.
>>>>>     What do the arrows signify? Please explain.
>>>>> 3.4 The short explanation of the tables could be augmented with some
>>>>>     information on what they represent and an idea of how they will
>>>>>     be used. ( RFC 4382 provides a good example).
>>>
>>>
>>> I have rewritten "Sec.3.1 Summary of MIB Module".
>>> Eight tables can be categorized into two groups: tables forconfiguration
>>> and
>>> tables for monitoring.
>>> In this revision, the diagram representing the relationship amongtables
>>> is
>>>
>>> divided to two separated diagrams based on the roles of tables.
>>>
>>>>> MIB definitions:
>>>>> 7. Wherever possible, please provide references for objects used in the
>>>>>     MIB. The references will point to specific sections/sub-sections of
>>>>>     RFCs defining the protocol for which the MIB is being designed.
>>>>
>>>>
>>>> This will be addressed in the next revision.
>>>
>>>
>>> I have added some references but more are required.
>>> I will keep working on this.
>>>
>>>>> 8. MOs.
>>>>> 8.2 mvpnMvrfNumber OBJECT-TYPE
>>>>>        SYNTAX         Unsigned32
>>>>>        DESCRIPTION
>>>>>            "The total number of MVRFs that are present on this device,
>>>>>             whether for IPv4, IPv6, or mLDP C-Multicast."
>>>>>     o Please make the description precise. E.g. if it is the sum of
>>>>>       IPv4 MVRFs, IPv6 MVRFs and mLDP C-Multicast MVRFs state it
>>>>>       explicitly.
>>>>>     o The expression "present on this device" is used.
>>>>>       Does "present" imply "configured" MVRFs or "active" MVRFs.
>>>>>       If it is number of active MVRFs then one would expect that
>>>>>       the number will vary (increase or decrease). If that is the
>>>>>       case:
>>>>>       replace
>>>>>        SYNTAX        Unsigned32
>>>>>       by
>>>>>        SYNTAX        Gauge32
>>>
>>>
>>> I will try to update description in the next revision.
>>>
>>>>> 8.5 mvpnGenOperStatusChange OBJECT-TYPE
>>>>>         SYNTAX      INTEGER { createdMvrf(1),
>>>>>                               deletedMvrf(2),
>>>>>                               modifiedMvrfIpmsiConfig(3),
>>>>>                              modifiedMvrfSpmsiConfig(4)
>>>>>                             }
>>>>>        DESCRIPTION
>>>>>            "This object describes the last operational change that
>>>>>     o The name does not look right. From the SYNTAX and the DESCRIPTION
>>>>>       it appears that this is about config or MVRF change rather than
>>>>>       "OperStatus" change. Please check and fix.
>>>>>     o Please confirm that the values in the row itself will not
>>>>> bechanged
>>>>>       after creation. ( you do not have a 'modifiedMvrfConfig')
>>>
>>>
>>> The name has been changed into mvpnGenMvrfStatusChange.
>>> The name of the related object (mvpnGenOperStatusChangeTime) has
>>> also been changed into mvpnGenMvrfStatusChangeTime.
>>>
>>>>> 8.6 mvpnGenCmcastRouteProtocol OBJECT-TYPE
>>>>>        MAX-ACCESS    read-write
>>>>>        ::= { mvpnGeneralEntry 4 }
>>>>>     o You cannot have MAX-ACCESS    read-write for a row that may be
>>>>>       dynamically created.
>>>>>       Replace
>>>>>        MAX-ACCESS    read-write
>>>>>       by
>>>>>        MAX-ACCESS    read-create
>>>>>       if you want to dynamically change that value, otherwise,
>>>>>        MAX-ACCESS    read-only
>>>>>       will suffice.
>>>
>>>
>>> Fixed. Now, "MAX-ACCESS    read-create" is used.
>>>
>>>>> 8.8 mvpnGenIpmsiConfig OBJECT-TYPE
>>>>>         DESCRIPTION
>>>>>            "This points to a row in mvpnPmsiConfigTable,
>>>>>             for I-PMSI configuration."
>>>>>     o Please specify the expected behaviour when it is not an I-PMSI
>>>>> 8.9 mvpnGenInterAsPmsiConfig
>>>>>     o same comment as above
>>>
>>>
>>> These will be addressed in the next revision.
>>>
>>>>> 8.10 mvpnGenRowStatus
>>>>>     mvpnGenRowStatus OBJECT-TYPE
>>>>>        SYNTAX        RowStatus
>>>>>        DESCRIPTION
>>>>>            "This is used to create or delete a row in this table."
>>>>>     o The description is inadequate for an implementor (and
>>>>>       others too).
>>>>>     o You must have a mvpnGenRowStorageType or the DESCRIPTION of
>>>>>       mvpnGenRowStatus must indicate what will happen to the row
>>>>>       after an agent restart
>>>
>>>
>>> I will try to address this comment in the next revision.
>>>
>>>>> 9. Similar comments (8.1 ~ 8.10) for the remaining tables in the MIB
>>>>>     Particularly 8.10 for the RowStatus type objects
>>>>>                          mvpnGenRowStatus
>>>>>                         mvpnPmsiConfigRowStatus
>>>>>                         mvpnSpmsiConfigRowStatus.
>>>>>    Please check and fix.
>>>
>>>
>>> I will try to address this comment in the next revision.
>>>
>>>>> 10. mvpnMvrfChange NOTIFICATION-TYPE
>>>>>         OBJECTS     {
>>>>>                      mvpnGenOperStatusChange
>>>>>                    }
>>>>>        ::= { mvpnNotifications 2 }
>>>>>
>>>>>     o should be  { mvpnNotifications 1 }
>>>>>     o Include the MOs that the administrator/manager may want to
>>>>>       see in OBJECTS.
>>>
>>>
>>> The first comment is addressed, the second one is TBD.
>>>
>>>>> 11. The Security Considerations section does not follow the Security
>>>>>      Guidelines for IETF MIB Modules
>>>>>      http://trac.tools.ietf.org/area/ops/trac/wiki/mib-security.
>>>>
>>>>
>>> I rewrite this part according to the guideline described in
>>> RFC4181Sec.3.4.
>>> However, there are some TBDs in this part that should be updatedaccording
>>> to the update in the main body of MIB module.
>>>
>>>>> 12.  COMPLIANCE.
>>>>> 12.1 You seem to mandate MAX-ACCESS read-write/read-create for
>>>>>       compliance. Is this intended? Configuration capability MUST be
>>>>>       supported?  Please note that sec 2.  MVPN MIB says
>>>>>       "This MIB enables configuring and/or monitoring of MVPNs ..."
>>>>>      The current compliance requirement contradicts the above claim.
>>>>>      Please check and fix.
>>>>>
>>>>>      It is general and sound practice to allow for MAX-ACCESS
>>>>>      read-only compliance. Some implementations may support
>>>>>      monitoring but not configuration.
>>>>>      Please check and fix.
>>>
>>>
>>> In this revision, I have added additional ReadOnly compliance
>>> Now, there are following two MODULE-COMPLIANCE statements
>>> are defined in this module.
>>>   - mvpnModuleFullCompliance
>>>   - mvpnModuleReadOnlyCompliance
>>>
>>> Best regards,
>>>
>>> -- tsuno
>>>
>>> _______________________________________________
>>> BESS mailing list
>>> [e-mail]
>>> https://www.ietf.org/mailman/listinfo/bess
>>>
>>
>> _______________________________________________
>> BESS mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/bess
>
>