CP-48676: Reuse pool sessions on slave logins

[snwoods]

This PR allows pool sessions to be reused, rather than wasting time creating new ones each time to talk to the slaves.

The average time spent across 15 slave get_servertime xenapi calls on a virtual pool was 20.97ms with reuse-pool-sessions = false.

Compared to an average of 17.11ms with reuse-pool-sessions = true

In particular, the part of the request that has actually been changed, the login_no_password call has gone from about 2.41ms to 0.008ms, 300x faster than previously.

One part of why this speedup is possible is because we no longer have to do a pool.get_all for each session (to force the newly created session's time to update). If we were to validate the reusable pool session is valid before use, we would still need to do this pool.get_all. However, as we are now no longer destroying the db session after use, it should continue to be valid. As such, to achieve the best speed increase, this option of validating the reusable pool session (validate-reusable-pool-session in xapi.conf) is off by default.

[lindig]

The use of match seems questionable as no destruction of the tuple is used and we still have when and if.

[snwoods]

Good point thanks, I think originally match made sense but I refactored it to the point it no longer does.

[edwintorok]

If we use a lock for writing we also need to use a lock for reading, otherwise this is not different from just having the assignment directly (at least on OCaml 4), and there is a race where a session would be created and potentially leaked on a race.

Usually the lock needs to be placed around the whole read-modify-write cycle.

[snwoods]

Thanks! I do wonder if the performance will be worse though if the threads have to wait for each other to get a session?

[snwoods]

I presume we also need to protect this read with the lock @edwintorok ? Otherwise one thread could assign to reusable_pool_session just after this is read, and then the session would be destroyed?

[robhoes]

One part of why this speedup is possible is because we no longer have to do a pool.get_all for each session (to force the newly created session's time to update).

You are referring to the following code in login_no_password_common:

  (* At this point, the session is created, but with an incorrect time *)
  (* Force the time to be updated by calling an API function with this session *)
  let rpc = Helpers.make_rpc ~__context in
  ignore (Client.Pool.get_all ~rpc ~session_id) ;

But does anyone understand why this is needed at all? That is, in what way in the time "incorrect" and how does the API call make it correct? The session is created in the DB in this function with last_active equal to Date.now (). The API call will trigger Session_check.check on the coordinator, which validates the session and may indeed update last_active, but it only does that if the current value if the DB field is more than 10 min in the past, which I hope won't ever be the case in this scenario. What am I missing?

I guess issues may occur if the pool member doing the login and the coordinator do not have the same time on their clocks. But if there is the case I bet lots of stuff may go wrong...

I found the original ticket now that caused this code to be added. It was due to a "clock skew" of 9 (!) days between the two hosts, which meant that the session got deleted by the GC as it was "not used" for 24 hours :)

[snwoods]

One part of why this speedup is possible is because we no longer have to do a pool.get_all for each session (to force the newly created session's time to update).

You are referring to the following code in login_no_password_common:

  (* At this point, the session is created, but with an incorrect time *)
  (* Force the time to be updated by calling an API function with this session *)
  let rpc = Helpers.make_rpc ~__context in
  ignore (Client.Pool.get_all ~rpc ~session_id) ;

But does anyone understand why this is needed at all? That is, in what way in the time "incorrect" and how does the API call make it correct? The session is created in the DB in this function with last_active equal to Date.now (). The API call will trigger Session_check.check on the coordinator, which validates the session and may indeed update last_active, but it only does that if the current value if the DB field is more than 10 min in the past, which I hope won't ever be the case in this scenario. What am I missing?

I guess issues may occur if the pool member doing the login and the coordinator do not have the same time on their clocks. But if there is the case I bet lots of stuff may go wrong...

I found the original ticket now that caused this code to be added. It was due to a "clock skew" of 9 (!) days between the two hosts, which meant that the session got deleted by the GC as it was "not used" for 24 hours :)

@robhoes I did some testing with removing the pool.get_all when creating a session and it was (obviously) faster than master (by about 3 seconds) but using reusable session was still 1s faster than even that. So it depends if the 9 day clock skew is likely to reappear or not, we could remove the pool.get_all? But I think reusable sessions is still worth it in either case. Removing pool.get_all from session creation would still be worth doing if possible, as it would work in all the occasions where we can't reuse the session i.e. when it's not an internal_xapi_master_to_xapi_slave_login

[snwoods]

Ring3: BVT + BST suiterun 205345 was all green with the latest changes (it looks like moving the lock to the appropriate places fixed the last few errors)

[snwoods]

I've run two pool bootstorm jobs, one for toolstack-next and one for my build tag with these changes, job ids 4116362 and 4116900 respectively. If you search the jaeger kfd endpoint for these job ids, you can see the speedups provided e.g. Xapi_session.login_no_password_common taking 2.79ms on toolstack-next and 0.14ms on my build tag.

[snwoods]

Rebased on master to fix merge conflict

[robhoes]

I'd prefer to just put this tuple into the if-statement below, as on it's own these values are meaningless (what Booleans are these etc.).

if (originator, pool, is_local_superuser, uname) =
   (xapi_internal_originator, true, true, None)

makes this clearer.

[robhoes]

Let's log the else-case here, so that it is clear why there is no Session.destroy in the logs as part of a session.logout call.