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

["Rakesh Gandhi (rgandhi)" <[e-mail]>]

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