From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 2D1401FF0B3 for ; Wed, 11 Nov 2026 17:20:39 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id AD4E421649; Wed, 11 Nov 2026 17:20:34 +0100 (CET) Message-ID: <35b0b233-581f-443e-9bb3-af1344d81f5e@proxmox.com> Date: Wed, 11 Nov 2026 17:20:29 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH docs/guest-common/manager/qemu-server v2 0/8] fix #7213: add `powercycle` reboot behavior To: Jakob Klocker , pve-devel@lists.proxmox.com References: <20261022125111.236538-1-j.klocker@proxmox.com> Content-Language: en-US From: David Riley In-Reply-To: <20261022125111.236538-1-j.klocker@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794414029432 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.637 Adjusted score from AWL reputation of From: address 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_MED -2.3 Sender listed at https://www.dnswl.org/, medium 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: 2XS2632W3A7UNDDNYJNMZFQOO3IIDKOD X-Message-ID-Hash: 2XS2632W3A7UNDDNYJNMZFQOO3IIDKOD X-MailFrom: d.riley@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: Thanks for the patch series! This series seems to be coming along nicely. I did however notice a compatibility issue with PDM and a potentially dangerous edge case that I want to mention here. PDM compatibility: Currently, there doesn't seem to be a companion patch for PDM. Because this series changes the reboot option from a boolean to a property string, viewing a VM configuration in PDM with this new property set throws the following error: "error:invalid value: string "1,powercycle=1", expected a boolean-like value" Pending config edge case: This introduces a dangerous edge case for separated teams (e.g., Guest Admins vs. Sys Admins). Because "-no-reboot" terminates the QEMU process immediately, PVE must perform a cold start. If a PVE Admin previously staged an invalid pending configuration (e.g., overprovisioning vCPUs), a simple guest-initiated reboot will permanently kill the VM until a Sys Admin intervenes to fix the config. "TASK ERROR: MAX 4 vcpus allowed per VM on this node" While this is technically 'user error' by whoever staged the bad config, the powercycle feature seems to amplify the blast radius by turning a normally safe, action, like a guest OS reboot into a fatal state change. I think a hint in the WebUI and Documentation mentioning this would be good. I also left some additional comments on the patches themselves. On 10/22/26 2:51 PM, Jakob Klocker wrote: > A reboot initiated inside the guest is handled by QEMU itself: the > process keeps running and only the guest OS boots again. That is fast, > but the VM stays on the configuration and QEMU binary it was started > with, so pending changes remain pending. > > This series adds a `powercycle` sub-property to the existing `reboot` > option. With it set, the VM is started with `-no-reboot`, so QEMU exits > on a reset; qmeventd reports the reason, and `qm cleanup` starts > the VM again. The result is a full start, so pending changes take > effect. > > To carry the new sub-property, `reboot` is converted to a property > string. Existing configurations stay valid, as the previous boolean > value is the default key. > > Note two behavior changes beyond the new sub-property: `reboot: 0` now > also prevents a restart for reboots triggered through the web interface, > `qm` or the API - previously only guest-initiated reboots were affected. > The documented meaning of the option is that the VM does not reboot, so > this seemed like the more consistent reading. And with `powercycle` set, > a reset requested through the web interface, `qm` or the API also becomes > a full stop and start, since `-no-reboot` makes QEMU exit on any reset. > > changes from v1 to v2 : > - fix pending delete for options with value 0 > - warn when restarting a VM with reboot disabled > - reboot on `qm reset` even if reboot is disabled, feels more > intuitive > - style fixes according to proxmox style guide (thanks @david) > > > Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7213 > > > pve-guest-common: > > Jakob Klocker (1): > AbstractConfig: fix pending delete for options with value 0 > > src/PVE/AbstractConfig.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > > pve-docs: > > Jakob Klocker (1): > qm: add reboot behavior information > > qm.adoc | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > > qemu-server: > > Jakob Klocker (5): > qm: do not restart VM when `reboot` is disabled > fix #7213: config: add `powercycle` sub-property to `reboot` > fix #7213: qm: cleanup: add `reset` parameter to honor `powercycle` > fix #7213: qmeventd: pass `reset` to `qm cleanup` > vm_reboot: warn when rebooting a VM with `reboot` disabled > > src/PVE/CLI/qm.pm | 25 ++++++++++++++++++--- > src/PVE/QemuServer.pm | 50 ++++++++++++++++++++++++++++++++++++----- > src/qmeventd/qmeventd.c | 36 ++++++++++++++++++++++------- > src/qmeventd/qmeventd.h | 1 + > 4 files changed, 96 insertions(+), 16 deletions(-) > > > pve-manager: > > Jakob Klocker (1): > ui: qemu: options: add editor for reboot behavior > > www/manager6/Makefile | 1 + > www/manager6/Utils.js | 17 +++++++ > www/manager6/form/RebootFeatureSelector.js | 59 ++++++++++++++++++++++ > www/manager6/qemu/Options.js | 14 +++++ > 4 files changed, 91 insertions(+) > create mode 100644 www/manager6/form/RebootFeatureSelector.js > > > Summary over all repositories: > 10 files changed, 228 insertions(+), 17 deletions(-) >