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 protected]] On Behalf Of Adrian Farrel Sent: 10 August 2014 12:44 To: [email protected] Cc: [email protected] Subject: [Pce] AD review of draft-ietf-pce-pcep-mib <snip> ===== 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. >> Agree. We will update as necessary. << --- 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. >> Agree. We will update as necessary. << --- 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. >> 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. << --- 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". >> Agree. We will update accordingly. << --- 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! >> 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. << 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. >> A single entity can be both PCC and PCE. << 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. >> 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... 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. >> No, pcePcepEntityIndex is the same for each peer that is a peer of a given >> local PCEP entity. << Shouldn't the peerTable have its own unique index for each peer? >> It does - see below. << 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. >> 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. << OTOH, if a peer has multiple addresses, will this allow you to have multiple sessions to the same peer. Is that what you intended? >> Yes << Alternatively, it seems unlikely that two different peers will have the same address. So why is any additional index (i.e., pcePcepEntityIndex) needed? >> 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. << 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. >> Multiple << - Decide whether entries in peerTable and sessTable should be indexed from entityTable, or with an index from peerTable. >> Indexed first by local entity index, then by remote peer IP address << --- 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. >> Agreed, we will include the example in the Appendix. << --- 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 >> 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. << --- 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. >> We will provide an example with ASCII illustration. This will be inserted in >> Section 4 - PCEP MIB Module Architecture. << --- 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." >> Agree. We'll include the suggested text and review the other fields to clean >> up any other instances. << --- 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? >> 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. << --- 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. >> Agree to this. There is a similar case in RFC 6825 that we can base this on. >> << --- 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.) >> After discussion the implementers feel they would not want to use a "never >> give up" option. Same for the similar comments below. << --- Similar for pcePcepEntityConnectMaxRetry Is there a value that means continue indefinitely? Although 2^32 attempts sounds like quite a few. >> Again no - as above. << 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. >> Agree, we will update the text. << --- pcePcepEntityOpenWaitTimer same question about "wait forever". >> Again no - as above. << 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" >> Agree, we will update the text. << --- 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. >> Agree, no point in stating this. We will update. << --- 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? >> Agree that zero is not covered by the final sentence. We'll delete the >> final sentence as it adds nothing. << --- pcePcepEntityAllowNegotiation seems out of order in amidst the various timer objects. Not important, but odd. >> Agree, we will reorder so pcePcepEntityAllowNegotiation appears before >> pcePcepEntityMaxKeepAliveTimer. << --- 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. >> Agree that zero is not covered by the final sentence. We'll delete the >> final sentence as it adds nothing. << --- pcePcepEntitySyncTimer needs to clarify that this object only has meaning if the entity is a PCE. >> Agree << 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? >> 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. << --- The backoff timer objects (pcePcepEntityInitBackoffTimer and pcePcepEntityMaxBackoffTimer) might be better grouped next to the other session-related timers (especially the OpenTimer). >> Agree, we will update accordingly << --- 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? >> Before terminating the session. We will clarify the text. << --- Same issue wrt pcePcepPeerAddrType as for pcePcepEntityAddrType >> Agree. We will update accordingly << --- In pcePcepPeerRole it might be more usual to have unknown(0). >> Agree. We will update accordingly << --- 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. >> It's the former - each time a retry fails. We'll clarify this. << --- 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. >> Agree, this is a gap. We will add a new object to cover this case, >> recording the last time a session failed from active. << --- 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. >> 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. << --- 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. >> OK << --- 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. >> 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. << --- 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. >> Agreed. We will go for the max-rate option, for consistency with RFC 6825. << _______________________________________________ Pce mailing list [email protected] https://www.ietf.org/mailman/listinfo/pce _______________________________________________ Pce mailing list [email protected] https://www.ietf.org/mailman/listinfo/pce
