Re: [Pce] [Gen-art] Genart last call review of draft-ietf-pce-association-diversity-10

[Dhruv Dhody <[e-mail]>]

Hi Alissa,

An update has been posted that makes changes based on the genart
review by the authors -
https://www.ietf.org/rfcdiff?url1=draft-ietf-pce-association-diversity-10&url2=draft-ietf-pce-association-diversity-12

A response to the Dale's email discussing these changes is pending.

Dhruv


On Wed, Oct 30, 2019 at 9:09 PM Alissa Cooper <[e-mail]> wrote:
>
> Thanks Dale. I entered a DISCUSS ballot to chat about a couple of your major points below and requested a response to the rest of your review.
>
> Alissa
>
>
> > On Oct 12, 2019, at 8:04 PM, Dale Worley via Datatracker <[e-mail]> wrote:
> >
> > Reviewer: Dale Worley
> > Review result: Ready with Issues
> >
> > I am the assigned Gen-ART reviewer for this draft.  The General Area
> > Review Team (Gen-ART) reviews all IETF documents being processed by
> > the IESG for the IETF Chair.  Please treat these comments just like
> > any other last call comments.
> >
> > For more information, please see the FAQ at
> >
> > <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
> >
> > Document:  draft-ietf-pce-association-diversity-10
> > Reviewer:  Dale R. Worley
> > Review Date:  2019-10-12
> > IETF LC End Date:  2019-10-18
> > IESG Telechat date:  not known
> >
> > Summary:
> >
> >       This draft is on the right track but has open issues, described
> >       in the review.
> >
> > There are various exposition issues, as detailed below.
> >
> > There are some minor technical issues about reporting of error
> > conditions, the significance of the P bit in DISJOINTNESS-STATUS-TLV,
> > the detailed definition of the new objective functions, etc. as
> > detailed below.
> >
> > But it seems like a number of complex cases haven't been clearly
> > specified:  (1) the effect of the P bit if multiple LSPs have P=1, (2)
> > the interaction of SVEC and disjoint-groups, and (3) the interaction
> > of partially overlapping disjoint-groups and/or SVEC sets.
> >
> > Major issues:
> >
> > The relationship of this mechanism with SVEC seems to be important but
> > is not clearly stated.  The relevant sections of the text seem to be:
> > section 4 para 2, section 5.3, and section 5.4 from "[RFC5440] uses
> > SVEC diversity flag" on.  I think that they need to be pulled into one
> > section.  Then it will be possible to have a good description of the
> > interaction with SVEC.
> >
> > At the end of section 5.6 is the revelation that (1) the
> > DISJOINTNESS-CONFIGURATION-TLV is not attached to a group (in some
> > sense), but instead a separate copy of it is attached to every LSP in
> > the group, (2) these copies must match in the group IDs they carry and
> > also the T, S, N, and L flags in them must agree, (3) (I suspect) any
> > OF-code must agree, and (4) the P flags in them may differ.  This
> > information is needed as background for a number of parts of the text
> > to make sense, particularly the description of the P bit in section
> > 5.2 and section 5.5.  I recommend that the last para of section 5.5 be
> > moved to section 5.1 or the beginning of section 5.2, so the reader is
> > aware that the descriptions of each LSP carry separate, and not
> > necessarily identical, DISJOINTNESS-CONFIGURATION-TLVs.
> >
> > The path computation effects of the P bit are described in the "P"
> > item in section 5.2 and section 5.5.  But the descriptions are
> > unclear, or perhaps they presume that there are only two LSPs in the
> > group.  I think the intended meaning is that all of the LSPs in the
> > group with P=1 are computed first, and then with those LSPs fixed, the
> > LSPs in the group with P=0 are computed.  This will cause
> > shortest-path constraints (and other objective functions) to be
> > optimized on the P=1 LSPs, and those paths will not be de-optimized by
> > competition from the other paths.  This should probably be pulled out
> > of the description of the "P" in its TLV and put into a separate
> > paragraph.
> >
> > Minor issues:
> >
> > Nits/editorial comments:
> >
> > In general, check the uses of "could" and "would".  I think "would" is
> > sometimes used to mean "... in that situation, the XXX MUST ...", and
> > "could" is sometimes used where "MAY" would be clearer.
> >
> > The running title is given as "ASSOC-DISJOINT".  I think "PCEP
> > Diversity Constraint Signaling" would be clearer.
> >
> > Abstract
> >
> >   The proposed extension allows a Path Computation Client (PCC) to
> >   advertise to a PCE that a particular LSP belongs to a
> >   disjoint-group, thus the PCE knows that ...
> >
> > It would be clearer if "a disjoint-group" was amplified to "a
> > particular disjoint-group".
> >
> > Table of Contents
> >
> > There are a few section titles in which important words aren't
> > capitalized ("functions" in section 5.4), or unimportant words are
> > capitalized ("On" in sections 8.5 and 8.6).
> >
> > The title of section 8.4 is a verb phrase, compared to the titles of
> > the other subsections of section 8.  I suggest "Verification of
> > Correct Operations" or maybe "Correct Operation Verification".
> >
> > Perhaps change the title of Appendix A to just "Contributors", since
> > there is no other enumeration of contributors.
> >
> > 1.  Introduction
> >
> >   This document specifies a PCEP extension to signal that a particular
> >   group of LSPs should use diverse paths including the requested type
> > , ---------------------------------------^
> >   of diversity.
> >
> > Add a comma as indicated, to make it clear that "including" modifies
> > "signal" rather than "paths".
> >
> >   This document specifies a PCEP extension to signal that a particular
> >   group of LSPs should use diverse paths including the requested type
> >   of diversity.
> >
> > This implies that the "group" here is an instance of the association
> > "grouping" of the preceding paragraph, but the Intro doesn't state
> > that this document's grouping is an instance of the generic grouping
> > mechanism.  Perhaps
> >
> >   This document specifies a PCEP grouping to signal that ...
> >
> > 3.  Motivation
> >
> >   A customer may request
> >   that the operator provide two end-to-end disjoint paths across the
> >   IP/MPLS core.
> >
> > I suggest "the operator's IP/MPLS core".
> >
> > [There are various places where the document seems to be phrased from
> > the point of view of the network vendor rather than the point of view
> > of the endpoint/user.  The IETF convention is to focus on the endpoint
> > or user.]
> >
> >   The customer may use those paths as primary/backup or
> >   active/active.
> >
> > This phrasing uses an adjective as a noun.  I prefer:
> >
> >   The customer may use these paths with a primary/backup or an
> >   active/active configuration.
> >
> > --
> >
> >   Different level of disjointness may be offered:
> >
> > Perhaps "Different levels of disjointness can be defined:"
> >
> > 4.  Applicability
> >
> >   In the figure above, let us consider that the customer wants to have
> >   two disjoint paths between CE1/CE2 and CE3/CE4.
> >
> > I think that to make this accurate, you need to say "two disjoint
> > paths, one between CE1 and CE2 and one between CE3 and CE4."
> >
> > This section seems disorganized.  In particular, 2nd and 3rd paras
> > don't seem to add much.  Para 2 should be relocated to the SVEC
> > discussion (see "Major issues).  Para 3 seems to have two sentences of
> > background information (I think it's duplicated in section 1) and one
> > sentence that essentially states that this document defines a
> > "diversity group" of LSPs which is a type of the generalized "LSP
> > group".
> >
> >   Using PCEP, the PCC could indicate that a disjoint path computation
> >   is required, such indication should include disjointness parameters
> >   such as the type of disjointness, the disjoint group identifiers, and
> >   any customization parameters according to the configured local
> >   policy.  As mentioned previously, the extension described in
> >   [I-D.ietf-pce-association-group] is well suited to associate a set of
> >   LSPs with a particular disjoint-group.
> >
> > This touches on the right topics, but doesn't really connect them.
> > Also, don't use "could" for an action that this document defines how
> > to do.  You want to say something like
> >
> >   Using the extension to PCEP defined in this document, the PCC uses
> >   the [I-D.ietf-pce-association-group] extension to indicate that a
> >   group of LSPs are required to be disjoint; such indication should
> >   include disjointness parameters such as the type of disjointness,
> >   the disjoint group identifiers, and any customization parameters
> >   according to the configured local policy.
> >
> > --
> >
> >     Figure 2 - Sample use-cases for carrying disjoint-group over PCEP
> >                                  session
> >
> > In this figure, there seems to be a conflict in notation:  Case 1 uses
> > "PCC" seemingly in the same role for which Case 2 uses "PE 1" and "PE
> > 3".  The captions of both cases refer to "PCC".
> >
> > Also, the Case 1 and Case 2 captions run into each other.
> >
> >   Using the disjoint-group within a PCEP messages may have two purpose:
> >
> > Better would be "The disjoint-group within PCEP messages is used for:"
> >
> > 5.1.  Association Group
> >
> >   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.
> >
> > To be exact, you want to say "by adding all of them"; by default
> > "them" refers only to the "LSPs" that is the subject of the sentence.
> >
> >   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.  If the optional TLVs - Global
> >   Association Source or Extended Association ID - are included, then
> >   they are included in combination with mandatory fields to uniquely
> >   identify the association group.
> >
> > Use either "field" or "TLV" consistently (unless the TLVs really are
> > different from the "fields").
> >
> > This is awkward.  Better would be
> >
> >   As described in [I-D.ietf-pce-association-group], the association
> >   group is uniquely identified by the combination of these fields in
> >   the ASSOCIATION object:  Association Type, Association ID,
> >   Association Source, and (if present) Global Association Source or
> >   Extended Association ID.
> >
> >   ... This document defines a new ...
> >
> > Start a new para with this sentence.
> >
> >   o  Association type = TBD1 ("Disjointness Association Type") for
> >      Disjoint Association Group (DAG).
> >
> > This seems redundant, unless there exists another type that is also
> > associated with Disjoint Association Group (DAG).  Otherwise, you can
> > probably use
> >
> >   o  Association type = TBD1 Disjoint Association Type
> >
> > --
> >
> >   This capability exchange for the association type
> >   described in this document (i.e.  Disjointness Association Type) MUST
> >   be done before using the disjointness association.
> >
> > This can be simplified to
> >
> >   This capability exchange for Disjointness Association Type MUST
> >   be done before using the disjointness association.
> >
> > There seem to be multiple uses of "Disjoint Association Group (DAG)"
> > and "Disjointness Association Type (TBD1)", each of which states the
> > equivalence of two things.  You should be able to use one instance of
> > each.  Also, it's probably worth putting "Disjointness Association
> > Type" into section 2.
> >
> >   This association type is considered to be both dynamic and operator-
> >   configured in nature.
> >
> > It might be worth providing references for the meaning of "dynamic"
> > and "operator-configured".
> >
> >   A disjoint group can have two or more LSPs, [...]
> >
> > The significance of this clause to the rest of the sentence is
> > unclear; it seems obvious that a disjoint group needs at least two
> > members before it is significant.  However, as a matter of protocol,
> > can a group that has only one LSP be established?
> >
> >   The disjoint status is informed to the PCC.
> >
> > This seems to be describing how to report to the PCC that the
> > computation was not done as requested.  I think you want to provide
> > more details here, or a pointer to them.  In any case, "disjoint
> > status" isn't really a clear statement that the error condition will
> > be reported.
> >
> >   Associating a particular LSP to multiple disjoint groups is
> >   authorized from a protocol perspective, however there is no assurance
> >   that the PCE will be able to compute properly the multi-disjointness
> >   constraint.
> >
> > You want to state *much* more clearly what might happen.  This starts
> > with "A particular LSP MAY be associated to multiple disjoint groups,
> > but in that case, the PCE MAY ..."  What are the outer boundaries of
> > the results that the PCE MAY return?  Is the PCE required to provide
> > any particular error indication?
> >
> > 5.2.  Disjoint TLVs
> >
> > See "Major issues" regarding explaining that there is a separate copy
> > of DISJOINTNESS-CONFIGURATION-TLV for each LSP, and they needn't all
> > be the same.  All of the constraints described in the last paragraph
> > of section 5.5 should be moved to here, or perhaps section 5.1.
> >
> >    0                   1                   2                   3
> >    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> >   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >   |         Type = TBD2           |            Length             |
> >   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >   |                 Flags                               |T|P|S|N|L|
> >   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >
> > "Flags" should be "Reserved".
> >
> >      *  P (Shortest path) bit: when set, this indicates that the
> >         computed path of the LSP SHOULD satisfy all the constraints and
> >         objective functions first without considering the diversity
> >         constraint.  ...
> >
> > See "Major issues" for providing a separate paragraph to explain the
> > semantics of the "P" bit.
> >
> >      *  T (Strict disjointness) bit: when set, if disjoint paths cannot
> >         be found, PCE SHOULD return no path for LSPs that could not be
> >         be disjoint.  When unset, the PCE is allowed to relax
> >         disjointness by either applying a requested objective function
> >         (cf.  Section 5.4 below) or using any other behavior if no
> >         objective function is requested (e.g. using a lower disjoint
> >         type (link instead of node) or fully relaxing disjointness
> >         constraint).  Further see Section 5.6 for details.
> >
> > Instead of "or using any other behavior if no objective function is
> > requested" it would be more conventional to say "or, if no objective
> > function is requested, using local policy".
> >
> >   The DISJOINTNESS-STATUS-TLV uses the same format as the DISJOINTNESS-
> >   CONFIGURATION-TLV with a different type TBD3 (in the TLV).  The L, N,
> >   and S flags are set based if the computed path meet the disjointness
> >   criteria.
> >
> > Should be "... are set if the respective disjointness criterion was
> > requested and the computed paths meet it"
> >
> >   The P flag is set to indicate that the computed path is
> >   the shortest
> >
> > Surely "the shortest" needs further explanation -- the shortest path
> > that meets what constraints?  I suspect that this just reflects the P
> > flag in the CONFIGURATION-TLV -- is there a situation where it would
> > not?
> >
> >   and the T flag has no meaning in the DISJOINTNESS-
> >   STATUS-TLV and MUST NOT be set while sending and ignored on receipt.
> >
> > This clause should be a separate sentence.
> >
> >   Any new flag defined for the DISJOINTNESS-CONFIGURATION-TLV is be
> >   automatically applicable to the DISJOINTNESS-STATUS-TLV.
> >
> > This can't be correct, as there is no defined way to automatically
> > apply a new flag to DISJOINTNESS-STATUS-TLV.  (The previous paragraph
> > defines three separate semantics for flags in DISJOINTNESS-STATUS-TLV!)
> > But you can say:
> >
> >   Any document defining a new flag for the
> >   DISJOINTNESS-CONFIGURATION-TLV automatically defines a new flag
> >   with the same name and in the same location in
> >   DISJOINTNESS-STATUS-TLV; the semantics of the flag in
> >   DISJOINTNESS-STATUS-TLV MUST be specified in the document that
> >   specifies the flag in DISJOINTNESS-CONFIGURATION-TLV.
> >
> > 5.3.  Relationship to SVEC
> >
> >   The PCE would consider both the objects
> >
> > I think you mean MUST instead of "would".
> >
> >   In case no such path is possible (or the
> >   constraints are incompatible), the PCE MUST send a path computation
> >   reply (PCRep) with a NO-PATH object indicating path computation
> >   failure as per [RFC5440].
> >
> > Adding "(or the constraints are incompatible)" to "no such path is
> > possible" is redundant!
> >
> > This discussion is not clear to me.  Partly it is because I'm not
> > familiar with SVEC.  But I suspect that "synchronization of a set of
> > path computation requests" means that "the computations have to be
> > done together" and "dependent requests" means something similar to the
> > constraints between computed paths specified by the
> > DISJOINTNESS-CONFIGURATION-TLV.
> >
> > Is it possible to provide a comparison between the requirements
> > specified by SVEC and the requirements specified by
> > DISJOINTNESS-CONFIGURATION-TLV?
> >
> > Also, is there any specific relationship between the set of LSPs in an
> > SVEC and the set of LSPs in a disjoint-group?  What if multiple SVECs
> > and disjoint-groups overlap in a complex way?
> >
> > 5.4.  Disjointness Objective functions
> >
> >   To minimize the common shared resources (Node, Link or SRLG) between
> >   a set of paths during path computation three new OF-codes are
> >   proposed:
> >
> > Actually, the new codes are to allow specification of minimization of
> > shared resources.  Also, an RFC doesn't just proposed the codes, it
> > specifies them.  So this is better phrased:
> >
> >   Three new OF-codes allow specification of minimization of common
> >   shared resources (Node, Link or SRLG) between a set of paths during
> >   path computation:
> >
> > --
> >
> >   MSL
> >
> >   * Name:  Minimize the number of shared (common) Links.
> >
> >   * Objective Function Code:  TBD4
> >
> >   * Description:  Find a set of paths such that it passes through the
> >      least number of shared (common) links.
> >
> >   MSS
> >
> >   * Name:  Minimize the number of shared (common) SRLGs.
> >
> >   * Objective Function Code:  TBD5
> >
> >   * Description:  Find a set of paths such that it passes through the
> >      least number of shared (common) SRLGs.
> >
> >   MSN
> >
> >   * Name:  Minimize the number of shared (common) Nodes.
> >
> >   * Objective Function Code:  TBD6
> >
> >   * Description:  Find a set of paths such that it passes through the
> >      least number of shared (common) nodes.
> >
> > The use of "it" in the above definitions is unclear, since the paths
> > are "they".
> >
> > There are a number of similar but different ways that "the number of
> > shared" entities can be defined.  E.g., if four LSPs pass through a
> > single node, does that count as 1 (a single node is shared), 4 (four
> > LSPs pass through a node that is shared), 6 (six pairs of LSPs
> > conflict, in that the paths in the pair share a node)?  That is, are
> > we counting (and thus minimizing) shared entities, the number of LSPs
> > that violate the requested constraint, or the number of pairs of LSPs
> > that overlap in an unacceptable way?
> >
> >   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).
> >
> > I find it surprising that you've defined a specific error code for an
> > unexpected OF-code, but you don't apply that error code to the
> > situation where there are multiple OF-codes.  Instead, you have all
> > but the first silently ignored.
> >
> > There's also the intersection case where the first OF-code is
> > legitimate, but the second OF-code is not one in this set.  Does that
> > get an "Incompatible OF code" error, or is the second one just
> > ignored?
> >
> > I think it's cleaner to change TBD9 to "Incompatible OF-list for
> > disjoint-group" and require it for all of these cases.
> >
> >   [RFC5440] uses SVEC diversity flag for node, link or SRLG to describe
> >   the potential disjointness between the set of path computation
> >   requests used in PCEP protocol.
> >
> > This paragraph seems very important, but completely out of place here.
> > Might it fit better in section 5.3?
> >
> >   This document defines three new OF-codes to maximize diversity as
> >   much as possible, in other words, minimize the common shared
> >   resources (Node, Link or SRLG) between a set of paths.
> >
> > This paragraph seems redundant after the detailed discussion of these
> > codes.
> >
> > The remaining paragraphs of this section seem like they should go in
> > section 5.3.
> >
> > 5.5.  P Flag Considerations
> >
> > See above queries about the P flag.
> >
> >   As mentioned in Section 5.2, the P flag (when set) indicates that the
> >   computed path of the LSP SHOULD satisfies all constraints and
> >   objective functions first without considering the diversity
> >   constraint.
> >
> > There are complexities that don't seem to be addressed.  E.g., if
> > there are two LSPs with P=1 and two with P=0, I would expect that the
> > first two would be computed with full consideration of the
> > disjointness constraint between them.  So "without considering the
> > diversity constraint" really isn't a good description of what is
> > intended.
> >
> > 5.6.  Disjointness Computation Issues
> >
> >   Also, in case of network event leading to an impossible
> >   strict disjointness, the PCE MUST send a PCUpd message ...
> >
> > I think you want to start a new paragraph here and say
> >
> >   In case of a network event which makes it impossible to satisfy the
> >   constraints, the PCE MUST send a PCUpd message ...
> >
> > --
> >
> >   The
> >   actual level of disjointness computed by the PCE ...
> >
> > More correct to say
> >
> >   The
> >   actual level of disjointness of the paths computed by the PCE ...
> >
> > --
> >
> >   While the DISJOINTNESS-CONFIGURATION-TLV defines the
> >   expected level of disjointness required by configuration, the
> >   DISJOINTNESS-STATUS-TLV defines the actual level of disjointness
> >   computed.
> >
> > Better to s/expected/desired/ and s/actual/achieved/.
> >
> >   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.
> >
> > This para doesn't seem to make any difference.  As long as the T bit
> > is clear and the local policy allows complete relaxation of the
> > requested disjointness, this case is covered within the mechanisms of
> > this document.  And this case will be reported in a straightforward
> > way by a DISJOINTNESS-STATUS-TLV with all bits clear.
> >
> > 6.  Security Considerations
> >
> >   This document defines one new type for association ...
> >
> > Better "This document defines one new PCEP association type ..."
> >
> > After para 1, you probably want to add this case: "A spurious LSP can
> > have flags that are inconsistent with those of the legitimate LSPs of
> > the group and thus cause LSP allocation for the legitimate LSPs to
> > fail with an error."
> >
> >
> > _______________________________________________
> > Gen-art mailing list
> > [e-mail]
> > https://www.ietf.org/mailman/listinfo/gen-art
>