On Mon, Sep 9, 2024 at 4:01 AM Jonathan Davies <[email protected]> wrote: > > The OVS DB has a bond_active_slave field. This gets read by > port_configure_bond() into struct bond_settings' active_member_mac > field. See commit 3e5aeeb5 ("bridge: Keep bond active slave selection > across OVS restart") for the original rationale for preserving the > active bond member. > > But since commit 30353934 ("ofproto/bond: Validate active-slave mac.") > the bond_settings' active_member_mac field is ignored by bond_create(), > which set bond->active_member_mac to eth_addr_zero. > > Instead, set it to the value of the bond_settings' active_member_mac so > that the selection is preserved across OVS restarts. > > Also add a test that checks this behaviour. > > Fixes: 303539348848 ("ofproto/bond: Validate active-slave mac.") > Signed-off-by: Jonathan Davies <[email protected]>
For posterity: I investigated what happens when the underlying mac address changes between restarts. And in that case a new active member is selected. It's unclear to me why eth_addr_zero was set in the first place. Acked-by: Mike Pattrick <[email protected]> > --- > ofproto/bond.c | 2 +- > tests/ofproto-dpif.at | 67 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 68 insertions(+), 1 deletion(-) > > diff --git a/ofproto/bond.c b/ofproto/bond.c > index c31869a4c..0858de374 100644 > --- a/ofproto/bond.c > +++ b/ofproto/bond.c > @@ -246,7 +246,7 @@ bond_create(const struct bond_settings *s, struct > ofproto_dpif *ofproto) > ovs_refcount_init(&bond->ref_cnt); > hmap_init(&bond->pr_rule_ops); > > - bond->active_member_mac = eth_addr_zero; > + bond->active_member_mac = s->active_member_mac; > bond->active_member_changed = false; > bond->primary = NULL; > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 42fb66de6..3323b0afa 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -757,6 +757,73 @@ Datapath actions: drop > OVS_VSWITCHD_STOP() > AT_CLEANUP > > +AT_SETUP([ofproto-dpif - active bond member survives restart]) > +dnl Create bond0 with members p1, p2 and p3. Initially, set p2 as active. > +dnl Restart ovs-vswitchd. Check that p2 is still active. > +OVS_VSWITCHD_START( > + [add-bond br0 bond0 p1 p2 p3 bond_mode=active-backup -- \ > + set interface p1 type=dummy ofport_request=1 -- \ > + set interface p2 type=dummy ofport_request=2 -- \ > + set interface p3 type=dummy ofport_request=3 --]) > +AT_CHECK([ovs-appctl bond/set-active-member bond0 p2], [0], [ignore]) > +OVS_WAIT_UNTIL_EQUAL([ovs-appctl bond/show | STRIP_RECIRC_ID | > STRIP_ACTIVE_MEMBER_MAC], [dnl > +---- bond0 ---- > +bond_mode: active-backup > +bond may use recirculation: no, <del> > +bond-hash-basis: 0 > +lb_output action: disabled, bond-id: -1 > +updelay: 0 ms > +downdelay: 0 ms > +lacp_status: off > +lacp_fallback_ab: false > +active-backup primary: <none> > +<active member mac del> > + > +member p1: enabled > + may_enable: true > + > +member p2: enabled > + active member > + may_enable: true > + > +member p3: enabled > + may_enable: true > +]) > + > +dnl Restart ovs-vswitchd with an empty ovs-vswitchd log file. > +OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) > +mv ovs-vswitchd.log ovs-vswitchd_1.log > +AT_CHECK([ovs-vswitchd --enable-dummy --disable-system > --disable-system-route --detach \ > + --no-chdir --pidfile --log-file -vfile:rconn:dbg -vvconn > -vofproto_dpif -vunixctl], > + [0], [], [stderr]) > + > +OVS_WAIT_UNTIL_EQUAL([ovs-appctl bond/show | STRIP_RECIRC_ID | > STRIP_ACTIVE_MEMBER_MAC], [dnl > +---- bond0 ---- > +bond_mode: active-backup > +bond may use recirculation: no, <del> > +bond-hash-basis: 0 > +lb_output action: disabled, bond-id: -1 > +updelay: 0 ms > +downdelay: 0 ms > +lacp_status: off > +lacp_fallback_ab: false > +active-backup primary: <none> > +<active member mac del> > + > +member p1: enabled > + may_enable: true > + > +member p2: enabled > + active member > + may_enable: true > + > +member p3: enabled > + may_enable: true > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([ofproto-dpif - bond - allow duplicated frames]) > dnl Receiving of duplicated multicast frames should be allowed with > 'all_members_active'. > OVS_VSWITCHD_START([dnl > -- > 2.43.0 > > _______________________________________________ > 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
