Closed Bug 680324 Opened 10 years ago Closed 10 years ago

ldap connections should have a max lifetime configurable

Categories

(Cloud Services :: Server: Core, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: petef, Assigned: petef)

References

Details

(Whiteboard: [qa+])

Attachments

(1 file)

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.
Blocks: 680379
Attached patch Max lifetimeSplinter Review
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__()
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
Blocks: 692452
validated by pete and now on its own lib : http://pypi.python.org/pypi/ldappool/
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [qa+]
OK. Marking as Verified.
Status: RESOLVED → VERIFIED
Attachment #558772 - Flags: feedback?(petef)
You need to log in before you can comment on or make changes to this bug.