search

jpirko / libteam

team netdevice library
GNU Lesser General Public License v2.1
230 stars 57 forks source link

Adding a port dynamically (i.e. thru PortAdd DBus method) fails for activebackup runner #18

Closed wipawel closed 9 years ago

wipawel commented 9 years ago

When a team device instance is started with no ports the following PortAdd method call fails due to inability to change MAC addresses according to hwaddr_policy specific to activebackup runner.

The error here is a result of inability to change MAC address of a device whose link is already up.

That happens because of the way PortAdd is handled i.e. call to add a port causes to immediately enslave the port (link is set up on it), then following netlink events caused that way are triggering registered callbacks where change MAC address routine resides among them e.g. ab_hwaddr_policy_same_all_port_added(). Afterwards when teamd_set_hwaddr() fails with EBUSY (that error code is mapped from corresponding result received via rtnl) the whole PortAdd method fails and port_obj_create() cleans half-initialized port object.

The MAC address for given devices should be changed before enslavement/link up happens.

jpirko commented 9 years ago

Devices that are unable to change mac address while they are up are old/broken. This is unsupported.

mtomaschewski commented 9 years ago

You understood it wrong: the devices (drivers) used here support setting of the mac, but AFAWS, libteam does it in wrong order: it sets the link UP and is trying to set the MAC then -> kernel rejects it because the device is already UP.

mtomaschewski commented 9 years ago

I mean, this simply breaks kernels < 3.18 or something like this / drivers not supporting IFF_LIVE_ADDR_CHANGE.

jpirko commented 9 years ago

It's just a flag. mac changes while UP are supported forever...

Team module in kernel ups the device when it is enslaved. I think it is the correct way to go. If your device does not support mac changes while UP, you should use different "hwaddr_policy". Please see runner.hwaddr_policy in manpages of teamd.conf. Thanks.

wipawel commented 9 years ago

Would it makes sense for you to add yet another hwaddr_policy: none? It happens I have such patch ;-)

That policy would not assign any callback changing mac on any port device. So user could do it manually or by other means (e.g. LLADDR).

jpirko commented 9 years ago

Well, sure, everything is possible, but I'm missing the usecase there. I don't understand why "by_active" is not acceptable for you:

by_active — Team device adopts the hardware address of the currently active port. This is useful when the port device is not able to change its hardware address.
wipawel commented 9 years ago

OK, after checking code I understood "by_active" better. It's the only hwaddr_policy that adjust team's device MAC address to the MAC address of currently active port. Not the new port's MAC being added to the MAC address of currently active one. In other words a difference between "adopts" and "adapts" ;-).

Yes, that would work and there is indeed no need for any none policy. Thanks.

jpirko commented 9 years ago

Feel free to send a patch making the manpage clearer....