On Mon, Sep 16, 2024 at 6:18 AM Eelco Chaudron <[email protected]> wrote:
>
>
>
> On 13 Sep 2024, at 16:20, Mike Pattrick wrote:
>
> > Currently a bond will not always revalidate when an active member
> > changes. This can result in counter-intuitive behaviors like the fact
> > that using ovs-appctl bond/set-active-member will cause the bond to
> > revalidate but changing other_config:bond-primary will not trigger a
> > revalidate in the bond.
> >
> > When revalidation is not set but the active member changes in an
> > unbalanced bond, OVS may send traffic out of previously active member
> > instead of the new active member.
> >
> > This change will always mark unbalanced bonds for revalidation if the
> > active member changes.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2214979
> > Signed-off-by: Mike Pattrick <[email protected]>
>
> Hi Mike,
>
> I think you missed Ilya’s comment on v4. Let me quote him here:
>
> “Slightly orthogonal question: why this needs to be a system test
> with pings and other stuff? Simple 'unit' tests with dummy datapath
> are more reliable, much faster and running more frequently in CI.”
Hello,
I tried to make a dummy test but it didn't reproduce the bug. When I
set a dummy interface down, OVS revalidates immediately. I've also
made changes to the system test to make it more reliable and faster to
execute.
Cheers,
M
>
> Cheers,
>
> Eelco
>
> > ---
> > v2: Added a test
> > v3: Made the test more reliable
> > v4: Made test much more reliable
> > v5: Improved test performance
> > v6: Improved system test by removing waits on ping.
> > ---
> > ofproto/bond.c | 8 +++++--
> > tests/system-traffic.at | 51 +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 57 insertions(+), 2 deletions(-)
> >
> > diff --git a/ofproto/bond.c b/ofproto/bond.c
> > index 0858de374..b9e2282f0 100644
> > --- a/ofproto/bond.c
> > +++ b/ofproto/bond.c
> > @@ -193,6 +193,7 @@ static void bond_update_post_recirc_rules__(struct bond
> > *, bool force)
> > static bool bond_is_falling_back_to_ab(const struct bond *);
> > static void bond_add_lb_output_buckets(const struct bond *);
> > static void bond_del_lb_output_buckets(const struct bond *);
> > +static bool bond_is_balanced(const struct bond *bond)
> > OVS_REQ_RDLOCK(rwlock);
> >
> >
> > /* Attempts to parse 's' as the name of a bond balancing mode. If
> > successful,
> > @@ -549,11 +550,15 @@ bond_find_member_by_mac(const struct bond *bond,
> > const struct eth_addr mac)
> >
> > static void
> > bond_active_member_changed(struct bond *bond)
> > + OVS_REQ_WRLOCK(rwlock)
> > {
> > if (bond->active_member) {
> > struct eth_addr mac;
> > netdev_get_etheraddr(bond->active_member->netdev, &mac);
> > bond->active_member_mac = mac;
> > + if (!bond_is_balanced(bond)) {
> > + bond->bond_revalidate = true;
> > + }
> > } else {
> > bond->active_member_mac = eth_addr_zero;
> > }
> > @@ -1121,7 +1126,7 @@ bond_get_recirc_id_and_hash_basis(struct bond *bond,
> > uint32_t *recirc_id,
> > /* Rebalancing. */
> >
> > static bool
> > -bond_is_balanced(const struct bond *bond) OVS_REQ_RDLOCK(rwlock)
> > +bond_is_balanced(const struct bond *bond)
> > {
> > return bond->rebalance_interval
> > && (bond->balance == BM_SLB || bond->balance == BM_TCP)
> > @@ -1725,7 +1730,6 @@ bond_unixctl_set_active_member(struct unixctl_conn
> > *conn,
> > }
> >
> > if (bond->active_member != member) {
> > - bond->bond_revalidate = true;
> > bond->active_member = member;
> > VLOG_INFO("bond %s: active member is now %s",
> > bond->name, member->name);
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index 724b25fa9..2b7e5530b 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -291,6 +291,57 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3
> > -W 2 10.1.1.2 | FORMAT_PING
> > OVS_TRAFFIC_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > +AT_SETUP([datapath - bond active-backup failover])
> > +OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])])
> > +
> > +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> > +AT_CHECK([ovs-ofctl add-flow br1 "actions=normal"])
> > +
> > +ADD_NAMESPACES(at_ns0, at_ns1)
> > +
> > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> > +ADD_VETH(p1, at_ns1, br1, "10.1.1.2/24")
> > +on_exit 'ip link del link0a'
> > +on_exit 'ip link del link0b'
> > +AT_CHECK([ip link add link0a type veth peer name link1a])
> > +AT_CHECK([ip link add link0b type veth peer name link1b])
> > +
> > +AT_CHECK([ip link set dev link0a up])
> > +AT_CHECK([ip link set dev link1a up])
> > +AT_CHECK([ip link set dev link0b up])
> > +AT_CHECK([ip link set dev link1b up])
> > +
> > +AT_CHECK([ovs-vsctl add-bond br0 bond0 link0a link0b
> > bond_mode=active-backup])
> > +AT_CHECK([ovs-vsctl add-bond br1 bond1 link1a link1b
> > bond_mode=active-backup])
> > +
> > +dnl Set primary bond member.
> > +AT_CHECK([ovs-vsctl set port bond0 other_config:bond-primary=link0a -- \
> > + set port bond1 other_config:bond-primary=link1a])
> > +
> > +OVS_WAIT_UNTIL([test -n "`ovs-appctl bond/show | grep 'active-backup
> > primary: link0a'`"])
> > +
> > +OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.2])
> > +
> > +dnl Keep traffic active in the background.
> > +NETNS_DAEMONIZE([at_ns0], [ping -q 10.1.1.2], [ping.pid])
> > +
> > +dnl Check correct port is used.
> > +AT_CHECK([ovs-appctl dpctl/dump-flows -m | grep -Eq
> > "actions:link[[01]]a"], [0])
> > +AT_CHECK([ovs-appctl dpctl/dump-flows -m | grep -Eq
> > "actions:link[[01]]b"], [1])
> > +
> > +dnl Change primary bond member.
> > +AT_CHECK([ovs-vsctl set port bond0 other_config:bond-primary=link0b -- \
> > + set port bond1 other_config:bond-primary=link1b])
> > +
> > +OVS_WAIT_UNTIL([test -n "`ovs-appctl bond/show | grep 'active-backup
> > primary: link0b'`"])
> > +
> > +dnl Check correct port is used.
> > +AT_CHECK([ovs-appctl dpctl/dump-flows -m | grep -Eq
> > "actions:link[[01]]a"], [1])
> > +AT_CHECK([ovs-appctl dpctl/dump-flows -m | grep -Eq
> > "actions:link[[01]]b"], [0])
> > +
> > +OVS_TRAFFIC_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> > AT_SETUP([datapath - ping over vxlan tunnel])
> > AT_SKIP_IF([test $HAVE_NC = no])
> > OVS_CHECK_VXLAN()
> > --
> > 2.43.5
> >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
