On Thu, Jun 23, 2022 at 10:22 PM Ilya Maximets <[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]>
>
> Hi, Christophe. Thanks for the patch and sorry for the late reply.
> See some comments inline.
Thanks for the complete review (specifically for the added test),
Ilya! I'll submit another patch (with other fixes for the doc part).
Christophe
>
> 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.
Ack. The packets being dropped on the backup interface is the intended
behavior, so I'll update the comment to clarify this.
>
> > };
> >
> > /* 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
