ldap connections should have a max lifetime configurable

VERIFIED FIXED

Status

Cloud Services
Server: Core
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: petef, Assigned: petef)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa+])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.

Updated

6 years ago
Blocks: 680379
Created attachment 558772 [details] [diff] [review]
Max lifetime

This patch will make the pool unbind and discard connectors that have reached a max lifetime.
Assignee: nobody → tarek
Status: NEW → ASSIGNED
Attachment #558772 - Flags: review?(ryan)
Attachment #558772 - Flags: feedback?(petef)
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__()

Updated

6 years ago
Attachment #558772 - Flags: review?(ryan) → review+
(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
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
Assignee: tarek → petef

Updated

6 years ago
Blocks: 692452
reapplied in 2.6.1 branch at http://hg.mozilla.org/services/server-core/rev/ef9480be050b
validated by pete and now on its own lib : http://pypi.python.org/pypi/ldappool/
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [qa+]
OK. Marking as Verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.