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

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

[Fiona Ebner]

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

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        | 14 ++++++++++++++
 PVE/QemuServer.pm        | 16 +++++++++++++---
 PVE/VZDump/QemuServer.pm | 12 ++++++------
 3 files changed, 33 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 v2 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 v2 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>
---

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

 PVE/QemuConfig.pm | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index ffdf9f03..51e027b4 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,19 @@ sub get_derived_property {
     }
 }
 
+sub write_config {
+    my ($class, $vmid, $conf) = @_;
+
+    # FIXME avoid this mixing of actual config data and metadata - for example, could make the
+    # configuration a blessed object with appropriate methods.
+    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

[pve-devel] [PATCH v2 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