Subject: [Pce] Shepherd's Review of draft-ietf-pce-pcep-extension-for-pce-controller-07

From: <[email-masked]>
Date: Wed, 21 Oct 2020 16:50:50 +0200

Hi authors of draft-ietf-pce-pcep-extension-for-pce-controller,

Please find below my review of the aforementioned I-D.

Regards,

Julien

---

*Main Concerns*

- From a PCECC operation stand point, it feels odd that what is referred
to as "basic PCECC LSP setup" is actually PCC triggered, whereas the
PCE-initiatied option does exist as well. Even though it adds an
additional PCInitiate message, I would suggest to define the latter as
the "basic setup", which would not only address that comment, but would
also mitigate the following concerns on PCC-triggered setup.
- I have 2 issues with PCC-triggered setup:
  * it adds LSP characteristics and egress-related information in the
ingress PCC, which is not straightforward in a PCECC context (certainly
not for a "base setup");
  * the behavior triggered by the initial PCRpt is
implementation-specific in the PCE: as per section 5.8.3 of RFC 8231, I
could configure my PCE to just store the reported info, or make it wait
for whatever event before actually starting something ("PCE *decides* to
update the LSP", "the PCE *can* modify LSP parameters of delegated LSPs").
  => Assuming the PCC-triggerred option is really needed, this
limitation should be more explicit.
- At the end of section 6.1, some errors are defined with respect to the
number of CCI objects in a message. If the ingress PCE has a particular
role (e.g. PLSP-ID allocation), how do other PCCs know if they are
transit or egress to be able to check the number of CCIs they get?
AFAIU, at 1st setup, only inconsistencies between the O bit and the
number of CCIs can trigger an error; on LSP updates the node may add an
additional check with respect to its role before the update: those
situations need to be clarified.
- There is no reference to RFC 8741. Is it applicable? If so, then how?

*Nits*
------
Global
---
- Consistency on capitalization of ingress/transit/egress to be corrected.
------
Abstract
---
- s/PCE-based central controller (PCECC)/PCE-based Central Controller
(PCECC)/
- s/"calculated/setup"/"calculated/set up"/
- s/each network devices/each network device/
------
Section 1.
---
- s/offload path computation function/offload the path computation function/
- s/PCE-based central controller (PCECC)/PCE-based Central Controller
(PCECC)/
- s/such a way that, extensions/such a way that extensions/
- s/extensions for other use cases is/extensions for other use cases are/
------
Section 4.
---
- s/PCE speaker supporting/A PCE speaker supporting/
- s/PCE speaker needs/A PCE speaker needs/
- s/needs the means/needs means/
- s/PCECC based LSP/PCECC-based LSPs/
- s/PCECC based label allocations/PCECC-based label allocations/
- s/provide a means/provide a mean/  [x2]
- s/between PCE and PCC/between the PCE and the PCC/
------
Sections 5. & 5.1.
---
- s/PCE as the Central Controller/PCE as a Central Controller/
- s/reuses existing Active stateful PCE mechanism/reuses the existing
active stateful PCE mechanism/
- s/control the LSP/control LSPs/
------
Section 5.4.
---
- s/the PCEP speaker MUST send a PCErr/the receiving PCEP speaker MUST
send a PCErr/
- OLD
   [...] If the PCEP
   Speakers support the extensions of this draft but did not advertise
   this capability attempts a PCECC operation then a PCErr message with
   Error-Type=19(Invalid Operation) and Error-Value=TBD3 (Attempted
   PCECC operations when PCECC capability was not advertised) will be
   generated and the PCEP session will be terminated.
  NEW
   [...] If a PCEP speaker
   which supports the extensions of this draft but did not advertise
   this capability attempts a PCECC operation, then a PCErr message with
   Error-Type=19 (Invalid Operation) and Error-Value=TBD3 (Attempted
   PCECC operations when PCECC capability was not advertised) MUST be
   generated by its peer and the PCEP session will be terminated.

- s/PCECC-CAPABILITY sub-TLV/the PCECC-CAPABILITY sub-TLV/  [x3]
- s/STATEFUL-PCE-CAPABILITY TLV/the STATEFUL-PCE-CAPABILITY TLV/  [x2]
- s/in OPEN Object/in the OPEN Object/
- s/it MUST send/the receiving peer MUST send/
- s/and I flag/and the I flag/
------
Section 5.5.
---
- s/pertaining to PCECC/pertaining to a PCECC/
- s/the PCECC LSP is intended/that PCECC LSP is intended/
------
Section 5.5.1.
---
- s/Basic PCECC LSP set up/Basic PCECC LSP Setup/
- s/included for PCECC LSP/included for PCECC LSPs/
- s/"so the transit LSR could"/"so a transit/egress LSR could"/
- s/include SPEAKER-ENTITY-ID TLV/include the SPEAKER-ENTITY-ID TLV/
- OLD
   [...] sets up
   the path by sending PCInitiate message to each node along the path of
   the LSP.  The PCC generates a Path Computation State Report (PCRpt)
   and includes the central controller instruction (CCI) and the
   identified LSP.  The CC-ID uniquely identifies the central controller
   instruction within a PCEP session.  The PCC further responds with the
   PCRpt messages including the CCI and LSP objects.
  NEW
   [...] sets up
   the path by sending a PCInitiate message to each node along the path of
   the LSP. The CC-ID uniquely identifies the central controller
   instruction within a PCEP session. Each PCC further responds with the
   PCRpt messages including the central controller instruction (CCI) and
   the LSP objects.

- s/LSP deletion operation/The LSP deletion operation/
- s/for PCECC LSP is/for PCECC LSPs is/
- s/does Label cleanup/does label cleanup/
- Figure 1: all figures have the same message ordering, though the order
only matters when using PCC-allocated labels (leaving initial and last
messages apart); have you considered shuffling the message order a bit
on figures for PCE-allocated cases?
- s/and where the PCC/and then the PCC/
- s/The PCC would further inform/The PCC further informs/
------
Section 5.5.2.
---
- s/know at both/known by both/
- s/download as part of CCI,/download, as part of CCI,/
- s/but failed/but fails/
------
Section 5.5.3.
---
- s/PCE Initiated PCECC LSP/PCE-Initiated PCECC LSP/  [section title +
figure tag]
- s/PCE Initiated LSP/PCE-initiated LSP/  [x2]
- OLD
   the removal procedure remains the same.
  NEW
   the removal procedure remains the same, with just an additional
constraint on the configuration sequence.
------
Section 5.5.4.
---
- s/the PCECC first update/the PCECC first updates/
- s/then update to ingress/then updates the ingress/
- s/the old instructions/the former instructions/
- s/the old LSP/the former LSP/
- s/Delete old CCI/Delete former CCI/  [Figure 5]
- OLD
   the removal procedure remains the same.
  NEW
   the removal procedure remains the same, adding the sequence constraint.
------
Section 5.5.5.
---
- s/the instruction, it wants/the instruction it wants/  [or "the
instruction that it wants"]
------
Section 5.5.8.
---
- s/PCC Based Allocations/PCC-Based Allocations/
- s/would allocate the label and would report/SHOULD allocate the label
and SHOULD report/
- s/the Label is 0 and the C flag is set,/the Label is 0 and the C flag
is set to 1,/
- s/the Label is 'n' and the C flag is set in the CCI/the Label is 'n'
and the C flag is set to 0 in the CCI/
- s/the PCC should report/the PCC MUST report/
------
Section 5.5.9.
---
- OLD
   to indicate the allocation needs to be
   made by the PCE.  This flag is used to indicate that the allocation
   needs to be made by the PCE.  A PCC would set [...]
  NEW
   to indicate the allocation needs to be
   made by the PCE.  A PCC would set [...]

- s/for a PCE-Initiated (or delegated LSP)./for a PCE-initiated (or
delegated) LSP./
------
Section 6.
---
- s/6. PCEP messages/6. PCEP Messages/
- s/6.1. The PCInitiate message/6.1. The PCInitiate Message/
- s/for the transit LSR./for a transit LSR./
- s/To cleanup,/To clean up entries,/
- s/instances of CCI object would be included in the case/instances of
the CCI object can be included, in the case/
- s/6.2. The PCRpt message/6.2. The PCRpt Message/
- s/the receiving PCC MUST/the receiving PCE MUST/
- s/for the transit LSR./for a transit LSR./
------
Section 7.
---
- s/The PCEP object defined/The PCEP objects defined/
- s/a PCECC based mechanism./a PCECC-based mechanism./
- s/for label download./"for label download/report."/
- s/Flags: is used/Flags: A field used/
- s/C-Bit/C bit/
------
Section 11.
---
- s/sub- registry/sub-registry/
- s/PATH-SETUP- TYPE-CAPABILITY/PATH-SETUP-TYPE-CAPABILITY/
- In section 11.7, a blank line before each Error-Type would improve
readability.
------
Section 12.
---
- The following references may be moved from Normative to Informative:
  * RFC 7420,
  * RFC 7225,
  * RFC 8126,
  * RFC 8174,
  * RFC 8233.

_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.