Re: [Gen-art] Gen-ART Review of draft-ietf-mpls-ldp-capabilities-03

Just to let people know, all of my comments from 02 have been addressed in 
the 03 revision. Ready for publication as a Proposed Standard.

Thanks,

Spencer

>I have been selected as the General Area Review Team (Gen-ART)
> reviewer for this draft (for background on Gen-ART, please see
> http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).
>
> Please resolve these comments along with any other Last Call comments
> you may receive.
>
> Document: draft-ietf-mpls-ldp-capabilities-02
> Reviewer: Spencer Dawkins
> Review Date: 2008-04-30
> IETF LC End Date: 2008-04-25 (yeah, I'm late)
> IESG Telechat date: (if known)
>
> Summary:
>
> This draft is almost ready for publication as a Proposed Standard.
>
> Comments:
>
> Almost all my "review comments" below are about 2119 language - this is a
> very SHOULD-y draft. I'd especially call Russ's attention to my comment
> about multiple occurrences of the same capability in one message, in 
> Section
> 7.
>
> idnits 2.08.08 reports "no issues", FWIW.
>
> Abstract
>
>   A number of enhancements to the Label Distribution Protocol (LDP)
>   have been proposed.  Some have been implemented, and some are
>   advancing toward standardization.  It is likely that additional
>   enhancements will be proposed in the future.  At present LDP has no
>   guidelines for advertising such enhancements at LDP session
>   initialization time.  There is also no mechanism to enable and
>   disable enhancements after the session is established.  This document
>   provides guidelines for advertising LDP enhancements at session
>   initialization time.  It also defines a mechanism to enable and
>   disable enhancements after LDP session establishment.
>
> Spencer (clarity): here and in the Introduction, the draft says 
> "guidelines
> at session initialization time, mechanism to enable/disable after session
> establishment". Aren't both of these mechanisms? s/provides
> guidelines/defines a mechanism/?
>
> 1. Introduction
>
>   A number of enhancements to LDP as specified in [RFC5036] have been
>   proposed.  These include LDP Graceful Restart [RFC3478], Fault
>   Tolerant LDP [RFC3479], multicast extensions [MLDP], signaling for
>   layer 2 circuits [RFC4447], a method for learning labels advertised
>   by next next hop routers in support of fast reroute node protection
>   [NNHOP], upstream label allocation [UPSTREAM_LDP], and extensions for
>   signaling inter-area LSPs [IALDP].  Some have been implemented, and
>   some are advancing toward standardization.  It is likely that
>   additional enhancements will be proposed in the future.
>
> Spencer (clarity): I think I understand what all the words mean, but 
> should
> this be "additional enhancements will be implemented", or even "deployed"?
>
>   At present LDP has no guidelines for advertising such enhancements at
>   LDP session initialization time.  There is also no mechanism to
>   enable and disable enhancements after the session is established.
>
>   This document provides guidelines for advertising LDP enhancements at
>   session initialization time.  It also defines a mechanism to enable
>   and disable enhancements after LDP session establishment.
>
>   LDP capability advertisement provides means for an LDP speaker to
>   announce what it can receive and process.  It also provides means for
>   a speaker to inform peers of deviations from behavior specified by
>
> Spencer (clarity): are "a speaker" and "an LDP speaker" interchangeable in
> this specification? I'm assuming so...
>
>   [RFC5036].  An example of such a deviation is LDP graceful restart
>   where a speaker retains MPLS forwarding state for LDP-signaled LSPs
>   when its LDP control plane goes down.  It is important to point out
>   that not all LDP enhancements require capability advertisement.  For
>   example, upstream label allocation does but inbound label filtering,
>   where a speaker installs forwarding state for only certain FECs, does
>   not.
>
> 3. The LDP Capability Mechanism
>
>   The LDP capability advertisement mechanism operates as follows:
>
>     - Each LDP speaker is assumed to implement a set of enhancements
>       each of which has an associated capability.  At any time a
>       speaker may have none, one or more of those enhancements
>       "enabled".  When an enhancement is enabled the speaker advertises
>       the associated capability to its peers.  By advertising the
>       capability to a peer the speaker asserts that it shall perform
>       the protocol actions specified for the associated enhancement.
>       For example, the actions may involve receiving and processing
>       messages from a peer that the enhancement requires.  Unless the
>
> Spencer (clarity): I got a bit lost here. Suggest "For example, the
> enhancement may require the LDP speaker to receive and process
> enhancement-specific messages from its peer" - is this still correct?
>
>       capability has been advertised the speaker will not perform
>       protocol actions specified for the corresponding enhancement.
>
>     - Includes an IANA considerations section that notes that an IANA-
>       assigned code point for the optional parameter corresponding to
>       the enhancement is required.
>
> Spencer (review comment): Is this saying "Includes an IANA Considerations
> section that requests assignment of a code point for the optional 
> parameter
> corresponding to the enhancement"?
>
> 4. Specifying Capabilities in LDP Messages
>
>   The format of a TLV that is a Capability Parameter is:
>
> Spencer (clarity): "The format of a Capability Parameter TLV is:"?
>
>       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
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>      |U|F| TLV Code Point            |            Length             |
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>      |S| Reserved    |                                               |
>      +-+-+-+-+-+-+-+-+       Capability Data                         |
>      |                                               +-+-+-+-+-+-+-+-+
>      |                                               |
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>   where:
>     U-bit
>       SHOULD be 1 (ignore if not understood).
>
> Spencer (review comment): I'm fairly comfortable with this being a SHOULD,
> but would be more comfortable with text that explains why the U-bit would 
> be
> set to zero ("unless the LDP speaker is unwilling to continue with a peer
> that does not understand the enhancement"?)
>
>     F-bit:
>       SHOULD be 0 (don't forward if not understood).
>
> Spencer (review comment): I'm confused by why this is a SHOULD... If the
> point is that
>
> - I can ask you to do something you don't understand, so you ignore it, 
> but
>
> - you wouldn't forward the capability to a third party who might think you
> DO understand it, and start using the enhancement you don't understand
>
> ... I'm having a hard time understanding why this isn't a MUST. If I 
> missed
> the point, my apologies.
>
>   To ensure backward compatibility with existing implementations the
>   following TLVs play the role of a Capability Parameter when included
>
> Spencer (clarity): was this section written when more than one type of TLV
> played the role of a Capability Parameter? the phrasing is oddly plural.
>
>   in Initialization messages:
>
>       - FT Session TLV [RFC3479]
>
>   This document refers to such TLVs as Backward Compatibility TLVs.
>
> 7. Procedures for Capability Parameters in Initialization Messages
>
>   An LDP speaker SHOULD NOT include more than one instance of a
>   Capability Parameter with the same type and value in an
>   Initialization message.  Note, however, that processing multiple
>   instances of such a parameter does not require special handling, as
>   additional instances do not change the meaning of an announced
>   capability.
>
> Spencer (review comment): I'm concerned that this is SHOULD NOT, because 
> (in
> my understanding) an implementation must still be prepared to accept
> messages that violate a SHOULD, but the document doesn't provide enough
> detail to process such messages in an interoperable way.
>
> "does not require special handling" seems to handwave some difficult
> problems - if I advertise a capability and withdraw it in the same 
> message,
> the order of evaluation isn't specified, for instance - so is the result
> that the capability is advertised, or withdrawn? If you really don't want 
> to
> specify this level of detail - and please don't - just make this MUST NOT
> (and, for extra credit, specify the error handling for a message that
> violates the MUST NOT).
>
>   These procedures enable an LDP speaker A that advertises a specific
>   LDP capability C to establish an LDP session with speaker B that does
>   not advertise C.  In this situation whether or not capability C may
>   be used for the session depends on the semantics of the enhancement
>   associated with C.  If the semantics do not require both A and B
>   advertise C to one another then B could use it; that is, A's
>   advertisement of C permits B to send messages to A used by the
>   enhancement.
>
> Spencer (clarity): call me thick. This text would be clearer if you s/A/X/
> and s/B/Y/, because using A and B for speakers and C for a capability 
> misled
> me into thinking that this paragraph was about transitive support, when A
> understands a capability, B does not, and SPEAKER C also understands it. 
> The
> paragraph is correct as written - it's just easy to misread.
>
>   It is the responsibility of the capability designer to specify the
>   behavior of an LDP speaker that has enabled a certain enhancement,
>   advertised its capability and determines that its peer has not
>   advertised the corresponding capability.  The document specifying
>   procedures for the capability MUST describe the behavior in this
>   situation.  If the specified procedure is to terminate the session
>   the LDP speaker SHOULD send a Notification message to the peer before
>   terminating the session.  The Status Code in the Status TLV of the
>   Notification message SHOULD be Unsupported Capability, and the
>
> Spencer (review comment): why SHOULD and not MUST? I'm not asking for 
> MUST,
> just a better understanding of why this wouldn't happen.
>
>   message SHOULD contain the unsupported capabilities (see Section 9
>   for more details).  In this case the session SHOULD NOT be re-
>   established automatically.  How the session is re-established is
>   beyond the scope of this document.
>
> Spencer (review comment): I'm not sure why the session SHOULD NOT be
> re-established, but given the following sentence, I'd suggest striking the
> whole point - as out of scope.
>
>   It depends on the LDP capability
>   and MUST be specified along with the procedures specifying the
>   capability.
>
>   An LDP speaker that supports capability advertisement and includes a
>   Capability Parameter in its Initialization message SHOULD set the TLV
>   U bit to 1.  This ensures that an [RFC5036] compliant peer that does
>   not support the capability mechanism will ignore the Capability
>   Parameter and allow the session to be established.
>
> Spencer (review comment): this may be the same comment as previously (on
> SHOULD for setting the U-bit to 1), but it seems like you expect that LDP
> speakers would want to allow the session to be established whether or not
> LDP Capability extension is supported or not - can you think of a scenario
> where this wouldn't be true? If not, I'd suggest MUST, or at least
> explaining the SHOULD a bit.
>
> 9. Extensions to Error Handling
>
>   This document defines a new LDP status code named Unsupported
>   Capability.  The E bit of the Status TLV carried in a Notification
>   message that includes this status code SHOULD be set to 0.
>
> Spencer (review comment): why not MUST? Or alternatively, what does the
> receiver do differently depending on whether the E-bit is 0, or 1?
>
>   When the Status Code in a Notification Message is Unknown TLV the
>   message SHOULD specify the TLV that was unknown.  When the
>
> Spencer (review comment): so, if the message does not specify the TLV that
> was unknown, what does the LDP speaker do then? I'm not sure why this 
> isn't
> a MUST.
>
>   Notification message specifies the TLV that was unknown it MUST
>   include the unknown TLV in a Returned TLVs TLV.
>
> 11. Backward Compatibility
>
>   Section 3 identifies a set of Backward Compatibility TLVs that may
>
> Spencer (clarity): again, it's a "set" of one TLV...
>
>   appear in Initialization messages in the role of a Capability
>   Parameter.  This permits existing LDP enhancements that use an ad hoc
>   mechanism for enabling capabilities at sesssion initialization time
>   to continue to do so.
>
>
> _______________________________________________
> Gen-art mailing list
> Gen-art@ietf.org
> https://www.ietf.org/mailman/listinfo/gen-art
> 