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

[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 next 
>>> revision.
>>
>> 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 for 
>> configuration and
>> tables for monitoring.
>> In this revision, the diagram representing the relationship among 
>> tables 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 be 
>>>> changed
>>>>       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 RFC4181 
>> Sec.3.4.
>> However, there are some TBDs in this part that should be updated 
>> according
>> 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