Re: [Gen-art] [Pce] Genart telechat review of draft-ietf-pce-gmpls-pcep-extensions-14

[Cyril Margaria <[e-mail]>]

Thanks for the review,

a new version has been posted addressing your comments.
Please also see inline

On Wed, 10 Apr 2019 at 13:47, Elwyn Davies via Datatracker <[e-mail]>
wrote:

> Reviewer: Elwyn Davies
> Review result: Almost Ready
>
> 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 wait for direction from your
> document shepherd or AD before posting a new version of the draft.
>
> For more information, please see the FAQ at
>
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
>
> Document: draft-ietf-pce-gmpls-pcep-extensions-14
> Reviewer: Elwyn Davies
> Review Date: 2019-04-10
> IETF LC End Date: 2018-10-29
> IESG Telechat date: 2019-04-11
>
> Summary:
> Almost ready, but with a large collection of nits in language and
> non-expansion
> of abbreviations. I am also concerned about the specification of behaviour
> in
> case PCC/PCEs with and without the extensions attempt to interact.  The
> requirements and behaviour are rather woolly, and are not fully covered by
> capability negotiations as the negotation capability itself is not in the
> original PCEP specification.
>
> Major issues:
> None
>
> Minor issues:
> Interacting with PCCs that do not support these GMPLS extensions: The
> draft is
> not very clear on interactions between PCCs that do support the extensions
> and
> ones that do not.  It is unclear whether a PCC that proposes to use the
> extensions must support the RFC 5088 or 5089 capability negotiation
> extensions
> and use them to determine if a PCEP exchange can use the extensions.  The
> text
> in para 1 of s2.1.2 appears to require that a node that does not support
> RFC
> 5088 or 5089 still has to understand that it has received the
> GMPLS-CAPABILITY
> type indicator and indicate a mismatch.  It seems to me that some
> additional
> explanation is needed to describe how mismatched PCC/PCEs understand the
> problem and deal with cases where a message with the new extensions is
> received
> (and presumably rejected) by a node that does not implement the extensions.
>
> [MC] The IGP-based discovery (RFC 5088 or 5089) are optional, and are
not capability negotiations. The Capability negotiation is done only
in PCEP, and the GMPLS-CAPABILITY TLV must be present from both peers
in order to make use of the extensions


> s9.2, RFC7025: Given the references to the requirements document for this
> work,
> I would consider RFC 7025 to be normative.
>
> [MC] 7025 is marked as Informational, so I am not sure it should
listed as normative.


> Nits/editorial comments:
>

[MC] All the nits have addressed


> General: s/e.g. /e.g., /g
>
Abstract: s/The Path Computation Element (PCE)/A Path Computation Element
> (PCE)/
>
> s1: Expand abbreviations OTN (Optical Transport Networks) and WSON
> (Wavelength
> Switched Optical Networks).
>
> s1, para 2: s/considered/addressed/, s/those application/these
> applications/
>
> s1.2, para 1: s/PCEP extension/PCEP extensions/, s/broken down in/broken
> down
> into/
>
> s1.2: Expand following acronyms/abbreviations on first occurrence: LSP,
> TE-LSP,
> L2SC, TDM, SONET, SDH, LSC [Query: Is LSC different from L2SC?], PCC, ERO
>
> s1.2, bullet 2: A reference for the G.709 standard is needed.
>
> s1.2 and s1.3.1, items (4) and (5): There doesn't seem to be a definition
> of
> Concatenation Number in any of the documents mentioned here or anywhere on
> the
> web.  I suspect it is supposed to be the number of streams that are
> concatenated but this needs to be properly defined or a reference provided.
>
> s1.2, bullet 5:  s/Label restriction/label restriction/.  I take it this
> refers
> to the use of Label Set objects as described in RFC 3473.  If so please
> add a
> reference.  If not lease provide the appropriate reference.
>
> s1.3.1: Expand following acronyms/abbreviations on first occurrence:
> TE-LSP,
> ODU, IRO, XRO, RRO, LSPA
>
> s1.3.1, item (4): s/Its scoped/It is scoped/ [English language note: 'Its'
> is
> the possessive pronoun derived from the third person singular impersonal
> pronoun 'it', whereas "It's" is a contraction of 'it is' that is not
> normally
> used in formal documents.]
>
> s1.3.1, item (4):
>
> > related to the BANDWIDTH object in MPLS networks
> I assume this relates to the BANDWIDTH object in RFC 5440 - please add a
> reference.
>
> s1.3.2, item (1):  The previous two comments on s.1.3.1, item (4) apply
> also to
> this item.
>
> s1.4:
>
> OLD:
>
>  1.4   GMPLS Support and Limitation of Base PCEP Objects
>
>    The support for requirements [RFC7025] is summarized in Table 1 and
>    Table 2
>
> NEW:
>
> 1.4   Existng Support for GMPLS in Base PCEP Objects and its Limitations
>
>    The support provided by specifications in [RFC8282] and [RFC5440]  for
> the
>    requirements listed in [RFC7025] is summarized in Table 1 and Table 2.
> In
>    some cases the support may not be complete, as noted, and additional
> support
>    need to be provided in this specification.
>
> ENDS
>
> s1.4, RFC 5440 bullets, ERO:  A reference for the RSVP specification
> covering
> ERO is needed.
>
> s1.4, XRO object. 1st bullet: Expand SRLG.
>
> s1.4, SWITCH-LAYER bullet: s/address/addresses/
>
> s1.4, list of coverage gaps: Expand NVC.
>
> s1.4, added functionality needed: s/to cover the gap/to cover the gaps/
>
> s2: Expand PCReq and PCRep message names to reflect names in RFC 5440.
>
> s2.1.2:
>
> > Moreover, in case that the PCC does not receive the
> >    GMPLS-CAPABILITY TLV it is RECOMMENDED that the PCC does not make use
> >    of the objects and TLVs defined in this document.
> I would have thought this ought to have been a MUST (when communicating
> with
> the PCC that didn't support the extensions.
>
> [MC]  The PCC is able to mandate or not the processing of objects on
per-request basic. To make it simpler though the capability negotiation has
been made mandatory (both peer must advertize the capability)


> s2.1.2: s/OPEN message/Open message/ in (at least) 3 places - for
> consistency
> and to match RFC 5440
>
> s2.1.2, GMPS-CAPABILITY TLV spec
>
> >    IANA has allocated value TBA-1 from the "PCEP TLV Type Indicators"
> >    sub-registry, as documented in Section 5.3 ("New PCEP TLVs").  The
> >    description is "GMPLS-CAPABILITY".  Its format is shown in the
> >    following figure.
> >
> >      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=14         |           Length              |
> >     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >     |                             Flags                             |
> >     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> Should 'Type=14' in the diagram be 'Type=TBA-1'?
>
> s2.2:
>
> > Depending
> >    on policies or switching layer, it can be necessary for the PCC to
> >    use explicit label control or expect explicit link,
>
> ^^^^^^^^^^^^^^^^^^^^^^^
>
> I cannot parse the marked phrase 'or expect explicit link'.  Please
> clarify.
>
>  [MC] it should read 'or explicit link ids'. The PCC may need to know
 the exact details of the path down to the label or list of links (no
 Loose hops)

s2.2, next to last para: s/requested route granularity/requested routing
> granularity/
>
> [MC] The goal is to express the granularity of the information in the
returned ERO, not on how the PCE does the routing.


> s2.3, para 1: s/to allow to express/to allow the object to express/, s/set
> of
> encoding/set of encodings/
>
> s2.3, para at top of page 12: s/The Bw Spec Type correspond to/The Bw Spec
> Type
> corresponds to/
>
> s2.3, Table 4:Expand MEF, SSON on first occurrence.
>
> s2.3, next to last para:
>
> OLD:
>
>   The Object Type TBA-3 MAY be used instead of object type 2
>    to indicate the existing TE-LSP bandwidth.  A PCC that requested a
>    path with a BANDWIDTH object of object type 1 SHOULD use object type
>    2 to represent the existing TE-LSP BANDWIDTH.
>
> NEW:
>
>    The Object Type TBA-3 MAY be used instead of the previously specified
> object
>    type 2 to indicate the existing TE-LSP bandwidth originally specified
> with
>    object type TBA-2. A PCC that requested a path with a BANDWIDTH object
> of
>    object type 1 SHOULD use object type 2 to represent the existing TE-LSP
>    BANDWIDTH.
>
> ENDS
>
> It is not clear to me why this is a SHOULD and not a MUST.
>
> [MC] I think we were a bit too permissive in the initial iterations, a MUST
is better


> s2.4, para below the format: s/MAY also include TLV different from the PCEP
> TLVs./MAY also include TLVs different from the basic PCEP TLVs./
>
> s2.4, para at top of page 14:
>
> >  The encoding of the fields Min Bandwidth Spec and Min Reverse
> >    Bandwidth Spec is the same as in RSVP-TE SENDER_TSPEC object, it can
> >    be found in Table 4 from Section 2.3.
> Might be helpful to add (after Section 2.3) 'of this document' - the use of
> RSVP-TE immediately before this got me thinking this is in some RSVP-TE
> document.
>
> s2.5, last bullet on page 14: s/constraints in/constraints on/
>
> s2.5.1, para 7 (one after bullet #5):  s/This TLVs express/These TLVs
> express/
> (probably, but could be 'This TLV expresses' as I think this applies to
> just
> the Label set TLV.)
>
> s2.5.1, para 7: s/label set allows to indicate which label/label set allows
> indication of which labels/
>
> s2.5.1, para 7: s/restricting then in GMPLS the possible label ranges on
> the
> interface/consequently restricting  the possible label ranges on the
> interface
> in GMPLS/
>
> s2.5.1: Expand P2MP on first occurrence.
>
> s2.5.1, para after Table 5: s/Value TBD/Value TBA-14/
>

[MC] Agree,  it should be TBA-15 (Unsupported endpoint type in END-POINTS
Generalized Endpoint object type)


> s2.5.1, page 17, para after generalized-endpoint-tlvs grammar: Suggest
>
> OLD:
>
>    For endpoint type Point-to-Multipoint, several endpoint objects MAY
>    be present in the message and each represents a leave, exact meaning
>    depend on the endpoint type defined of the object.
>
> NEW:
>
>    For endpoint type Point-to-Multipoint, several endpoint objects MAY
>    be present in the message and each represents a leaf of the P2MP tree,
>    with the exact meaning depending on the endpoint type defined for the
> object.
> ENDS
>
> s2.5.1, last para on page 17: For consistency, the (new) vlaue of the Type
> 4
> error should be specified here (TBA-15, i think).
>
> s2.5.2.x, last para in each case: s/responded/returned/
>
> s2.5.2.3, para 1 s/as follow/as follows/
>
> s2.5.2.4, paa 1: s/The LABEL-REQUEST TLV use/The LABEL-REQUEST TLV uses/
>
> s2.5.2.4, para 1: Suggest using fully expanded name of G-PID - Generalized
> Payload Identifier.
>
> s2.5.2.4, para 2: Suggest adding reference(s) for where the definitions of
> Tspec and FlowSpec can be found.
>
> s2.5.2.5, para 1: s/Those TLV/These TLVs/, s/responded/returned/, s/as
> follow/as follows/
>
> s2.5.2.5, first bullet: s/set of label/set of labels/
>
> s2.5.2.5, para after diagram: s/Those parameters/These parameters/
>
> s2.5.2.5:  There are two more or less equivalent descriptions of the
> functions
> of the U, O and L flag bits.  For avadance of error, it would be better if
> the
> first summary was removed and the reader referred to the full description
> after
> the diagram.  This would mean adding the text 'following the semantic of
> SUGGESTED_LABEL defined by  [RFC3471]'  to the full description.
>
> s2.5.2.5, last para on page 20:
>
> > At most 2 LABEL_SET TLVs MUST be present with
> >    the O bit set, at most one with the U bit set and at most one with
> >    the U bit cleared.
> Since all LABEL_SET TLVs contain a U bit field, I read the above text as
> limiting the number of LABEL_SETs to two (it isn't clear that the U
> constraint
> is a sub-constraint after the O constraint).  I think what is meant is
>
> > At most 2 LABEL_SET TLVs MAY be present with
> >    the O bit set, with at most one of these having the U bit set and
> >    at most one of these having the U bit cleared.
> s2.5.2.5, last 3 paras: For consistency the new error values TBA-x should
> be
> specified.
>
> s2.6, para 1: s/allows to include label definition/allows the inclusion of
> a
> label definition/
>
> s2.7, para 1: s/allows to exclude labels/allows the exclusion of certan
> labels/
>
s2.7: More information about U, C-Type and Label is allegedly found in RFC
> 3471.  I can't find a reference to C-Type in that document, and it is
> unclear
> which part of the document has the Label info.  Is this a miatake?
>
> [MC] The reference should be RFC3473 for th C-Tye, the U bit is
described in RFC3471 section 6.1
The Label field is technology-dependent and defined in RFC3471, the
per-technology labels are defined in the technology-specific RFCs.


> s2.8, para 1: s/fulfill/fulfil/
>
> s2.8: Pointers to the relevant sections in RFC 4872 and 4873 would help.
>
> s2.9: s/The NO-PATH object MAY carries/The NO-PATH object MAY carry/,
> s/like/such as/, s/Few/Several/, s/and no/but no/
>
> s3, para 1: s/few error/several error/, s/Error- values/Error-values/
>
> s3, para 2: s/but not path found/but the PCe is unable to find a path/,
> s/NO-PATH is to be used/the NO-PATH error is to be used/
>
> s4.1, last para: s/Those parameters configuration are/The configuration of
> the
> above parameters is/
>
> s4.2: s/This document does not introduces new ERO sub object,/This document
> does not introduce any new ERO sub objects, so that the/
>
> s4.4: s/should be covered by/should satisfy/
>
> s5:  It would be worth adding a note to IANA that TBA-11 is not used
> (assuming
> this is not a bug).
>
> s5.1 and s5.3: s/(section /(/ (removing duplicate word 'section') in
> several
> places
>
> s5.2, para 2: s/qualities/attributes/
>
> s6, para 1: s/amount/volume/
>
> s6, first bullet: s/The answer can make that the LSP traverses/The
> resulting
> LSP might be arranged to traverse/
>
> s6, first bullet: s/the answer can omit/the rsulting LSP can omit/, s/the
> answer can lead to provide a LSP/the result can lead to the creation of an
> LSP/
>
> _______________________________________________
> Pce mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/pce
>