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

[Dale Worley via Datatracker <[e-mail]>]

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."