CP-48676: Reuse pool sessions on slave logins #6258
Conversation
snwoods
force-pushed
the
private/stevenwo/CA-400339
branch
from
81c5fd5 to
f17fb1b
Compare
ocaml/xapi/xapi_session.ml
Outdated
| if session <> Ref.null && is_valid_session session then ( | ||
| if | ||
| session <> Ref.null | ||
| && ((not !Xapi_globs.validate_reusable_pool_session) |
There was a problem hiding this comment.
Please add the flag check to is_valid as well
snwoods
force-pushed
the
private/stevenwo/CA-400339
branch
from
f17fb1b to
b94335a
Compare
|
The Ring3: BVT + BST suiteruns I have run were all green, both with (211574) and without (211571) additional debug logging. The debug suiterun shows that the pool sessions are being reused as expected. |
| try | ||
| (* Call an API function to check the session is still valid *) | ||
| let rpc = Helpers.make_rpc ~__context in | ||
| ignore (Client.Pool.get_all ~rpc ~session_id) ; |
There was a problem hiding this comment.
Is this call reasonably cheap (since the result is thrown away)?
There was a problem hiding this comment.
It's the same call that is used after creating a session to force the time to update (see line 666 above), so I assume it is the cheapest call we can do. It does take some time though, which is why I've disabled calling is_valid_session by default (!Xapi_globs.validate_reusable_pool_session defaults to false)
There was a problem hiding this comment.
During testing (BVT + BST etc.) the resuable_pool_session never became invalid because we prevent the resuable_pool_session from being destroyed in destroy_db_session. Something I'm unsure about though is, is there any possible situation where the session does become invalid (excluding a toolstack restart where a new session will be made anyway)?
If that were to happen and Xapi_globs.validate_reusable_pool_session = false, is_valid_session would still return true (as session != null) and whatever the session is used for would fail. I'm unsure if we would then get out of this state.
snwoods
force-pushed
the
private/stevenwo/CA-400339
branch
from
b94335a to
3efb3bb
Compare
snwoods
force-pushed
the
private/stevenwo/CA-400339
branch
from
3efb3bb to
151686b
Compare
I've marked all the comments as resolved where appropriate. This is still an outstanding question for me though. |
Prevent this reusable pool session from being destroyed so that it remains valid. This feature can be toggled with the flag reuse-pool-sessions. This commit has been reworked to use Atomic instead of a mutex to prevent a deadlock as shown in CA-400339. Signed-off-by: Steven Woods <[email protected]>
Add a new flag validate-reusable-pool-session to xapi globs which skips the reusable pool session validity check if it is false. This saves time as we are no longer calling pool.get_all for each session. Signed-off-by: Steven Woods <[email protected]>
snwoods
force-pushed
the
private/stevenwo/CA-400339
branch
from
151686b to
c6da552
Compare
This was first introduced in #5986 but was later reverted as the mutex it introduced caused a deadlock with the DB mutex. This PR has now been reworked to use an Atomic variable, removing the need for the mutex.
This PR allows pool sessions to be reused (if the flag is turned on, which it is by default). This greatly speeds up communications between hosts which is important for repetitive calls e.g. one customer was running get_servertime frequently on all hosts in a pool which was causing problems due to the length of each call.
When we get this reusable session, we can optionally validate the session before using it but this increases the duration of the call, so this is off by default. Excepting a toolstack restart, the reusable session should always be valid as it is excluded from deletion in destroy_db_session