Thanks for the review, pushed both patches to master and branch-2.2
On Wed, Apr 30, 2014 at 1:38 PM, Ethan Jackson <et...@nicira.com> wrote:
> 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
