[pve-devel] [PATCH-SERIES guest-common/manager/qemu-server/container 00/12] backup/restore: improve missing drive and owned CD-ROM handling

[pve-devel] [PATCH-SERIES guest-common/manager/qemu-server/container 00/12] backup/restore: improve missing drive and owned CD-ROM handling

[Fiona Ebner]

Missing drive handling during restore:

Currently, the configuration line for a drive that is marked for
backup (i.e. 'backup' flag not explicitly set to 0), but missing from
the restore map will be added verbatim to the restored configuration.

As reported in the community forum [0], this can cause issues with a
VM with a configured EFI disk, but not using OVMF BIOS. In such a
case, the EFI disk is not backed up, see the get_backup_volumes()
helper in QemuConfig.pm.

Writing out references to non-existent volumes to the restored config
will lead to issues starting the VM. Write such references as comments
instead, like is already done for volumes explicitly excluded from
backup. But add a warning since this is not expected, and info for the
EFI disk case.


Owned CD-ROM handling:

Currently, all volumes with a 'media=cdrom' marker are excluded from
backup. For owned volumes, this is very surprising. Treat owned CD-ROM
volumes like other volumes and consider the 'backup' marker instead.

Cloud-init volumes are owned, but still skipped, because they are
generated from the configuration/snippets.

Add a reminder to start removing owned CD-ROMs with PVE 9 when
destroying a VM.


The first 5 qemu-server patches are improvements that make sense even
if we don't go for the rest. The get_backup_volumes() patches belong
together and are a prerequisite for qemu-server patch "backup: include
owned CD-ROM volumes".

No hard dependency bumps, but optionally pve-manager ->
pve-guest-common -> qemu-server,pve-container for the
get_backup_volumes() signature change and to make the change from
"backup: include owned CD-ROM volumes" take effect.


guest-common:

Fiona Ebner (1):
  abstract config: make get_backup_volumes() method more flexible

 src/PVE/AbstractConfig.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


manager:

Fiona Ebner (1):
  api: backup: adapt to new get_backup_volumes() signature

 PVE/API2/Backup.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


qemu-server:

Fiona Ebner (9):
  cloudinit: avoid undefined value warning when no searchdomain is
    defined
  drive: move vm_is_volid_owner() and try_deallocate_drive() helpers to
    Drive module
  test: restore config: use diff to make failure human-readable
  test: restore config: add config with missing disks
  test: restore config: test behavior for disks with absolute paths
  backup restore: improve missing drive handling
  backup: adapt to new get_backup_volumes() signature
  backup: include owned CD-ROM volumes
  destroy vm: add reminder to also remove owned CD-ROMs starting with
    PVE 9

 PVE/API2/Qemu.pm                      |  8 +++-
 PVE/QemuConfig.pm                     | 12 +++++-
 PVE/QemuServer.pm                     | 55 ++++++++-------------------
 PVE/QemuServer/Cloudinit.pm           |  2 +-
 PVE/QemuServer/Drive.pm               | 38 ++++++++++++++++++
 PVE/VZDump/QemuServer.pm              |  2 +-
 test/restore-config-expected/110.conf | 16 ++++++++
 test/restore-config-expected/140.conf | 17 +++++++++
 test/restore-config-input/110.conf    | 18 +++++++++
 test/restore-config-input/140.conf    | 18 +++++++++
 test/run_qemu_restore_config_tests.pl | 13 +++++--
 11 files changed, 149 insertions(+), 50 deletions(-)
 create mode 100644 test/restore-config-expected/110.conf
 create mode 100644 test/restore-config-expected/140.conf
 create mode 100644 test/restore-config-input/110.conf
 create mode 100644 test/restore-config-input/140.conf


container:

Fiona Ebner (1):
  backup: adapt to new get_backup_volumes() signature

 src/PVE/LXC/Config.pm | 2 +-
 src/PVE/VZDump/LXC.pm | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)


Summary over all repositories:
  15 files changed, 154 insertions(+), 55 deletions(-)

-- 
Generated by git-murpp 0.5.0


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

[pve-devel] [PATCH guest-common 01/12] abstract config: make get_backup_volumes() method more flexible

[Fiona Ebner]

Add the VM ID to the parameters for the get_backup_volumes() method.
Like this, implementations can distinguish between owned and non-owned
volumes.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/AbstractConfig.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
index 3d4fcbb..c307b71 100644
--- a/src/PVE/AbstractConfig.pm
+++ b/src/PVE/AbstractConfig.pm
@@ -555,7 +555,7 @@ sub get_replicatable_volumes {
 #   },
 # ]
 sub get_backup_volumes {
-    my ($class, $conf) = @_;
+    my ($class, $conf, $vmid) = @_;
 
     die "implement me - abstract method\n";
 }
-- 
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 manager 02/12] api: backup: adapt to new get_backup_volumes() signature

[Fiona Ebner]

See commit "abstract config: make get_backup_volumes() method more
flexible" in the pve-guest-common repository.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Backup.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index f37e2393..0f6a3a8e 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -695,11 +695,11 @@ __PACKAGE__->register_method({
 
 	    if ($type eq 'qemu') {
 		$conf = PVE::QemuConfig->load_config($vmid, $node);
-		$volumes = PVE::QemuConfig->get_backup_volumes($conf);
+		$volumes = PVE::QemuConfig->get_backup_volumes($conf, $vmid);
 		$name = $conf->{name};
 	    } elsif ($type eq 'lxc') {
 		$conf = PVE::LXC::Config->load_config($vmid, $node);
-		$volumes = PVE::LXC::Config->get_backup_volumes($conf);
+		$volumes = PVE::LXC::Config->get_backup_volumes($conf, $vmid);
 		$name = $conf->{hostname};
 	    } else {
 		die "VMID $vmid is neither Qemu nor LXC guest\n";
-- 
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 03/12] cloudinit: avoid undefined value warning when no searchdomain is defined

[Fiona Ebner]

No functional change intended.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer/Cloudinit.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index f1143aeb..5c6eb153 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -102,7 +102,7 @@ sub get_dns_conf {
     my $host_resolv_conf = PVE::INotify::read_file('resolvconf');
 
     my $searchdomains = [
-	split(/\s+/, $conf->{searchdomain} // $host_resolv_conf->{search})
+	split(/\s+/, $conf->{searchdomain} // $host_resolv_conf->{search} // ''),
     ];
 
     my $nameserver = $conf->{nameserver};
-- 
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 04/12] drive: move vm_is_volid_owner() and try_deallocate_drive() helpers to Drive module

[Fiona Ebner]

While at it, avoid making the if conditions/lines for the
try_deallocate_drive() callers even longer, and bring the code in-line
with the style guide.

No functional change intended.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm        |  8 ++++++--
 PVE/QemuServer.pm       | 42 +++--------------------------------------
 PVE/QemuServer/Drive.pm | 38 +++++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 41 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 5ac61aa5..fc0df860 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1972,13 +1972,17 @@ my $update_vm_api  = sub {
 		    my $drive = PVE::QemuServer::parse_drive($opt, $val);
 		    PVE::QemuConfig->check_protection($conf, "can't remove unused disk '$drive->{file}'");
 		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
-		    if (PVE::QemuServer::try_deallocate_drive($storecfg, $vmid, $conf, $opt, $drive, $rpcenv, $authuser)) {
+		    my $remove_from_config = PVE::QemuServer::Drive::try_deallocate_drive(
+			$storecfg, $vmid, $conf, $opt, $drive, $rpcenv, $authuser);
+		    if ($remove_from_config) {
 			delete $conf->{$opt};
 			PVE::QemuConfig->write_config($vmid, $conf);
 		    }
 		} elsif ($opt eq 'vmstate') {
 		    PVE::QemuConfig->check_protection($conf, "can't remove vmstate '$val'");
-		    if (PVE::QemuServer::try_deallocate_drive($storecfg, $vmid, $conf, $opt, { file => $val }, $rpcenv, $authuser, 1)) {
+		    my $remove_from_config = PVE::QemuServer::Drive::try_deallocate_drive(
+			$storecfg, $vmid, $conf, $opt, { file => $val }, $rpcenv, $authuser, 1);
+		    if ($remove_from_config) {
 			delete $conf->{$opt};
 			PVE::QemuConfig->write_config($vmid, $conf);
 		    }
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 9d06ac8b..454ee64a 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1830,20 +1830,6 @@ sub add_random_macs {
     }
 }
 
-sub vm_is_volid_owner {
-    my ($storecfg, $vmid, $volid) = @_;
-
-    if ($volid !~  m|^/|) {
-	my ($path, $owner);
-	eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); };
-	if ($owner && ($owner == $vmid)) {
-	    return 1;
-	}
-    }
-
-    return;
-}
-
 sub vmconfig_register_unused_drive {
     my ($storecfg, $vmid, $conf, $drive) = @_;
 
@@ -1853,7 +1839,7 @@ sub vmconfig_register_unused_drive {
 	delete $conf->{cloudinit};
     } elsif (!drive_is_cdrom($drive)) {
 	my $volid = $drive->{file};
-	if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
+	if (PVE::QemuServer::Drive::vm_is_volid_owner($storecfg, $vmid, $volid)) {
 	    PVE::QemuConfig->add_unused_volume($conf, $volid, $vmid);
 	}
     }
@@ -5027,29 +5013,6 @@ sub vmconfig_hotplug_pending {
     }
 }
 
-sub try_deallocate_drive {
-    my ($storecfg, $vmid, $conf, $key, $drive, $rpcenv, $authuser, $force) = @_;
-
-    if (($force || $key =~ /^unused/) && !drive_is_cdrom($drive, 1)) {
-	my $volid = $drive->{file};
-	if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
-	    my $sid = PVE::Storage::parse_volume_id($volid);
-	    $rpcenv->check($authuser, "/storage/$sid", ['Datastore.AllocateSpace']);
-
-	    # check if the disk is really unused
-	    die "unable to delete '$volid' - volume is still in use (snapshot?)\n"
-		if PVE::QemuServer::Drive::is_volume_in_use($storecfg, $conf, $key, $volid);
-	    PVE::Storage::vdisk_free($storecfg, $volid);
-	    return 1;
-	} else {
-	    # If vm is not owner of this disk remove from config
-	    return 1;
-	}
-    }
-
-    return;
-}
-
 sub vmconfig_delete_or_detach_drive {
     my ($vmid, $storecfg, $conf, $opt, $force) = @_;
 
@@ -5060,7 +5023,8 @@ sub vmconfig_delete_or_detach_drive {
 
     if ($force) {
 	$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
-	try_deallocate_drive($storecfg, $vmid, $conf, $opt, $drive, $rpcenv, $authuser, $force);
+	PVE::QemuServer::Drive::try_deallocate_drive(
+	    $storecfg, $vmid, $conf, $opt, $drive, $rpcenv, $authuser, $force);
     } else {
 	vmconfig_register_unused_drive($storecfg, $vmid, $conf, $drive);
     }
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 1041c1dd..8ff44641 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -998,4 +998,42 @@ sub get_scsi_device_type {
 
     return $devicetype;
 }
+
+sub vm_is_volid_owner {
+    my ($storecfg, $vmid, $volid) = @_;
+
+    if ($volid !~  m|^/|) {
+	my ($path, $owner);
+	eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); };
+	if ($owner && ($owner == $vmid)) {
+	    return 1;
+	}
+    }
+
+    return;
+}
+
+sub try_deallocate_drive {
+    my ($storecfg, $vmid, $conf, $key, $drive, $rpcenv, $authuser, $force) = @_;
+
+    if (($force || $key =~ /^unused/) && !drive_is_cdrom($drive, 1)) {
+	my $volid = $drive->{file};
+	if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
+	    my $sid = PVE::Storage::parse_volume_id($volid);
+	    $rpcenv->check($authuser, "/storage/$sid", ['Datastore.AllocateSpace']);
+
+	    # check if the disk is really unused
+	    die "unable to delete '$volid' - volume is still in use (snapshot?)\n"
+		if is_volume_in_use($storecfg, $conf, $key, $volid);
+	    PVE::Storage::vdisk_free($storecfg, $volid);
+	    return 1;
+	} else {
+	    # If vm is not owner of this disk remove from config
+	    return 1;
+	}
+    }
+
+    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 05/12] test: restore config: use diff to make failure human-readable

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 test/run_qemu_restore_config_tests.pl | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/test/run_qemu_restore_config_tests.pl b/test/run_qemu_restore_config_tests.pl
index 1e1e8072..231975c6 100755
--- a/test/run_qemu_restore_config_tests.pl
+++ b/test/run_qemu_restore_config_tests.pl
@@ -80,9 +80,14 @@ dir_glob_foreach('./restore-config-input', '[0-9]+.conf', sub {
 	);
     }
 
-    my $expected = file_get_contents("${EXPECTED_DIR}/${file}");
-
-    is_deeply($got, $expected, $file);
+    my $cmd = ['diff', '-u', "${EXPECTED_DIR}/${file}", '-'];
+    eval { PVE::Tools::run_command($cmd, input => $got); };
+    if (my $err = $@) {
+	note($err);
+	fail($file);
+    } else {
+	pass($file);
+    }
 });
 
 done_testing();
-- 
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 06/12] test: restore config: add config with missing disks

[Fiona Ebner]

Record the behavior for missing disks in the backup.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 test/restore-config-expected/140.conf | 17 +++++++++++++++++
 test/restore-config-input/140.conf    | 18 ++++++++++++++++++
 test/run_qemu_restore_config_tests.pl |  2 +-
 3 files changed, 36 insertions(+), 1 deletion(-)
 create mode 100644 test/restore-config-expected/140.conf
 create mode 100644 test/restore-config-input/140.conf

diff --git a/test/restore-config-expected/140.conf b/test/restore-config-expected/140.conf
new file mode 100644
index 00000000..af1a4d59
--- /dev/null
+++ b/test/restore-config-expected/140.conf
@@ -0,0 +1,17 @@
+# missing disks in backup
+bios: seabios
+boot: order=scsi0;ide2;net0
+cores: 1
+efidisk0: mydir:140/vm-140-disk-0.qcow2,size=128K
+ide2: local:iso/debian-10.6.0-amd64-netinst.iso,media=cdrom
+memory: 2048
+name: eficloneclone
+net0: virtio=7A:6C:A5:8B:11:93,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:140/vm-140-disk-0.raw,size=4G
+scsi1: rbdkvm:vm-140-disk-2,size=1G
+scsihw: virtio-scsi-pci
+smbios1: uuid=21a7e7bc-3cd2-4232-a009-a41f4ee992ae
+sockets: 1
+vmgenid: 0
diff --git a/test/restore-config-input/140.conf b/test/restore-config-input/140.conf
new file mode 100644
index 00000000..4a6e191c
--- /dev/null
+++ b/test/restore-config-input/140.conf
@@ -0,0 +1,18 @@
+# missing disks in backup
+bios: seabios
+boot: order=scsi0;ide2;net0
+cores: 1
+efidisk0: mydir:140/vm-140-disk-0.qcow2,size=128K
+ide2: local:iso/debian-10.6.0-amd64-netinst.iso,media=cdrom
+memory: 2048
+name: eficloneclone
+net0: virtio=7A:6C:A5:8B:11:93,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: rbdkvm:vm-140-disk-1,size=4G
+scsi1: rbdkvm:vm-140-disk-2,size=1G
+scsihw: virtio-scsi-pci
+smbios1: uuid=21a7e7bc-3cd2-4232-a009-a41f4ee992ae
+sockets: 1
+vmgenid: 0
+#qmdump#map:scsi0:drive-scsi0:rbdkvm::
diff --git a/test/run_qemu_restore_config_tests.pl b/test/run_qemu_restore_config_tests.pl
index 231975c6..245258c8 100755
--- a/test/run_qemu_restore_config_tests.pl
+++ b/test/run_qemu_restore_config_tests.pl
@@ -25,7 +25,7 @@ $pve_cluster_module->mock(
 );
 
 # NOTE update when you add/remove tests
-plan tests => 4;
+plan tests => 5;
 
 my $cfs_mock = Test::MockModule->new("PVE::Cluster");
 $cfs_mock->mock(
-- 
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 07/12] test: restore config: test behavior for disks with absolute paths

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 test/restore-config-expected/110.conf | 16 ++++++++++++++++
 test/restore-config-input/110.conf    | 18 ++++++++++++++++++
 test/run_qemu_restore_config_tests.pl |  2 +-
 3 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 test/restore-config-expected/110.conf
 create mode 100644 test/restore-config-input/110.conf

diff --git a/test/restore-config-expected/110.conf b/test/restore-config-expected/110.conf
new file mode 100644
index 00000000..fcbeaa5c
--- /dev/null
+++ b/test/restore-config-expected/110.conf
@@ -0,0 +1,16 @@
+# drives with absolute paths
+boot: order=scsi0;ide2;net0
+cores: 1
+cpu: x86-64-v2-AES
+ide2: none,media=cdrom
+memory: 2048
+meta: creation-qemu=9.0.2,ctime=1726234504
+net0: virtio=BC:24:11:B1:BF:4B,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:110/vm-110-disk-0.raw,aio=io_uring,iothread=1,size=1G
+scsi2: target:110/vm-110-disk-1.raw,size=1G
+#scsi3: /dev/sdj,backup=0,size=1G
+scsihw: virtio-scsi-single
+smbios1: uuid=ebc66780-e174-4abc-8860-afaaeab758d4
+sockets: 1
diff --git a/test/restore-config-input/110.conf b/test/restore-config-input/110.conf
new file mode 100644
index 00000000..8c122fe6
--- /dev/null
+++ b/test/restore-config-input/110.conf
@@ -0,0 +1,18 @@
+# drives with absolute paths
+boot: order=scsi0;ide2;net0
+cores: 1
+cpu: x86-64-v2-AES
+ide2: none,media=cdrom
+memory: 2048
+meta: creation-qemu=9.0.2,ctime=1726234504
+net0: virtio=BC:24:11:B1:BF:4B,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: zfs:vm-110-disk-0,aio=io_uring,iothread=1,size=1G
+scsi2: /dev/sdm,size=1G
+scsi3: /dev/sdj,backup=0,size=1G
+scsihw: virtio-scsi-single
+smbios1: uuid=ebc66780-e174-4abc-8860-afaaeab758d4
+sockets: 1
+#qmdump#map:scsi0:drive-scsi0:zfs:raw:
+#qmdump#map:scsi2:drive-scsi2::raw:
diff --git a/test/run_qemu_restore_config_tests.pl b/test/run_qemu_restore_config_tests.pl
index 245258c8..f2d220fc 100755
--- a/test/run_qemu_restore_config_tests.pl
+++ b/test/run_qemu_restore_config_tests.pl
@@ -25,7 +25,7 @@ $pve_cluster_module->mock(
 );
 
 # NOTE update when you add/remove tests
-plan tests => 5;
+plan tests => 6;
 
 my $cfs_mock = Test::MockModule->new("PVE::Cluster");
 $cfs_mock->mock(
-- 
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 08/12] backup restore: improve missing drive handling

[Fiona Ebner]

Currently, the configuration line for a drive that is marked for
backup (i.e. 'backup' flag not explicitly set to 0), but missing from
the restore map will be added verbatim to the restored configuration.

As reported in the community forum [0], this can cause issues with a
VM with a configured EFI disk, but not using OVMF BIOS. In such a
case, the EFI disk is not backed up, see the get_backup_volumes()
helper in QemuConfig.pm.

Writing out references to non-existent volumes to the restored config
will lead to issues starting the VM. Write such references as comments
instead, like is already done for volumes explicitly excluded from
backup. But add a warning since this is not expected, and info for the
EFI disk case.

Note that drives using absolute paths are either also backed-up and
will be allocated as new volumes during restore or explicitly marked
as excluded.

Continue preserving configuration lines for CD-ROMs. No CD-ROMs were
included in backups before commit "backup: include owned CD-ROM
volumes" and non-owned CD-ROM and cloud-init drives continue to be
excluded. Skipping owned CD-ROMs that are missing from the backup here
too (and maybe even those owned by another VM?) could be done in the
future, but might be considered a breaking change. Currently, the old
VM ID is also not readily available for such a check.

[0]: https://forum.proxmox.com/threads/160483/

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm                     | 12 +++++++++++-
 test/restore-config-expected/140.conf |  4 ++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 454ee64a..f937b1d7 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6714,6 +6714,7 @@ sub restore_update_config_line {
 	$res .= "$id: $netstr\n";
     } elsif ($line =~ m/^((ide|scsi|virtio|sata|efidisk|tpmstate)\d+):\s*(\S+)\s*$/) {
 	my $virtdev = $1;
+	my $interface = $2;
 	my $value = $3;
 	my $di = parse_drive($virtdev, $value);
 	if (defined($di->{backup}) && !$di->{backup}) {
@@ -6723,8 +6724,17 @@ sub restore_update_config_line {
 	    $di->{file} = $map->{$virtdev};
 	    $value = print_drive($di);
 	    $res .= "$virtdev: $value\n";
-	} else {
+	} elsif (PVE::QemuServer::Drive::drive_is_cdrom($di)) {
+	    # TODO PVE 9 - skip owned (either by this or also other VM?) CD-ROMs that are not in
+	    # backup (except cloud-init)
 	    $res .= $line;
+	} else {
+	    if ($interface eq 'efidisk') {
+		print "$virtdev: not part of the backup - SeaBIOS configured?\n";
+	    } else {
+		log_warn("$virtdev: not part of the backup");
+	    }
+	    $res .= "#$line";
 	}
     } elsif (($line =~ m/^vmgenid: (.*)/)) {
 	my $vmgenid = $1;
diff --git a/test/restore-config-expected/140.conf b/test/restore-config-expected/140.conf
index af1a4d59..33ba37a7 100644
--- a/test/restore-config-expected/140.conf
+++ b/test/restore-config-expected/140.conf
@@ -2,7 +2,7 @@
 bios: seabios
 boot: order=scsi0;ide2;net0
 cores: 1
-efidisk0: mydir:140/vm-140-disk-0.qcow2,size=128K
+#efidisk0: mydir:140/vm-140-disk-0.qcow2,size=128K
 ide2: local:iso/debian-10.6.0-amd64-netinst.iso,media=cdrom
 memory: 2048
 name: eficloneclone
@@ -10,7 +10,7 @@ net0: virtio=7A:6C:A5:8B:11:93,bridge=vmbr0,firewall=1
 numa: 0
 ostype: l26
 scsi0: target:140/vm-140-disk-0.raw,size=4G
-scsi1: rbdkvm:vm-140-disk-2,size=1G
+#scsi1: rbdkvm:vm-140-disk-2,size=1G
 scsihw: virtio-scsi-pci
 smbios1: uuid=21a7e7bc-3cd2-4232-a009-a41f4ee992ae
 sockets: 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 09/12] backup: adapt to new get_backup_volumes() signature

[Fiona Ebner]

See commit "abstract config: make get_backup_volumes() method more
flexible" in the pve-guest-common repository.

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

diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index b60cc398..43967b43 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -170,7 +170,7 @@ sub get_replicatable_volumes {
 }
 
 sub get_backup_volumes {
-    my ($class, $conf) = @_;
+    my ($class, $conf, $vmid) = @_;
 
     my $return_volumes = [];
 
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 3238e344..d2e40d7c 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -88,7 +88,7 @@ sub prepare {
 
     my $vollist = [];
     my $drivehash = {};
-    my $backup_volumes = PVE::QemuConfig->get_backup_volumes($conf);
+    my $backup_volumes = PVE::QemuConfig->get_backup_volumes($conf, $vmid);
 
     foreach my $volume (@{$backup_volumes}) {
 	my $name = $volume->{key};
-- 
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 10/12] backup: include owned CD-ROM volumes

[Fiona Ebner]

Currently, all volumes with a 'media=cdrom' marker are excluded from
backup. For owned volumes, this is very surprising. Treat owned CD-ROM
volumes like other volumes and consider the 'backup' marker instead.

Cloud-init volumes are owned, but still skipped, because they are
generated from the configuration/snippets.

Stay compatible with old callers avoiding the need for a versioned
breaks.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuConfig.pm | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 43967b43..92b21259 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -172,12 +172,20 @@ sub get_replicatable_volumes {
 sub get_backup_volumes {
     my ($class, $conf, $vmid) = @_;
 
+    my $storecfg = PVE::Storage::config();
+
     my $return_volumes = [];
 
     my $test_volume = sub {
 	my ($key, $drive) = @_;
 
-	return if PVE::QemuServer::drive_is_cdrom($drive);
+	if (PVE::QemuServer::Drive::drive_is_cdrom($drive)) {
+	    return if !defined($vmid); # for compat - TODO PVE 9 - remove
+	    my $owned = PVE::QemuServer::Drive::vm_is_volid_owner($storecfg, $vmid, $drive->{file});
+	    my $is_cloudinit = PVE::QemuServer::Drive::drive_is_cloudinit($drive);
+
+	    return if !$owned || $is_cloudinit;
+	}
 
 	my $included = $drive->{backup} // 1;
 	my $reason = "backup=";
-- 
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 11/12] destroy vm: add reminder to also remove owned CD-ROMs starting with PVE 9

[Fiona Ebner]

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

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index f937b1d7..98cf2c1c 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2131,6 +2131,7 @@ sub destroy_vm {
     my $volids = {};
     my $remove_owned_drive = sub {
 	my ($ds, $drive) = @_;
+	# TODO PVE 9 - destroy all owned CD-ROMs, not just cloud-init (i.e. vm_is_volid_owner())
  	return if drive_is_cdrom($drive, 1);
 
 	my $volid = $drive->{file};
-- 
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 container 12/12] backup: adapt to new get_backup_volumes() signature

[Fiona Ebner]

See commit "abstract config: make get_backup_volumes() method more
flexible" in the pve-guest-common repository.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/LXC/Config.pm | 2 +-
 src/PVE/VZDump/LXC.pm | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 0740e8c..6fcbaa0 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1852,7 +1852,7 @@ sub get_replicatable_volumes {
 }
 
 sub get_backup_volumes {
-    my ($class, $conf) = @_;
+    my ($class, $conf, $vmid) = @_;
 
     my $return_volumes = [];
 
diff --git a/src/PVE/VZDump/LXC.pm b/src/PVE/VZDump/LXC.pm
index 9029387..cf9c383 100644
--- a/src/PVE/VZDump/LXC.pm
+++ b/src/PVE/VZDump/LXC.pm
@@ -129,7 +129,7 @@ sub prepare {
 
     my $volids = $task->{volids} = [];
 
-    my $backup_volumes = PVE::LXC::Config->get_backup_volumes($conf);
+    my $backup_volumes = PVE::LXC::Config->get_backup_volumes($conf, $vmid);
 
     for my $volume (@{$backup_volumes}) {
 	my $name = $volume->{key};
-- 
2.39.5



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