[bess] Document shepherd review of draft-ietf-bess-evpn-lsp-ping-04

["Bocci, Matthew (Nokia - GB)" <[e-mail]>]

Authors,

I am the document shepherd for this draft. As is customary, please find below by review. Please treat these comment as you would any other working group last call comments.

Best regards

Matthew

General Comments.
================

Implementation status: Please can you indicate if there are any known implementations of this draft, as per https://mailarchive.ietf.org/arch/msg/bess/cG3X1tTqb_vPC4rg56SEdkjqDpw

I think the document is readable and useful. I have a few comments below. These are mostly related to the clarity of the text or are minor spelling/grammar issues that would be better to resolve before we forward the document to the IESG. However, there are some more significant issues related to the clarity of the specification.

As a general high level comment, there is a lack of RFC2119 language used in the body of the document. For example, in section 5 you say that “packets are encapsulated” but I think it would be better to use mandatory language such as “packets MUST be encapsulated”. Elsewhere, it is not clear what the normative behavior is. Please go through and apply terms such as “MUST’, ‘SHOULD’ etc as needed. Maybe look at RFC4379 for an example of how to use this terminology for LSP ping when applied to a service.

The mechanisms in this draft are presumably based on RFC4379 / RFC8029 (Detecting Multi-Protocol Label Switched (MPLS) Data Plane Failures), and I am assuming that an implementation must follow the basic rules in that RFC but use the EVPN identifiers in the target FEC stack. If so, perhaps you should make that clear.

There is a normative down reference to “draft-ietf-bess-evpn-prefix-advertisement-11”. This will delay the publication of the draft.

Please also check the use of the definite article (a, an, the, etc) in the draft. There are a few cases where it is used unnecessarily e.g. “the PE1”, or it is missing.

Detailed Comments
================

1. Introduction
[…]
BB-EVPN maintains the C-MAC learning in data plane
MB> Please expand ‘C-MAC’ on first use
[…]

This draft defines 4 new Sub-TLVs
MB> s/draft/document

[…]
2. Proposed Target FEC Stack Sub-TLVs

   This document introduces four new Target FEC Stack sub-TLVs that are
   included in the LSP-Ping Echo Request packet sent for detecting faults in data-plane connectivity in EVPN and PBB-EVPN networks.
   These Target FEC Stack sub-TLVs are described next.

MB> Maybe keep the purpose in line with RFC4385, which says the echo request is used to provide a connectivity check, rather than only saying we are detecting faults. Strictly speaking, we are detecting both a fault and the absence of a fault.

MB> Also, I suggest adding a sentence to clarify that what the target FEC stack TLVs are used for in the context of EVPN. Since we only introduce EVPN unicast FECs in draft-ietf-bess-evpn-oam-req-frmwk, maybe you can say something like “These MAY be used to validate that an identifier for a given EVPN is programmed at the target node.”

[…]

Throughout:
s/GAL label/GAL

The ‘L’ in the abbreviation means ‘Label’ so you don’t need to say it twice.

[…]


5.  Encapsulation of OAM Ping Packets

   The LSP Ping Echo request IPv4/UDP packets are encapsulated with the
   Transport and EVPN Label(s) followed by the Generic Associated
   Channel Label (GAL) [RFC6426] which is the bottom most label.  The
   GAL label is followed by IPv4(0x0021) or IPv6(0x0057) Associated
   Channel Header (ACH) [RFC4385].

MB> I think the references are incorrect. GAL is defined in RFC 5586, not RFC6426. RFC4385 is the reference for the PW ACH, not the G-ACH. RFC5586 gives you the format of the G-ACH that you use following the GAL. The code points for IPv4 and IPv6 channels are in Generic Associated Channel (G-ACh) Parameters (iana.org)<https://www.iana.org/assignments/g-ach-parameters/g-ach-parameters.xhtml>
So you should probably change the second reference to [RFC5586] and then the IANA registry.
[…]

6.2.2.  Using P2MP P-tree
[…]
When using Aggregate Inclusive P-tree, a PE announces an upstream
   assigned MPLS label along with the P-tree ID, in that case both the
   p2mp p-tree MPLS transport label and the upstream MPLS label can be
   used to identify the L2 service.

MB> this does not parse well. I suggest rephrasing to:
“When using Aggregate Inclusive P-tree, a PE announces an upstream
   assigned MPLS label along with the P-tree ID, so both the
   p2mp p-tree MPLS transport label and the upstream MPLS label can be
   used to identify the L2 service.”

[…]
The Leaf PE(s) of the p2mp tree will process the packet and perform
   checks for the EVPN Inclusive Multicast sub-TLV present in the Target
   FEC Stack TLV as described in Section 4.4 in [RFC8029] and respond
   according to [RFC8029] processing rules.  A PE that is not the DF for
   the EVI on the ESI in the Inclusive Multicast sub-TLV, will reply
   with a special code indicating that FEC exists on the router and the
   behavior is to drop the packet because of not DF as described in
   Section 8.

MB> I think you are defining a new return code here. I suggest rephrasing to something like:
“A PE that is not the DF for the EVI on the ESI in the Inclusive Multicast sub-TLV, will reply
   with a code indicating that “The FEC exists on the router and the
   behavior is to drop the packet because of ‘not DF’”, as described in
   Section 8.”
MB> Also, is this a ‘MUST’?