On 9/16/24 16:41, Eelco Chaudron wrote:
>
>
> 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'm a little confused by this statement as you're not setting any interfaces
down in your system test... Or am I missing something?
The current system test seems straightforward to re-write as a unit test.
Though I didn't try, so maybe that is not the case. However the reason
provided above doesn't seem to make sense at the first glance.
Note: dummy ports can be paired with options:[p]stream the same way veths
are in the test.
Best regards, Ilya Maximets.
>> 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
