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

[Dominik Csapak <d.csapak@proxmox.com>]

On 2/24/26 10:49 AM, Fiona Ebner wrote:
> Am 24.02.26 um 10:37 AM schrieb Dominik Csapak:
>> On 2/24/26 10:30 AM, Fiona Ebner wrote:
>>> 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?
>>
>> yes, that seems good to me,  i'll play around with that and send a next
>> version
> 
> Ah, one more thing. With stop mode backup, we'd still run into the issue
> that the cleanup triggered by qmeventd might run into a newly started
> instance and then wait around for nothing. We already skip cleanup if we
> detect the 'rollback' lock since we know rollback does its own cleanup.
> I think we can do the same for the backup lock (if shutdown was clean),
> since we know stop mode backup does its own cleanup too. And it might be
> better to do warn+return instead of die, since the situation is not
> really unexpected (the one for rollback could be adapted too).


sure makes sense, but i'd split that in a separate patch