Re: [RFC PATCH 0/9] ipvs network name space (netns) aware

[Daniel Lezcano <daniel.lezcano@free.fr>]

On 10/18/2010 01:55 PM, Hans Schillstrom wrote:
> On Sunday 17 October 2010 08:47:31 Simon Horman wrote:
>    
>> On Fri, Oct 08, 2010 at 01:16:36PM +0200, Hans Schillstrom wrote:
>>      
>>> This patch series adds network name space (netns) support to the LVS.
>>>
>>> REVISION
>>>
>>> This is version 1
>>>
>>> OVERVIEW
>>>
>>> The patch doesn't remove or add any functionality except for netns.
>>> For users that don't use network name space (netns) this patch is
>>> completely transparent.
>>>
>>> No it's possible to run LVS in a Linux container (see lxc-tools)
>>> i.e.  a light weight virtualization. For example it's possible to run
>>> one or several lvs on a real server in their own network name spaces.
>>> > From the LVS point of view it looks like it runs on it's own machine.
>>>
>>> IMPLEMENTATION
>>> Basic requirements for netns awareness
>>>   - Global variables has to be moved to dyn. allocated memory.
>>>
>>> Most global variables now resides in a struct ipvs { } in netns/ip_vs.h.
>>> What is moved and what is not ?
>>>
>>> Some cache aligned locks are still in global, module init params and some debug_level.
>>>
>>> Algorithm files they are untouched.
>>>
>>> QUESTIONS
>>> Drop rate in ip_vs_ctl per netns or grand total ?
>>>        
> This is a tricky one (I think),
> if the interface is shared with root name-space and/or other name-spaces
>   - use grand total
> if it's an "own interface"
>   - drop rate can/should be in netns...
>
>    
>> My gut-feeling is that per netns makes more sense.
>>
>>      
>>> Should more lock variables be moved (or less) ?
>>>        
>> I'm unsure what you are asking here but I will make a general statement
>> about locking in IPVS: it needs work.
>>      
> Some locks still resides as global variables, and others in netns_ipvs struct.
> Since you have a lot of experience with IPVS locks,
> you might have ideas what to move and what to not move.
>
>    
>>      
>>> PATCH SET
>>> This patch set is based upon net-next-2.6 (2.6.36-rc3) from 4 oct 2010
>>> and [patch v4] ipvs: IPv6 tunnel mode
>>>
>>> Note: ip_vs_xmit.c will not work without "[patch v4] ipvs: IPv6 tunnel mode"
>>>        
>> Unfortunately the patches don't apply with the persistence engine
>> patches which were recently merged into nf-next-2.6 (although
>> "[patch v4.1 ]ipvs: IPv6 tunnel mode" is still unmerged).
>>
>>      
> I do have a patch based on the nf-next without the SIP/PE patch
>
>    
>> I'm happy to work with you to make the required changes there.
>>      
> I would appreciate that.
>
>    
>> (I realise those patches weren't merged when you made your post.
>>   But regardless, either your or me will need to update the patches).
>>
>> Another issue is that your patches seem to be split in a way
>> where the build breaks along the way. E.g. after applying
>> patch 1, the build breaks. Could you please split things up
>> in a manner such that this doesn't happen. The reason being
>> that it breaks bisection.
>>
>>      
> Hmm, Daniel also pointed at this,
> The Patch is quite large, and will become even larger with pe and sip.
> My Idea was to review the patch in pieces and put it together in one or two large patches when submitting it.
> I don't know that might be a stupid ?
> It's hard to break it up, making the code reentrant causes changes every where.
>
> Daniel L, had another approach break it into many many tiny patches.
>    

These patches can be folded if the result is an acceptable patch size. 
The more important is to not break the init_net. Introducing the 
namespace concept step by step, from the bottom-up in the call stack is 
good because we focus first on making the namespace available in the 
code while keeping the init_net functional. The small patches help to 
git bisect and spot the patch which lead to a regression. As the patches 
are small, it is easy to quickly find the culprit and fix it, especially 
for the people who are not author of these patches.

During this step of namespace introduction, we prevent  to unshare the 
network for ipvs, with a non-op when the namespace is not init_net.

When everything is ready and the init_net is running without problem, we 
remove the non-op and consolidate the namespace part of ipvs.

This is how I am used to do, but that does not imply it is the best way 
to do that :)

>> Lastly, could you provide a unique subject for each patch.
>> I know its a bit tedious, but it does make a difference when
>> browsing the changelog.
>>
>>      
> Yepp, no problem
>
>    
>> --
>> To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>      
>