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

[Dhruv Dhody <[e-mail]>]

Hi,

Thanks for making the update (-07). Just one more nit in the update -

https://tools.ietf.org/html/draft-ietf-pce-association-diversity-07

Section 4.1

   As per [I-D.ietf-pce-association-group], LSPs are associated with
   other LSPs with which they interact by adding them to a common
   association group.  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
   belonging to this association.

Not sure about "belonging to this association". I feel we should just stop at
"....that uniquely identify the association group."

Thanks,
Dhruv

On Thu, May 16, 2019 at 4:41 PM Dhruv Dhody <[e-mail]> wrote:
>
> 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