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

Glenn Mansfield Keeni <glenn@cysols.com> Mon, 26 June 2017 07:53 UTC

Return-Path: <glenn@cysols.com>
X-Original-To: bess@ietfa.amsl.com
Delivered-To: bess@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 4E843126DEE; Mon, 26 Jun 2017 00:53:01 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.901
X-Spam-Level:
X-Spam-Status: No, score=-1.901 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id b07aXMPt154o; Mon, 26 Jun 2017 00:52:58 -0700 (PDT)
Received: from niseko.cysol.co.jp (niseko.cysol.co.jp [210.233.3.236]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 65095126C23; Mon, 26 Jun 2017 00:52:58 -0700 (PDT)
Received: from [192.168.0.200] (Lenovo-X1Carbon.win2004.cysol.co.jp [192.168.0.200]) (authenticated bits=0) by aso.priv.cysol.co.jp (8.14.9/8.14.9) with ESMTP id v5Q7qi6F044898 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 26 Jun 2017 16:52:44 +0900 (JST) (envelope-from glenn@cysols.com)
From: Glenn Mansfield Keeni <glenn@cysols.com>
To: Hiroshi Tsunoda <tsuno@m.ieice.org>
Cc: Mach Chen <mach.chen@huawei.com>, "Jeffrey (Zhaohui) Zhang" <zzhang@juniper.net>, "bess@ietf.org" <bess@ietf.org>, Benoit Claise <bclaise@cisco.com>, "ops-ads@ietf.org" <ops-ads@ietf.org>, Martin Vigoureux <martin.vigoureux@nokia.com>, "EXT - thomas.morin@orange.com" <thomas.morin@orange.com>
References: <56E7D219.7000902@orange.com> <56FBD402.9040102@cisco.com> <56FBDD81.6080502@cysols.com> <11152_1459347064_56FBDE78_11152_10229_1_56FBDE77.6030605@orange.com> <56FBE17E.5090609@cisco.com> <570C9586.7030905@cysols.com> <570DC523.3040907@cysols.com> <F73A3CB31E8BE34FA1BBE3C8F0CB2AE28CC742F5@SZXEMA510-MBX.china.huawei.com> <BLUPR0501MB1715A41533590C81B9E54601D45C0@BLUPR0501MB1715.namprd05.prod.outlook.com> <CAPbjwkyWzJ7AJAFKXcWvhnBvu6C-oY3J+rH8rY4sb1U2tWH35w@mail.gmail.com> <CAPbjwkycKadkFO5wTdmTAzdstN54P6CpPJ_eNUTWe3wYB7-q6Q@mail.gmail.com> <5d16d75c-e518-20ba-35ca-ab6c979637e8@cysols.com>
Message-ID: <c6ebe8cc-2083-bfb7-8647-0f2dee32da1f@cysols.com>
Date: Mon, 26 Jun 2017 16:52:39 +0900
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <5d16d75c-e518-20ba-35ca-ab6c979637e8@cysols.com>
Content-Type: multipart/mixed; boundary="------------FA56F9D48955FD78C4AE2096"
Content-Language: en-US
Archived-At: <https://mailarchive.ietf.org/arch/msg/bess/xJQOx3Z03Oi5x5a9VlzShng_bto>
Subject: Re: [bess] MIBDoc review of draft-ietf-bess-mvpn-mib-02.txt
X-BeenThere: bess@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: BGP-Enabled ServiceS working group discussion list <bess.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/bess>, <mailto:bess-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/bess/>
List-Post: <mailto:bess@ietf.org>
List-Help: <mailto:bess-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/bess>, <mailto:bess-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 26 Jun 2017 07:53:01 -0000

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 <tsuno@m.ieice.org>rg>:
>>>> 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
>> BESS@ietf.org
>> https://www.ietf.org/mailman/listinfo/bess
>>
> 
> _______________________________________________
> BESS mailing list
> BESS@ietf.org
> https://www.ietf.org/mailman/listinfo/bess