Re: [VRRP] AD review of draft-ietf-vrrp-unified-mib

Mukesh Gupta <mukesh@juniper.net> Thu, 13 January 2011 20:19 UTC

Return-Path: <mukesh@juniper.net>
X-Original-To: vrrp@core3.amsl.com
Delivered-To: vrrp@core3.amsl.com
Received: from localhost (localhost [127.0.0.1]) by core3.amsl.com (Postfix) with ESMTP id 2C25B3A6A81 for <vrrp@core3.amsl.com>; Thu, 13 Jan 2011 12:19:39 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -6.149
X-Spam-Level:
X-Spam-Status: No, score=-6.149 tagged_above=-999 required=5 tests=[AWL=-0.151, BAYES_00=-2.599, HTML_MESSAGE=0.001, J_CHICKENPOX_56=0.6, RCVD_IN_DNSWL_MED=-4]
Received: from mail.ietf.org ([64.170.98.32]) by localhost (core3.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id EoEhAHiarA29 for <vrrp@core3.amsl.com>; Thu, 13 Jan 2011 12:19:36 -0800 (PST)
Received: from exprod7og122.obsmtp.com (exprod7og122.obsmtp.com [64.18.2.22]) by core3.amsl.com (Postfix) with ESMTP id 4EB5F3A6A64 for <vrrp@ietf.org>; Thu, 13 Jan 2011 12:19:36 -0800 (PST)
Received: from source ([66.129.224.36]) (using TLSv1) by exprod7ob122.postini.com ([64.18.6.12]) with SMTP ID DSNKTS9e3dSoXeoZH5A1GLuAO9EptPn+gRhf@postini.com; Thu, 13 Jan 2011 12:22:00 PST
Received: from EMBX01-HQ.jnpr.net ([fe80::c821:7c81:f21f:8bc7]) by P-EMHUB01-HQ.jnpr.net ([fe80::fc92:eb1:759:2c72%11]) with mapi; Thu, 13 Jan 2011 12:20:43 -0800
From: Mukesh Gupta <mukesh@juniper.net>
To: Rio Asnara <asnara101@gmail.com>
Date: Thu, 13 Jan 2011 12:20:41 -0800
Thread-Topic: [VRRP] AD review of draft-ietf-vrrp-unified-mib
Thread-Index: AcuxZw+Fakr9hP4uSJy0XnpXiFNymAB9+Mqg
Message-ID: <497B6D90E0023142AF34948DEFFAB38D3B35815809@EMBX01-HQ.jnpr.net>
References: <00a701cbaf6b$6dfdc460$49f94d20$@huawei.com> <9FFC3234F1B7F0439C9B8BF94A83F482153242AA3F@USEXCHANGE.ad.checkpoint.com> <DD265143-A586-44F3-A9E8-EA3AEA7A90CF@juniper.net> <AANLkTi=n3=Z=Vmf0SmrCoSkUhHdAG0zei7JFscHVgHtb@mail.gmail.com>
In-Reply-To: <AANLkTi=n3=Z=Vmf0SmrCoSkUhHdAG0zei7JFscHVgHtb@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
acceptlanguage: en-US
Content-Type: multipart/alternative; boundary="_000_497B6D90E0023142AF34948DEFFAB38D3B35815809EMBX01HQjnprn_"
MIME-Version: 1.0
X-Mailman-Approved-At: Thu, 13 Jan 2011 12:51:43 -0800
Cc: "vrrp@ietf.org" <vrrp@ietf.org>, "vrrp-chairs@tools.ietf.org" <vrrp-chairs@tools.ietf.org>, "Adrian.Farrel@huawei.com" <Adrian.Farrel@huawei.com>, "draft-ietf-vrrp-unified-mib@tools.ietf.org" <draft-ietf-vrrp-unified-mib@tools.ietf.org>
Subject: Re: [VRRP] AD review of draft-ietf-vrrp-unified-mib
X-BeenThere: vrrp@ietf.org
X-Mailman-Version: 2.1.9
Precedence: list
List-Id: Virtual Router Redundancy Protocol <vrrp.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/listinfo/vrrp>, <mailto:vrrp-request@ietf.org?subject=unsubscribe>
List-Archive: <http://www.ietf.org/mail-archive/web/vrrp>
List-Post: <mailto:vrrp@ietf.org>
List-Help: <mailto:vrrp-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/vrrp>, <mailto:vrrp-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 13 Jan 2011 20:19:39 -0000

Asnara,

Thanks for your interest.  Do you have any timeline in mind?

As you might know, IETF believes in rough consensus and running code.  So, having a few implementations of the draft would make all of us comfortable with the quality of the draft.

Regards
Mukesh

From: Rio Asnara [mailto:asnara101@gmail.com]
Sent: Tuesday, January 11, 2011 12:12 AM
To: Mukesh Gupta
Cc: Kalyan (Srinivas)Tata; vrrp-chairs@tools.ietf.org; Adrian.Farrel@huawei.com; draft-ietf-vrrp-unified-mib@tools.ietf.org; vrrp@ietf.org
Subject: Re: [VRRP] AD review of draft-ietf-vrrp-unified-mib

All,
I'm interested to implemented on my virtual system :)

Regards,
Asnara


On Tue, Jan 11, 2011 at 5:56 AM, Mukesh Gupta <mukesh@juniper.net<mailto:mukesh@juniper.net>> wrote:
Kalyan,

Thanks a lot.  Please go ahead and address the comments and publish
the next rev. We all want this draft done asap :)

- Mukesh

Sent from my iPhone

On Jan 10, 2011, at 2:12 PM, "Kalyan (Srinivas)Tata" <stata@checkpoint.com<mailto:stata@checkpoint.com>
 > wrote:

> Thanks Adrian for the review. I will update the draft (Once WG
> chairs confirm). I will getback if I need any clarifications.
>
> Thanks
> Kalyan
>
> -----Original Message-----
> From: vrrp-bounces@ietf.org<mailto:vrrp-bounces@ietf.org> [mailto:vrrp-bounces@ietf.org<mailto:vrrp-bounces@ietf.org>] On Behalf
> Of Adrian Farrel
> Sent: Saturday, January 08, 2011 11:37 AM
> To: draft-ietf-vrrp-unified-mib@tools.ietf.org<mailto:draft-ietf-vrrp-unified-mib@tools.ietf.org>
> Cc: vrrp-chairs@tools.ietf.org<mailto:vrrp-chairs@tools.ietf.org>; vrrp@ietf.org<mailto:vrrp@ietf.org>
> Subject: [VRRP] AD review of draft-ietf-vrrp-unified-mib
>
> Hi,
>
> Don't panic!
>
> I have performed my AD review of your draft. The purpose of the
> review is to catch any nits or issues before the document goes
> forward to IETF last call and IESG review. By getting these issues
> out at this stage we can hope for a higher quality review and a
> smoother passage through the process.
>
> There are a good number of small issues that I believe can be fixed
> really easily.
>
> I appreciate that a number of these issues are inherited from RFC
> 2787, but this is an ideal chance to clean up.
>
> You will need a new revision to address these points, and I'd ask
> the WG chairs to evaluate whether the changes are large enough to
> warrant a further WG last call.
>
> I have moved the draft into "AD-review:Revised-ID-needed" state in
> the datatracker, and I look forward to seeing the new revision which
> I can put forward for IETF last call.
>
> Thanks for all your work with this draft,
>
> Adrian
>
> ---
>
> Document header
>
> OLD
>   Document: draft-ietf-vrrp-unified-mib-08.txt               July 2010
>   Intended Status: Proposed Standard
> NEW
>   Document: draft-ietf-vrrp-unified-mib-08.txt               July 2010
>   Obsoletes: 2787 (if approved)
>   Intended Status: Proposed Standard
> END
>
> ---
>
> Abstract
>
> OLD
>   This specification defines a Management Information Base (MIB) for
> NEW
>   This specification defines a portion of the Management Information
>   Base (MIB) for
> END
>
> ---
>
> Section 2
>
> OLD
>   This specification defines a Management Information Base (MIB) for
> NEW
>   This specification defines a portion of the Management Information
>   Base (MIB) for
> END
>
> ---
>
> Section 2 etc.
>
> Is it necessary to introduce the term "IPvX"? It is only used a
> couple of times, and the time it is used in the MIB module is a
> problem because the definition of the term is outside the module.
> (Typically, MIB modules are extracted from RFCs and have to survive
> as standalone
> text.)
>
> Can you:
> - remove "(IPvX)" form section 2
> - remove the definition for section 3
> - say "IPv4 and IPv6" in the two uses in section 6 and section 9
>
> ---
>
> Section 4
>
> You need to add text to describe what has changed from 2787. Not a
> lot of details - perhaps a series of bullet points.
>
> --
>
> Section 6 etc.
>
> You need to say "MIB module" not "MIB" because there is only one
> MIB, and you are making just a module in the MIB.
>
> s/This MIB is designed/This MIB module is designed/
>
>
> ---
>
>
> Section 7
>
>   Tables in the MIB include the following:
>
> In fact, there are exactly these three tables, so how about...
>
>   This MIB module contains three tables:
>
> ---
>
> Section 8
>
> Trivial, but...
>
>           -----   MIB Tables For VRRP Router "VR 1":   -----
>
> ...should read "VR1"
> Similarly for VR2 later in the section.
>
> ---
>
> IMPORTS
>
> It is helpful, but mandatory, to show the RFC numbers from which
> things are imported. Thus...
>
> OLD
>       IMPORTS
>           MODULE-IDENTITY, OBJECT-TYPE,
>           NOTIFICATION-TYPE, Counter32,
>           Integer32, mib-2, Unsigned32        FROM SNMPv2-SMI
>
>           TEXTUAL-CONVENTION, RowStatus,
>           MacAddress, TruthValue, TimeStamp,
>           TimeInterval                        FROM SNMPv2-TC
>
>           MODULE-COMPLIANCE, OBJECT-GROUP,
>           NOTIFICATION-GROUP                  FROM SNMPv2-CONF
>           ifIndex                             FROM IF-MIB
>           InetAddressType, InetAddress        FROM INET-ADDRESS-MIB;
> NEW
>       IMPORTS
>           MODULE-IDENTITY, OBJECT-TYPE,
>           NOTIFICATION-TYPE, Counter32,
>
>           Integer32, mib-2, Unsigned32
>               FROM SNMPv2-SMI                               -- RFC2578
>
>           TEXTUAL-CONVENTION, RowStatus,
>           MacAddress, TruthValue, TimeStamp,
>           TimeInterval
>               FROM SNMPv2-TC                                -- RFC2579
>
>           MODULE-COMPLIANCE, OBJECT-GROUP,
>           NOTIFICATION-GROUP
>               FROM SNMPv2-CONF                              -- RFC2580
>
>           ifIndex
>               FROM IF-MIB                                   -- RFC2863
>
>
>           InetAddressType, InetAddress
>               FROM INET-ADDRESS-MIB;                        -- RFC3291
> END
>
> ---
>
> Section 9
>
> In order to ensure that references can be provided, it is customary
> to begin Section 9 (i.e. before the module BEGIN statement) with
> some text such as:
>
>   This MIB module makes reference to the following documents
> [RFC2578],
>   [RFC2579], [RFC2580], [RFC2863], [RFC3291], and [RFC4001].
>
> ---
>
> Vrrpv3VrIdTC
>
> Typos
>               (ifIndex)and IP version, serves to uniquely identify a
> Missing space.
>
>           REFERENCE " RFC 5798 (Sections 3 and 5.2.3"
> Missing close brace.
>
> ---
>
> vrrpv3OperationsTable
>
>               "Unified Operations table for a VRRP router which
>                consists of a sequence (i.e., one or more conceptual
>                rows) of 'vrrpv3OperationsEntry' items which describe
>                the operational characteristics of a virtual router."
>
> I think "which describe" should read "each of which describes"
>
> ---
>
> vrrpv3OperationsEntry
>
>                Rows in the table cannot be modified unless the value
>                of 'vrrpv3OperStatus' has transitioned to
>                'initialize' state.
>
> I think a little more precision would help...
>
>                A rows in this table cannot be modified unless the
> value
>                of 'vrrpv3OperStatus' in the row has transitioned to
>                'initialize' state.
>
> ---
>
> vrrpv3OperationsInetAddrType
>
> As far as I can tell, you only support two values: ipv4(1) and ipv6
> (2).
> Other values of the InetAddressType textual conventions are, I
> think, not supported.
>
> You should add this fact as a note to the DESCRIPTION clause.
>
> ---
>
> vrrpv3OperationsPrimaryIpAddr
>
>               "In the case where there are more than one IP
>
> s/are/is/
>
> ---
>
> vrrpv3OperationsVirtualMacAddr
>
>           REFERENCE "STD 58 RFC 2578"
>
> I am not clear why this reference is cited. MacAddress is defined in
> RFC 2579, but you don't need to provide a reference because that is
> implicit in the IMPORTS clause. Perhaps you mean to give a reference
> to where the mapping from VRID to MAC address is defined?
>
> ---
>
> vrpv3OperStatus
>
> This object is incongruously named.
> To fit with the naming convention for the table you need one of:
> - vrpv3OperationsStatus
> - vrpv3OperationsOperStatus
>
> Note that there are many references to this object throughout the
> document.
>
> ---
>
> vrrpv3OperationsAcceptMode
>
>               "Controls whether a virtual router in Master state
>               will accept packets addressed to the address owner's
>               IPv6 address as its own if it is not the IPv6 address
>               owner.  Default is False.
>               This object is not relevant for rows representing VRRP
>               over IPv4 and should be set to false."
>
> Should read...
>
>   Default is false(2)
>
> ...and...
>
>   should be set to false(2)."
>
> ---
>
> vrrpv3OperationsUpTime
>           SYNTAX       TimeStamp
>           MAX-ACCESS   read-only
>           STATUS       current
>           DESCRIPTION
>               "This is the value of the `sysUpTime' object when this
>               virtual router (i.e., the `vrrpv3OperStatus')
>               transitioned out of `initialized'."
>
> I am not saying that you MUST change this, but I wonder how useful
> it is, because to make sense of it, a management station must also
> read the current value of sysUpTime.
>
>
> An alternative is to supply the up time in timer ticks. That means
> that the management agent has to perform a computation each the row
> is read. If you did this you would have...
>
>           SYNTAX       TimeTicks
>           MAX-ACCESS   read-only
>           STATUS        current
>           DESCRIPTION
>               "This value represents the amount of time since this
>               virtual router (i.e., the `vrrpv3OperStatus')
>               transitioned out of `initialize'."
>
> I just need you to think about which you prefer (there is a trade-
> off) and make a decision. No need to tell me which you chose, or why.
>
> Note also s/initialized/initialize/
>
> ---
>
> Creation and deletion of a vrrpv3OperationsTable row
>
> I'm slightly confused :-(
>
> We have...
>
>       vrrpv3OperationsEntry
>
>                Rows in the table cannot be modified unless the value
>
>                of 'vrrpv3OperStatus' has transitioned to
>                'initialize' state.
>
> and
>
>       vrrpv3OperationsRowStatus
>
>               When `vrrpv3OperationsRowStatus' is set to
>               active(1), no other objects in the conceptual row can
>               be modified.
>
> In general, the instructions in the description of the rowStatus are
> good an detailed. But I have some difficulty with row creation. What
> I think is missing is a statement that the row must be created with
> operStatus set to initialize(1) and cannot transition to backup(2) or
> master(3) until rowStatus is transitioned to active(1).
>
> Similarly, there is an issue with row deletion. In order to delete,
> the row I must first set rowStatus to notInService(2), and later to
> delete(6). But, according to the description of operStatus, I cannot
> modify the row (including the rowStatus) until operStatus has gone
> to initialize(1). How do I get that to happen?
>
> I suspect this can be fixed by allowing rowStatus to be changed
> regardless of the value of operStatus.
>
> ---
>
> vrrpv3AssociatedIpAddrTable
>
>               "The table of addresses associated with this virtual
>                router."
>
> I think that there is just one table, and it contains the addresses
> of all virtual routers. What about...
>
>               "The table of addresses associated with each virtual
>                router."
>
> ---
>
> vrrpv3AssociatedIpAddrEntry
>
> OLD
>               Rows in the table cannot be modified unless the value
>               of `vrrpv3OperStatus' has transitioned to
>               `initialize'.
> NEW
>               Rows in the table cannot be modified unless the value
>               of `vrrpv3OperStatus' for the corresponding entry in the
>               vrrpv3OperationsTable has transitioned to initialize(1).
> END
>
> ---
>
> vrrpv3AssociatedIpAddr
>
> This object's name is odd given the convention for naming objects
> within their tables. You probably need:
>
> vrrpv3AssociatedIpAddrAddress
>
> ---
>
> vrrpv3AssociatedIpAddr
>
> You should add a statement the description that says that the
> content of the object is to be interpreted in the context of the
> setting of vrrpv3OperationsInetAddrType in the index of this row.
>
> ---
>
> VRRP Router Statistics
>
> Do you need a discontinuity timer for the three global objects:
> - vrrpv3RouterChecksumErrors
> - vrrpv3RouterVersionErrors
> - vrrpv3RouterVrIdErrors
>
> ---
>
> VRRP Router Statistics
>
> In the presence of an attack or a broken router or host nearby, is
> it possible that Countr32 will not be large enough for the up-time
> of this router?
>
> You can choose to use Counter64 or describe wrapping conditions.
>
> ---
>
> vrrpv3RouterVrIdErrors
>
>               "The total number of VRRP packets received with an
>               invalid VRID for this virtual router."
>
> This object is global, so it is wider than the scope of a single VR.
> I think you need:
>
>               "The total number of VRRP packets received with a
>               VRID that is not valid for any virtual router on this
>               router."
>
> ---
>
> vrrpv3StatisticsAdvIntervalErrors
>
>               "The total number of VRRP advertisement packets
>               received for which the advertisement interval is
>               different than the one configured for the local virtual
>               router.
>
> Can you add a reference to vrrpv3OperationsAdvInterval
>
> ---
>
> vrrpv3ProtoError
>
> Don't you think this is a *really* dangerous notification?
>
> If a VR is under attack or receiving packets from a faulty speaker,
> it will spew notifications.
>
> You should probably either add some thresholding objects (which is a
> fair bit of work) or a single object to turn notifications on and
> off (with the default being "off").
>
> ---
>
> Notifications
>
> As currently specified, both of the notifications will come from the
> management agent which will identify the physical router that
> sourced the notification, but not the VR.
>
> Don't you need to add some index values to the notifications as well?
>
> ---
>
> Section 11
>
> Since you are obsoleting RFC 2787, is it your intention to ask IANA
> to deprecate {mib-2 68} ?
>
> ---
>
> Would you please consider adding
>
> ---
>
> Section 12
>
> A reference to RFC 4001 needs to be added as it shows up in some
> REFERENCE clauses.
>
> ---
>
> Would you please consider adding a short section on migrating from
> VRRP-MIB to VRRPV3-MIB?
>
> _______________________________________________
> vrrp mailing list
> vrrp@ietf.org<mailto:vrrp@ietf.org>
> https://www.ietf.org/mailman/listinfo/vrrp
>
> Scanned by Check Point Total Security Gateway.
> _______________________________________________
> vrrp mailing list
> vrrp@ietf.org<mailto:vrrp@ietf.org>
> https://www.ietf.org/mailman/listinfo/vrrp
_______________________________________________
vrrp mailing list
vrrp@ietf.org<mailto:vrrp@ietf.org>
https://www.ietf.org/mailman/listinfo/vrrp