On Thu, May 5, 2022 at 12:21 PM Adrian Moreno <[email protected]> wrote:
>
>
>
> 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]>
> > ---
> > 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
> >
>
> I have not reviewed this patch fully since I seen others already have. But
> this
> just caught my eyes: Is this userspace-datapath specific? I see all changes
> are
> done in the common bond code.
Hi Adrian,
This is indeed not userspace-datapath specific as this patch provides
the same tunable
as the kernel bond.
The unit test is using the kernel datapath, and my own tests were run
with the user-space datapath.
So indeed: s/Userspace datapath/bond/
Christophe
>
> >
> > 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;
> >
> > 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) {
> > 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. */
> > };
> >
> > /* 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
> >
> > 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
> > +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 ])
> > +
> > +ovs-appctl bond/set-active-member bond0 p1
> > +ovs-ofctl add-flow br0 actions=NORMAL
> > +
> > +AT_CHECK([ovs-appctl bond/show bond0 | STRIP_REBALANCE_TIME |
> > STRIP_ACTIVE_MEMBER_MAC], [0], [dnl
> > +---- 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>
> > +
> > +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
> > +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])
> > +# In some specific deployments, we may want to allow these multicast
> > +# frames on the second interface
> > +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 ])
> > +
> > +ovs-appctl bond/set-active-member bond0 p1
> > +ovs-ofctl add-flow br0 actions=NORMAL
> > +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 */
> > + s->all_members_active = smap_get_bool(&port->cfg->other_config,
> > + "all_members_active", false);
> > }
> >
> > /* 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>.
> > + </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.
> > + </p>
> > + </column>
> > +
> > <group title="Link Failure Detection">
> > <p>
> > An important part of link bonding is detecting that links are
> > down so
>
> --
> Adrián Moreno
>
> _______________________________________________
> 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
