Re: [Pce] PCE WG LC for draft-ietf-pce-pcep-extension-for-pce-controller

[Adrian Farrel <[e-mail]>]

Hi Julien, WG, authors,

I'm listed as one of the eight Contributors to this document, although
my affiliation should read "Old Dog Consulting".

I was a co-author of RFC 8283, so I am generally glad to see protocol
work addressing this space.

This document is almost ready to progress, but there are quite a few
nits that we should take care of before asking to advance the document.
After that's done, I support this document being published as an RFC.

Thanks,
Adrian

===

Figures

If you make the table in Section 5.2 into "Figure 1" then you will not 
need to renumber Figure 2.

All figures should have a caption such as you have for Figure 2.

All figures should be mentioned explicitly in the text. Thus, instead of
saying "as shown below" you should say "as shown in Figure 2".  This is
important because:
- it helps the RFC Editor check that you are using all of the figures
- it protects the text from edits that might move it around in relation
  to the figure.

---

Abstract

s/(G)MPLS/MPLS and GMPLS/
s/PCEP protocol extensions/PCEP extensions/

---

Introduction

s/draft specify/document specifies/

---

Introduction

   The extension for PCECC in Segment Routing (SR) is specified in a
   separate draft [I-D.zhao-pce-pcep-extension-pce-controller-sr].

This paragraph is, of course, completely true. But it is absolutely
unclear what it is doing here. There has been no previous discussion of
SR in the document, and the only other mention of SR is in 5.5.9. I
think we can probably do without this paragraph.

---

2.

OLD
   Terminologies used in this document is same as described in the draft
   [RFC8283].
NEW
   Terminology used in this document is that same as that described in 
   [RFC8283].
END

---

3.

OLD
   it is assumed that label range to be used
NEW
   it is assumed that the label range to be used
END

OLD
   A future extension could add this capability
NEW
   A future extension could add the capability
END

OLD
   This document also allow a case where the label space is maintained
   by PCC itself
NEW
   This document also allows a case where the label space is maintained
   by the PCC itself
END

---

4.

OLD
   Following key requirements associated PCECC should be considered when
   designing the PCECC based solution:
NEW
   The following key requirements should be considered when
   designing the PCECC based solution:
END

---

5.3
s/This document specify/This document specifies/
s//new CCI object-type/new CCI object-types/

---

5.4
s/During PCEP Initialization Phase/During the PCEP Initialization Phase/
s/Path is setup/Path is set up/
s/sub-TLV in PCC's/sub-TLV in a PCC's/
s/PCE's OPEN message/a PCE's OPEN message/

---

5.4

   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.

This is good. But you also need to include what happens if an
implementation that does not understand these extensions receives one.
I suspect you can do this with a reference to another document.

---

There are a couple of cases of s/a LSP/an LSP/

---

5.5.1

s/based on PCECC mechanism/based on the PCECC mechanism/
s/LSP-IDENTIFIER TLV MUST/An LSP-IDENTIFIER TLV MUST/
s/with D flags/with D flag/
s/and set up the/and sets up the/
s/include the central/includes the central/
s/The CC-ID is uniquely identify/The CC-ID uniquely identifies/
s/two CCI object/two CCI objects/
s/PCECC LSP is same as/PCECC LSP is the same as/
s/receives PCRpt message/receives a PCRpt message/   (twice)
s/The PCECC LSP are/The PCECC LSPs are/
s/In case where/In the case where/
s/label allocation are/label allocations are/
s/Similarly C bit/Similarly the C bit/

---

5.5.1

You have:
   This PCUpd message is as per [RFC8231] SHOULD include the path
   information as calculated by the PCE.

I think that you are not defining anything new here. You are just
describing what 8231 already says. So how about...

   Per [RFC8231], this PCUpd message should include the path
   information calculated by the PCE.

---

5.5.1

   The Ingress MAY further choose to deploy a
   data plane check mechanism and report the status back to the PCE via
   PCRpt message.

This is fine, but you should probably add some explanation of why an
Ingress might choose to do this.

---

5.5.1

   It should be noted that in this example, the request is made to the
   egress node with C bit set in the CCI object to indicate that the
   label allocation needs to be done by the egress and it responds with
   the allocated label to the PCE.

This is a little ambiguous. "...it responds" - what is it?
Perhaps you should have:
"...done by the egress, and the egress responds..."

---

5.5.2.1

s/to setup an LSP based/to set up an LSP based/
s/included in LSP object/included in the LSP object/   (twice)
s/New PCEP object/A new PCEP object/

---

5.5.2.2

   If the PCC receives a PCInitiate message but does not recognize the
   label in the CCI, the PCC MUST generate a PCErr message with Error-
   Type 19(Invalid operation) and Error-Value=TBD8, "Unknown Label" and
   MUST include the SRP object to specify the error is for the
   corresponding label cleanup (via PCInitiate message).

This is OK as a choice, but I wonder why you need to report this as an
error. The intention of the request is to make sure that the label in 
the CCI is removed from the PCC. If the label is not recognised, isn't
this the right result meaning that the answer should be a positive
response?

I'm not saying you must change this, but please consider it.

---

5.5.3

s/In order to setup/In order to set up/

OLD
   The
   PCC responds with first PCRpt message with the status as "GOING-UP"
   and assigned PLSP-ID.
NEW
   The
   PCC responds with a PCRpt message with the status set to "GOING-UP"
   and carrying the assigned PLSP-ID.
END

s/from PCECC are send/from PCECC are sent/
s/setup operations are/setup operations are the/
s/LSP is same as/LSP is the same as/
s/the PCE SHOULD send the/the PCE SHOULD send a/
s/In case where/In the case where/
s/label allocation are made/label allocations are made/

---

5.5.3

   In case where the label allocation are made by the PCC itself (see
   Section 5.5.8), the procedure remains similar.

Do you mean "remains similar" or "is the same"?
If it is only similar then you need to describe how it is different.

---

5.5.4                                                  `
s/modification of PCECC/modification of a PCECC/
s/In case where/In the case where/
s/label allocation are made/label allocations are made/

---

5.5.4

   In case where the label allocation are made by the PCC itself (see
   Section 5.5.8), the procedure remains similar.

Same question as 5.5.3

---

5.5.5

s/control over the/control over an/
s/In case of/In the case of a/

---

5.5.9

s/the PCE via PCRpt message as per/the PCE via a PCRpt message as per/

---

5.5.9

   The PCECC capability MUST be exchanged on the PCEP session, before
   PCE could allocate binding label.

Maybe turn this around to read...

   Before a PCE can allocate a binding label the PCECC capability MUST
   be exchanged on the PCEP session.

---

6.1

OLD
   the message has been extended as shown below
NEW
   this document extends the message as shown below
END

---

Either in 6.1 or in section 2, you should include something like

   The message formats in this document are specified using Routing
   Backus-Naur Form (RBNF) encoding as specified in [RFC5511].

And obviously add a reference to 5511.

---

6.1

c/At max two instances/At most two instances/

---

7.1

s/defines a new optional TLVs/defines new optional TLVs/

---

7.1.1

OLD
   No flags are assigned right now.
NEW
   No flags are defined in this document.
END

---

7.3

OLD
   Currently, the following flag bit is defined:
NEW
   Currently, the following flag bits are defined:
END

s/A PCE set this bit/A PCE sets this bit/

---

11.3

Can you add to the definition of this registry what the size is? I 
think there are 32 bits available.

---

11.4

I think the name of this section is wrong. you are not creating a new
registry in this section.

---

11.6

You should say how many bits this registry can track. I think it is 16.

---

11.7

It's not too important, but it might be nice to order this list by
increasing error type.

-----Original Message-----
From: Pce <[e-mail]> On Behalf Of [e-mail]
Sent: 05 August 2020 17:19
To: [e-mail]
Subject: [Pce] PCE WG LC for
draft-ietf-pce-pcep-extension-for-pce-controller

Hi all,

This message initiates a 3-week WG Last Call on
draft-ietf-pce-pcep-extension-for-pce-controller-06. Please review and
share your feedback, whatever it is, using the PCE mailing list.
This LC will end on Wednesday August 26, 11:59pm (any timezone).

Please note that this I-D is related to
draft-zhao-pce-pcep-extension-pce-controller-sr which is already in our
WG adoption queue.

Thanks,

Dhruv & Julien


____________________________________________________________________________
_____________________________________________

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.

_______________________________________________
Pce mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/pce