public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH qemu-server 1/1] create_vm: assume HA state 'started' when live-restoring guests
@ 2025-11-20 16:34 Michael Köppl
  2025-11-21  9:45 ` Fiona Ebner
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Köppl @ 2025-11-20 16:34 UTC (permalink / raw)
  To: pve-devel

To avoid shutting down the VM when performing a live-restore, consider
live-restore=1 to translate to a HA state of 'started', similar to
start=1.

Reported-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
Used defined-or since in pve-manager, we only ever set one of the two
values and in any other case I'd give precedence to the 'start'
parameter if it is explicitly set.

 src/PVE/API2/Qemu.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index c580bf63..33427ca7 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -1237,7 +1237,8 @@ __PACKAGE__->register_method({
         my $bwlimit = extract_param($param, 'bwlimit');
         my $force = extract_param($param, 'force');
         my $pool = extract_param($param, 'pool');
-        my $start_after_create = extract_param($param, 'start');
+        my $start_after_create = extract_param($param, 'start')
+            // extract_param($param, 'live-restore');
         my $ha_managed = extract_param($param, 'ha-managed');
         my $storage = extract_param($param, 'storage');
         my $unique = extract_param($param, 'unique');
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH qemu-server 1/1] create_vm: assume HA state 'started' when live-restoring guests
  2025-11-20 16:34 [pve-devel] [PATCH qemu-server 1/1] create_vm: assume HA state 'started' when live-restoring guests Michael Köppl
@ 2025-11-21  9:45 ` Fiona Ebner
  2025-11-21 10:05   ` Thomas Lamprecht
  0 siblings, 1 reply; 8+ messages in thread
From: Fiona Ebner @ 2025-11-21  9:45 UTC (permalink / raw)
  To: Michael Köppl, pve-devel

Am 20.11.25 um 5:34 PM schrieb Michael Köppl:
> To avoid shutting down the VM when performing a live-restore, consider
> live-restore=1 to translate to a HA state of 'started', similar to
> start=1.
> 
> Reported-by: Fiona Ebner <f.ebner@proxmox.com>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
> Used defined-or since in pve-manager, we only ever set one of the two
> values and in any other case I'd give precedence to the 'start'
> parameter if it is explicitly set.

This could be part of the commit message. But from looking at the code,
it seems like the actual behavior if start=0 and live-restore=1 is still
doing a live restore, so the HA state should still be started in that case.

> 
>  src/PVE/API2/Qemu.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
> index c580bf63..33427ca7 100644
> --- a/src/PVE/API2/Qemu.pm
> +++ b/src/PVE/API2/Qemu.pm
> @@ -1237,7 +1237,8 @@ __PACKAGE__->register_method({
>          my $bwlimit = extract_param($param, 'bwlimit');
>          my $force = extract_param($param, 'force');
>          my $pool = extract_param($param, 'pool');
> -        my $start_after_create = extract_param($param, 'start');
> +        my $start_after_create = extract_param($param, 'start')
> +            // extract_param($param, 'live-restore');

It's already started during create, so using the $start_after_create
variable seems like a slight misfit. Why not just also check for
$live_restore when setting the HA state when the resource is added?

>          my $ha_managed = extract_param($param, 'ha-managed');
>          my $storage = extract_param($param, 'storage');
>          my $unique = extract_param($param, 'unique');



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH qemu-server 1/1] create_vm: assume HA state 'started' when live-restoring guests
  2025-11-21  9:45 ` Fiona Ebner
@ 2025-11-21 10:05   ` Thomas Lamprecht
  2025-11-21 10:15     ` Fiona Ebner
  2025-11-25 13:44     ` Michael Köppl
  0 siblings, 2 replies; 8+ messages in thread
From: Thomas Lamprecht @ 2025-11-21 10:05 UTC (permalink / raw)
  To: Proxmox VE development discussion, Fiona Ebner, Michael Köppl

Am 21.11.25 um 10:45 schrieb Fiona Ebner:
> Am 20.11.25 um 5:34 PM schrieb Michael Köppl:
>> To avoid shutting down the VM when performing a live-restore, consider
>> live-restore=1 to translate to a HA state of 'started', similar to
>> start=1.
>>
>> Reported-by: Fiona Ebner <f.ebner@proxmox.com>
>> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
>> ---
>> Used defined-or since in pve-manager, we only ever set one of the two
>> values and in any other case I'd give precedence to the 'start'
>> parameter if it is explicitly set.
> 
> This could be part of the commit message. But from looking at the code,
> it seems like the actual behavior if start=0 and live-restore=1 is still
> doing a live restore, so the HA state should still be started in that case.
> 
>>
>>  src/PVE/API2/Qemu.pm | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
>> index c580bf63..33427ca7 100644
>> --- a/src/PVE/API2/Qemu.pm
>> +++ b/src/PVE/API2/Qemu.pm
>> @@ -1237,7 +1237,8 @@ __PACKAGE__->register_method({
>>          my $bwlimit = extract_param($param, 'bwlimit');
>>          my $force = extract_param($param, 'force');
>>          my $pool = extract_param($param, 'pool');
>> -        my $start_after_create = extract_param($param, 'start');
>> +        my $start_after_create = extract_param($param, 'start')
>> +            // extract_param($param, 'live-restore');
> 
> It's already started during create, so using the $start_after_create
> variable seems like a slight misfit. Why not just also check for
> $live_restore when setting the HA state when the resource is added?
> 

We lock the config on restore, or?
If so, couldn't we handle this in the HA stack and do not shutdown if a
restore lock is present in the config?


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH qemu-server 1/1] create_vm: assume HA state 'started' when live-restoring guests
  2025-11-21 10:05   ` Thomas Lamprecht
@ 2025-11-21 10:15     ` Fiona Ebner
  2025-11-21 10:24       ` Thomas Lamprecht
  2025-11-25 13:44     ` Michael Köppl
  1 sibling, 1 reply; 8+ messages in thread
From: Fiona Ebner @ 2025-11-21 10:15 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion, Michael Köppl

Am 21.11.25 um 11:04 AM schrieb Thomas Lamprecht:
> Am 21.11.25 um 10:45 schrieb Fiona Ebner:
>> Am 20.11.25 um 5:34 PM schrieb Michael Köppl:
>>> To avoid shutting down the VM when performing a live-restore, consider
>>> live-restore=1 to translate to a HA state of 'started', similar to
>>> start=1.
>>>
>>> Reported-by: Fiona Ebner <f.ebner@proxmox.com>
>>> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
>>> ---
>>> Used defined-or since in pve-manager, we only ever set one of the two
>>> values and in any other case I'd give precedence to the 'start'
>>> parameter if it is explicitly set.
>>
>> This could be part of the commit message. But from looking at the code,
>> it seems like the actual behavior if start=0 and live-restore=1 is still
>> doing a live restore, so the HA state should still be started in that case.
>>
>>>
>>>  src/PVE/API2/Qemu.pm | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
>>> index c580bf63..33427ca7 100644
>>> --- a/src/PVE/API2/Qemu.pm
>>> +++ b/src/PVE/API2/Qemu.pm
>>> @@ -1237,7 +1237,8 @@ __PACKAGE__->register_method({
>>>          my $bwlimit = extract_param($param, 'bwlimit');
>>>          my $force = extract_param($param, 'force');
>>>          my $pool = extract_param($param, 'pool');
>>> -        my $start_after_create = extract_param($param, 'start');
>>> +        my $start_after_create = extract_param($param, 'start')
>>> +            // extract_param($param, 'live-restore');
>>
>> It's already started during create, so using the $start_after_create
>> variable seems like a slight misfit. Why not just also check for
>> $live_restore when setting the HA state when the resource is added?
>>
> 
> We lock the config on restore, or?
> If so, couldn't we handle this in the HA stack and do not shutdown if a
> restore lock is present in the config?

But that'd be more coupling? Further below in the endpoint we have (two
instances of):

>             if ($ha_managed) {
>                 print "Add as HA resource\n";
>                 my $state = $start_after_create ? 'started' : 'stopped';
>                 my $cmd = ['ha-manager', 'add', "vm:$vmid", '--state', $state];
>                 eval { PVE::Tools::run_command($cmd); };
>                 warn $@ if $@;
>             }

So we could just put $start_after_create || $live_restore there


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH qemu-server 1/1] create_vm: assume HA state 'started' when live-restoring guests
  2025-11-21 10:15     ` Fiona Ebner
@ 2025-11-21 10:24       ` Thomas Lamprecht
  2025-11-21 10:32         ` Fiona Ebner
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Lamprecht @ 2025-11-21 10:24 UTC (permalink / raw)
  To: Fiona Ebner, Proxmox VE development discussion, Michael Köppl

Am 21.11.25 um 11:15 schrieb Fiona Ebner:
> Am 21.11.25 um 11:04 AM schrieb Thomas Lamprecht:
>> We lock the config on restore, or?
>> If so, couldn't we handle this in the HA stack and do not shutdown if a
>> restore lock is present in the config?
> 
> But that'd be more coupling? Further below in the endpoint we have (two
> instances of):

Making the HA actually aware of how the resources it manages work, so that
it can make better decisions, is definitively not more coupling.

> 
>>             if ($ha_managed) {
>>                 print "Add as HA resource\n";
>>                 my $state = $start_after_create ? 'started' : 'stopped';
>>                 my $cmd = ['ha-manager', 'add', "vm:$vmid", '--state', $state];
>>                 eval { PVE::Tools::run_command($cmd); };
>>                 warn $@ if $@;
>>             }
> 
> So we could just put $start_after_create || $live_restore there

Does not solves the case for when restores over a existing VM that is a HA
resource already, FWICT.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH qemu-server 1/1] create_vm: assume HA state 'started' when live-restoring guests
  2025-11-21 10:24       ` Thomas Lamprecht
@ 2025-11-21 10:32         ` Fiona Ebner
  2025-11-21 11:13           ` Thomas Lamprecht
  0 siblings, 1 reply; 8+ messages in thread
From: Fiona Ebner @ 2025-11-21 10:32 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion, Michael Köppl

Am 21.11.25 um 11:23 AM schrieb Thomas Lamprecht:
> Am 21.11.25 um 11:15 schrieb Fiona Ebner:
>> Am 21.11.25 um 11:04 AM schrieb Thomas Lamprecht:
>>> We lock the config on restore, or?
>>> If so, couldn't we handle this in the HA stack and do not shutdown if a
>>> restore lock is present in the config?
>>
>> But that'd be more coupling? Further below in the endpoint we have (two
>> instances of):
> 
> Making the HA actually aware of how the resources it manages work, so that
> it can make better decisions, is definitively not more coupling.

Fair point.

>>
>>>             if ($ha_managed) {
>>>                 print "Add as HA resource\n";
>>>                 my $state = $start_after_create ? 'started' : 'stopped';
>>>                 my $cmd = ['ha-manager', 'add', "vm:$vmid", '--state', $state];
>>>                 eval { PVE::Tools::run_command($cmd); };
>>>                 warn $@ if $@;
>>>             }
>>
>> So we could just put $start_after_create || $live_restore there
> 
> Does not solves the case for when restores over a existing VM that is a HA
> resource already, FWICT.

The code block above still needs fixing regardless? Since it sets the
state for a newly added resource.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH qemu-server 1/1] create_vm: assume HA state 'started' when live-restoring guests
  2025-11-21 10:32         ` Fiona Ebner
@ 2025-11-21 11:13           ` Thomas Lamprecht
  0 siblings, 0 replies; 8+ messages in thread
From: Thomas Lamprecht @ 2025-11-21 11:13 UTC (permalink / raw)
  To: Proxmox VE development discussion, Fiona Ebner, Michael Köppl

Am 21.11.25 um 11:32 schrieb Fiona Ebner:
> Am 21.11.25 um 11:23 AM schrieb Thomas Lamprecht:
>> Am 21.11.25 um 11:15 schrieb Fiona Ebner:
>>> Am 21.11.25 um 11:04 AM schrieb Thomas Lamprecht:
>>>> We lock the config on restore, or?
>>>> If so, couldn't we handle this in the HA stack and do not shutdown if a
>>>> restore lock is present in the config?
>>>
>>> But that'd be more coupling? Further below in the endpoint we have (two
>>> instances of):
>>
>> Making the HA actually aware of how the resources it manages work, so that
>> it can make better decisions, is definitively not more coupling.
> 
> Fair point.

To also be fair, it is coupling, but this is warranted one (at least for the
current design); not all coupling is inherently bad, it always should be
rather explicit and not hidden though.

>>>>             if ($ha_managed) {
>>>>                 print "Add as HA resource\n";
>>>>                 my $state = $start_after_create ? 'started' : 'stopped';
>>>>                 my $cmd = ['ha-manager', 'add', "vm:$vmid", '--state', $state];
>>>>                 eval { PVE::Tools::run_command($cmd); };
>>>>                 warn $@ if $@;
>>>>             }
>>>
>>> So we could just put $start_after_create || $live_restore there
>>
>> Does not solves the case for when restores over a existing VM that is a HA
>> resource already, FWICT.
> 
> The code block above still needs fixing regardless? Since it sets the
> state for a newly added resource.

Yes, I think so. One is for not stopping during restore and the other is
for not stopping them after the restore, and live-restore already *does*
imply start-after-create (well, restore here, but same same).


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH qemu-server 1/1] create_vm: assume HA state 'started' when live-restoring guests
  2025-11-21 10:05   ` Thomas Lamprecht
  2025-11-21 10:15     ` Fiona Ebner
@ 2025-11-25 13:44     ` Michael Köppl
  1 sibling, 0 replies; 8+ messages in thread
From: Michael Köppl @ 2025-11-25 13:44 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion, Fiona Ebner,
	Michael Köppl

On Fri Nov 21, 2025 at 11:05 AM CET, Thomas Lamprecht wrote:
> Am 21.11.25 um 10:45 schrieb Fiona Ebner:
>> Am 20.11.25 um 5:34 PM schrieb Michael Köppl:
>>> To avoid shutting down the VM when performing a live-restore, consider
>>> live-restore=1 to translate to a HA state of 'started', similar to
>>> start=1.
>>>
>>> Reported-by: Fiona Ebner <f.ebner@proxmox.com>
>>> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
>>> ---
>>> Used defined-or since in pve-manager, we only ever set one of the two
>>> values and in any other case I'd give precedence to the 'start'
>>> parameter if it is explicitly set.
>> 
>> This could be part of the commit message. But from looking at the code,
>> it seems like the actual behavior if start=0 and live-restore=1 is still
>> doing a live restore, so the HA state should still be started in that case.
>> 
>>>
>>>  src/PVE/API2/Qemu.pm | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
>>> index c580bf63..33427ca7 100644
>>> --- a/src/PVE/API2/Qemu.pm
>>> +++ b/src/PVE/API2/Qemu.pm
>>> @@ -1237,7 +1237,8 @@ __PACKAGE__->register_method({
>>>          my $bwlimit = extract_param($param, 'bwlimit');
>>>          my $force = extract_param($param, 'force');
>>>          my $pool = extract_param($param, 'pool');
>>> -        my $start_after_create = extract_param($param, 'start');
>>> +        my $start_after_create = extract_param($param, 'start')
>>> +            // extract_param($param, 'live-restore');
>> 
>> It's already started during create, so using the $start_after_create
>> variable seems like a slight misfit. Why not just also check for
>> $live_restore when setting the HA state when the resource is added?
>> 
>
> We lock the config on restore, or?
> If so, couldn't we handle this in the HA stack and do not shutdown if a
> restore lock is present in the config?

Thought a bit about this for my approach for v2. I implemented v2 like
@Fiona suggested ($start_after_create || $live_restore), but also added
a check that only sets the status of an existing resource instead of
trying to add one when restoring over an existing VM that already is a
HA resource. I'm not sure I understand why this would instead be handled
in the HA stack depending on the existence of the lock, though. Setting
the state correctly should already be enough, no? Or am I missing
something?



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-11-25 13:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-11-20 16:34 [pve-devel] [PATCH qemu-server 1/1] create_vm: assume HA state 'started' when live-restoring guests Michael Köppl
2025-11-21  9:45 ` Fiona Ebner
2025-11-21 10:05   ` Thomas Lamprecht
2025-11-21 10:15     ` Fiona Ebner
2025-11-21 10:24       ` Thomas Lamprecht
2025-11-21 10:32         ` Fiona Ebner
2025-11-21 11:13           ` Thomas Lamprecht
2025-11-25 13:44     ` Michael Köppl

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal