On 6/5/20 8:39 PM, Thomas Lamprecht wrote:
On 5/6/20 11:57 AM, Aaron Lauterer wrote:
[...]
diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 68a3de89..77dac1b1 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -69,19 +69,26 @@ __PACKAGE__->register_method ({
return 'OK' if $param->{node} && $param->{node} ne $nodename;
my $cmdline = PVE::VZDump::Common::command_line($param);
- my ($vmids, $skiplist) = PVE::VZDump->get_included_guests($param);
+
+ my $vmids_per_node = PVE::VZDump->get_included_guests($param);
+
+ my $vmids = $vmids_per_node->{$nodename} // [];
maybe it could make belows looping easier if you delete the local ones out?
So, for above:
my $local_vmids = delete $vmids_per_node->{$nodename} // [];
+
+ my $skiplist = [];
+
+ for my $current_node (keys %{$vmids_per_node}) {
+ push @{$skiplist}, @{$vmids_per_node->{$current_node}}
+ if $current_node ne $nodename;
with the delete above you could drop the post-if here actually you should be
able to just do:
my $skiplist = [ map { @$_ } values $vmids_per_node->%* ];
Thanks for that idea :)
(FYI: $vmids_per_node->%* is just alternate writing of %$vmids_per_node IIRC,
Wolfgang prefers it)
+ }
if($param->{stop}){
PVE::VZDump::stop_running_backups();
return 'OK' if !scalar(@{$vmids});
}
- # silent exit if specified VMs run on other nodes
+ # silent exit if specified guests run on other nodes
return "OK" if !scalar(@{$vmids});
- my @exclude = PVE::Tools::split_list(extract_param($param, 'exclude'));
- $param->{exclude} = PVE::VZDump::check_vmids(@exclude);
-
# exclude-path list need to be 0 separated
if (defined($param->{'exclude-path'})) {
my @expaths = split(/\0/, $param->{'exclude-path'} || '');
@@ -106,6 +113,7 @@ __PACKAGE__->register_method ({
die "interrupted by signal\n";
};
+ $param->{vmids} = $vmids;
my $vzdump = PVE::VZDump->new($cmdline, $param, $skiplist);
eval {
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 68c08f47..cd278f4d 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -1039,29 +1039,18 @@ sub exec_backup {
if scalar(@{$self->{skiplist}});
my $tasklist = [];
+ my $vzdump_plugins = {};
+ foreach my $plugin (@{$self->{plugins}}) {
maybe add:
next if exists $vzdump_plugins->{$type};
as then we'd just overwrite the same type again.
yeah sounds good.
+ my $type = $plugin->type();
+ $vzdump_plugins->{$type} = $plugin;
+ }
- if ($opts->{all}) {
- foreach my $plugin (@{$self->{plugins}}) {
- my $vmlist = $plugin->vmlist();
- foreach my $vmid (sort @$vmlist) {
- next if grep { $_ eq $vmid } @{$opts->{exclude}};
- next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup'
], 1);
- push @$tasklist, { vmid => $vmid, state => 'todo', plugin =>
$plugin, mode => $opts->{mode} };
- }
- }
- } else {
- foreach my $vmid (sort @{$opts->{vmids}}) {
- my $plugin;
- foreach my $pg (@{$self->{plugins}}) {
- my $vmlist = $pg->vmlist();
- if (grep { $_ eq $vmid } @$vmlist) {
- $plugin = $pg;
- last;
- }
- }
- $rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ]);
- push @$tasklist, { vmid => $vmid, state => 'todo', plugin => $plugin,
mode => $opts->{mode} };
- }
+ my $vmlist = PVE::Cluster::get_vmlist();
+ foreach my $vmid (sort @{$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});
+ push @$tasklist, { vmid => $vmid, state => 'todo', plugin => $plugin, mode
=> $opts->{mode} };
I now it blows up lines a bit but I prefer having multiple hash elements on
separate ones most of the time.
push @$tasklist, {
vmid => $vmid,
...,
};
Easier to read and change.
hehe, I just read our coding guidelines a few days ago and that
particular thing was one of my personal take aways ;)
}
# Use in-memory files for the outer hook logs to pass them to sendmail.
@@ -1169,34 +1158,39 @@ sub get_included_guests {
my $nodename = PVE::INotify::nodename();
my $vmids = [];
+ my $vmids_per_node = {};
+
+ my $vmlist = PVE::Cluster::get_vmlist();
- # convert string lists to arrays
if ($job->{pool}) {
$vmids = PVE::API2Tools::get_resource_pool_guest_members($job->{pool});
+ } elsif ($job->{vmid}) {
+ $vmids = [ PVE::Tools::split_list($job->{vmid}) ];
} else {
- $vmids = [ PVE::Tools::split_list(extract_param($job, 'vmid')) ];
+ # all or exclude
+ my @exclude = PVE::Tools::split_list($job->{exclude});
+ @exclude = @{PVE::VZDump::check_vmids(@exclude)};
+ my $excludehash = { map { $_ => 1 } @exclude };
+
+ for my $id (keys %{ $vmlist->{ids} }) {
you could sort it here already, avoiding the sort after the loop.
Whole thing could also be written as:
$vmids = [ grep { !$excludehash->{$_} } sort keys $vmlist->{ids}->%* ];
but no hard feeling here.
I agree on the early sorting, but I am not sure about that oneliner.
It's not as easy to comprehend what's going on for someone who isn't
used to this kind of syntax already.
+ next if $excludehash->{$id};
+ push @$vmids, $id;
+ }
}
+ $vmids = [ sort {$a <=> $b} @$vmids];
- my $skiplist = [];
- if (!$job->{all}) {
- if (!$job->{node} || $job->{node} eq $nodename) {
- my $vmlist = PVE::Cluster::get_vmlist();
- my $localvmids = [];
- foreach my $vmid (@{$vmids}) {
- my $d = $vmlist->{ids}->{$vmid};
- if ($d && ($d->{node} ne $nodename)) {
- push @{$skiplist}, $vmid;
- } else {
- push @{$localvmids}, $vmid;
- }
- }
- $vmids = $localvmids;
- }
+ $vmids = PVE::VZDump::check_vmids(@$vmids);
+
+ foreach my $vmid (@$vmids) {
+ my $vmid_data = $vmlist->{ids}->{$vmid};
+ my $node = $vmid_data->{node};
+
+ next if (defined $job->{node} && $job->{node} ne $node);>
- $job->{vmids} = PVE::VZDump::check_vmids(@{$vmids})
+ push @{$vmids_per_node->{$node}}, $vmid;
}
- return ($vmids, $skiplist);
+ return $vmids_per_node;
}
1;
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
