| Message ID | 20240822140938.241689-1-jonathan.davies@nutanix.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [ovs-dev,v4] ofproto/bond: Preserve active bond member over restarts. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Thu, Aug 22, 2024 at 02:09:38PM +0000, Jonathan Davies 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. > > Fixes: 303539348848 ("ofproto/bond: Validate active-slave mac.") > Signed-off-by: Jonathan Davies <jonathan.davies@nutanix.com> Thanks Jonathan, I am wondering if this also works in the when the bond is reconfigured. Say, if the bond is created without bond_active_slave set, and then reconfigured so that it is set. Perhaps it that can't happen, or is ok for some reason. But I am concerned about the call to bond_reconfigure() in bundle_set(). Also, I think it would be really nice to add a test for this. ...
On 30/08/2024 10:02, Simon Horman wrote: > On Thu, Aug 22, 2024 at 02:09:38PM +0000, Jonathan Davies 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. >> >> Fixes: 303539348848 ("ofproto/bond: Validate active-slave mac.") >> Signed-off-by: Jonathan Davies <jonathan.davies@nutanix.com> > > Thanks Jonathan, > > I am wondering if this also works in the when the bond is reconfigured. > Say, if the bond is created without bond_active_slave set, and then > reconfigured so that it is set. Perhaps it that can't happen, or is ok for > some reason. But I am concerned about the call to bond_reconfigure() in > bundle_set(). I don't think that does work, ever since commit 3035393488 removed setting the active_member_mac from bond_reconfigure. Currently, there's no code path in which active_member_mac is set from the value configured in the DB. That also feels like a bug, but orthogonal to the one my patch fixes. Both issues would be fixed by reverting commit 3035393488, but that commit seems to be addressing a legitimate issue, although I don't have a good grasp on the details to know how to accommodate fixes for all three issues... Any suggestions? > Also, I think it would be really nice to add a test for this. That sounds like a good idea. See patch v5, coming up. Jonathan
On Mon, Sep 09, 2024 at 08:59:21AM +0100, Jonathan Davies wrote: > On 30/08/2024 10:02, Simon Horman wrote: > > On Thu, Aug 22, 2024 at 02:09:38PM +0000, Jonathan Davies 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. > > > > > > Fixes: 303539348848 ("ofproto/bond: Validate active-slave mac.") > > > Signed-off-by: Jonathan Davies <jonathan.davies@nutanix.com> > > > > Thanks Jonathan, > > > > I am wondering if this also works in the when the bond is reconfigured. > > Say, if the bond is created without bond_active_slave set, and then > > reconfigured so that it is set. Perhaps it that can't happen, or is ok for > > some reason. But I am concerned about the call to bond_reconfigure() in > > bundle_set(). > > I don't think that does work, ever since commit 3035393488 removed setting > the active_member_mac from bond_reconfigure. Currently, there's no code path > in which active_member_mac is set from the value configured in the DB. That > also feels like a bug, but orthogonal to the one my patch fixes. > > Both issues would be fixed by reverting commit 3035393488, but that commit > seems to be addressing a legitimate issue, although I don't have a good > grasp on the details to know how to accommodate fixes for all three > issues... Any suggestions? Hi Jonathan, I would need to dig deeper in order to make any suggestions. But I agree with your analysis above: let's treat this as a separate issue. > > Also, I think it would be really nice to add a test for this. > > That sounds like a good idea. See patch v5, coming up. Thanks.
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;
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. Fixes: 303539348848 ("ofproto/bond: Validate active-slave mac.") Signed-off-by: Jonathan Davies <jonathan.davies@nutanix.com> --- ofproto/bond.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)