Subject: [mpls] Tsvart last call review of draft-ietf-mpls-summary-frr-rsvpte-07

From: Gorry Fairhurst via Datatracker <[email-masked]>
Date: Thu, 12 Dec 2019 12:47:03 -0800

Reviewer: Gorry Fairhurst
Review result: Ready with Nits

This document has been reviewed as part of the transport area review team's
ongoing effort to review key IETF documents. These comments were written
primarily for the transport area directors, but are copied to the document's
authors and WG to allow them to address any issues raised and also to the IETF
discussion list for information.

When done at the time of IETF Last Call, the authors should consider this
review as part of the last-call comments they receive. Please always CC
[email-masked] if you reply to or forward this review.

I did not understand that there were transport-related issues, but I have one
request, some comments on RFC2119 language and a set of NiTs that I think
should be addressed.

I could not see where it explained how this could be done:
/   When using procedures defined in this document, the PLR MUST ensure
   bypass tunnel assignment can satisfy the protected LSP MTU
   requirements post FRR./

RFC2119 Language:

I think the following sentence is long and hard to parse. I think this needs to
be reworded to clarify the meaning of the SHOULD: /The PLR SHOULD assign the
same Bypass_Group_Identifier to all
   protected LSPs that share the egress link, and bypass tunnel as long
   as the protected LSP(s) have the common group attributes, including
   the modified tunnel sender address used for backup path
   identification as described in [RFC4090]./

It seems strange to have the RFC2119 requirements as parenthesis, please
consider making these separate requirements: /The PLR also
   generates a MESSAGE_ID object (flags SHOULD be clear, Epoch and
   Message_Identifier MUST be set according to [RFC2961])./

Is the following intended as an RFC2119 /MAY/? - mentioned more than once:
/One or more Bypass_Group_Identifiers may be included./

NiTs:

The abbreviation MTU was not defined in the abbreviations list, although
well-known this is probably helpful.

The following typos should be corrected (suggestions included):
/MP node for similar/MP node for a similar/
/to help with the scale. /to help with scaling/
/one ore more groups/one or more groups/
/are proposed in this draft to describe/are proposed in this document to
describe/ /With exception of /With the exception of /

This phrase didn’t make sense to me:
/that are being protected by the specified bypass tunnel are being rerouted./

This sentence is long and hard to parse:
/  The PLR node that supports Summary FRR procedures adds the Extended
   ASSOCIATION object with Type B-SFRR-Ready and respective Extended
   Association ID in the RSVP Path message of the protected LSP to
   inform the MP of the PLR's assigned bypass tunnel, Summary FRR
   Bypass_Group_Identifier, and the MESSAGE_ID that the PLR will use to
   refresh the protected LSP Path state after FRR occurs./

Is this:
/The PLR considers the protected LSP as Summary FRR capable only if
   all the fields in the B-SFRR-Ready Extended ASSOCIATION ID that are
   sent in the RSVP Path message and the ones received in the RSVP Resv
   message (with exception of the MESSAGE_ID) match. /
Better as:
/The PLR considers the protected LSP as Summary FRR capable only if
   all the fields in the B-SFRR-Ready Extended ASSOCIATION ID that are
   sent in the RSVP Path message match the fields received in the RSVP Resv 
   message (with exception of the MESSAGE_ID). /

Is this correct English, or should it be:
/If it does not match,/If the fields do not match,/

---

From: Tarek Saad <[email-masked]>
Date: Sun, 29 Dec 2019 19:17:58 +0000

Hi Gorry,

Happy new year!
Thanks for your thorough review and comments. We have reviewed them, and provided responses. Please see inline for more details.

On 12/12/19, 3:47 PM, "Gorry Fairhurst via Datatracker" <[email-masked]> wrote:

    

    I could not see where it explained how this could be done:
    /   When using procedures defined in this document, the PLR MUST ensure
       bypass tunnel assignment can satisfy the protected LSP MTU
       requirements post FRR./
[TS]: the facility backup method introduced in RF4090 takes advantage of MPLS label stacking (PLR imposing additional MPLS label post FRR) to allow rerouting of protected traffic over backup path. The backup path may have stricter MTU requirement and due to label stacking at PLR, the protected traffic may exceed the backup path MTU. The operator is assumed to engineer their network to allow rerouting of protected traffic and the additional label stacking at PLR to not exceed the backup path MTU.

    RFC2119 Language:
    
    I think the following sentence is long and hard to parse. I think this needs to
    be reworded to clarify the meaning of the SHOULD: /The PLR SHOULD assign the
    same Bypass_Group_Identifier to all
       protected LSPs that share the egress link, and bypass tunnel as long
       as the protected LSP(s) have the common group attributes, including
       the modified tunnel sender address used for backup path
       identification as described in [RFC4090]./
[TS]:
NEW:
       The PLR SHOULD assign the same Bypass_Group_Identifier to all
       protected LSPs that egress the same protected interface, and are protected by the
       same bypass tunnel. This minimizes the number of SFRR groups and
       optimizes the amount of signaling needed between the PLR and MP after FRR.

       The PLR MUST ensure all protected LSP(s) that are assigned the
       same Bypass_Group_Identifier use the same modified tunnel sender address
       for the backup path identification after FRR as described in [RFC4090].

    It seems strange to have the RFC2119 requirements as parenthesis, please
    consider making these separate requirements: /The PLR also
       generates a MESSAGE_ID object (flags SHOULD be clear, Epoch and
       Message_Identifier MUST be set according to [RFC2961])./
[TS]:
NEW:    
     The PLR MUST generate a MESSAGE_ID object with Epoch and Message_Identifier
     set according to [RFC2961]. The MESSAGE_ID object flags SHOULD be cleared
     when transmitted by PLR and ignored when received at the MP.

    Is the following intended as an RFC2119 /MAY/? - mentioned more than once:
    /One or more Bypass_Group_Identifiers may be included./
[TS]: yes.
NEW:
    One or more Bypass_Group_Identifiers MAY be included.

    NiTs:
    
    The abbreviation MTU was not defined in the abbreviations list, although
    well-known this is probably helpful.
[TS]: OK, will be expanded in the abbreviations section.

[TS]: OK for all.

    This phrase didn’t make sense to me:
    /that are being protected by the specified bypass tunnel are being rerouted./
[TS]:
NEW:
B-SFRR-Active: Bypass Summary FRR Active Extended ASSOCIATION
object.  Used to notify the MP node that one or more groups of
protected LSP(s) have been rerouted over the associated bypass tunnel.

    This sentence is long and hard to parse:
    /  The PLR node that supports Summary FRR procedures adds the Extended
       ASSOCIATION object with Type B-SFRR-Ready and respective Extended
       Association ID in the RSVP Path message of the protected LSP to
       inform the MP of the PLR's assigned bypass tunnel, Summary FRR
       Bypass_Group_Identifier, and the MESSAGE_ID that the PLR will use to
       refresh the protected LSP Path state after FRR occurs./
[TS]:
NEW:
       The PLR node that supports Summary FRR procedures adds an Extended
       ASSOCIATION object with B-SFRR-Ready Extended Association ID in the
       RSVP Path message of the protected LSP. The PLR adds the protected LSP
       Bypass_Group_Identifier, information from the assigned bypass tunnel, and
       MESSAGE_ID object into the B-SFRR-Ready Extended Association ID.
       The MP uses the information contained in the received B-SFRR-Ready
       Extended Association ID to refresh and merge the protected LSP Path state after
       FRR occurs.

    Is this:
[TS]: accepted, we will update as per your suggestion.

    Is this correct English, or should it be:
    /If it does not match,/If the fields do not match,/
[TS]: suggestion is reasonable, we will adopt it.

Regards,
Tarek