From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id A89951FF0E5 for ; Wed, 29 Jul 2026 17:45:58 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 57CC120836; Wed, 29 Jul 2026 17:45:57 +0200 (CEST) Message-ID: <5df38186-136a-4478-b49d-ecb3413bd501@proxmox.com> Date: Wed, 29 Jul 2026 17:45:42 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server 1/1] fix #7590: qemu-server: apply timeout to QEMU start fork To: =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= , pve-devel@lists.proxmox.com References: <20260625114500.159384-1-s.rufinatscha@proxmox.com> <20260625114500.159384-2-s.rufinatscha@proxmox.com> <1783952852.13tv1348l8.astroid@yuna.none> Content-Language: en-US From: Samuel Rufinatscha In-Reply-To: <1783952852.13tv1348l8.astroid@yuna.none> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: UJZRXHKF6P3JVX2WNQTI5S74KWFRDVJN X-Message-ID-Hash: UJZRXHKF6P3JVX2WNQTI5S74KWFRDVJN X-MailFrom: s.rufinatscha@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 7/13/26 4:37 PM, Fabian Grünbichler wrote: > On June 25, 2026 1:45 pm, Samuel Rufinatscha wrote: >> Apply the existing start timeout to the forked QEMU startup path and >> clean up the VM scope on timeout. This prevents stuck qmstart tasks >> from holding the config lock indefinitely or leaving a partially >> started QEMU process running. >> >> Signed-off-by: Samuel Rufinatscha >> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7590 >> --- >> src/PVE/QemuServer.pm | 28 +++++++++++++++++++++++++++- >> 1 file changed, 27 insertions(+), 1 deletion(-) >> >> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm >> index 55e9f520..df400376 100644 >> --- a/src/PVE/QemuServer.pm >> +++ b/src/PVE/QemuServer.pm >> @@ -5762,7 +5762,7 @@ sub vm_start_nolock { >> }; >> >> my $run_qemu = sub { >> - PVE::Tools::run_fork sub { >> + my $run_qemu_child = sub { >> PVE::Systemd::enter_systemd_scope($vmid, "Proxmox VE VM $vmid", >> %systemd_properties); >> >> @@ -5791,6 +5791,32 @@ sub vm_start_nolock { >> die "QEMU exited with code $exitcode\n"; >> } >> }; >> + >> + my (undef, $timed_out) = >> + PVE::Tools::run_fork_with_timeout($start_timeout || undef, $run_qemu_child); > > the timeout here is not the same that is used by the run_command > invocation in the sub above.. and it would also need to account for the > time it takes to start swtpm and all virtiofsd instances on top of that. I see, makes sense.. we need to count for the helpers. Will fix in v2. > >> + >> + if ($timed_out) { >> + eval { >> + run_command( >> + [ >> + '/bin/systemctl', >> + 'kill', >> + '--kill-whom=all', >> + '--signal=KILL', >> + "$vmid.scope", >> + ], >> + %silence_std_outs, >> + noerr => 1, >> + timeout => 10, >> + ); >> + PVE::Systemd::wait_for_unit_removed("$vmid.scope", 20); >> + }; >> + warn "failed to clean up timed-out VM start scope - $@" if $@; >> + >> + $cleanup_qsd->(); > > isn't the order here wrong? first we should cleanup any running QSD > instances, then kill the scope - after all the QSD instance is running > inside the scope.. similarly, the tpmpid killing should also be done Good point, yes. $cleanup_qsd expects QSD to be running else it is not doing much. tpmpid should be explicitly cleaned too like in the regular cleanup path. > here.. and in the regular cleanup path, we do not explicitly kill the > scope? Good point, one surviving helper would be enough to keep $vmid.scope active, so this should be fixed too. > > I think we want unified error handling here, whether the start > explicitly failed, or implicitly failed by timing out.. Makes sense, I will align the 2 paths in v2 too. Thanks @Fabian! > >> + >> + die "QEMU start timed out after $start_timeout seconds\n"; >> + } >> }; >> >> if ($conf->{hugepages}) { >> -- >> 2.47.3 >> >> >> >> >> >>