[pve-devel] [PATCH-SERIES v2 manager] Make backup with IDs for non-existent guests visibly fail

[pve-devel] [PATCH-SERIES v2 manager] Make backup with IDs for non-existent guests visibly fail

[Fabian Ebner]

#1 and #2 are just cleanups

#3 and #4 make the necessary changes for the improved behavior
by ensuring that exec_backup_task will cleanly fail when there
is no plugin specified, and then including the orphaned IDs
without assigning them a plugin. This is closer to the behavior
of PVE 6.0 and ensures that the backup task is marked as failed,
and the mail notification includes an error for the orphaned IDs.

#5 ensures that the IDs are still numerically ordered when there
are non-existing guests

#6 and #7 are minor improvements and can be applied independently

#8 is an RFC and changes the data structure returned by get_included_guests


Changes from v1:
    * everything, as the approach is different


Fabian Ebner (8):
  remove unused variable
  remove out-of-date comment
  only use plugin after truthiness check
  backup: include IDs for non-existent guests
  order guest IDs numerically in exec_backup
  sort the skip list numerically
  simplify get_included_vmids function
  don't group by node in get_included_guests

 PVE/API2/Backup.pm                 |  23 ++--
 PVE/API2/BackupInfo.pm             |  18 +---
 PVE/API2/VZDump.pm                 |  19 +++-
 PVE/VZDump.pm                      |  68 +++++++-----
 test/vzdump_guest_included_test.pl | 163 ++++++++++++++++++++++++++---
 5 files changed, 212 insertions(+), 79 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH v2 manager 1/8] remove unused variable

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/VZDump.pm | 1 -
 1 file changed, 1 deletion(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 542228d6..ee4e68b5 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -1192,7 +1192,6 @@ sub stop_running_backups {
 sub get_included_guests {
     my ($job) = @_;
 
-    my $nodename = PVE::INotify::nodename();
     my $vmids = [];
     my $vmids_per_node = {};
 
-- 
2.20.1

[pve-devel] [PATCH v2 manager 2/8] remove outdated comment

[Fabian Ebner]

Commit be30864709752195926f0a06c8f0d4d11c3c3302 moved the
all/exclude logic into the single method

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 test/vzdump_guest_included_test.pl | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/test/vzdump_guest_included_test.pl b/test/vzdump_guest_included_test.pl
index a0f40a55..eb663ad3 100755
--- a/test/vzdump_guest_included_test.pl
+++ b/test/vzdump_guest_included_test.pl
@@ -1,9 +1,5 @@
 #!/usr/bin/perl
 
-# Some of the tests can only be applied once the whole include logic is moved
-# into one single method. Right now parts of it, (all, exclude)  are in the
-# PVE::VZDump->exec_backup() method.
-
 use strict;
 use warnings;
 
-- 
2.20.1

[pve-devel] [PATCH v2 manager 3/8] only use plugin after truthiness check

[Fabian Ebner]

Commit 62fc2aa9fa2eb82596f98aa014d3b0ccfc0ec542 introduced
a usage of plugin before the truthiness check for plugin.

At the moment it might not be possible to trigger this anymore,
because of the guest inclusion rework that happened later on.
But to make tasks for inexistent guest IDs visibly fail again,
this check will be necessary. Also, to get the error message in
the mail, it needs to fail inside the eval block.

Thus, keep the check in the eval block and move the block of code
using the plugin to below the check.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/VZDump.pm | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index ee4e68b5..e6082f3b 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -672,25 +672,12 @@ sub exec_backup_task {
     my $cfg = PVE::Storage::config();
     my $vmid = $task->{vmid};
     my $plugin = $task->{plugin};
-    my $vmtype = $plugin->type();
 
     $task->{backup_time} = time();
 
     my $pbs_group_name;
     my $pbs_snapshot_name;
 
-    if ($self->{opts}->{pbs}) {
-	if ($vmtype eq 'lxc') {
-	    $pbs_group_name = "ct/$vmid";
-	} elsif  ($vmtype eq 'qemu') {
-	    $pbs_group_name = "vm/$vmid";
-	} else {
-	    die "pbs backup not implemented for plugin type '$vmtype'\n";
-	}
-	my $btime = strftime("%FT%TZ", gmtime($task->{backup_time}));
-	$pbs_snapshot_name = "$pbs_group_name/$btime";
-    }
-
     my $vmstarttime = time ();
 
     my $logfd;
@@ -708,6 +695,20 @@ sub exec_backup_task {
     eval {
 	die "unable to find VM '$vmid'\n" if !$plugin;
 
+	my $vmtype = $plugin->type();
+
+	if ($self->{opts}->{pbs}) {
+	    if ($vmtype eq 'lxc') {
+		$pbs_group_name = "ct/$vmid";
+	    } elsif  ($vmtype eq 'qemu') {
+		$pbs_group_name = "vm/$vmid";
+	    } else {
+		die "pbs backup not implemented for plugin type '$vmtype'\n";
+	    }
+	    my $btime = strftime("%FT%TZ", gmtime($task->{backup_time}));
+	    $pbs_snapshot_name = "$pbs_group_name/$btime";
+	}
+
 	# for now we deny backups of a running ha managed service in *stop* mode
 	# as it interferes with the HA stack (started services should not stop).
 	if ($opts->{mode} eq 'stop' &&
-- 
2.20.1

[pve-devel] [PATCH v2 manager 4/8] backup: include IDs for non-existent guests

[Fabian Ebner]

Like this, there will be a backup task (within the big worker task)
for such IDs, which will then visibly (i.e. also visible in the
notification mail) fail with, e.g.:
unable to find VM '123'

In get_included_guests, the key '' was chosen for the orphaned IDs,
because it cannot possibly denote a nodename.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/VZDump.pm                 |  4 ++++
 PVE/VZDump.pm                      | 19 +++++++++++++------
 test/vzdump_guest_included_test.pl | 13 ++++++++++++-
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 19fa1e3b..806ac7fd 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -74,6 +74,10 @@ __PACKAGE__->register_method ({
 
 	my $local_vmids = delete $vmids_per_node->{$nodename} // [];
 
+	# include IDs for deleted guests, and visibly fail later
+	my $orphaned_vmids = delete $vmids_per_node->{''} // [];
+	push @{$local_vmids}, @{$orphaned_vmids};
+
 	my $skiplist = [ map { @$_ } values $vmids_per_node->%* ];
 
 	if($param->{stop}){
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index e6082f3b..7ff32ce2 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -1073,9 +1073,12 @@ sub exec_backup {
 
     my $vmlist = PVE::Cluster::get_vmlist();
     foreach my $vmid (@{$opts->{vmids}}) {
-	my $guest_type = $vmlist->{ids}->{$vmid}->{type};
-	my $plugin = $vzdump_plugins->{$guest_type};
-	next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ], $opts->{all});
+	my $plugin;
+	if (defined($vmlist->{ids}->{$vmid})) {
+	    my $guest_type = $vmlist->{ids}->{$vmid}->{type};
+	    $plugin = $vzdump_plugins->{$guest_type};
+	    next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ], $opts->{all});
+	}
 	push @$tasklist, {
 	    mode => $opts->{mode},
 	    plugin => $plugin,
@@ -1217,10 +1220,14 @@ sub get_included_guests {
     $vmids = check_vmids(@$vmids);
 
     for my $vmid (@$vmids) {
-	my $node = $vmlist->{ids}->{$vmid}->{node};
-	next if (defined $job->{node} && $job->{node} ne $node);
+	if (defined($vmlist->{ids}->{$vmid})) {
+	    my $node = $vmlist->{ids}->{$vmid}->{node};
+	    next if (defined $job->{node} && $job->{node} ne $node);
 
-	push @{$vmids_per_node->{$node}}, $vmid;
+	    push @{$vmids_per_node->{$node}}, $vmid;
+	} else {
+	    push @{$vmids_per_node->{''}}, $vmid;
+	}
     }
 
     return $vmids_per_node;
diff --git a/test/vzdump_guest_included_test.pl b/test/vzdump_guest_included_test.pl
index eb663ad3..157644de 100755
--- a/test/vzdump_guest_included_test.pl
+++ b/test/vzdump_guest_included_test.pl
@@ -5,7 +5,7 @@ use warnings;
 
 use lib '..';
 
-use Test::More tests => 9;
+use Test::More tests => 10;
 use Test::MockModule;
 
 use PVE::VZDump;
@@ -174,6 +174,17 @@ $addtest->('Test selected VMIDs on other nodes', {
     }
 });
 
+$addtest->('Test VMID not present in vmlist', {
+    expected => {
+	node1 => [ 100 ],
+	node2 => [ 201, 212 ],
+	'' => [ 7654 ],
+   },
+    param => {
+	vmid => '100, 201, 212, 7654',
+    }
+});
+
 
 for my $test (@{$tests}) {
     my $testname = $test->{name};
-- 
2.20.1

[pve-devel] [PATCH v2 manager 5/8] order guest IDs numerically in exec_backup

[Fabian Ebner]

The assumption that they already are sorted is no longer valid,
because of the IDs for non-existent guests.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Should also be more future-proof to do it locally.

This could be squashed into either the previous or
the following patch.

 PVE/VZDump.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 7ff32ce2..e1c26b42 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -1072,7 +1072,8 @@ sub exec_backup {
     }
 
     my $vmlist = PVE::Cluster::get_vmlist();
-    foreach my $vmid (@{$opts->{vmids}}) {
+    my $vmids = [ sort { $a <=> $b } @{$opts->{vmids}} ];
+    foreach my $vmid (@{$vmids}) {
 	my $plugin;
 	if (defined($vmlist->{ids}->{$vmid})) {
 	    my $guest_type = $vmlist->{ids}->{$vmid}->{type};
-- 
2.20.1

[pve-devel] [PATCH v2 manager 6/8] sort the skip list numerically

[Fabian Ebner]

The skip list was not always sorted if there were external IDs for multiple
external nodes.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/VZDump.pm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index e1c26b42..2f31c534 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -1060,8 +1060,11 @@ sub exec_backup {
     my $opts = $self->{opts};
 
     debugmsg ('info', "starting new backup job: $self->{cmdline}", undef, 1);
-    debugmsg ('info', "skip external VMs: " . join(', ', @{$self->{skiplist}}))
-	if scalar(@{$self->{skiplist}});
+
+    if (scalar(@{$self->{skiplist}})) {
+	my $skip_string = join(', ', sort { $a <=> $b } @{$self->{skiplist}});
+	debugmsg ('info', "skip external VMs: $skip_string");
+    }
 
     my $tasklist = [];
     my $vzdump_plugins =  {};
-- 
2.20.1

[pve-devel] [PATCH v2 manager 7/8] simplify get_included_vmids function

[Fabian Ebner]

by collecting all the guest IDs first.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/BackupInfo.pm | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/PVE/API2/BackupInfo.pm b/PVE/API2/BackupInfo.pm
index 909a5de1..4c461e59 100644
--- a/PVE/API2/BackupInfo.pm
+++ b/PVE/API2/BackupInfo.pm
@@ -17,30 +17,18 @@ use PVE::VZDump::Common;
 
 use base qw(PVE::RESTHandler);
 
-sub map_job_vmids {
-    my ($job_included_guests, $included_vmids) = @_;
-
-    for my $node_vmids (values %{$job_included_guests}) {
-	for my $vmid (@{$node_vmids}) {
-	    $included_vmids->{$vmid} = 1;
-	}
-    }
-
-    return $included_vmids;
-}
-
 sub get_included_vmids {
-    my $included_vmids = {};
     my $vzconf = cfs_read_file('vzdump.cron');
 
     my $all_jobs = $vzconf->{jobs} || [];
+    my @all_vmids = ();
 
     for my $job (@$all_jobs) {
 	my $job_included_guests = PVE::VZDump::get_included_guests($job);
-	$included_vmids = map_job_vmids($job_included_guests, $included_vmids);
+	push @all_vmids, ( map { @{$_} } values %{$job_included_guests} );
     }
 
-    return $included_vmids;
+    return { map { $_ => 1 } @all_vmids };
 }
 
 __PACKAGE__->register_method({
-- 
2.20.1

[pve-devel] [RFC/PATCH v2 manager 8/8] don't group by node in get_included_guests

[Fabian Ebner]

Seems to simplify the handling for two of the callers and not complicate it for
the remaining one.

Including the type as well avoids the need to use the vmlist again in the
included_volumes API call.

In get_included_volumes, returning {} in the else branch was made explicit.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

This is mostly a matter of taste and completely optional to the goal of the
series.

I'm not completely happy with using the '' key for the orphaned IDs and this
representation feels closer to what the callers actually need. Although, one
can argue about the caller in the vzdump API call.

And just some thought for the future: if we'd pass along the full hash to
exec_backup instead of just the IDs, the skipping of external IDs could be done
more locally.

@Aaron: Does getting rid of the grouping complicate some of the things that are
still planned with get_included_guests?

 PVE/API2/Backup.pm                 |  23 ++---
 PVE/API2/BackupInfo.pm             |   2 +-
 PVE/API2/VZDump.pm                 |  23 +++--
 PVE/VZDump.pm                      |  15 ++-
 test/vzdump_guest_included_test.pl | 152 ++++++++++++++++++++++++++---
 5 files changed, 169 insertions(+), 46 deletions(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 3b343636..63d9c261 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -422,22 +422,13 @@ __PACKAGE__->register_method({
 	}
 	raise_param_exc({ id => "No such job '$param->{id}'" }) if !$job;
 
-	my $vmlist = PVE::Cluster::get_vmlist();
-
-	my @job_vmids;
-
 	my $included_guests = PVE::VZDump::get_included_guests($job);
 
-	for my $node (keys %{$included_guests}) {
-	    my $node_vmids = $included_guests->{$node};
-	    push(@job_vmids, @{$node_vmids});
-	}
-
 	# remove VMIDs to which the user has no permission to not leak infos
 	# like the guest name
 	my @allowed_vmids = grep {
-		$rpcenv->check($user, "/vms/$_", [ 'VM.Audit' ], 1);
-	} @job_vmids;
+	    $rpcenv->check($user, "/vms/$_", [ 'VM.Audit' ], 1);
+	} keys %{$included_guests};
 
 	my $result = {
 	    children => [],
@@ -447,11 +438,11 @@ __PACKAGE__->register_method({
 
 	    my $children = [];
 
-	    # It's possible that a job has VMIDs configured that are not in
-	    # vmlist. This could be because a guest was removed but not purged.
+	    # When removing a guest without purge, the backup configuration
+	    # is not updated. Such orphaned IDs don't have a node.
 	    # Since there is no more data available we can only deliver the VMID
 	    # and no volumes.
-	    if (!defined $vmlist->{ids}->{$vmid}) {
+	    if (!defined($included_guests->{$vmid}->{node})) {
 		push(@{$result->{children}}, {
 		    id => int($vmid),
 		    type => 'unknown',
@@ -460,8 +451,8 @@ __PACKAGE__->register_method({
 		next;
 	    }
 
-	    my $type = $vmlist->{ids}->{$vmid}->{type};
-	    my $node = $vmlist->{ids}->{$vmid}->{node};
+	    my $type = $included_guests->{$vmid}->{type};
+	    my $node = $included_guests->{$vmid}->{node};
 
 	    my $conf;
 	    my $volumes;
diff --git a/PVE/API2/BackupInfo.pm b/PVE/API2/BackupInfo.pm
index 4c461e59..aa22ad34 100644
--- a/PVE/API2/BackupInfo.pm
+++ b/PVE/API2/BackupInfo.pm
@@ -25,7 +25,7 @@ sub get_included_vmids {
 
     for my $job (@$all_jobs) {
 	my $job_included_guests = PVE::VZDump::get_included_guests($job);
-	push @all_vmids, ( map { @{$_} } values %{$job_included_guests} );
+	push @all_vmids, keys %{$job_included_guests};
     }
 
     return { map { $_ => 1 } @all_vmids };
diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 806ac7fd..f8aa46b7 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -70,15 +70,20 @@ __PACKAGE__->register_method ({
 
 	my $cmdline = PVE::VZDump::Common::command_line($param);
 
-	my $vmids_per_node = PVE::VZDump::get_included_guests($param);
-
-	my $local_vmids = delete $vmids_per_node->{$nodename} // [];
-
-	# include IDs for deleted guests, and visibly fail later
-	my $orphaned_vmids = delete $vmids_per_node->{''} // [];
-	push @{$local_vmids}, @{$orphaned_vmids};
-
-	my $skiplist = [ map { @$_ } values $vmids_per_node->%* ];
+	my $included_guests = PVE::VZDump::get_included_guests($param);
+
+	my $local_vmids = [];
+	my $skiplist = [];
+
+	foreach my $vmid (keys %{$included_guests}) {
+	    my $node = $included_guests->{$vmid}->{node};
+	    # include IDs for deleted guests here, and visibly fail later
+	    if (!defined($node) || $node eq $nodename) {
+		push @{$local_vmids}, $vmid;
+	    } else {
+		push @{$skiplist}, $vmid;
+	    }
+	}
 
 	if($param->{stop}){
 	    PVE::VZDump::stop_running_backups();
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 2f31c534..f6399cf7 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -1201,7 +1201,7 @@ sub get_included_guests {
     my ($job) = @_;
 
     my $vmids = [];
-    my $vmids_per_node = {};
+    my $vmid_hash = {};
 
     my $vmlist = PVE::Cluster::get_vmlist();
 
@@ -1219,22 +1219,27 @@ sub get_included_guests {
 	    push @$vmids, $id;
 	}
     } else {
-	return $vmids_per_node;
+	return {};
     }
     $vmids = check_vmids(@$vmids);
 
     for my $vmid (@$vmids) {
 	if (defined($vmlist->{ids}->{$vmid})) {
 	    my $node = $vmlist->{ids}->{$vmid}->{node};
+	    my $type = $vmlist->{ids}->{$vmid}->{type};
+
 	    next if (defined $job->{node} && $job->{node} ne $node);
 
-	    push @{$vmids_per_node->{$node}}, $vmid;
+	    $vmid_hash->{$vmid} = {
+		node => $node,
+		type => $type,
+	    };
 	} else {
-	    push @{$vmids_per_node->{''}}, $vmid;
+	    $vmid_hash->{$vmid} = {};
 	}
     }
 
-    return $vmids_per_node;
+    return $vmid_hash;
 }
 
 1;
diff --git a/test/vzdump_guest_included_test.pl b/test/vzdump_guest_included_test.pl
index 157644de..6ebe009a 100755
--- a/test/vzdump_guest_included_test.pl
+++ b/test/vzdump_guest_included_test.pl
@@ -85,8 +85,38 @@ my $addtest = sub {
 
 $addtest->('Test all guests', {
     expected => {
-	node1 => [ 100, 101, 112, 113 ],
-	node2 => [ 200, 201, 212, 213 ],
+	100 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	101 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	112 => {
+	    'type' => 'lxc',
+	    'node' => 'node1',
+	},
+	113 => {
+	    'type' => 'lxc',
+	    'node' => 'node1',
+	},
+	200 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	212 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
+	213 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
     },
     param => {
 	all => 1,
@@ -95,7 +125,22 @@ $addtest->('Test all guests', {
 
 $addtest->('Test all guests with node limit', {
     expected => {
-	node2 => [ 200, 201, 212, 213 ],
+	200 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	212 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
+	213 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
     },
     param => {
 	all => 1,
@@ -105,8 +150,30 @@ $addtest->('Test all guests with node limit', {
 
 $addtest->('Test exclude', {
     expected => {
-	node1 =>[ 101, 112, 113 ],
-	node2 => [ 201, 212,  213 ],
+	101 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	112 => {
+	    'type' => 'lxc',
+	    'node' => 'node1',
+	},
+	113 => {
+	    'type' => 'lxc',
+	    'node' => 'node1',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	212 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
+	213 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
     },
     param => {
 	all => 1,
@@ -116,7 +183,18 @@ $addtest->('Test exclude', {
 
 $addtest->('Test exclude with node limit', {
     expected => {
-	node1 =>[ 101, 112, 113 ],
+	101 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	112 => {
+	    'type' => 'lxc',
+	    'node' => 'node1',
+	},
+	113 => {
+	    'type' => 'lxc',
+	    'node' => 'node1',
+	},
     },
     param => {
 	all => 1,
@@ -127,8 +205,22 @@ $addtest->('Test exclude with node limit', {
 
 $addtest->('Test pool members', {
     expected => {
-	node1 =>[ 100, 101 ],
-	node2 => [ 200, 201 ],
+	100 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	101 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	200 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
     },
     param => {
 	pool => 'testpool',
@@ -137,7 +229,14 @@ $addtest->('Test pool members', {
 
 $addtest->('Test pool members with node limit', {
     expected => {
-	node2 => [ 200, 201 ],
+	200 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
     },
     param => {
 	pool => 'testpool',
@@ -147,8 +246,18 @@ $addtest->('Test pool members with node limit', {
 
 $addtest->('Test selected VMIDs', {
     expected => {
-	node1 =>[ 100 ],
-	node2 => [ 201, 212 ],
+	100 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	212 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
     },
     param => {
 	vmid => '100, 201, 212',
@@ -157,7 +266,10 @@ $addtest->('Test selected VMIDs', {
 
 $addtest->('Test selected VMIDs with node limit', {
     expected => {
-	node1 =>[ 100 ],
+	100 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
     },
     param => {
 	vmid => '100, 201, 212',
@@ -176,9 +288,19 @@ $addtest->('Test selected VMIDs on other nodes', {
 
 $addtest->('Test VMID not present in vmlist', {
     expected => {
-	node1 => [ 100 ],
-	node2 => [ 201, 212 ],
-	'' => [ 7654 ],
+	100 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	212 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
+	7654 => {},
    },
     param => {
 	vmid => '100, 201, 212, 7654',
-- 
2.20.1

[pve-devel] applied-partially: [PATCH-SERIES v2 manager] Make backup with IDs for non-existent guests visibly fail

[Thomas Lamprecht]

On 22.10.20 12:30, Fabian Ebner wrote:
> #1 and #2 are just cleanups
> 
> #3 and #4 make the necessary changes for the improved behavior
> by ensuring that exec_backup_task will cleanly fail when there
> is no plugin specified, and then including the orphaned IDs
> without assigning them a plugin. This is closer to the behavior
> of PVE 6.0 and ensures that the backup task is marked as failed,
> and the mail notification includes an error for the orphaned IDs.
> 
> #5 ensures that the IDs are still numerically ordered when there
> are non-existing guests
> 
> #6 and #7 are minor improvements and can be applied independently
> 
> #8 is an RFC and changes the data structure returned by get_included_guests
> 
> 
> Changes from v1:
>     * everything, as the approach is different
> 
> 
> Fabian Ebner (8):
>   remove unused variable
>   remove out-of-date comment
>   only use plugin after truthiness check
>   backup: include IDs for non-existent guests
>   order guest IDs numerically in exec_backup
>   sort the skip list numerically
>   simplify get_included_vmids function
>   don't group by node in get_included_guests
> 
>  PVE/API2/Backup.pm                 |  23 ++--
>  PVE/API2/BackupInfo.pm             |  18 +---
>  PVE/API2/VZDump.pm                 |  19 +++-
>  PVE/VZDump.pm                      |  68 +++++++-----
>  test/vzdump_guest_included_test.pl | 163 ++++++++++++++++++++++++++---
>  5 files changed, 212 insertions(+), 79 deletions(-)
> 



applied all but  the RFC, not for any specific reason but I had to cut an release,
thanks!