[ovs-dev] [PATCH v3 0/7] create tunnel devices using rtnetlink interface

[Eric Garver]

On Mon, May 01, 2017 at 01:55:14PM -0400, Eric Garver wrote:
> On Thu, Apr 27, 2017 at 02:36:18PM -0700, Joe Stringer wrote:
> > On 13 April 2017 at 13:47, Eric Garver <e at erig.me> wrote:
> > > This series adds support for the creation of tunnels using the rtnetlink
> > > interface. This will open the possibility for new features and flags on those
> > > vports without the need to change vport compatibility code.
> > >
> > > Support for STT and LISP have not been added because these are not upstream yet,
> > > so we don't know how the interface will be like upstream. And there are no
> > > features in the current drivers right now we could make use of.
> > >
> > > Note: This work originally started by Thadeu Lima de Souza Cascardo.
> > >
> > > Testing:
> > >   - kernel 4.9.22, in-tree datapath
> > >     - rtnetlink successfully creates devices
> > >   - kernel 4.2.8, in-tree datapath
> > >     - rtnetlink is tried, but fails due to no COLLECT_METADATA support
> > >     - genetlink successfully creates devices
> > >   - kernel 4.2.8, out-of-tree datapath
> > >     - rtnetlink is not tried
> > >     - genetlink successfully creates devices
> > >
> > > v3:
> > >   - commonzie code to get port data to verify port
> > >   - eliminate dpif_netlink_rtnl_vxlan_destroy() and alike
> > >   - minor changes for coding style guidelines
> > >   - add ACKs from previous reviews
> > 
> > Sorry about the delay. I almost pushed it the other day, but had a
> > concern around how this series handles shutdown/restart of OVS.
> > 
> > I have a couple of minor pieces of feedback on some patches, please
> > roll those in.
> > 
> > In my setup I removed the vport-* modules so there was no way to fall
> > back to compat code, then ran depmod to fix up the dependencies.
> > 
> > When I run the GRE tests, the first gre test succeeds then the second fails:
> > 
> > $ make check-kernel TESTSUITEFLAGS="-k gre"
> >  10: datapath - ping over gre tunnel                 ok
> > 14: datapath - truncate and output to gre tunnel    FAILED
> > (system-traffic.at:507)
> > 
> > $ ip link | grep gre_sys
> > 62: gre_sys at NONE: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1472 qdisc
> > pfifo_fast state UNKNOWN mode DEFAULT group default qlen 1000
> > 
> > $ sudo ip li del dev gre_sys
> > 
> > $ make check-kernel TESTSUITEFLAGS="14"
> >  14: datapath - truncate and output to gre tunnel    ok
> > 
> > $ make check-kernel TESTSUITEFLAGS="14"
> >   14: datapath - truncate and output to gre tunnel    FAILED
> > (system-traffic.at:507)
> > 
> > This isn't failing randomly; first time it works, but if the device
> > exists, it fails. This is similar to ovs restart or crash with
> > --monitor.
> > 
> > Looking at the logs:
> > > 2017-04-27T21:24:19.270Z|00041|dpif_netlink|INFO|Failed to create at_gre0 with rtnetlink: Invalid argument
> > > 2017-04-27T21:24:19.271Z|00042|dpif|WARN|system at ovs-system: failed to add at_gre0 as port: Address family not supported by protocol
> > > 2017-04-27T21:24:19.271Z|00043|bridge|WARN|could not add network device at_gre0 to ofproto (Address family not supported by protocol)
> > > 2017-04-27T21:24:19.272Z|00044|dpif_netlink|INFO|Failed to create at_gre0 with rtnetlink: Invalid argument
> > 
> > Seems like the kernel is incorrectly returing -EINVAL rather than
> > -EEXIST. So even if we were to make the probe/tunnel creation logic a
> > bit smarter to understand if the port already exists (eg, because OVS
> > was restarted), it won't handle correctly.
> > 
> > Could you look further into this? Ideally the kernel could tell us
> > when we're trying to create a device that already exists, and return
> > EEXIST.. then it would be up to our validation logic to ensure that
> > all the correct options are configured on that device. But maybe the
> > workaround for all existing kernels is to try to delete the tunnel
> > device the first time, then try to recreate it.
> > 
> > Thoughts?
> 
> Thanks for the feedback Joe. I'll investigate this and all your other
> feedback.

Thanks for catching this Joe!

We need to add the NLM_F_EXCL flag, otherwise the rtnetlink code in the
kernel will call changelink(). I looked back to kernel 3.10 - If the dev
already exists and NLM_F_EXCL is set then the rtnetlink common code will
return EEXIST.

I settled on the following:

error = create_interface()
if (error == EEXIST) {
    error = verify_interface()
    if (error) {
        error = delete_interface()
        if (error) {
            goto fail
        }
        error = create_interface()
    }
}
if (!error) {
    error = verify_interface()
}