Re: [RFC PATCH qemu-server] cleanup: refactor to make cleanup flow more consistent

[Fiona Ebner <f.ebner@proxmox.com>]

Am 23.02.26 um 4:50 PM schrieb Fiona Ebner:
> Am 23.02.26 um 11:56 AM schrieb Dominik Csapak:
>> There are two ways a cleanup can be triggered:
>>
>> * When a guest is stopped/shutdown via the API, 'vm_stop' calls 'vm_stop_cleanup'.
>> * When the guest process disconnects from qmeventd, 'qm cleanup' is
>>   called, which in turn also tries to call 'vm_stop_cleanup'.
>>
>> Both of these happen under a qemu config lock, so there is no direct
>> race condition that it will be called out of order, but it could happen
>> that the 'qm cleanup' call happened in addition so cleanup was called
>> twice. Which could be a problem when the shutdown was called with
>> 'keepActive' which 'qm cleanup' would simply know nothing of and ignore.
>>
>> Also the post-stop hook might not be triggered in case e.g. a stop-mode
>> backup was done, since that was only happening via qm cleanup and this
>> would detect the now again running guest and abort.
>>
>> To improve the situation we move the exec_hookscript call at the end
>> of vm_stop_cleanup. At this point we know the vm is stopped and we still
>> have the config lock.
>>
>> On _do_vm_stop (and in the one case for migration) a 'cleanup-flag' is
>> created that marks the vm is cleaned up by the api, so 'qm cleanup'
>> should not do it again.
>>
>> On vm start, this flag is cleared.

It feels untidy to have something left after cleaning up, even if it's
just the file indicating that cleanup was done. Maybe we can switch it
around, see below:

>>
>> There is still a tiny race possible:
>>
>> a guest is stopped from within (or crashes) and the vm is started again
>> via the api before 'qm cleanup' can run
>>
>> This should be a very rare case though, and all operation via the API
>> (reboot, shutdown+start, stop-mode backup, etc.) should work as intended.
> 
> How difficult is it to trigger the race with an HA-managed VM?
> 
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> I'm not sure how we could possibly eliminate the race i mentioned:
>> * we can't block on start because i don't think we can sensibly decide between:
>>   - vm was crashing/powered off
>>   - vm was never started
>>
>> We could maybe leave a 'started' flag somewhere too and clear the
>> cleanup flag also in 'qm cleanup', then we would start the vm
>> only when the cleanup flag is cleared
>> (or have the cleanup flags have multiple states, like 'started',
>> 'finished')

We already have something very similar, namely, the PID file. The issue
is that the PID file is removed automatically by QEMU upon clean
termination. For our use case we would need a second, more persistent
file. Then we could solve the issue of duplicate cleanup and the issue
of starting another instance before cleanup:

1. create a flag file at startup with an identifier for the
   QEMU instance, a second manual PID file?
2. at cleanup, check the file:
   a) if there is no such file, skip, somebody else already cleaned up
      NOTE: we need to ensure that pre-existing instances are still
      cleaned up. One possible way would be to create a flag file during
      host startup and only use the new behavior when that is present.
   b) if the file exists, check if the QEMU instance is still around. If
      it is, wait for the instance to be gone until hitting some
      timeout. Once it's gone, do cleanup.
3. make sure to run the post-stop hook whenever we remove the file
4. if the file still exists at startup, cleanup was not done yet, wait
   until some timeout and when hitting the timeout, either proceed with
   start anyway or suggest running cleanup manually. The latter would be
   safer, but also worse from an UX standpoint, since cleanup is
   root-only

What do you think?