Re: [bess] Alvaro Retana's Discuss on draft-ietf-bess-mvpn-fast-failover-13: (with DISCUSS and COMMENT)

[Greg Mirsky <[e-mail]>]

Hi Alvaro,
thank you for the review and comments. And thank you for the re-sent
comments as I was one of those who missed it the first time (may try
blaming Gmail). Please find my answers, notes below tagged by GIM>>,

Also, attached is the diff highlighting the updates.

Regards,
Greg

On Thu, Dec 17, 2020 at 12:47 PM Alvaro Retana <[e-mail]>
wrote:

> Hi!
>
> For some reason the original DISCUSS message didn’t make it into
> everyone’s mailbox (including mine), so I’m “hijacking” this reply to
> resend the comments.
>
> Note that the archive has the messages:
> https://mailarchive.ietf.org/arch/msg/iesg/PByc1h2E-xSBnqXUtFTcltutEkQ/
>
> Alvaro.
>
>
> ===
>
>
>
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
>
> (1) This document describes several methods to determine the status of a
> tunnel (in §3), none of which "provide a "fast failover" solution when used
> alone, but can be used together with the mechanism described in Section 4"
> (§1).  §3 also says this:
>
>    An implementation may support any combination of the methods
>    described in this section and provide a network operator with control
>    to choose which one to use in the particular deployment.
>
> While §3.1 is clear in the fact that it is not a requirement for all
> downstream PEs to use the same mechanism, there are no guidelines to aid
> the
> operator to chose which mechanism to use.  Some cases may be obvious (e.g.
> §3.1.3 applies to tunnels of a specific type), but others are not.  I would
> like to see deployment considerations related to the
> advantages/disadvantages
> that each method may have in specific situations (including their possible
> combination).
>
GIM>> I think it might be not that simple to compare deployment challenges
and benefits resulting from the deployment of different P-tunnel monitoring
methods because we have to somehow abstract from an impact of choices made
by respective implementations. Also, there probably should be a reference
environment, a use case we agree with. I agree that such a comparative
analysis of P-tunnel monitoring methods is useful it doesn't seem to
benefit this document as the choice of a developer which ones to implement
and an operator which one to use don't affect the functionality discussed
in the draft. Would you agree?

>
>
> (2) The BFD Discriminator Attribute has a very narrow application in this
> document when compared to the potential other uses given the extensibility
> possibilities related to bootstrapping BFD.  I have serious concerns about
> the attribute being defined in this document, amongst a series of other
> mechanisms.
>
> (2a) The tunnel can be monitored without the new BGP Attribute (assuming
> proper configuration of course).  Why is that option is not even mentioned
> in
> the document?
>
GIM>> You are right, there could be other methods to bootstrap a p2mp BFD
session. But since RFC 8562 has no discussion of bootstrapping a session,
having it in this draft seemed somewhat out of place. We can add an
informational reference to Section 4 of draft-mirsky-mpls-p2mp-bfd
<https://datatracker.ietf.org/doc/draft-mirsky-mpls-p2mp-bfd/>where several
bootstrapping options discussed. Would that be acceptable?

>
> In fact, the document recommends deleting the BFD session if the Attribute
> is
> not present.  Why is that recommendation in place, and what are the cases
> when
> it can not be followed?
>
GIM>> That recommendation is to ensure that resources are not leaked and
this monitoring mechanism does not amplify possible DoS attacks. In course
of the discussion with Murray, we've changed the normative language and now
the deletion of the BFD state information is required
   o  the P2MP BFD session MUST be deleted.  The session MAY be deleted
      after some configurable delay, which should have a reasonable
      default.


>
>
> (2b) The fact that BFD monitoring can be achieved without the new attribute
> makes me think that the bootstrapping of BFD using BGP would be better
> served
> in a document produced by the BFD WG.  One of the editors has expressed the
> same opinion [1] [2].  Has a discussion taken place in the BFD WG (or at
> least
> with the Chairs) about this work?  Why was it not taken up there?
>
>
> [1]
> https://mailarchive.ietf.org/arch/msg/rtg-bfd/T1jVpgyXuPatTpuD_wA0JC3CT1c/
> [2] https://tools.ietf.org/wg/bess/minutes?item=minutes-96-bess.html

GIM>> Work on draft-mirsky-mpls-p2mp-bfd
<https://datatracker.ietf.org/doc/draft-mirsky-mpls-p2mp-bfd/?include_text=1>
is
progressing at MPLS WG. AFAIK, it is in the state Candidate for WG
Adoption. Moving the definition of the BFD Discriminator attribute to that
draft, as I understand, would require using it as the normative reference.
Comparing the current states of both documents, would severely delay the
publication of MVPN Fast Failover specification and likely affect
implementations of P-tunnel monitoring mechanisms. I hope you can agree
with the current organization of the document.

>
>
>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> (0) I support Ben's DISCUSS.
>
>
> (1) This document (currently) specifies a new BGP attribute, but I couldn't
> find any discussion about it in the idr mailing list.  Has idr been given
> the
> opportunity to review?
>
GIM>> I hope BESS Chairs and the Responsible AD can help with the answer.

>
>
> (2) s/is an OPTIONAL procedure/is an optional procedure
> This is not a normative statement to require capitalization.
>
GIM>>  I think that the use of the normative form is reasonable. The
sentence can be re-worded using MAY. For example:
OLD TEXT:
   The procedure described here is an OPTIONAL procedure that is based
   on a downstream PE taking into account the status of P-tunnels rooted
   at each possible Upstream PE, for including or not including each
   given PE in the list of candidate UMHs for a given (C-S, C-G) state.
NEW TEXT:
   The procedure described here MAY be used in a BGP/MPLS MVPN [RFC6513].
It  is based
   on a downstream PE taking into account the status of P-tunnels rooted
   at each possible Upstream PE, for including or not including each
   given PE in the list of candidate UMHs for a given (C-S, C-G) state.
We wanted to stress that without this mechanism BGP/MPLS MVPN, as defined
in RFCs 6513 and 6514, is fully functional and architecturally complete.
This draft discusses mechanisms that support the protection and faster
convergence in MVPN's control plane. And since these mechanisms only an
improvement compared to the BGP mechanisms, we wanted to emphasize that by
using the normative form. Would you agree?

>
>
> (3) "VRF Route Import BGP attribute" is mentioned twice (§3 and §5), but
> it is
> not an attribute; it is an extended community (rfc6514).
>
GIM>>  Thank you for pointing that out. Updated.

>
>
> (4) §3.1.1: "similar to BGP next-hop tracking"  Is this specified
> somewhere?
> I don't remember seeing a specification for next-hop tracking, but do know
> that implementations do it -- in an implementation-specific way.  Please
> add a
> little more text about what is meant/expected.
>
GIM>> Indeed, BGP next-hop tracking is internal to a system behavior that
has not been, to the best of my knowledge, been documented at IETF or any
SDO. Would the following text provide reasonable context information:
   That is similar to BGP next-hop tracking for VPN routes, except that
   the address considered is not the BGP next-hop address but the root
   address in the x-PMSI Tunnel attribute.  BGP next-hop tracking is a
   feature that reduces the BGP convergence time comparing to the
   "regular" BGP by monitoring BGP next-hop address changes in the
   routing table.  It's event-based because it detects changes in the
   routing table.  When it detects a change, it performs a next-hop scan
   to find if any of the next hops in the BGP table is affected and updates
   it accordingly.


>
> (5) §3.1.1: "If BGP next-hop tracking is done...and the root
> address...[is]...the same as the next-hop address...then checking...whether
> the tunnel root is reachable, will be unnecessary duplication...."  I guess
> this means that the existing next-hop tracking functionality can be used
> and
> that it doesn't have to be function-specific (root vs next-hop).  This
> paragraph seems to assume a specific implementation -- but again, given
> that
> next-hop tracking implementations are internal then this paragraph doesn't
> make much sense to me.
>
GIM>> This section is intended as informative since, as you've mentioned
earlier, there are many implementations of BGP next-hop tracking and the
functionality is enabled by default in some of the implementations. Our
intent was to point out that this well-known and broadly used mechanism can
be used in combination with PE redundancy to provide a faster convergence
in MVPN's control plane.

>
>
> (6) The "reachability condition" is mentioned in §3.1.1/§3.1.3/§3.1.4.
> Does
> this mean that that root tracking (§3.1.1) should be used with the other
> mechanisms?  The specific text says that "the downstream PE can immediately
> update its UMH when the reachability condition changes", giving the
> impression
> that the combination is possible but not required.
>
> Note that §4.3 is titled "Reachability Determination", which I hoped would
> shed more light, but all it does is point back to §3.1.
>
GIM>> I don't see any benefit of concurrently using more than one of the
described in the document mechanisms that can monitor the state of a
P-tunnel. That is certainly possible but, in my opinion, would add
unnecessary complexity. The last paragraph in Section 3.1 is intended as
the introduction to sub-sections that discuss different monitoring
mechanisms:
   An implementation may support any combination of the methods
   described in this section and provide a network operator with control
   to choose which one to use in the particular deployment.
Would you suggest an update to this paragraph to clarify the statement?

>
>
> (7) §3.1.2: What is the "last-hop link" of the tunnel?  Is it the physical
> link, or the virtual hop from the previous waypoint?
>
GIM>> A last-hop link is a link between a PE and the penultimate P node.

>
>
> (8) §3.1.2 mentions that "careful consideration and coordination" is needed
> when using other mechanisms such as rfc4090 "because uncorrelated timers
> might
> cause unnecessary switchovers and destabilize the network."  What are the
> associated timers related to the mechanisms in this section?
>
GIM>> This is in reference to the defect detection timers. When using
multi-layer protection particular consideration must be given to the
interaction of defect detections at different layers of a network. It s
recommended to use longer detection intervals at the higher layers. Some
recommendations suggest using a multiplier of 3 or larger, e.g., 10 msec
detection for FRR and at least 100 msec for e2e detection.

>
>
> (9) §3.1.3:
>
>    When using this method and if the signaling state for a P2MP TE LSP
>    is removed (e.g., if the ingress of the P2MP TE LSP sends a PathTear
>    message) or the P2MP TE LSP changes state from Up to Down as
>    determined by procedures in [RFC4875], the status of the
>    corresponding P-tunnel MUST be re-evaluated.  If the P-tunnel
>    transitions from Up to Down state, the Upstream PE that is the
>    ingress of the P-tunnel MUST NOT be considered a valid UMH.
>
> These two sentences are redundant.  It seems to me that they could be
> replaced by:
>
>    When using this method and if the signaling state for a P2MP TE LSP
>    is removed (e.g., if the ingress of the P2MP TE LSP sends a PathTear
>    message) or the P2MP TE LSP changes state from Up to Down as
>    determined by procedures in [RFC4875], the Upstream PE that is the
>    ingress of the P-tunnel MUST NOT be considered a valid UMH.
>
GIM>> Thank you for the suggested update of the text. I think that the
original text includes an important requirement to re-evaluate the status
of the corresponding P-tunnel rather than remove the Upstream PE from the
UMH candidate list only based on the change in p2mp TE LSP.

>
>
> (10) §3.1.4: "An Upstream PE SHOULD be removed from the UMH candidate
> list...if...the upstream one-hop branch of the tunnel from P to PE cannot
> be
> built."   When is it ok to not remove the PE?  IOW, why is this action not
> required?
>
GIM>> Thank you for bringing this case. It was discussed during Shepherd's
review and we've asked for the expert's opinion. Jeffrey Zhang kindly suggested
the current text.
<https://mailarchive.ietf.org/arch/browse/bess/?gbt=1&index=LxKYi9F6u1tl2qKtR2Q8wkQPMOA>
I'll
check with him and get back with the answer.

>
>
> (11) [nit] s/mechanism to execute its actions/mechanism execute its actions
>
GIM>> Thank you. Done

>
>
> (12) §3.1.5 says that "where this mechanism is used in conjunction with the
> method described in Section 5...downstream PEs can compare reception on the
> two P-tunnels to determine when one of them is down", but §5 says that
> "downstream PEs accept traffic from the primary or standby tunnel, based on
> the status of the tunnel (based on Section 3)".  IOW, §3.1.5 points at §5
> as
> providing a way to determine if a tunnel is down, while §5 points back at
> §3
> as the way to determine which tunnel to receive from.  This pointing back
> and
> forth is not a total contradiction, but it needs to be clarified.
>
 GIM>> Though it might appear as a circular reference, it was not the
intention. An implementation of the method described in the last paragraph
of Section 3.1.5 can periodically accept traffic from primary and standby
tunnels s the method of determining the state of the primary P-tunnel. I've
updated that paragraph to note that the comparison can be done periodically:
OLD TEXT:
   In cases where this mechanism is used in conjunction with the method
   described in Section 5, no prior knowledge of the rate or maximum
   inter-packet time on the multicast streams is required; downstream
   PEs can compare actual packet reception statistics on the two
   P-tunnels to determine when one of them is down.  The detailed
   specification of this mechanism is outside the scope of this
   document.
NEW TEXT:
   In cases where this mechanism is used in conjunction with the method
   described in Section 5, no prior knowledge of the rate or maximum
   inter-packet time on the multicast streams is required; downstream
   PEs can periodically compare actual packet reception statistics on
   the two P-tunnels to determine when one of them is down.  The
   detailed specification of this mechanism is outside the scope of this
   document.

>
>
> (13) §3.1.6: "An implementation that does not recognize or is configured
> not
> to support this attribute MUST follow procedures defined for optional
> transitive path attributes in Section 5 of [RFC4271]."
>
> There cannot be a Normative action specified for a node that "does not
> recognize...this attribute" because, by definition, it can't be assumed
> that
> it is aware of this specification.  In this case, it is not necessary to
> say
> anything about unrecognized attributes because that is already specified in
> rfc4271.
>
> For the "configured not to support this attribute" case, it should be
> pointed
> out that the node should operate as if the attribute was unrecognized.
>
> Suggestion>
>    An implementation that is configured not to support this attribute MUST
>    follow the procedures defined in Section 5 of [RFC4271] as if the
> attribute
>    was unrecognized.
>
GIM>> Thank you for pointing this out. In the course of addressing comments
from other IESG reviewers, this text was updated to:
   This document defines the format and ways of using a new BGP
   attribute called the "BFD Discriminator".  It is an optional
   transitive BGP attribute.  Thus it is expected that an implementation
   that does not recognize or is configured not to support this
   attribute follows procedures defined for optional transitive path
   attributes in Section 5 of [RFC4271].
Would the current text address your concern?

>
>
> (14) [nit] s/BFD Mode field is the one octet long./The BFD Mode field is
> one
> octet long.
>
GIM>> Thank you. Done.

>
>
> (15) §3.1.6: "The BFD Discriminator attribute MUST be considered malformed
> if
> its length is not a non-zero multiple of four."  Ok, except that the
> specification of the attribute doesn't mention the length (only the length
> of
> the TLVs).  Please specify the length and any considerations related to the
> Extended Length bit.  Also, given that this is a new attribute, with an
> unspecified potential number of TLVs, and that the length is apparently
> unbounded, all leading to the potential need for extended messages, please
> specify how to handle peers that cannot accommodate more than 4k octet
> messages (rfc8654).
>
GIM>> I've noticed a note from Jeff Hass. Would you agree with his opinion?

>
>
> (16) §3.1.6.1: "MUST set the IP destination address of the inner IP
> header to
> one of the internal loopback addresses..."   Where do these addresses come
> from?  How does the Upstream PE figure out which one to use?  At least
> please
> include a reference to where that is explained.
>
GIM>> Thank you for pointing out the missing reference to RFC 1122, added
it. I think that the selection of the inner destination IP address is an
implementation issue and how the address is selected should not affect a
MultipointTail node. Would you agree?

>
>
> (17) §3.1.6.1: "MUST use its IP address as the source IP address"  Which
> address?  Please be specific.
>
GIM>> Section 3.1.6.1 includes the list that an Upstream PE is required to
follow:
   To enable downstream PEs to track the P-tunnel status using a point-
   to-multipoint (P2MP) BFD session the Upstream PE:
....
   o  MUST use its IP address as the source IP address when transmitting
      BFD Control packets;
Would adding the reference to the Upstream PE address your concern:
    o  MUST use its, i.e., the Upstream PE's, IP address as the source IP
address
        when transmitting BFD Control packets;

>
>
> (18) §3.1.6.2: If the IP address doesn't map correctly at the downstream
> PE
> (for example, a different local address is used that doesn't correspond to
> the
> information in the PMSI attribute), what action should it take?  Can the
> tunnel still be monitored?
>
GIM>> There's a possibility that the same downstream PE is monitoring more
than one P-tunnel. Since each Upstream PE assigns its own BFD
Discriminator, there's a chance that the same value is picked by more than
one Upstream PE. According to Section 5.7 of the RFC 8562:
   IP and MPLS multipoint tails MUST demultiplex BFD packets based on a
   combination of the source address, My Discriminator, and the identity
   of the multipoint path that the multipoint BFD Control packet was
   received from.  Together they uniquely identify the head of the
   multipoint path.
We may consider adding the source address in the BFD Discriminator
attribute as an optional TLV. I think that might be a good extension that
can be introduced in a new document.

>
>
> (19) §3.1.6.2: "SHOULD NOT switch the traffic to the Standby Upstream PE"
> When is it ok to do it?  IOW, why is this action recommended, and not
> required.
>
GIM>> Thank you for pointing this out. I agree that it is the requirement.
s/SHOULD NOT/MUST NOT/

>
>
> (20) §3.1.7: "set the bfd.LocalDiag of the P2MP BFD session to Concatenated
> Path Down and/or Reverse Concatenated Path Down"    Which one?
>  bfd.LocalDiag
> carries a single value.
>
GIM>> Thank you. This is the editorial error, clearly, it is "or". Which of
the two values is used may depend upon the detection mechanism used
monitoring the CE-PE link.

>
>
> (21) §3.1.7: "...it is desired for the downstream PEs to switch to a backup
> Upstream PE.  To achieve that...it SHOULD set the bfd.LocalDiag..."  If not
> set, then the objective won't be achieved.  When is it ok to not set the
> bfd.LocalDiag to indicate the concatenated failure?
>
GIM>> I agree with you, that is the required behavior. s/SHOULD/MUST/

>
>
> (22) §4: "Such behavior is referred to as "revertive" behavior and MUST be
> supported."  The text around this sentence seems to indicate that the
> revertive behavior is the default, is that the intent?  Or if the intent
> for
> it just to be supported (as written)?  Please be clear.
>
GIM>> This part of the document has been updated in the course of
addressing IESG comments:
   Such behavior is referred to as
   "revertive" behavior and MUST be supported.  Non-revertive behavior
   refers to the behavior of continuing to select the backup PE as the
   UMH even after the Primary has come up.  This non-revertive behavior
   MAY also be supported by an implementation and would be enabled
   through some configuration.  Selection of the behavior, revertive or
   non-revertive, is an operational issue, but it MUST be consistent on
   all PEs in the given MVPN.
Do you find the updated text clear enough?

>
>
> (23) §4.1: "...routes that carry the "Standby PE" BGP Community MUST have
> the
> LOCAL_PREF attribute set to zero."  What should a receiver do if the
> LOCAL_PREF is not zero?
>
GIM>> I believe that the preceding text describes the situation when the
LOCAL_PREF != 0:
   ... 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.
>
>
>
> (24) §4.1: In the last paragraph of this section, if I follow correctly,
> the
> text talks about the case where the standby becomes the primary and the
> updated advertisement doesn't have the Standby PE community.  If that is
> correct, then s/ presence/absence of the Standby PE BGP Community/ absence
> of
> the Standby PE BGP Community
>
> Also, the last sentence says that the "LOCAL_PREF attribute MUST be set to
> zero".  If the community is not present, how can a receiver enforce this?
> What action should it take if the LOCAL_PREF has a different value?
>
GIM>> Thank you for the suggestion, it clarifies the text. As I understand,
the requirement is for the Standby Upstream PE, not for a downstream PE.
Would the following update make that clearer:
OLD TEXT:
   The LOCAL_PREF attribute MUST be set to zero.
NEW TEXT:
   The new Upstream PE MUST set the LOCAL_PREF attribute to
   zero for that C-multicast route.

>
>
> (25) §4.3: "other mechanisms MAY be used"   s/MAY/may   This is just a
> statement of fact.
>
GIM>> Agree.

>
>
> (26) §4.4.2: "MUST try to locate"   To try is not an action that can be
> normatively enforced.   Also, I don't think that using "MUST" here adds
> value
> since the normative action is in the next sentence ("MUST perform as
> follows"). s/MUST/must
>
GIM>> Agree.

>
>
> (27) s/"hot root standby" mode is used (Section 4)/"hot root standby" mode
> is
> used (Section 5)
>
GIM>> Thank you for catching the wrong reference. Updated.

>
>
> (28) §7.3: s/sub-TLV/TLV/g   The attribute includes TLVs, not sub-TLVs.
>
GIM>> Agree and updated.