[mpls] Re: John Scudder's Discuss on draft-ietf-mpls-ri-rsvp-frr-18: (with DISCUSS and COMMENT)

[Chandrasekar Ramachandran <[e-mail]>]

Thanks for the detailed review. Please see inline (prefixed [CR]) for responses.

Thanks,
Chandra.


Juniper Business Use Only
> -----Original Message-----
> From: John Scudder via Datatracker <[e-mail]>
> Sent: Wednesday, May 29, 2024 6:06 AM
> To: The IESG <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> Nicolai Leymann <[e-mail]>; [e-mail]
> Subject: John Scudder's Discuss on draft-ietf-mpls-ri-rsvp-frr-18: (with
> DISCUSS and COMMENT)
>
> [External Email. Be cautious of content]
>
>
> John Scudder has entered the following ballot position for
> draft-ietf-mpls-ri-rsvp-frr-18: Discuss
>
> When responding, please keep the subject line intact and reply to all email
> addresses included in the To and CC lines. (Feel free to cut this introductory
> paragraph, however.)
>
>
> Please refer to
> https://urldefense.com/v3/__https://www.ietf.org/about/groups/iesg/statem
> ents/handling-ballot-positions/__;!!NEt6yMaO-gk!E-
> wtrMVo1qpsxwkeOZLVQjhalO1sPdAEG-o4lpb0Wn4W-
> lL70xoGf8s0cEIBtrEpTeWgFiMeSkm3cw$
> for more information about how to handle DISCUSS and COMMENT
> positions.
>
>
> The document, along with other ballot positions, can be found here:
> https://urldefense.com/v3/__https://datatracker.ietf.org/doc/draft-ietf-mpls-
> ri-rsvp-frr/__;!!NEt6yMaO-gk!E-wtrMVo1qpsxwkeOZLVQjhalO1sPdAEG-
> o4lpb0Wn4W-lL70xoGf8s0cEIBtrEpTeWgFiN5pzldoQ$
>
>
>
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
>
> # John Scudder, RTG AD, comments for draft-ietf-mpls-ri-rsvp-frr-18 CC
> @jgscudder
>
> Thanks for this document. It's a dense read for someone like me who is not
> an expert in RSVP, but it seems important, useful, and carefully done. I
> appreciate the precise and detailed work.
>

[CR] Thank you.

> I have one concern I've flagged as a DISCUSS point, which may turn out not to
> be a big deal if I've just misunderstood some nuance, but in any case, we
> should talk through it. I also have some other comments I hope might be
> helpful.
>
> ## DISCUSS
>
> ### Section 4.1
>
>    A node supporting facility backup protection [RFC4090] MUST set the
>    RI-RSVP flag (I bit) that is defined in Section 3.1 of RSVP-TE
>    Scaling Techniques [RFC8370] only if it supports all the extensions
>    specified in the rest of this document.
>
> I have several concerns about this. At least some of them probably relate to
> my limited expertise in the subject area, but maybe I can at least expose some
> areas where additional explanation might be helpful in the document.
>

[CR] Please also refer to the responses sent to Ketan (https://mailarchive.ietf.org/arch/msg/mpls/gMpvz7ez80a9AfOdPOvPSUZEcpM/)

> First and foremost, as written this sentence doesn't seem like it's achievable in
> practice. Couldn't there be a legacy router in the field that supports both RFC
> 4090 and RFC 8370? Wouldn't that router then advertise the I bit, at least in
> some circumstances?
>

[CR] As noted in the response to Ketan, the authors believe that it is extremely unlikely that an MPLS RSVP-TE implementation that supports RFC4090 would implement only RI-RSVP and not RI-RSVP-FRR. So, we don't envision there being a legacy MPLS router in the field that supports both RFC 4090 and RFC 8370. In the unlikely scenario where such an implementation choice was made, the requirements on the establishment of signaling handshake and the backwards compatibility procedures discussed in the document provide for sufficient cover.

> Second, this MUST seems to conflict with a SHOULD in section 4.6.1, which
> says,
>
>    An implementation supporting RI-RSVP-FRR extensions SHOULD set the
>    flag "Refresh interval Independent RSVP" or RI-RSVP flag in the
>    CAPABILITY object carried in Hello messages as specified in RSVP-TE
>    Scaling Techniques [RFC8370].
>

[CR] We will update this to a "MUST".

> It makes me wonder if the section 4.1 paragraph should be more like this.
>
> NEW:
>    A node supporting facility backup protection [RFC4090] MUST NOT set the
>    RI-RSVP flag (I bit) that is defined in Section 3.1 of RSVP-TE
>    Scaling Techniques [RFC8370] unless it supports all the extensions
>    specified in the rest of this document.
>

[CR] Based on the three responses above, is this change still necessary?

> (Although even though that might be clearer, the first point stands, that it
> might not be achievable.)
>

[CR] As noted above, we don't envision there being a legacy MPLS router in the field that supports both RFC 4090 and RFC 8370. There was some text in a draft version of RFC8370, that explicitly stated "use RI-RSVP-FRR procedures if RFC4090 procedures are implemented" (see section 2.2 of https://datatracker.ietf.org/doc/draft-ietf-teas-rsvp-te-scaling-rec/02/) but it was removed in later stages based on WG feedback.

> Third, moving on to the end of the paragraph,
>
>    Procedures for backward compatibility (see Section 4.6.2.3 of this
>    document) delves on this in detail.
>
> As far as I can tell, despite the title of Section 4.6.2, Subsection 4.6.2.3 isn't a
> procedure for backward compatibility, it's a warning about the terrible things
> that can happen if some node in the network is incompatible.
>

[CR]  Subsection 4.6.2.3 was added based on the discussion in the following thread (ask was to add some text describing the operational considerations for this particular scenario): https://mailarchive.ietf.org/arch/msg/mpls/KDRVWBAajl08pnXDkit0ykRyZDg/

> Is all of this right? That there could be a legacy node in the network, with no
> way to detect that it doesn't comply with the present specification, and that
> terrible things such as 4.6.2.3 describes, could happen as a result?
>

[CR] Please refer to the previous response above.

> (I see Ketan Talaulikar raised a related concern in his RTGDIR review.)
>
>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> ## COMMENT
>
> ### RTGDIR review
>
> Please take a look at Ketan Talaulikar's RTGDIR review that was posted
> yesterday,
> (https://urldefense.com/v3/__https://mailarchive.ietf.org/arch/msg/rtg-
> dir/PRZJa7LH9b3J1aRFo3BvYZ3DQUQ/__;!!NEt6yMaO-gk!E-
> wtrMVo1qpsxwkeOZLVQjhalO1sPdAEG-o4lpb0Wn4W-
> lL70xoGf8s0cEIBtrEpTeWgFiP0TOSq2g$ ). I won't repeat his points here, other
> than the one that appeared in my DISCUSS, but please consider all of them.
>
> ### Section 4.3.3, use of RFC 2119 keyword in example
>
> You have a number of sections that include examples. The examples are very
> helpful for understanding the specification, thank you! However, you use RFC
> 2119/BCP 14 keywords in the examples, and I think you shouldn't. Those
> keywords are reserved for specifying procedures, and an example isn't
> specifying procedure, it's demonstrating it.
>
> I'll call out each example separately but only provide the explanation here.
>
> In this section, what I noticed was,
>
>    1.  When C receives a Conditional PathTear from B, it decides to
>       retain LSP state as it is the NP-MP of the PLR A.  C also MUST
>       check whether Phop B had previously signaled availability of node
>       protection.  As B had previously signaled NP availability by
>       including B-SFRR-Ready Extended Association object, C MUST remove
>       the B-SFRR-Ready Extended Association object containing
>       Association Source set to B from the Path message and trigger a
>       Path to D.
>
> There are various ways this could be rewritten, the absolute easiest would be
> to simply substitute "must" for "MUST", although it would be cleaner to fully
> rewrite in terms of actions instead of expectations, as in,
>
> NEW:
>    1.  When C receives a Conditional PathTear from B, it decides to
>       retain LSP state as it is the NP-MP of the PLR A.  It also
>       checks whether Phop B had previously signaled availability of node
>       protection.  As B had previously signaled NP availability by
>       including B-SFRR-Ready Extended Association object, C removes
>       the B-SFRR-Ready Extended Association object containing
>       Association Source set to B from the Path message and triggers a
>       Path to D.
>
[CR] Ack. Will make the change in all appropriate places.

> ### Section 4.4.3, RFC 2119 keyword misuse
>
>    As any implementation that does not support Conditional PathTear MUST
>    ignore the new object but process the message as a normal PathTear
>    without generating any error, the Class-Num of the new object MUST be
>    10bbbbbb where 'b' represents a bit (from Section 3.10 of [RFC2205]).
>
> I think the first MUST is unnecessary since you're stating a need, not a
> requirement. The need is fulfilled as a natural consequence of your choice of
> code point and the underlying requirement from RFC 2205. The second MUST
> is also unnecessary, you are not telling the implementer anything, you're just
> explaining why you chose the type code you did.
>
> One possible fix would be to delete the paragraph, but it's probably nicer to
> the reader if you leave the explanation in place. Perhaps something like,
>
> NEW:
>    Any implementation that does not support Conditional PathTear needs
>    to ignore the new object but process the message as a normal PathTear
>    without generating any error. For this reason, the Class-Num of the
>    new object follows the pattern 10bbbbbb where 'b' represents a bit.
>    (The behavior for objects of this type is specified in Section 3.10
>    of [RFC2205]).
>
[CR] Thanks. Will make the suggested change.

> ### Section 4.5, clarification of requirement
>
>    If the ingress wants to tear down the LSP because of a management
>    event while the LSP is being locally repaired at a transit PLR, it
>    would not be desirable to wait till the completion of backup LSP
>    signaling to perform state cleanup.  To enable LSP state cleanup when
>    the LSP is being locally repaired, the PLR MUST send a "Remote"
>    PathTear message instructing the MP to delete the PSB and RSB states
>    corresponding to the LSP.  The TTL in the "Remote" PathTear message
>    MUST be set to 255.
>
> In this paragraph, I am unclear exactly what the sentence "To enable LSP state
> cleanup when the LSP is being locally repaired, the PLR MUST send a
> "Remote"
> PathTear message instructing the MP to delete the PSB and RSB states
> corresponding to the LSP" is telling me. My best guess is,
>
> NEW:
>    If the ingress wants to tear down the LSP because of a management
>    event while the LSP is being locally repaired at a transit PLR, it
>    would not be desirable to wait till the completion of backup LSP
>    signaling to perform state cleanup.  In this case, the PLR MUST send
>    a "Remote" PathTear message instructing the MP to delete the PSB and
>    RSB states corresponding to the LSP.  The TTL in the "Remote"
>    PathTear message MUST be set to 255. Doing this enables LSP state
>    cleanup when the LSP is being locally repaired,
>
> Is that rewrite faithful to what you intended? If so, I suggest using it, or any
> other rewrite of your choosing that clarifies matters. In particular, my intent in
> the rewrite is to make it clear that the PLR is unequivocally required to do this,
> which IMO wasn't clear before.
>

[CR] Yes, the rewrite is faithful. Will make the suggested edit.

> ### Section 4.5.1, clarification of requirement
>
>    If local repair fails on the PLR after a failure, then this MUST be
>    considered as a case for cleaning up LSP state from the PLR to the
>    Egress.  The PLR achieves state cleanup by sending "Remote" PathTear
>    to the MP.  The MP MUST delete the states corresponding to the LSP
>    also propagate the PathTear downstream thereby achieving state
>    cleanup from all downstream nodes up to the LSP egress.  Note that in
>    the case of link protection, the PathTear MUST be directed to the LP-
>    MP's Node-ID IP address rather than the Nhop interface address.
>
> Similar to the previous case, the combination of the RFC 2119 keyword with
> the casual writing style leaves the intent of the requirement unclear to me.
> Here's an attempt at a rewrite, in the same spirit.
>
> NEW:
>    If local repair fails on the PLR after a failure, the PLR MUST send a
>    "Remote" PathTear to the MP.  The purpose of doing this is to clean
>    up LSP state from the PLR to the Egress. Upon receiving the PathTear,
>    the MP will delete the states corresponding to the LSP and also
>    propagate the PathTear downstream thereby achieving state cleanup
>    from all downstream nodes up to the LSP egress.  Note that in the
>    case of link protection, the PathTear MUST be directed to the LP-
>    MP's Node-ID IP address rather than the Nhop interface address.
>
> Note that I removed the second MUST, on the assumption that you aren't
> specifying a new requirement for the MP, but stating the natural consequence
> of a requirement you've already specified elsewhere. Please double-check
> that I haven't broken something!
>

[CR] Regarding the second MUST, we believe it is necessary (please refer to section 4.2.4). We will adopt the rest of the suggested changes as shown below.

NEW:
   If local repair fails on the PLR after a failure, the PLR MUST send a
   "Remote" PathTear to the MP.  The purpose of doing this is to clean
   up LSP state from the PLR to the Egress. Upon receiving the PathTear,
   the MP MUST delete the states corresponding to the LSP and also
   propagate the PathTear downstream thereby achieving state cleanup
   from all downstream nodes up to the LSP egress.  Note that in the
   case of link protection, the PathTear MUST be directed to the LP-
   MP's Node-ID IP address rather than the Nhop interface address.

> ### Section 4.5.2, use of RFC 2119 keyword in example
>
> As with my 4.3.3 comment. In this case my suggestion is,
>
> OLD:
>    RRO of the LSP carried in the Resv will not contain C.  When A
>    processes the Resv message with a new RRO not containing C - its
>    former NP-MP, A MUST send a "Remote" PathTear to C.  When C receives
>
> NEW:
>    RRO of the LSP carried in the Resv will not contain C.  When A
>    processes the Resv message with a new RRO not containing C - its
>    former NP-MP, A sends a "Remote" PathTear to C.  When C receives
>
[CR] Ack. Will make the suggested change.

> ### Section 4.5.3.1, I'm confused
>
>    If an LSP is preempted on an LP-MP after its Phop or the incoming
>    link has already failed
>
> Why "Phop or the incoming link" and not just "incoming link"? It's a link-
> protecting merge point, so why does it care about the failure of an upstream
> *node*? Also, this seems as though it conflicts with the title of the subsection,
> which is "Preemption on LP-MP after Phop Link Failure" and not "...
> after Phop or Phop Link Failure".
>
> Could it be rewritten as follows?
>
>    If an LSP is preempted on an LP-MP after its Phop
>    link has already failed
>

[CR] You are right. Will make the change as suggested.

> ### Section 4.5.3.2, I'm still confused but in the other direction
>
>    If an LSP is preempted on an NP-MP after its Phop link has already
>    failed
>
> If it's a node-protecting merge point, shouldn't this one care about the Phop
> as well as the Phop link? (Also in the title of the subsection.)
>

[CR] When the phop link goes down, the NP-MP can't determine if it is just the phop link that went down or the phop that went down. So, we don't need a separate section to discuss "Phop Failure".

> ### Section 4.5.3.2, use of RFC 2119 keyword in example
>
> As with my 4.3.3 comment. In this case my suggestion is,
>
> OLD:
>    3.  As the only reason for C having retained state after Phop node
>       failure was that it was an NP-MP, C MUST send a normal PathTear to
>       D and delete its PSB state also.  D would also delete the PSB and
>       RSB states on receiving a PathTear from C.
>
> NEW:
>    3.  As the only reason for C having retained state after Phop node
>       failure was that it was an NP-MP, C sends a normal PathTear to
>       D and deletes its PSB state also.  D would also delete the PSB and
>       RSB states on receiving a PathTear from C.
>

[CR] Ack. Will make the suggested change.

> ### Section 4.6, ALL CAPS
>
> I probably wouldn't even flag this if I weren't doing a full review already, but
>
>    DOES NOT support RI-RSVP-FRR extensions.  Note that for LSPs
>
> Although RFC 2119 doesn't forbid the use of all caps for non-reserved
> keywords, it's often considered to be inadvisable. I would suggest,
>
> NEW:
>    does not support RI-RSVP-FRR extensions.  Note that for LSPs
>

[CR] Ack. Will make the suggested edit.

> ### Section 4.6.1, SHOULD set
>
>    An implementation supporting RI-RSVP-FRR extensions SHOULD set the
>    flag "Refresh interval Independent RSVP" or RI-RSVP flag in the
>    CAPABILITY object carried in Hello messages as specified in RSVP-TE
>    Scaling Techniques [RFC8370].
>
> Depending on the resolution of my DISCUSS point, I guess you might want to
> revisit this SHOULD.
>

[CR] As noted above in one of the responses, we will make this a "MUST".

> ### Section 4.6.2.1, use of RFC 2119 keyword in example
>
> As with my 4.3.3 comment. In this case my suggestion is,
>
> OLD:
>    -  A and B MUST reduce the refresh time to the default short refresh
>       interval of 30 seconds and trigger a Path message
>
>    -  If B is not an MP and if Phop link of B fails, B cannot send
>       Conditional PathTear to C but MUST time out the PSB state from A
>       normally.  Note that B can time out the PSB state A normally only
>       if A did not set long refresh in the TIME_VALUES object carried in
>       the Path messages sent earlier.
>
> NEW:
>    -  A and B reduce the refresh time to the default short refresh
>       interval of 30 seconds and trigger a Path message
>
>    -  If B is not an MP and if Phop link of B fails, B cannot send
>       Conditional PathTear to C but times out the PSB state from A
>       normally.  Note that B can time out the PSB state A normally only
>       if A did not set long refresh in the TIME_VALUES object carried in
>       the Path messages sent earlier.
>

[CR] Thanks. Will make the changes as suggested.

> ## Notes
>
> This review is in the ["IETF Comments" Markdown format][ICMF], You can use
> the [`ietf-comments` tool][ICT] to automatically convert this review into
> individual GitHub issues.
>
> [ICMF]: https://urldefense.com/v3/__https://github.com/mnot/ietf-
> comments/blob/main/format.md__;!!NEt6yMaO-gk!E-
> wtrMVo1qpsxwkeOZLVQjhalO1sPdAEG-o4lpb0Wn4W-
> lL70xoGf8s0cEIBtrEpTeWgFiOieqUlIQ$
> [ICT]: https://urldefense.com/v3/__https://github.com/mnot/ietf-
> comments__;!!NEt6yMaO-gk!E-wtrMVo1qpsxwkeOZLVQjhalO1sPdAEG-
> o4lpb0Wn4W-lL70xoGf8s0cEIBtrEpTeWgFiP1IMYQ5w$
>
>