Re: [Gen-art] Genart last call review of draft-ietf-teas-rsvp-ingress-protection-13

[Huaimo Chen <[e-mail]>]

Hi Robert,

    Thank you very much for your time and valuable comments.
    Answers to them are inline below with prefix [HC].
    Would you mind reviewing them to see if they address the issues?

Best Regards,
Huaimo
-----Original Message-----
From: Robert Sparks [mailto:[e-mail]] 
Sent: Wednesday, February 21, 2018 12:20 PM
To: [e-mail]
Cc: [e-mail]; [e-mail]; [e-mail]
Subject: Genart last call review of draft-ietf-teas-rsvp-ingress-protection-13

Reviewer: Robert Sparks
Review result: Not Ready

I am the assigned Gen-ART reviewer for this draft. The General Area Review Team (Gen-ART) reviews all IETF documents being processed by the IESG for the IETF Chair.  Please treat these comments just like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-teas-rsvp-ingress-protection-13
Reviewer: Robert Sparks
Review Date: 2018-02-21
IETF LC End Date: 2018-03-01
IESG Telechat date: 2018-03-08

 Summary: Not Ready for publication as an Experimental RFC

I found this document hard to read. I encourage a significant editorial pass that focuses on presenting the core ideas early, and simplifies the language used throughout the document.
[HC] We have presented the core ideas early and simplify the language used in
the document accordingly.


Major Issues:

1) The IANA considerations section is unclear. You ask IANA to put something in "Class Names, Class Numbers, and Class Types" at <http://www.iana.org/assignments/rsvp-te-parameters/rsvp-te-parameters.xhtml>.
But there is no registry with that title on that page. (Nor is there a registry with a structure matching the row you are asking IANA to add.) Did you mean the registry at <https://www.iana.org/assignments/rsvp-parameters/rsvp-parameters.xhtml#rsvp-parameters-4>?
[HC] Yes. You are right. We have corrected it in the document. 


If so, the number you are suggesting is in a "Reserved for Private Use" range.
If that _was_ your target, the structure of your requested record does not match the structure of the existing registry. Please clarify.
[HC] We are considering a new C-Type under the existing Protection Class. 
The related text has be updated accordingly as below:
Upon approval of this document, IANA is requested to assign a new
Class Type or C-Type under Class Number 37 and Class Name PROTECTION
located at <https://www.iana.org/assignments/rsvp-parameters/
rsvpparameters.xhtml#rsvp-parameters-39>, as follows:

Value Description                   Reference
----- -----------                   ---------
  4    Type 4 INGRESS_PROTECTION   This Document


The instructions to IANA for what to do when this document moves to the standards track are inappropriate. You can say that you anticipate that the future document that moves the idea to the standard track expects to create a new registry under rsvp-te-parameters, but this document cannot instruct IANA to do so.
[HC] We have revised the text according to your suggestion as below:
It is anticipated that the future document that moves the idea to the 
standard track expects IANA to create and maintain a new registry under
PROTECTION object class, Class Number 37, C-Type 4. Initial values for 
the registry are given below. The future assignments are to be made through 
IETF Review.


2) "After one approach is selected, the document SHOULD become proposed standard." is an innappropriate use of SHOULD, and doesn't correctly reflect the process that would be followed in any case. You could, instead, say "After one approach is selected, a document revising the ideas here to reflect that selection and any other items learned from the experiment is expected to be submitted for publication on the standards track."
[HC] We have updated the text accordingly as below:
After one approach is selected, the document will be revised 
to reflect that selection and any other items learned from 
the experiment. The revised document is expected to be submitted 
for publication on the standards track.


3) It is unclear until you get to section 5 (11 pages into the document) which elements provide and which elements consume the information in the proposed new object. The document is inconsistent about which messages the object can/should appear in. Moving the overview of behavior earlier in the document would help.
Simplifying the description of that behavior will help more. Making the other sections consistent with what that overview describes is necessary.
[HC] We have corrected the inconsistency and updated the document according to 
your suggestions.


Minor Issues:

a) It is not clear what the difference between source-detect and backup-source-detect really are (I agree with the comments in the rtgdir review on these sections). In section 2.1, where you say "reliably detect", do you really mean "verify"? The detection has already been done by the source.
[HC] Yes. We have used "verify" instead of "reliably detect" and updated the 
text accordingly. We have added more details on source-detect and backup-source-detect as below:
Note that one of the differences between Source-Detect and Backup-
Source-Detect is: in the former, the backup ingress verifies the
failure of the primary ingress slowly such as in seconds; in the
latter, the backup ingress detects the failure fast such as in a few
or tens of milliseconds.


b) Section 4.1 is particularly hard to extract roles and responsibilities from.
Calling out early which elements will use the object and in which messages will clarify the definition of behavior. Perhaps you should more carefully separate the definition of the object from normative statements about how the object gets used.
[HC] We have added a brief description on the roles of the object as below:
   The primary ingress of a primary LSP sends the
   backup ingress this object in a PATH message.  In this case, the
   object contains the information needed to set up ingress protection.
   The information includes:

   o  Backup ingress IP address indicating the backup ingress,

   o  Traffic Descriptor describing the traffic that the primary LSP
      transports, this traffic is imported into the backup LSP(s) on the
      backup ingress when the primary ingress fails,

   o  Label and Routes indicating the first hops of the primary LSP,
      each of which is paired with its label, and

   o  Desire options on ingress protection such as P2MP option
      indicating a desire to use a backup P2MP LSP to protect the
      primary ingress of a primary P2MP LSP.

   The backup ingress sends the primary ingress this object in a RESV
   message.  In this case, the object contains the information about the
   status on the ingress protection.


c) The document is vague about source behavior, though it requires behaviors of the source when it detects failure, and when a previously failed primary becomes available again. A clearer description of what you are requiring of the source, and what you are intentionally leaving unspecified would help.
[HC] We have added the following text and updated the document accordingly.
When the previously failed primary ingress of a primary LSP becomes available 
again and the primary LSP is recovered from its primary ingress, the source 
may switches the traffic to the primary ingress from the backup ingress. A
operator may control the traffic switch through using a command on the 
source node after seeing that the primary LSP has recovered.


d) The MUST in the first bullet of section 5.2 is unclear. As written it's an incorrect use of 2119. Are you trying to say that the primary ingress must know the IP address of the backup it wants to be used before it can use the INGRESS_PROTECTION object, or are you trying to say that if primary ingress doesn't know the address of the backup, it MUST NOT include a backup ingress address sub-object?
[HC] It is the former. We have updated the text accordingly as below:
    o Backup Ingress Address: The primary ingress MUST know the IP
      address of the backup ingress it wants to be used before it can
      use the INGRESS_PROTECTION object.


Nits and editorial comments:

FRR is used in the title, but does not appear in the abstract. (The abstract could say more about what the document is about.)
[HC] We have added more as follows:
It extends the fast-reroute (FRR) protection for transit nodes 
of an LSP to the ingress node of the LSP.


The first sentence of the second paragraph of the Introduction is complex.
Please break it apart into several sentences.
[HC] We have rephrased it as below:
To protect against the failure of the (primary) ingress node of a
primary end to end P2MP (or P2P) TE LSP, a typical existing solution
is to set up a secondary backup end to end P2MP (or P2P) TE LSP.
The backup LSP is from a backup ingress node to backup egress nodes (or node).
The backup ingress node is different from the primary ingress node.
The backup egress nodes (or node) are (or is) different from the primary 
egress nodes (or node) of the primary LSP.


In the introduction, please delete ", which are briefed as follows". The introduction to the issues can simply start "There are a number of issues in this solution:".
[HC] We have revised the text according to your suggestion.


At the second bullet in this list of issues, I suggest "configuration" instead of "configurations".
[HC] We have changed it to "configuration".


At the third bullet in this list of issues, what does "the BFD down" mean?
[HC] We have updated the text as below:
Any failure on the path of the BFD from an egress node
to an ingress node may cause the BFD to indicate the failure
of the ingress node.


In section 3, I suggest replacing ", we call it is off-path" with "it is off-path". Please make a similar change to the sentence about on-path. In the third sentence, why do you say "configured or computed"? What does "computed"
mean in this context?
[HC] We have revised the related text in the document regarding to off-path and 
on-path. "configured or computed" says that a backup ingress is "configured"
statically or "computed" automatically.
"A backup ingress is computed" means 
that some software suggests a backup ingress automatically for the given
information about the primary LSP.


In the introduction to section 4, say _what_ the new object is backwards compatible with.
[HC] We have removed "It is backward compatible."


In section 4.1, it's unclear what "keeps it" means. Please try reworking this sentence without using "it".
[HC] "keeps it" means records it (this flag). 
We have rephrased this sentence accordingly.


It was not clear in section 4.1.1 that a backup would use the presence of the backup ingress IPv4 (or 6) sub-objects to discover that the primary wanted it to behave as a backup (as described in section 5.3).
[HC] We have added some explanations. 
A node X is configured as a backup ingress on the primary ingress of an LSP. 
The node X does not know whether it is a backup ingress for any primary ingress
of any LSP until the node X receives a PATH message with backup ingress 
sub-object for an LSP. 


In the 3rd paragraph of section 5.1.1, do you mean "efficient processing" where you say "efficient process"?
[HC] Yes. We have revised it.


The last paragraph of 5.1.1 does not help the document. "simple" compared to what? "less" compared to what? I suggest deleting the paragraph, or significantly expanding on the discussion.
[HC] We have deleted it.


At the first sentence of the last paragraph of 5.1.2, did you mean "required"
instead of "requires"?
[HC] Yes. We have changed it to required.


At the second paragraph of 5.3, what are "administrative-colors"?
[HC] They are administrative-groups. We have changed it to administrative-groups.