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

[Hiroshi Tsunoda <[e-mail]>]

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