* [pve-devel] [PATCH v2 qemu-server 1/1] Do not start VM twice when rollbacking with --start
@ 2022-11-21 13:13 Stefan Hanreich
2022-12-21 15:02 ` Fabian Grünbichler
0 siblings, 1 reply; 3+ messages in thread
From: Stefan Hanreich @ 2022-11-21 13:13 UTC (permalink / raw)
To: pve-devel
When rollbacking to the snapshot of a VM that includes RAM, the VM
gets started by the rollback task anyway, so no additional start task is
needed. Previously, when running rollback with the --start parameter
and the VM snapshot includes RAM, a start task was created. That task
failed because the VM had already been started by the rollback task.
Additionally documented this behaviour in the description of the --start
parameter
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
Changes v1 -> v2:
Do not parse config for checking type of snapshot but rather directly check
whether VM is running or not via check_running()
PVE/API2/Qemu.pm | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 6bdcce2..691202d 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -5064,7 +5064,8 @@ __PACKAGE__->register_method({
snapname => get_standard_option('pve-snapshot-name'),
start => {
type => 'boolean',
- description => "Whether the VM should get started after rolling back successfully",
+ description => "Whether the VM should get started after rolling back successfully."
+ . " A VM will always be started when rollbacking a snapshot with RAM included, regardless of this parameter.",
optional => 1,
default => 0,
},
@@ -5091,7 +5092,7 @@ __PACKAGE__->register_method({
PVE::Cluster::log_msg('info', $authuser, "rollback snapshot VM $vmid: $snapname");
PVE::QemuConfig->snapshot_rollback($vmid, $snapname);
- if ($param->{start}) {
+ if ($param->{start} && !PVE::QemuServer::check_running($vmid)) {
PVE::API2::Qemu->vm_start({ vmid => $vmid, node => $node });
}
};
--
2.30.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH v2 qemu-server 1/1] Do not start VM twice when rollbacking with --start
2022-11-21 13:13 [pve-devel] [PATCH v2 qemu-server 1/1] Do not start VM twice when rollbacking with --start Stefan Hanreich
@ 2022-12-21 15:02 ` Fabian Grünbichler
2022-12-21 15:17 ` Stefan Hanreich
0 siblings, 1 reply; 3+ messages in thread
From: Fabian Grünbichler @ 2022-12-21 15:02 UTC (permalink / raw)
To: Proxmox VE development discussion
On November 21, 2022 2:13 pm, Stefan Hanreich wrote:
> When rollbacking to the snapshot of a VM that includes RAM, the VM
> gets started by the rollback task anyway, so no additional start task is
> needed. Previously, when running rollback with the --start parameter
> and the VM snapshot includes RAM, a start task was created. That task
> failed because the VM had already been started by the rollback task.
>
> Additionally documented this behaviour in the description of the --start
> parameter
>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>
> Changes v1 -> v2:
> Do not parse config for checking type of snapshot but rather directly check
> whether VM is running or not via check_running()
>
> PVE/API2/Qemu.pm | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 6bdcce2..691202d 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -5064,7 +5064,8 @@ __PACKAGE__->register_method({
> snapname => get_standard_option('pve-snapshot-name'),
> start => {
> type => 'boolean',
> - description => "Whether the VM should get started after rolling back successfully",
> + description => "Whether the VM should get started after rolling back successfully."
> + . " A VM will always be started when rollbacking a snapshot with RAM included, regardless of this parameter.",
this is worded a bit weird (I don't think that "rollbacking" is a word ;)), how about:
. "(Note: VMs will be automatically started if the snapshot includes RAM.)",
> optional => 1,
> default => 0,
> },
> @@ -5091,7 +5092,7 @@ __PACKAGE__->register_method({
> PVE::Cluster::log_msg('info', $authuser, "rollback snapshot VM $vmid: $snapname");
> PVE::QemuConfig->snapshot_rollback($vmid, $snapname);
>
> - if ($param->{start}) {
> + if ($param->{start} && !PVE::QemuServer::check_running($vmid)) {
unless I am missing something, this should use
PVE::QemuServer::Helpers::vm_running_locally($vmid)
we are holding the guest migration lock for the whole rollback worker, and
snapshot_rollback loads the config, so we know it is on the current node at this
point and just checking whether a matching qemu process is running after the
rollback is enough.
> PVE::API2::Qemu->vm_start({ vmid => $vmid, node => $node });
> }
> };
> --
> 2.30.2
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH v2 qemu-server 1/1] Do not start VM twice when rollbacking with --start
2022-12-21 15:02 ` Fabian Grünbichler
@ 2022-12-21 15:17 ` Stefan Hanreich
0 siblings, 0 replies; 3+ messages in thread
From: Stefan Hanreich @ 2022-12-21 15:17 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Grünbichler
On 12/21/22 16:02, Fabian Grünbichler wrote:
> On November 21, 2022 2:13 pm, Stefan Hanreich wrote:
>> When rollbacking to the snapshot of a VM that includes RAM, the VM
>> gets started by the rollback task anyway, so no additional start task is
>> needed. Previously, when running rollback with the --start parameter
>> and the VM snapshot includes RAM, a start task was created. That task
>> failed because the VM had already been started by the rollback task.
>>
>> Additionally documented this behaviour in the description of the --start
>> parameter
>>
>> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
>> ---
>>
>> Changes v1 -> v2:
>> Do not parse config for checking type of snapshot but rather directly check
>> whether VM is running or not via check_running()
>>
>> PVE/API2/Qemu.pm | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 6bdcce2..691202d 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -5064,7 +5064,8 @@ __PACKAGE__->register_method({
>> snapname => get_standard_option('pve-snapshot-name'),
>> start => {
>> type => 'boolean',
>> - description => "Whether the VM should get started after rolling back successfully",
>> + description => "Whether the VM should get started after rolling back successfully."
>> + . " A VM will always be started when rollbacking a snapshot with RAM included, regardless of this parameter.",
>
> this is worded a bit weird (I don't think that "rollbacking" is a word ;)), how about:
>
> . "(Note: VMs will be automatically started if the snapshot includes RAM.)",
>
sounds way better! will also improve the commit message ;)
>> optional => 1,
>> default => 0,
>> },
>> @@ -5091,7 +5092,7 @@ __PACKAGE__->register_method({
>> PVE::Cluster::log_msg('info', $authuser, "rollback snapshot VM $vmid: $snapname");
>> PVE::QemuConfig->snapshot_rollback($vmid, $snapname);
>>
>> - if ($param->{start}) {
>> + if ($param->{start} && !PVE::QemuServer::check_running($vmid)) {
>
> unless I am missing something, this should use
>
> PVE::QemuServer::Helpers::vm_running_locally($vmid)
>
> we are holding the guest migration lock for the whole rollback worker, and
> snapshot_rollback loads the config, so we know it is on the current node at this
> point and just checking whether a matching qemu process is running after the
> rollback is enough.
will do
>
>> PVE::API2::Qemu->vm_start({ vmid => $vmid, node => $node });
>> }
>> };
>> --
>> 2.30.2
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-12-21 15:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-21 13:13 [pve-devel] [PATCH v2 qemu-server 1/1] Do not start VM twice when rollbacking with --start Stefan Hanreich
2022-12-21 15:02 ` Fabian Grünbichler
2022-12-21 15:17 ` Stefan Hanreich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal