Re: [Pce] PCE WG Last Call on draft-ietf-pce-pcep-domain-sequence-07

[Dhruv Dhody <[e-mail]>]

Hi Adrian, 

Thank you for a detailed set of comments as well as text suggestions. 
I have handled most of your comments, request you/WG to have a look in the diff attached.  
Some comments need confirmation, and hence open. 

Regards,
Dhruv

> -----Original Message-----
> From: Pce [mailto:[e-mail]] On Behalf Of Adrian Farrel
> Sent: 24 April 2015 01:12
> To: [e-mail]
> Subject: Re: [Pce] PCE WG Last Call on draft-ietf-pce-pcep-domain-sequence-07
> 
> Hi,
> 
> While I have frequently questioned the need for this function in an IRO, I
> have done a quick review of this document. I don't guarantee to have found
> all of the issues, but I do conclude that the document is not ready for
> publication as an RFC.

I hope with these changes, you would be okay with moving the document forward :)

> 
> Cheers,
> Adrian
> 
> ===
> 
> It seems to me that the text here treats the domain sequence as a separate
> thing rather than just some new IRO subobjects that can be included in a
> PCReq. This shows in some of my comments below. Perhaps you need some more
> words to explain how to interleave the normal subobjects with the new ones.

I see, once the I-D was suggesting a new IRO type but that is not the case now. I have added some text to clarify this.
Please check and let me know if further changes are needed. 

> 
> ---
> 
> It is fine that this is an experimental document, but I do wonder: what the
> experiment is; how to confine the experiment so it doesn't impact other nodes
> or networks (see my comments on 3.4.3 and 3.5.1.2; what I need to do to all
> nodes in the experimental network; how I judge the success of the experiment.
> 

I have added a section -

1.1.  Scope

   The procedures described in this document are experimental.  The
   experiment is intended to enable research for the usage of Domain-
   Sequence at the PCEs for inter-domain paths.  For this purpose this
   document specify new domain subobjects as well as how they
   incorporate with existing subobjects to represent a Domain-Sequence.

   This document does not change the procedures for handling existing
   subobjects in PCEP.

   When the result of implementation and deployment are available, this
   document will be updated and refined, and then be moved from
   Experimental to Standard Track.

> ---
> 
> I question the inclusion of the word "shortest" in the Abstract, and I don't
> find it repeated in the Introduction where I looked to find a citation that
> indicated that it is a "key requirement". In fact, I think that the term
> "shortest" is in some senses conflicting with the concept of including other
> constraints. I would solve this by accepting that "as short as possible given
> the other constraints" is itself just a constraint. Thus...
> 
> OLD
>    The ability to compute shortest constrained Traffic Engineering Label NEW
>    The ability to compute constrained Traffic Engineering Label END
> 
> ...and...
> 
> OLD
>    to compute inter-domain constrained
>    shortest paths across a predetermined sequence of domains .
> NEW
>    to compute inter-domain constrained
>    paths across a predetermined sequence of domains.
> END
> 
> And in 3.4.3
> OLD
>    A PCC builds an IRO to encode the Domain-Sequence, so that the
>    cooperating PCEs should compute an inter-domain shortest constrained
>    path across the specified sequence of domains.
> NEW
>    A PCC builds an IRO to encode the Domain-Sequence, so that the
>    cooperating PCEs should compute an inter-domain constrained
>    path across the specified sequence of domains.
> END
> 

I hear you, but RFC5441 does use the term "shortest constrained path". 
I can either -
- leave as it is, as there is precedence. 
- add text in Introduction, with reference to RFC5441. 
- remove "shortest" as suggested above.

Status: Open

> ---
> 
> Section 1 has some duplication. I think you can delete:
> 
>                                              A PCE determines the next
>    PCE to forward the request based on the Domain-Sequence.  In a multi-
>    domain path computation, a Path Computation Client (PCC) MAY indicate
>    the sequence of domains to be traversed using the Include Route
>    Object (IRO) defined in [RFC5440].
> 
>    When the sequence of domains is not known in advance, the
>    Hierarchical PCE (H-PCE) [RFC6805] architecture and mechanisms can be
>    used to determine the Domain-Sequence.
> 
> And move:
> 
>    This document defines a standard way to represent and encode a
>    Domain-Sequence in various scenarios including P2P LSP, P2MP LSP, and
>    use of H-PCE.
> 
> ... to later in the section.
> 
> This also removes two problems with:
> 
>    In a multi-
>    domain path computation, a Path Computation Client (PCC) MAY indicate
>    the sequence of domains to be traversed using the Include Route
>    Object (IRO) defined in [RFC5440].
> 
> 
> 1. I don't think the upper case "MAY" is necessary or appropriate.
> 
> 2. I think that 5440 only gives you half of what you want, which is why
>    you are writing this document! But you actually catch and explain
>    this in the paragraphs that follow.
> 

Understood & done!  

> ---
> 
> The 2119 language in Section 3.2 doesn't seem right. You are describing how
> things are, not how things have to be. I think you can use just normal
> language.
> 
> It also doesn't seem right in 3.3 where you are describing how things work,
> not what implementations have to do.
> 

Agreed, thanks for catching this! 

> ---
> 
> There seems to be some duplication in section 3.3. For example,
> 
>    o  Include Route Object (IRO): As per [RFC5440], used to specify set
>       of network elements that MUST be traversed.  The subobjects in IRO
>       are used to specify the Domain-Sequence that MUST be traversed to
>       reach the destination.
> 

Ack. 

> ---
> 
> In 3.4.1 you are repeating some material that isn't really needed.
> 
>    The following subobject types are used in IRO.
> 
>                 Type   Subobject
>                  1     IPv4 prefix
>                  2     IPv6 prefix
>                  4     Unnumbered Interface ID
>                  32    Autonomous system number (2 Byte)
>                  33    Explicit Exclusion (EXRS)
> 

Ack. 

> ---
> 
> In 3.4.1.2, I don't understand why you need two sub-object types. You could
> certainly handle the encoding within the same sub-object so you are
> presumably worried about distinguishing between an OSPF area ID and an IS-IS
> area ID. I suppose that there is a faint chance that one AS is running two
> IGPs at once and the administrator has decided to give the same ID to two
> areas, one from each IGP. Is that what you're worried about?
> 

Since the format of the area ID are different in the two routing protocols; as well as we already have different TLV for OSPF and ISIS area-id in RSVP-TE in RFC4920, we feel that this is a better encoding option. 

Status: Open

> ---
> 
> 3.4.1.2 gives [ISO10589] as a reference for the definition of an area ID. It
> surprised me that we don't have an IETF reference for this. Can you check
> with the chairs of the ISIS WG that this is the right reference.
> 

[ISO10589] is also republished as RFC 1142 (historic). I can use that instead, but even RFC5089 uses the [ISO] reference. Anyways I will shoot a mail to WG chairs to get a confirmation. 

Status: Open

> ---
> 
> In 3.4.1.2 for the IS-IS variant, you have...
> 
>    Length:  Variable.  As per [RFC3209], the total length of the
>       subobject in bytes, including the L, Type and Length fields.  The
>       Length MUST be at least 4, and MUST be a multiple of 4.
> 
> Wouldn't a length of 4 mean that there was no area ID included?

Updated. 

> 
> ---
> 
> 3.4.3 lists four references for IRO subobjects: [RFC3209], [RFC3473],
> [RFC3477], and [RFC4874]. But RFC 3473 is not listed at
> http://www.iana.org/assignments/pcep/pcep.xhtml#iro-subobject
> 

Oops! Copy-paste error! Fixed. 

> ---
> 
> In 3.4.3
> 
>    If a PCE encounters a subobject that it does not support or
>    recognize, it MUST act according to the setting of the L-bit in the
>    subobject.  If the L-bit is clear, the PCE MUST respond with a PCErr
>    with Error-Type TBD4 "Unrecognized subobject" and set the Error-Value
>    to the subobject type code.  If the L-bit is set, the PCE MAY respond
>    with a PCErr as already stated or MAY ignore the subobject: this
>    choice is a local policy decision.
> 
> This links to 5.2.
> 
> You are defining behavior in this I-D for a non-compliant node. But a non-
> compliant node doesn't support this I-D!
> 
> That is, an implementation of 5440 will look at the received IRO and see the
> unknown subobject and act according to 5440 not according to this document.
> 
> You have to ask yourselves:
> - What does my 5440 implementation do with an unknown subobject?
> - Is that behavior documented and do I simply need to point to that?
> - Do we need an update to 5440 to say how to handle unknown subobjects?
> 

Understood. I have updated the text, taking a cue from 5521 where EXRS got added. 
The text now says - 

   If a PCE receives an IRO in a Path Computation request (PCReq)
   message that contains subobjects defined in this document, that it
   does not recognize, it will respond according to the rules for a
   malformed object as per [RFC5440].  The PCE MAY also include the IRO
   in the PCErr to indicate in which case the IRO SHOULD be terminated
   immediately after the unrecognized subobject. 

> ---
> 
> In 3.4.3
> 
>    Note that a particular domain in the Domain-Sequence can be
>    identified by :-
> 
>    o  A single IGP Area: Only the IGP (OSPF or ISIS) Area subobject is
>       used to identify the next domain.  (Refer Figure 1)
> 
>    o  A single AS: Only the AS subobject is used to identify the next
>       domain.  (Refer Figure 2)
> 
>    o  Both an AS and an IGP Area: AS and Area in combination are used to
>       identify the next domain.  In this case the order is AS Subobject
>       followed by Area.  (Refer Figure 3)
> 
> This is not clear. If I identify two domains in the sequence, the first with
> an AS subobject and the second with an Area subobject, how is this
> distinguished from the third case that you list?
> 
> I think you have to clarify how this works. Isn't it the case that an area ID
> can only ever have context within a specific AS? Isn't it the case that if
> you supply an AS followed by an area you are not controlling the entry area
> to the AS? Does all that actually mean that you need the area subobject to
> carry the AS number?
> 

I have removed this text. 
Text as per another comment which describes the relationship between subobjects is added in its place. 

> ---
> 
> 3.4.3
> 
>    The Subobjects representing an internal node, a Boundary Node or an
>    Inter-AS-Link MAY also influence the selection of the path.
> 
> I guess you mean "MAY be included by a PCC to influence the selection of the
> path".
> 

Agreed, the sentence is removed as part of handling other comment. 


> ---
> 
> In 3.5 you are repeating some material that isn't really needed because it is
> already in other documents and the IANA registry.
> 
>    The following subobject types are defined to be used in XRO as
>    defined in [RFC3209], [RFC3477], [RFC4874], and [RFC5521].
> 
> 
>                 Type   Subobject
>                  1     IPv4 prefix
>                  2     IPv6 prefix
>                  4     Unnumbered Interface ID
>                  32    Autonomous system number (2 Byte)
>                  34    SRLG
>                  64    IPv4 Path Key
>                  65    IPv6 Path Key
> 
> But, anyway, it should be 5520 not 5521.
> 

Ack. 

> ---
> 
> For 3.5.1.2 I have the same questions as for 3.4.1.2

See the explanation for 3.4.1.2 above. 

> 
> ---
> 
> 3.5.1.2 has
> 
>    If a PCE that supports XRO and encounters a subobject that it does
>    not support or recognize, it MUST act according to the setting of the
>    X-bit in the subobject.  If the X-bit is clear, the PCE MUST respond
>    with a PCErr with Error-Type TBD4 "Unrecognized subobject" and set
>    the Error-Value to the subobject type code.  If the X-bit is set, the
>    PCE MAY respond with a PCErr as already stated or MAY ignore the
>    subobject: this choice is a local policy decision.
> 
> Similar to 3.4.3...
> 
> This links to 5.2.
> 
> You are defining behavior in this I-D for a non-compliant node. But a non-
> compliant node doesn't support this I-D!
> 
> That is, an implementation of 5440 will look at the received IRO and see the
> unknown subobject and act according to 5440 not according to this document.
> 
> You have to ask yourselves:
> - What does my 5440 implementation do with an unknown subobject?
> - Is that behavior documented and do I simply need to point to that?
> - Do we need an update to 5440 to say how to handle unknown subobjects?
> 
> ---

Similar to 3.4.2, I have updated the text. 

> 
> 3.7 lead me to look at 5.1.
> 
> 5.1 is confusing...
> 
>    The "PCEP Parameters" registry contains a subregistry "PCEP Objects"
>    with an entry for the Include Route Object (IRO), Exclude Route
>    Object (XRO) and Explicit Route Object (ERO).  IANA is requested to
>    add further subobjects as follows:
> 
>        7  ERO
>        10 IRO
>        17 XRO
> 
>        Subobject Type                          Reference
>        TBD1      4 byte AS number              [This I.D.]
>        TBD2      OSPF Area ID                  [This I.D.]
>        TBD3      IS-IS Area ID                 [This I.D.]
> 
> I think you mean to ask IANA to make the same three assignments from all
> three registries. This text:
> - doesn't make that clear
> - doesn't identify the registries correctly
> - there is no PCEP ERO registry!
> 
> You probably want...
> 
>    IANA maintains the "Path Computation Element Protocol (PCEP) Numbers"
>    at http://www.iana.org/assignments/pcep/pcep.xhtml. Within this
>    registry IANA maintains two sub-registries:
> 
>    - "IRO Subobjects"
>      http://www.iana.org/assignments/pcep/pcep.xhtml#iro-subobject
>    - "XRO Subobjects"
>      http://www.iana.org/assignments/pcep/pcep.xhtml#xro-subobject
> 
>    IANA is requested to make identical additions to these registries as
>    follows:
> 
>        Subobject Type                          Reference
>        TBD1      4 byte AS number              [This I.D.]
>        TBD2      OSPF Area ID                  [This I.D.]
>        TBD3      IS-IS Area ID                 [This I.D.]
> 
> But you will also need to change the text in 3.7 because you can't make
> additions to the PCEP ERO subobject registry when it doesn't exist! And you
> can't make changes to the RSVP ERO subobject registry in the PCE working
> group.
> 

I was trying to follow 5520 and 5553; but I have updated the section as suggested by you. 

> ---
> 
> In 3.7 you have..
> 
>    The format of the new ERO subobjects is similar to new IRO
>    subobjects, refer Section 3.4.
> 
> "similar"?
> 

Sentence is removed. 

> ---
> 
> Figure 1 has some extra pipes (|) in the ABR between area 2 and area 0.
> 

Thanks for catching it. Updated.  

> ---
> 
> 4.1.1 says...
> 
>    AS is optional and it MAY be skipped.
> 
> I don't think you mean "skipped". If the AS is present it MUST be processed
> because it has to be checked.
> 

The text is updated. 

> Additionally, you have already told us that the examples are not normative.
> So:
> 
> - you must not use 2119 language
> - you need to check that the normative text *is* present in section 3
> 

The text is updated.

> So in 4.2.1 I think you mean...
> 
>    AS is optional. If it is not present, the receiver will deduce that
>    there is no change in the AS.
> 
> But somewhere you will need to clarify a few things (I think I asked
> questions about some of this as I read section 3) about the order of
> subobjects and how they are interleaved in the three objects.
> 
> Essentially you have...
> - The Area subobject is optional.
> - The AS subobject is optional.
> - If an Area subobject is not preceded by an AS subobject then the
>   receiver MUST act as though there is no change in AS from the
>   previous subobject.
> - If an AS subobject is present then it changes the AS for all
>   subsequent subobjects that do not themselves change the AS.
> - Subobjects that may change the AS are:
>   - IP addresses that are present in another AS
>   - Unnumbered interfaces that are present in another AS
>   - AS numbers
>   - Path key identifiers that expand to path segments that include
>     changes to the AS
> - AS and Area subobjects may be interspersed with other subobjects
>   without change to the previously specified processing.
> 
> Whether this goes in section 3 (which feels natural) or in section 4 (where
> you have any number of other small normative items) is up to you, but if it
> is in section 4 you really need to make much clearer that I need to read the
> section!
> 

Thank you for the text, I have expanded and updated in section 3. 
Note that I have added some text for area subobject but removed the PKS (as that is not an IRO subobject). 

The new text in section 3 say - 

   Following processing rules apply -

   o  The Area subobject is optional.

   o  The AS subobject is optional.

   o  If an Area subobject is present then it changes the Area for all
      subsequent subobjects that do not change the area themselves.
      Subobjects that may change the Area are:

      *  IP addresses that are present in another Area (via IPv4/IPv6
         subobject)

      *  Unnumbered interfaces that are present in another Area (via
         Unnumbered Interface ID subobject)

      *  Area ID (via OSPF/ISIS area subobjects)

      *  AS number of another AS (via AS subobjects)

   o  If an Area subobject is not preceded by an AS subobject then the
      receiver MUST act as though there is no change in AS from the
      previous subobject.

   o  If an AS subobject is present then it changes the AS for all
      subsequent subobjects that do not change the AS themselves.
      Subobjects that may change the AS are:

      *  IP addresses that are present in another AS (via IPv4/IPv6
         subobject)

      *  Unnumbered interfaces that are present in another AS (via
         Unnumbered Interface ID subobject)

      *  AS number (via AS subobjects)

   o  AS and Area subobjects may be interspersed with other subobjects
      without change to the previously specified processing of those
      subobjects in the IRO.

> ---
> 
> Figure 3 is "interesting". Aren't AS interconnects always provided in Area 0,
> or am I embarrassing myself?
> 

It usually is, I have added text to re-iterate that the figure is for illustrative purpose only and the usual convention is for AS interconnects to Area 0.
Other option is to remove this example altogether.  

Status: Open.

> ---
> 
> The option for encoding the inter-AS link in section 4.3 is legitimate, but
> excessive. You do not need to show both link ends since it would be
> impossible to use one end of the link without using the other end!
> 

Updated. 

> ---
> 
> I think it is fine for section 4.5 to make a reference to 7334, but it is
> wrong to do so in a way that suggests that 7334 is *the* way to do inter-
> domain P2MP path computation since that RFC describes an experiment.
> 
> Similarly, [PCE-P2MP-PER-DEST] is also experimental (and has expired!).
> 
> In both cases, this is notwithstanding that this document is an experiment
> itself.

Updated. 

> 
> ---
> 
> Figure 4 has the same typo in the ABR as in Figure 1.
> 

Ack. 

> ---
> 
> In 4.6 "ERO Object" should be EROO :-)
>

Oops! Fixed. 

> ---
> 
> In 4.6 you have:
> 
>    The ERO can contain an ordered sequence of
>    subobjects such as AS and Area (OSPF/ISIS) subobjects.  In this case,
>    the Domain-Sequence appear as:
> 
> 
>      +---------+ +---------+ +---------+ +---------+
>      |ERO      | |Sub      | |Sub      | |Sub      |
>      |Object   | |Object   | |Object   | |Object   |
>      |Header   | |Area 2   | |Area 0   | |Area 4   |
>      |         | |         | |         | |         |
>      |         | |         | |         | |         |
>      +---------+ +---------+ +---------+ +---------+
> 
> 
>      +---------+ +---------+ +---------+ +---------+ +---------+
>      |ERO      | |Sub      | |Sub      | |Sub      | |Sub      |
>      |Object   | |Object AS| |Object   | |Object   | |Object   |
>      |Header   | |100      | |Area 2   | |Area 0   | |Area 4   |
>      |         | |         | |         | |         | |         |
>      |         | |         | |         | |         | |         |
>      +---------+ +---------+ +---------+ +---------+ +---------+
> 
> This is confusing. I think you mean to indicate a choice.
> 
> However, I have no clear understanding of why this is a good example
> since:
> - the Ingress PCE 'PCE(1)' would more logically be in Area 1
> - the ERO could contain the identifiers of the ABRs
> - Area 2 is presumably taken as read by the requesting PCE so doesn't
>   need to be included
> - if Area 2 needs to connect to anything out of area it has to go
>   through Area 0, so including that is superfluous
> 

I see. 
I have updated the section and describe based on the example in 6805 instead. 

> ---
> 
> Section 4.7 uses "SHOULD". Under what circumstances MAY the PCE-Sequence have
> lower or equal precedence?
> 

Hmm can't think of any, looks like "MUST" is needed in this case. 
Updated. 

> You also say:
> 
>    Note
>    that Domain-Sequence IRO constraints should still be checked as per
>    the rules of processing IRO.
> 
> ...Why don't I have to check the XRO and EXRS?
>

True, removed the statement.  


> ---
> 
> 4.8 has
> 
>    [DOMAIN-SUBOBJ] extends the notion of abstract nodes by adding new
>    subobjects for IGP Areas and 4-byte AS numbers.  These subobjects MAY
>    be included in Explicit Route Object (ERO), Exclude Route object
>    (XRO) or Explicit Exclusion Route Subobject (EXRS) in RSVP-TE.
> 
> Is it right to provide normative language for RSVP-TE in this document?
> 

Fixed. 

> ---
> 
> This is a bit odd...
> 
> 7.6.  Impact On Network Operations
> 
>    Mechanisms defined in this document do not have any impact on network
>    operations in addition to those already listed in [RFC5440].
> 
> I thought the whole point was to impact network operations!
> 

I see your point. Have added text.

7.6.  Impact On Network Operations

   The mechanisms described in this document can provide the operator
   with the ability to exert control by inclusion or exclusion of domain
   subobjects.  There may be some scaling benefit when a single domain
   subobject may substitute for many subobjects and can reduce the
   overall message size and processing.

   Backward compatibility issues associated with the new subobjects
   arise when a PCE does not recognize them, in which case PCE responds
   according to the rules for a malformed object as per [RFC5440].  For
   successful operations the PCEs in the network would need to be
   upgraded.

> ---
> 
> While it is OK for this document and [DOMAIN-SUBOBJ] to only have
> informational references to each other, I think it would be sensible to try
> to tie them together in the RFC Editor Queue so that they get considered
> together, so that any issues can be synchronised, and so that there are no
> surprises with code points.
> 
> I guess the chairs can talk to the TEAS chairs and the ADs to make this
> happen, or you could upgrade the references to be normative.
> 
> Maybe a normative reference would be more in keeping with the revision to 5.1
> and 4.8 I think you should make because there is no PCEP ERO subobject
> registry.
> 

Agreed and done. 

Thanks again for a detailed review. 

The working copy is also attached, and can be uploaded post WGLC. 

Regards,
Dhruv 

> > -----Original Message-----
> > From: Pce [mailto:[e-mail]] On Behalf Of
> > [e-mail]
> > Sent: 15 April 2015 17:43
> > To: [e-mail]
> > Subject: [Pce] PCE WG Last Call on
> > draft-ietf-pce-pcep-domain-sequence-07
> >
> > Dear PCE & TEAS WGs,
> >
> > This message initiates a 2-week WG last call on
> > draft-ietf-pce-pcep-domain-sequence-07. Please send your comments to
> > the PCE mailing list by Wednesday April 29.
> >
> > Thanks,
> >
> > JP & Julien
> >
> >
> > ______________________________________________________________
> > ___________________________________________________________
> >
> > Ce message et ses pieces jointes peuvent contenir des informations
> > confidentielles ou privilegiees et ne doivent donc pas etre diffuses,
> > exploites ou copies sans autorisation. Si vous avez recu ce message
> > par erreur, veuillez le signaler a l'expediteur et le detruire ainsi
> > que les pieces jointes. Les messages electroniques etant susceptibles
> > d'alteration, Orange decline toute responsabilite si ce message a ete
> > altere, deforme ou falsifie. Merci.
> >
> > This message and its attachments may contain confidential or
> > privileged information that may be protected by law; they should not
> > be distributed, used or copied without authorisation.
> > If you have received this email in error, please notify the sender and
> > delete
> this
> > message and its attachments.
> > As emails may be altered, Orange is not liable for messages that have
> > been modified, changed or falsified.
> > Thank you.
> >
> > _______________________________________________
> > Pce mailing list
> > [e-mail]
> > https://www.ietf.org/mailman/listinfo/pce
> 
> _______________________________________________
> Pce mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/pce