[pve-devel] [PATCH-SERIES qemu-server 0/3] fix #6007: fix PBS template backup for certain configurations

[pve-devel] [PATCH-SERIES qemu-server 0/3] fix #6007: fix PBS template backup for certain configurations

[Fiona Ebner]

Fiona Ebner (3):
  backup: also restore VM power state for early failures
  config: introduce a flag to prevent writing the configuration
  fix #6007: template backup: use minimized configuration for handling
    the full vm start

 PVE/QemuConfig.pm        | 12 ++++++++++++
 PVE/QemuServer.pm        | 16 +++++++++++++---
 PVE/VZDump/QemuServer.pm | 12 ++++++------
 3 files changed, 31 insertions(+), 9 deletions(-)

-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 1/3] backup: also restore VM power state for early failures

[Fiona Ebner]

A failure during enforce_vm_running_for_backup() would result in
restore_vm_power_state() not being called, but the VM might already
have been started before the failure. In particular, this could happen
in the context of bug #6007 [0], where the 'set_link' QMP command
fails right after VM start.

If the failure happens before successful start, there will be an
additional error message issued by restore_vm_power_state() (that the
VM is not running). This could be avoided by returning early if the VM
is not running anymore, but that would mean not warning about it in
other scenarios where it is not expected and keeping track of whether
the VM was actually started or not does not seem to be worth it just
for avoiding that error message in this edge case.

[0]: https://bugzilla.proxmox.com/show_bug.cgi?id=6007

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/VZDump/QemuServer.pm | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 44b68ae9..9d7c26a3 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -698,15 +698,15 @@ sub archive_pbs {
     # get list early so we die on unknown drive types before doing anything
     my $devlist = _get_task_devlist($task);
 
-    $self->enforce_vm_running_for_backup($vmid);
-    $self->{qmeventd_fh} = PVE::QemuServer::register_qmeventd_handle($vmid);
-
     my $backup_job_uuid;
     eval {
 	$SIG{INT} = $SIG{TERM} = $SIG{QUIT} = $SIG{HUP} = $SIG{PIPE} = sub {
 	    die "interrupted by signal\n";
 	};
 
+	$self->enforce_vm_running_for_backup($vmid);
+	$self->{qmeventd_fh} = PVE::QemuServer::register_qmeventd_handle($vmid);
+
 	my $qemu_support = eval { mon_cmd($vmid, "query-proxmox-support") };
 	my $err = $@;
 	if (!$qemu_support || $err) {
@@ -893,9 +893,6 @@ sub archive_vma {
 
     my $devlist = _get_task_devlist($task);
 
-    $self->enforce_vm_running_for_backup($vmid);
-    $self->{qmeventd_fh} = PVE::QemuServer::register_qmeventd_handle($vmid);
-
     my $cpid;
     my $backup_job_uuid;
 
@@ -904,6 +901,9 @@ sub archive_vma {
 	    die "interrupted by signal\n";
 	};
 
+	$self->enforce_vm_running_for_backup($vmid);
+	$self->{qmeventd_fh} = PVE::QemuServer::register_qmeventd_handle($vmid);
+
 	# Currently, failing to determine Proxmox support is not critical here, because it's only
 	# used for performance settings like 'max-workers'.
 	my $qemu_support = eval { mon_cmd($vmid, "query-proxmox-support") };
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 2/3] config: introduce a flag to prevent writing the configuration

[Fiona Ebner]

To be used in the context of template backup, where a minimized
temporary configuration is created to start the VM in 'prelaunch'
mode. Issue a warning, so that code paths where this happens will be
noted and can be evaluated and adapted.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuConfig.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index ffdf9f03..a2156abb 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -13,6 +13,7 @@ use PVE::QemuServer::Monitor qw(mon_cmd);
 use PVE::QemuServer;
 use PVE::QemuServer::Machine;
 use PVE::QemuServer::Memory qw(get_current_memory);
+use PVE::RESTEnvironment qw(log_warn);
 use PVE::Storage;
 use PVE::Tools;
 use PVE::Format qw(render_bytes render_duration);
@@ -561,6 +562,17 @@ sub get_derived_property {
     }
 }
 
+sub write_config {
+    my ($class, $vmid, $conf) = @_;
+
+    if ($conf->{'no-write-config'}) {
+	log_warn("refusing to write temporary configuration with 'no-write-config' flag");
+	return;
+    }
+
+    return $class->SUPER::write_config($vmid, $conf);
+}
+
 # END implemented abstract methods from PVE::AbstractConfig
 
 sub has_cloudinit {
-- 
2.39.5



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

Re: [pve-devel] [PATCH qemu-server 2/3] config: introduce a flag to prevent writing the configuration

[Thomas Lamprecht]

Am 24.01.25 um 11:08 schrieb Fiona Ebner:
> To be used in the context of template backup, where a minimized
> temporary configuration is created to start the VM in 'prelaunch'
> mode. Issue a warning, so that code paths where this happens will be
> noted and can be evaluated and adapted.
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuConfig.pm | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index ffdf9f03..a2156abb 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -13,6 +13,7 @@ use PVE::QemuServer::Monitor qw(mon_cmd);
>  use PVE::QemuServer;
>  use PVE::QemuServer::Machine;
>  use PVE::QemuServer::Memory qw(get_current_memory);
> +use PVE::RESTEnvironment qw(log_warn);
>  use PVE::Storage;
>  use PVE::Tools;
>  use PVE::Format qw(render_bytes render_duration);
> @@ -561,6 +562,17 @@ sub get_derived_property {
>      }
>  }
>  
> +sub write_config {
> +    my ($class, $vmid, $conf) = @_;
> +
> +    if ($conf->{'no-write-config'}) {
> +	log_warn("refusing to write temporary configuration with 'no-write-config' flag");

hmm, mixing config values and metadata for control flow/debugging is
always getting hairy fast, making the config a blessed object with
method for QEMU stuff and a clean separation of config data and such
flags and other non-config data info might be a better alternative,
but demanding that now would add a ton of scope and delay the fix
significantly, so fine to go this way as stop gap.

On a v2 or on applying adding a (one of) fixme/todo/hack comment to
note that this is not ideal, to minimize the chance that doing this
sort of thing spreads, could be good though.

> +	return;
> +    }
> +
> +    return $class->SUPER::write_config($vmid, $conf);
> +}
> +
>  # END implemented abstract methods from PVE::AbstractConfig
>  
>  sub has_cloudinit {



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

Re: [pve-devel] [PATCH qemu-server 2/3] config: introduce a flag to prevent writing the configuration

[Fiona Ebner]

Am 24.01.25 um 11:18 schrieb Thomas Lamprecht:
> Am 24.01.25 um 11:08 schrieb Fiona Ebner:
>> +sub write_config {
>> +    my ($class, $vmid, $conf) = @_;
>> +
>> +    if ($conf->{'no-write-config'}) {
>> +	log_warn("refusing to write temporary configuration with 'no-write-config' flag");
> 
> hmm, mixing config values and metadata for control flow/debugging is
> always getting hairy fast, making the config a blessed object with
> method for QEMU stuff and a clean separation of config data and such
> flags and other non-config data info might be a better alternative,
> but demanding that now would add a ton of scope and delay the fix
> significantly, so fine to go this way as stop gap.
> 
> On a v2 or on applying adding a (one of) fixme/todo/hack comment to
> note that this is not ideal, to minimize the chance that doing this
> sort of thing spreads, could be good though.
> 

Ack, will add such a FIXME comment.


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

[pve-devel] [PATCH qemu-server 3/3] fix #6007: template backup: use minimized configuration for handling the full vm start

[Fiona Ebner]

Previously, the template's configuration was used as-is for the rest
of handling the VM start even if config_to_command() uses a minimized
configuration to build the command. This can lead to issues with a
network device with the 'link_down' flag set, because the network
device will not be present, but the start handling will still issue a
QMP command for it, leading to a failed backup operation.

Use the minimized configuration for the whole start-up handling to
avoid such issues.

Use the 'no-write-config' flag to safeguard against accidentally
writing out the temporarily modified config.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 808c0e1c..837b8916 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3396,6 +3396,7 @@ sub config_to_command {
 	    scsihw => $conf->{scsihw}, # so that the scsi disks are correctly added
 	    bios => $conf->{bios}, # so efidisk gets included if it exists
 	    name => $conf->{name}, # so it's correct in the process list
+	    'no-write-config' => 1, # make sure to not write this configuration
 	};
 
 	# copy all disks over
@@ -4017,7 +4018,7 @@ sub config_to_command {
 	push @$cmd, @$aa;
     }
 
-    return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices) : $cmd;
+    return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices, $conf) : $cmd;
 }
 
 sub check_rng_source {
@@ -5613,8 +5614,17 @@ sub vm_start_nolock {
 	print "Resuming suspended VM\n";
     }
 
-    my ($cmd, $vollist, $spice_port, $pci_devices) = config_to_command($storecfg, $vmid,
-	$conf, $defaults, $forcemachine, $forcecpu, $params->{'live-restore-backing'});
+    # Note that for certain cases like templates, the configuration is minimized, so need to ensure
+    # the rest of the function here uses the same configuration that was used to build the command
+    (my $cmd, my $vollist, my $spice_port, my $pci_devices, $conf) = config_to_command(
+	$storecfg,
+	$vmid,
+	$conf,
+	$defaults,
+	$forcemachine,
+	$forcecpu,
+	$params->{'live-restore-backing'},
+    );
 
     my $migration_ip;
     my $get_migration_ip = sub {
-- 
2.39.5



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

Re: [pve-devel] [PATCH qemu-server 3/3] fix #6007: template backup: use minimized configuration for handling the full vm start

[Fiona Ebner]

Am 24.01.25 um 11:08 schrieb Fiona Ebner:
> Previously, the template's configuration was used as-is for the rest
> of handling the VM start even if config_to_command() uses a minimized
> configuration to build the command. This can lead to issues with a
> network device with the 'link_down' flag set, because the network
> device will not be present, but the start handling will still issue a
> QMP command for it, leading to a failed backup operation.
> 
> Use the minimized configuration for the whole start-up handling to
> avoid such issues.
> 
> Use the 'no-write-config' flag to safeguard against accidentally
> writing out the temporarily modified config.
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 808c0e1c..837b8916 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3396,6 +3396,7 @@ sub config_to_command {
>  	    scsihw => $conf->{scsihw}, # so that the scsi disks are correctly added
>  	    bios => $conf->{bios}, # so efidisk gets included if it exists
>  	    name => $conf->{name}, # so it's correct in the process list
> +	    'no-write-config' => 1, # make sure to not write this configuration
>  	};
>  
>  	# copy all disks over
> @@ -4017,7 +4018,7 @@ sub config_to_command {
>  	push @$cmd, @$aa;
>      }
>  
> -    return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices) : $cmd;
> +    return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices, $conf) : $cmd;
>  }
>  

Hmm, thinking about it again, to reduce regression potential, we could
also just return the temporary config if it was actually required and
have the caller only assign it when really present.

>  sub check_rng_source {
> @@ -5613,8 +5614,17 @@ sub vm_start_nolock {
>  	print "Resuming suspended VM\n";
>      }
>  
> -    my ($cmd, $vollist, $spice_port, $pci_devices) = config_to_command($storecfg, $vmid,
> -	$conf, $defaults, $forcemachine, $forcecpu, $params->{'live-restore-backing'});
> +    # Note that for certain cases like templates, the configuration is minimized, so need to ensure
> +    # the rest of the function here uses the same configuration that was used to build the command
> +    (my $cmd, my $vollist, my $spice_port, my $pci_devices, $conf) = config_to_command(
> +	$storecfg,
> +	$vmid,
> +	$conf,
> +	$defaults,
> +	$forcemachine,
> +	$forcecpu,
> +	$params->{'live-restore-backing'},
> +    );
>  
>      my $migration_ip;
>      my $get_migration_ip = sub {



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

Re: [pve-devel] [PATCH qemu-server 3/3] fix #6007: template backup: use minimized configuration for handling the full vm start

[Fiona Ebner]

Am 24.01.25 um 11:23 schrieb Fiona Ebner:
>> @@ -4017,7 +4018,7 @@ sub config_to_command {
>>  	push @$cmd, @$aa;
>>      }
>>  
>> -    return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices) : $cmd;
>> +    return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices, $conf) : $cmd;
>>  }
>>  
> 
> Hmm, thinking about it again, to reduce regression potential, we could
> also just return the temporary config if it was actually required and
> have the caller only assign it when really present.

Although I suppose it doesn't really give us anything after all. I was
worried a bit about things like potential auto-vivification, but since
we already use the original $conf hash ref throughout the function
(except in the template case), such things would already affect the caller.


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