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

[Glenn Mansfield Keeni <[e-mail]>]

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
>