Subject: review of draft-ietf-mpls-lsr-mib-09.txt

From: "Wijnen, Bert (Bert)" <[email-masked]>
Date: Fri, 6 Dec 2002 16:04:12 +0100

Oh well... 

Several of my earlier comments have not been addressed at all.
Several others have been addressed (thanks).
I had quickly skimmed through the new rev before the Atlanta
MIB meeting, assuming all my concerns were addressed. 
These earlier comments (1H2002) were sent to authors only... 

Here we go. Mix of nits and serious stuff

- I am missing IPR section

- abstract says:
    community.  In particular, it describes managed objects for
    modeling a Multiprotocol Label Switching (MPLS) Label Switching
    Router  (LSR).
  Not sure it is so much "objects for modeling" as it is "objects to
  configure and/or monitor"

- sect 9 talks about MIB II. I think you better just talk about
  RFC2863 (IF-MIB).

- sect 10 says:
     RowPointer is a textual convention used to identify a conceptual
     row in an SNMP Table by pointing to one of its objects. In this
  REPEAT for the umptieth time:
     it is not just pointing to "one of its objects", but to "the
     first accessible object in a row". Again see RFC2579 for that

- sect 10 last sentence has an empty [] at the end?

- I wonder why mplsInterfaceConfTable and ...ConfEntry and ...ConfIndex
  have the "Conf" in there. 
  NONE of the accessible objects in the table havae that prefxi "Conf"
  in them. Besides, the "Conf" to me suggests that you can use the
  table to configure something, but it is a read-only table, so
  it only lists details about existing mplsInterfaces. 
  mplsInterfaceTable and mplsInterfaceEntry seem better names to me
  You may also want to say something about the table being read-only
  in the atble DESCRIPTION clause

- mplsInterfaceConfEntry
  s/iff/if/
  It talks about what a index of zero means. That is OK... bit really
  that info should be in the DESCRIPTION clause of the index object
  itself. If nothing else, then repeat the meaning of zero in the
  index object.

- mplsInterfaceConfIndex OBJECT-TYPE
   SYNTAX        InterfaceIndexOrZero
   MAX-ACCESS    not-accessible
   STATUS        current
   DESCRIPTION
       "This is a unique index for an entry in the
        MplsInterfaceConfTable.  A non-zero index for an
        entry indicates the ifIndex for the corresponding
        interface entry in of the MPLS-layer in the ifTable.
==> is the above a proper English sentence?

- probably want to add a UNITS clause to mplsInterfaceTotalBandwith
  and ...AvailableBandwidth

- mplsInterfaceLabelParticipationType OBJECT-TYPE
   SYNTAX  BITS {
                  perPlatform (0),
                  perInterface (1)
              }
   MAX-ACCESS    read-only
   STATUS        current
   DESCRIPTION
       "Either the perPlatform(0) or perInterface(1) bit
        MUST be set. Note however that an interface could
        participate in both label spaces in which case both
        bits would be set. If the value of the
        mplsInterfaceConfIndex for this entry is zero, then
        only the perPlatform(0) bit MUST be set and the
        perInterface(1) bit is meaningless. If the
        perInterface(1) bit is set then the value of
        mplsInterfaceLabelMinIn, mplsInterfaceLabelMaxIn,
        mplsInterfaceLabelMinOut, and
        mplsInterfaceLabelMaxOut for this entry reflect the
        label ranges for this interface.  If only the
        perPlatform(0) bit is set, then the value of
        mplsInterfaceLabelMinIn, mplsInterfaceLabelMaxIn,
        mplsInterfaceLabelMinOut, and
        mplsInterfaceLabelMaxOut for this entry must be
        identical to the instance of these objects with
        index 0."

  I think you mean well... but the 1st and 2nd sentence seem 
  to contradict each other.
  Also the last "must" probably is better a "MUST".
  The text is also not easy to read... what do you think about"

   DESCRIPTION
       "Specifies if the interface uses labels from the interface
        label-space and/or the per-platform label-space. 

        If the value of the mplsInterfaceConfIndex for this entry is
        zero, then only the perPlatform(0) bit MUST be set and the
        perInterface(1) bit is meaningless.

        If the perInterface(1) bit is set then the value of
        mplsInterfaceLabelMinIn, mplsInterfaceLabelMaxIn,
        mplsInterfaceLabelMinOut, and mplsInterfaceLabelMaxOut for
        this entry reflect the label ranges for this interface.

        If only the perPlatform(0) bit is set, then the value of
        mplsInterfaceLabelMinIn, mplsInterfaceLabelMaxIn,
        mplsInterfaceLabelMinOut, and mplsInterfaceLabelMaxOut for
        this entry MUST be identical to the instance of these 
        objects with index 0."

  What is still not clear from the above:
  - assume per-platform has label range MinIn/MaxIn  50-100
  - assume interface itself has label range MinIn/MaxIn   60-90
  - assume both bits are set to 1.

  What happens? does it have to allocate/use labels in range 60-90
  first and only when that range is exhausted in ranges 50-59 and
  91-100 ??

-     mplsInterfaceInLabelsUsed          Gauge32,
      mplsInterfaceLabelLookupFailures   Counter32,
      mplsInterfaceOutLabelsUsed         Gauge32,
      mplsInterfaceOutFragments          Counter32

  I (still) believe that ....LabelsUsed is an incorrect name.
  It should be ...LabelsInUse (at this point in time, as the
  DESCRIPTION clause explains).

  I also still think that ..OutFragments is probably better called
  ....OutFragmentedPackets (as per description clause)

  Why do you not let all these names start with mplsInterfacePerfXxxx
  so that it is VERY CLEAR that these objects are located in this
  specific table?

- mplsInSegmentEntry  OBJECT-TYPE
     INDEX { mplsInSegmentIfIndex, mplsInSegmentLabel }

  Question: does there have to be one or more entries for every entry
  in the mplsInterfaceConfTable?
  If so, the first index probably should be mplsInterfaceConfIndex

  In any event, in my previous comment I had mentioned that ...IfIndex
  in the name of the index object is confusing, cause IfIndex is an
  object in the ifTable. I had suggested to use ...Interface
  or ...LabelSpace or such

- mplsInSegmentAddrFamily OBJECT-TYPE
   SYNTAX        AddressFamilyNumbers
  In my previosu comments I had asked what this is used for?
  I either have missed the answer or forgot.

- As per earlier comment:
  mplsTrafficParamEntry
   "resource sharing" ???? see also my earlier comment on this:
  d. In sect 4, you state that the MIB allows resource sharing
   between multiple LSPs. Not sure, but I think you mean that
   various Tables can have entries that are shared by multiple
   entries in Other tables. So it is sharing "memory resources"
   when implementing the MIB I think. Such is not clear.
   It could be interpreted to mean that LSPs share other
   physical resources (which migth be possible too, but I
   am not sure that the MIB has anything to do with that
   or if so, pls explain). 
   There is more of this "resource sharing" talk throughout 
   the MIB. You may want to check that and make sure it is
   clear what you mean.
  
- mplsInSegmentRowStatus OBJECT-TYPE
   SYNTAX        RowStatus
   MAX-ACCESS    read-create
   STATUS        current
   DESCRIPTION
       "This variable is used to create, modify, and/or
        delete a row in this table."

   As per RFC2579, you must specify which columns can be changed
   if row is active. Seems the simple sentence to add is:

   "When this row is in active(a) state, no objects in this row can
    be changed."

   This is true for all your RowStatus objects in this MIB module

- mplsInSegmentStorageType OBJECT-TYPE
   SYNTAX        StorageType
   MAX-ACCESS    read-create
   STATUS        current
   DESCRIPTION
       "This variable indicates the storage type for this
        object. This object cannot be modified if
        mplsInSegmentRowStatus is active(1). If this
        variable is set to readOnly(5), and the
        corresponding LFIB entry is removed, then the agent
        must remove this row shortly thereafter [RFC2579]."

  As explained earlier, you MUST add text as to what objects (if any)
  MUST be writable for a 'permanent' row. See RFC2579. This is true
  for all you StorageType objetcs in this MIB module.

  Also... it might be good/wise to add a DEFAULT. In many MIB modules
  we have used volatile as the DEFATULT value.

- MplsInSegmentPerfEntry ::= SEQUENCE {
    mplsInSegmentOctets                 Counter32,
    mplsInSegmentPackets                Counter32,
    mplsInSegmentErrors                 Counter32,
    mplsInSegmentDiscards               Counter32,

    -- high capacity counter
    mplsInSegmentHCOctets               Counter64,

    mplsInSegmentPerfDiscontinuityTime  TimeStamp

   Why do you not add "Perf" in the name of all these objects
   so that the names start with mplsInSegmentPerfXxxx and so that 
   it is VERY clear in which table they are located?

   Since it is a short/small table, I guess it is easy to see from the
   DiscontinuityTime object that that is the discontinuity timer.
   RFC2578 says that each Counterxx object must include the text that
   explains whihc timer is to be used for discontinuity.
   So consider this a details-nit

- mplsXxxxIndexNext
  We have talked about this a number of times. It still bothers me that
  you state that on every retrieval operation the agent MUST change the
  value. This sort of ENSURES that index values are non-contiguous.
  I guess that what you describe works.. but far from ideal.

  I really believe that you guys should look at the IndexInteger and
  IndexIntegerNextFree TCs in RFC3289, page 38. I think doing it that way
  is MUCH MUCH (yeas really MUCH) better.
  In fact, you could just use those TCs... although it makes you dependent
  on that MIB module. It is already at PS, so... 

- mplsOutSegmentIndex OBJECT-TYPE
   SYNTAX        Unsigned32(0..2147483647)
   MAX-ACCESS    not-accessible
   STATUS        current
   DESCRIPTION
       "This value contains a unique index for this row.
        While a value of 0 is not valid as an index for this
        row it can be supplied as a valid value to index
        mplsXCTable to access entries for which no out-
        segment has been configured."
   ::= { mplsOutSegmentEntry 1 }

  I think that this states that this object SHOULD have a range of
  (1..2147483647) and so EXCLUDE value zero. And you should NOT
  be using this object as index in the XCtable. The XCtable does
  NOT augnment or extend this table! In fact, your current XCtable
  has defined new objects for indexing, so you can safely start
  the range at 1.  I think we discussed this several times before.

- mplsOutSegmentNextHopIpAddrType OBJECT-TYPE
   SYNTAX        InetAddressType
   MAX-ACCESS    read-create
   STATUS        current
   DESCRIPTION
       "Indicates that the next hop address type is IPv4
        when set to ipv4 (1) or IPv6 when set to ipv6 (2).
        A value of unknown (0) is allowed only when the
        outgoing interface is of type point-to-point. If any
        other values are attempted in a set operation the
        agent MUST return an inconsistentValue error. This
        object cannot be modified if mplsOutSegmentRowStatus
        is active(1)."

  I think a MUCH better way to do this is:
   DESCRIPTION
       "Indicates that the next hop Internet address type. Only
        values unknown (0), ipv4 (1) and ipv6 (2) have to be
        supported.

        A value of unknown (0) is allowed only when the
        outgoing interface is of type point-to-point. If any
        other values are attempted in a set operation the
        agent MUST return an inconsistentValue error.

        This object cannot be modified if mplsOutSegmentRowStatus
        is active(1)."

- mplsOutSegmentNextHopIpAddr OBJECT-TYPE
   SYNTAX        InetAddress
   MAX-ACCESS    read-create
   STATUS        current
   DESCRIPTION
       "The IPv4 or IPv6 address of the next hop.  Its value
        is significant only when
        mplsOutSegmentNextHopIpAddrType is ipv4 (1) or ipv6
        (2). Otherwise a management entity SHOULD set this
        to 0 and an agent MUST ignore it. Likewise an agent
        MUST return a value of 0 for a retrieval operation.
        This object cannot be modified if
        mplsOutSegmentRowStatus is active(1)."

  You cannot tell an agent what to return for retrieval... 
  It MUST always return whatever the value happens to be!!
  I think a MUCH better way to do this is (and in sync with RFC3291):
   DESCRIPTION
       "The Internet address of the next hop. The type of
        this address is determined by the value of the
        mplsOutSegmentNextHopIpAddrType object. 

        This object cannot be modified if mplsOutSegmentRowStatus
        is active(1)."

- mplsOutSegmentXCIndex
  s/is MUST/MUST/

- mplsOutSegmentRowStatus and ... Storagetype. 
  See my comments about RowStatus and Storagetype above

- mplsXCEntry
  talks about mplsXCOutSegmentIndex being able to have avlue of zero
  but mplsXCOutSegmentIndex object itself has a range that excludes zero
  So which one of the two is incorrect?

- as discussed before:
  mplsXCIsPersistent and mplsXCStorageType seem to be conflicting and/or
  overlapping objects. What is the story here?

- mplsXCRowStatus and STorageType
  see my earlier comments on these two object types above

- mplsXCAdminStatus
  does it not make sense to add a DEFAVL ??

- mplsLabelStackRowStatus and StorageType
  see my earlier comments on these two object types above

- mplsTrafficParamRowStatus and StorageType
  see my earlier comments on these two object types above

- mplsNotificationsEnable.
  Add at least a ptr to RFC2573 that explains that notifications
  are under control of the MIB modules in that document
  In other words, if you enable in the MPLS MIB module, these
  notifications may still not be emitted based on settings in the
  MIB modules in RFC2573.

- mplsXCdown
  s/be the identical/be identical/

- mplsXCUp and mplsXCDown
  So will the OperStatus be the value 'up' and 'down' respectively?
  Or will it be the value that it had before going into the Up or Down
  state. The latter might pass more info. But in any event, it is
  best to be explicit as to what an agent is supposed to send and
  what a manager can expect to receive.

- The "too quickly" in you xxxHCxxx DESCRIPTION clauses are not very
  specific. I would rather be explicit what that means. Aka as is done
  in IF-MIB.

- But in general, one might wonder why you do not just do 64bit counters
  and require them on all devices. these LSR boxes will certainly have
  the power to do so.

- I wonder if it is not better to have a mplsLsrReadOnlyCompliance and
  a mplsLsrFullCompliance as is also done in RFC3289 (as one of
  the better examples). Specifically, something like:

      OBJECT      mplsOutSegmentRowStatus
      SYNTAX      INTEGER {
         active(1),
         notInService(2),
         createAndGo(4),
         destroy(6)
      }
      MIN-ACCESS  read-only
      DESCRIPTION
          "The notReady(3) and createAndWait(5) states need
           not be supported."

  is confusing. You do NOT use INTEGER by the way, but RowStatus.
  (SMICng will complain, but that is OK).

  If you split in two COMPLIANCEs. then for read-only you would have

      OBJECT      mplsOutSegmentRowStatus
      SYNTAX      RowStatus { active(1) }
      MIN-ACCESS  read-only
      DESCRIPTION "write access need not be supported."
  
  for Full compliance you would have

      OBJECT      mplsOutSegmentRowStatus
      SYNTAX      RowStatus { active(1) }
      WRITE-SYNTAX RowStatus { createAndGo(4), destroy(6) }
      DESCRIPTION
         "Support for createAndWait and notInService is not required."

- I do not understand why you need two groups:
  mplsXcIsPersistentGroup and mplsXCIsNotPersistentGroup that both have
  only one and the same object in it?

- on page 43 I see:

      -- mplsInSegmentTable

      OBJECT     mplsInSegmentXCIndex
      DESCRIPTION
          "Write access is not required."

  Well, I would then expect:  

      OBJECT     mplsInSegmentXCIndex
      MIN-ACCESS read-only
      DESCRIPTION
          "Write access is not required."

  But the object already is MAX-ACCESS read-only.
  So no need to list this object here

- I find
      OBJECT      mplsInSegmentStorageType
      SYNTAX      INTEGER { other(1) }
      DESCRIPTION
          "Only other(1) needs to be supported."
  REALLY REALLY strange.
  Other is defined as none of the values that have a well defined
  meaning. So you have no idea what this means.
  Why is this?

-     OBJECT      mplsOutSegmentNextHopIpAddrType
      SYNTAX      InetAddressType { unknown(0), ipv4(1), ipv6(2) }
      MIN-ACCESS  read-only
      DESCRIPTION
          "An implementation is only required to support
           ipv4(1) and ipv6(2)."

  The DESCRIPTION conflicts with the SYNTAX clause, does it not?
  Same for
  
      OBJECT      mplsOutSegmentNextHopIpAddr

- mplsHCInSegmentPerfGroup OBJECT-GROUP
   OBJECTS { mplsInSegmentHCOctets }
   STATUS  current
   DESCRIPTION
          "Object(s) providing performance information
           specific to out-segments for which the object
           mplsInterfaceInOctets wraps around too quickly."

  I guess: s/out-segments/in-segments/

- Why is the mplsSegemntDiscontinuityGroup a separate group?
  If you have any counters that use this as a discontinutiy timer,
  the they MUST also support this group, so I would say that
  these object should be part of the mplsPerfGroup.
  I guess this was caused by the splitting of HC counters.
  Another argument to just do away with the 32-bit counter for
  octets and just use 64 bit counters. This is 2002 (close to 2003),
  and not 1989 anymore.

- References need to be split in normative and informative
  For you current SNMP references, the split is:
> > Normative        Informative
> > ----------       ------------
> >  1905                1155
> >  1906                1157
> >                      1212
> >                      1215
> >  2571                1901
> >  2572                2570
> >  2573
> >  2574
> >  2575
> >  2578
> >  2579
> >  2580
  
Oh well... a lot of these things should not show up this late in the
process... or so I feel.

Bert

---

From: "Adrian Farrel" <[email-masked]>
Date: Fri, 6 Dec 2002 11:40:17 -0500

Bert,
I haven't been party to the discussions with the MPLS MIB authors on this point,
but I will jump in because the GMPLS MIBs do/might use this construct.

>   I guess that what you describe works.. but far from ideal.

It is certainly not intentional that the index values are non-contiguous. In
fact, it is intended that they ARE contiguous and also that they are uniquely
reserved for users which is important when there are multiple users trying to
create rows.

>   on that MIB module. It is already at PS, so...

This construct appears to be the same except that the one in the LSR MIB
attempts to close the window where two applications are attempting to create
rows at once. In RFC3289, both may read the object of type  IndexIntegerNextFree
and get the same value: the second create is doomed to fail. In the LSR MIB when
the first application reads the xxxxIndexNext the value is incremented so that
the next read (by the second application) will return a different value.

It is true that the LSR MIB approach may lead to lacunas if an application
decides not to create a row after all, but that is not necessarily a problem.

I assume that this issue has been addressed in multiple MIB modules in the past
and guess there isn't a standard way of solving it.
FWIW the LSM MIB approach is used in the AToM MIBs. For example,
atmVpCrossConnectIndexNext in RFC2515.

A common approach across all MIB modules would certainly be welcome.
Is there a MIB Doctor in the house?

Adrian