Re: [RFC v4 3/5] vxlan: add support for underlay in non-default VRF

Le 26 nov. 2018 à 19:26, Roopa Prabhu [off-list ref] a écrit :

On Mon, Nov 26, 2018 at 9:54 AM David Ahern [off-list ref] wrote:

quoted

On 11/26/18 9:32 AM, Alexis Bauvin wrote:

quoted

Thanks for the review. I’ll send a v5 if you have no other comment on
this version!

A few comments on the test script; see attached which has the changes.

Mainly the cleanup does not need to be called at the end since you setup
the exit trap. The cleanup calls ip to delete veth-hv-1 and veth-tap but
those are moved to other namespaces. 'ip netns exec NAME ip ...' is more
efficiently done as 'ip -netns NAME ...'. The test results should align
like this:

Checking HV connectivity                                          [ OK ]
Check VM connectivity through VXLAN (underlay in the default VRF) [ OK ]
Check VM connectivity through VXLAN (underlay in a VRF)           [ OK ]

So it is easy for users to see the PASS/FAIL.

It would be good to copy the topology ascii art into the test script as
well for future users.

Also, add the test as a separate patch at the end and include it in
tools/testing/selftests/net/Makefile

Finally, I think you should drop the RFC and send it as a 'ready for
inclusion'.

I cant seem to find patch 5 in my mail box... so commenting here
(Using reference to patch5 from here
https://marc.info/?l=linux-netdev&m=154284885815549&w=2)

Still not convinced that the auto reopen is justified here IMO because
it can be done from user-space and there are many cases where this is
already done from user-space. A few questions for alexis on that,

I do agree on this. The test shows that a simple down/up is enough, and
the patch was written as a mere convenience.

- What is the reason for handling NETDEV_CHANGE on the vxlan device
from the notifier handler. It can be really done in the changelink
handler, correct  ?

Looks correct to me. The reason is nothing more than me not thinking
about the netlink handlers.

- Also, IIUC, patch5 blindly re-opens the vxlan device without
considering if the admin had set it to down last (ie the last state on
it was vxlan_close). is that correct ?

It is correct. This is a big oversight from my side, that could have
led to crashes. Fortunately the underlying code will check if the
sockets are null (which they are if the interface is down) before
accessing them.

(Don't want to block the entire series for just patch5. Patch5 can be
done incrementally after we converge on it. The rest of the series
looks good as David has already reviewed.  And nice to see the test!).

Thanks!

Given the aforementioned oversight when handling a down interface, it is
best to wait for a better solution for this patch.
Moreover, the issue of mixing default and non-default vrf needs to be
addressed. For now it is stale, as I don’t see any solution (except for
rewriting the whole thing as you suggested before) to address the
"Address already in use" made by a socket of the default vrf owning the
port across all vrfs.
I tested both Vyatta’s changes and SO_REUSEPORT, and neither of them seem
to work for this case.