Subject: Re: [Pce] Benjamin Kaduk's No Objection on draft-ietf-pce-association-diversity-12: (with COMMENT)

From: Mahend Negi <[email-masked]>
Date: Wed, 18 Dec 2019 21:46:02 +0530

Hi Ben,

Thanks for the detailed review, please find the details inline to your mail.
B/w new version is uploaded.

 htmlized versions available at:
https://tools.ietf.org/html/draft-ietf-pce-association-diversity-13
https://datatracker.ietf.org/doc/html/draft-ietf-pce-association-diversity-13

A diff from the previous version is available at:
https://www.ietf.org/rfcdiff?url2=draft-ietf-pce-association-diversity-13

Thanks,
Mahendra

On Wed, Oct 30, 2019 at 5:37 AM Benjamin Kaduk via Datatracker <
[email-masked]> wrote:

> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-pce-association-diversity-12: No Objection
>
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
>
>
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
>
>
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-pce-association-diversity/
>
>
>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> Thanks for this generally well-written document!  Most of my comments are
> pretty minor, but please do note the edge case in objective function
> handling, the question about which TLVs are allowed when the ASSOCIATION
> object is carried in which messages, the notation clarification question,
> and the question about what "completely relax the disjointness constraint"
> means.
>
> Please check the TBD<N> placeholders; it looks like (e.g.) TBD8 is used for
> both a NO-PATH-VECTOR TLV bit and for an error code (but I didn't
> double-check the others).
>
>
> On Wed, Oct 30, 2019 at 5:37 AM Benjamin Kaduk via Datatracker
> <[email-masked]> wrote:
> >
> > Benjamin Kaduk has entered the following ballot position for
> > draft-ietf-pce-association-diversity-12: No Objection
> >
> > When responding, please keep the subject line intact and reply to all
> > email addresses included in the To and CC lines. (Feel free to cut this
> > introductory paragraph, however.)
> >
> >
> > Please refer to
> https://www.ietf.org/iesg/statement/discuss-criteria.html
> > for more information about IESG DISCUSS and COMMENT positions.
> >
> >
> > The document, along with other ballot positions, can be found here:
> > https://datatracker.ietf.org/doc/draft-ietf-pce-association-diversity/
> >
> >
> >
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> >
> > Thanks for this generally well-written document!  Most of my comments are
> > pretty minor, but please do note the edge case in objective function
> > handling, the question about which TLVs are allowed when the ASSOCIATION
> > object is carried in which messages, the notation clarification question,
> > and the question about what "completely relax the disjointness
> constraint"
> > means.
> >
> > Please check the TBD<N> placeholders; it looks like (e.g.) TBD8 is used
> for
> > both a NO-PATH-VECTOR TLV bit and for an error code (but I didn't
> > double-check the others).
> >
>
> Fixed.
>
> As a general comment (but mostly for my own curiousity), I'm curious
> whether
> you predict much usage of the DAG association type with none of the L/N/S/T
> bits set in the configuration, effectively using the association to carry
> the new objective function(s) from this document.
>

> Yes, your interpretation is correct.
>
> Section 5.1
>
>    A disjoint group can have two or more LSPs, but a PCE may be limited
>    in the number of 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 policy.  Local
>
> Just to double-check: this "only apply disjointness to a subset, using
> local
> policy" behavior is preferred to returning an error for the last LSP
> attempting to join the group?  I do see that the relaxation of disjointness
> is reported back via the DISJOINTNESS-STATUS-TLV, so at least there
> shouldn't be surprises about what's (not) happening, unless the PCE decides
> to not send that TLV for some reason.
>

Yes, but the key here is for "computation algorithm" limitation. There
> is a generic error in
> https://tools.ietf.org/html/draft-ietf-pce-association-group-10#section-6.4
> that might still be applied in case of too many LSPs irrespective of the
> limitation of the disjoint computation algorithm.
>
>

>    of T, S, N, L flags in the DISJOINTNESS-CONFIGURATION-TLV.  If a PCEP
>    peer receives a PCEP messages for LSPs belonging to the same disjoint
>    group but having an inconsistent combination of T, S, N, L flags, the
>    PCEP peer MUST NOT try to add the LSPs in disjoint group and MUST
>    reply with a PCErr with Error-type 26 (Association Error) and Error-
>    Value 6 (Association information mismatch).
>
> nit: s/in disjoint group/to the disjoint group/
>
>    A particular LSP MAY be associated to multiple disjoint groups, but
>    in that case, the PCE SHOULD try to consider all the disjoint groups
>    during path computation if possible.  Otherwise a local policy MAY
>    define the computational behavior.  If a PCE does not support such a
>    path computation it MUST NOT add the LSP into association group and
>    return a PCErr with Error-type 26 (Association Error) and Error-Value
>    7 (Cannot join the association group).
>
> It's interesting that "be in multiple disjoint groups" gets
> error-on-failure
> semantics but (above) "too many LSPs in a single group" gets
> degrade-and-report semantics.  But it's clearly documented, so I don't see
> any problems per se.
>
> Section 5.2
>
> It looks like we only talk about which messages carry the
> DISJOINTNESS-STATUS-TLV; is the implication that the other TLVs are not
> restricted in which message can carry them a correct one?  (Also, that
> DISJOINTNESS-CONFIGURATION-TLV must be present even in PCRep?)
>
>   Yes. In PCRep both the TLVs are included.

   The disjoint group MUST carry the following TLV:
>
> Since we're talking about protocol elements now, I'd suggest to explicitly
> say "TBD1 Disjoint Association Type group" or something else that clearly
> identifies the association-group as a protocol element.
>
>    ACK

>    In addition, the disjoint group MAY carry the following TLV:
>
> nit: "TLVs" plural.
>
>    If a PCEP speaker receives a disjoint-group without DISJOINTNESS-
>    CONFIGURATION-TLV, it SHOULD reply with a PCErr Error-type=6
>    (Mandatory Object missing) and Error-value=TBD8 (DISJOINTNESS-
>    CONFIGURATION-TLV missing).
>
> I suggest being more explicit about (e.g) "ASSOCIATION object of type
> TBD1",
> since "disjoint-group" is somewhat of a colloquialism.
>

> ACK

> Seciion 5.3
>
>    An objective function (OF) MAY be applied to the disjointness
>    computation to drive the PCE computation behavior.  In this case, the
>    OF-List TLV (defined in ([RFC5541]) is used as an optional TLV in the
>    Association Group Object.  Whereas the PCEP OF-List TLV allows
>    multiple OF-codes inside the TLV, a sender SHOULD include a single
>    OF-code in the OF-List TLV when included in the Association Group,
>    and the receiver MUST consider the first OF-code only and ignore
>    others if included.
>
> This usage seems a little weird (albeit unlikely to be problematic), since
> RFC 5441 uses the OF-List TLV to indicate what OFs are supported, and has
> it
> carried in the OPEN object, with a dedicated OF object used to indicate the
> particular OF requested/used for a given path computation.  Repurposing the
> TLV to now be a container for the requested OF for a specific computation
> feels like a qualitatively different usage than the original one.
>
> Agreed, defining a new TLV that only allows one OF code was also bit
of a overkill; plus there is precedence for this sort of a thing -
https://www.rfc-editor.org/rfc/rfc8685.html#section-3.4.2

>    MSL
>      [...]
>       *  A path P passes through K links {Lpi,(i=1...K)}.
>
>       *  A set of paths {P1...Pm} have L links that are common to more
>          than one path {Lpi,(i=1...L)}.
>
> Can you double-check the notation here?  In the first quoted item it seems
> that Lpi indicates the i-th link on path P, but in the second it looks like
> Lpi indicates the i-th link in common across paths P1...Pm.  I'd suggest
> using a different term for the different meaning.
>
> ACK

>    MSS
>    [...]
>       *  A path P passes through K links {Lpi,(i=1...K)} belonging to
>          unique M SRLGs {Spi,(i=1..M)}.
>
> What is the relationship between K and M?  Is it always true that M <= K?
>
>  One link could have multiple SRLGs, so no!

      *  A set of paths {P1...Pm} have L SRLGs that are common to more
>          than one path {Spi,(i=1...L)}.
>
> [same comment about terminology reuse]
>
>    MSN
>       [...]
>       *  A path P passes through K nodes {Npi,(i=1...K)}.
>
>       *  A set of paths {P1...Pm} have L nodes that are common to more
>          than one path {Npi,(i=1...L)}.
>
> [same comment about terminology reuse]
>
>    If the OF-list TLV is included in the Association Object, the OF-code
>    inside the OF Object MUST include one of the disjoint OFs defined in
>    this document.  If this condition is not met, the PCEP speaker MUST
>    respond with a PCErr message with Error-Type=10 (Reception of an
>    invalid object) and Error-Value=TBD9 (Incompatible OF code).
>
> Looking at edge cases, I think that the MUST-level requirements allow for
> me
> to send an OF-list TLV with two items the first of which is nonsense (i.e.,
> not one of these three), and the second of which is one of these three.
> The
> recipient would be obligated to use the first one in the list (by the
> previous text "the receiver MUST consider the first OF-code only") despite
> it being nonsensical.  We should probably try to close that edge case,
> though there are several approaches to choose from and I don't have a sense
> for what might cause us to prefer one over the other.
>
> As per Suresh's suggestion, this is changed to -

   If the OF-list TLV is included in the Association Object, the first
   OF-code inside the OF Object MUST be one of the disjoint OFs defined
   in this document.  If this condition is not met, the PCEP speaker
   MUST respond with a PCErr message with Error-Type=10 (Reception of an
   invalid object) and Error-Value=TBD9 (Incompatible OF code).

Which should handle your case and we would return an error.

Section 5.4
>
>    SVEC object.  The PCE MUST consider both the objects as per the
>    processing rules and aim to find a path that meets both of these
>    constraints.  In case no such path is possible, the PCE MUST send a
>    path computation reply (PCRep) with a NO-PATH object indicating path
>    computation failure as per [RFC5440].  It should be noted that the
>
> I would consider reminding the reader that the 'T' bit controls how
> strictly
> the PCE needs to interpret the DAG constraints, and thus that it's possible
> for the PCE to degrade the request without needint to return NO-PATH in
> such
> cases.
>
> How about - "The PCE MUST consider both the objects (including the
flags set inside the objects) as per the processing rules and aim to
find a path that meets both of these constraints."

Section 5.5
>
>    into account the disjointness constraint.  Setting P flag changes the
>    relationship between LSPs to a one-sided 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).  Multiple LSPs in the same disjoint group may have the P
>
> nits: "Setting the P flag", "depends on LSP 2"
>
> I suggest specifying "link disjoint" for at least the example using Figure
> 4
> (if not the one using Figure 3 as well), since the paths
> PE1->R1->R4->R2->PE2 and PE3->R3->R4->PE4 are not node-disjoint.
>
> ACK

Section 5.6
>
>    There are some cases where the PCE may need to completely relax the
>    disjointness constraint in order to provide a path to all the LSPs
>    that are part of the association.  A mechanism that allows the PCE to
>    fully relax a constraint is considered by the authors as more global
>    to PCEP rather than linked to the disjointness use case.  As a
>    consequence, it is considered as out of scope of the document.
>
> I'm not sure how to interpret this.  Is it supposed to prevent a PCE from
> falling back to "no disjointness" (e.g., all of L/N/S/T are clear in
> DISJOINTNESS-STATUS-TLV)?  If so, I would have expected this limitation to
> be spelled out more clearly much earlier in the document.
>
> There is a proposal to make an objects as optional to process in
stateful messages via
https://datatracker.ietf.org/doc/draft-dhody-pce-stateful-pce-optional/,
this already exist for stateless messages via RFC 5440. The above text
is simply to say that relaxation to "no disjointness" can be handled
via a common technique and out of scope of this I-D.

> Section 6
>
> I suggest also mentioning the security considerations of RFC 7470, as we
> have very little control over what information goes in
> VENDOR-INFORMATION-TLV.  (Not that there's a whole lot of content there,
> but
> maybe it will get people thinking.)
>
> I might also consider mentioning again that certain combinations of flags
> (notably, the 'T' bit) can result in unsatisfiable requests.  But that's
> only somewhat a "security" consideration; security aspects only really come
> into play for it if someone is spoofing that T bit, and we already
> recommend
> TLS.
>
> ACK

Section 8.1
>
>    Range MUST be allowed to be set by the operator.  Operator SHOULD be
>    allowed to set the local policies to define various disjoint
>
> nit: The operator"
>
> Section 8.2
>
>    associations configured or created dynamically.  Further
>    implementation SHOULD allow to view disjoint associations reported by
>    each peer, and the current set of LSPs in this association.  The PCEP
>
> nit: "Furthermore, implementations"
>
> Section 8.4
>
>    Mechanisms defined in this document do not imply any new operation
>    verification requirements in addition to those already listed in
>    [RFC5440].
>
> We don't think that someone should check that the computed LSPs actually
> supply the disjointness properties they're claimed to provide?
>
>
Updated to -

   Apart from the operation verification requirements already listed in
   [RFC5440], a PCEP implementation SHOULD provide parameters related to
   disjoint path computation, such as number of DAG, number of disjoint
   path computation failures etc.  A PCEP implementation SHOULD log
   failure events (e.g., incompatible Flags).

Section 10.2
>
> I guess a strict reading of
>
> https://www.ietf.org/blog/iesg-statement-normative-and-informative-references/
> would require RFC 7470 to be a normative reference, but that doesn't really
> seem like the right thing to do for this specific case.
>
> On the other hand, a RECOMMENDation for RFC 8253's use does feel like it
> would place it as a normative reference.
>
> ACK

---

From: Benjamin Kaduk <[email-masked]>
Date: Mon, 23 Dec 2019 21:37:22 -0800

Hi Mahend,

Most of this looks good; I'll trim most things.
As a side note, the quoting in your message seems to have gotten mixed up
somehow, though I think that I figured out the important parts.

[...]
> find a path that meets both of these constraints."

It is up to you what is best, though I had in mind a phrase including the
specific words "the 'T' bit", since it seemed particularly noteworthy to
me.  But what is noteworthy to me is not necessarily noteworthy to anyone
else, so I do not insist on anything.

> via a common technique and out of scope of this I-D.

So if I understand correctly, the relaxation would be done at the object
level and would not involve specifically giving meaning to L/N/S/T of
all-zeros?  That makes things much more clear, and I agree that there's no
need to say more in this document other than perhaps a reference to
draft-dhody-pce-stateful-pce-optional.

Thanks,

Ben