Subject: Re: [RTG-DIR] RtgDir Review: draft-ietf-mpls-ri-rsvp-frr-05

From: Chandrasekar Ramachandran <[email-masked]>
Date: Thu, 20 Jun 2019 13:42:29 +0000

Hi Julien,
Draft version 6 uploaded today addresses most of your comments.
Please refer to my detailed responses inline.

Thanks,
Chandra.

Juniper Internal

> -----Original Message-----
> From: [email-masked] <[email-masked]>
> Sent: Thursday, April 11, 2019 10:03 PM
> To: [email-masked]
> Cc: [email-masked]; [email-masked]; [email-masked]
> Subject: RtgDir Review: draft-ietf-mpls-ri-rsvp-frr-05
> 
> Hello,
> 
> I have been selected as the Routing Directorate reviewer for this draft.
> The Routing Directorate seeks to review all routing or routing-related drafts
> as they pass through IETF last call and IESG review, and sometimes on
> special request. The purpose of the review is to provide assistance to the
> Routing ADs. For more information about the Routing Directorate, please
> see ​https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__trac.tools.ietf.org_area_rtg_trac_wiki_RtgDir&d=DwIDaQ&c=HAkYuh63
> rsuhr6Scbfh0UjBXeMK-
> ndb3voDTXcWzoCI&r=iEQmXlRGWdNbtvVr6ghcatwLYhZUbMF-u63wi_-
> VTtA&m=y53rabR7nvvG-
> 1gvqqZPbmO5lH7P9g4v_e1Yj8unbjg&s=fT9WTkI4LNFIV3mjukePVngq8cvop
> WoONP1JCE18yFg&e=
> 
> Although these comments are primarily for the use of the Routing ADs, it
> would be helpful if you could consider them along with any other IETF Last
> Call comments that you receive, and strive to resolve them through
> discussion or by updating the draft.
> 
> Document: draft-ietf-mpls-ri-rsvp-frr-05
> Reviewer: Julien Meuric
> Review Date: April 10, 2019
> Intended Status: Proposed Standard
> 
> *Summary:*
> I have some minor concerns about this document that I think should be
> resolved before publication.
> 
> *Comments:*
> The document is well written and self consistent. The clear problem
> statement and the solution's "spirit" description make the detailed behavior
> convenient to follow. The main issue is about the use of 2119
> language: only 4 MUSTs are used (2 of them being similar), far too many
> SHOULDs.

[Chandra] The motivation is to enable the implementations to seamlessly interoperate with implementations that do not support the draft during incremental deployment. We have converted a few more SHOULD to MUST keeping the above in consideration. That is, wherever we consider MUST will enable better interoperability, we have upgraded SHOULD to MUST.

> *Major Issues:*
> No major issues found.
> 
> *Minor Issues:*
> Unless I missed an assumption somewhere, I think that, for a Standards
> Track document, many of the existing SHOULDs should actually be MUSTs.
> It is a bit odd to read a document trying to be exhaustive about the possible
> situations and backward compatibility consideration (nice
> work!) while only requiring optional behaviors.

[Chandra] Yes, backward compatibility is an important consideration. You could refer to the latest version for the updates.

> *Nits:*
> ------
> Global
> ---
> - Two nits are repeated along the full document and need to be fixed:
>   * Most of the time, PATH and RESV messages are fully capitalized while
> PathErr, PathTear, ResvTear... are not. Replacing them by Path/Resv would
> bring consistency.
>   * When it comes to nodes or messages names, many definite/indefinite
> articles (the/a) are missing. I have spotted some of them but got lazy...

[Chandra] I have gone through the whole draft again and has fixed all sections. You could refer to the latest version.
 
> ------
> Header
> ---
> - Was RFC 8370 considered to be added to the updated document list? The
> specification is clearly about combining RFC 4090 and 8370.
> - s/Refresh Interval Independent/Refresh-Interval Independent/

[Chandra] Fixed.

> ------
> Abstract
> ---
> - s/LSP related states/LSP-related states/  [x2]
> - LSP is not expanded on 1st use (only in introduction), it may be worth to
> expand earlier.
> - s/fast reroute (FRR)/Fast ReRoute (FRR)/
> - s/Refresh-interval/Refresh-Interval/  [for consistency]

[Chandra] Fixed.

> ------
> 1. Introduction
> ---
> - s/label switched path (LSP)/Label Switched Path (LSP)/
> - s/Standard RSVP/Base RSVP/  [x2]
> - OLD
>    eliminate facility backup protection
>    dependency on refresh timeouts for stale state cleanup including the
>    cleanup for facility backup protection.
>   NEW
>    eliminate facility backup protection
>    dependency on refresh timeouts for stale state cleanup.

[Chandra] Fixed.

> ------
> 2. Terminology
> ---
> - The terms Nhop and NNhop, extensively used, are missing.
> - Adding "B-SFRR-Ready" may be considered.
> - The link between "Merge Point" and "MP" is only implicit: MP and PLR
> deserve to be added as defined terms.
> - s/Link-protecting/Link-Protecting/
> - s/Node-protecting/Node-Protecting/

[Chandra] Fixed.

> ------
> 3. Problem Description
> ---
> - s/Figure 1, consider/Figure 1, let us consider/
> - s/Also assume/Also let us assume/
> - s/A is the Point of Local Repair (PLR) and C is Node Protecting Merge Point
> (NP-MP)/A is the PLR and C is the NP-MP/
> - s/D is the Link Protecting Merge Point (LP-MP)/D is the LP-MP/
> - s/are refreshed has failed/are refreshed, has failed/
> - s/send tear down message/send a tear down message/
> - s/as a Merge Point/as an MP/
> - s/receive PathTear/receive a PathTear/
> - Sentences #2 and #3 in the bullet at the top of page 7 seem to duplicate
> the bullet at the bottom of page 6. This need to be skimmed (I personally
> prefer the wording of the 2nd bullet over the 1st one, including s/send
> PathTear/send a PathTear/).

[Chandra] Fixed.

> ------
> 4. Solution Aspects
> ---
> - s/send tear down message/send a tear down message/  [x2]
> ------
> 4.1.
> ---
> - s/RFC 4090/[RFC4090]/  [x4, excluding section title]
> ------
> 4.2.
> ---
> - s/RFC 4090/[RFC4090]/
> - OLD
>       a LP-bypass LSP to Nhop node avoiding only the link that
>       protected LSP takes to reach Nhop
>   NEW
>       an LP-bypass LSP to the Nhop node avoiding only the link that
>       the protected LSP takes to reach the Nhop.
> 
> - OLD
>       a LP-bypass LSP to Nhop node avoiding the link that
>       protected LSP takes to reach Nhop
>   NEW
>       an LP-bypass LSP to the Nhop node avoiding the link that
>       the protected LSP takes to reach the Nhop.
> 
> - s/RFC 8370/[RFC8370]/
> - OLD
>      included RRO object carried in RESV message.
>      If the MP has not included Node-ID sub-object in RESV RRO
>    NEW
>      included in the RRO object carried in the Resv message.
>      If the MP has not included a Node-ID sub-object in the Resv RRO
> 
> - s/PATH message/Path message/  [x2]
> - s/in CAPABILITY object/in the CAPABILITY object/  [x2]
> - OLD
>     then the PLR SHOULD include B-SFRR-Ready Extended Association object
> and triggers PATH message
>   NEW
>     then [I-D.ietf-mpls-summary-frr-rsvpte] applies: the PLR SHOULD include a
> B-SFRR-Ready Extended Association object and triggers a Path message
> 
> - s/PATH message/Path message/
> - s/ordering rules object/object ordering rules/
> - s/RFC 4090/[RFC4090]/
> - s/RFC 8370/[RFC8370]/
> - s/sub-object in the RRO object carried in the RESV message/sub-object of
> the RRO object carried in the Resv message/
> - s/included Node-ID sub-object in the RRO object carried in PATH
> message/included a Node-ID sub-object in the RRO object carried in the
> Path message/
> - s/The node should determine whether the incoming PATH messages
> contains B-SFRR-Ready/A node receiving Path messages should determine
> whether they contain a B-SFRR-Ready/
> - s/followed by implementations supporting/followed by the
> implementations supporting/
> - s/"Remote" state on MP/"Remote" State on MP/
> - OLD
>     The "remote" state is identical to the protected LSP path state except for
> the difference in RSVP_HOP object.
>   NEW
>     The only difference between the "remote" path state and the LSP path
> state is in the RSVP_HOP object.
> 
> - s/in "remote" Path state/in the "remote" path state/
> - s/MP.../The MP.../  [x4]
> - s/Node signaling/The node signaling/
> - s/a PATH with/a Path message with/
> - s/in PATH RRO/in the Path RRO/
> - s/receives PathTear/receives a PathTear message
> - s/on the Ingress or created from a PATH message from/on the ingress or
> created by a Path message from/
> - s/from PLR/from the PLR/
> - s/called "Remote PathTear"/called "Remote" PathTear/

[Chandra] Fixed.
> ------
> 4.3.
> ---
> - s/Immediate node failures/Node failures/
> - s/SHOULD send Conditional PathTear/SHOULD send a "Conditional
> PathTear" downstream/
> - s/Node-ID signaling/The Node-ID signaling/
> - s/MP receives normal or "Remote" PathTear for PSB/The MP receives a
> normal or "Remote" PathTear for its PSB/  [x3]
> - s/MP receives ResvTear RSB/The MP receives a ResvTear for its RSB/  [x3]
> - s/Remote Node-ID/The remote Node-ID/  [x2]
> - s/Receiving Conditional PathTear/Receiving a Conditional PathTear/
> - s/Figure 1, assume/Figure 1, we assume/
> - s/PATH/Path/  [x5]
> - s/MP receives normal or "Remote" PathTear for PSB/The MP receives a
> normal or "Remote" PathTear for its PSB/  [x2]
> - s/MP receives ResvTear RSB/The MP receives a ResvTear for its RSB/  [x2]

[Chandra] Fixed.

> ------
> 4.4.
> ---
> - s/Conditional Path Tear/Conditional PathTear/  [x3]
> - s/Ingress has/The ingress has/
> - s/and PathTear is not received/and no PathTear is received/
> - Section 4.4.2:
>   * need "a/the" before most (conditional/normal) PathTears
>   * s/included B-SFRR-Ready Extended Association/included the B-SFRR-
> Ready Extended Association/
>   * s/PATH/a Path message/
>   * should consider upgrading some SHOULDs into MUST.
> - s/CONDITIONS object/CONDITIONS Object/
> - s/called as "CONDITIONS" object that/called "CONDITIONS" object, that/
> - "Length, Class, C-type, M bit" would better be followed  by ":" and drop
> the trailing carriage return (like the Terminology section).
> - s/If M-bit is set/If the M bit is set/  [x2]
> - s/processed based on the condition if the receiver router is a Merge Point
> or not./processed according to the receiver router role, i.e. if it is an MP or
> not./
> - s/as normal PathTear message./as a normal PathTear message./

[Chandra] Fixed.

> - The M bit is newly defined: is there any reason not to specify it using
> MUSTs?

[Chandra] Please refer to the earlier comment on backward compatibility consideration.

> ------
> 4.5.
> ---
> - s/the Ingress wants/the ingress wants/
> - s/in "remote" PathTear message/in the "Remote" PathTear message/
> - s/Consider node C in example topology (Figure 1) has/Let us consider that
> node C, in the example topology (Figure 1), has/
> - s/sends normal PathTear/send a normal PathTear/
> - s/Assume B/Let us assume that B/
> - s/send "remote" PathTear/sends a "Remote" PathTear/
> - s/deletes PSB and RSB states/deletes the PSB and RSB states/
> - s/the remote PathTear and delete PSB and RSB states/the "Remote"
> PathTear and delete the PSB and RSB states/
> - s/a router that/a PLR that/
> - s/in RESV message, and if the RRO change indicates that/in the Resv
> message indicating that/
> - s/send "Remote" PathTear/send a "Remote" PathTear/
> - s/assume/let us assume/
> - s/NP-MP for A/NP-MP for PLR A/
> - s/trigger RESV/trigger a Resv/
> - s/in RESV/in the Resv/
> - s/the RESV with/the Resv with/
> - s/send "Remote" PathTear/send a "Remote" PathTear/
> - s/send normal PathTear/send a normal PathTear/
> - s/both PSB and RSB states corresponding/both the PSB and the RSB states
> corresponding/
> - s/Phop Link failure/Phop Link Failure/
> - s/send normal PathTear and delete both PSB and RSB states
> corresponding/send a normal PathTear and delete both the PSB and the RSB
> states corresponding/
> - s/send normal PathTear and delete PSB and RSB states corresponding/send
> a normal PathTear and delete the PSB and RSB states corresponding/
> - s/Consider B-C/Let us consider that B-C/
> - s/send PathErr or ResvTear/send a PathErr nor a ResvTear/
> - s/because backup LSP/because the backup LSP/
> - s/send normal PathTear/send a normal PathTear/
> - s/on receiving PathTear/when receiving a PathTear/
> - s/reject backup LSP PATH and send PathErr/reject the backup LSP Path and
> send a PathErr/

[Chandra] Fixed.

> ------
> 4.6.
> ---
> - s/have been proposed in/have been defined in/
> - s/and remote PathTear/and "Remote" PathTear/
> - s/should support/need to support/  [unless moving to 2119 language]
> - s/in CAPABILITY object/in the CAPABILITY object/
> - s/initiate remote Node-ID/initiate a remote Node-ID/
> - s/with NNhop/with its NNhop/
> - s/set RI-RSVP flag in CAPABILITY object/set the RI-RSVP flag in the
> CAPABILITY object/
> - s/that NNhop/that the NNhop/
> - s/PPhop/the PPhop/  [x3]
> - s/in PATH/in its Path messages/
> - s/set RI-RSVP flag in CAPABILITY object/set the RI-RSVP flag in the
> CAPABILITY object/
> - s/for backward compatibility/for Backward Compatibility/
> - s/in TIME_VALUES object carried in PATH to default short refresh default
> value/in the TIME_VALUES object carried in the Path message to a default
> short refresh value/
> - s/in TIME_VALUES object carried in PATH to a short refresh default
> value/in the TIME_VALUES object carried in the Path message to a default
> short refresh value/
> - s/send remote PathTear or/send any "Remote" PathTear nor/
> - s/trigger PATH/trigger a Path message./
> - s/send Conditional PathTear/send a Conditional PathTear/
> - s/in TIME_VALUES object carried in RESV to default short refresh default
> value/in the TIME_VALUES object carried in the Resv message to a default
> short refresh value/
> - s/in TIME_VALUES object carried in PATH to default value/in the
> TIME_VALUES object carried in the *Resv* message to a default value/
> - s/and PPhop node/and the PPhop node/
> - s/in TIME_VALUES object carried in RESV to default value/in the
> TIME_VALUES object carried in the Resv message to a default value/
> - To be consistent with 4.6.2.1, the last paragraph of section 4.6.2.2.
> should NOT start with a bullet.

[Chandra] Fixed.

Thanks,
Chandra.

> ------
> 
> Cheers,
> 
> 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.

---

From: <[email-masked]>
Date: Thu, 20 Jun 2019 16:07:43 +0200

Hi Chandra,

Thanks for the update. I've just taken a look at the diff: it looks
fine, with an improved MUST/SHOULD ratio. I haven't checked all nits,
but it seems you missed a key one in section 4.6.2.2., leaving a Path
message when talking about upstream (see below).

Cheers,

Julien

On 20/06/2019 15:42, Chandrasekar Ramachandran wrote:
> [...]

_________________________________________________________________________________________________________________________

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.

---

From: Chandrasekar Ramachandran <[email-masked]>
Date: Thu, 20 Jun 2019 15:08:52 +0000

Hi Julien,
Please refer inline.

Juniper Internal

> Sent: Thursday, June 20, 2019 7:38 PM
> To: Chandrasekar Ramachandran <[email-masked]>
> Cc: [email-masked]; [email-masked]; [email-masked];
> [email-masked]
> Subject: Re: RtgDir Review: draft-ietf-mpls-ri-rsvp-frr-05
> 
> Hi Chandra,
> 
> Thanks for the update. I've just taken a look at the diff: it looks fine, with an
> improved MUST/SHOULD ratio. I haven't checked all nits, but it seems you
> missed a key one in section 4.6.2.2., leaving a Path message when talking
> about upstream (see below).

[Chandra] The text "carried in the Path to" is intended because the expected outcome is not to apply long refresh in the two hop neighborhood. That is, if the Phop node (say node X) does not support the extensions, then two nodes downstream of node X must use backward compatible refresh interval. This will enable the NP-MP of node X to time out the path state normally when the refresh timeout expires.

Thanks,
Chandra.

---

From: <[email-masked]>
Date: Fri, 21 Jun 2019 10:55:16 +0200

Hi Chandra,

Your response seems to summarize section 4.6.2.1. about the "downstream
direction", which is fine. My comment below is about section 4.6.2.2.
which describes the "upstream direction": how could an RSVP-TE message
sent to a Phop be a Path instead of a Resv? Am I missing something?

Cheers,

Julien

---

From: Chandrasekar Ramachandran <[email-masked]>
Date: Fri, 21 Jun 2019 09:14:21 +0000

Hi Julien,

Juniper Business Use Only

> Sent: Friday, June 21, 2019 2:25 PM

[Chandra] Let me illustrate with an example LSP as shown below.

    A  -> B -> C -> D

If ingress A does not support the extensions defined in the draft and requests node protection for the LSP, then long refresh will not be enforced on the LSP segment A -> B -> C. In other words, Path messages from A to B, Path messages from B to C, Resv messages from B to A, and Resv messages from C to B, will not use long refresh interval. This behavior ensures that neither primary Path from B to C nor backup LSP Path from A to C during local repair would use long refresh. Hence, node C may opt to time out LSP state normally in case of B-C link failure without requiring any explicit state cleanup messages from node A or node B.

In general, if there is a non-compliant node along the path of an LSP and if the ingress node has requested node protection, then backward compatibility procedures will come into force on two hops upstream & two hops downstream of that non-compliant node.

Thanks,
Chandra.

> > [Chandra] The text "carried in the Path to" is intended because the
> expected outcome is not to apply long refresh in the two hop
> neighborhood. That is, if the Phop node (say node X) does not support the
> extensions, then two nodes downstream of node X must use backward
> compatible refresh interval. This will enable the NP-MP of node X to time
> out the path state normally when the refresh timeout expires.
> >
> > Thanks,
> > Chandra.
> >
> >> Cheers,
> >>
> >> Julien
> >>
> >>
> >> 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.

---

From: <[email-masked]>
Date: Fri, 21 Jun 2019 15:30:19 +0200

Hi Chandra,

OK, after refreshing the context, I think I get it and why I got confused.

What puzzles me then is the 1st sentence of section 4.6.2.2, and
especially the "upstream direction" phrase, which happens to be wrong
with respect to the section title and content. I suggest two
clarification tweaks:
- drop the confusing phrase (e.g., just say "The procedures are as
follows."), as the section title already does the right job;
- add a parenthesis at the end of the 2nd bullet, e.g. "(Thus, the Nhop
is informed and can use compatible values when sending a Resv.)"

Another typo spotted from the diff: in the terminology section,
Next-Next-hop points to the PPhop acronym.

Thanks for your time,

Julien