CP-51352: Compare before setting a new value for last_active
#5976
Conversation
There was a problem hiding this comment.
Makes sense to me but I wonder if we ought (or, can for that matter) switch out the usage of Unix.time internally with Mtime (as has been the trend in recent contributions).
I'm unsure what's viable, so I can only speculate (and it's a bit of a yak shave). Would need people more familiar with the datetime's format, usage of the last_active field outwith xapi itself, and the relevant libraries (Mtime) to comment.
ocaml/xapi/session_check.ml
Outdated
| let last_active = | ||
| Db_actions.DB_Action.Session.get_last_active ~__context | ||
| ~self:session_id | ||
| |> Xapi_stdext_date.Date.to_float |
There was a problem hiding this comment.
Can the conversion to float be avoided?
There was a problem hiding this comment.
Something like this 7a9935e?
GabrielBuica
force-pushed
the
private/dbuica/CP-50723-set-last-active
branch
from
041c0db to
ec5f252
Compare
GabrielBuica
changed the title
last_activelast_active
ocaml/xapi/session_check.ml
Outdated
| in | ||
| let threshold_time = Xapi_stdext_date.Date.diff n last_active in | ||
| if | ||
| Ptime.Span.compare threshold_time |
There was a problem hiding this comment.
I would prefer the is_earlier, is_later functions because they are more obvious to read.
There was a problem hiding this comment.
b95ddb7 I agree. I a looked over the implementation of diff and it does the same conversion. So, I did it explicitly and use is later. The error path should not be happening.
|
I will retest the minor changes, squash the fixups properly and then marked it as ready for review again. |
GabrielBuica
force-pushed
the
private/dbuica/CP-50723-set-last-active
branch
from
423c3bf to
c03e20d
Compare
Experimenting with xapi under different loads of `xe vm-list`s revealed that xapi often waits a considerable amount of time for the database lock. e.g. Attempting to start a VM under a constant load of 20 `xe vm-list`s can take up to 30s and the total amount of time that xapi requests would spend in `Db_actions.Session.set_last_active` would be close to 9s. It seems that each API call does a `Session_check.check` implicitly updating the last time the current session is been used. From what I gathered, this value is used for the garbage collection of session where the current default threshold value is 24 hours, `Xapi.globs.inactive_session_timeout`. To avoid holding the database lock to update the `last_active` field every time, this commit gets the current value of `last_active` and compares it with the current time. If the difference is greater than 10 minutes only then we set a new value in the field. Otherwise, we continue without writing in the database. This has the following improvement on the total time of `xe vm-start` under different loads: - 20 `xe vm-list`: - - Before: 30.670s | 30.086s | 30.958s | - - After: 28.185s | 28.383s | 28.909s | - 80 `xe vm-list`: - - Before: 2m38.425s | 2m43.539s | 2m39.045s | - - After: 1m46.266s | 1m49.327s | 1m36.825s | For smaller loads, the improvement is between 5-10%. Whereas for bigger loads, the improvement is much more visible. Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
Follow up to the previous commit, this makes the hardcoded threshold of 10 mins configurable from `xapi.conf`. This enables flexibilty in case different values are needed for updating the `last_active` field of a session more or less often. Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
GabrielBuica
force-pushed
the
private/dbuica/CP-50723-set-last-active
branch
from
c03e20d to
67df157
Compare
|
Latest BVT + BST 204612 |
5 participants
Experimenting with xapi under different loads of
xe vm-listsrevealed that xapi often waits a considerable amount of time for
the database lock.
e.g. Attempting to start a VM under a constant load of 20
xe vm-listscan take up to 30s and the total amount of time that xapi requests would
spend in
Db_actions.Session.set_last_activewould be close to 9s.It seems that each API call does a
Session_check.checkimplicitlyupdating the last time the current session is been used. From what I
gathered, this value is used for the garbage collection of session where
the current default threshold value is 24 hours,
Xapi.globs.inactive_session_timeout.To avoid holding the database lock to update the
last_activefieldevery time, this commit gets the current value of
last_activeandcompares it with the current time. If the difference is greater than 10
minutes only then we set a new value in the field. Otherwise, we
continue without writing in the database.
This has the following improvement on the total time of
xe vm-startunder different loads:
20
xe vm-list:80
xe vm-list:For smaller loads, the improvement is between 5-10%. Whereas for bigger
loads, the improvement is much more visible.
Passes: