Hi Dale,
Thanks for detailed review. All the comments are addressed in version-12:
PSB inline[M.Negi] to your comments for details, further a working copy also maintained, any further comments I'll update and post new version.
Working copy: https://github.com/mahendnegi/ietf/blob/master/pce/assoc-diversity/draft-ietf-pce-association-diversity-13.txt
On Sun, Oct 13, 2019 at 6:34 AM Dale Worley via Datatracker <noreply@xxxxxxxx> 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.
[M Negi] Done
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.
[M Negi] Done
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.
[M Negi] Done, section 5.4.1
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.
[M Negi] Done
The running title is given as "ASSOC-DISJOINT". I think "PCEP
Diversity Constraint Signaling" would be clearer.
[M Negi] Done, PCEP-Diversity-Constraints-Signaling
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".
[M Negi] Done
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).
[M Negi] Done
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".
[M Negi] Done
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".
[M Negi] Done
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 ...
[M Negi] Done, changed to:
This document specifies a PCEP extension to signal that a set of LSPs
in a particular group should use diverse paths, including the requested type of diversity.
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".
[M Negi] Done
[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.
[M Negi] Done
--
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.
--
[M Negi] Done
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".
[M Negi] Done, changed to PCC1 and PCC2
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:"
[M Negi] Done
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.
[M Negi] Done
... This document defines a new ...
Start a new para with this sentence.
o Association type = TBD1 ("Disjointness Association Type") for
Disjoint Association Group (DAG).
[M Negi] Done
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
[M Negi] Done
--
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.
[M Negi] Done
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.
[M Negi] Done
This association type is considered to be both dynamic and operator-
configured in nature.
[M Negi] Done
It might be worth providing references for the meaning of "dynamic"
and "operator-configured".
[M Negi] Done
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?
[M Negi] Yes, during synh or configuration change and LSP can 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?
[M Negi] Done
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".
[M. Negi] Flags field is defined and a IANA registry created so that
future extension can add more flags.
future extension can add more flags.
* 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.
[M. Negi] Done
* 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".
[M Negi] Done
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"
[M Negi] Done
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?
[M Negi] Done
and the T flag has no meaning in the DISJOINTNESS-
STATUS-TLV and MUST NOT be set while sending and ignored on receipt.
[M Negi] Done
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.
[M Negi] Done
5.3. Relationship to SVEC
The PCE would consider both the objects
I think you mean MUST instead of "would".
[M Negi] Done
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?
[M.Negi] Done, section 5.4 explains
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:
[M.Negi] Phrased as:
This document defines three new OF-codes <xref target="Disjointness-objective-functions"/>
to maximize diversity as much as possible, in other words, new OF-codes
allow specification of minimization of common shared resources (Node, Link or SRLG) among 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".
[M.Negi] Done
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?
[M. Negi] In general its for path-protection with least common resource.
sharing minimized resources(node/link/srlg) give absolute protection
during any unwanted network event.
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.
[M.Negi] Done
[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?
[M.Negi] As part of major issue fix
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.
[M.Negi] Done
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.
[M.Negi] When multiple LSPs have P=1 then they might not
be disjoint among themselves. 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. Because if we consider disjointness among the LSP with p=1 they may not be shortest anymore!
be disjoint among themselves. 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. Because if we consider disjointness among the LSP with p=1 they may not be shortest anymore!
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 ...
[M.Negi] Done
--
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 ....
[M.Negi] Done
--
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/.
[M.Negi] Done
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.
[M.Negi] This is related to ignoring the whole object itself i.e as per draft-dhody-pce-stateful-pce-optional/
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."
[M.Negi] Done
