Re: [PATCH qemu-server v3 3/3] fix #7119: qm cleanup: wait for process exiting for up to 30 seconds

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

Am 15.05.26 um 11:52 AM schrieb Dominik Csapak:
> On 5/13/26 5:14 PM, Fiona Ebner wrote:
>> Am 26.02.26 um 3:08 PM schrieb Dominik Csapak:
>>> When qmeventd detects a vm exiting, it starts 'qm cleanup'.
>>>
>>> Since the vm process exits is sometimes not instant, wait up to 30
>>> seconds here to start the cleanup process instead of immediately
>>> aborting if the pid still exits. This prevented executing the hookscript
>>> on the 'post-stop' phase when either
>>> * the cleanup mechanism is still the old one
>>> * the guest was powered down from inside, not via the API
>>>
>>> This can be reproduced by e.g. passing through a usb device, which
>>> delays the qemu process exit for a few seconds (for most devices).
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>> ---
>>>   src/PVE/CLI/qm.pm | 16 ++++++++++++++--
>>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/PVE/CLI/qm.pm b/src/PVE/CLI/qm.pm
>>> index 6aff5b7a..ee3ccedd 100755
>>> --- a/src/PVE/CLI/qm.pm
>>> +++ b/src/PVE/CLI/qm.pm
>>> @@ -1101,7 +1101,7 @@ __PACKAGE__->register_method({
>>>               60,
>>>               sub {
>>>                   my $conf = PVE::QemuConfig->load_config($vmid);
>>> -                my $pid = PVE::QemuServer::check_running($vmid);
>>> +                my $pid =
>>> PVE::QemuServer::Helpers::vm_running_locally($vmid);
>>>                     # With a stop mode backup, we might run here into
>>> a running vm with a backup
>>>                   # lock, but this already did the cleanup and is an
>>> expected state, so abort
>>> @@ -1109,7 +1109,19 @@ __PACKAGE__->register_method({
>>>                   die "skipping cleanup - 'backup' lock is present
>>> and vm is running again\n"
>>>                       if $pid && $clean && $conf->{lock} && $conf-
>>> >{lock} eq 'backup';
>>>   -                die "vm still running\n" if $pid;
>>> +                # wait for some time until the QEMU process exits
>>> after the QMP
>>> +                # 'SHUTDOWN' event, since this might not be instant
>>> +                my $timeout = 30;
>>> +                my $starttime = time();
>>> +                warn "QEMU process $pid for VM $vmid still running
>>> (or newly started)\n"
>>> +                    if $pid;
>>
>> Should we maybe warn once after 10? seconds rather than instantly? Not
>> sure if we should warn at all if it's expected to take a while in some
>> cases. If we time out, we still get the error below.
>>
> 
> ofc i can drop the warning, but IMO getting a warning that an operation
> might take longer is also helpful?

Not if you then get it every time in the cases where it is fully
expected. That's just noise and not helpful information, making actual
unusual situations less visible.

> maybe a different wording like:
> 
> 'QEMU process [..] (or newly started), waiting up to $timeout seconds\n'
> 
> ?
> 
> logging only after some feels wrong since the first 10 seconds i don't
> know what's going on...
Even without logging in the first bit of time, you do know that the
cleanup handling is happening and taking some time, why would you need
more? If terminating the QEMU instance takes longer than some time, that
is an unusual situation, we might want to inform people about.

I guess we should include "VM cleanup:" as a prefix to the message to
make it clear which operation it is for.