Subject: [VRRP] Re: Re: MIB Dr. Review of draft-ietf-vrrp-unified-mib-05.txt

From: <[email-masked]>
Date: Thu, 13 Jul 2006 07:32:36 -0400

> 
> ----- Original Message ----- 
> From: <[email-masked]>
> To: <[email-masked]>; <[email-masked]>
> Cc: <[email-masked]>; <[email-masked]>
> Sent: Tuesday, July 11, 2006 8:05 PM
> Subject: RE: MIB Dr. Review of draft-ietf-vrrp-unified-mib-05.txt
> 
> 
> Hi Joan,
> Thanks once again for the detailed review. I was shooting for some
> deadline at work and couldn't
> Reply in detail earlier.
> 
> My comments/ explanation  inline : (I removed the parts of your mail to
> which I agree and do not have comments )
> 
> 

Great!  Thanks for your reply.
My comments are inline.

-Joan

> 
> 5) I consulted with other MIB Doctors and the rough consensus was to
> leave deprected objects where they are in the MIB, even though they are
> deprecated but to make it very obvious that they are deprected.
> Also, Guidelines state to add the reason why they have been deprecated
> to the DESCRIPTION.
> 
> -- I moved the order to the end of the MIB based on Bill Fenner's review
> comments in the last revision.
> -- Will move it back if it is required.
> 

This was discussed in an email to Bill Fenner.  Essentially, it is okay to
leave the objects where they are.

>       vrrpNodeVersion  OBJECT-TYPE  --  ****  DEPRECATED OBJECT ****
>         SYNTAX       Integer32
>         MAX-ACCESS   read-only
>         STATUS       deprecated
>         DESCRIPTION
>            "This value identifies the particular version of the VRRP
>             supported by this node.
>             This object is deprecated in the IP Version Independent
>             MIB.
> 
>             This object has been deprecated and replaced by
>             the vrrpOperationsVersion object in the
> vrrpOperationsTable."
>         ::= { vrrpOperations 1 }
> 
> Also, please add to the DESCRIPTION clause why the object has been
> deprecated/obsoleted.
> 
> -- I thought "This object is deprecated in the IP Version Independent
> MIB." would indicate that
> --  the object is deprecated infavor of a version independent object.
> -- Is the following a better way of describing than what I have above? 
> -- " This object has been deprecated and replaced by IP version neutral
> xxxx object."

Yes, please specify by name the new object which replaces this one.

> ----- Snip ---
> 
> >2. Every deprecated object needs an explanation in the DESCRIPTION
> saying why it's deprecated (RFC 2578, section >10.2 (3)).  Text like
> "This object is deprecated in favor of the IP Version Independent
> object, vrrpFoobarObject" 
> >seems appropriate.  Similarly for the deprecated Groups.
> 

Yes.

> > 4. Is it feasible to the deprecated objects to the end (or, rather,
> middle, just before the groups) to make itmore > readable for someone
> who wants to come and see the current stuff?
> 
> -- End Snip ----
> 

Discussed in an email to Bill Fenner.

> 8) Could notifications be generated or not, using the
>   the notification MIB and target MIB (RFC2583)?
>   I don't see a need for this object, but please let me
>   know the answer to the above question.
> 
> -- I did not understand this. Are you saying a particular object is not
> Required in the notification?
> 
> 

Sorry, I meant to specify that the vrrpNotificationCntrl object
seemed to be specifying whether or not a notification would 
be sent, but that is now done by the MIB Modules in RFC2583,
unless there is another purpose for this object, then I suggest
it be removed.

> 
> Again, with regard to this DESCRIPTION clause:
> 
>                 Rows in the table cannot be modified unless the value
>                 of 'vrrpOperationsState' has transitioned to
>                 'initialize'"
> 
> Why was the ability to change the OperationsState removed?
> In other words, the original table had an AdminState object, but this
> table does not.  Please explain.  Typically, there is an AdminState type
> object so that an operator is able to force a transition to a different
> state.
> 
> -- This change was done after MIB Dr. review of Dave Thaler. 
> -- Snip from previous discussion on this with Dave Thaler --- 
> 
> > 5) Regarding vrrpOperationsAdminState vs vrrpOperationsRowStatus, why 
> > do you need both?  Why isn't the row status sufficient?
> > What's the difference between CreateAndWait vs CreateAndGo with 
> > AdminState=down?
> > 
> > -- As you are aware, Rowstatus only indicates the status of the row 
> > (whether the creation of the row is complete or not) while 
> > OperationsState
> indicates
> > the
> > state of Virtual router the row indicates. I guess you are saying that
> we
> > could
> > use rowStatus valid/invalid to convey the meaning of adminstatus up & 
> > down.
> > Is my understanding correct?
> 
> RowStatus does not only indicate whether creation of the row is complete
> or not.  The 'notInService' value of the RowStatus is meant to be used
> for adminstatus down for tables using RowStatus.  From RFC 2579:
>     - `notInService', which indicates that the conceptual
>     row exists in the agent, but is unavailable for use by
>     the managed device (see NOTE below); 'notInService' has
>     no implication regarding the internal consistency of
>     the row, availability of resources, or consistency with
>     the current state of the managed device;
> 
> As shown in the table on page 9 of 2579, a normal use case of
> notInService is to change the row to that from the active state (i.e.
> administratively
> take it down).
> 
> > If you think this is required, I will 
> > change it.
> 
> I think it would make more sense, and reduce the implementation
> complexity, yes.
>  
> 
> -- end snip ---

I wil get back to you on this. 

> 
> 11)           INDEX    { vrrpOperationsInetAddrType,
>                       vrrpOperationsVrId, ifIndex }
> 
>    W: f(VRRP-MIB), (132,23) Row "vrrpOperationsEntry" does not have a
>    consistent indexing scheme - index items from current table must
>    come after index items from other tables
> 
> Wrt to the above warning, did want to ask about the indexing used here?
> In the original MIB ifIndex appears first, so why is this changed here.
> 
> -- I had a discussion with Dave Thaler and  Wijnen Bert a while back on
> the order and was decided that 
> -- operators would like to list all the v4 or v6 entries rather than
> based on VRID or ifIndex.
> 
> 

Okay.  That is a good reason.

> Also, there is a blurb in the document itself which says that ifIndex
> represents the physical interface, is that still the case with this
> ifIndex?  If so, please mention this someplace in the Table or Entry
> DESCRIPTION clause.
> 
> -- Will clarify this.
> 
> 
> 
> 13)
>        vrrpOperationsVersion OBJECT-TYPE
>            SYNTAX       INTEGER {
>                vrrpv2   (1),
>                vrrpv3   (2)
>            }
>            MAX-ACCESS   read-create
>            STATUS       current
>            DESCRIPTION
>                "This object contains the VRRP version on which this
>                 VRRP instance is running."
> 
>            ::= { vrrpOperationsEntry 6 }
> 
> 
> Wanted to check and make sure that the version of the VRRP protocol is
> settable (object is read-create).
> Did you intend for this to be read-only or read-create?
> 
> The DESCRIPTION makes the object sound like a read-only object.
> 
> Also, would the value of vrrpOperationsInetAddrType dictate the version?
> Not sure why this object is necessary.
> 
> -- I was supposed to remove this object and I think I missed this one. I
> will remove it.
> 

Thanks.

> 14)
>        vrrpOperationsAddrCount OBJECT-TYPE
>            SYNTAX       Integer32 (0..255)
> 
> Could you explain why there would only be a maximum of 255?  Also, this
> should probably be Gauged32(0..255)
> 
> Would also change the name of the object since it does not really seem
> to be a counter.  Maybe something like vrrpOperationsCurrentAddrs
> 
> -- VRRP protocol limits the number of associated addresses to be 255
> which is what this object indicates. 
> -- "Count IP Addrs" field is 8 bits.
> -- I will change it to Gauge32(0..255), will also change the name.

Thank you.  Please also, add a REF clause to refer to the
Count IP Addrs field.  I see that the name is for the field now, so 
that is why you used the word Count.

> 
> 15)       vrrpOperationsPrimaryIpAddr OBJECT-TYPE
> 
> Does this object have a default value?
> The original object does, so wanted to check on that.
> 
> -- I removed the default value because it will be different based on
> vrrpOperationsInetAddrType. 
> -- I will probably mention this in the description of the object.
>  
> 

okay.

> 18)        vrrpOperationsStorageType OBJECT-TYPE
> 
> The description is confusing.  I would think that most implementations
> would want to support nonVolatile(3) or something even more stable.
> 
> Please explain the choice of volatile(2)?  Also, please consider adding
> a DEFVAL for this object.
> 
> -- I meant to say that the implimentations can implement as 'volatile'.
> If they chose to implement as 'nonVolatile'
> That would be good too. I will rephrase the description.
> 
> 

okay.

> 
> 19)       vrrpOperationsRowStatus OBJECT-TYPE
> 
> 
> notInService(2) should be used to administratively bring the row down,
> does this have an impact on the vrrpOperationsState object?
> (I think this gets back to my point of having an Admin object, the
> notInService refers to the row not being available for a manager, is
> this what you mean, or are you trying to say that the vrrp is
> notInService ??
> 
> -- I mean that the row is 'notInService' and not VRRP . 
> 
> Adding a Row:
> Please list again the objects which need to have valid values for #2.
>                2. Populate the vrrpOperationsEntry with at least
>                minimal elements.
> 
> 
> Are steps 2 and 3 out of order?
> 
> Deleting a row:
> Can the same row in the vrrpAssociatedIpAddrTable be used by more than
> one row in the vrrpOperationsTable?
> 
> - No.
> 
> Also, if you set the vrrpAssociatedIpAddrTable row to notInService(2),
> do you need to go back and delete it, after you delete the corresponding
> row in the vrrpOperationsTable?
> 
> 
> 
> 22) vrrpAssociatedStorageType
> 
> Same questions/concerns about StorageType here as in the previous table.
> 
> -- Again I meant that 'volatile' is allowed. I will change the wording.
> 

okay.

> 
> 24) vrrpRouterStatisticsTable
> 
> The index is the same as the vrrpOperationsTable so have a question as
> to why this table does not use AUGMENTS?
> 
>    W: f(VRRP-MIB), (563,23) Row "vrrpRouterStatisticsEntry" does not
>    have a consistent indexing scheme - cannot specify an index item
>    from additional "base row" vrrpOperationsEntry, since can
>    have only one "base row" which is ifEntry
> 
> Also, as discussed above, have a question as to why ifIndex is not used
> first?
> 
> -- Good point. I will change the table to use AUGMENTS. 
> 

okay.

> 25) naming of objects in vrrpRouterStatisticsTable:  Counters usually
> end in "s".  Some of these counters do, so please try to do this, for
> example:  vrrpStatisticsBecomeMaster could be
> vrrpStatisticsMasterTransitions,
> 
> vrrpStatisticsAdvertiseRcvd could be vrrpStatisticsRcvdAdvertisements
> and so forth.
> 
> vrrpStatisticsInvldTypePktsRcvd could be
> vrrpStatisticsRcvdInvalidTypePkts
> 
> (Also, please spell out some of these names, such as Priority/Pri,
> Advertisement/Adv).
> 
> -- Will do.
> 

okay.

> 26)    vrrpStatisticsRefreshRate
> 
> Not sure that I fully understand the benefit of this object.
> Could you please explain why this is needed for vrrp?
> 
> -- I thought this would be a good idea for implementations that may have
> to do costly processing (like reading from kernel) to update the
> statistics.  
> 
> 

okay.

> 29) Concern on the use of accessible-for-notify:  it might be a good
> idea to make these objects read-only, along with a timestamp type of
> object.  Notifications are unreliable and so, could be lost.  Also, some
> customers do not like to enable a lot of notifications and prefer
> polling.
> Would you consider making these objects read-only?
> 
> -- Will do.
> 
> 

Thanks,
  -Joan

> 
> 
> Thanks again for the review.
> -Kalyan

_______________________________________________
vrrp mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/vrrp

---

From: <[email-masked]>
Date: Thu, 13 Jul 2006 12:42:00 -0700

Thanks Joan again for an excelent review.  
I will make the changes you mntioned and get back to you if
I have any other questions.

Thanks
Kalyan

-----Original Message-----
From: ext [email-masked] [mailto:[email-masked]] 
Sent: Thursday, July 13, 2006 4:33 AM
To: Tata Kalyan (Nokia-ES/MtView); [email-masked]
Cc: [email-masked]; [email-masked]
Subject: [VRRP] Re: Re: MIB Dr. Review of
draft-ietf-vrrp-unified-mib-05.txt

> deadline at work and couldn't Reply in detail earlier.
> 
> My comments/ explanation  inline : (I removed the parts of your mail 
> to which I agree and do not have comments )
> 
> 

-Joan

> leave deprected objects where they are in the MIB, even though they 
> are deprecated but to make it very obvious that they are deprected.

> to the DESCRIPTION.
> 
> -- I moved the order to the end of the MIB based on Bill Fenner's 
> review comments in the last revision.

This was discussed in an email to Bill Fenner.  Essentially, it is okay
to leave the objects where they are.

> -- " This object has been deprecated and replaced by IP version 
> neutral xxxx object."

Yes.

> -- I did not understand this. Are you saying a particular object is 
> not Required in the notification?
> 
> 

Sorry, I meant to specify that the vrrpNotificationCntrl object seemed
to be specifying whether or not a notification would be sent, but that
is now done by the MIB Modules in RFC2583, unless there is another
purpose for this object, then I suggest it be removed.

Thanks.

okay.

okay.

okay.

okay.

okay.

okay.

Thanks,
  -Joan