On 16 Sep 2024, at 16:14, Mike Pattrick wrote:
> 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. FYI you could pause the revalidator, but not sure if that solves the problem. I’ll wait for Ilya to respond, as it was his earlier question. //Eelco > 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
