[Pce] Chair review of draft-ietf-pce-association-diversity

[Dhruv Dhody <[e-mail]>]

Hi Authors,

I did a chair's review of the I-D. Expect a separate shepherd review from
Julien. A little effort needs to be made by the authors to make this I-D
crisp.

(1) Section 3, discussion related to SVEC feels incorrect inside a section
called 'Motivation'. Lets create a new section (I made minor changes) -

Relationship to SVEC

   [RFC5440] defines a mechanism for the synchronization of a set of
   path computation requests by using the SVEC (Synchronization VECtor)
   object, that specifies the list of synchronized requests that can
   either be dependent or independent.  The SVEC object identify the
   relationship between the set of path computation requests, identified
   by 'Request-ID-number' in RP (Request Parameters) object.  [RFC6007]
   further clarified the use of the SVEC list for synchronized path
   computations when computing dependent requests as well as described a
   number of usage scenarios for SVEC lists within single-domain and
   multi-domain environments.

   The SVEC object includes a Flags field that indicates the potential
   dependency between the set of path computation request in a similar
   way as the Flags field in the TLVs defined in this document.  The
   path computation request in the PCReq message MAY use both SVEC and
   ASSOCIATION object to identify the related path computation request
   as well as the diversity association group.  The PCE MUST try to find a
   path that meets both the constraints.  It is possible that the
   diversity requirement in the association group is different from the one
   in SVEC object.  The PCE would consider both the objects as per the
   processing rules and aim to find a path that meets both these constraints.
   In case no such path is possible (or the constraints are incompatible),
   the PCE MUST send a path computation reply (PCRep) with NO-PATH object
   indicating path computation failure as per [RFC5440].

(2) Section 3, I am not sure about this -

   Using the disjoint-group within a PCEP messages may have two purpose:

   o  Information: in case the PCE is performing the path computation,
      it may communicate to the PCC the disjoint parameters.

   o  Configuration: in case the PCC are configured with disjoint
      requirements, these are communicated to the PCE.

This feels incomplete as it singles out information from PCE to PCC and
configuration at PCC. Maybe better to re-word along our TLVs - where the
disjoint status is included by PCE but configuration is included by both.

(3) Section 4.1,
   But a PCE may be limited
   in how many LSPs it can take into account when computing
   disjointness.  If a PCE receives more LSPs in the group than it can
   handle in its computation algorithm, it SHOULD apply disjointness
   computation to only a subset of LSPs in the group.  The subset of
   disjoint LSPs will be decided by PCE as a local matter.

You then say

   Local polices on the PCC or PCE MAY define the computational behavior
   for the other LSPs in the group.  For example, the PCE may provide no
   path, a shortest path, or a constrained path based on relaxing
   disjointness, etc.

Better to merge the above with the previous paragraph and also include some
text on the disjoint status information.

(4) Section 4.2,

      *  P (Shortest path) bit: when set, this indicates that the
         computed path of the LSP SHOULD satisfies all constraints and
         objective functions first without considering the diversity
         constraint.  This means that an LSP with P flag set should be
         placed as if the disjointness constraint has not been
         configured, while the other LSP in the association with P flag
         unset should be placed by taking into account the disjointness
         constraint.  Setting P flag changes the relationship between
         LSPs to a unidirectional relationship (LSP 1 with P=0 depends
         of LSP 2 with P=1, but LSP 2 with P=1 does not depend of LSP 1
         with P=0).

Instead of unidirectional, maybe one-sided relationship?

It will be better to include this text from section 4.4 here instead -

   Multiple LSPs in the same disjoint group may have the P-flag set.  In
   such a case, those LSPs may not be disjoint from each other but will
   be disjoint from others LSPs in the group that have the P-flag unset.

Also, add "Unassigned bits MUST be set to 0 on transmission and MUST be
ignored on receipt."

(5) Section 4.2, are all flags valid in DISJOINTNESS-STATUS-TLV? What does 'P'
and 'T' mean here?

(6) Section 4.3, it would be good to limit the OF-Codes allowed inside the
disjoint association to the new OF code defined in this document. Add an error
in case some other OF code is used.

(7) Section 4.4, we should remove 'both' as we lack this in PCEP-SR right now.

   An implementation may choose one
   of the paths or even use both (using both may happen in case Segment
   Routing TE is used, allowing ECMP).

(8) Why 'MAY' in -

   When P-flag is set for an LSP and when ECMPs are available, an
   implementation MAY select a path that allows disjointness.

It should be MUST or SHOULD.
Can you do a check on RFC2119 keywords through out the I-D?

(9) The IANA considerations section needs some more work. The request to
IANA are not spelled out clearly, please review this section.

(10) The manageability considerations needs some meat. Top of my head -
reference to Yang, Operator configured range for disjoint group, number of
LSPs inside an association group etc.

Nits
----
- Expand on 1st use - PCRpt, PCUpd, PCInitiate, PLSP-ID...

- Section 1,
OLD
   [I-D.ietf-pce-association-group] introduces a generic mechanism to
   create a grouping of LSPs which can then be used to define
   associations between a set of LSPs and a set of attributes (such as
   configuration parameters or behaviors) and is equally applicable to
   the active and passive modes of a stateful PCE [RFC8231] or a
   stateless PCE [RFC5440].
NEW
   [I-D.ietf-pce-association-group] introduces a generic mechanism to
   create a grouping of LSPs in the context of a PCE which can then be
   used to define associations between a set of LSPs or between a set of
   LSPs and a set of attributes (such as configuration parameters or
   behaviour), and is equally applicable to the stateful PCE [RFC8231]
   (active and passive modes) as well as the stateless PCE [RFC5440].
END

-Section 2, the term LSR is not used. We could expand this section to point
to some other useful terms, just by adding reference to where they are
defined.

- Section 4.1
OLD:
   The Association parameters, as described in
   [I-D.ietf-pce-association-group] as the combination of the mandatory
   fields Association type, Association ID and Association Source in the
   ASSOCIATION object, that uniquely identify the association group,
   uniquely identify the disjoint group.
NEW:
   The Association parameters (as described in
   [I-D.ietf-pce-association-group]) is the combination of the mandatory
   fields Association type, Association ID and Association Source in the
   ASSOCIATION object, that uniquely identify the association group.
END
s/to uniquely identifying/to uniquely identify/
s/belonging to this associations/belonging to this association/
s/insurance/assurance/

- Section 4.2
s/relaxing disjointness constraint at all
 /relaxing disjointness constraint fully/
add OF-List TLV (as per Section 4.3) into the list of TLVs

-Section 4.4
In figure 3, it would be good to say the cost of other links is 1?
s/allowed to be longer/allowed to be costlier/
What is RTD? Is it round trip delay? Better to use Path delay metric as per
RFC8233 .
OLD:
   Driving the PCE disjointness computation may be done in other ways by
   for instance setting a metric boundary reflecting an RTD boundary.
NEW:
   Driving the PCE disjointness computation may be done in other ways,
   for instance by setting a path delay metric boundary.
END

-Section 4.5
OLD:
   All LSPs in a particular disjoint group MUST use the same combination
   of T,S,N,L flags in the DISJOINTNESS-CONFIGURATION-TLV.  If a PCE
   receives PCRpt messages for LSPs belonging to the same disjoint group
   but having an inconsistent combination of T,S,N,L flags, the PCE
   SHOULD NOT try to compute disjointness path and SHOULD reply a PCErr
   with Error-type 26 (Association Error) and Error-Value 6 (Association
   information mismatch) to all PCCs involved in the disjoint group.
NEW:
   All LSPs in a particular disjoint group MUST use the same combination
   of T,S,N,L flags in the DISJOINTNESS-CONFIGURATION-TLV.  If a PCEP peer
   receives a PCEP message for LSPs belonging to the same disjoint group
   but having an inconsistent combination of T,S,N,L flags, the PCEP peer
   SHOULD NOT try to add the LSP in the disjoint group and SHOULD reply with
   a PCErr with Error-type 26 (Association Error) and Error-Value 6 (Association
   information mismatch).
END
The change is suggested to make this more generic and not just for the PCRpt
message.

- Section 6, use [This.I-D]

Thanks!
Dhruv