Subject: review of draft-ietf-mpls-ftn-mib-05.txt

From: "Wijnen, Bert (Bert)" <[email-masked]>
Date: Wed, 4 Dec 2002 19:10:14 +0100

Here is my review comments for the FTN mib document

First a complexity problem ----------

I have trouble understanding the complexity of the
indexing in the mplsFTNMapTable. Can someone explain it to me?

The document claims that entries can be inserted (see section 
5.2 and DESCRIPTION clause of mplsFTNMapTable and mplsFTNMapEntry),
but I wonder how you do that?

For example, let us assume you have for ifIndex 1 these entries
with these indices:

   1.0.1
   1.1.2
   1.2.4

And you wanted to insert map entry 3, then how do you do that?

If it turns out that the only way to do it is to first remove the
last one and then add 2 new ones

   1.2.3
   1.3.4

then why is that handy? And does it justify the complexity.
And that is just for inserting one entry... think about 
multiple entries?

Does anyone by the way think that the figure on page 6 (sect 5.4)
is easy to understand (or to understand at all) ??
 
This (complex) indexing  is also the reason why we get an 
"inconsistent index" error from SMICng for the Perf Table.

------------ now the regular review comments/nits ------

- sect 3. 
  references [2] but that one is not listed in references section
  probbaly easiest to reference [RFC2119] anyway and add that one.

- sect 5.4
  The 3 items at the bottom of the figure:
                                                       | | |
                                                       | | |
              (mplsFTNMapIndex = 1,    ----------------| | |
               mplsFTNMapCurrIndex = 2)                  | |
                                                         | |
              (mplsFTNMapIndex = 1,                      | |
               mplsFTNMapCurrIndex = 3) -----------------| |
                                                           |
               mplsFTNPerfEntry:                           |
              (mplsFTNMapIndex = 1,     -------------------|
               mplsFTNMapCurrIndex = 1)

  Are they the 3 entries in the mplsFTNPerfTable? cause that
  is not clear for the first two is it?

- I tried to work through the example in sect 6.
  It seems OK now (at least it is much better than the one
  of half a year ago or so). Can someone else check it too
  please and confirm that it looks correct?

- sect 7
  You have fixed it based on earlier comment of mine.
  The 1st sentence is still incorrect:
    RowPointer is a textual convention used to identify a conceptual
    row in a conceptual Table in a MIB by pointing to one of its objects.
  
  s/one/the first accessible/

- MplsFTNIndexOrZero ::= TEXTUAL-CONVENTION
   STATUS              current
   DESCRIPTION
       "Index for a FTN entry or zero. If
        the number of unassigned  entries  is
        exhausted, this object MUST return a
        value  of  0."
   SYNTAX              Unsigned32 (0..4294967295)
 
  talks about "this object MUST return" !!!
  this is a TC, not an object. There can be an object that has
  this SYNTAX though.
  I think for objects that use this SYNTAX, it means that it does not
  point to an FTNEntry. That thing about "exhausted" is the next TC

- The DESCRIPTION clauses of the MplsFTNIndexIntegerNextFree TC
  and the first usage of it in mplsFTNIndexNext object DESCRIPTION
  clause seem to conflict as far as I understand them.
  In any event, I donlt think you want the value to change after 
  every GET or GETNEXT.

- DESCRIPTION in mplsFTNTable ends with:

        The action pointer points at either an MPLS-LSR MIB
        mplsXCEntry when the NHLFE entry is a non-TE LSP, or
        it points at an mplsTunnelEntry in the MPLS-TE MIB
        if we wish to make the NHLFE the start of a TE tunnel."

  I don't this the "if we wish to make" is good language.
  Maybe better "in which case the NHLFE is the start of a TE
  tunnel."

- mplsFTNRowStatus OBJECT-TYPE
   SYNTAX              RowStatus
   MAX-ACCESS          read-create
   STATUS              current
   DESCRIPTION
       "Used for controlling the creation and deletion of this
        row.  All writable objects in this row may be
        modified at any time. Setting this variable to
        'destroy' when the MIB contains one or more RowPointers
        pointing to it results in destruction being
        delayed until the row is no longer used."
   ::= { mplsFTNEntry 2 }

  I think I already mentioned at the MPLS MIB meeting in Atalanta that
  "destruction being delayed until the row is no longer used." is
  not acceptable.

- mplsFTNAddrType OBJECT-TYPE
   SYNTAX             InetAddressType
   MAX-ACCESS         read-create
   STATUS             current
   DESCRIPTION
       "Type of IP packet that this entry will match against.
        Values  unknown(0) and dns(16) are disallowed. All
        source and destination IP address types MUST use the
        same address type."
  
  Are values (0) and (16) really disAllowed. Or is it better to say
  that only IPv4 and IPv6 need to be supported. In the future, that 
  InetAddressType TC may get expanded, and there may be yet other
  values....
  And in fact in the compliance section you do include value (0)
  as a valid value !!!
  Also. This is not the "Type of IP packet". but the "Type of address
  used for the following InetAddress fields/objects". So my DESCRIPTION
  would rather be something aka (using RFC3291 as example):
          "The type of Internet address to be used for the comparison.
           The only values that MUST be supported are unknown(0), ipv4(1)
           and ipv6(2). Note that both source and destinations Internet
           addresses must be of the same InetAddressType."
           
- mplsFTNSourceAddrMin OBJECT-TYPE
   SYNTAX             InetAddress
   MAX-ACCESS         read-create
   STATUS             current
   DESCRIPTION
       "The lower end of source address range.
        The IP address type for this object MUST
        use the mplsFTNAddrType object defined
        for this row."
   ::= { mplsFTNEntry 6 }

  The way to describe it I think  is not that this object MUST use
  .....
  If I follow the example in RFC3291 (the INET-ADDRESS-MIB) then 
  my descripion clause would be something aka:
  This address is 
       "The lower end of source Internet address range.
        The type of this address is determined by the value of 
        the mplsFTNAddrType object."

-mplsFTNSourceAddrMax OBJECT-TYPE
   SYNTAX             InetAddress
   MAX-ACCESS         read-create
   STATUS             current
   DESCRIPTION
       "The upper end of source address range."
   ::= { mplsFTNEntry 7 }

  I think you need to change DESCRIPTION to something aka:
       "The upper end of source Internet address range.
        The type of this address is determined by the value of 
        the mplsFTNAddrType object."

  Yep, you MUST repeat that which InetAddressType object controls format
  of this object as well

- Same for next two InetAddress objects

- mplsFTNExpBits OBJECT-TYPE
   SYNTAX             Unsigned32 (0..127)

  If I remember correctly then we discussed and agreed at the Atlanta
  MPLS MIB meeting that this should probably be a Dscp or DscpOrAny TC
  from the DIFFSERV-DSCP-TC mib module in RFC3289.

- mplsFTNMapIndex OBJECT-TYPE
   SYNTAX              InterfaceIndexOrZero
   MAX-ACCESS          not-accessible
   STATUS              current
   DESCRIPTION
       "The interface index that this FTN entry is being
        applied to. An index with value zero is applied all
        interfaces.  If this entry is created with index zero
        and other indexes are created with non-zero values,
        these entries superceede the entry with index zero."
   ::= { mplsFTNMapEntry 1 }

  To me it is still not exactly clear what the expected behaviour is.
  If I have and entry with MapIndex == 0 and one with MapIndex == 1,
  but not one woith MapIndex == 2.
  Now a packet comes in on Interface with ifIndex == 1, then what?
  I think that you mean in that case that it uses the row(s) with 
  MapIndex == 1, and the row(s) with MapIndex == 0 is ignored/skipped.
  If a packet comes in on interface with ifIndex == 2, then (since
  the is no row with MapIndex == 2) it will be handled
  by the row(s) which have MapIndex == 0.

  That is not 100% clear from the description, or at least, I am
  not sure that I understand it correctly. Can you make it clearer?
  Or... if I am the only one who does not understand, then ignore me.

- mplsFTNMapLastChanged OBJECT-TYPE
   SYNTAX             TimeStamp
   MAX-ACCESS         read-only
   STATUS             current
   DESCRIPTION
       "Indicates the last time an entry was added, deleted
        or modified in this table. Management stations should
        consult this object to determine if the table has
        been modified and requires their attention."
   ::=  { mplsFTNMapEntry 4 }

   This sounds so STRANGE to me. It says "the last time an entry..."
   So if any entry gets added or changed, then ALL rows in this table
   will be updated to reflect the new TimeStamp ???
   If it is only meant to indicate when this RowWasLastChanged, then I
   wonder how much use it has.
 
   An object that indicates when a table was last changed is normally
   an object that is defined outside the table, e.g. as a scalar.

- If you have this (compliance section)

      OBJECT mplsFTNAddrType
      SYNTAX  InetAddressType { unknown(0), ipv4(1), ipv6(2) }
      MIN-ACCESS   read-only
      DESCRIPTION
        "Write access is not required. An implementation is only required
         to support IPv4 and IPv6 addresses."

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

  then you normally also limit the sie of the InetAddress objects, so
  it would be

      OBJECT mplsFTNSourceAddrMin
      SYNTAX InetAddress (SIZE (0 | 4 | 20))
      MIN-ACCESS   read-only
      DESCRIPTION
        "Write access is not required."

  Same for otehr InetAddress objects

  And you do that also for the FullCompliance section.

- I am not sure if Security ADs will be happy with security section.
  They tend to want to see details as to exactly which objects are
   sensitive and why. 

- Split of normative and informative references not correct.
  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

Bert

---

From: "Thomas D. Nadeau" <[email-masked]>
Date: Wed, 04 Dec 2002 13:39:49 -0500

>And that is just for inserting one entry... think about

         I agree with Bert here, but I think I was overruled on this
by the other co-authors a while back. I had proposed a simpler
scheme that only had 2 indexes and only had the "previous"
and "next"pointers as objects within the table:

    INDEX {
          mplsFTNMapIndex,
          mplsFTNMapCurrIndex
    }

         The problem is that if you rely on the ordering
of these indexes to imply how to apply the rules,
then you still run into the same problem you describe
above where you have to move the entire list around.
The best solution is to have the indexing with
MapIndex and CurrIndex and have previous/next
objects to show the relationship. This way you only move
pointers around instead of having to create/destroy rows
in the table. You create the entries for each interface
and then move the pointers.   This is actually an easy and
painless change to the MIB that makes it work well
for configuration cases. I would argue that it makes it
easier to create entries for agents too. The only issue is that
the agent has to keep track of the consistency if rows
are deleted unless you allow configuration, in which case
the manager should do this while deleting rows.

>is easy to understand (or to understand at all) ??

         Sorry, my ASCII art isn't what it used to be (or ever
was?).  I think that if we change the indexing as described
above, it will be greatly simplified.

         --Tom