The MPLS WG Archive

[jcucchiara@mindspring.com]

Bert,

Thanks for this review!

At 01:53 AM 5/9/03 +0200, Wijnen, Bert (Bert) wrote:
>First a few serious things that need fixing (or a acceptable
>answer):
>
>1. All the documents that contain a MIB module from which
>   you IMPORT anything must be listed as normative references.
>   I think a few are now under informative and a few may even
>   be missing.

okay.

>
>2. I see at several place in DESCRIPTION clause of StorageType
>   objects this text:
>      DESCRIPTION
>       "The storage type for this conceptual row.  Conceptual rows
>        having the value 'permanent(4)' MAY allow write-access to any
>        columnar objects in the row, except for setting the
>        mplsLdpEntityRowStatus to 'destroy(6)'."
>   And that is very weird. It seems to allow to change the storagetype
>   object itself (which is a nono). And the cannot destroy is also
>   already covered by being a permanent entry.
>   The required text from RFC2579 (Storage Type TC) is something aka:
>       "The storage type for this conceptual row.  Conceptual rows
>        having the value 'permanent(4)' need not allow write-access
>        to any columnar objects in the row."
>   Which you have in some other cases, and which I think expresses
>   the same and is correct accoring to RFC2579 requirements.
>   mplsLdpEntityAtmLRStorageType has it specified correctly!
>

Sorry, but am confused by this, if the type is permanent(4), then
the agent does not have to support any subsequent sets (modifications)
to objects in this row, which includes setting the RowStatus object
to destroy, is this correct?

So then once a row is permanent(4), then the agent does not need to provide
a way to remove/change this row using SNMP, is this correct?

We can rephrase this using the text suggested above, but just want to
make sure we understand ;)

>3 I see:
>         mplsFecAddrPrefixLength    InetAddressPrefixLength,
>         mplsFecAddrFamily          InetAddressType,
>         mplsFecAddr                InetAddress,
>  I believe that RFC3291 suggests to put InetAddressType BEFORE
>  any objects that are to be intepreted within the context of
>  the specific addressType. so the sequence should probaly be:
>         mplsFecAddrFamily          InetAddressType,
>         mplsFecAddrPrefixLength    InetAddressPrefixLength,
>         mplsFecAddr                InetAddress,
>
>  I would also check the DESCRIPTION clauses with RFC3291.
>  I am not so sure it is all in sync with prescriptions of RFC3291
>  I am specifically worried about the reference to RFC1700 (which is
>  obsolete anyway, probably you better refer to
>      http://www.iana.org/assignments/address-family-numbers
>  if that is what you want). But is it not just IPv4 and IPv6
>  addresses that you support (at least that is what compliance
>  seems to state?


The reference to ADDRESS-FAMILY is because the LDP Spec (3036) uses
in a few TLV and they do specify [RFC1700] as the reference.  Here
is the Specific example from the LDP Spec (rfc3036):


"Prefix FEC Element value encoding:

       0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |  Prefix (2)   |     Address Family            |     PreLen    |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                     Prefix                                    |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

      Address Family
         Two octet quantity containing a value from ADDRESS FAMILY
         NUMBERS in [RFC1700] that encodes the address family for the
         address prefix in the Prefix field."



We do not mean to cause confusion in the MIB, BUT,  the LDP Spec
which is "Standards Track" document refers to the obsoleted document (RFC1700)
and the MIB is actually refering to the LDP Spec.

In the MIB we are trying to use descriptions based on the LDP Spec so that
folks can use this MIB to configure values for the LDP TLVs.  If we change
this description too much, then the reference back to the LDP Spec is lost.

Originally we did use the ADDRESS-FAMILY-NUMBERS TC but changed this
for the INET-ADDRESS-MIB which is better for MIB purposes.

Please let us know if we can keep the DESCRIPTION as it is or change
it?  


>  In the DESCRIPTION of PrefixLength I would change 
>            "If the value of the 'mplsFecType' is 'hostAddress(2)'
>            then this object is undefined.
>  Into either:
>            "If the value of the 'mplsFecType' is 'hostAddress(2)'
>            then the value of object is irrelevant and ignored.
>  Or (maybe better, not sure):
>            "If the value of the 'mplsFecType' is 'hostAddress(2)'
>            then the value should be equal to the length of the
>            address in mplsFecAddr.
>  Also in that same DESCRIPTION clause:
>             prefix matches all addresses.  In this case the
>             prefix MUST  also be zero (i.e. 'mplsFecAddr' will
>             have the value of zero.)"
>  What is a "value of zero"? I think you mean zero length.
>  But in order for that to be valid for mplsFecAddr, the
>  mplsFecAddrFamily value should be zero (i.e. unknown).
>
>4. How many notifications can be generated per second?

Are you asking for a worst case scenario?  

In a network where LDP is configured on each device (and not being
reconfigured)
would not expect notifications to be generated once sessions
are established.  So would not quantify X notifications per second.

Maybe another way to ask this question is: how long does the
average LDP Session last?  

Would say most last longer than a second, but
would be good to get operator experience here.

>   I worry based on the description clauses. I see a risk that it
>   could be many if not implemented properly or with a proper
>   throttle? Can you add some text about that?

There are not huge amounts of LDP sessions on a device and
these (one established) are fairly stable unless an operator reconfigures
or something goes wrong with the network, but this would fall into
a worst case scenario.

Certainly, would be good to get some operator experience here, but 
in my experience, sessions (once established) are fairly stable as
long as the network's physical connections are not being changed and 
LDP is not being reconfigured.

>
>5. I worry about a statement in a RowStatus DESCRIPTION clause
>   (it is doen several times):
>            NOTE:  This RowStatus object should
>            have the same value of the 'mplsLdpEntityRowStatus'
>            related to this entry."
>   I will agree that that is the ideal situation. But the rowStatus
>   should always represent the real status of the row itself.
>   If that is not consistent with some other row in another table,
>   then that is bad, but it should not be a cause for the 
>   rowStatus of thos column to take an invalid value. I guess
>   you did not mean to say it should... but that is how people
>   might read it. 

Okay, I understand I think.  We have had many issues with folks not
understanding the relationship of these tables and essentially, any
table that has a configurable row is related to a row in the 
mplsLdpEntityTable.  This is outside of the scope of RowStatus though,
so will reword this to remove the "should".

>
>
>6. MPLS-LDP-GENERIC-MIB
>   W: f(mplsldpgen.mi2), (58,17) The first revision should
>      match the last update for MODULE-IDENTITY mplsLdpGenericMIB
>   I think you just mistyped 2002 instead of 2003.

yes. typo.

>
>Nits (some more serious than otehrs)...
>would be nice if they can be fixed:
>
>- I would not use capitals for FULL, READ-ONLY, NEW and such.
>  There is the risk that someone wants to know if they have
>  special meaning.
>- sect 3.5 under NOTE: This table
>  My question: which table?
>- You have in many places a reference aka:
>     REFERENCE
>       "[RFC3036] LDP Specification, Section on LDP Identifiers."
>  Now, once people extract the MIB module from the document (and many
>  people do) then you loose the citation. So I always recommend:
>     REFERENCE
>       "RFC3036, LDP Specification, Section on LDP Identifiers."
>- I see:
>     mplsLdpPeerTransportAddrType OBJECT-TYPE
>         SYNTAX      InetAddressType
>         MAX-ACCESS  read-only
>         STATUS      current
>         DESCRIPTION
>             "The object specifies how to interpret the address
>             for the mplsLdpPeerTransportAddr object."
>     .. snip 
>
>     mplsLdpPeerTransportAddr OBJECT-TYPE
>         SYNTAX      InetAddress
>         MAX-ACCESS  read-only
>         STATUS      current
>         DESCRIPTION
>             "The transport address advertized by the peer
>             in the hello message or the Hello source address."
>
>   I think that the DESCRIPTION for InetAddress object MUST contain
>   the explanation how it is interpreted. The example in RFC3291 is
>   so explicit and clear (or so I think). I also think that it is
>   in fact an Internet address and not a transport address (the
>   latter needs a port number too, does it not? see RFC3419)
>
>   So something aka the next seems better (maybe even rename
>   TransportAddr to IntenetAddr:
>
>     mplsLdpPeerTransportAddrType OBJECT-TYPE
>         SYNTAX      InetAddressType
>         MAX-ACCESS  read-only
>         STATUS      current
>         DESCRIPTION
>             "The type of Internet address for the
>              mplsLdpPeerTransportAddr object."
>     .. snip 
>
>     mplsLdpPeerTransportAddr OBJECT-TYPE
>         SYNTAX      InetAddress
>         MAX-ACCESS  read-only
>         STATUS      current
>         DESCRIPTION
>           "The Internet address advertized by the peer in the hello
>            message or the Hello source address. 
>
>            The type of this address is determined by the value of
>            the mplsLdpPeerTransportAddressType object."
>
>  If you do change this, pls check all your InetAddress objects

okay, thank you for the text.

>
>- mplsLdpSesKeepaliveTime
>  What does value of zero mean?

The value cannot be zero.  This has to be a nonzero, 2 octet
value, which represents the amount of seconds between keep alive
messages negoatiated between the entity and the peer for that
specific session.

The mplsLdpEntityKeepAliveHoldTimer is the object which 
configures this value.  This Peer also sends a value and
these are negotiated to be the smaller of the 2 values.

This object represents the negotiated value.

Do we put a range on this even though it is read-only and
we already have a range on the configurable counter-part?


The range will be the same range as for the
mplsLdpEntityKeepAliveHoldTimer object which is configurable.

>
>- mplsLdpHelloAdjacencyHoldTimeRem
>  could use a UNITS clause

We debated this.  If there are special values (in this case for
0 and for 0xffff/65535)  then if we have a UNITS "seconds" 
is it clear that these other values are not seconds??


>  I also wonder, since 0xffff has a special meaning,does that mean
>  that that is also the max value? If so, a range of (0..65535) 
>  would make good sense.

The object which is used to configure this value for the entity
is mplsLdpEntityHelloHoldTimer which does have a range.

SO same question as above, do we put a range on this object even if
it is read-only and we already have the range for the configurable
counter-part?



>
>- I see:
>       mplsOutSegmentLdpLspTable OBJECT-TYPE
>         SYNTAX      SEQUENCE OF MplsOutSegmentLdpLspEntry
>         MAX-ACCESS  not-accessible
>         STATUS      current
>         DESCRIPTION
>             "A table of LDP LSP's which
>             map to the InSegment Table in the
>             the LSR MIB's."
>  s/InSegment/OutSegment/ ??
>  s/MIB's/MIB/ ??
>
>- I may have said this before. Could the mplsLdpSesUp and mplsLdpSesDown
>  notification not be combined to mplsLdpSesStateChange?
>  The included mplsLdpSesState object will identify if it is an Up or a
>  Down, will it not?


Yes, this came up.  We wanted to leave this because we do know NMS apps which
do not decode the notifications too much but resolve by saying this
notification
matches this other notification.  So one notification causes an alarm and the
other notification clears it.  Not sure if it matters too much in this
situation
so we can change it as suggested above.


>
>- In the DESCRIPTION clauses of your OBJECT-GROUP definitions, you have
>  langauge that talsk about "optional" and/or "required to implement".
>  That is not the goal of the OBJECT-GROUP macro. It is needed to GROUP
>  objects that logically belong together. So the DESCRIPTION should
>  have text to explain why these objects belong in one group.
>  The "optional" and "required" and susch is language that goes
>  in DESCRIPTION clauses of the MODULE-COMPLIANCE GROUP clauses.

Okay, thanks for this info.

>
>- Sect 4.1 talks about mplsLdpEntityAtmLabelRangeTable while the actual
>  table name is: mplsLdpEntityAtmLRTable
>  Same for FrameRelay and Generic MIB

Sorry, yes, we changed this and missed the text.

>
>- mplsLdpEntityAtmTable
>  You talk about "extends". I think what the table does is that it
>  is a "sparse augments". Your indexes are OK though.


Yes, this is true for the mplsLdpEntityFrameRelayTable also.

>
>- mplsLdpEntityAtmLREntry talks about overlaps and such that may
>  (should, can) not occur. But it does not say anything about what
>  sort of error to return if someone tries to SNMP SET (or create)
>  incorrect values. And It seems like 2 errors are possible:
>  - inconcsistentName (if it is part of the (not-accessible) index)
>  - inconsistentValue (if it is a value in read-create column)
>  Not 100% sure... anyway, better to specify what you want to 
>  happen than let people guess
>

Okay we will add to the table description.


>- mplsLdpEntityAtmLRRowStatus has in DESCRIPTION clause
>            There must exist at least one entry in this
>            table for every LDP Entity that has
>            'mplsLdpEntityOptionalParameters' object with
>            a value of 'atmSessionParameters'.
>  Does not seem right in RowStatus object. Maybe in the Entry
>  or Table object?
>  I have seen this in multiple RowStatus Objects

We will check on this.  Agree it is better in table/entry description.

>
>- mplsLdpAtmSesTable talks about  'mplsLdpSessionTable' 
>  In fact many places in the doc talk about it.
>  Seems you renamed the table to mplsLdpSesTable (not sure why,
>  cause I find the full Session better... but that is another
>  subject).

Okay, we will check this.  Sorry, had a mix and changed to to
be shorter, but sounds like you prefer the longer, i.e. session ;)

>
>- mplsLdpEntityAtmLRRowStatus talks about
>      'mplsLdpEntityOptionalParameters'
>  and so do some other places, but I cannot find the latter object

We changed the name of the object to be mplsLdpEntityLabelType, so 
will update these descriptions.

>  
>- I can live with the security considerations section.
>  You might be able to shorten it somewhat if you take the last 3
>  paragraphs of each 9.x section and state that once in its own
>  9.x section. Up to you.

Okay, we can do that.

Thanks again for the review.

  -Joan


>
>Thanks,
>Bert 
>