Subject: [Pce] Shepherd Review of draft-ietf-pce-association-group-06

From: <[email-masked]>
Date: Wed, 6 Mar 2019 18:40:02 +0100

Hi authors,

Please find below the comments to be resolved to move
draft-ietf-pce-association-group forward. Most of them are editorial, so
this looks promising.

Thanks,

Julien

------
Abstract
---
- "LSP[s]" should be expanded at first use, all the more as it has 5
listed expansions in RFC Editor's dictionary (some of them being
incorrect IMHO).
------
1. Introduction
---
- s/GMPLS- controlled/GMPLS-controlled/
------
2. Terminology
---
- When importing definitions, PCEP messages are sometimes referred to as
"Full Phrase" (e.g. "LSP State Report", "LSP Update Request"), sometimes
as "PCAbbrev" (e.g. PCReq, PCRep, PCInitiate). Consistency among imports
would be welcome, combining both ways may be the easiest (e.g. "LSP
Update Request/PCUpd").
------
3.1. Motivation
---
- OLD
   Stateful PCE provides the ability to update existing LSPs and to
   instantiate new ones. To enable support for PCE-controlled make-
   before-break and for protection...
  NEW
   Stateful PCE provides the ability to update existing LSPs and to
   instantiate new ones. There are various situations where several
   LSPs need to share common information. E.g., to support for
   PCE-controlled make-before-break and for protection...

- s/working and protections LSPs/working and protection LSPs/
- s/tunnel. Whereas some/tunnel, whereas some/
- s/some association could/some associations could/
- s/policy based association/policy-based association/
------
3.2. Relationship with the SVEC List
---
- s/are quite different./are quite different, though some use case may
overlap./
- s/association type, should/association type should/
------
3.3.  Operation Overview
---
- s/Whereas, some/Whereas some/
- s/The association are/The associations are/
- s/LSP state clean up/LSP state clean-up/
------
3.4. Operator-configured Association Range
---
- OLD
   it is necessary to configure a range of association identifiers that
   are marked for operator-configured associations to avoid any
   association identifier clash within the scope of the association
   source.
  NEW
   it is necessary to distinguish a range of association identifiers that
   are marked for operator-configured associations to avoid any
   association identifier clash within the scope of the association
   source. This document assumes that these 2 ranges are configured.
------
4.1.1. Procedure
---
- OLD
   The PCEP
   speaker could still use the ASSOCIATION object, if the peer does not
   support the association, it would react as per the procedure
   described in Section 6.4.
  NEW
   In this case, the PCEP
   speaker could still use the ASSOCIATION object: if the peer does not
   support the association, it will react as per the procedure described
   in Section 6.4.

- s/a association type/an association type/
- s/that specify/that specifies/
- The last sentence of the paragraph puzzled me a bit. The current
wording may suggests that "it is RECOMMENDED to support the
aforementioned OPTIONAL TLV", which is inconsistent 2119 language. My
guess is that it should say: "In case the use of the ASSOC-Type-List TLV
is triggered by a mandatory association type, then it is RECOMMENDED
that the PCEP implementation include..." Is my understanding correct?
------
5.1. Procedure
---
- s/In which case/In this case/
- s/would be applied./applies./
- s/the Assoc-Type/the Assoc-type/
- s/carry overlapping range/carry overlapping ranges/
- s/an association-type/an association type/
- s/receives overlapping range/receives overlapping ranges/
- s/for the association type/for an association type/
- The current text only indirectly tackles the case where a given
Assoc-type is advertised multiple times, when forbidding overlapping
ranges. A complementary sentence explicitly mentioning non-overlapping
ranges would be welcome.
- OLD
   In case, there is an operator-configured association that was
   configured with association parameters (such as association
   identifier, association type and association source) at the local
   PCEP speaker, later the PCEP session gets established with the
   association source and a new operator-configured range is learned
   during session establishment. At this time...
  NEW
   There may be cases where an operator-configured association was
   configured with association parameters (such as association
   identifier, association type and association source) at the local
   PCEP speaker, and later the PCEP session gets established with the
   association source and a new operator-configured range is learned
   during session establishment. At this time...

- s/that were not/that are not/
- s/new operator configured range/new operator-configured range/
- s/removing any LSPs/disassociating any LSPs/
- s/an operator configured association/an operator-configured association/
------
6.1.  Object Definition
---
- s/The value 0xffff and 0x0/The values 0xffff and 0x0/
------
6.1.3.  Association Source
---
- s/node that originate/node that originates/
- OLD
   set the source as virtual IP which identify all instances of PCE
  NEW
   set the source as a virtual IP address which identifies all instances
of the PCE

- s/operator configured association/operator-configured association/
------
6.2.  Relationship with the RSVP ASSOCIATION
---
- s/defined in this document, is/defined in this document is/
- s/extensions that defines/extensions that define/
- s/new association type/new association types/
- s/PCEP association/PCEP associations/
- s/RSVP association/RSVP associations/
------
6.3.1.  Stateful PCEP messages
---
- s/Unless, a PCEP speaker/Unless a PCEP speaker/
- s/a new LSP, can/a new LSP can/
------
6.3.2.  Request Message
---
- s/In case of passive stateful or stateless PCE/In case of passive
(stateful or stateless) PCE/
- s/Note that LSP object/Note that the LSP object/
------
6.3.3.  Reply Message
---
- s/In case of passive stateful or stateless PCE/In case of passive
(stateful or stateless) PCE/
- s/PCRep message, indicates/PCRep message indicates/
- s/Note that LSP object/Note that the LSP object/
------
6.4.  Processing Rules
---
- s/The operator configured association is/The operator-configured
associations are/
- s/these configured association/these configured associations/
- s/The dynamic association are/The dynamic associations are/
- s/The association groups is/The association group is/
- s/is combination of/is the combination of/
- s/identifier that uniquely identify/identifiers that uniquely identify/
- s/These number/These numbers/
- s/receives ASSOCIATION object/receives an ASSOCIATION object/
- s/an operator configured association/an operator-configured association/
- s/the association-type and association source/the Association type and
Association Source/
- s/local operator configured information/local operator-configured
information/
- s/receives ASSOCIATION object with R bit set/receives an ASSOCIATION
object with the R bit set/
------
7.  IANA Considerations
---
- s/"PCEP TLV Type Indicators registry"/"PCEP TLV Type Indicators" registry/
- s/future document should /future documents should/
------
9.  Manageability Considerations
---
- "On" in section titles 9.5 and 9.6 may remain in lower case (RFC
Editor's job?).
------
12.  References
---
- I do not believe that RFC 8253 is really normative here (only appears
in manageability section and may lie next to RFC 7525 in informative
references). The same for RFC 8126.
------
Appendix A.
---
- I am not comfortable with reading ranges in round brackets. Have
square brackets been considered?
- s/Consider that because/Consider that, because/
- s/where as/, whereas/  [twice]
- s/a operator configured association/an operator-configured association/
- s/with PCC as/with the PCC as/
- s/if PCE is/if the PCE is/
- I am not sure about the meaning of the sentence "not PCC or PCE as set
as NMS id", please rephrase.
------

_________________________________________________________________________________________________________________________

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.

---

From: Dhruv Dhody <[email-masked]>
Date: Thu, 7 Mar 2019 08:28:15 +0000

Hi Julien, 

Thanks for your detailed review and providing suggested text. 

Here is the working copy - https://raw.githubusercontent.com/dhruvdhody-huawei/ietf/master/draft-ietf-pce-association-group-08.txt
Diff - https://tools.ietf.org/rfcdiff?url1=draft-ietf-pce-association-group-06&url2=https://raw.githubusercontent.com/dhruvdhody-huawei/ietf/master/draft-ietf-pce-association-group-08.txt

> ------
> 4.1.1. Procedure
> ---
> - The last sentence of the paragraph puzzled me a bit. The current wording
> may suggests that "it is RECOMMENDED to support the aforementioned
> OPTIONAL TLV", which is inconsistent 2119 language. My guess is that it
> should say: "In case the use of the ASSOC-Type-List TLV is triggered by a
> mandatory association type, then it is RECOMMENDED that the PCEP
> implementation include..." Is my understanding correct?

[[Dhruv Dhody]] That is correct. Updated.

> ------
> 5.1. Procedure
> ---
> - The current text only indirectly tackles the case where a given Assoc-
> type is advertised multiple times, when forbidding overlapping ranges. A
> complementary sentence explicitly mentioning non-overlapping ranges would
> be welcome.
[[Dhruv Dhody]] Added - 

   An Assoc-Type MUST be present only once in the OP-CONF-ASSOC-RANGE
   TLV, if the same Assoc-Type is present more than once, the PCEP
   session MUST be rejected with error type 1 and error value 1 (PCEP
   session establishment failure / Reception of an invalid Open
   message).

> ------
> Appendix A.
> ---
> - I am not comfortable with reading ranges in round brackets. Have square brackets been considered?

 [[Dhruv Dhody]] In case the association source is not a PCEP peer (for
   example an NMS system), then the default range of <0x1000, 0xffff> is
   considered.

Use of [] throws idnits, and thus I am using <>. 

Thanks again for your review. 

Regards,
Dhruv

---

From: <[email-masked]>
Date: Thu, 7 Mar 2019 14:13:48 +0100

Hi Dhruv,

Congratulation from the prompt update. I'm fine with the <a, b> notation
for ranges.

The only open issue is the text you add below:
- Is there a reason to prohibit, for a given Association type, split
operator-configured ranges? I don't think this is what the original
version suggested.
- Assuming we proceed with this new rule, then why so much text about
overlapping ranges? To have this happen, the TLV would already break
that "present only once" rule: why would an implementation care about
checking if ranges overlap if the TLV is already wrong?

Thanks,

Julien

---

From: Dhruv Dhody <[email-masked]>
Date: Thu, 7 Mar 2019 19:30:46 +0530

Hi Julien,

My bad!
I did some digging and we added this based on Adrian's comment [1] back in
Feb last year (and my memory failed me).

How about -

   The Assoc-type MAY appear more than once in the OP-CONF-ASSOC-RANGE
   TLV in the case of a non-contiguous Operator-configured Association
   Range.  The PCEP speaker originating this TLV MUST NOT carry
   overlapping ranges for an association type.  If a PCEP peer receives
   overlapping ranges for an association type, it MUST consider the Open
   message malformed and MUST reject the PCEP session with error type 1
   and error value 1 (PCEP session establishment failure / Reception of
   an invalid Open message).

Thanks!
Dhruv

[1] https://mailarchive.ietf.org/arch/msg/pce/QSobS2pul-lIlMLXV4KcBhwgBM0

On Thu, Mar 7, 2019 at 6:45 PM <[email-masked]> wrote:

> 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,
> This message and its attachments may contain confidential or privileged
> information that may be protected by law;

---

From: <[email-masked]>
Date: Thu, 7 Mar 2019 17:19:47 +0100

This sentence is clear enough to address my concern.

Thank you!

Julien

On 07/03/2019 15:00, Dhruv Dhody wrote:
> Hi Julien, 
>
> My bad! 
> I did some digging and we added this based on Adrian's comment [1]
> back in Feb last year (and my memory failed me). 
>
> How about - 
>
> On Thu, Mar 7, 2019 at 6:45 PM <[email-masked]
> <mailto:[email-masked]>> wrote:
>
>     Hi Dhruv,
>
>     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,
>     This message and its attachments may contain confidential or
>     privileged information that may be protected by law;
>     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.
>