Subject: [Pce] AD review of draft-ietf-pce-pcep-mib

From: "Adrian Farrel" <[email-masked]>
Date: Sun, 10 Aug 2014 12:44:13 +0100

Hello authors,

This is very substantial and detailed piece of work. Many thanks for
the considerable effort it represents.

I have done my usual AD review of this document in order to process the
publication request. The purpose of the review is to catch any issues
that might otherwise show up in IETF last call or IESG review, and to
ensure that I can fully support the document.

I'm glad to hear that there are implementations underlying this work and
that should be used as a moderating influence on my comments and 
questions. For many of the points I raise it may be OK to answer "We 
have thought about it, but this is how we chose to implement it."

Please look through the comments and let me know your thoughts either by
providing a new revision, or by answering via email.

Thanks for the work,
Adrian

=====

Section 1 para 2

   The PCE communication protocol (PCEP)

The convention these days seems to be "Path Computation Element
Communications Protocol" or "Path Computation Element communications
Protocol".

You might apply that to the document title as well.

---

Section 1

   The PCE communication protocol (PCEP) is the communication protocol
   between a PCC and PCE for point-to-point (P2P) path computations and
   is defined in [RFC5440].

That reference to P2P seemed odd to me. Any reason to include it?
In particular, nothing in this module appears to be specific to whether
P2P or P2MP request are being supported.

---

That previous point does cause me to wonder about "PCE capabilities".
I can see that you have tried to limit this module to describing the
features that are core to 5440, but I wonder whether that is wise.

For example, RFCs 5088 and 5089 define a set of PCE capabilities that
may be advertised in the IGPs and are surely relevant to the model of
a PCE that speaks PCEP. That information might usefully be seen in the
module at the PCE, and also at the PCC so that an operator can check
"why does that PCC keep sending these requests to the wrong PCE?"

Similarly, the Open Object can carry TLVs that indicate further
capabilities. Obviously you can't just include all future capabilities
TLVs since you don't know what they are (well, you know some, but that
have already been defined, but you can't know the undefined ones). But
you might want to to look at how those capabilities will be hooked in
for future accessibility.

Of particular interest will be the OF-list TLV of RFC 5541.

---

Convention has it that you have written a "MIB module" which is part of
"the MIB", so can you look through the document and mainly change "MIB"
to "MIB module".

---

I'm wondering under what circumstances the pcepEntityTable has more than
one entry. You might describe that in 4.1 and also in the Description
clause for the table.

That would tie in with explaining why you have an index of
Unsigned32 (1..2147483647) which allows for quite a lot of entities!

For a moment I thought that maybe there would be one entry when acting
as PCE and one when acting as PCC (i.e., a max of 2 < 2147483647) but
there is no mode field in the entityTable, only the peerTable.

Actually, I'm a bit confused about the indexing of the three tables.
I think you think that every PCE and PCC in the network shows in the
entityTable. But that isn't how you have set up the entityTable to
contain information that is about the local PCEP entity/entities. So
my confusion...

Now, looking at the indexes to pcePcepPeerTable. There are two
questions.

1. Why do you use pcePcepEntityIndex as an index? It points back into
   entityTable which we have established is basically the local PCEP
   speaker or which there is probably just one.

   The text in 4.2 says...

      The pcePcepPeerTable contains one row for each PCEP peer that the
      PCEP entity (PCE or PCC) knows about.

   I think your intention is that pcePcepEntityIndex is different for
   each peer. But pcePcepEntityIndex is the index to PcePcepEntityTable
   which is full of local PCEP entity information and (I suspect) will
   only ever have one entry.

   Shouldn't the peerTable have its own unique index for each peer?

2. Making pcePcepPeerAddrType and pcePcepPeerAddr indexes as well would
   appear to allow two or more entries for the same peer with different
   addresses.

   Is that the intention? If so, you might make it clear in section 4.2.
   And you would also need to make clear that there is an expectation
   that an implementation will assign the same value of
   pcePcepEntityIndex (or the new per-peer index) to each table entry
   that represents the same peer.

   OTOH, if a peer has multiple addresses, will this allow you to have
   multiple sessions to the same peer. Is that what you intended?

   Alternatively, it seems unlikely that two different peers will have
   the same address. So why is any additional index (i.e.,
   pcePcepEntityIndex) needed?

The same questions apply to the indexing of pcePcepSessTable although
pcePcepSessInitiator is understandable.

The solution to all this will be:
- Decide how many entries you really expect in entityTable.
- Decide whether entries in peerTable and sessTable should be indexed
  from entityTable, or with an index from peerTable.

---

A small worked example would be cool (either between Sections 4 and 5,
or in an Appendix).  I would draw a figure such as:

    PCE1---PCE2   PCE3
      |   /  |    /  |
      |  /   |   /   |
    PCCa/    PCCb   PCCc

...and give an example of the module as read at PCE2 and PCCb.

---

The OID structure is unusual. I'm not saying it is wrong, but it is
different around pcePcepObjects. It would probably help to include a
diagrammatic representation. I think you have something like the
following.

(BTW, the "unusual" is the additional indirection between pcePcepObjects
and the various tables.)

  mib-2
  |
  |--- XXX pcePcepMIB
           |
           |--- 0 pcePcepNotifications
           |      |
           |      |--- 1 pcePcepSessUp
           |      :    :
           |
           |--- 1 pcePcepObjects
           |      |
           |      |--- 1 pcePcepEntityObjects
           |      |      |
           |      |      |--- 1 pcePcepEntityTable
           |      |
           |      |--- 2 pcePcepPeerObjects
           |      |      |
           |      |      |--- 1 pcePcepPeerTable
           |      |
           |      |--- 3 pcePcepSessObjects
           |             |
           |             |--- 1 pcePcepSessTable
           |
           |--- 2 pcePcepConformance
                  |
                  |--- 1 pcePcepCompliances
                  |--- 2 pcePcepGroups

---

I'd also like to see a short section (probably just containing a figure)
that shows the relationship between the three tables in this module.

It can't be complex (there are only three tables!), but a figure that
shows which objects in which tables lead you to find rows in other
tables would be a help. And where the relationship is shared indexes or
augmentation that can be called out.

---

I am quite happy that this module is entirely read-only. But it makes
some of the "usual" objects a little odd without additional information
in their Description clauses. For example, pcePcepEntityAdminStatus says
   "The administrative status of this PCEP Entity."
That is fine, but what does it mean?
You could probably add:
   "... This is the desired operational status as currently set by an
   operator or by default in the implementation. The value of
   pcePcepEntityOperStatus represents the current status of an attempt
   to reach this desired status."

---

I like the values you have offered in pcePcepEntityOperStatus
A common accompaniment to the failure cases (and maybe the inactive /
deactivating cases) is an unformatted reason string. Did you consider
adding one of these?

---

Do you support values of pcePcepEntityAddrType other than ipv4(1) and
ipv6(2)?

Maybe unknown(0) is used when pcePcepEntityAddr has not been set up?

If there is some limit on the full range of values from the Syntax
InetAddressType you should:
- say so in the Description clause
- say so in the Conformance statement.

That will make the text in pcePcepEntityAddr easier to cope with
unchanged.

---

Just asking...

Is there a value of pcePcepEntityConnectTimer that means "never give
up"?  You could use zero if you wanted to, but you don't have to.
(18 hours is probably long enough.)

---

Similar for pcePcepEntityConnectMaxRetry
Is there a value that means continue indefinitely?
Although 2^32 attempts sounds like quite a few.

The Description for this object says "...before going back to the Idle
state."  Could you reference that back to an object (presumably in the
peerTable since the entry in the sessTable does not exist in idle state)
and the associated value.

---

pcePcepEntityOpenWaitTimer same question about "wait forever".

Also, perhaps clarify that this is the time after the TCP connection has
come up.

I know the protocol spec makes this clear, but "...aborts the session
setup attempt" could be enhanced with "...terminates the TCP connection
and removes the associate entry from the sessTable"

---

            pcePcepEntityDeadTimer is recommended to be 4 times the
            pcePcepEntityKeepAliveTimer value.

I don't think that giving this advice is helpful since this object is
read-only and can only report the configured (through other means)
value.

---

   pcePcepEntityMaxDeadTimer
       DESCRIPTION
           "The maximum value that this PCEP entity will accept from a
            peer for the Dead timer.  Zero means that the PCEP entity
            will allow not running a Dead timer.

            A Dead timer will not be accepted unless it is both greater
            than the session Keepalive timer and less than this field."

Are you sure? Where does a zero Dead timer fit with that statement?

---

pcePcepEntityAllowNegotiation seems out of order in amidst the various
timer objects. Not important, but odd.

---

   pcePcepEntityMinDeadTimer OBJECT-TYPE
       DESCRIPTION
           "In PCEP session parameter negotiation, the minimum value
            that this PCEP entity will accept for the Dead timer.  Zero
            means that the PCEP entity insists on not running a Dead
            timer.

            A Dead timer will not be accepted unless it is both greater
            than the session Keepalive timer and greater than this
            field."

Again, the second paragraph seems to conflict with the use of zero.

---

pcePcepEntitySyncTimer needs to clarify that this object only has
meaning if the entity is a PCE.

So you should give a value that a PCC can return in this object, or
say that a PCC should not return this object.

Furthermore, the syncTimer is only recommended in RFC 5440. How should
an implementation that does not implement a syncTimer return this
object?

---

The backoff timer objects (pcePcepEntityInitBackoffTimer and
pcePcepEntityMaxBackoffTimer) might be better grouped next to the
other session-related timers (especially the OpenTimer).

---

   pcePcepEntityMaxUnknownReqs OBJECT-TYPE
       DESCRIPTION
           "The maximum number of unrecognized requests and replies that
            any session on this PCEP entity is willing to accept per
            minute.

   pcePcepEntityMaxUnknownMsgs OBJECT-TYPE
       DESCRIPTION
           "The maximum number of unknown messages that any session
            on this PCEP entity is willing to accept per minute."

...before doing what?

---

Same issue wrt pcePcepPeerAddrType as for pcePcepEntityAddrType

---

In pcePcepPeerRole it might be more usual to have unknown(0).

---

It is not clear (to me) whether pcePcepPeerNumSessSetupFail is
incremented each time a retry fails or only each time a session
set-up attempt is abandoned.

---

   pcePcepPeerSessionFailTime
       DESCRIPTION
           "The value of sysUpTime the last time a session with this
            peer failed to be established.

This is consistent with other fields, but does not record a session
that was up, but then failed.

---

Now, I'll grant that pcePcepEntityMaxUnknownMsgs allows for a remarkably
high rate of receipt of unknown messages with...
       SYNTAX      Unsigned32
       DESCRIPTION
           "The maximum number of unknown messages that any session
            on this PCEP entity is willing to accept per minute."

But, given that rate, it seems that pcePcepPeerNumUnknownRcvd could
overflow within just one minute.
       SYNTAX      Counter32
       DESCRIPTION
           "The number of unknown messages received from this peer."

The same is going to apply to pcePcepEntityMaxUnknownReqs and
pcePcepPeerNumReqRcvdUnknown.

Also, of course, the same applies to the counters in the sessTable.

---

Many of the same comments apply to the objects in the sessEntry as
applied to the peerEntry and entityEntry, and I won't repeat them here.

---

I don't think pcePcepSessOverloadTime and pcePcepSessPeerOverloadTime
are very useful. The values are presumably stored from sent or received
OVERLOADED-DURATION TLVs in PCNtf messages that indicate 'Overloaded'.

However, if I come and look at the object some time later I have no idea
how much longer the overloaded state may last.

I think you need (I use the peer as an example)

pcePcepSessPeerOverload          TruthValue
  Whether or not this peer is overloaded.
pcePcepSessPeerLastOverloaded    Timestamp
  Time at which last overload event occurred for this peer.
  Not cleared when pcePcepSessPeerOverload becomes false.
pcePcepSessPeerLastOverloadTime  Unsigned32
  Duration of last overload event for this peer.
  Not cleared when pcePcepSessPeerOverload becomes false.

---

Although this is carefully a read-only MIB module, I wonder whether you
need a way to quiesce the Notifications issued by an implementation.

Something like...

   pcePcepNotificationsMaxRate OBJECT-TYPE
      SYNTAX       Unsigned32
      MAX-ACCESS   read-write
      STATUS       current
      DESCRIPTION
           "This variable indicates the maximum number of
            notifications issued per second. If events occur
            more rapidly, the implementation may simply fail to
            emit these notifications during that period, or may
            queue them until an appropriate time. A value of 0
            means no notifications are emitted and all should be
            discarded (i.e., not queued)."

...or...

   pcePcepNotificationsEnable OBJECT-TYPE
      SYNTAX        TruthValue
      MAX-ACCESS    read-write
      STATUS        current
      DESCRIPTION
           "If this object is true, then it enables the
            generation of notifications."

Without one of these, your management station will be bombed with
Notifications from the PCCs in the network.

---

From: Jonathan Hardwick <[email-masked]>
Date: Thu, 14 Aug 2014 10:58:43 +0000

Hi Adrian

Many thanks for your detailed review and comments.  The authors discussed your comments on a conference call on Wednesday.  I have provided our consolidated replies below as >> ... <<.

We will produce a new version of the draft to address your comments.

Thanks again
Jon + authors

-----Original Message-----
From: Pce [mailto:[email-masked]] On Behalf Of Adrian Farrel
Sent: 10 August 2014 12:44
To: [email-masked]
Cc: [email-masked]
Subject: [Pce] AD review of draft-ietf-pce-pcep-mib

<snip>

=====

Section 1 para 2

>> Agree. We will update as necessary. <<

---

Section 1

>> Agree. We will update as necessary. <<

---

>> The scope of the PCEP MIB is currently limited to objects that are core to RFC 5440 only.
-  The capabilities from RFC 5088/5089 belong in the PCE discovery MIB.
-  The OFs in the OF-list TLV deserve their own MIB table, which could be defined in a separate MIB if anyone wants it.
-  There are lots of other PCEP RFCs that we could bring into the scope of this document, but it is already quite large.

Our preference is to clarify the scope of this document, provide an informational reference to the discovery MIB, and allow other documents to augment / supplement the tables we have defined. <<

---

>> Agree. We will update accordingly. <<

---

>> There is one row in this table for each local PCEP entity, of which there may be multiple.

One scenario is partitioning a physical router into multiple virtual routers, each with its own PCC. Another scenario is managing a device which front-ends multiple PCE compute resources, each with a different set of capabilities that are accessed via different IP addresses.

Although there clearly won't be billions, we feel there's no point in placing an arbitrary upper limit on the number of local PCEP entities. Our proposal, therefore, is to remove the artificial restriction on the entity index range. <<

>> A single entity can be both PCC and PCE. <<

>> Each local PCEP entity has its own set of peers.  A given remote PCEP speaker may be a peer of more than one of the local PCEP entities in the pcepEntityTable, so the pcePcepEntityIndex is needed to differentiate the information that is recorded for the remote PCEP speaker by each of the local PCEP entities. <<

   The text in 4.2 says...

>> No, pcePcepEntityIndex is the same for each peer that is a peer of a given local PCEP entity. <<

>> It does - see below. <<

>> Yes, that's the intention.  To be clear, we believe that a PCEP speaker is identified in the network by its IP address.

The rationale is as follows.  If you have a single box that is speaking PCEP using two different IP addresses then that box looks like two distinct peers to the other PCEP speakers in the network.  RFC 5440 gives no way to detect that this is in fact a single box, and it gives no mechanism to prevent you from connecting to that box on both of its IP addresses. <<

>> Yes <<

>> If there are two local PCEP entities connected to the same PCEP speaker then there will be two rows in peerTable (and sessTable) with the same pcePcepPeerAddrType and pcePcepPeerAddr indexes. <<

>> Multiple <<
>> Indexed first by local entity index, then by remote peer IP address <<

---

    PCE1---PCE2   PCE3
      |   /  |    /  |
      |  /   |   /   |
    PCCa/    PCCb   PCCc

>> Agreed, we will include the example in the Appendix. <<

---

  mib-2
  |
  |--- XXX pcePcepMIB
           |

>> Agreed, the extra indirection serves no purpose.  We will update by removing pcePcepEntityObjects, pcePcepPeerObjects and pcePcepSessObjects so that pcePcepEntityTable, pcePcepPeerTable and pcePcepSessTable are grouped directly under pcePcepObjects. <<

---

>> We will provide an example with ASCII illustration. This will be inserted in Section 4 - PCEP MIB Module Architecture. <<

---

>> Agree. We'll include the suggested text and review the other fields to clean up any other instances. <<

---

>> This is an interesting idea.  We would prefer to use SNMP for reporting status and to use other available interfaces to provide more detailed logging / reason strings. <<

---

>> Agree to this. There is a similar case in RFC 6825 that we can base this on. <<

---

Just asking...

>> After discussion the implementers feel they would not want to use a "never give up" option. Same for the similar comments below. <<

---

>> Again no - as above. <<

>> Agree, we will update the text. <<

---

>> Again no - as above. <<

>> Agree, we will update the text. <<

---

>> Agree, no point in stating this. We will update. <<

---

>> Agree that zero is not covered by the final sentence.  We'll delete the final sentence as it adds nothing. <<

---

>> Agree, we will reorder so pcePcepEntityAllowNegotiation appears before pcePcepEntityMaxKeepAliveTimer. <<

---

>> Agree that zero is not covered by the final sentence.  We'll delete the final sentence as it adds nothing. <<

---

>> Agree <<

>> We will extend the allowed range for this field to include zero, and state that zero must be returned if and only if the entity does not use the syncTimer. <<

---

>> Agree, we will update accordingly <<

---

...before doing what?

>> Before terminating the session. We will clarify the text. <<

---

>> Agree. We will update accordingly <<

---

>> Agree. We will update accordingly <<

---

>> It's the former - each time a retry fails. We'll clarify this. <<

---

>> Agree, this is a gap.  We will add a new object to cover this case, recording the last time a session failed from active. <<

---

>> We prefer no change here.  Regardless of whether we impose a limit on MaxUnknownMsgs etc. we don't think there is a realistic prospect that these counters will wrap on a timescale of less than months.  Although configuring 2^32 for MaxUnknownMsgs is clearly not meaningful, we don't think it's important in practice to limit it to some arbitrary number. <<

---

>> OK <<

---

>> Actually, pcePcepSessOverloadTime and pcePcepSessPeerOverloadTime are both supposed to give the remaining interval of time until the overload clears.  The MIB does not currently report the original value that was received in the TLV.  We will make the meaning of these fields clearer.  We feel it is not important to report the original value from the TLV, but please let us know if you disagree. <<

---

Something like...

...or...

>> Agreed. We will go for the max-rate option, for consistency with RFC 6825. <<

_______________________________________________
Pce mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/pce

---

From: "Adrian Farrel" <[email-masked]>
Date: Wed, 20 Aug 2014 10:42:05 +0100

Jon,

Thanks for the work.

Very few responses from me.

> >> The scope of the PCEP MIB is currently limited to objects that are core to
RFC
> 5440 only.
> -  The OFs in the OF-list TLV deserve their own MIB table, which could be
defined
> in a separate MIB if anyone wants it.
> -  There are lots of other PCEP RFCs that we could bring into the scope of
this
> document, but it is already quite large.
> 
> Our preference is to clarify the scope of this document, provide an
informational
> reference to the discovery MIB, and allow other documents to augment /
> supplement the tables we have defined. <<

OK. Sure.
So make sure you have the cross-pointer to the disco MIB module, and think about
how future modules might be able to extend/augment without high price.
For example, a TC that is the bit flags for capability is easily extended (if it
is in a separate module).

> >> There is one row in this table for each local PCEP entity, of which there
may be
> multiple.
> 
> One scenario is partitioning a physical router into multiple virtual routers,
each
> with its own PCC. Another scenario is managing a device which front-ends
> multiple PCE compute resources, each with a different set of capabilities that
are
> accessed via different IP addresses.
> 
> Although there clearly won't be billions, we feel there's no point in placing
an
> arbitrary upper limit on the number of local PCEP entities. Our proposal,
> therefore, is to remove the artificial restriction on the entity index range.
<<

OK, I have a bit of a hope that you are doing this because you have a need, not
because it looks like a cool idea! If your text above is just you scraping
around for reasons that might justify the structure you have documented, well,
then you know what you should do :-)

I am trying to compare the case of six physically diverse nodes each with their
own instance of the MIB module, and one physical node hosting 6 virtual
pcepEntities each as a separate entry in the pcepEntityTable.

In the first case, SNMP would use a different IP address to Get each entry in
each pcepEntityTable.

In the second case you are suggesting, I think, that there is one SNMP agent on
the physical node that maintains one copy of the pcepEntityTable, but that each
entry in the table is a separate virtual node. This, I suppose, saves you from
having to run a separate SNMP agent on each virtual node even though those
virtual nodes are separately addressable.

I have no experience of building virtual nodes in this way. I went back to 4750
to see how this is modelled in OSPF and found that it looks to me that in that
case you would run a separate agent for each router instance (of course how you
manage the code and executables -- and even the data -- is up to you, but the
structure of the MIB module is for a single agent per router instance). That is
clearly shown by the fact that ospfRouterId is a member of ospfGeneralGroup
(what you might call a global variable).

Because I have no experience in this, I am not going to require you to change
this structure, but I will ask you to explain how "One scenario is partitioning
a physical router into multiple virtual routers, each
with its own PCC" integrates with 4750.

Bottom line: if you intend that multiple local PC* instantiations can appear in
the same table, please make this a bit clearer in the text.

Anyway, I still think that Unsigned32 (1..2147483647) may be generous.

> Actually, I'm a bit confused about the indexing of the three tables.

OK, we can snip here. Given your use of the pcepEntityTable, the remaining
indexing makes sense. You will, however, need to add text to explain it.

Cheerio,
Adrian

---

From: Jonathan Hardwick <[email-masked]>
Date: Thu, 28 Aug 2014 19:02:27 +0000

Hi Adrian

I owe you a reply on the requirement for the entity table - my apologies for the delay.

The entity table was a feature of the PCEP MIB before I took over editorship of it, so I will defer to the original authors for their motivation in including it.  However, speaking purely for my own implementation, we do need it for the reasons I gave below and it is used in practice.

I don't think this MIB structure is inconsistent with RFC 4750 as it does not prevent an implementation from having one SNMP subagent per routing instance, as RFC 4750 would seem to require.  (I am not sure I understand the RFC 4750 model though.  As it stands, wouldn't you need one subagent per PE/CE routing instance, or per network layer routing instance in a multi-layer router?  Is that really the normal case?)

I'm currently in the process of clarifying the text regarding the entity table and will get back to you on that.

Best regards
Jon

-----Original Message-----
From: Adrian Farrel [mailto:[email-masked]] 
Sent: 20 August 2014 10:42
To: Jonathan Hardwick
Cc: [email-masked]; [email-masked]
Subject: RE: [Pce] AD review of draft-ietf-pce-pcep-mib

Jon,

Thanks for the work.

RFC
defined
this
informational

OK. Sure.

may be
each
are
an
<<

Cheerio,
Adrian