Acked-by: Ethan Jackson <et...@nicira.com>
On Tue, Apr 29, 2014 at 4:15 PM, Alex Wang <al...@nicira.com> wrote:
> This commit adds a requirement that bfd session must receive at least
> one bfd control packet every 100 * bfd->cfg_min_rx amount of time in
> forwarding_if_rx mode. Otherwise, even if the data packets are received
> on the monitored interface, the bfd->forwarding is still false.
>
> Since the datapath flow is not purged when the userspace Open Vswitch
> crashes, data packet can still be forwarded through the tunnel and
> fool the remote BFD session in forwarding_if_rx mode. Thus, this commit
> can prevent the remote BFD session from falsely declaring tunnel liveness
> in this situation.
>
> Signed-off-by: Alex Wang <al...@nicira.com>
> ---
> PATCH -> V2:
> - hard code the demand_rx_bfd interval to be 100 * cfm_interval.
> ---
> lib/bfd.c | 27 ++++++++++----
> tests/bfd.at | 101
> ++++++++++++++++++++++++++++++++++++++------------
> vswitchd/vswitch.xml | 11 ++++--
> 3 files changed, 105 insertions(+), 34 deletions(-)
>
> diff --git a/lib/bfd.c b/lib/bfd.c
> index e3e3ae5..2d53bd2 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -203,6 +203,12 @@ struct bfd {
> bool forwarding_if_rx;
> long long int forwarding_if_rx_detect_time;
>
> + /* When 'bfd->forwarding_if_rx' is set, at least one bfd control packet
> + * is required to be received every 100 * bfd->cfg_min_rx. If bfd
> + * control packet is not received within this interval, even if data
> + * packets are received, the bfd->forwarding will still be false. */
> + long long int demand_rx_bfd_time;
> +
> /* BFD decay related variables. */
> bool in_decay; /* True when bfd is in decay. */
> int decay_min_rx; /* min_rx is set to decay_min_rx when */
> @@ -841,6 +847,10 @@ bfd_process_packet(struct bfd *bfd, const struct flow
> *flow,
> }
> /* XXX: RFC 5880 Section 6.8.6 Demand mode related calculations here. */
>
> + if (bfd->forwarding_if_rx) {
> + bfd->demand_rx_bfd_time = time_msec() + 100 * bfd->cfg_min_rx;
> + }
> +
> out:
> bfd_forwarding__(bfd);
> ovs_mutex_unlock(&mutex);
> @@ -876,19 +886,22 @@ bfd_set_netdev(struct bfd *bfd, const struct netdev
> *netdev)
> static bool
> bfd_forwarding__(struct bfd *bfd) OVS_REQUIRES(mutex)
> {
> - long long int time;
> + long long int now = time_msec();
> + bool forwarding_if_rx;
> bool last_forwarding = bfd->last_forwarding;
>
> if (bfd->forwarding_override != -1) {
> return bfd->forwarding_override == 1;
> }
>
> - time = bfd->forwarding_if_rx_detect_time;
> - bfd->last_forwarding = (bfd->state == STATE_UP
> - || (bfd->forwarding_if_rx && time > time_msec()))
> - && bfd->rmt_diag != DIAG_PATH_DOWN
> - && bfd->rmt_diag != DIAG_CPATH_DOWN
> - && bfd->rmt_diag != DIAG_RCPATH_DOWN;
> + forwarding_if_rx = bfd->forwarding_if_rx
> + && bfd->forwarding_if_rx_detect_time > now
> + && bfd->demand_rx_bfd_time > now;
> +
> + bfd->last_forwarding = (bfd->state == STATE_UP || forwarding_if_rx)
> + && bfd->rmt_diag != DIAG_PATH_DOWN
> + && bfd->rmt_diag != DIAG_CPATH_DOWN
> + && bfd->rmt_diag != DIAG_RCPATH_DOWN;
> if (bfd->last_forwarding != last_forwarding) {
> bfd->flap_count++;
> bfd_status_changed(bfd);
> diff --git a/tests/bfd.at b/tests/bfd.at
> index 3723d60..f6f5592 100644
> --- a/tests/bfd.at
> +++ b/tests/bfd.at
> @@ -401,10 +401,9 @@ AT_CLEANUP
>
> # forwarding_if_rx Test1
> # Test1 tests the case when bfd is only enabled on one end of the link.
> -# Under this situation, the bfd state should be DOWN and the forwarding
> -# flag should be FALSE by default. However, if forwarding_if_rx is
> -# enabled, as long as there is packet received, the bfd forwarding flag
> -# should be TRUE.
> +# Under this situation, the forwarding flag should always be false, even
> +# though there is data packet received, since there is no bfd control
> +# packet received during the demand_rx_bfd interval.
> AT_SETUP([bfd - bfd forwarding_if_rx - bfd on one side])
> OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy -- \
> add-port br1 p1 -- set Interface p1 type=patch \
> @@ -438,15 +437,8 @@ do
> AT_CHECK([ovs-ofctl packet-out br1 3 2
> "90e2ba01475000101856b2e80806000108000604000100101856b2e80202020300000000000002020202"],
> [0], [stdout], [])
> done
> -# the forwarding flag should be true, since there is data received.
> -BFD_CHECK([p0], [true], [false], [none], [down], [No Diagnostic], [none],
> [down], [No Diagnostic])
> -
> -# reset bfd forwarding_if_rx.
> -AT_CHECK([ovs-vsctl set Interface p0 bfd:forwarding_if_rx=false], [0])
> -# forwarding flag should turn to false since the STATE is DOWN.
> +# the forwarding flag should be false, due to the demand_rx_bfd.
> BFD_CHECK([p0], [false], [false], [none], [down], [No Diagnostic], [none],
> [down], [No Diagnostic])
> -BFD_CHECK_TX([p0], [1000ms], [1000ms], [0ms])
> -BFD_CHECK_RX([p0], [500ms], [500ms], [1ms])
>
> AT_CHECK([ovs-vsctl del-br br1], [0], [ignore])
> AT_CLEANUP
> @@ -605,6 +597,74 @@ BFD_CHECK_RX([p0], [1000ms], [1000ms], [300ms])
> AT_CHECK([ovs-vsctl del-br br1], [0], [ignore])
> AT_CLEANUP
>
> +# forwarding_if_rx Test4
> +# Test4 is for testing the demand_rx_bfd feature.
> +# bfd is enabled on both ends of the link.
> +AT_SETUP([bfd - bfd forwarding_if_rx - demand_rx_bfd])
> +OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy -- \
> + add-port br1 p1 -- set Interface p1 type=patch \
> + options:peer=p0 ofport_request=2 -- \
> + add-port br0 p0 -- set Interface p0 type=patch \
> + options:peer=p1 ofport_request=1 -- \
> + set Interface p0 bfd:enable=true bfd:min_tx=300
> bfd:min_rx=300 bfd:forwarding_if_rx=true -- \
> + set Interface p1 bfd:enable=true bfd:min_tx=500
> bfd:min_rx=500])
> +
> +ovs-appctl time/stop
> +# advance the clock, to stablize the states.
> +for i in `seq 0 19`; do ovs-appctl time/warp 500; done
> +BFD_CHECK([p0], [true], [false], [none], [up], [No Diagnostic], [none],
> [up], [No Diagnostic])
> +BFD_CHECK([p1], [true], [false], [none], [up], [No Diagnostic], [none],
> [up], [No Diagnostic])
> +BFD_CHECK_TX([p0], [500ms], [300ms], [500ms])
> +
> +# disable the bfd on p1.
> +AT_CHECK([ovs-vsctl set Interface p1 bfd:enable=false], [0])
> +
> +# advance clock by 4000ms, while receiving packets.
> +# the STATE should go DOWN, due to Control Detection Time Expired.
> +# but forwarding flag should be still true.
> +for i in `seq 0 7`
> +do
> + ovs-appctl time/warp 500
> + AT_CHECK([ovs-ofctl packet-out br1 3 2
> "90e2ba01475000101856b2e80806000108000604000100101856b2e80202020300000000000002020202"],
> + [0], [stdout], [])
> +done
> +BFD_CHECK([p0], [true], [false], [none], [down], [Control Detection Time
> Expired], [none], [down], [No Diagnostic])
> +
> +# advance clock long enough to trigger the demand_bfd_rx interval
> +# (100 * bfd->cfm_min_rx), forwarding flag should go down since there
> +# is no bfd control packet received during the demand_rx_bfd.
> +for i in `seq 0 120`
> +do
> + ovs-appctl time/warp 300
> + AT_CHECK([ovs-ofctl packet-out br1 3 2
> "90e2ba01475000101856b2e80806000108000604000100101856b2e80202020300000000000002020202"],
> + [0], [stdout], [])
> +done
> +BFD_CHECK([p0], [false], [false], [none], [down], [Control Detection Time
> Expired], [none], [down], [No Diagnostic])
> +
> +# now enable the bfd on p1 again.
> +AT_CHECK([ovs-vsctl set Interface p1 bfd:enable=true], [0])
> +# advance clock by 5000ms. and p1 and p0 should be all up.
> +for i in `seq 0 9`; do ovs-appctl time/warp 500; done
> +BFD_CHECK([p0], [true], [false], [none], [up], [Control Detection Time
> Expired], [none], [up], [No Diagnostic])
> +BFD_CHECK([p1], [true], [false], [none], [up], [No Diagnostic], [none],
> [up], [Control Detection Time Expired])
> +BFD_CHECK_TX([p0], [500ms], [300ms], [500ms])
> +
> +# disable the bfd on p1 again.
> +AT_CHECK([ovs-vsctl set Interface p1 bfd:enable=false], [0])
> +# advance clock long enough to trigger the demand_rx_bfd,
> +# forwarding flag should go down since there is no bfd control packet
> +# received during the demand_rx_bfd.
> +for i in `seq 0 120`
> +do
> + ovs-appctl time/warp 300
> + AT_CHECK([ovs-ofctl packet-out br1 3 2
> "90e2ba01475000101856b2e80806000108000604000100101856b2e80202020300000000000002020202"],
> + [0], [stdout], [])
> +done
> +BFD_CHECK([p0], [false], [false], [none], [down], [Control Detection Time
> Expired], [none], [down], [No Diagnostic])
> +
> +AT_CHECK([ovs-vsctl del-br br1], [0], [ignore])
> +AT_CLEANUP
> +
> # test bfd:flap_count.
> # This test contains three part:
> # part 1. tests the flap_count on normal bfd monitored link.
> @@ -683,6 +743,7 @@ BFD_VSCTL_LIST_IFACE([p1], ["s/^.*flap_count=\(.*\),
> forwarding.*$/\1/p"], ["1"]
>
> # Part-3 now turn on forwarding_if_rx.
> AT_CHECK([ovs-vsctl set Interface p0 bfd:forwarding_if_rx=true], [0])
> +for i in `seq 0 10`; do ovs-appctl time/warp 100; done
> # disable the bfd on p1.
> AT_CHECK([ovs-vsctl set Interface p1 bfd:enable=false], [0])
>
> @@ -699,9 +760,9 @@ BFD_CHECK([p0], [true], [false], [none], [down], [Control
> Detection Time Expired
> # flap_count should remain unchanged.
> BFD_VSCTL_LIST_IFACE([p0], ["s/^.*flap_count=\(.*\), forwarding.*$/\1/p"],
> ["5"])
>
> -# stop the traffic for 4000ms, the forwarding flag of p0 should turn false.
> +# stop the traffic for more than 100 * bfd->cfm_min_rx ms, the forwarding
> flag of p0 should turn false.
> # and there should be the increment of flap_count.
> -for i in `seq 0 7`; do ovs-appctl time/warp 500; done
> +for i in `seq 0 120`; do ovs-appctl time/warp 100; done
> BFD_CHECK([p0], [false], [false], [none], [down], [Control Detection Time
> Expired], [none], [down], [No Diagnostic])
> BFD_VSCTL_LIST_IFACE([p0], ["s/^.*flap_count=\(.*\), forwarding.*$/\1/p"],
> ["6"])
>
> @@ -712,21 +773,15 @@ do
> AT_CHECK([ovs-ofctl packet-out br1 3 2
> "90e2ba01475000101856b2e80806000108000604000100101856b2e80202020300000000000002020202"],
> [0], [stdout], [])
> done
> -BFD_CHECK([p0], [true], [false], [none], [down], [Control Detection Time
> Expired], [none], [down], [No Diagnostic])
> -# flap_count should be incremented again.
> -BFD_VSCTL_LIST_IFACE([p0], ["s/^.*flap_count=\(.*\), forwarding.*$/\1/p"],
> ["7"])
> -
> -# stop the traffic for 4000ms, the forwarding flag of p0 should turn false.
> -# and there should be the increment of flap_count.
> -for i in `seq 0 7`; do ovs-appctl time/warp 500; done
> +# forwarding should be false, since there is still no bfd control packet
> received.
> BFD_CHECK([p0], [false], [false], [none], [down], [Control Detection Time
> Expired], [none], [down], [No Diagnostic])
> -BFD_VSCTL_LIST_IFACE([p0], ["s/^.*flap_count=\(.*\), forwarding.*$/\1/p"],
> ["8"])
> +BFD_VSCTL_LIST_IFACE([p0], ["s/^.*flap_count=\(.*\), forwarding.*$/\1/p"],
> ["6"])
>
> # turn on the bfd on p1.
> AT_CHECK([ovs-vsctl set interface p1 bfd:enable=true])
> for i in `seq 0 49`; do ovs-appctl time/warp 100; done
> # even though there is no data traffic, since p1 bfd is on again, should
> increment the flap_count.
> -BFD_VSCTL_LIST_IFACE([p0], ["s/^.*flap_count=\(.*\), forwarding.*$/\1/p"],
> ["9"])
> +BFD_VSCTL_LIST_IFACE([p0], ["s/^.*flap_count=\(.*\), forwarding.*$/\1/p"],
> ["7"])
> BFD_VSCTL_LIST_IFACE([p1], ["s/^.*flap_count=\(.*\), forwarding.*$/\1/p"],
> ["1"])
>
> OVS_VSWITCHD_STOP
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 91dfdf9..d17da50 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -1968,10 +1968,13 @@
> </column>
>
> <column name="bfd" key="forwarding_if_rx" type='{"type": "boolean"}'>
> - True to consider the interface capable of packet I/O as long as it
> - continues to receive any packets (not just BFD packets). This
> - prevents link congestion that causes consecutive BFD control
> packets
> - to be lost from marking the interface down.
> + When <code>true</code>, traffic received on the
> + <ref table="Interface"/> is used to indicate the capability of
> packet
> + I/O. BFD control packets are still transmitted and received. At
> + least one BFD control packet must be received every 100 * <ref
> + column="bfd" key="min_rx"/> amount of time. Otherwise, even if
> + traffic are received, the <ref column="bfd" key="forwarding"/>
> + will be <code>false</code>.
> </column>
>
> <column name="bfd" key="cpath_down" type='{"type": "boolean"}'>
> --
> 1.7.9.5
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
