Re: Multiple VRRP instances with 1.2.17

Hi Michael,

Thanks for your time investigating. The reason why I reverted is due to the bunch of email I received off-line about regression. To be transparent, I didn’t investigated that much because I am just rushed on other stuff right now.

I need to push a new release to website in order to have a no regression release out. We can merge/work on your patch after. No reason to get your work out of mainstream, really not.

There is also another commit I will revert, since I really found this one bad (I dont remind who made it, and mainly why I just merged this). It is the patch adding support to accept-mode, forking an execv calling iptables is just horrible.

I will be back to on this topic for sure.

regs,
Alexandre


> On 22 Jun 2015, at 16:05, Michael Littlejohn <[email protected]> wrote:
> 
> Hi Alexandre, Ryan et al,
> 
> Further to my previous message, I have tested a few ways of solving the issue Ryan reported and have the following proposals (based on the now reverted code):
> 
>  1) Add an initialisation clause into vrrp_init_instances_sands(vrrp_t *)
>     ---------------------------------------------------------------------
> 
>     <diff>   
>     --- keepalived.orig/keepalived/vrrp/vrrp_sync.c
>     +++ keepalived/keepalived/vrrp/vrrp_sync.c
>     @@ -36,6 +36,17 @@ vrrp_init_instance_sands(vrrp_t * vrrp)
>      {
>             set_time_now();
>     
>     +       /*
>     +        * For first round initialisation, if the vrrp timer hasn't been set at
>     +        * all (only common during first pass) then set the timer to time now.
>     +        */
>     +       if (timer_isnull(vrrp->sands)) {
>     +               vrrp->sands = time_now;
>     +               return;
>     +       }
>     +
>             /*
>              * When in MASTER state the expiry time for the group controls when
>              * advertisements are transmitted.
>     </diff>
> 
> This clause should only really be tripped during initialisation (when being called via the vrrp_register_workers(list) call chain). I don't see any other calls to timer_reset(timeval_t) being made on VRRP sands timers. It'll set the sands timers so that the tie-breaker call to vrrp_timer_vrid_timeout(const int fd) from vrrp_dispatcher_read_to(int) will work as expected when the thread expires off of the READ queue.
> 
> I think this is probably the cleanest solution, and the one I'd prefer to see merged.
> 
> ---
> 
>  2) Catch uninitialised timers in the tiebreaker
>     --------------------------------------------
> 
>     <diff>
>     --- vyatta-keepalived.orig/keepalived/vrrp/vrrp_scheduler.c
>     +++ vyatta-keepalived/keepalived/vrrp/vrrp_scheduler.c
>     @@ -364,6 +364,10 @@ vrrp_timer_vrid_timeout(const int fd)
>             for (e = LIST_HEAD(l); e; ELEMENT_NEXT(e)) {
>                     vrrp = ELEMENT_DATA(e);
>     +               if (timer_isnull(vrrp->sands)) {
>     +                       vrid = vrrp->vrid;
>     +                       return vrid;
>     +               }
>                     if (timer_cmp(vrrp->sands, timer) < 0 ||
>                         timer_isnull(timer)) {
>                             timer = timer_dup(vrrp->sands);
>     </diff>
> 
> This is really doing the same as above, just later on in the execution. It's less neat, IMO, because it's hidden away in the depths. I'd rather it was better exposed and grouped with similar functionality (as with Option 1). Doing it this way also puts wider spacings between the VRRP group's sands times, which is probably undesirable given how the tie-breaker works.
> 
> ---
> 
> Option 1, initialisation time differences looks like this:
> 
> Initialised Time (ųs)    Diff (ųs)
> 230016    
> 230075                   59
> 230094                   19
> 230111                   17
> 230198                   87
> 230282                   84
> 230336                   54
> 230353                   17
> 230418                   65
> 230480                   62
> 230544                   64
>     
> Max:                     87
> Min:                     17
> Mean:                    52.8
> 
> 
> Option 2, initialisation time differences look like this:
> 
> Initialised Time (ųs)    Diff (ųs)
> 465202    
> 466514                   1312
> 468192                   1678
> 470026                   1834
> 471557                   1531
> 472738                   1181
> 474146                   1408
> 476640                   2494
> 478569                   1929
> 480553                   1984
> 482770                   2217
>     
> Max:                     2494
> Min:                     1181
> Mean:                    1756.8
> 
> ---
> 
> Alexandre, you alluded to other problems that my previous change could introduce to the state machine - could you give me an example of such a problem? From my point of view I need to better understand what other use-cases might exist before I can give any thought to re-working my fix for the previous preemption issue (since my change has now been reverted).
> 
> Regards,
> M
> 
> 
> 
> On 19/06/15 12:16, Michael Littlejohn wrote:
>> Hi Alexandre, Ryan, et al,
>> 
>> [Preface: Forgive me, I'm not familiar with SourceForge tooling or protocol, so if I make a mess of this or get into too much detail let me know.]
>> 
>> Regarding this regression, I have found the root-cause of the issue and, tentatively, I have a possible fix for it.
>> 
>> The regression _was_ caused by my earlier commit ( 76f9ce36ae389983b48a83a933182739786908c9) which inadvertently changed the behaviour of vrrp_init_sands_instance(vrrp_t *) for Ryan's use-case. We don't have a similar use-case so the regression went undetected.
>> 
>> In the previous version of the code the following would have happened to Ryan's use-case:
>>     1. vrrp_init_sands would have initialised the sands timer of each VRRP group (such that the sands were non-zero).
>>     2. The interface those groups were on would have been given to a thread and enqueued to the READ queue
>>     3. The above thread would have timed-out of the READ queue and been passed to vrrp_dispatcher_read_to()
>>     4. vrrp_dispatcher_read_to() would have used vrrp_timer_vrid_timeout() to determine which group on that interface was the "most expired"
>>     5. The "most expired" group would have been run through the FSM and would have had it's sands timer's updated.
>>     6. Generally return to step 2 and repeat.
>> 
>> In the current version of the code, Ryan's use-case goes through the following:
>>     1. vrrp_init_sands tries to initialise the groups but the change made to vrrp_init_sands_instance mean that the sands **are not** initialised (they are ZERO when vrrp_init_sands_instance returns).
>>     2. The interface those groups were on would have been given to a thread and enqueued to the READ queue
>>     3. The above thread would have timed-out of the READ queue and been passed to vrrp_dispatcher_read_to()
>>     4. vrrp_dispatcher_read_to() tries to use vrrp_timer_vrid_timeout() to determine which group on that interface is "most expired" however, it's logic is such that vrrp sands timers which have **not** been initialised are ignored. Consequently, it defaults to using the last group of the pair(^1).
>>     5. The last group is run through the FSM and has it's timers updated.
>>     6. Generally return to step 2 and repeat.
>> 
>> 
>> 1. Extrapolating this where n > 2, I believe the last group on the interface is always processed. Any other groups will be ignored.
>> 
>> So ends the facts.
>> 
>> ---
>> 
>> As for my tentative solution.
>> 
>> I have *not* reverted my previous changes to vrrp_init_sands_instance(vrrp_t *) on my local copy. This is because those changes prevent a different issue where in ~10% of cases, preemption behaviour becomes stuck and does not transition over to the correct MASTER router (this is due to a timing issue caused by the old logic of vrrp_init_sands_instance where a group would get stuck in GOTO_MASTER, but would have it's timers updated while still in that state because it was still receiving packets).
>> 
>> Instead, I have modified the tie-breaker mechanism vrrp_timer_vrid_timeout() so that if a group has uninitialised sands timers it will be returned by the tie-breaker immediately and processed by steps 6 and 6 (above). However, I have concerns about that approach in the cold light of day as I don't think it will perform well at scale. Testing required.
>> 
>> Another tentative solution would be to change how the timers were initialised at startup, so that they were set to time_now rather than being processed by the modified logic of vrrp_init_sands_instance(). This would overcome the issue with the logic in vrrp_timer_vrid_timeout() and should also scale nicely. I'll test this solution later today and report back.
>> 
>> ---
>> 
>> As for the changes as they stand in vrrp_init_sands_instance(), what other use-cases are broken by my change and are there ways of working around them that I could help out with?
>> 
>> Regards,
>> M
>> 
>> 
>> 
>> 
>> 
>> On 17/06/15 22:30, [email protected] <mailto:[email protected]> wrote:
>>> Send Keepalived-devel mailing list submissions to
>>> 	[email protected] <mailto:[email protected]>
>>> 
>>> To subscribe or unsubscribe via the World Wide Web, visit
>>> 	https://lists.sourceforge.net/lists/listinfo/keepalived-devel <https://lists.sourceforge.net/lists/listinfo/keepalived-devel>
>>> or, via email, send a message with subject or body 'help' to
>>> 	[email protected] <mailto:[email protected]>
>>> 
>>> You can reach the person managing the list at
>>> 	[email protected] <mailto:[email protected]>
>>> 
>>> When replying, please edit your Subject line so it is more specific
>>> than "Re: Contents of Keepalived-devel digest..."
>>> 
>>> 
>>> Today's Topics:
>>> 
>>>    1. Re: Keepalived-devel Digest, Vol 107, Issue 2 (Alexandre Cassen)
>>> 
>>> 
>>> ----------------------------------------------------------------------
>>> 
>>> Message: 1
>>> Date: Wed, 17 Jun 2015 23:24:00 +0200
>>> From: Alexandre Cassen <[email protected]> <mailto:[email protected]>
>>> Subject: Re: [Keepalived-devel] Keepalived-devel Digest, Vol 107,
>>> 	Issue 2
>>> To: David Stapleton <[email protected]> <mailto:[email protected]>
>>> Cc: [email protected] <mailto:[email protected]>
>>> Message-ID: <[email protected]> <mailto:[email protected]>
>>> Content-Type: text/plain; charset="utf-8"
>>> 
>>> please provide feedback here tomorrow, otherwise I will revert this commit? which anyway seems strange reading the code, since it breaks some use-cases? VRRP FSM is very sensitive stuff, most of the time trying to optimise stuff is ending on breaking stuff :/ 
>>> 
>>> regs,
>>> Alexandre
>>> 
>>> 
>> 
> 
> ------------------------------------------------------------------------------
> Monitor 25 network devices or servers for free with OpManager!
> OpManager is web-based network management software that monitors 
> network devices and physical & virtual servers, alerts via email & sms 
> for fault. Monitor 25 devices for free with no restriction. Download now
> http://ad.doubleclick.net/ddm/clk/292181274;119417398;o_______________________________________________
> Keepalived-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/keepalived-devel

------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors 
network devices and physical & virtual servers, alerts via email & sms 
for fault. Monitor 25 devices for free with no restriction. Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o

_______________________________________________
Keepalived-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/keepalived-devel