Last Call Review of draft-ietf-mpls-ri-rsvp-frr-05
review-ietf-mpls-ri-rsvp-frr-05-rtgdir-lc-meuric-2019-04-11-00
Request | Review of | draft-ietf-mpls-ri-rsvp-frr |
---|---|---|
Requested revision | No specific revision (document currently at 22) | |
Type | Last Call Review | |
Team | Routing Area Directorate (rtgdir) | |
Deadline | 2019-03-08 | |
Requested | 2019-02-22 | |
Requested by | Deborah Brungard | |
Authors | Chandrasekar R , Tarek Saad , Ina Minei , Dante Pacella | |
I-D last updated | 2019-04-11 | |
Completed reviews |
Secdir Early review of -17
by David Mandelberg
(diff)
Rtgdir Early review of -18 by Ketan Talaulikar (diff) Rtgdir Last Call review of -05 by Julien Meuric (diff) Genart Last Call review of -07 by Reese Enghardt (diff) Rtgdir Last Call review of -16 by Carlos Pignataro (diff) |
|
Comments |
Prep for IETF Last Call |
|
Assignment | Reviewer | Julien Meuric |
State | Completed | |
Request | Last Call review on draft-ietf-mpls-ri-rsvp-frr by Routing Area Directorate Assigned | |
Reviewed revision | 05 (document currently at 22) | |
Result | Has issues | |
Completed | 2019-04-11 |
review-ietf-mpls-ri-rsvp-frr-05-rtgdir-lc-meuric-2019-04-11-00
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 http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir 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. *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. *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... ------ 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/ ------ 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] ------ 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. ------ 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/ ------ 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/). ------ 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/ ------ 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] ------ 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./ - The M bit is newly defined: is there any reason not to specify it using MUSTs? ------ 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/ ------ 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. ------ 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.