680324 - ldap connections should have a max lifetime configurable

Status: VERIFIED FIXED

(Cloud Services :: Server: Core, defect)

Description

[Pete Fritchman [:petef]]

The LDAP pooling we use needs to support a way to assign a maximum lifetime to a connection.  Pooling is great -- but when one of the LDAP slaves is out for maintenance, everyone reconnects to the VIP and gets routed to the in-service slaves. Then when we bring the out-of-service slave back to life, load isn't evenly spread out.

If we set max_lifetime to 10 minutes, we'll still get all of the connection pooling benefits plus better backend load balancing.

Comment 1

[Tarek Ziadé (:tarek)]

Attachment: Max lifetime

This patch will make the pool unbind and discard connectors that have reached a max lifetime.

Comment 2

[Ryan Kelly [:rfkelly]]

Comment on attachment 558772 [details] [diff] [review]
Max lifetime

Review of attachment 558772 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, modulo a docstring typo.

Seeing time.sleep() in tests always makes me nervous, but I guess it can't be avoided in this case (well, short of setting up infrastructure to mock out the time module of course)

::: services/exceptions.py
@@ +55,4 @@
>            returned. If set to 0, the header is explicitely skipped.
>          - backend: if provided, the backend from where originated the error.
>            it will be used to render more details, It can be any kind of object
> +          as __str__()will be called on it.

accidentally deleted a space here after __str__()

Comment 3

[Tarek Ziadé (:tarek)]

(In reply to Ryan Kelly [:rfkelly] from comment #2)
> Comment on attachment 558772 [details] [diff] [review]
> Max lifetime
> 
> Review of attachment 558772 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, modulo a docstring typo.
> 
> Seeing time.sleep() in tests always makes me nervous, but I guess it can't
> be avoided in this case (well, short of setting up infrastructure to mock
> out the time module of course)

Yes. In this particular case it can't be avoided and it's a legitimate use case:

A thread is started to do a particular job and the main thread look at the state of a flag right after.

If there's no sleep, the lookup in the main thread can win the race while the thread starts up, and the test can fail. The sleep give enough time for the thread to start its job.

This particular test was failing from time to time in fact and that fixes it. 


> ::: services/exceptions.py
> @@ +55,4 @@
> >            returned. If set to 0, the header is explicitely skipped.
> >          - backend: if provided, the backend from where originated the error.
> >            it will be used to render more details, It can be any kind of object
> > +          as __str__()will be called on it.
> 
> accidentally deleted a space here after __str__()

Good catch, thx

Comment 4

[Tarek Ziadé (:tarek)]

Pushed in https://hg.mozilla.org/services/server-core/rev/58360f363e8b

Pete, I will let you test this and close the bug once you're happy with the result

Comment 5

[Tarek Ziadé (:tarek)]

reapplied in 2.6.1 branch at http://hg.mozilla.org/services/server-core/rev/ef9480be050b

Comment 6

[Tarek Ziadé (:tarek)]

validated by pete and now on its own lib : http://pypi.python.org/pypi/ldappool/

Comment 7

[James Bonacci [:jbonacci]]

OK. Marking as Verified.