[BUG?]fail_timeout/max_fails: code doesn't do what doc says

[Maxim Dounin]

Hello!

On Tue, May 21, 2013 at 04:16:05PM +0400, Dmitry Popov wrote:

> On Mon, 20 May 2013 21:11:03 +0400
> Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> > Patch:
> > 
> > diff --git a/src/http/modules/ngx_http_upstream_least_conn_module.c b/src/http/modules/ngx_http_upstream_least_conn_module.c
> > --- a/src/http/modules/ngx_http_upstream_least_conn_module.c
> > +++ b/src/http/modules/ngx_http_upstream_least_conn_module.c
> > @@ -282,7 +282,10 @@ ngx_http_upstream_get_least_conn_peer(ng
> >      }
> >  
> >      best->current_weight -= total;
> > -    best->checked = now;
> > +
> > +    if (now - best->checked > best->fail_timeout) {
> > +        best->checked = now;
> > +    }
> >  
> >      pc->sockaddr = best->sockaddr;
> >      pc->socklen = best->socklen;
> > diff --git a/src/http/ngx_http_upstream_round_robin.c b/src/http/ngx_http_upstream_round_robin.c
> > --- a/src/http/ngx_http_upstream_round_robin.c
> > +++ b/src/http/ngx_http_upstream_round_robin.c
> > @@ -523,7 +523,10 @@ ngx_http_upstream_get_peer(ngx_http_upst
> >      rrp->tried[n] |= m;
> >  
> >      best->current_weight -= total;
> > -    best->checked = now;
> > +
> > +    if (now - best->checked > best->fail_timeout) {
> > +        best->checked = now;
> > +    }
> >  
> >      return best;
> >  }
> 
> Still a bug exists because of this code:
> 
>     if (state & NGX_PEER_FAILED) {
>         /* ... */
>         peer->fails++;
>         peer->accessed = now;
>         peer->checked = now;
>         /* .... */
>     } else {
>         if (peer->accessed < peer->checked) {
>             peer->fails = 0;
>         }
>     }
> 
> Consider upstream which fails at least once each second (actually it's enough
> to fail each fail_timeout seconds). This upstream will be disabled as soon as it
> will fail max_fails times no matter what fail_timeout is.

This is expected behaviour.  Documentation is a bit simplified 
here, and fail_timeout is used like session time limit - the 
peer->fails counter is reset once there are no failures within 
fail_timeout.

While this might be non-ideal for some use cases, it's certainly 
not a bug.

> I propose the following solution:
> let's have two bits per peer: 
> 1) avail - set if this peer is available for usage (fails < max_fails)
> 2) avail_check - set if this peer was disabled, but its fail_timeout is expired and
> we sent one request to check if it's alive
> and two timestamps:
> 3) first_fail - we'll count number of fails only inside
> [first_fail, first_fail + fail_timeout) interval
> 4) unavail_from - peer is unavailable inside 
> [unavail_from, unavail_from + fail_timeout) interval
> 
> first_fail will only be used if avail == 1, unavail_from only if avail == 0,
> so we may use union here.
> 
> Simplified code:
> get:
>         if (!avail
>             && (now < unavail_from + fail_timeout || avail_check))
>         {
>             continue;
>         }
> 
>         if (!avail)
>             avail_check =1;
> free:
>         if (avail && now >= first_fail + fail_timeout)
>             fails = 0;
> 
>         if (request_failed) {
>             if (fails == 0)
>                 first_fail = now;
>             fails++;
>             if (fails >= max_fails) {
>                 avail = 0;
>                 unavail_from = now;
>             }
>             avail_check = 0;
>         } else {
>             if (avail_check) {
>                 fails = 0;
>                 avail = 1;
>                 avail_check = 0;
>             }
>         }

Such algorithm forget everything about previous failures once per 
fail_timeout, and won't detect bursts of failures split across 
two fail_timeout intervals.

Consider:

- max_fails = 3, fail_timeout = 10s
- failure at 0s
- failure at 9s
- at 10s peer->fails counter is reset
- failure at 11s
- failure at 12s

While 3 failures are only 3 seconds away from each other, this 
is not detected due to granularity introduced by the algorithm.

-- 
Maxim Dounin
http://nginx.org/en/donation.html