Subject: [Gen-art] Last Call Review: draft-ietf-mpls-proxy-lsp-ping-03

From: Tom Taylor <[email-masked]>
Date: Fri, 13 Feb 2015 01:19:56 -0500

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments
you may receive.

Sorry to be so last-minute. Life happens.

Tom Taylor

Document:  draft-ietf-mpls-proxy-lsp-ping-03
Reviewer:  Tom Taylor
Review Date: 2015-02-12
IETF LC End Date: 2015-02-12
IESG Telechat date: 2015-02-19

Summary: Generally well written, but a few minor issues and a number of 
nits and editorial suggestions.

Major issues:

Minor issues:

1. Section 3.1, "Proxy Echo Parameter TLV MPLS payload size field".

This section describes initiator actions, but the paragraph contains the 
text:

" When the payload size is non
    zero, if sending the MPLS Echo Request involves using an IP header,
    the Do not Fragment (DF) bit MUST be set to 1."

This sentence logically belongs in Section 3.2 (3.2.4.1), which has no 
instructions at all regarding the payload size field.

Noted that the description of the MPLS Payload Size field in Section 5.1 
has a full description, but says the DF bit SHOULD be set. Is it MUST or 
SHOULD, and if the latter, what is the exception?

2. Section 3.2.1, third paragraph, second sentence currently reads:

   "If the Proxy LSR is a budnode, a MPLS
    proxy ping reply SHOULD be sent to the initiator with the return code
    set to 3 (Reply router is Egress for FEC) with return Subcode set to
    0 and DS/DDMAPs only if the Proxy initiator requested information to
    be returned in a MPLS proxy ping reply."

Do you mean that the DS/DDMAP TLV is included only if the initiator 
asked for it (which seems like redundant advice) or do you mean that the 
reply is sent only if the initiator asked for the DS/DDMAP information?

3. Same paragraph, next sentence: why SHOULD rather than MUST? What is 
the exception?

4. Section 3.2.1, last paragraph (dealing with MP2MP case): the second 
to fourth sentences read:

"LSP pings sent from a leaf of a MP2MP has
    different behavior in this case. MPLS Echo Request are sent to all
    upstream/downstream neighbors. The Proxy LSRs need to be consistent
    with this variation in behavior."
s/has/have/ to get that out of the way.

Do you mean that the initiator is a leaf of the MP2MP? How does the 
Proxy LSR know this? Does it matter? If it is the Proxy LSR that is the 
leaf, the statement makes more sense. The different behavior is that the 
Proxy LSR always sends MPLS Echo Requests in at least one direction if 
it is not sending an MPLS proxy ping reply. You could refer to the 
previous paragraph for the remaining behavioural description.

The next sentence in this paragraph has the same problem as Minor Issue 
2. above, except that I see a stronger hint that the second 
interpretation suggested above is your intention.

5. You are creating a new IANA registry for sub-types of the Proxy Echo 
Parameters TLV. You need to document registration procedures as per RFC 
5226.

Nits/editorial comments:

General: Is there a reason for capitalizing "MPLS Echo Request" but not 
"MPLS proxy ping request[reply]"?

General: Sometimes return code text is enclosed in quotes, sometimes in 
parentheses. Choose one.

1. ABSTRACT third line: s/Routers/Router/

2. Section 2.1.1, first line: s/If/if/

3. Same section, second paragraph has both typos and editorial glitches.

OLD

    In the case the targeted proxy LSR does not understand LSP ping Echo
    Request at all, like any other LSR which do not understand the
    messages, they MUST be dropped and no messages is set back to the
    initiator.

NEW

    In the case where the targeted proxy LSR does not understand
    the LSP ping Echo Request at all, like any other LSR which does not
    understand the messages, it MUST drop them and MUST NOT(?) send any
    message back to the initiator. [Was the MUST NOT intended? -- PTT]

4. Section 3.1, last sentence: s/is//

5. Section 3.2, first paragraph, fourth line: s/set to/to/

6. Same section, fifth paragraph:
   s/Request messages/Request message/
   s/one is sent/either is sent/

7. Same section, sixth paragraph, end of second-last line: s/an//

8. Same section, seventh paragraph: invocation is something that happens 
after configuration. Maybe you want to say:

   "If a configured filter has been invoked and an address ..."

10. Same section, later paragraph as shown below (editorial suggestion):

OLD

    If the "Request for FEC Neighbor Address info" flag is set, a
    Upstream Neighbor Address TLV and/or Downstream Neighbor Address
    TLV(s) is/are formatted for inclusion in the MPLS proxy ping reply.
    If the Upstream or Downstream address is unknown they are not
    included in the Proxy Reply.

NEW

    If the "Request for FEC Neighbor Address info" flag is set, the
    Upstream Neighbor Address and Downstream Neighbor Address
    TLVs are formatted for inclusion in the MPLS proxy ping reply.
    If the Upstream or Downstream address is unknown the corresponding
    TLV is omitted.

11. Same section, paragraph re detailed downstream mapping:
     s/LSR/Proxy LSR/
     s/inclusions/inclusion/
You use the abbreviation DS/DDMAP in the following section. Perhaps, 
since you spell that out in this section, you might add "(DS/DDMAP)"
before "TLV".

12. Same section, next paragraph:
     s/vary/varies/
Capitalization is inconsistent: Proxy/proxy, egress/ Egress.

The last sentence is referring to Section 3.2.1. Suggest either copying 
that section's heading exactly or using the section number.

13. Section 3.2.1: inconsistent capitalization of "egress".

14. Section 3.2.2:

     s'DSMAP/DDMAP'DS/DDMAP' (3 instances) or spell out if this is a
     new abbreviation.

     In the first paragraph, mLDP is not a well-known abbreviation 
<http://www.rfc-editor.org/rfc-style-guide/abbrev.expansion.txt>, hence 
needs to be spelled out. ("Multipoint LDP", RFC 6826).

     In the second paragraph, fourth from last line: s/egresses/egress/

15. Section 3.2.4.1 first paragraph, second sentence:

OLD

    If Next Hop sub-TLVs were
    included in the received Proxy Parameters TLV, the Next_Hop_List
    created from the address in those sub-TLVs as adjusted above.

NEW

    If Next Hop sub-TLVs were
    included in the received Proxy Echo Parameters TLV, the Next_Hop_List
    is created from the addresses in those sub-TLVs adjusted as
    described in Section 3.2.

16. Section 5.1, first paragraph:
     s/to either to/either to/

     Why are "Composing" and "Sending" capitalized?

     Seventh line down, end of line, add comma after "stack".

     Next line: s/ie/i.e.,/

     Next line: s/Proxy Reply/MPLS Proxy Ping Reply/

17. Section 5.1, Proxy Flags field description, 0x01, first paragraph:
      Fourth line: s/FEC types/FEC type/
      Fifth line: s/LSPs/LSP/.
      Following sentence:

OLD
             The Proxy LSR MUST
             respond as applicable with a Upstream Neighbor Address TLV
             and Downstream Neighbor Address TLV(s) in the MPLS proxy
             ping reply message.

NEW
             The Proxy LSR MUST
             respond as applicable with Upstream Neighbor Address
             and Downstream Neighbor Address TLV(s) in the MPLS proxy
             ping reply message.

Following sentence: begin with "The".

Last sentence:

OLD
     for which the LSR learned bindings from.

NEW
     from which the LSR learned bindings.

17. Section 5.1, Proxy Flags field description, 0x01, second paragraph:
     s/an Echo request/any MPLS Echo Request/

     Second sentence:

OLD
     Information learned with such proxy reply may be used by the proxy
     initiator to generate subsequent proxy requests.

NEW
     The initiator may use information learned from the MPLS proxy
     ping reply that is sent instead to generate subsequent proxy
     requests.

[Also applies to the last sentence of the 0x04 description)

18. Section 5.1, Sub-TLVs: "TLV-encoded list" means to my mind that 
there is a TLV type and length preceding the first Sub-TLV. Maybe you 
mean "List of TLV-encoded sub-TLVs"?

19. Section 5.1.1, "Next Hop Interface": why is the interface identifier 
16 octets long in the case of IPv6 numbered? Should there be text saying 
that numbered interfaces are identified by the addresses assigned to 
them, unnumbered interfaces by ??

20. Section 6, second paragrph: refers to "these points". What points?

21. IANA Considerations

It would be helpful to indicate that the registries involved are found 
under the heading "Multi-Protocol Label Switching (MPLS) Label Switched 
Paths (LSPs) Ping Parameters".

---

From: "Adrian Farrel" <[email-masked]>
Date: Fri, 13 Feb 2015 21:43:52 -0000

Tom,

Thanks for the detailed review. Much appreciated. Late is better than not at all, and one day is not the end of the world.

> Please resolve these comments along with any other Last Call comments

Well, we'll handle the comments, of course, but we'll make a special trip to do that since Last Call comments have already been handled.

Authors - I've pushed the I-D out to the 5/3 telechat. Sorry for the delay that represents, but we have to give the IESG time to review the final version that you'll produce by addressing Tom's comments.

Adrian