Re: [Pce] Shepherd's Review of draft-ietf-pce-association-bidir-08

[Dhruv Dhody <[e-mail]>]

Hi Rakesh,

Thanks for making the changes and handling my comments. I plan to hit the
button to send it out of the WG tomorrow Friday EoD (for me), giving some
time to the WG to look over the recent changes.

Thanks!
Dhruv

On Thu, Dec 17, 2020 at 7:27 AM Rakesh Gandhi (rgandhi) <[e-mail]>
wrote:

> Thank you Dhruv for the detailed review and comments. Latest revision is
> uploaded that addresses your review comments:
>
>
>
> Htmlized:
> https://tools.ietf.org/html/draft-ietf-pce-association-bidir-09
> Diff:
> https://www.ietf.org/rfcdiff?url2=draft-ietf-pce-association-bidir-09
>
>
>
> Please see replies inline with <RG>…
>
>
>
> *From: *Dhruv Dhody <[e-mail]>
> *Date: *Thursday, October 22, 2020 at 7:45 AM
> *To: *[e-mail] <
> [e-mail]>
> *Cc: *[e-mail] <[e-mail]>
> *Subject: *Shepherd's Review of draft-ietf-pce-association-bidir-08
>
> Hi Authors,
>
> Please find my review of your I-D. The core idea is clear but I feel
> the document needs some cleanup and tightening of specifications at
> par standards track document.
>
> ==
>
> Major
> ******
> (1) We need to use normative text in this standards track document.
> Section 3 and section 5 could benefit from it. To illustrate my point
> -
> ~ Section 3.1, the use of 'may' in -
>
>    The originating endpoint (PCC) node may report/ delegate the forward
>    and reverse direction LSPs to a PCE.  The remote endpoint (PCC) node
>    may report its forward direction LSP to a PCE.
>
>    - Similar text in section 3.2
>
> ~ There is only 1 normative MUST in section 5.1 and 5.2.
>
>
>
> <RG> Added normative language in Section 5. For Section 3 – Overview, as
> it is using the same style as RFC 7551, prefer to keep it this way. RFC
> 7551 describes the model similar way. The procedure and extensions in
> Section 5 are updated using the normative language.
>
>
> ~ My overall suggestion: Since you have multiple scenarios
> (single-sided & double-sided) and (PCE-Initiated & PCC-Initiated) and
> each requiring different processing, the current text ends up being
> generic (and thus not possible to use Normative). It is better to
> break the test for the 4 cases (or sub-sections) -
>   ~ single-sided PCE-Initiated
>   ~ single-sided PCC-Initiated
>   ~ double-sided PCE-Initiated
>   ~ double-sided PCC-Initiated
> and use the normative language for message exchange (as well as the
> use of ASSOCIATION) for each case separately. That should improve the
> document clarity for interoperability.
>
>
>
> <RG> Created four sub-sections in Section 3 and moved the relevant text
> there.
>
> (2) Section 4,
>
>    o  An LSP (forward or reverse) cannot be part of more than one
>       Bidirectional LSP Association Group.  More than one forward LSP
>       and/ or reverse LSP can be part of a Bidirectional LSP Association
>       Group.
>
>    ~ Rewrite to use normative text!
>
>
>
> <RG> Fixed.
>
>
>    ~ Are you sure about the 2nd sentence? Isn't this association
> between only 2 LSPs - one forward, one backward? In my reading, RFC
> 7551 is about 2 LSPs. If not, the use-case and handling need to be
> explained.
>
> <RG> Changed to SHOULD NOT, to leave some flexibility (e.g. make before
> break).
>
>
>    o  The Tunnel (as defined in [RFC3209]) of forward and reverse LSPs
>       of the Single-sided Bidirectional LSP Association on the
>       originating node MUST be the same.
>
>    ~ Do you mean the Tunnel ID (as carried in LSP-IDENTIFIERS TLV
> [RFC8231])? I ask because the source and destination would be opposite
> for the forward and reverse LSPs in the LSP-IDENTIFIERS TLV? If yes,
> then also add a restriction on the endpoint mismatch.
>
> <RG> Added Error code for endpoint mismatch.
>
>
> (3) We should be clear that existing forward or reverse LSP can be
> bi-directionally associated. In this document, the focus seems to be
> on initiation only. You have some text in section 5.1/5.2 but it would
> be good to make it prominent something like Section-3.2 of RFC 7551.
>
>
>
> <RG> Added in Section 5.
>
>
> (4) Include text for exchanging association types in the open message
>
>    [RFC8697] specifies the mechanism for the capability advertisement of
>    the Association types supported by a PCEP speaker by defining an
>    ASSOC-Type-List TLV to be carried within an OPEN object.  This
>    capability exchange for the Bidirectional LSP Association Types MUST be
>    done before using the Bidirectional LSP Association.  Thus, the PCEP
>    speaker MUST include the Bidirectional LSP Association Types in the
>    ASSOC-Type-List TLV and MUST receive the same from the PCEP peer
>    before using the Bidirectional LSP Association in PCEP messages.
>
>
>
> <RG> Added.
>
> (5) Error Reporting that needs to be added -
>
>     ~ Bidirectional LSP Association types are not supported
>     ~ Both LSPs marked as forward
>     ~ Both LSPs marked as reverse
>     ~ One direction LSP says co-routed, another direction non-co-routed
>
>
>
> <RG> Added these in Error handling.
>
>
>     ~ More than 2 LSPs in the group (if you agree with the comment in (2))
>
>
>
> <RG> As replied in (2).
>
>
> (6) For single-sided Bidirectional LSP, is the association information
> configured on D? We claim this to be an operator-configured
> association and thus want to make sure that is the case here!
>
>
>
> <RG> Added text in Section 4.1 for dynamic association on node D.
>
> =
>
> Minor
> ******
> - Global comment: Change RSVP to RSVP-TE in this I-D.
>
>
>
> <RG> Fixed.
>
>
> - Section 1:
>
>    The MPLS Transport Profile (MPLS-TP) requirements document [RFC5654]
>    specifies that MPLS-TP MUST support associated bidirectional point-
>    to-point LSPs.  [RFC7551] defines RSVP signaling extensions for
>    binding two reverse unidirectional LSPs [RFC3209] into an associated
>    bidirectional LSP.  The fast reroute (FRR) procedures for associated
>    bidirectional LSPs are described in [RFC8537].
>
> ~ The use of normative MUST in the first sentence is incorrect.
> ~ Reference to RFC 3209 seems out of place as that term is not used in the
> RFC
>
>
>
> <RG> Updated text.
>
> - Section 1.1: This summary feels out of place as we are yet to talk
> about the single-sided and the double-sided bidirectional LSPs. You
> can think about placing it after section 3.3. I was also wondering if
> it makes sense to break the starting sentence (in the first 3 items)
> into two, one for single-sided and another for double-sided. Also, the
> last item about co-routed is mentioned in the context of stateless PCE
> only - why?
>
> <RG> Moved and updated the text.
>
>
> - At the end of section 1, you should mention that this document
> defines two association types (and corresponding association group)
> and a Bidirectional LSP Association Group TLV with forward-references.
> So that when they are mentioned in section 3 it is not out of the
> blue.
>
>
>
> <RG> Added.
>
> - Section 3.1, Since we expect both endpoints to be PCC; better to
> break it into 2 sentences.
>   OLD:
>    As specified in [RFC7551], in the single-sided case, the
>    bidirectional tunnel is provisioned only on one endpoint node (PCC)
>    of the tunnel.
>   NEW:
>    As specified in [RFC7551], in the single-sided case, the
>    bidirectional tunnel is provisioned only on one endpoint node
>    of the tunnel. Both endpoints act as a PCC.
>   END
>
> <RG> Fixed.
>
>
> - Section 3.1, do you mean PathTear below?
>
>
>
>    Similarly, the remote endpoint node deletes the
>    reverse LSP when it receives the RSVP Path delete message [RFC3209]
>    for the forward LSP.
>
>    ~ It might be better to just refer to the RSVP-TE RFCs without
> mentioning the message as the state could also be cleared in case of
> other messages like PathErr.
>
>
>
> <RG> Fixed.
>
> - Section 3, for the figures, please add a legend so that the readers
> understand what does F,R means? Maybe also the '0' as PLSP-ID=0.
>
>
>
> <RG> Added.
>
> - Section 4.2, cleaning up the text and adding MUST
> OLD:
>    The Bidirectional LSP Association Group TLV is defined for use with
>    the Single-sided and Double-sided Bidirectional LSP Association Group
>    Object Types.
> NEW:
>    The Bidirectional LSP Association Group TLV an OPTIONAL TLV for use
> with the
>    Bidirectional LSP Associations (ASSOCIATION object with Association
>    type = TBD1 for Single-sided or TBD2 for Double-sided).
> END
>
> <RG> Fixed.
>
>
> - Section 4.2, Reserved should be called Flags field. You should say
> unassigned flags MUST be set to zero and ignored on receipt. You would
> then also align this section with the IANA section.
>
>
>
> <RG> Fixed.
>
> - Section 5.3, add a reference to RFC 8697. Do we need some mechanism
> to tell computation failure, especially for the co-routed case?
>
>
>
> <RG> Added reference. I think existing path failure mechanism should be
> suffice given path could fail for many reasons.
>
>
> =
>
> Nits
> ******
> - Expand PCEP in Title to Path Computation Element Communication Protocol
>
>
>
> <RG> Fixed.
>
>
> - Section 1: s/Path Computation Element Protocol (PCEP)/Path
> Computation Element Communication Protocol (PCEP)/
>
>
>
> <RG> Fixed.
>
>
> - Section 1: To align with RFC 8697
>   OLD:
>    [RFC8697] introduces a generic mechanism to create a grouping of LSPs
>    which can then be used to define associations between a set of LSPs
>    and/or a set of attributes, for example primary and secondary LSP
>    associations, and is equally applicable to the active and passive
>    modes of a Stateful PCE [RFC8231] or a stateless PCE [RFC5440].
>   NEW:
>    [RFC8697] introduces a generic mechanism to create a grouping of LSPs.
>    This grouping can then be used to define associations between
>    sets of LSPs or between a set of LSPs and a set of attributes,
>    and it is equally applicable to the stateful PCE (active and
>    passive modes) and the stateless PCE.
>   END
>
> <RG> Fixed.
>
>
> - Section 3:
>
>    As shown in Figure 1, two reverse unidirectional LSPs can be grouped
>    to form an associated bidirectional LSP.
>
> ~ Shouldn't this be 'forward and reverse' instead of 'two reverse'?
>
>
>
> <RG> Fixed. RFC 7551 uses such sentence.
>
> - Section 4: As per changes made in other association draft by the
> RFC-Editor
> OLD:
>    This document defines two new Association Types for the ASSOCIATION
>    Object (Object-Class value 40) as follows:
>
>    o  Association Type (TBD1) = Single-sided Bidirectional LSP
>       Association Group
>
>    o  Association Type (TBD2) = Double-sided Bidirectional LSP
>       Association Group
> NEW:
>    This document defines two new Association types, called
>    "Single-sided Bidirectional LSP" (TBD1) and "Double-sided
>    Bidirectional LSP" (TBD2), based on the generic ASSOCIATION
>    object.
> END
>
> <RG> Fixed.
>
>
> - Section 4.2
>   OLD:
>    o  For co-routed LSPs, this TLV MUST be present.
>
>    o  For reverse LSPs, this TLV MUST be present.
>   NEW:
>    o  For co-routed LSPs, this TLV MUST be present and C flag set.
>
>    o  For reverse LSPs, this TLV MUST be present and R flag set.
>   END
>
> <RG> Fixed.
>
>
> - Section 5.7, can the same error handling for PCC and PCE be changed
> to use terms PCEP speaker and PCEP peer instead?
>    ~ Applicable to other errors as well
>
>
>
> <RG> Fixed.
>
> - Section 9.1
>   ~s/defined [RFC8697]/defined in [RFC8697]/
>
>
>
> <RG> Fixed.
>
>
>   OLD:
>    This document adds new Association Types for the ASSOCIATION Object
>    (Object-class value 40) defined [RFC8697].  IANA is requested to make
>    the assignment of values for the sub-registry "ASSOCIATION Type"
>    [RFC8697], as follows:
>   NEW:
>    This document defines two new Association types, originally described in
>    [RFC8697].  IANA is requested to assigned the following new values in
> the
>    "ASSOCIATION Type Field" subregistry [RFC8697] within the "Path
>    Computation Element Protocol (PCEP) Numbers" registry:
>   END
>
>
>
> <RG> Fixed.
>
>
>   ~ Remove 'group' from the name.
>
>
>
> <RG> Fixed.
>
> - Section 9.2.1, add 0-28 as Unassigned
>
>
>
> <RG> Fixed.
>
> - Acknowledgments: You are thanking me twice :)
>
> <RG> And now adding one more time😊
>
>
>
> Thanks,
>
> Rakesh
>
>
>
>
> ==
>
> Thanks!
> Dhruv
>