Message-ID: <ab2443f2-11e0-4773-319a-27004520d178@oracle.com>
Date:   Sat, 6 Apr 2019 00:21:09 -0700
From:   si-wei liu <si-wei.liu@...cle.com>
To:     Stephen Hemminger <stephen@...workplumber.org>,
        "Michael S. Tsirkin" <mst@...hat.com>
Cc:     sridhar.samudrala@...el.com, davem@...emloft.net, kubakici@...pl,
        alexander.duyck@...il.com, jiri@...nulli.us,
        netdev@...r.kernel.org, virtualization@...ts.linux-foundation.org,
        liran.alon@...cle.com, boris.ostrovsky@...cle.com,
        vijay.balakrishna@...cle.com
Subject: Re: [PATCH net v6] failover: allow name change on IFF_UP slave
 interfaces



On 4/5/2019 2:47 PM, Stephen Hemminger wrote:
> On Fri, 5 Apr 2019 17:28:55 -0400
> "Michael S. Tsirkin" <mst@...hat.com> wrote:
>
>> On Wed, Apr 03, 2019 at 12:52:47AM -0400, Si-Wei Liu wrote:
>>> When a netdev appears through hot plug then gets enslaved by a failover
>>> master that is already up and running, the slave will be opened
>>> right away after getting enslaved. Today there's a race that userspace
>>> (udev) may fail to rename the slave if the kernel (net_failover)
>>> opens the slave earlier than when the userspace rename happens.
>>> Unlike bond or team, the primary slave of failover can't be renamed by
>>> userspace ahead of time, since the kernel initiated auto-enslavement is
>>> unable to, or rather, is never meant to be synchronized with the rename
>>> request from userspace.
>>>
>>> As the failover slave interfaces are not designed to be operated
>>> directly by userspace apps: IP configuration, filter rules with
>>> regard to network traffic passing and etc., should all be done on master
>>> interface. In general, userspace apps only care about the
>>> name of master interface, while slave names are less important as long
>>> as admin users can see reliable names that may carry
>>> other information describing the netdev. For e.g., they can infer that
>>> "ens3nsby" is a standby slave of "ens3", while for a
>>> name like "eth0" they can't tell which master it belongs to.
>>>
>>> Historically the name of IFF_UP interface can't be changed because
>>> there might be admin script or management software that is already
>>> relying on such behavior and assumes that the slave name can't be
>>> changed once UP. But failover is special: with the in-kernel
>>> auto-enslavement mechanism, the userspace expectation for device
>>> enumeration and bring-up order is already broken. Previously initramfs
>>> and various userspace config tools were modified to bypass failover
>>> slaves because of auto-enslavement and duplicate MAC address. Similarly,
>>> in case that users care about seeing reliable slave name, the new type
>>> of failover slaves needs to be taken care of specifically in userspace
>>> anyway.
>>>
>>> It's less risky to lift up the rename restriction on failover slave
>>> which is already UP. Although it's possible this change may potentially
>>> break userspace component (most likely configuration scripts or
>>> management software) that assumes slave name can't be changed while
>>> UP, it's relatively a limited and controllable set among all userspace
>>> components, which can be fixed specifically to listen for the rename
>>> and/or link down/up events on failover slaves. Userspace component
>>> interacting with slaves is expected to be changed to operate on failover
>>> master interface instead, as the failover slave is dynamic in nature
>>> which may come and go at any point.  The goal is to make the role of
>>> failover slaves less relevant, and userspace components should only
>>> deal with failover master in the long run.
>>>
>>> Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module")
>>> Signed-off-by: Si-Wei Liu <si-wei.liu@...cle.com>
>>> Reviewed-by: Liran Alon <liran.alon@...cle.com>
>> Acked-by: Michael S. Tsirkin <mst@...hat.com>
>>
>> Stephen are you happy with this approach?
> I think it is the best solution for what you want to do.

Since you're asking specifically, I tried what you suggested below.

> Did you test with some things like Free Range Routing,

Although there might be spurious warning (which is a check for sanity 
more than an error) while slave interface is up, slave rename had been 
handled quite well there, no matter which state slave is at.

https://github.com/FRRouting/frr/blob/master/zebra/if_netlink.c#L97

The FRR users are supposed to operate on failover master interface 
anyway. No one is expected to configure those passive interfaces for 
routing.

> VPP
Nothing particular was seen for this one. The netlink usage there 
doesn't seem related to my change:
https://github.com/FDio/vpp/blob/master/src/vnet/devices/netlink.c

> or other userspace
> control planes that consume netlink?
dhcpcd (https://github.com/kobolabs/dhcpcd/blob/kobo/if-linux.c#L761) 
was tested OK.

In addition, the patch seems to play quite well with systemd-udev and 
dracut/initramfs-tools. No breakage, no weird error message was seen.

What else do you suggest we should try/test with?

Thanks,
-Siwei

Powered by blists - more mailing lists