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

From: "Wijnen, Bert (Bert)" <[email-masked]>
Date: Fri, 6 Dec 2002 18:23:10 +0100

> 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.
> 
> > - 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.
> 
> 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.
> 
> >   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...
> 
> 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.
> 
And do you realize that there are LOTS and LOTS of tools/mib-browsers out there
that just do MIB walks and so everytime they pass your xxxIndexNext object, they
will cause it to bump and for sure they will NOT create a new row.

> 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.
> 
But because of the many many WALK operations (GETNEXT, GETBULK) it may happen
far more often than you expect.,

> 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 xample,
> atmVpCrossConnectIndexNext in RFC2515.
> 
> A common approach across all MIB modules would certainly be welcome.
> Is there a MIB Doctor in the house?
> 

I think that an approach as per RFC3289 is reasonable and better than what you have.
I think an even better approach might be to use a TestAndIncr object (TC from RFC2579).
You would have to 
- retrieve the value
- issue a SET with as first varbind the TestAndIncr object and the following
  varBinds the varbinds to create a row.
- If another manager has done the same, then you fail on the first varBind.
  so it is a nice way to sync managers.
- If you succeed, then the value gets incremented at the same time you
  indeed create a row (assuming that that does succeed.).
- A negative is that once you have wrapped around, incrementing by one
  does not guarantee a free entry. So if you expect a lifetime where 
  a unsigned 32 indeed wraps around, then this is not a good approach.

Bert

---

From: "Thomas D. Nadeau" <[email-masked]>
Date: Fri, 06 Dec 2002 15:18:33 -0500

>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

         Conf stands for "Configured" (i.e.: the configured MPLS interfaces).

>- 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.

         The last comments asked to put them into the table description.
We can put them here too.

>- 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.

         Replace:

         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.

         With:

         Either the perPlatform(0) or perInterface(1) bit
          MUST be set, but both MAY be set to denote an
         interface participating in both label spaces.

>   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."

         Okay.

>   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 ??

         I think that most LSRs out there just allocate from the
set of labels they are assigned; the order is not specified anywhere.

>-     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).

         Okay.

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

         OutFragmentedPkts.

>   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

         If a label exists, a corresponding ifIndex needs to go with it.

>   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

         mpslInSegmentIf ?

>- 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.

         I thought we agreed to remove this entire table during
the last review?

>- 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

         Will make clear for all tables.

>- 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.

         Okay. Must have missed this one.  Will check everywhere
anyway.

>- 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?

         Okay.  This is part of the directive the make object
names consistent that I planned on doing through the MIBs.

>    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

         There is another one of these in the FTN MIBthat was
fixed correctly as per your and Mike's requests. This was missed I think
because it was not flagged directly; will fix as appears in the
FTN MIB currently (which is based on the DiffServ MIB's use).

>- 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.

         Yes, will fix.

>- 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)."

         Cool.

>- 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)."

         Cool.

>- 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.

         XCup is issued on a transition from anything to up,
and XCdown is issued on a transition from anything to
down.  will clarify.

>- 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.

         Okay.

>- 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 think we have done this elsewhere (TE MIB), so I can do this
here as well.

>- 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:

         Sure. We did this for FTN and TE correctly, just need to
do it here too.

>       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).

         Will change to RowStatus.

>   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?

         I think we used to have some stuff in here RE: persistence,
so this is an artifact. Can clean up as suggested.

>- 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.

         Yes.

>   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.

         Will be fixed with just 64 bit counters.

>- 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.

         Certainly, but how often does one have 5 drafts to
edit at the same time that are all inter-related?  Anyway, I will
fix.  You should see some updated drafts on your door
step next week.

         --Tom

---

From: "Wijnen, Bert (Bert)" <[email-masked]>
Date: Fri, 6 Dec 2002 21:58:43 +0100

> FTN MIB currently (which is based on the DiffServ MIB's use).
Aha... so if you want to use a MPLS wide method, why not add the things
you did for FTN in the MPLS TC MIB and then make them

MplsIndex, MplsIndexOrZero, MplsIndexNextFree
And then do as the rfc3289 does. Pls do check my comments I had on the
TCs in FTN, they were close to OK but had some weird stuff in them.

You can then use thse throughout all the MPLS mib modules

Bert

---

From: "Thomas D. Nadeau" <[email-masked]>
Date: Fri, 06 Dec 2002 16:01:41 -0500

>You can then use thse throughout all the MPLS mib modules

         Okay. Will work with Joan on this.

         --Tom