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

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

[Fiona Ebner]

Changes in v3:
* actually decouple config data and control flow by using a blessed object

Changes in v2:
* add comment about decoupling config data and metadata

Fiona Ebner (3):
  backup: also restore VM power state for early failures
  config: add special class that prevents writing the configuration
  fix #6007: template backup: use minimized configuration for handling
    the full vm start

 PVE/Makefile              |  1 +
 PVE/QemuConfig.pm         | 15 +++++++++++++++
 PVE/QemuConfig/Makefile   |  5 +++++
 PVE/QemuConfig/NoWrite.pm | 25 +++++++++++++++++++++++++
 PVE/QemuServer.pm         | 17 ++++++++++++++---
 PVE/VZDump/QemuServer.pm  | 12 ++++++------
 6 files changed, 66 insertions(+), 9 deletions(-)
 create mode 100644 PVE/QemuConfig/Makefile
 create mode 100644 PVE/QemuConfig/NoWrite.pm

-- 
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 v3 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>
---

No changes in v3.

 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 v3 2/3] config: add special class that prevents 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.

Since the code currently doesn't use blessed config objects, special
dispatching is needed to potentially defer to the new child class in
the write_config() method.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v3:
* use dedicated class

 PVE/Makefile              |  1 +
 PVE/QemuConfig.pm         | 15 +++++++++++++++
 PVE/QemuConfig/Makefile   |  5 +++++
 PVE/QemuConfig/NoWrite.pm | 25 +++++++++++++++++++++++++
 4 files changed, 46 insertions(+)
 create mode 100644 PVE/QemuConfig/Makefile
 create mode 100644 PVE/QemuConfig/NoWrite.pm

diff --git a/PVE/Makefile b/PVE/Makefile
index dc173681..440fbe77 100644
--- a/PVE/Makefile
+++ b/PVE/Makefile
@@ -11,4 +11,5 @@ install:
 	$(MAKE) -C VZDump install
 	$(MAKE) -C API2 install
 	$(MAKE) -C CLI install
+	$(MAKE) -C QemuConfig install
 	$(MAKE) -C QemuServer install
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index ffdf9f03..b60cc398 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -3,6 +3,8 @@ package PVE::QemuConfig;
 use strict;
 use warnings;
 
+use Scalar::Util qw(blessed);
+
 use PVE::AbstractConfig;
 use PVE::INotify;
 use PVE::JSONSchema;
@@ -561,6 +563,19 @@ sub get_derived_property {
     }
 }
 
+sub write_config {
+    my ($class, $vmid, $conf) = @_;
+
+    # Dispatch to class the object was blessed with if caller invoked the method via the
+    # 'PVE::QemuConfig' class name explicitly. This is hack, but the code currently doesn't
+    # generally use blessed config objects. Safeguard against infinite recursion.
+    if (blessed($conf) && !blessed($class)) {
+	return $conf->write_config($vmid, $conf);
+    }
+
+    return $class->SUPER::write_config($vmid, $conf);
+}
+
 # END implemented abstract methods from PVE::AbstractConfig
 
 sub has_cloudinit {
diff --git a/PVE/QemuConfig/Makefile b/PVE/QemuConfig/Makefile
new file mode 100644
index 00000000..c3a0d277
--- /dev/null
+++ b/PVE/QemuConfig/Makefile
@@ -0,0 +1,5 @@
+SOURCES=NoWrite.pm
+
+.PHONY: install
+install: ${SOURCES}
+	for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/QemuConfig/$$i; done
diff --git a/PVE/QemuConfig/NoWrite.pm b/PVE/QemuConfig/NoWrite.pm
new file mode 100644
index 00000000..f697506f
--- /dev/null
+++ b/PVE/QemuConfig/NoWrite.pm
@@ -0,0 +1,25 @@
+package PVE::QemuConfig::NoWrite;
+
+use strict;
+use warnings;
+
+use PVE::RESTEnvironment qw(log_warn);
+
+use base qw(PVE::QemuConfig);
+
+sub new {
+    my ($class, $conf) = @_;
+
+    bless($conf, $class);
+
+    return $conf;
+}
+
+sub write_config {
+    my ($class, $vmid, $conf) = @_;
+
+    log_warn("refusing to write temporary configuration");
+    return;
+}
+
+1;
-- 
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 v3 2/3] config: add special class that prevents writing the configuration

[Wolfgang Bumiller]

On Wed, Feb 12, 2025 at 12:04:26PM +0100, Fiona Ebner wrote:
> 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.
> 
> Since the code currently doesn't use blessed config objects, special
> dispatching is needed to potentially defer to the new child class in
> the write_config() method.
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes in v3:
> * use dedicated class
> 
>  PVE/Makefile              |  1 +
>  PVE/QemuConfig.pm         | 15 +++++++++++++++
>  PVE/QemuConfig/Makefile   |  5 +++++
>  PVE/QemuConfig/NoWrite.pm | 25 +++++++++++++++++++++++++
>  4 files changed, 46 insertions(+)
>  create mode 100644 PVE/QemuConfig/Makefile
>  create mode 100644 PVE/QemuConfig/NoWrite.pm
> 
> diff --git a/PVE/Makefile b/PVE/Makefile
> index dc173681..440fbe77 100644
> --- a/PVE/Makefile
> +++ b/PVE/Makefile
> @@ -11,4 +11,5 @@ install:
>  	$(MAKE) -C VZDump install
>  	$(MAKE) -C API2 install
>  	$(MAKE) -C CLI install
> +	$(MAKE) -C QemuConfig install
>  	$(MAKE) -C QemuServer install
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index ffdf9f03..b60cc398 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -3,6 +3,8 @@ package PVE::QemuConfig;
>  use strict;
>  use warnings;
>  
> +use Scalar::Util qw(blessed);
> +
>  use PVE::AbstractConfig;
>  use PVE::INotify;
>  use PVE::JSONSchema;
> @@ -561,6 +563,19 @@ sub get_derived_property {
>      }
>  }
>  
> +sub write_config {
> +    my ($class, $vmid, $conf) = @_;
> +
> +    # Dispatch to class the object was blessed with if caller invoked the method via the
> +    # 'PVE::QemuConfig' class name explicitly. This is hack, but the code currently doesn't
> +    # generally use blessed config objects. Safeguard against infinite recursion.
> +    if (blessed($conf) && !blessed($class)) {
> +	return $conf->write_config($vmid, $conf);
> +    }
> +
> +    return $class->SUPER::write_config($vmid, $conf);
> +}
> +
>  # END implemented abstract methods from PVE::AbstractConfig
>  
>  sub has_cloudinit {
> diff --git a/PVE/QemuConfig/Makefile b/PVE/QemuConfig/Makefile
> new file mode 100644
> index 00000000..c3a0d277
> --- /dev/null
> +++ b/PVE/QemuConfig/Makefile
> @@ -0,0 +1,5 @@
> +SOURCES=NoWrite.pm
> +
> +.PHONY: install
> +install: ${SOURCES}
> +	for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/QemuConfig/$$i; done
> diff --git a/PVE/QemuConfig/NoWrite.pm b/PVE/QemuConfig/NoWrite.pm
> new file mode 100644
> index 00000000..f697506f
> --- /dev/null
> +++ b/PVE/QemuConfig/NoWrite.pm
> @@ -0,0 +1,25 @@
> +package PVE::QemuConfig::NoWrite;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::RESTEnvironment qw(log_warn);
> +
> +use base qw(PVE::QemuConfig);
> +
> +sub new {
> +    my ($class, $conf) = @_;
> +
> +    bless($conf, $class);

^ This blesses the original reference.
In the next patch it is used as:

    $newconf = NoWrite->new($newconf);

which *suggests* that the original stays untouched - which is not the
case.

Generally, I'd recommend such methods to do a shallow copy for the sake
of clarity (simply start with `$conf = {%$conf};`).

Otherwise the following code counter-intuitively prints
'PVE::QemuConfig::NoWrite'.

    my $original = { ... };
    my $blessed = NoWrite->new($original);
    print(ref($original));

> +
> +    return $conf;
> +}
> +
> +sub write_config {
> +    my ($class, $vmid, $conf) = @_;
> +
> +    log_warn("refusing to write temporary configuration");
> +    return;
> +}
> +
> +1;
> -- 
> 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 0/2] follow-ups for "v3 fix #6007: fix PBS template backup for certain configurations"

[Fabian Grünbichler]

two patches as suggestions for follow-ups after some off-list discussion

Fabian Grünbichler (2):
  config: revamp NoWrite interface
  config: make attempts at writing out NoWrite configs fatal

 PVE/QemuConfig/NoWrite.pm | 7 ++-----
 PVE/QemuServer.pm         | 4 +++-
 2 files changed, 5 insertions(+), 6 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/2] config: revamp NoWrite interface

[Fabian Grünbichler]

instead of blessing a passed-in config and returning it, explicitly only bless
without returning to make it more obvious to callers that this affects the
argument.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 PVE/QemuConfig/NoWrite.pm | 4 +---
 PVE/QemuServer.pm         | 4 +++-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/PVE/QemuConfig/NoWrite.pm b/PVE/QemuConfig/NoWrite.pm
index f697506f..02e5f158 100644
--- a/PVE/QemuConfig/NoWrite.pm
+++ b/PVE/QemuConfig/NoWrite.pm
@@ -7,12 +7,10 @@ use PVE::RESTEnvironment qw(log_warn);
 
 use base qw(PVE::QemuConfig);
 
-sub new {
+sub mark_config {
     my ($class, $conf) = @_;
 
     bless($conf, $class);
-
-    return $conf;
 }
 
 sub write_config {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b23b79e1..9d06ac8b 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3398,7 +3398,6 @@ sub config_to_command {
 	    bios => $conf->{bios}, # so efidisk gets included if it exists
 	    name => $conf->{name}, # so it's correct in the process list
 	};
-	$newconf = PVE::QemuConfig::NoWrite->new($newconf);
 
 	# copy all disks over
 	for my $device (PVE::QemuServer::Drive::valid_drive_names()) {
@@ -3407,6 +3406,9 @@ sub config_to_command {
 
 	# remaining configs stay default
 
+	# mark config to prevent writing it out
+	PVE::QemuConfig::NoWrite->mark_config($newconf);
+
 	$conf = $newconf;
     }
 
-- 
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/2] config: make attempts at writing out NoWrite configs fatal

[Fabian Grünbichler]

attempting to write such a config is already a bug, and execution should not
continue in this case. very often a write of the config will be followed by
reloading it, expecting changes to be persisted and possibly missing
re-checking of the original reason for marking the config as NoWrite.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
this might be a bit opinionated, but I'd rather err on the side of
caution/aborting here..

 PVE/QemuConfig/NoWrite.pm | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/PVE/QemuConfig/NoWrite.pm b/PVE/QemuConfig/NoWrite.pm
index 02e5f158..53a34990 100644
--- a/PVE/QemuConfig/NoWrite.pm
+++ b/PVE/QemuConfig/NoWrite.pm
@@ -16,8 +16,7 @@ sub mark_config {
 sub write_config {
     my ($class, $vmid, $conf) = @_;
 
-    log_warn("refusing to write temporary configuration");
-    return;
+    die("refusing to write temporary configuration\n");
 }
 
 1;
-- 
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 0/2] follow-ups for "v3 fix #6007: fix PBS template backup for certain configurations"

[Fiona Ebner]

Am 18.02.25 um 14:08 schrieb Fabian Grünbichler:
> two patches as suggestions for follow-ups after some off-list discussion

Both sound sensible to me :)

> 
> Fabian Grünbichler (2):
>   config: revamp NoWrite interface
>   config: make attempts at writing out NoWrite configs fatal
> 
>  PVE/QemuConfig/NoWrite.pm | 7 ++-----
>  PVE/QemuServer.pm         | 4 +++-
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>


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

[pve-devel] [PATCH qemu-server v3 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 special QemuConfig::NoWrite class 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>
---

Changes in v3:
* use blessed object instead of config data flag.

 PVE/QemuServer.pm | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 808c0e1c..c1fbd468 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -49,6 +49,7 @@ use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_for
 
 use PVE::QMPClient;
 use PVE::QemuConfig;
+use PVE::QemuConfig::NoWrite;
 use PVE::QemuServer::Helpers qw(config_aware_timeout min_version kvm_user_version windows_version);
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CGroup;
@@ -3397,6 +3398,7 @@ sub config_to_command {
 	    bios => $conf->{bios}, # so efidisk gets included if it exists
 	    name => $conf->{name}, # so it's correct in the process list
 	};
+	$newconf = PVE::QemuConfig::NoWrite->new($newconf);
 
 	# copy all disks over
 	for my $device (PVE::QemuServer::Drive::valid_drive_names()) {
@@ -4017,7 +4019,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 +5615,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

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

[Fabian Grünbichler]

including my follow-ups with Fiona's R-B

On February 12, 2025 12:04 pm, Fiona Ebner wrote:
> Changes in v3:
> * actually decouple config data and control flow by using a blessed object
> 
> Changes in v2:
> * add comment about decoupling config data and metadata
> 
> Fiona Ebner (3):
>   backup: also restore VM power state for early failures
>   config: add special class that prevents writing the configuration
>   fix #6007: template backup: use minimized configuration for handling
>     the full vm start
> 
>  PVE/Makefile              |  1 +
>  PVE/QemuConfig.pm         | 15 +++++++++++++++
>  PVE/QemuConfig/Makefile   |  5 +++++
>  PVE/QemuConfig/NoWrite.pm | 25 +++++++++++++++++++++++++
>  PVE/QemuServer.pm         | 17 ++++++++++++++---
>  PVE/VZDump/QemuServer.pm  | 12 ++++++------
>  6 files changed, 66 insertions(+), 9 deletions(-)
>  create mode 100644 PVE/QemuConfig/Makefile
>  create mode 100644 PVE/QemuConfig/NoWrite.pm
> 
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> 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