On 10/4/24 16:27, Eelco Chaudron wrote: > > > On 3 Oct 2024, at 23:00, 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://issues.redhat.com/browse/FDP-845 >> Signed-off-by: Mike Pattrick <[email protected]> > > Thanks for the updated revision. The patch looks good to me. I'm not sure if > we still need the DP test now that we have the unit test. I'll leave it to > Ilya to decide, as he originally requested it.
I think we should remove the system test as it is very similar to the unit test. See some more comments below. > > //Eelco > > Acked-by: Eelco Chaudron <[email protected]> > >> --- >> 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. >> v7: Added a unit test. >> --- >> ofproto/bond.c | 8 +++++-- >> tests/ofproto-dpif.at | 40 ++++++++++++++++++++++++++++++++ >> tests/system-traffic.at | 51 +++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 97 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; >> + } Why we revalidate only if there is an active member? If all members end up disabled we should also revalidate to start dropping traffic directed to this bond. >> } 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/ofproto-dpif.at b/tests/ofproto-dpif.at >> index 12cb7f7a6..7a916de54 100644 >> --- a/tests/ofproto-dpif.at >> +++ b/tests/ofproto-dpif.at >> @@ -351,6 +351,46 @@ >> recirc_id(0),in_port(4),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:0b,dst=ff: >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([ofproto-dpif - active-backup bonding set primary]) >> + >> +OVS_VSWITCHD_START( >> + [add-bond br0 bond0 p1 p2 bond_mode=active-backup \ >> + other_config:bond-primary=p1 -- \ >> + set bridge br0 other-config:hwaddr=aa:66:aa:66:aa:00 -- \ >> + set interface p1 type=dummy options:pstream=punix:$OVS_RUNDIR/p1.sock >> ofport_request=1 -- \ >> + set interface p2 type=dummy options:pstream=punix:$OVS_RUNDIR/p2.sock >> ofport_request=2 -- \ >> + add-port br0 p7 -- set interface p7 ofport_request=7 type=dummy -- \ >> + add-br br1 -- \ >> + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ >> + set bridge br1 datapath-type=dummy -- \ >> + add-bond br1 bond1 p3 p4 bond_mode=active-backup \ >> + other_config:bond-primary=p3 -- \ >> + set interface p3 type=dummy options:stream=unix:$OVS_RUNDIR/p1.sock >> ofport_request=3 -- \ >> + set interface p4 type=dummy options:stream=unix:$OVS_RUNDIR/p2.sock >> ofport_request=4 -- \ >> + add-port br1 p8 -- set interface p8 ofport_request=8 type=dummy]) >> + >> +WAIT_FOR_DUMMY_PORTS([p3], [p4]) >> + >> +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) >> +AT_CHECK([ovs-ofctl add-flow br1 action=normal]) >> + >> +dnl Create datapath flow with bidirectional traffic. >> +AT_CHECK([ovs-appctl netdev-dummy/receive p8 >> 'in_port(8),eth(dst=50:54:00:00:00:09,src=50:54:00:00:00:0a),eth_type(0x0800),ipv4(dst=10.0.0.2,src=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) >> +AT_CHECK([ovs-appctl netdev-dummy/receive p7 >> 'in_port(7),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) >> +AT_CHECK([ovs-appctl netdev-dummy/receive p8 >> 'in_port(8),eth(dst=50:54:00:00:00:09,src=50:54:00:00:00:0a),eth_type(0x0800),ipv4(dst=10.0.0.2,src=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) >> +AT_CHECK([ovs-appctl netdev-dummy/receive p7 >> 'in_port(7),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) >> + I'd suggest to not swap places for src and dst, otherwise it's really hard to tell if these packets are different or not. >> +dnl Set p2 and p4 as primary. >> +AT_CHECK([ovs-vsctl set port bond0 other_config:bond-primary=p2 -- \ >> + set port bond1 other_config:bond-primary=p4]) >> + >> +OVS_WAIT_UNTIL([test -n "`ovs-appctl bond/show | grep 'active-backup >> primary: p4'`"]) There is no need for 'test', just grep -q would be enough. And the apperance of p4 in the bond/show output doesn't mean the revalidation is done, so there is a race here. We need to wait for revalidators: AT_CHECK([ovs-appctl revalidator/wait]) >> + >> +AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep "actions:p[[13]]"], >> [1], []) The [] at the end is not needed. Also, we need to check that flows didn't just expire, i.e. that there are new updated flows in there. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
