On 4/12/22 13:49, Christophe Fontaine wrote:
> This config param allows the delivery of broadcast and multicast packets
> to the secondary interface of non-lacp bonds, equivalent to the option
> "all_slaves_active" for kernel bonds.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1720935
> Signed-off-by: Christophe Fontaine <[email protected]>
Hi, Christophe. Thanks for the patch and sorry for the late reply.
See some comments inline.
Best regards, Ilya Maximets.
> ---
> NEWS | 2 +
> ofproto/bond.c | 5 +-
> ofproto/bond.h | 2 +
> tests/automake.mk | 3 +-
> tests/bond.at | 125 +++++++++++++++++++++++++++++++++++++++++++
> tests/testsuite.at | 1 +
> vswitchd/bridge.c | 3 ++
> vswitchd/vswitch.xml | 20 +++++++
> 8 files changed, 159 insertions(+), 2 deletions(-)
> create mode 100644 tests/bond.at
>
> diff --git a/NEWS b/NEWS
> index 5074b97aa..38777eea8 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,8 @@ Post-v2.17.0
> - OVSDB:
> * 'relay' service model now supports transaction history, i.e. honors
> the
> 'last-txn-id' field in 'monitor_cond_since' requests from clients.
> + - Userspace datapath:
> + * New configuration knob 'all_members_active' for non lacp bonds
As Adrian already pointed out that should not be in the
'Userspace datapath' section. It may have no section.
Please, specify that it is 'other_config:all_members_active'.
And it's better to have a period at the end of a sentence.
>
>
> v2.17.0 - 17 Feb 2022
> diff --git a/ofproto/bond.c b/ofproto/bond.c
> index 845f69e21..aee420890 100644
> --- a/ofproto/bond.c
> +++ b/ofproto/bond.c
> @@ -145,6 +145,7 @@ struct bond {
> struct eth_addr active_member_mac; /* MAC address of the active member.
> */
> /* Legacy compatibility. */
> bool lacp_fallback_ab; /* Fallback to active-backup on LACP failure. */
> + bool all_members_active;
This field should nt be placed in the 'Legacy compatibility'
section. It should be in 'Bonding info' instead. And it
needs some comment.
>
> struct ovs_refcount ref_cnt;
> };
> @@ -448,6 +449,7 @@ bond_reconfigure(struct bond *bond, const struct
> bond_settings *s)
>
> bond->updelay = s->up_delay;
> bond->downdelay = s->down_delay;
> + bond->all_members_active = s->all_members_active;
>
> if (bond->lacp_fallback_ab != s->lacp_fallback_ab_cfg) {
> bond->lacp_fallback_ab = s->lacp_fallback_ab_cfg;
> @@ -893,7 +895,8 @@ bond_check_admissibility(struct bond *bond, const void
> *member_,
>
> /* Drop all multicast packets on inactive members. */
> if (eth_addr_is_multicast(eth_dst)) {
> - if (bond->active_member != member) {
> + if (bond->active_member != member &&
> + bond->all_members_active == false) {
Please, don't compare with 'false', just use '!'.
> goto out;
> }
> }
> diff --git a/ofproto/bond.h b/ofproto/bond.h
> index 1683ec878..ce4a25e73 100644
> --- a/ofproto/bond.h
> +++ b/ofproto/bond.h
> @@ -62,6 +62,8 @@ struct bond_settings {
> ovs run. */
> bool use_lb_output_action; /* Use lb_output action. Only applicable for
> bond mode BALANCE TCP. */
> + bool all_members_active; /* For non LACP bond, also accept multicast
> + packets on secondary interface. */
Looking at bond_check_admissibility(), it looks like active-backup
bond will drop packets received on backup interface. So, the 'non LACP'
part doesn't seem to be correct. Do we want to receive packets on backup
interface? In this case bond_check_admissibility() needs a corection.
If not, we need to update comments and docs clarifying that it's only
for balanced bonding modes.
> };
>
> /* Program startup. */
> diff --git a/tests/automake.mk b/tests/automake.mk
> index 8a9151f81..5c6400c0d 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -109,7 +109,8 @@ TESTSUITE_AT = \
> tests/mcast-snooping.at \
> tests/packet-type-aware.at \
> tests/nsh.at \
> - tests/drop-stats.at
> + tests/drop-stats.at \
> + tests/bond.at
I'm not sure why we need a separate file for these tests.
We need to move all existing bonding tests there or just
keep adding tests to ofproto-dpif.at.
>
> EXTRA_DIST += $(FUZZ_REGRESSION_TESTS)
> FUZZ_REGRESSION_TESTS = \
> diff --git a/tests/bond.at b/tests/bond.at
> new file mode 100644
> index 000000000..be0be8433
> --- /dev/null
> +++ b/tests/bond.at
> @@ -0,0 +1,125 @@
> +AT_BANNER([bond])
> +
> +# Strips out Recirculation ID information since it may change over time.
> +m4_define([STRIP_RECIRC_ID], [[sed '
> + s/Recirc-ID.*$/<del>/
> +' ]])
> +
> +# Strips out rebalance time information since it changes over time.
> +m4_define([STRIP_REBALANCE_TIME], [[sed '
> + s/next rebalance.*$/<next rebalance del>/
> +' ]])
> +
> +# Strips out active member mac address since it may change over time.
> +m4_define([STRIP_ACTIVE_MEMBER_MAC], [[sed '
> + s/active member mac.*$/<active member mac del>/
> +' ]])
> +
> +AT_SETUP([bond - discard duplicated frames])
> +# With an active/active non-lacp bond, the default behaviour
> +# is to discard multicast frames on the secondary interface
Please, use 'dnl' for comments.
Period at the end of a sentence.
> +OVS_VSWITCHD_START([dnl
> + add-bond br0 bond0 p1 p2 --\
> + set Port bond0 bond-mode=balance-slb --\
> + set Interface p1 type=dummy ofport_request=1 --\
> + set Interface p2 type=dummy ofport_request=2 ])
I know that existing tests written this way, but please,
use 'dnl' to split lines instead of '\'.
> +
> +ovs-appctl bond/set-active-member bond0 p1
> +ovs-ofctl add-flow br0 actions=NORMAL
These should be under AT_CHECK.
> +
> +AT_CHECK([ovs-appctl bond/show bond0 | STRIP_REBALANCE_TIME |
> STRIP_ACTIVE_MEMBER_MAC], [0], [dnl
This should be OVS_WAIT_UNTIL_EQUAL.
> +---- bond0 ----
> +bond_mode: balance-slb
> +bond may use recirculation: no, Recirc-ID : -1
> +bond-hash-basis: 0
> +lb_output action: disabled, bond-id: -1
> +updelay: 0 ms
> +downdelay: 0 ms
> +<next rebalance del>
> +lacp_status: off
> +lacp_fallback_ab: false
> +active-backup primary: <none>
> +<active member mac del>
State of the 'all_members_active' config should be printed
in the above list.
> +
> +member p1: enabled
> + active member
> + may_enable: true
> +
> +member p2: enabled
> + may_enable: true
> +
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0 in_port=1,dl_dst=ff:ff:ff:ff:ff:ff],
> [0], [dnl
> +Flow:
> in_port=1,vlan_tci=0x0000,dl_src=00:00:00:00:00:00,dl_dst=ff:ff:ff:ff:ff:ff,dl_type=0x0000
> +
> +bridge("br0")
> +-------------
> + 0. priority 32768
> + NORMAL
> + -> no learned MAC for destination, flooding
> +
> +Final flow: unchanged
> +Megaflow:
> recirc_id=0,eth,in_port=1,dl_src=00:00:00:00:00:00,dl_dst=ff:ff:ff:ff:ff:ff,dl_type=0x0000
> +Datapath actions: 100
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0 in_port=2,dl_dst=ff:ff:ff:ff:ff:ff],
> [0], [stdout])
> +AT_CHECK([cat stdout], [0], [dnl
Looks inconsistent to have direct check in the first run
and > stdout and 'cat stdout' in the second one.
> +Flow:
> in_port=2,vlan_tci=0x0000,dl_src=00:00:00:00:00:00,dl_dst=ff:ff:ff:ff:ff:ff,dl_type=0x0000
> +
> +bridge("br0")
> +-------------
> + 0. priority 32768
> + NORMAL
> + -> bonding refused admissibility, dropping
> +
> +Final flow: unchanged
> +Megaflow:
> recirc_id=0,eth,in_port=2,dl_src=00:00:00:00:00:00,dl_dst=ff:ff:ff:ff:ff:ff,dl_type=0x0000
> +Datapath actions: drop
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +AT_SETUP([bond - allow duplicated frames])
We should, probably, use the 'all_members_active' in the name somehow.
> +# In some specific deployments, we may want to allow these multicast
> +# frames on the second interface
'dnl'
Also, we're not trying to justify the feature, we need to describe
what we're trying to test. e.g. "Receiving of duplicated multicast
frames should be allowed with 'all_members_active'."
> +OVS_VSWITCHD_START([dnl
> + add-bond br0 bond0 p1 p2 --\
> + set Port bond0 bond-mode=balance-slb \
> + other_config:all_members_active=true --\
> + set Interface p1 type=dummy ofport_request=1 --\
> + set Interface p2 type=dummy ofport_request=2 ])
dnl
> +
> +ovs-appctl bond/set-active-member bond0 p1
> +ovs-ofctl add-flow br0 actions=NORMAL
AT_CHECK
There should be a bond/show check here veifying that
'all_members_active' is correctly set.
> +AT_CHECK([ovs-appctl ofproto/trace br0 in_port=1,dl_dst=ff:ff:ff:ff:ff:ff],
> [0], [dnl
> +Flow:
> in_port=1,vlan_tci=0x0000,dl_src=00:00:00:00:00:00,dl_dst=ff:ff:ff:ff:ff:ff,dl_type=0x0000
> +
> +bridge("br0")
> +-------------
> + 0. priority 32768
> + NORMAL
> + -> no learned MAC for destination, flooding
> +
> +Final flow: unchanged
> +Megaflow:
> recirc_id=0,eth,in_port=1,dl_src=00:00:00:00:00:00,dl_dst=ff:ff:ff:ff:ff:ff,dl_type=0x0000
> +Datapath actions: 100
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0 in_port=2,dl_dst=ff:ff:ff:ff:ff:ff],
> [0], [dnl
> +Flow:
> in_port=2,vlan_tci=0x0000,dl_src=00:00:00:00:00:00,dl_dst=ff:ff:ff:ff:ff:ff,dl_type=0x0000
> +
> +bridge("br0")
> +-------------
> + 0. priority 32768
> + NORMAL
> + -> no learned MAC for destination, flooding
> +
> +Final flow: unchanged
> +Megaflow:
> recirc_id=0,eth,in_port=2,dl_src=00:00:00:00:00:00,dl_dst=ff:ff:ff:ff:ff:ff,dl_type=0x0000
> +Datapath actions: 100
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> diff --git a/tests/testsuite.at b/tests/testsuite.at
> index 58adfa09c..b91edc5f2 100644
> --- a/tests/testsuite.at
> +++ b/tests/testsuite.at
> @@ -78,3 +78,4 @@ m4_include([tests/mcast-snooping.at])
> m4_include([tests/packet-type-aware.at])
> m4_include([tests/nsh.at])
> m4_include([tests/drop-stats.at])
> +m4_include([tests/bond.at])
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index e328d8ead..6034bb032 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -4615,6 +4615,9 @@ port_configure_bond(struct port *port, struct
> bond_settings *s)
> s->use_lb_output_action = (s->balance == BM_TCP)
> && smap_get_bool(&port->cfg->other_config,
> "lb-output-action", false);
> + /* all_members_active is disabled by default */
Not sure if we need this comment. But this comment needs
a period at the end.
> + s->all_members_active = smap_get_bool(&port->cfg->other_config,
> + "all_members_active", false);
Please, align to the next character after '('.
> }
>
> /* Returns true if 'port' is synthetic, that is, if we constructed it locally
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 0c6632617..fe6d6f3b8 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -2083,6 +2083,26 @@
> <code>true</code>).
> </column>
>
> + <column name="other_config" key="all_members_active"
> + type='{"type": "boolean"}'>
> + <p>
> + Enable/Disable delivery of broadcast/multicast packets on secondary
> + interface of a bond. Relevant only when <ref column="lacp"/> is
> + <code>off</code>.
We need to mention that this is only for balance-tcp and balance-slb.
This option will not work with active-backup.
> + </p>
> +
> + <p>
> + This parameter is identical to <code>all_slaves_active</code> for
> + kernel bonds. This is useful when we need to share 2 physical
> + functions between an ovs bond and a linux bond, as 2 LACP sessions
> + can't be negotiated over the same physical link.
> + LACP session will be managed by the kernel bond, while an
> + active-active bond is used for ovs. In that particular
> configuration,
> + the physical switch will send a unique copy of broadcast packets
> over
> + the 2 physical links, eventually to the secondary link of the bond.
Please, use 2 spaces between sentenses. Sometimes some single-spaced text
slips in, but it should be 2 according to the coding style document.
> + </p>
> + </column>
> +
> <group title="Link Failure Detection">
> <p>
> An important part of link bonding is detecting that links are down
> so
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
