xapi_xenops: Avoid a race during suspend

[last-genius]

As described in #6451, a xapi event could prevent update_vm from pulling the latest Xenopsd metadata, overwriting it with stale information. In case of suspend, this would make the snapshot unresumable, raising an assert in xenopsd due to incongruities in memory values.

Instead pull the xenopsd metadata right before updating DB.power_state inXapi_vm_lifecycle.force_state_reset_keep_current_operations, eliminating the window for the race.

Closes #6451

[last-genius]

I've manually tested this fix and could not reproduce the issue with the xenopsd assert making a snapshot unresumable anymore.

[lindig]

I this correct even if the lock was aquired not in this function but higher up?

[last-genius]

Not strictly - you're right. But there's no other users of the function and in this case we no longer need to hold the lock once pull is done, so it's alright

[lindig]

You might want to leave a comment that release of the lock is ok. Altogether it would be better if we don't have to check dynamically if the lock is held already

[last-genius]

Added a comment

[Vincent-lau]

On line 2094 there is a with_lock call which will do an unlock at the end of that call, and here if the lock is already held higher up, and a Mutex.unlock is called, metadata_m will be unlocked, and later on when it reaches the with_lock on line 2094, it will be unlocked again. Is this true? If so, it is not safe to unlock a mutex twice in ocaml, see https://ocaml.org/manual/5.1/api/Mutex.html

[last-genius]

I thought so too - but didn't see any exceptions in testing. (the exception will anyhow be caught and logged by finally)

[robhoes]

So is the problem that this code in Xapi_xenops.create_metadata

  let domains =
    (* For suspended VMs, the last_booted_record contains the "live" xenopsd state. *)
    if vm.API.vM_power_state = `Suspended then
      Some vm.API.vM_last_booted_record
    else
      None

grabs a stale VM.last_booted_record during the window where – in another thread that updates the DB based on an event from xenopsd – the power_state was already set to Suspended but the "real" live xenopsd state was not yet written to VM.last_booted_record?

[last-genius]

So is the problem that this code in Xapi_xenops.create_metadata

  let domains =
    (* For suspended VMs, the last_booted_record contains the "live" xenopsd state. *)
    if vm.API.vM_power_state = `Suspended then
      Some vm.API.vM_last_booted_record
    else
      None

grabs a stale VM.last_booted_record during the window where – in another thread that updates the DB based on an event from xenopsd – the power_state was already set to Suspended but the "real" live xenopsd state was not yet written to VM.last_booted_record?

Yes! Exactly.

Perhaps I should describe it better in the commit message/code comment?

[Vincent-lau]

Is it better to use a reentrant lock here?
I think Colin has done something before to make the db lock reentrant (fae9b55), could we borrow that or use a library?

[minglumlu]

If it has a single user always, how about making it as pull_no_lock so that it could solely rely on its caller to acquire the lock?

[minglumlu]

Ah, I see. The outer lock will be acquired only when "power_state = `Suspended".

[last-genius]

It won't be called unless power_state = Suspended, so I could just make it pull_no_lock indeed and leave the reentrant lock refactoring for the future if someone else starts using this function.

[minglumlu]

Yeah, locking/unlocking twice seems a bit strange to me.

[minglumlu]

So is the problem that this code in Xapi_xenops.create_metadata

let domains =
(* For suspended VMs, the last_booted_record contains the "live" xenopsd state. *)
if vm.API.vM_power_state = `Suspended then
Some vm.API.vM_last_booted_record
else
None
grabs a stale VM.last_booted_record during the window where – in another thread that updates the DB based on an event from xenopsd – the power_state was already set to Suspended but the "real" live xenopsd state was not yet written to VM.last_booted_record?

So the xapi event handler pushed the wrong metadata to the xenopsd, and then the update_vm pulled the wrong metadata from the xenopsd?
If so, should the xenopsd reject the pushing since the VM was in suspended state already?

[last-genius]

So is the problem that this code in Xapi_xenops.create_metadata
let domains =
(* For suspended VMs, the last_booted_record contains the "live" xenopsd state. *)
if vm.API.vM_power_state = `Suspended then
Some vm.API.vM_last_booted_record
else
None
grabs a stale VM.last_booted_record during the window where – in another thread that updates the DB based on an event from xenopsd – the power_state was already set to Suspended but the "real" live xenopsd state was not yet written to VM.last_booted_record?

So the xapi event handler pushed the wrong metadata to the xenopsd, and then the update_vm pulled the wrong metadata from the xenopsd? If so, should the xenopsd reject the pushing since the VM was in suspended state already?

This could be another way of doing this - though I am not 100% sure we wouldn't prevent some desired behaviour this way.

[minglumlu]

This could be another way of doing this - though I am not 100% sure we wouldn't prevent some desired behaviour this way.

I just thought the locking scope is bigger and the lock is shared by all operations. But it may be fine as the bigger locking scope only covers the suspended case.

[robhoes]

Sorry, pressed the Approve button by mistake 😅

[robhoes]

Instead pull the xenopsd metadata as soon as the power_state is updated,
shrinking the window for the race.

It's not just shrinking the window, but eliminating it: if the VM.power_state field in the DB is not Suspended, then the xenopsd metadata for the VM will not be overwritten at all. And Xapi_vm_lifecycle.force_state_reset_keep_current_operations is what sets VM.power_state in the DB.