Re: [bess] Shepherd's review of draft-ietf-bess-mvpn-fast-failover

[Greg Mirsky <[e-mail]>]

Hi Stephane,
thank you for the review and your thoughtful comments. Please find my
answers and notes in-lined under GIM>> tag.
Attached, please find the diff and copy of the working version.

Regards,
Greg

Hi,



Please find below my review of the document.



Nits:



  Checking nits according to https://www.ietf.org/id-info/checklist :

----------------------------------------------------------------------------

  == There are 1 instance of lines with non-RFC6890-compliant IPv4 addresses
     in the document.  If these are example addresses, they should be
changed.

  == There are 1 instance of lines with non-RFC3849-compliant IPv6 addresses
     in the document.  If these are example addresses, they should be
changed.

 GIM>> MPLS OAM, i.e., LSP Ping and BFD, often uses IP/UDP encapsulation.
In these cases, the destination IP address is one of loopback addresses.
RFC 4379 (RFC 8029) and RFC 5884 have updated RFC 1122 in how routers
process packets with the destination IP address from 127/8 range (but only
for the specified UDP ports).



Section 3:

s/no longer “known to down”/no longer “known to be down”/ ?
GIM>> Fixed


Section 3.1.1:

As the document is standard track, could you introduce normative language
in the expected behavior description ?

GIM>> I can propose the following modification of the text:
 OLD TEXT:
   If BGP next-hop tracking is done for VPN routes and the root address
   of a given tunnel happens to be the same as the next-hop address in
   the BGP auto-discovery route advertising the tunnel, then this
   mechanisms may be omitted for this tunnel, as it will not bring any
   specific benefit.
NEW TEXT:
   If BGP next-hop tracking is done for VPN routes and the root address
   of a given tunnel happens to be the same as the next-hop address in
   the BGP auto-discovery route advertising the tunnel, then this
   mechanisms may be omitted for this tunnel, as it will not bring any
   specific benefit.

Section 3.1.2:

As the document is standard track, could you introduce normative language
in the expected behavior description ?



“This method should not be used”. Wouldn’t this be a normative statement ?
GIM>> Would the following modification of the text be acceptable:
OLD TEXT:
   This method should not be used when there is a fast restoration
   mechanism (such as MPLS FRR [RFC4090]) in place for the link.
NEW TEXT:
    Using this method when a fast restoration mechanism (such as MPLS FRR
   [RFC4090]) is in place for the link requires careful consideration
   and coordination of defect detection intervals for the link and the
   tunnel.  In many cases, it is not practical to use both methods at
   the same time.


Section 3.1.4:

As the document is standard track, could you introduce normative language
in the expected behavior description ?
GIM>> Updating to the normative language as follows:
OLD TEXT:
   A PE can be removed from the UMH candidate list for a given (C-S,
   C-G) if the P-tunnel (I or S, depending) for this (S, G) is leaf
   triggered (PIM, mLDP), but for some reason internal to the protocol
   the upstream one-hop branch of the tunnel from P to PE cannot be
   built.  In this case, the downstream PE can immediately update its
   UMH when the reachability condition changes.
NEW TEXT:
   A PE MAY be removed from the UMH candidate list for a given (C-S,
   C-G) if the P-tunnel (I-PMSI or S-PMSI) for this (S, G) is leaf-
   triggered (PIM, mLDP), but for some reason internal to the protocol
   the upstream one-hop branch of the tunnel from P to PE cannot be
   built.  In this case, the downstream PE MAY immediately update its
   UMH when the reachability condition changes.


s/(I or S , depending)/(I-PMSI or S-PMSI)/
GIM>> Done (above)


It is not crystal clear which branch exactly is the “upstream one-hop
branch of the tunnel from P to PE” ? Do you refer to the PEreceiver-P
branch ? or P – PE_source branch ?
GIM>> It is the latter. Would the following re-wording make it clearer:
OLD TEXT:
   A PE can be removed from the UMH candidate list for a given (C-S,
   C-G) if the P-tunnel (I or S, depending) for this (S, G) is leaf
   triggered (PIM, mLDP), but for some reason internal to the protocol
   the upstream one-hop branch of the tunnel from P to PE cannot be
   built.
NEW TEXT:
   An upstream PE MAY be removed from the UMH candidate list for a given
   (C-S, C-G) if the P-tunnel (I-PMSI or S-PMSI) for this (S, G) is
   leaf-triggered (PIM, mLDP), but for some reason, internal to the
   protocol, the upstream one-hop branch of the tunnel from P to PE
   cannot be built.


Section 3.1.5:

As the document is standard track, could you introduce normative language
in the expected behavior description ?
GIM>> I propose the following update to the second paragraph of this
sub-section:
OLD TEXT:
   When such a procedure is used, in the context where fast restoration
   mechanisms are used for the P-tunnels, downstream PEs should be
   configured to wait before updating the UMH, to let the P-tunnel
   restoration mechanism happen.  A configurable timer MUST be provided
   for this purpose, and it is recommended to provide a reasonable
   default value for this timer.
NEW TEXT:
   When such a procedure is used, in the context where fast restoration
   mechanisms are used for the P-tunnels, a configurable timer MUST be
   configured on the downstream PE to wait before updating the UMH, to
   let the P-tunnel restoration mechanism happen.  It is RECOMMENDED to
   provide a reasonable default value for this timer.  An implementation
   MAY use three seconds as the default value for this timer.


“it is recommended to provide a reasonable

   default value for this timer.”

Why not providing it in this document ?
GIM>> Though the restoration time of P-tunnel may differ depending on the
method being used, I think that 3 seconds may be the default. The NEW TEXT
above includes the proposed update.


“In cases where this mechanism is used in conjunction with
   Hot leaf standby »
Can you add the section reference for the Hot leaf standby ?
GIM>> I've changed the reference from the title to the number of the
section:
   In cases where this mechanism is used in conjunction with the method
   described in Section 5, no prior knowledge of the rate of the
   multicast streams is required; downstream PEs can compare reception
   on the two P-tunnels to determine when one of them is down.

Section 3.1.6:

As the document is standard track, could you introduce normative language
in the expected behavior description ?
GIM>> Sub-sections of 3.1.6 define the use of RFC 8562 and the new
attribute. In the introduction to these sub-sections, I propose s/can/MAY/

Could you use the right spacing in “BGP- BFD attribute” ? Would “BFD
discriminator” attribute or just “BFD” attribute be a better name ? we know
that it is BGP 😊
GIM>> I agree. Will use BFD Discriminator throughout the document.

>From a wider perspective, do you foresee other use case of signaling BFD
information in BGP ? I’m just wondering if we may need something extensible
for future use or not.
GIM>> Great question. BGP, and I'm speculating here, may be used to for
other BFD-related scenarios. I think that we may use the Flags field.

Even in this use case, I’m wondering if the BFD session type should be
included as part of the attribute rather than being deduced from the fact
that the attribute is attached to an x-PMSI route. I would be glad to hear
your feedback on that.
GIM>> Great suggestion, thank you. I've added the new p2mp Session value
for Flags and the request to IANA to create the appropriate new registry.
Please see the attached diffs and the working version of the draft.




Section 3.1.6.2:

s/Then The/ Then the/
GIM>> Thank you. Done.

“A different p2mp BFD session MAY monitor the state of the
   Standby Upstream PE. »

This is not fully clear to me. I thought that this BFD session was used as
part of the UMH selection process, which means that there is no UMH
selected yet. Do you mean monitoring an alternate P tunnel rooted at a
different upstream PE ?
GIM>> Thank you for pointing to this text. I agree, it can be improved and
below is the proposed update:
OLD TEXT:
   According to [RFC8562], if the Downstream PE receives Down or
   AdminDown in the State field of the BFD control packet or associated
   with the BFD session Detection Timer expires, the BFD session state
   is down, i.e., bfd.SessionState == Down.  When the BFD session state
   is Down, then the P-tunnel associated with the BFD session as down
   MUST be declared down.  Then The Downstream PE MAY initiate a
   switchover of the traffic from the Primary Upstream PE to the Standby
   Upstream PE only if the Standby Upstream PE deemed available.  A
   different p2mp BFD session MAY monitor the state of the Standby
   Upstream PE.
NEW TEXT:
   According to [RFC8562], if the Downstream PE receives Down or
   AdminDown in the State field of the BFD control packet or associated
   with the BFD session Detection Timer expires, the BFD session is
   down, i.e., bfd.SessionState == Down.  When the BFD session state is
   Down, then the P-tunnel associated with the BFD session MUST be
   declared down.  As a result, the Downstream PE MAY initiate a
   switchover of the traffic from the Primary Upstream PE to the Standby
   Upstream PE only if the Standby Upstream PE deemed available.  A
   different p2mp BFD session MAY be used to monitor the state of the
   P-tunnel from Standby Upstream PE.


Section 3.1.7

This is not clear to me.

Are you using BFD only to signal a Down state without really “probing” ?
GIM>> This section describes how the existing p2Mp BFD session from the
upstream PE to downstream PEs can be used to relay the state change
(failure) of the upstream PE-CE link. The exact mechanism to detect the
failure of the PE-CE link is not specified though it could be a single-hop
BFD session.


Section 4

“This non-revertive behavior can also be optionally supported by an
   implementation and would be enabled through some configuration.”


Is it a “MAY also be supported” ?
GIM>> Yes, updated to use MAY.



What if there are more than two PEs connected to the source ?
GIM>> The scenario in Section 4 is with the Primary and Standby Upstream
PEs. Each downstream PE MUST support revertive behavior. I'd expect that
non-revertive behavior is used consistently across all downstream PEs.


Section 4.1:

“The normal and the standby C-multicast routes must have their Local”
Is it a “MUST” ?
GIM>> Updated to MUST

It would be great to add a text talking about what happens if an
implementation does not understand the standby community.
GIM>> As I understand, if a BGP peer does not support Standby PE community
then it does not support this specification. If that is the case, then the
following may happen (as described in the document):
... two different downstream PEs consider different upstream PEs to be the
   primary one; in that case, without any precaution taken, both
   upstream PEs would process a standby C-multicast route and possibly
   stop forwarding at the same time.
Would adding to "Such a situation can happen when, for instance, due to
transient unicast routing inconsistencies ..." "or lack of support of the
Standby PE community" address your concern?

“Note that, when a PE advertises such a Standby C-multicast join for
   an (C-S, C-G) it must join the corresponding P-tunnel.”
Is it a “MUST” ?
 GIM>> Indeed, thank you. s/must/MUST/

Section 4.2
How does it determine that C-S is not reachable through some other PE ?
 GIM>> I imagine that several methods can be used to detect a failure. I've
added a note that "the use of the particular method to detect the failure
is outside the scope of this document". Would this update address your
concern?

Section 4.4.2:

Please you normative language.

Security section:
Please add some text/
 GIM>> So sorry. The added text is very basic, I know. Please suggest which
specific mechanisms described in the document require explicit discussion
in this section.

IANA considerations:

You need a request for the BFD attribute.
GIM>> Updated, please let me know if it is acceptable now.

You need to be more precise on the community request (from which registry
is it taken from). I suppose that you are requested a Well-Known community.
Please use upper case for the community name, it is more compliant with
other name used.