Re: [summary] virtio network device failover writeup

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Mar 21, 2019 at 04:16:14PM +0200, Liran Alon wrote:
> 
> 
> > On 21 Mar 2019, at 15:51, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Thu, Mar 21, 2019 at 03:24:39PM +0200, Liran Alon wrote:
> >> 
> >> 
> >>> On 21 Mar 2019, at 15:12, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>> 
> >>> On Thu, Mar 21, 2019 at 03:04:37PM +0200, Liran Alon wrote:
> >>>> 
> >>>> 
> >>>>> On 21 Mar 2019, at 14:57, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>> 
> >>>>> On Thu, Mar 21, 2019 at 02:47:50PM +0200, Liran Alon wrote:
> >>>>>> 
> >>>>>> 
> >>>>>>> On 21 Mar 2019, at 14:37, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>>> 
> >>>>>>> On Thu, Mar 21, 2019 at 12:07:57PM +0200, Liran Alon wrote:
> >>>>>>>>>>>> 2) It brings non-intuitive customer experience. For example, a customer may attempt to analyse connectivity issue by checking the connectivity
> >>>>>>>>>>>> on a net-failover slave (e.g. the VF) but will see no connectivity when in-fact checking the connectivity on the net-failover master netdev shows correct connectivity.
> >>>>>>>>>>>> 
> >>>>>>>>>>>> The set of changes I vision to fix our issues are:
> >>>>>>>>>>>> 1) Hide net-failover slaves in a different netns created and managed by the kernel. But that user can enter to it and manage the netdevs there if wishes to do so explicitly.
> >>>>>>>>>>>> (E.g. Configure the net-failover VF slave in some special way).
> >>>>>>>>>>>> 2) Match the virtio-net and the VF based on a PV attribute instead of MAC. (Similar to as done in NetVSC). E.g. Provide a virtio-net interface to get PCI slot where the matching VF will be hot-plugged by hypervisor.
> >>>>>>>>>>>> 3) Have an explicit virtio-net control message to command hypervisor to switch data-path from virtio-net to VF and vice-versa. Instead of relying on intercepting the PCI master enable-bit
> >>>>>>>>>>>> as an indicator on when VF is about to be set up. (Similar to as done in NetVSC).
> >>>>>>>>>>>> 
> >>>>>>>>>>>> Is there any clear issue we see regarding the above suggestion?
> >>>>>>>>>>>> 
> >>>>>>>>>>>> -Liran
> >>>>>>>>>>> 
> >>>>>>>>>>> The issue would be this: how do we avoid conflicting with namespaces
> >>>>>>>>>>> created by users?
> >>>>>>>>>> 
> >>>>>>>>>> This is kinda controversial, but maybe separate netns names into 2 groups: hidden and normal.
> >>>>>>>>>> To reference a hidden netns, you need to do it explicitly. 
> >>>>>>>>>> Hidden and normal netns names can collide as they will be maintained in different namespaces (Yes I’m overloading the term namespace here…).
> >>>>>>>>> 
> >>>>>>>>> Maybe it's an unnamed namespace. Hidden until userspace gives it a name?
> >>>>>>>> 
> >>>>>>>> This is also a good idea that will solve the issue. Yes.
> >>>>>>>> 
> >>>>>>>>> 
> >>>>>>>>>> Does this seems reasonable?
> >>>>>>>>>> 
> >>>>>>>>>> -Liran
> >>>>>>>>> 
> >>>>>>>>> Reasonable I'd say yes, easy to implement probably no. But maybe I
> >>>>>>>>> missed a trick or two.
> >>>>>>>> 
> >>>>>>>> BTW, from a practical point of view, I think that even until we figure out a solution on how to implement this,
> >>>>>>>> it was better to create an kernel auto-generated name (e.g. “kernel_net_failover_slaves")
> >>>>>>>> that will break only userspace workloads that by a very rare-chance have a netns that collides with this then
> >>>>>>>> the breakage we have today for the various userspace components.
> >>>>>>>> 
> >>>>>>>> -Liran
> >>>>>>> 
> >>>>>>> It seems quite easy to supply that as a module parameter. Do we need two
> >>>>>>> namespaces though? Won't some userspace still be confused by the two
> >>>>>>> slaves sharing the MAC address?
> >>>>>> 
> >>>>>> That’s one reasonable option.
> >>>>>> Another one is that we will indeed change the mechanism by which we determine a VF should be bonded with a virtio-net device.
> >>>>>> i.e. Expose a new virtio-net property that specify the PCI slot of the VF to be bonded with.
> >>>>>> 
> >>>>>> The second seems cleaner but I don’t have a strong opinion on this. Both seem reasonable to me and your suggestion is faster to implement from current state of things.
> >>>>>> 
> >>>>>> -Liran
> >>>>> 
> >>>>> OK. Now what happens if master is moved to another namespace? Do we need
> >>>>> to move the slaves too?
> >>>> 
> >>>> No. Why would we move the slaves?
> >>> 
> >>> 
> >>> The reason we have 3 device model at all is so users can fine tune the
> >>> slaves.
> >> 
> >> I Agree.
> >> 
> >>> I don't see why this applies to the root namespace but not
> >>> a container. If it has access to failover it should have access
> >>> to slaves.
> >> 
> >> Oh now I see your point. I haven’t thought about the containers usage.
> >> My thinking was that customer can always just enter to the “hidden” netns and configure there whatever he wants.
> >> 
> >> Do you have a suggestion how to handle this?
> >> 
> >> One option can be that every "visible" netns on system will have a “hidden” unnamed netns where the net-failover slaves reside in.
> >> If customer wishes to be able to enter to that netns and manage the net-failover slaves explicitly, it will need to have an updated iproute2
> >> that knows how to enter to that hidden netns. For most customers, they won’t need to ever enter that netns and thus it is ok they don’t
> >> have this updated iproute2.
> > 
> > Right so slaves need to be moved whenever master is moved.
> > 
> > Given the amount of mess involved, should we just teach
> > userspace to create the hidden netns and move slaves there?
> 
> That’s a good question.
> 
> However, I believe that it is easier and more suitable to happen in kernel. This is because:
> 1) Implementation is generic across all various distros.
> 2) We seem to discover more and more issues with userspace as we keep testing this on various distros, configurations and workloads.
> 3) It seems weird that kernel does some things automagically and some things don’t. i.e. Kernel automatically binds the virtio-net and VF to net-failover master
> and automatically opens the net-failover slave when the net-failover master is opened, but it doesn’t care about the consequences these actions have on userspace.
> Therefore, I propose let’s go “all in”: Kernel should also be responsible for hiding it’s artefacts unless customer userspace explicitly wants to view and manipulate them.

Just a minor point: failover device is an artefact of kernel. Standy and
primary devices are created by the hypervisor.

> > 
> >>> 
> >>>> The whole point is to make most customer ignore the net-failover slaves and remain them “hidden” in their dedicated netns.
> >>> 
> >>> So that makes the common case easy. That is good. My worry is it might
> >>> make some uncommon cases impossible.
> >>> 
> >>>> We won’t prevent customer from explicitly moving the net-failover slaves out of this netns, but we will not move them out of there automatically.
> >>>> 
> >>>>> 
> >>>>> Also siwei's patch is then kind of extraneous right?
> >>>>> Attempts to rename a slave will now fail as it's in a namespace…
> >>>> 
> >>>> I’m not sure actually. Isn't udev/systemd netns-aware?
> >>>> I would expect it to be able to provide names also to netdevs in netns different than default netns.
> >>> 
> >>> I think most people move devices after they are renamed.
> >> 
> >> So?
> >> Si-Wei patch handles the issue that resolves from the fact the net-failover master will be opened before the rename on the net-failover slaves occur.
> >> This should happen (to my understanding) regardless of network namespaces.
> >> 
> >> -Liran
> > 
> > My point was that any tool that moves devices after they
> > are renamed will be broken by kernel automatically putting
> > them in a namespace.
> 
> I’m not sure I follow. How is this related to Si-Wei patch?
> Si-Wei patch (and the root-cause that leads to the issue it fixes) have nothing to do with network namespaces.
> 
> What do you mean tool that moves devices after they are renamed will be broken by kernel?
> Care to give an example to clarify?
> 
> -Liran

I'll have to get back to you next week when I'm less jetlaged and more
lucid.

> > 
> >>> 
> >>>> If that’s the case, Si-Wei patch to be able to rename a net-failover slave when it is already open is still required. As the race-condition still exists.
> >>>> 
> >>>> -Liran
> >>>> 
> >>>>> 
> >>>>>>> 
> >>>>>>> -- 
> >>>>>>> MST