[mpls] AD review of draft-ietf-mpls-tp-te-mib

["Adrian Farrel" <[e-mail]>]

Hello authors,

I have performed my usual AD review of your draft on receipt of the
publication request. As you can see below, it has generated quite a few
comments and as a result I have placed the document into "Revised I-D
Needed" state. I have tried to flag where I think my comment might be
a function of "I wouldn't have done it like this" and I am open to 
discussion of all of the points I have raised.

Thanks for your work.

Adrian

===

I think Tom may want to change his coordinates.

---

It would be nice if someone could do a pass on this document for
English and format. It is not essential because the RFC Editor will
resolve these issues, but it is wise because the fewer changes the RFC
Editor makes, the less chance there is of an accidental error.

---

You have the RFC 2119 boilerplate present twice.

---

What does it mean to use RFC 2119 language in the examples in Section 9?

---

I see a contradiction in

 In
 particular, it describes managed objects of Tunnels, Identifiers, Label
 Switching Router and Textual conventions for Multiprotocol Label
 Switching (MPLS) based Transport Profile (TP). These MIB modules extend
 the existing MPLS MIB objects for both MPLS-TP and Non-MPLS-TP
 operations, so the MPLS-TP name is not included in the MIB module name.


Either the extensions are for MPLS-TP or they are not. Your second
sentence says they are for more general applicability, and I agree with
that. So why does the first sentence and the document title talk about
MPLS-TP?

---

I think the Introduction would be made more useful and relevant if it
described the purpose and content of this document. Currently it says
what the existing documents don't do (although even that is pretty
vague).

 The existing Multiprotocol Label Switching (MPLS) Traffic Engineering
 (TE) Management Information Base (MIB) [RFC3812] and Generalized
 Multiprotocol Label Switching (GMPLS) Traffic Engineering Management
 Information Base [RFC4802] do not support the transport network
 requirements of NON-IP based management and static bidirectional
 tunnels.

There is also an implication here that this MIB module supports non-IP
based management. I looked but didn't see any description of how to run
SNMP in the absence of IP.

It goes on...

 These MIB modules should be used in conjunction with [RFC3812]
 and companion document [RFC3813] for MPLS-TP tunnel configuration and
 management.

What is an "MPLS-TP tunnel"?

---

Section 4 might have been an opportunity to provide the information I
think is missing from the Introduction. But unfortunately it repeats
some of the text form the Introduction and then gives a very scanty
description of the purpose of the work.

Here, however, I find the text

 As the existing MPLS-
 TE-STD-MIB is not sufficient to capture all the characteristics of the
 tunnels, enhancing the MIB to support MPLS TP tunnels is required. As
 most of the attributes of MPLS Traffic Engineering tunnels are also
 applicable to MPLS-TP tunnels, it is optimal to re-use the existing MIB
 definition instead of a new MIB.

This doesn't explain why you don't leverage GMPLS-TE-STD-MIB and
GMPLS-LSR-STD-MIB. Naively, it seems that the co-routed case is already
handled, and the associated case can be handled with just one cross-
pointer (the association) between the forward and reverse entries in
MPLS-TE-STD-MIB.

I hate to say "I would not have done it this way", but you seem to be
introducing a lot of complexity.

---

I think the boilerplate in Section 2 is supposed to have the references
in square brackets as citations.

---

Not sure you need all of your acronyms

GMPLS is only used in the same place as it is expanded.
IP is surely too well known
ITU and ITU-T are "well-known" according to the RFC editor
MIB is "well-known" according to the RFC editor
MPLS is "well-known" according to the RFC editor
OSPF is "well-known" according to the RFC editor

---

Why does section 5 only discuss MPLS-TE-EXT-STD-MIB? What about the
other MIB modules defined in this document?

---

Why does section 6 only discuss MPLS-TE-EXT-STD-MIB? What about the
other MIB modules defined in this document?

---

Section 6 introduces 5 new tables. Section 8 shows a figure containing
only 3 of the new tables. Something missing?

---

Section 6 is titled

6. Brief description of MPLS-TE-EXT-STD-MIB Objects

but I think it really only describes the tables.

---

Section 6.1 has

   Each ICC_Operator_ID::Node_ID or Global_ID::Node_ID contains one
   unique entry in the table representing a node.

What does this containing relationship mean?

---

Section 6.1

   As the regular TE
   tunnels use IP address as LSR ID, the local identifier should be
   below the first valid IP address, which is 16777216[1.0.0.0].

What does "should" mean in this context?

---

There is no clear mention of the dependency on MPLS-TC-STD-MIB.
I think this should be added to Section 7.
It is probably best to not attempt to add this to the figure.
Instead you could add text to say:
   Note that all of the MB modules shown in the figure also have a
   dependency on MPLS-TC-STD-MIB.

---

I think you could usefully clarify the meanings of "extends" versus
"augments" versus "sparse augments".

In 6.4 you have:
   mplsTunnelExtTable extends the mplsTunnelTable

In 6.5 you have:
   This table sparse augments the mplsTunnelTable

In Section 7 you have:
   MPLS-TE-STD-MIB [RFC3812] is extended by MPLS-TE-EXT-STD-MIB
   MIB module for associating the reverse direction tunnel
   information.

   Note that the nature of the 'extends' relationship
   is a sparse augmentation so that the entry in the
   mplsTunnelExtTable has the same index values as the in the
   mplsTunnelTable.

   MPLS-LSR-STD-MIB [RFC3813] is extended by MPLS-LSR-EXT-STD-MIB
   MIB module for pointing back to the tunnel entry for easy tunnel
   access from XC entry.

   Note that the nature of the 'extends' relationship
   is a sparse augmentation so that the entry in the
   mplsXCExtTable has the same index values as the in the mplsXCTable.

I think that all of uses of "table A extends table B" mean "sparse
augments" so I suggest you just use the latter language and avoid
confusion and the need to explain the meaning of "extends".

---

There is an awful lot of write-access in these MIB modules. Haven't we
moved on from 2004 to a view that SNMP SETs on this type of complex
data are not useful?

Why did the working group decide that creatable/writeable objects were
valuable?

---

Section 9 would be a lot clearer if it began with a description of the
LSP being "configured".

---

Entries in MplsTunnelExtNodeConfigEntry are indexed by
mplsTunnelExtNodeConfigLocalId.

A combination of [Global_ID and Node_ID] or [CC::ICC and Node_ID] has
a unique entry.

Suppose I have a new LSP in my hand. It has [CC=AB, ICC=123456]. I
want to create an entry or use an existing one.

Do I have to read every entry in the table to find out if one
already exists?

---

The first and only mention of pseudowires is in the Description clause
of MPLS-TC-EXT-STD-MIB.

This either means you are missing lots of explanations and examples, or
that the Description clause (and terminology section) should have PW
removed.

---

Shouldn't the description and definition of MplsGlobalId be more
proscriptive and precise?

The Description says...

            the Global_ID can
            contain the 2-octet or 4-octet value of the operator's
            Autonomous System Number (ASN).

I.e. "can".  Also...

            It is expected that the Global_ID will be derived from
            the globally unique ASN of the autonomous system hosting
            the PEs containing the actual AIIs.

I.e. "expected". And...

            When the Global_ID is derived from a 2-octet AS number,
            the two high-order octets of this 4-octet identifier
            MUST be set to zero.

I.e. "derived from". And finally a "MUST" but still "derived from"

            A non-zero Global_ID MUST be derived from an ASN owned by
            the operator."

But the Syntax is given as...

      SYNTAX  OCTET STRING (SIZE (4))

...and since the mechanism for derivation is not explained and there is
no limitation stated that the characters in the Octet String be digits,
I can think of many ways to "derive" the Global ID.

Furthermore, in...

            When the Global_ID is derived from a 2-octet AS number,
            the two high-order octets of this 4-octet identifier
            MUST be set to zero.

... Does "set to zero" mean set to 0x00 or set to 0x30?

---

Why is it necessary to allow a zero length of MplsIccId when you don't
allow a zero length of MplsGlobalId or MplsCcId. I don't see why the ICC
is special in this respect.

---

In MplsIccId what does this mean?

            Alphabetic characters in the ICC SHOULD be represented
            with upper case letters.

How does that change the implementation? I think it means that it has
to accept upper and lower case characters. Furthermore, what might be
signaled?

I suggest you should be consistent with T.50 for the CC and ICC
character sets.

---

   MplsNodeId ::= TEXTUAL-CONVENTION
      DISPLAY-HINT "d"
      STATUS      current
      DESCRIPTION
          "The Node_ID is assigned within the scope of
           the Global_ID/ICC_Operator_ID.
           The value 0(or 0.0.0.0 in dotted decimal notation)
           is reserved and MUST NOT be used.

           When IPv4 addresses are in use, the value of this object
           can be derived from the LSR's IPv4 loop back address.
           When IPv6 addresses are in use, the value of this object
           can be a 32-bit value unique within the scope of
           a Global_ID.

Is the mention of 0.0.0.0 meaningful?
The display hint is "d" and the value might just be a 32 bit number
for the IPv6 case or for the non-IP case.

---

   MplsNodeId ::= TEXTUAL-CONVENTION
      DISPLAY-HINT "d"
      STATUS      current
      DESCRIPTION
          "The Node_ID is assigned within the scope of
           the Global_ID/ICC_Operator_ID.
           The value 0(or 0.0.0.0 in dotted decimal notation) is
           reserved and MUST NOT be used.
      SYNTAX  Unsigned32 (0|1..4294967295)

If zero MUST NOT be used, why is it allowed in the Syntax?
What is it reserved for?

---

What is the point of defining

     -- notifications
     mplsIdNotifications OBJECT IDENTIFIER ::= { mplsIdStdMIB 0 }

if you don't use it?

---

I am slightly bothered that MPLS-ID-STD-MIB includes TCs with wider
applicability and scalar objects.

I suspect that the CC and ICC TCs could be used more generally in the
IETF. I suspect that many uses of these TCs would not be interested in
the scalar MPLS objects, or that those scalar objects are not limited
to MPLS. And I wonder why the scalars are not in the
MPLS-LSR-EXT-STD-MIB module as they, like the LSR ID, apply to the local
node.

---

The objects in MPLS-ID-STD-MIB are read-write (not read-create). Taking
one as an example, you have...

   mplsIdGlobalId OBJECT-TYPE
        SYNTAX      MplsGlobalId
        MAX-ACCESS  read-write
        STATUS      current
        DESCRIPTION
            "This object allows the operator to assign a unique
             operator identifier also called MPLS-TP Global_ID.
             If this value is used in mplsTunnelExtNodeConfigGlobalId
             for mapping Global_ID::Node_ID with the local identifier
             then this object value SHOULD NOT be changed."
       ::= { mplsIdObjects 1 }

Since the object is not read-create, we can assume that it is pre-
populated by the node (from configuration). The meaning of "SHOULD NOT
be changed" is then in conflict with the writeability of the object.
Furthermore, "SHOULD NOT" is not "MUST NOT" so I think you need to
explain the circumstances under which it can be changed - that probably
really means explaining the consequences if it is changed.

And anyway, I think you mean "...SET operations on this object SHOULD
be rejected by the node with the return code foo" since the management
agent is unlikely to know that the condition applies.

But really...
Why are any of these objects writeable?

---

   mplsIdNodeId OBJECT-TYPE
        SYNTAX      MplsNodeId
        MAX-ACCESS  read-write
        STATUS      current
        DESCRIPTION
           "This object allows the operator or service provider to
            assign a unique MPLS-TP Node_ID.

Why can operators and service providers assign Node_IDs, but only
operators assign Global_IDs in mplsIdGlobalId?

---

   mplsIdCc OBJECT-TYPE
        SYNTAX      MplsCcId
        MAX-ACCESS  read-write
        STATUS      current
        DESCRIPTION
            "This object allows the operator or service provider to
             assign a unique Country Code (CC). Global uniqueness is
             assured by concatenating the ICC with a
             Country Code (CC).

"...assign a unique CC" to what?
"Global uniqueness" of what?

---

   mplsIdIcc OBJECT-TYPE
        SYNTAX      MplsIccId
        MAX-ACCESS  read-write
        STATUS      current
        DESCRIPTION
            "This object allows the operator or service provider to
             assign a unique MPLS-TP ITU-T Carrier Code (ICC) to a
             network.

I don't think so!
This object is going to be realised on a node, not "on a network".
Setting this object will not make a change to the ICC applied to the
rest of the network.

---

   mplsIdModuleFullCompliance MODULE-COMPLIANCE
      STATUS current

      DESCRIPTION
           "Compliance statement for agents that provide full
             support the MPLS-ID-STD-MIB module."

      MODULE -- this module

         -- The mandatory group has to be implemented by all
         -- LSRs that originate/terminate MPLS-TP paths.

This may be true, but it implies that other nodes do not need to
support this module. Surely it is needed for MPLS-TP OAM support.

---

The definition of mplsIdModuleReadOnlyCompliance seems to imply that an
IP-capable node MUST support mplsIdIcc and mplsIdCc. Why is that?

---

Odd, but maybe not important, that the objects in
mplsIdModuleReadOnlyCompliance and mplsIdScalarGroup are not in the
same order as those in mplsIdObjects.

---

I don't see why you need mplsXCExtTable. I think there may be some
confusion about how the TE MIB and LSR MIB are related.

In 3812 and 3813 it is entirely deliberate that there is no back pointer
from the XC to the tunnel. The point being that you do not look up an XC
and then try to find out which tunnel it belongs to.

Perhaps you were looking at how to associate the forward and reverse
XCs, but I note (in your example in 9.2) that you have used the same XC
index for the forward and reverse XCs (which is the right way to do it).
You should note that the extensions in 4803 give directions to the
segments to support this and allow you to distinguish between the
forward XC and the reverse XC. You should also note that
mplsTunnelXCPointer gives a pointer from tunnel to XC, so all that is
missing is the association between forward and reverse tunnels.

I would think that the correct way is with an extension to the
mplsTunnelTable to point to "associated tunnels" in a generic way
that supports all association types not just this one specific
association.

So I am left wondering what is the purpose of mplsXCExtTable

Ah, is it the case that you think the forward and reverse tunnels and
their corresponding XCs might be set up *before* you know that they are
associated? I don't think this happens.

To try to clarify this, I think you have...

Tunnel1-->XC1<--------------
   A      | |               |
   |      | |-->InSeg1      |
   |      | |-->OutSeg1     |
   |      v                 |
    ------XCext1            |
           |                |
           v                |
Tunnel2-->XC2               |
   A      | |               |
   |      | |-->InSeg2      |
   |      | |-->OutSeg2     |
   |      v                 |
    ------XCext2------------

And it might be useful to show this figure somewhere.
But your example doesn't do this.

What I was expecting is:

Tunnel1-->XC1
 A        A |
 |        | |-->InSeg1
 |        | |-->OutSeg1
 V        V
Tunnel2-->XC2
            |
            |-->InSeg2
            |-->OutSeg2

Where the only thing that is not already in the existing MIB modules is
the pointers associating Tunnel1 and Tunnel2.

Noting that for a bidir tunnel what you currently have (in the existing
MIB modules) is:

Tunnel1-->XC1
          A |
          | |-->InSeg1
          | |-->OutSeg1
          V
          XC2
            |
            |-->InSeg2
            |-->OutSeg2

And if you insist on knowing the tunnel that applies to an XC (which, as
you point out would be read-only information) then your extension table
only needs a pointer from XC to tunnel to give:

Tunnel1<->XC1
 A        A |
 |        | |-->InSeg1
 |        | |-->OutSeg1
 V        V
Tunnel2<->XC2
            |
            |-->InSeg2
            |-->OutSeg2

I can't decide how much of this is "I wouldn't have done it this way"
and how much is broken. You are certainly failing to take advantage of
the fact that all related XCs have the same XCindex value.

---

Assuming that you continue with the mplsXCExtTable, what happens
when the entry in mplsXCTable corresponding to
mplsXCExtOppositeDirXCPtr is deleted?

---

When I create a new entry in mplsTunnelExtNodeConfigTable, how do I
know what is a suitable value for mplsTunnelExtNodeConfigLocalId?

You probably need an mplsTunnelExtNodeConfigLocalIdNextFree scalar.

---

The Description clause of mplsTunnelExtOppositeDirPtr is pretty hard to
read. It might be easier if used the term "associated bidirectional".

            "This object is applicable only for the bidirectional
             tunnel that has the forward and reverse LSPs in the
             same tunnel or in the different tunnels.

...is quite confusing.

             The value of zeroDotZero indicates single tunnel entry
             is used for bidirectional tunnel setup."

Surely it is also used when only one of the two tunnels has been set up.
That means that you cannot use the value of zeroDotZero to determine
that a single bidirectional tunnel is in use. And this is presumably
why you have mplsTunnelExtOppositeDirTnlValid

---

I have the same problem understanding the Description of 
mplsTunnelExtDestTnlIndex 

            "This object is applicable only for the bidirectional
             tunnel that has the forward and reverse LSPs in the
             same tunnel or in the different tunnels.

---

Why did you need to define mplsTunnelExtReversePerfTable?

If you have two associated tunnels then each has its own
mplsTunnelPerfEntry.

If you have a single bidirectional tunnel then GMPLS-TE-STD-MIB has
gmplsTunnelReversePerfTable.

---

Section 14 should also mention objects with MAX-ACCESS of read-create.