[ovs-dev] [PATCH repost] physical: Improve treatment of localnet non-VLAN logical ports.

[Russell Bryant]

On 12/22/2015 04:18 PM, Ben Pfaff wrote:
> On Mon, Nov 30, 2015 at 09:55:21PM -0500, Russell Bryant wrote:
>> On 11/29/2015 02:48 PM, Ben Pfaff wrote:
>>> Until now, the flow table treated localnet logical ports that have a VLAN
>>> quite differently from those that don't.  The ones without a VLAN were
>>> essentially trunk ports: any packets that came in, that weren't picked off
>>> by a localnet port with a VLAN, were passed to the ones without a VLAN.
>>> This wasn't the intended behavior.
>>>
>>> This commit changes behavior to the intended behavior.  Now, localnet ports
>>> without a specific VLAN only receive packets without a VLAN header or those
>>> with VLAN ID 0 (with that header stripped off).
>>>
>>> Found by inspection.
>>>
>>> Signed-off-by: Ben Pfaff <blp at ovn.org>
>>> ---
>>> Repost of a patch from Oct. 15:
>>> https://patchwork.ozlabs.org/patch/530988/
>>>
>>>  ovn/controller/physical.c | 30 ++++++++++++++++++++----------
>>>  1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> First of all, sorry for missing this patch earlier.
>>
>> I tried testing this patch and I'm having problems with it.  After I
>> apply this patch, if I exercise this code path using part of
>> tutorial/OVN-Tutorial.md, ovn-controller gets stuck and eats a CPU.  I
>> actually accidentally got two instances of ovn-controller running in
>> this state and almost melted my laptop.  :-)
>>
>> I'm not sure where the error is, but figured I'd at least report my test
>> result.
>>
>> To replicate, just start the sandbox and run env5's setup.
>>
>> $ make sandbox SANDBOXFLAGS="--ovn"
>> $ ovn/env5/setup.sh
>>
>> If I revert this patch, that env works fine again.
> 
> Thanks for reporting the problem.
> 
> The problem was a missing call to ofpact_pad() after putting the "strip
> vlan" here:
> 
>             ofpact_put_STRIP_VLAN(&ofpacts);
>             uint32_t ofpacts_orig_size = ofpacts.size;
> 
> However, the need for that call is utterly unintuitive.  Instead of
> adding it, I think it's better to get rid of the need for it.  So, I
> posted v2:
>         http://openvswitch.org/pipermail/dev/2015-December/063769.html
> 
>> Yes, I should turn this into a test case.  I'm happy to do that once we
>> sort this out.
> 
> We don't currently have any tests for localnet, so that would be greatly
> appreciated.
> 

I'm working on a test case now.  I'm hoping I can use it to help test
out your v2.

Thanks,

-- 
Russell Bryant