Subject: MIB Dr. review of draft-ietf-ccamp-gmpls-te-mib-09.txt

From: [email-masked]
Date: Thu, 08 Sep 2005 21:59:48 -0400

Hi Tom and Adrian,

Here are some comments on draft-ietf-ccamp-gmpls-te-mib-09.txt.
All 3 docs show a good deal of work.

  -Joan

draft-ietf-ccamp-gmpls-te-mib-09.txt
======================================

Section 1.1 Migration Strategy
------------------------------

1) Please remove "and OBJECT-IDENTIFIERS"

   Textual conventions and OBJECT-IDENTIFIERS are defined in [RFC3811]
   and [GMPLSTCMIB].

Section 5.4 gmplsTunnelCHoptable
---------------------------------

2) Please capitalize Table in the Section's title.

Section 8. GMPLS Traffic Engineering MIB Module
-----------------------------------------------

smicngPRO gives the following:

W: f(GMPLS-TE-STD-MIB), (1181,14) Item "gmplsTunnelErrorTLVs" should have
SIZE specified
W: f(GMPLS-TE-STD-MIB), (1362,13) For "gmplsTunnelDirection", syntax is
identical
W: f(GMPLS-TE-STD-MIB), (1371,13) For "gmplsTunnelPathComp", syntax is
identical
W: f(GMPLS-TE-STD-MIB), (1382,14) For "gmplsTunnelUpNotRecip", syntax is
identical
W: f(GMPLS-TE-STD-MIB), (1389,14) For "gmplsTunnelDownNotRecip", syntax is
identical
W: f(GMPLS-TE-STD-MIB), (1401,14) For "gmplsTunnelExtraParamsPtr", syntax
is identical

Disconnect of numbering in the Conformance Section
   \-3 gmplsTeConformance
     \-v-1 gmplsTeGroups
       | \-v-1 gmplsTunnelGroup

2 is missing
       |   |-3 gmplsTunnelSignaledGroup
       |   |-4 gmplsTunnelScalarGroup
       |   |-5 gmplsTunnelIsIntfcGroup
       |   |-6 gmplsTunnelIsNotIntfcGroup
       |   |-7 gmplsTunnelOptionalGroup
       |   \-8 gmplsTeNotificationGroup

3)   gmplsTunnelsConfigured OBJECT-TYPE
     SYNTAX  Unsigned32

SYNTAX should be Gauge32

4)   gmplsTunnelsActive OBJECT-TYPE
     SYNTAX  Unsigned32

SYNTAX should be Gauge32

5) gmplsTunnelTable 

DESCRIPTION

       "The row status of an entry in this table is controlled by
        mplsTunnelRowStatus in the corresponding entry in
        mplsTunnelTable. That is, it is not permitted to create a row in
        this table, nor to modify an existing row, when the
        corresponding mplsTunnelRowStatus has value active(1)....

The sentence beginning with "That is,"  is awkward.  Could you say
something like:  A row in this table is created when a row is created
in the mplsTunnelTable.  Entries in this table cannot be modified
when the corresponding mplsTunnelRowStatus has a value of active(1).
The exception....

Please also update this other places in this MIB (e.g gmplsTunnelHopTable).

6) gmplsTunnelEntry
   DESCRIPTION:
      "...
        An entry can be created by a network administrator or by an SNMP
        agent as instructed by a signaling protocol."

Please change the above to:

"An entry can be created by a network administrator via SNMP SET commands, or
by the network management entity as instructed by a signaling 
protocol."

7) gmplsTunnelUnnumIf

What IF-MIB object are referring to in the DESCRIPTION?
Could you state which object in the description?

8) gmplsTunnelAttributes
Could you specify what happens when the bit is set (i.e. 1)
vs. 0?

9) gmplsTunnelLSPEncoding

The DESCRIPTION states:
        ...A value of 'tunnelLspNotGmpls' indicates that GMPLS signaling is
        not in use. Some objects in this MIB module may be of use for
        MPLS signaling extensions that do not use GMPLS signaling. By
        setting this object to 'tunnelLspNotGmpls', an application may
        indicate that only those objects meaningful in MPLS should be
        examined....

I believe that such objects should be put in to an MPLS MIB module
and not included in this GMPLS MIB module.  This is also true with
the gmplsTunnelErrorTable.

10) gmplsTunnelLinkProtection
Same comment as with gmplsTunnelAttributes.

11) gmplsTunnelPathComp

DESCRIPTION states:
        This object deprecates mplsTunnelHopEntryPathComp."

The term "deprecates" is loaded.  Perhaps something like,
this object takes precedence over mplsTunnelHopEntryPathComp.
In other words, for GMPLS tunnels, the value of 
mplsTunnelHopEntryPathComp is meaningless.

12) gmplsTunnelUpNotRecip

Need to use InetAddressType and InetAddress.  IpAddress
is not used in MIBs anymore.  Please be sure to support
both v4 and v6 IP address Types.  If you can't support
v6 for some reason, that should be stated clearly.

Please change the name of this object to:
gmplsTunnelUpstreamRecipientForNotify

13) gmplsTunnelDownNotRecip
Same comment as above wrt to IpAddress.

Please change the name to gmplsTunnelDownstreamRecipientForNotify

14) gmplsTunnelAdminStatusFlags (please see comments
regarding the name of this TC and make adjustments
to the name and DESCRIPTION of this object as appropriate).

15)    gmplsTunnelExtraParamsPtr  OBJECT-TYPE

This object does not seem appropriate for this MIB.
It would seem an enterprise MIB could do something
like this (and possibly not use a RowPointer) so
please explain what the intention is.

16)   gmplsTunnelHopTable  OBJECT-TYPE
        The storage type for an entry in this table is inherited from
        mplsTunnelHopStorageType in the corresponding entry in
        mplsTunnelHopTable.

Please change to:

The storage type for this entry is given by the value
of mplsTunnelHopStorageType in the corresponding entry in the
mplsTunnelHopTable.

17)   gmplsTunnelHopExpLabel OBJECT-TYPE

     DESCRIPTION
       "If gmplsTunnelHopLabelStatuses object indicates that a forward
        label is present and gmplsTunnelHopExpLabelPtr contains the
        value zeroDotZero, then the label to use on this hop is found in
        object encoded within a 32-bit integer."

Think the last part of this sentence is awkward:

...to use on this hop is represented by the value of this object.

18) gmplsTunnelHopExpLabel and gmplsTunnelHopExpLabelPtr

Need to add the word "Forward" in here as in
gmplsTunnelHopExpForwardLabel and gmplsTunnelHopExpForwardLabelPtr

That way it will be consistent with the Reverse counterparts.

19) gmplsTunnelHopExpRvrsLabel and gmplsTunnelHopExpRvrsLabelPtr

Please change Rvrs to Reverse.

20) gmplsTunnelHopLabelStatuses and gmplsTunnelARHopLabelStatuses 

Please change Statuses to State.

21) gmplsTunnelARHopExpLable and gmplsTunnelARHopExpLabelPtr

Need to add the word "Forward" in here as in
gmplsTunnelARHopExpForwardLabel and gmplsTunnelARHopExpForwardLabelPtr

22) gmplsTunnelARHopExpRvrsLabel and gmplsTunnelARHopExpRvrsLabelPtr

Please change Rvrs to Reverse

23)    gmplsTunnelARHopProtection  OBJECT-TYPE

Please add what the BIT set (i.e. 1) vs. 0 means in the
DESCRIPTION.

24) gmplsTunnelCHopLabelTable objects

Similar comments regarding the objects in this table
to those above with regard to naming
conventions (Forward and Reverse) and DESCRIPTIONs.  Will not repeat the
details, please refer to above comments on the
prior 2 tables.

25) gmplsTunnelReversePerfTable
Please add to the DESCRIPTION how a user knows if
this is a bidirectional tunnel (i.e. what object
or objects in the gmplsTunnelTable need to have
what values?).

26) gmplsTunnelReversePerfPackets

Does this object represent the 32-bit
value of the least significant part of the
64-bit value (which is contained in gmplsTunnelReversePerfHCPackets)?

27) gmplsTunnelReversePerfBytes

Does this object represent the 32-bit
value of the least significant part of the
64-bit value (which is contained in gmplsTunnelReversePerfHCBytes)?

28) gmplsTunnelErrorTable  OBJECT-TYPE

Since this table applies to MPLS, it should be
put in a separate MIB module which is MPLS specific.  The
prefix of gmpls could be confusing.

29)    gmplsTunnelErrorTLVs OBJECT-TYPE

Need to add a variable size to this string.  When the string
size is 0 that would indicate that no TLVs are present, so
the DESCRIPTION should be modified to reflect this.

30)   gmplsTunnelErrorHelpString OBJECT-TYPE
     SYNTAX  DisplayString

DisplayString is not used in newer MIBs because it does not
meet internationalization standards, better to use
SnmpAdminString or something which meets internationalization
standards.

31) Compliance

Please move some of the following comment in the
DESCRIPTION of gmplsTeModuleFullCompliance

   -- The mandatory group has to be implemented by all LSRs that
   -- originate, terminate or act as transit for TE-LSPs/tunnels.
   -- In addition, depending on the type of tunnels supported, other
   -- groups become mandatory as explained below.

32) gmplsTunnelUnnumIf does not appear in the ReadOnly compliance
and it probably should.

Section 11.2.1. IANA-GMPLS-MIB Definition
-----------------------------------------
33) Please change the name of this MIB to
IANA-GMPLS-TC-MIB 

and instead of:   ianaGmpls MODULE-IDENTITY
please use:       ianaGmplsTcMIB MODULE-IDENTITY

34)  Please add the following to the DESCRIPTION:
       "Copyright (C) The Internet Society (2005). The initial version
        of this MIB module was published in RFC XXXX. For full legal
        notices see the RFC itself.  Supplementary information
        may be available on:
        http://www.ietf.org/copyrights/ianamib.html

35) Please change GMPLS-TE-MIB's  to GMPLS-TE-STD-MIB's [RFCXXX] (RFC-Editor
please fill in the XXX based on the RFC number for
draft-ietf-ccamp-gmpls-te-mib
and remove this note)  in the 
DESCRIPTION clauses of ALL TCs.

36) Please change the name IANAGmplsLSPEncoding to 
   IANAGmplsLSPEncodingType.  (more closely matches what's on
IANA's website now)

37) Please change the name IANAGmplsGPid to 
   IANAGmplsGeneralizedPid  

38) I believe that the TC 

   IANAGmplsAdminStatusFlags ::= TEXTUAL-CONVENTION

should be put in the GMPLS-TC-STD-MIB or should just
remain in the GMPLS-TE-STD-MIB.  The reason is that this
is not an assigned numbers list maintained by IANA, so 
should not be in the IANA GMPLS MIB.  If I am incorrect
about this please let me know, but I didn't see an IANA
maintained Administrative Status Information enum.

Additionally, this should be renamed to 
GmplsAdminStatusInformation which more closely matches rfc3471.

39) Several comments on the SYNTAX clause:

40a) smicngPRO gives the error

E: f(IANA-GMPLS-MIB.my), (252,22) Named bits for BITS 
must be in contiguous positions

      SYNTAX  BITS {
                     delInProgress (0),
                     adminDown (1),
                     testing (2),
                     reflect (31)  
                   }

RFC2578 specifies that BITS should be contiguous
(although there are exceptions to this, but this
object does not seem to be an exception).

Could delInProgress, be renamed to deleteInProgress ?
Could adminDown be renamed to administrativelyDown?
These names more closely match 3471.

40b)  Please put in the specific section number on this
reference (section 8?).

-- the end --

---

From: "Adrian Farrel" <[email-masked]>
Date: Thu, 22 Sep 2005 13:59:39 +0100

Hi Joan,

Last but not least, the GMPLS TE MIB module.

> All 3 docs show a good deal of work.

Yes, but fortunatley most of the work appears to be editorial with only
very minor changes to the basis of the module.

Please find acks and nacks in line below.

We will also include the comments made by David and Tom Petch.

Cheers,
Adrian

> Section 1.1 Migration Strategy

Ack

> Section 5.4 gmplsTunnelCHoptable

Ack

> Section 8. GMPLS Traffic Engineering MIB Module

Ack
Understood this to be optional, but planning to add size as per David's
comments

> W: f(GMPLS-TE-STD-MIB), (1401,14) For "gmplsTunnelExtraParamsPtr",
syntax

Nack
Meaningless warning message. Cannot fix without explanation.

> Disconnect of numbering in the Conformance Section

Ack

> 3)   gmplsTunnelsConfigured OBJECT-TYPE

Ack

> 4)   gmplsTunnelsActive OBJECT-TYPE

Ack

> Please also update this other places in this MIB (e.g
gmplsTunnelHopTable).

Nack
"A row in this table is created when a row is created in the
mplsTunnelTable" is false.
Will attempt to reword to make this clearer.

> by the network management entity as instructed by a signaling

Nack
"network management entity" implies a remote entity such as an NMS.
Will change to:

  "An entry can be created by a network administrator via SNMP SET
   commands, or in response to signaling protocol events."

> Could you state which object in the description?

Ack
An entry in ifTable.

> Could you specify what happens when the bit is set (i.e. 1)

Ack
Don't want to restate the protocol I-D, so this text will simply
replace "This flag indicates that" with "This flag is set to indicate
that"

> the gmplsTunnelErrorTable.

Nack
We strongly disagree with any implication that MPLS and GMPLS should be
separated in this way. GMPLS protocol extensions are likely to be
moved into MPLS implementations piecemeal and there is no clear
separation that you imply. The G in GMPLS stands for "generalized".

Can you please indicate whether this is a sticking point for you.

> Same comment as with gmplsTunnelAttributes.

Ack
Same answer.

> mplsTunnelHopEntryPathComp is meaningless.

Ack

> v6 for some reason, that should be stated clearly.

Ack per exchange with David.

> gmplsTunnelUpstreamRecipientForNotify

Nack
We are trying to keep to the (mythical?) 32 character limit.
How about:
gmplsTunnelUpNotifyRecipient

> Same comment as above wrt to IpAddress.

Ack

> Please change the name to gmplsTunnelDownstreamRecipientForNotify

Nack as above
How about:
gmplsTunnelDownNotifyRecipient

> to the name and DESCRIPTION of this object as appropriate).

Ack

> please explain what the intention is.

Nack

See discussions of rowPointer in the LSR MIB.
We need to be easily extensible to standard MIB modules that will provide
additional parameters as described in the DESCRIPTION clause. Such MIB
modules will contain tables the entries of which will each be used by
multiple tunnels.
In this context a row pointer is appropriate.

> of mplsTunnelHopStorageType in the corresponding entry in the

Ack

> ...to use on this hop is represented by the value of this object.

Ack

> That way it will be consistent with the Reverse counterparts.

Nack
Please see point 21 about name lengths.
We will insert "Fwd"

> 19) gmplsTunnelHopExpRvrsLabel and gmplsTunnelHopExpReverseLabelPtr
>

Nack
Please see point 21 about name lengths

> Please change Statuses to State.

Nack
Status is the correct word. State would be incorrect.

> gmplsTunnelARHopExpForwardLabel and gmplsTunnelARHopExpForwardLabelPtr

Nack
Would excede 32 characters.
Will use "Fwd"

> Please change Rvrs to Reverse

Nack
See point 21

> Please add what the BIT set (i.e. 1) vs. 0 means in the

Ack
Limited change as before.

> details, please refer to above comments on the

Nack
Same answers as before.
Will use "Fwd"

> or objects in the gmplsTunnelTable need to have

Ack

> 64-bit value (which is contained in gmplsTunnelReversePerfHCPackets)?

Ack

> 64-bit value (which is contained in gmplsTunnelReversePerfHCBytes)?

Ack

> prefix of gmpls could be confusing.

Nack
This is applicable across MPLS and GMPLS.
Error TLVs are not present in MPLS RSVP-TE at this time. They may be added
later.

> the DESCRIPTION should be modified to reflect this.

Ack
As per discussion with David.

> SnmpAdminString or something which meets internationalization

Ack

>    -- groups become mandatory as explained below.

Ack

> 32) gmplsTunnelUnnumIf does not appear in the ReadOnly compliance

Ack

> Section 11.2.1. IANA-GMPLS-MIB Definition

Ack

>         http://www.ietf.org/copyrights/ianamib.html

Ack

> DESCRIPTION clauses of ALL TCs.

Ack

>    IANAGmplsLSPEncodingType.  (more closely matches what's on

Ack

> 37) Please change the name IANAGmplsGPid to

Ack

> maintained Administrative Status Information enum.

Nack
There are no fewer than three requests in CCAMP I-Ds that have passed WG
last call
for this space to be managed by IANA.

> GmplsAdminStatusInformation which more closely matches rfc3471.

Ack

> object does not seem to be an exception).

Nack

To quote the DESCRIPTION...
             The relationship between the assignment of
             gmplsTunnelAdminStatusFlags values and of the bit flags
             in the Admin Status object or TLV within the GMPLS
             signaling protocols is solely the purview of IANA and is
             subject to change without notice."
However, it seems to us that making the bits match the flags in the
Admin Status object is a Good Idea (TM).

> These names more closely match 3471.

Ack

> 40b)  Please put in the specific section number on this

Ack

---

From: <[email-masked]>
Date: Thu, 29 Sep 2005 22:13:33 -0400

hi Adrian,

Please see comments inline below, I have snipped parts where we
are in agreement.

thanks, joan

----- Original Message -----
From: Adrian Farrel <[email-masked]>
To: <[email-masked]>; <[email-masked]>; <[email-masked]>
Cc: <[email-masked]>; <[email-masked]>
Sent: Thursday, September 22, 2005 8:59 AM
Subject: Re: MIB Dr. review of draft-ietf-ccamp-gmpls-te-mib-09.txt

> Meaningless warning message. Cannot fix without explanation.

The object is defined as:

gmplsTunnelDirection OBJECT-TYPE
     SYNTAX  INTEGER {
       forward (0),
       bidirectional (1)
     }
     MAX-ACCESS read-create
     STATUS  current
     DESCRIPTION
       "Whether this tunnel carries forward data only (is
        unidirectional) or is bidirectional.

        Values of this object other than 'forward' are meaningful
        only if gmplsTunnelLSPEncoding is not set to
        'tunnelLspNotGmpls'."
     DEFVAL { forward }
   ::= { gmplsTunnelEntry 8 }

The following (part of the ReadOnly compliance statement) is causing
the warning because the SYNTAX defined in the object and the
SYNTAX below are the same.  (Following the OBJECT are a
few alternatives.)

  OBJECT gmplsTunnelDirection
     SYNTAX INTEGER {
       forward (0),
       bidirectional (1)
     }
     MIN-ACCESS  read-only
     DESCRIPTION
       "Only forward (0) is required."

If you do intend for the SYNTAX in both the OBJECT-TYPE and
the OBJECT to be the same, then removing it from the OBJECT
would get rid of the warning.  This is not a blocking issue, but
it is duplicate info that does not need to be duplicated.
So here is what the OBJECT statement would like:

  OBJECT gmplsTunnelDirection
     MIN-ACCESS  read-only
     DESCRIPTION
       "Only forward (0) is required."

Another alternative would be to specify forward(0) only since
this is what the DESCRIPTION is specifying, however, it is
likely that you are probably allowing both, so I don't think this
is what you intend, but this would also solve the compiler warning:

  OBJECT gmplsTunnelDirection
     SYNTAX INTEGER {
       forward (0),
     }
     MIN-ACCESS  read-only
     DESCRIPTION
       "Only forward (0) is required."

> commands, or

Yes, I should have said, "network management entity (e.g., snmp agent).

>    commands, or in response to signaling protocol events."

good.

> > Could you state which object in the description?

Could you specify the ifType(s)?

> Can you please indicate whether this is a sticking point for you.

As was suggested in an email on draft-ietf-ccamp-gmpls-lsr-mib, could
you put these objects in a separate object group?  Also, please separate
MPLS and MPLS-TE objects  into separate object groups.

> gmplsTunnelUpNotifyRecipient

How about gmplsTunnelUpstreamNotifyRecipient

As for the 32 char limit, we've already addressed that, so won't
repeat that discussion.

> gmplsTunnelDownNotifyRecipient

How about gmplsTunnelDownstreamNotifyRecipient

> Please see point 21 about name lengths

In other objects in this MIB module are named using the entire
word Reverse ( e.g.
GmplsTunnelReversePerfEntry ::= SEQUENCE {
     gmplsTunnelReversePerfPackets     Counter32,
     gmplsTunnelReversePerfHCPackets   Counter64,
     gmplsTunnelReversePerfErrors      Counter32,
     gmplsTunnelReversePerfBytes       Counter32,
     gmplsTunnelReversePerfHCBytes     Counter64
   })

So please either change these to all use Rvrs or instead of Rvrs use
the entire word, Reverse.   Similarly with the use of Fwd and Forward,
if you decide to use Reverse, then please use the entire word, Forward.
If you decide to use Rvrs, then please use Fwd.    I prefer the words
spelled out but just please be consistent.

> Would excede 32 characters.

See discussion above wrt Rvrs and Reverse.

> Error TLVs are not present in MPLS RSVP-TE at this time. They may be added

Discussed previously about separate OBJECT groups.

> for this space to be managed by IANA.

Is this going to be managed by IANA soon?  (just curious)

> Admin Status object is a Good Idea (TM).

Here is the TLV that the object represents:

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
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |R|                        Reserved                       |T|A|D|
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

      Reflect (R): 1 bit
      Reserved: 28 bits
      Testing (T): 1 bit
      Administratively down (A): 1 bit
      Deletion in progress (D): 1 bit

The BITS as specified look backwards when viewing the TLV.
Are the BITS correctly specified?

I consulted with other MIB Doctors and the following was suggested:
     SYNTAX  BITS {
                      delInProgress (0),
                      adminDown (1),
                      testing (2),
                      reserved3(3),       -- 0 on send, ignored on receive
                      reserved4(4),       -- 0 on send, ignored on receive
                      ..
                      reserved30(30),      -- 0 on send, ignored on receive
                      reflect (31)
                    }

Would this be acceptable?