Re: [Idr] Early MIB Dr. Review of draft-ietf-idr-bgp4-mibv2-06.txt

["Joan Cucchiara" <[e-mail]>]

Hi Jeff,

----- Original Message ----- 
From: "Jeffrey Haas" <[e-mail]>
To: "Joan Cucchiara" <[e-mail]>
Cc: <[e-mail]>; "Dan Romascanu (E-mail)" <[e-mail]>; "David
Ward" <[e-mail]>; <[e-mail]>; "MIB Doctors (E-mail)"
<[e-mail]>
Sent: Saturday, May 03, 2008 6:37 PM
Subject: Re: Early MIB Dr. Review of draft-ietf-idr-bgp4-mibv2-06.txt


> Joan,
>
> Thank you for the quick review and many apologies a delayed response.
> Please find my response inline.  Once your issues and any further issues
> raised by the working group have been addressed, I will issue a new
> version of this MIB.
>

Great!

> Working group - within this response are sections marked:
> *** Working group feedback requested:
>
> Even if you're not strongly interested in MIB work, please inspect these
> sections as it may have further standardization implications.
>
> On Fri, Mar 21, 2008 at 08:54:50AM -0500, Joan Cucchiara wrote:
>> First, MIB compiler output is given, followed by "General Comments"
>> on the document, then "Specific Comments".
>
> I hadn't run smicng on this.  I will add this to my toolkit for the
> future.
>

Thank you!

>> SMICNG output:
>> =============
>> W: f(BGP4-MIB), (30,22) The first revision should
>> match the last update for MODULE-IDENTITY bgp
>
> Corrected.
>
>> E: f(BGP4-MIB), (1834,13) Item "bgpAfPathAttrUnknownFlags"
>> in sequence "BgpAfPathAttrUnknownEntry" has conflicting syntax specified
>
> Corrected.
>
>> E: f(BGP4-MIB), (233,13) Index item "bgpPeerAfInstance" must be
>> E: f(BGP4-MIB), (1041,13) Index item "bgpPeerAfInstance" must be
>> E: f(BGP4-MIB), (1152,13) Index item "bgpNlriIndex" must be
>> E: f(BGP4-MIB), (1153,13) Index item "bgpPeerAfInstance" must be
>> E: f(BGP4-MIB), (1362,13) Index item "bgpPeerAfInstance" must be
>> E: f(BGP4-MIB), (1371,13) Index item "bgpAdjRibsOutIndex" must be
>> E: f(BGP4-MIB), (1445,13) Index item "bgpAfPathAttrIndex" must be
>> E: f(BGP4-MIB), (1711,13) Index item "bgpAsPathIndex" must be
>> E: f(BGP4-MIB), (1712,13) Index item "bgpAsPathSegmentIndex" must be
>> E: f(BGP4-MIB), (1713,13) Index item "bgpAsPathElementIndex" must be
>> E: f(BGP4-MIB), (1825,13) Index item "bgpAfPathAttrUnknownIndex"
>> E: f(BGP4-MIB), (1826,13) Index item "bgpAfPathAttrUnknownCode"
>> defined with syntax that includes a range
>
> This was intentional.  The full range was to be made available for
> implementation since this is an opaque handle.  Did you want the
> Unsigned32 range to be added?
>

Yes, the range should be specified since it provides an indication that
this was intentional.

Based on your response, could the range start at 1?
In other words, since this index is an opaque handle and since the
range is large, RFC2578, Section 7.7 requests that an index like
this should start at 1, since zero should be avoided.
The specific quote is:
   "Instances identified by use of integer-valued objects should be
   numbered starting from one (i.e., not from zero).  The use of zero as
   a value for an integer-valued index object should be avoided, except
   in special cases."


>> E: f(BGP4-MIB), (1228,5) Item "bgpNlriPrefixType" has invalid
>> value for MAX-ACCESS
>
> I don't see an error here?

This is not listed in the INDEX clause and it has a MAX-ACCESS of
"not-accessible".    Is this supposed to be part of the INDEX?  If not,
then it should have a different type of MAX-ACCESS.

bgpNlriPrefixType OBJECT-TYPE
        SYNTAX InetAddressType
        MAX-ACCESS not-accessible


>
>> W: f(BGP4-MIB), (1703,5) Sequence "BgpAsPathEntry" and
>> Row "bgpAsPathTableEntry" should have related names
>
> I have globally substituted bgpAsPathTableEntry for bgpAsPathEntry.
>
>> W: f(BGP4-MIB), (2689,5) Table "bgpRcvdPathAttrTable" and
>> Row "bgpPathAttrEntry" should have same name prefix
>
> This issue was previously existing for earlier versions of this MIB and
> cannot be changed.
>

I would encourage the working group to fix these sorts of obvious mistakes
rather than carry them forward for a few reasons:

1) A great deal of this MIB is changing already so the working group may 
want
to reconsider fixing this mistake,
2) MIB tools which generate code "might" find errors here and so developers
are left dealing with fixing these sorts of bugs by going in and editing
files that are generated (usually not what we want to do).
3)  If the intention is to make this MIB one that is the basis for other BGP
MIBs then having a solid foundation to build upon is important IMHO, so 
again
would request that the working group consider this.  Certainly, I will abide
by their decision.

>> W: f(BGP4-MIB), (1148,13) Row "bgpNlriEntry" does not have a
>> consistent indexing scheme - index items from current table
>> must come after index items from other tables
>
> This was intentionally done to mirror the semantics of bgpPathAttrEntry.
> This yields a MIB walk that presents common prefixes first followed by
> the peers that advertise them.
>
>> W: f(BGP4-MIB), (1362,13) Row "bgpAdjRibsOutEntry" does not have
>> a consistent indexing scheme - cannot specify an index
>> item from additional "base row" bgpPeerAfEntry, since can have only
>> one "base row" which is bgpNlriEntry
>
> This table, however, wasn't in a consistent order with bgpNlriEntry.
> I've corrected that.
>
>> W: f(BGP4-MIB), (1445,13) Row "bgpAfPathAttrEntry" does not have a
>> consistent indexing scheme - index item bgpAfPathAttrIndex from base
>> row bgpNlriEntry is not defined as an index item
>>
>> W: f(BGP4-MIB), (1711,13) Row "bgpAsPathTableEntry" does not have a
>> consistent indexing scheme - index item bgpAsPathIndex from base row
>> bgpAfPathAttrEntry is not defined as an index item
>>
>> W: f(BGP4-MIB), (1825,13) Row "bgpAfPathAttrUnknownEntry" does not have a
>> consistent indexing scheme - index item bgpAfPathAttrUnknownIndex from
>> base row bgpNlriEntry is not defined as an index item
>
> These tables contain common information that may be shared by a number of
> entries within a given table and thus represents a many to one
> relationship.  Is there a different way you would prefer to see this
> represented?
>

Randy and Bert addressed this one!


>> W: f(BGP4-MIB), (3144,19) Variable "bgpPeerRemoteAddr" in notification
>> "bgpEstablishedNotification" is an index for a table
>>
>> W: f(BGP4-MIB), (3158,19) Variable "bgpPeerRemoteAddr" in notification
>> "bgpBackwardTransNotification" is an index for a table
>
> These issues are the result of the SMIv1 to SMIv2 porting issues that
> were largely addressed by RFC 4273.  Consensus at the time was to not
> alter the MAX-ACCESS of those objects.
>

Would again suggest fixing mistakes likes these for the reasons stated above
wrt the other error.

>> E: f(BGP4-MIB), (1970,15) Group "bgpTimersGroup" is both a MANDATORY
>> and conditional group for module "BGP4-MIB"
>>
>> E: f(BGP4-MIB), (1973,15) Group "bgpCountersGroup" is both a MANDATORY
>> and conditional group for module "BGP4-MIB"
>>
>>
>> E: f(BGP4-MIB), (1979,15) Group "bgpBaseGroup" is both a MANDATORY and
>> conditional group for module "BGP4-MIB"
>>
>> E: f(BGP4-MIB), (1982,15) Group "bgpErrorsGroup" is both a MANDATORY and
>> conditional group for module "BGP4-MIB"
>>
>> E: f(BGP4-MIB), (1985,15) Group "bgpPeerAfGroup" is both a MANDATORY and
>> conditional group for module "BGP4-MIB"
>>
>> E: f(BGP4-MIB), (1988,15) Group "bgpAfPathAttributesGroup" is both a
>> MANDATORY
>> and conditional group for module "BGP4-MIB"
>
> These have been corrected to be only in the MANDATORY-GROUPS section.
>
>> E: f(BGP4-MIB), (1976,15) Group "bgpAsPathGroup" is both a MANDATORY
>> and conditional group for module "BGP4-MIB"
>
> Per feedback, this group has been made completely optional.
>
>>
>> SIMPLE WEB
>> ----------
>> Severity level requested: 6
>
> All comments from this output have been addressed above and are
> artifacts of the RFC 4273 work.
>
>> General Comments:
>> -----------------
>>
>> 1) With regard to the Textual Conventions, has there
>> been any thoughts to a separate TC document?
>>
>> The reason is that, there is a strong indication that more
>> BGP MIB Modules will be forthcoming, thus, having a
>> separate TC document would help in 2 possible ways:
>>
>>
>> A. would help to clarify where the TCs were (i.e in a document
>> named (for example)  BGP4-TC-MIB ) and
>>
>> B. may encourage other MIB Module authors to add to the document
>> with their TCs.
>
> Three of the four TEXTUAL-CONVENTIONs in this draft would be appropriate
> to put into a separate document: BgpIdentifierTC, BgpAfiTC and
> BgpSafiTC.  I will separate these into a separate draft for the next rev
> of this document.
>
> The fourth TEXTUAL-CONVENTION, BgpPathAttrFlagsTC seems appropriate to
> leave in this document.  I don't ever expect it to be used elsewhere.
>
>> 2) There aren't any read-write objects in the current MIB Module
>> (except deprecated objects).  I expect this will change in future
>> versions of the document, but it did impact my review, since this
>> kind of information is good to know.  This will likely also impact
>> the Conformance Section of the MIB, since a read-only Conformance
>> might be wanted by the working group.  Not to mention, either
>> StorageType objects or verbage will need to be added.
>
> One of the most contentious portions of feedback to prior structure
> attempts for this MIB was configuration objects or read-write objects
> vs. no-config and read-only.  The working group consensus was that the
> complexity of BGP configuration, especially with regards to policy, was
> so complicated and vendor-specific that trying to represent it in a MIB
> was inappropriate.  The remaining vendor-common functionality, such as
> configuring BGP peering sessions, was the MIB would interfere with
> required vendor-specific knobs.
>

Please point me to the relevant emails in the archive.  I would like to
understand this because the motivation seems contrary to what the IETF is 
trying to do
with Standards.


>> 3) Many objects were using Integer32 (timers for example) and
>> should probably be Unsigned32.
>>
>> Here is relevant text from RFC4181:
>>
>>    "If the value range is between 0..2147483647 (inclusive), and the
>>     value of the information being modelled does not increase above the
>>     maximum value nor decrease below the minimum value, then:
>>
>>     - Unsigned32 is RECOMMENDED;
>>     - INTEGER, Integer32, and Gauge32 are acceptable."
>
> All objects that are Integer32 are part of the SMIv1 to SMIv2 work
> completed in RFC 4273 and cannot be altered.
>

I don't understand the above because these tables are new to this MIB (e.g.
bgpPeerAfConfiguredTimersTable
and bgpPeerAfNegotiatedTimersTable).

>> 4) I have suggested a different structure to this MIB.  Currently,
>> bgp instance, remote peer and peering session information are in
>> the same table (bgpPeerAfTable).  Many tables then AUGMENT this table.
>>
>> The suggestion is to separate bgpPeerAfTable into
>> 3 tables: bgp instance information in one table
>> remote peer information into another table, and
>> peering session information into a 3rd table.
>
> The primary purpose of the bgpPeerAfTable is to provide most of the key
> information relating to the defintion of a BGP peering session.  This
> also gives us a place to put the small pieces of status that are
> pertinent to the status of the peering session including its status and
> other components related to its transport session.

Yes, but there are other tables, for example timers, which are grouped
based on the type of data (i.e. timer) that it is, when some of this is
really related to the bgp router information or the bgp peer connection.
An analogy might be that some of MIB constructs are approached as
object-oriented and some is classic C data structs.   (Modeling a bgpPeer 
and bgpPeer's connection
which is OO, and Timer data which is C.)

Sometimes this type of mixing of two styles is not at all a concern, but I
think there is a better organization that could be done with this MIB.  So, 
for
example, a table which contains info on this Bgp's Instance (so some of the 
timer objects
and other objects  that were  previously configurable would likely be in 
this table),
the current bgpAfPeerTable, and
then possibly a third table to contain information regarding the actual 
session
information (i.e. counters which change when the FSM to established).

I think this is related to the issue of the counters Discontinuity also. 
Some counters
are related to the Peer and some are related to the established session.

Also, I suspect that part of the working group's decision to remove 
configuration objects in the
MIB is due to the underlying structure.  If the organization of a MIB is 
poor, then
knowing where to put objects is difficult, and DESCRIPTIONS can become 
complex.

>
>> This structure would then have a ripple effect throughout the MIB
>> such that you will probably see some counters which AUGMENT the
>> table with bgp instance info, some which would AUGMENT the bgp session,
>> etc.
>
> I believe operationally that keeping this information in a single table
> for walk purposes would be consistent with operator expectations based
> on common CLI output.  I don't think operators really want cluttered MIB
> walks where they have to aggregate the table output themselves to
> correlate against what they were previously getting together.  Compare
> this against bgpPeerTable.
>
> I'm certainly willing to be argued out of this if it's to the benefit of
> operators or future expandability of the MIB.
>

Yes, I agree and would like to hear from operators on this issue also.
I am confused when you talk about MIB walks and CLI output.  Although many
vendors do provide a way to dump MIB data to the CLI and page through it,
I wouldn't necessarily consider that common CLI output since most of this
(at least in my experience) is dictated by the CLI request (i.e. which 
columns to show).


>> 5) InetAddress/InetAddressType  the place for specifying the supported
>> address types and addresses is in the conformance, not in the MIB Module.
>> If there is a change in the supported types, then the objects with those
>> types, would all need to be deprecated, but if it is in the conformance
>> then only the conformance would need to be deprecated and rewritten.
>> The exception being if the address is an index (then a SIZE should be
>> used) see RFC4001 for details. Please see the DIFF-SERV-MIB (rfc3289) as
>> an
>> example of using the Conformance statements to denote Address Type
>> and size.
>
> I have made the suggested changes to the following objects:
> bgpPeerAfLocalAddr, bgpPeerAfRemoteAddr SIZE(4|16|20)
> bgpNlriPrefix SIZE(0..16) - note that the previous SIZE(4..20) was an
> error.
>
> I have left bgpAfPathAttrLinkLocalNextHop alone at SIZE(20) since it is
> constrained by RFC 2545.
>
> *** Working group feedback requested:
> The original defintion of the supported sizes was 4..20.  The intention
> was to support IPv4, IPv6 and to not preclude IPv6 link local peering
> sessions.  Since stronger wording is now required, could I have feedback
> on this verbiage?
>
> OBJECT bgpPeerAfLocalAddr
> SYNTAX InetAddress (SIZE(4|16|20))
> DESCRIPTION
>    "An implementation is required to support IPv4 peering
>     sessions.  An implementation MAY support IPv6 peering
>     sessions.  IPv6 link-local peering sessions MAY
>     supported by this MIB."
>
> The reason for this wording is that we've never come to proper consensus
> about ipv6-only routers.
>
> TODO:
>
>> 6) Deprecated and Obsoleted Objects
>>
>> One of the other MIB Doctors (David T. Perkins) suggested to me
>> the following and I would like to pass this on as a suggestion to you
>>
>> >RFC 2578 has language, sich as that found in section 10.2:
>> >(3)  A STATUS clause value of "current" may be revised as "deprecated"
>> >    or "obsolete".  Similarly, a STATUS clause value of "deprecated"
>> >    may be revised as "obsolete".  When making such a change, the
>> >    DESCRIPTION clause should be updated to explain the rationale.
>> >
>> <snip>
>> >For deprecated items, I start the DESCRIPTION text
>> >with something like..
>> >"This item is DEPRECRATED. This is done because..."
>> >I then leave the original DESCRIPTION text. For obsoleted
>> >items, I generally remove the original DESCRIPTION cause
>> >text.
>>
>> Some of the DESCRIPTION clauses have been updated to various degrees
>> so you are definitely on the right path, but it did seem some places
>> where skipped and so the deprecated and obsoleted objects should be
>> reviewed to ensure that the DESCRIPTIONs have been modified.
>
> I believe I commented in each of the tables what that table had replaced
> the deprecated table.  The primary explanation of why the tables have
> been deprecated is in the RFC text - namely that the tables are being
> replaced by more flexible tables that can accomodate more reachability.
> Did you think this needed to be carried over to the Table DESCRIPTIONS?

Yes, that would be ideal.

>
> Updating each deprecated table column's DESCRIPTION with the new columns
> in the new table, especially where there is an obvious match, seemed
> excessive.  Was this what you wanted done?
>

Yes, because there is not a one-to-one correspondence between DEPRECATED 
Tables
and New Tables.

>
>> Specific Comments
>> -----------------
>>
>> Section 4. Overview
>>
>>   "Multi-protocol extensions [RFC4760] were introduced along which
>> allowed
>>   advertisement of reachability such as IPv6 [RFC2545], MPLS Labeled
>>   routes [RFC3107], etc."
>>
>> *) Awkward (suggest rewording)
>>
>> s/introduced along which allowed/introduced, along with/
>
> s/along//
>
>> Section 5.2 Tables
>>
>>
>> *) What does Af mean? (Address Family ?)
>>
>> *) What does AFI adn SAFI stand for?  Okay, I see
>> Address Family Identifier and Subsequent Address Family Identifier.
>> Please spell out entire meaning prior to abbreviation.
>
> Added clarifying text at the place of first use:
>
> bgpPeerAfTable - The BGP peer table.  This table is capable of
> representing IPv6 and other address-family (Af) independent sessions.
> This table replaces the bgpPeerTable from previous versions of this MIB.
>
> bgpPrefixCountersTable - A table of per-peer per Address Family
> Identifer-Subsequent Address Family Identifier (AFI-SAFI) [RFC 4760]
> counters for prefixes.
>
>> Section 5.4
>> -----------
>>
>> For the TC could you use:
>>
>> BgpAddressFamilyIdentifierTC
>>
>> BgpSubsequentAddressFamilyIdentifierTC
>>
>> s/BgoPathAttrFlagsTC/BgpPathAttrFlagsTC
>
> Done.
>
>> 5.6 Extensions
>> -----------------
>> How do you plan to enforce using bgpExtensions for
>> other MIB Modules?
>
> Please see draft-jhaas-idr-bgp4-mibv2-community-00 as an example.
>

Okay, thanks that gives me more information.  I think you should formalize
this via a Standards Action as specified in RFC2434.  Please see the
IANA Considerations sections in the MPLS WG (RFC3811-RFC3815) and
CCAMP WG (RFC4801-RFC4803) for some examples.


>> WRT bgpAfPathAttrIndex, in the event of a reboot of the router,
>> or a restart of the BGP subsystem, is all the information in the tables
>> that reference or use this object/index lost?  In other words,
>> this object becomes an index for other tables.  In the event of
>> a restart of BGP or restart of the router, or restart of the
>> SNMP Agent, etc. this object gets re-initialized, but what happens
>> to the indexes that use this object?
>
> The integrity of the internal structures for BGP peering sessions in an
> implmentation is a somewhat out of scope here.  Most implementations
> would not use the same index from instantiation to instantiation but
> some implementations will.  Much like other forms of SNMP
> discontinuities (i.e. counters) you're correct in that there may be some
> concern as to whether one of these abstract indices means the same thing
> from query to query.
>
> This applies also to bgpAsPathIndex and bgpAfPathAttrUnknownIndex.
>
> I believe standard practice would be to include an object that marks
> discontinuities.  Do you have a reference to a suggested discontinuity
> object that would address this problem?
>

A Discontinuity object would help with respect to counters, but I don't
understand how a discontinuity object would help with this situation.
I see that draft-jhaas-idr-bgp4-mibv2-community-00.txt  uses 
bgpAfPathAttrIndex
also.

Basically, this object can be re-initialized, and this object is also an 
index for other
Tables in this MIB Module and at least one other MIB Module.  If a 
re-initialization
takes place, I assume the value of the object is re-initialized, but what 
happens to the
Index values that were based on this object's initial value?




>> Section 6.1
>> ---------------
>> *) Awkward:
>>   "Note that conducting BGP peering sessions over transport protocols
>>   other that TCP over IP are out of scope of the current BGP
>>   specifications."
>>
>> s/that/than/
>
> Done.
>
>> Specific Comments on the MIB Module
>> -----------------------------------
>>
>> BgpIdentifierTC
>>
>> *) DESCRIPTION is somewhat awkward.
>
> Very.  How about:
> "The representation of a BGP Identifier.  BGP Identifiers
> are presented in the received network byte order.
>
> The BGP Identifier is displayed as if it is an IP address,
> even if it would be an illegal one."
>
> The intention is to say that the octet string contains the on-the-wire
> representation, much like an IP address.
>

Should the DISPLAY-HINT be "1d.1d.1d.1d"?


>> *) Could InetAddress be used instead of creating a new
>> TC?  In other words, you seem to be saying this is an
>> IPv4 Address, but I am not sure what is specific to
>> BGP wrt to this.
>
> IpAddress would be appropriate.  Additionally, the semantics of this
> protocol field have trended somewhat away from being required to be an
> IP address to being "just a 4 octet number".
>
> I chose a new TC for that reason.
>

Part of my confusion is how this value is to be displayed.  The description 
makes
it sound like  "1d.1d.1d.1d" but the DISPLAY-HINT is not that.

>> BgpAfiTC
>>
>> *) rename BgpAddressFamilyIdentifierTC
>> *) Please spell out what AFI stand for (both first time in
>> the document text and first time in the MIB Module.
>>
>>
>> BgpSafiTC
>>
>> *) rename BgpSubsequentAddressFamilyIdentifierTC
>> *) Please spell out what SAFI stand for (both first time in
>> the document text and first time in the MIB Module.
>
> Addressed above.
>
>> BgpPathAttrFlagsTC
>>
>> *) Please rename to BgpPathAttributeFlagsTC
>
> Done.
>
>>    -- bgp 2 and 3 have been deprecated and are documented
>>    -- elsewhere in this MIB
>>
>> *) Are you referring to the protocol versions 2 and 3, or do you
>> mean the MIB objects that are specific to these versions have
>> been deprecated?
>
> The objects.  I've changed this to:
>
> -- { bgp 2 } and { bgp 3 } have been deprecated and are documented
>
>> bgpIdentifier
>> SYNTAX  IpAddress
>>
>> *) According to RFC4181 IpAddress should not be used. Can
>> InetAddress/InetAddressType or even InetAddressIPv4 be used?
>> (Or is the BgpIdentifierTC is more appropriate?)
>
> RFC 4273 object and thus cannot be changed.
>

Since it is a scalar, okay....

>> INDEX Clause for bgpPeerAfEntry and the order
>> of the objects in the entry
>>
>> *) I don't understand the ordering here.
>> The Indexes need to uniquely identify an entry
>> in the table and they should be at the beginning of
>> the table.
>
> The original intention was to cluster the transport session information
> together.  The table has also undergone churn during design.  Does the
> following address your concerns?
>
>    BgpPeerAfEntry ::= SEQUENCE {
>        -- INDEX information
>        bgpPeerAfInstance
>            Unsigned32,
>        bgpPeerAfLocalAddrType
>            InetAddressType,
>        bgpPeerAfLocalAddr
>            InetAddress,
>        bgpPeerAfRemoteAddrType
>            InetAddressType,
>        bgpPeerAfRemoteAddr
>            InetAddress,
>
>        -- Local
>        bgpPeerAfLocalPort
>            InetPortNumber,
>        bgpPeerAfLocalAs
>            InetAutonomousSystemNumber,
>
>        -- Remote
>        bgpPeerAfRemotePort
>            InetPortNumber,
>        bgpPeerAfRemoteAs
>            InetAutonomousSystemNumber,
>        bgpPeerAfIdentifier
>            BgpIdentifierTC,
>
>        -- Session status
>        bgpPeerAfAdminStatus
>            INTEGER,
>        bgpPeerAfPeerState
>            INTEGER,
>        bgpPeerAfConfiguredVersion
>            Unsigned32,
>        bgpPeerAfNegotiatedVersion
>            Unsigned32
>    }
>

Would need to see what the INDEX Clause looks like before
I can answer ;)

I think we should revisit the issues below once we converge on the
above.   I do agree that  a Counter Discontinuity object is likely going
to be helpful, but would like to think about this more.

Also, the issue of "no configuration" object concerns me, so would like
to read over the email archives first and then follow-up on that.

Thanks,
   Joan


>> *) Additionally, are all the indexes necessary.
>
> I'm afraid so.
>
> RFC 1771 language originally suggested that there was a single BGP
> peering session with a remote router.  Multiple peering sessions were
> permitted, but only if the all of the endpoints for a peering session
> were different.  RFC 4271 language was altered so that multiple peering
> sessions to one of the endpoints was permitted.
>
> The instance object was requested by other working groups to cover the
> cases where multiple bgp instances were running on the same platform
> under the same administrative engine.  This provided more flexibility
> than the ENTITY-MIB would readily permit when centralized access to
> peering information was desired.
>
>> *) Looking this over, I'd like to make a suggestion
>> that this table be made into two  (or more?) tables.
>>
>> This one table represents the "bgp instances" on this
>> router AND the remote peers AND peering sessions.
>
> See above for the comment about the set of information being common
> under existing CLIs.
>
> More importantly for a MIB, the INDEX represents the fact that a single
> BGP peering session is uniquely identified by that 5 tuple.
>
> I don't believe we'll get much in the way of efficiencies by doing this
> split since the index components aren't separable.
>
>> My suggestion would be
>> to have a table for just the "bgp instances" to include
>> ConfiguredVersion, AdminStatus, PeerState, Local Info, etc.
>> and the other table for the Remote Peer Information (and
>> perhaps, another for peering session information?)
>>
>> Maybe another way to think of this is when information
>> would be available.  "bgp Instances" should have information
>> prior to Peer Information, right?
>>
>>
>> bgpPeerAfInstance
>>
>> *) Needs to have a range beginning with 1.
>
> I have tightened the range to be 1..4294967295.
>
>> The DESCRIPTION is a little confusing, but I think you
>> are saying that if there is only one BGP routing process
>> then it is recommended that this index have the value
>> of 1.
>
> Yes.  What language would you suggest instead?
>
>> bgpPeerAfPeerState
>>
>> *) the name of this object is misleading.  This is
>> the state of the connection so please rename.
>
> The name is consistent with the previous bgpPeerState object.  What
> would you suggest instead?
>
>> bgpPeerAfAdminStatus
>> *) why is this read-only and not read-write?
>
> While this is probably a reasonable exception, working group consensus
> was to not add new writeable objects.
>
>> bgpPeerAfConfiguredVersion
>> *) why is this read-only and not read-write?
>
> This represents the value configured for the peering session.  This MIB
> does not permit the configuration of BGP peering sessions.  See comments
> above.
>
>> bgpPeerAfLastErrorReceived
>> bpgPeerAfLastErrorSent
>> *) Why did you choose to represent this as a 2 byte
>> OCTET STRING as opposed to ErrorCode and ErrorSubCode (2 objects)?
>
> I've split this into two objects.
>
>> *) General Comment for this table, may want to group all the
>> received objects together followed by the sent objects.
>
> Reordered to group received and sent together as suggested.
>
>> bpgPeerAfLastErrorReceivedText
>> bpgPeerAfLastErrorSentText
>> *) Could a REFERENCE clause be added for these 2 objects?
>
> "This object contains an implementation specific explanation of the
> error that is being reported."
>
> The appropriate reference would be vendor-specific.
>
>> bpgPeerAfFsmEstablishedTime
>>
>> *) Do peering sessions always reach the Established State?
>> If not, what is the value of this object (zero?).  That should
>> probably be added to the DESCRIPTION
>
> How about the following text added to the DESCRIPTION:
> If the peer has never reached the established state, the value remains
> zero.
>
>> bpgPeerAfConfiguredTimersTable
>>
>> *) General comment (getting back to the issue of
>> separating the tables into "bgp instance" vs. "remote peer info"
>> vs "peering session info", this table would seem to largely
>> be part of the bpg instance information.  I don't understand
>> why these objects are read-only?  Is configuration not allowed
>> via SNMP for these objects?
>
> No configuration is permitted in this MIB per working group consensus.
>
>> *) Also the name of this table, might be better to take out
>> the "Peer" so bgpConfiguredTimersTable?
>
> The intention had been to convey that these timers were per-peer.  If
> you strongly object I can change this.
>
>> bgpPeerAfNegotiatedTimersTable
>>
>> *) Could we consider renaming some of these tables?
>> This is bgpPeeringSession info, correct?
>
> Yes.  Let's revisit this point once we've converged on the fate of the
> peer table.
>
>> *) Timers in this and the previous table were Integer32,and
>> should be Unsigned32.
>> see rfc4181 section 4.6.1.1
>
> I believe the only Integer32 counters were ones that could not be
> changed from RFC 4273.
>
>> bgpPeerAfCountersTable
>>
>> *) Is there any Discontinuity Time Object(s) associated with
>> these counters?
>
> No.  See above for my question about how we should handle
> discontinuities.
>
>> DESCRIPTION:
>> "...This object should be initialized to zero when the connection
>> is established"
>>
>> *) Counters by definition cannot be reset.  Please see rfc
>> 2578.txt.  You could create a TimeStamp object which is set to
>> the time when the FSM transitioned to the established state.
>
> I've removed that text.
>
>> bpgPeerAfFsmEstablishedTrans
>> *) Please rename to bgpPeerAfFsmEstablishedTransitions
>
> Done.
>
>> bpgPrefixCountersTable
>>
>> *) The name says counters but the table has
>> Gauge32 objects.
>
> Counters was meant in the general English sense rather than the SNMP
> sense.  Do you have a better suggestion?
>
>> bgpPrefixInPrefixes
>> bgpPrefixInPrefixesAccepted
>> bgpPrefixOutPrefixes
>>
>> *) Could references be added to these objects
>
> Done.
>
>> *) Counters need to be pluralized.
>
> ?
>
>> *) Discontinuity for these?
>
> The discontinuity would be the same as the bgp peering session.  See
> above for questions about a general strategy for this.
>
>> *) This is "new" to the MIB and doesn't appear in rfc4273.
>> Is it needed for BGP4's basic deployment?  Was there any
>> consideration to putting this into a separate MIB Module?
>
> The prefix counters were a key request for the new MIB.  I'm not sure
> splitting this into a separate MIB makes sense.
>
>>    bgpRib
>>        OBJECT IDENTIFIER ::= { bgp 11 }
>>
>> *) Please rename to bpgRibObjects
>
> While the names don't necessarily make good SNMP MIB design sense, in my
> opinion they make proper protocol sense.  The BGP routing information
> base consists of network layer reachability which includes prefix and
> path attribute tuples stored in several logical views: The adjacent
> ribs-in, local rib and adjacent ribs-out.
>
>> bpgNlriTable
>>
>> *) Could REFERENCES be given for this table?
>
> Done.
>
>> *) I didn't understand the indexing and the indexes
>> do not match the entry of this table.  Very difficult
>> to review this one and the bgpAdjRibsOutTable.  Please
>> fix the indexes to match the table entry.
>
> In this particular case, I think things are probably correctly grouped.
> Since this is the basis case for the messy BGP AFI-SAFI version of a
> prefix versus the InetAddressType/InetAddress version of a prefix I'll
> copy in the relevant entries:
>
>    bgpNlriEntry OBJECT-TYPE
>        SYNTAX BgpNlriEntry
>        MAX-ACCESS not-accessible
>        STATUS current
>        DESCRIPTION
>            "Information about a path to a network."
>        INDEX {
>            bgpNlriAfi,
>            bgpNlriSafi,
>            bgpNlriPrefix,
>            bgpNlriPrefixLen,
>            bgpNlriIndex,
>            bgpPeerAfInstance,
>            bgpPeerAfLocalAddrType,
>            bgpPeerAfLocalAddr,
>            bgpPeerAfRemoteAddrType,
>            bgpPeerAfRemoteAddr
>        }
>        ::= { bgpNlriTable 1 }
>
>    BgpNlriEntry ::= SEQUENCE {
>        bgpNlriIndex
>            Unsigned32,
>        bgpNlriAfi
>            BgpAddressFamilyIdentifierTC,
>        bgpNlriSafi
>            BgpSubsequentAddressFamilyIdentifierTC,
>        bgpNlriPrefixType
>            InetAddressType,
>        bgpNlriPrefix
>            InetAddress,
>        bgpNlriPrefixLen
>            InetAddressPrefixLength,
>        bgpNlriBest
>            TruthValue,
>        bgpNlriCalcLocalPref
>            Unsigned32,
>        bgpAfPathAttrIndex
>            Unsigned32,
>        bgpAfPathAttrUnknownIndex
>            Unsigned32
>    }
>
> Within BGP, a prefix is defined by the tuple <AFI, SAFI, Prefix Length,
> Prefix>.  The AFI defines the general IANA address type of the prefix.
> The SAFI, however, may imply additional semantics.  Consider the case of
> an RFC 3107 MPLS labeled prefix.  The prefix encoding is thus:
>
> <1,4,32,777,10.0.0.0>
>
> This corresponds to a labeled route with label 777 for prefix 10/8.  The
> label counts as 24 bits of length.
>
> For MIB access, the InetAddressType doesn't do us any good but is required
> by InetAddress.  AFI, as carried in BGP, may be a subset of the entries
> allowed by InetAddressType and thus I kept them different.
>
> RFC 3107 allows multiple instances of the same prefix to be advertised
> so long as they have different label stacks.  The bgpNlriIndex permits
> these multiple instances.  An extension MIB would give access to the
> label stack by AUGMENTing this table.
>
> Even for the non RFC 3107 case, IDR has been churning over the
> possibility of adding extensions that permit multiple routes for the
> past several years.  I don't relish a full set of table revisions just
> to accomodate such a change - this is meant to future proof the table
> somewhat.
>
> Does this help?
>
>> bgpAfPathAttrCount
>>
>> *) Why is this object needed?
>
> It is a common operational statistic polled from routers, especially for
> memory usage.  Since it is related to path attributes it went into the
> RIB related MIB.
>
>> *) Please keep in mind that Counter type objects need to be
>> pluralized.
>
> I renamed this bgpAfPathAttrCounter.
>
>> bgpAfPathAttrTable
>> *) Noncontiguous numbering between bgpAfPathAttrTable (4) and
>> bgpAsPathTable (6) (Number 5 is missing)
>> Should this be renumbered?
>
> Renumbered.  5 was deleted at some point during design.
>
>> bgpAfPathAttrNextHop
>>
>> *) Please rename bgpAfPathAttrNextHopAddr (to go along with
>> bgpAfPathAttrNextHopAddrType)
>
> Done.
>
>>
>> bgpAfPathAttrLinkLocalNextHop
>> *) Please add an AddrType object
>> prior to this and use the Conformance Statement to specify what is
>> supported.
>
> Done.  For the record, I asked 2 other people about this and you're the
> tie-breaking voice. :-)
>
>> bgpAfPathAttrMedPresent
>>
>> *) Please add to the DESCRIPTION an explanation of what true(1) means?
>>
>
> Done.
>
>> bgpAfPathAttrLocalPref
>>
>> *) What does the DESCRIPTION mean when it says: "this object will be
>> absent"?
>
> No instance of this object will be present in the MIB for that index.
>
>> bgpAfPathAttrAggregatorAS and bgpAfPathAttrAggregatorAddr
>>
>> *) What does the DESCRIPTION mean when it says:
>> "this object will not be present in the conceptual row"?
>
> Same as bgpAfPathAttrLocalPref.
>
>> bgpAsPathIndex
>>
>> *) In the event of a reboot of the router, or a restart of the
>> BGP subsystem, is all the information in the tables
>> that reference or use this object/index lost?  In other words,
>> this object becomes an index for other tables.  In the event of
>> a restart of BGP or restart of the router, or restart of the
>> SNMP Agent, etc. this object gets re-initialized, but what happens
>> to the indexes that use this object?
>>
>> bgpAfPathAttrUnknownIndex
>> *) same question as above.
>
> See above for questions about handling the discontinuity.
>
>> bgpAfEstablishedNotification
>>
>> *) Would the FSM ever be in a state that would fluxuate in and out
>> of the established state?  In other words, would this notification
>> ever get flooded to the network?
>
> Yes.  This is a problem with the object present in RFC 4273, although it
> is worse for the backwards transition notification.
>
>> bgpAfBackwardTransNotification
>>
>> *) Please spell out what "Trans" stands for?
>
> Done.
>
>> Conformance Section
>> *) If read-write objects are added to the MIB, then a read-only
>> conformance
>> may be wanted.
>
> No read-write objects are present that were not part of RFC 4273.
>
> -- Jeff

_______________________________________________
Idr mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/idr