Re: [pve-devel] [RFC v2 manager 1/5] api: backup: add endpoint to list included guests and volumes
On 4/7/20 5:55 PM, Thomas Lamprecht wrote:
> On 4/6/20 4:24 PM, Aaron Lauterer wrote:
>> [...]
>> +description => 'Root node of the tree object. Children
represent guests, grandchildren represent volumes of that guest.',
>> +properties => {
>> +not_all_permissions => {
>> +type => 'boolean',
>> +optional => 1,
>> +description => 'Wheter the user is missing permissions to
view all guests.',
>
> s/Wheter/Whether/
thanks for catching that typo
>
>> +},
>> +children => {
>> +type => 'array',
>> +items => {
>> +type => 'object',
>> +properties => {
>> +id => {
>> +type => 'integer',
>> +description => 'VMID of the guest.',
>> +},
>> +name => {
>> +type => 'string',
>> +description => 'Name of the guest',
>> +optional => 1,
>> +},
>> +type => {
>> +type => 'string',
>> +description => 'Type of the guest, VM or CT.',
>> +enum => ['qemu', 'lxc', 'unknown'],
>
> We die in the unknown case, so that cannot be returned
right
>
>> +},
>> +children => {
>> +type => 'array',
>> +optional => 1,
>> +description => 'The volumes of the guest with the
information if they will be included in backups.',
>> +items => {
>> +type => 'object',
>> +properties => {
>> +id => {
>> +type => 'string',
>> +description => 'Configuration key of the volume.',
>> +},
>> +name => {
>> +type => 'string',
>> +description => 'Name of the volume.',
>> +},
>> +included => {
>> +type => 'boolean',
>> +description => 'Wheter the volume is included
in the backup or not.',
>> +},
>> +reason => {
>> +type => 'string',
>> +description => 'The reason why the volume is
included (or excluded).',
>> +},
>> +},
>> +},
>> +},
>> +},
>> +},
>> +},
>> +},
>> +},
>> +code => sub {
>> +my ($param) = @_;
>> +
>> +my $rpcenv = PVE::RPCEnvironment::get();
>> +
>> +my $user = $rpcenv->get_user();
>> +
>> +my $vzconf = cfs_read_file('vzdump.cron');
>> +my $all_jobs = $vzconf->{jobs} || [];
>> +my $job;
>> +my $rrd = PVE::Cluster::rrd_dump();
>> +
>> +foreach my $j (@$all_jobs) {
>> +$job = $j if $j->{id} eq $param->{id};
>
> This could let the reader think that this is a bug, as it looks like
it gets
> overwritten, plus on huge amount of jobs you would iterate a lot of
those for
> nothing, if the $job with the requested id was found early. Rather do:
>
> if ($j->{id} eq $param->{id}) {
> $job = $j;
> last;
> }
>
> This makes it much more explicit and easier to grasp when just
quickly reading over
> the code.
will do
>
>> +}
>> +raise_param_exc({ id => "No such job '$param->{id}'" }) if !$job;
>> +
>> +my $vmlist = PVE::Cluster::get_vmlist();
>> +
>> +my @vmids = @{PVE::VZDump->get_included_guests($job)};
>
> s/vmid/job_vmids/
>
> and the comment on the other series regarding passing this as reference,
> and even as hash.
Yeah, this will be adapted to the changes of the other series, one of
the reasons why this is labeled as RFC
>
>> +
>> +# remove VMIDs to which the user has no permission to not leak
infos
>> +# like the guest name
>> +my $not_all_permissions = 0;
>> +@vmids = grep {
>> +my $allowed = $rpcenv->check($user, "/vms/$_", [
'VM.Backup' ], 1);
>
> hmm, is VM.Backup really the info we want to hide, why not VM.Audit?
> (or those which the user has either permission?)
>
> As VM.Backup is the permission for being allowed to make a backup or
restore one,
> not for being allowed to view a VM in the cluster.
I haven't fully groked the permission system yet. What I want to achieve
is to not show any VMs the user does not have permission to see.
I think VM.Audit should be enough then.
>
>> +$not_all_permissions = 1 if !$allowed && !$job->{all};
>
> This could be found out afterwards, avoids a O(n) expression.
$job->{all} is
> available anyway and if you remember how many elements @vmids had
before you
> can just check if it has less after the permission checking. That has the
> additional advantage that you can omit the $allowed intermediate
variable. For
> example (not fully thought out - so don't just copy 1:1)
>
> my $job_guest_count = scalar(@vmids);
> my @allowed_vmids = grep {
> $rpcenv->check_any($user, "/vms/$_", [ 'VM.Audit',
Re: [pve-devel] [RFC v2 manager 1/5] api: backup: add endpoint to list included guests and volumes
On 4/6/20 4:24 PM, Aaron Lauterer wrote:
> This patch adds a new API endpoint that returns a list of included
> guests, their volumes and whether they are included in a backup.
>
> The output is formatted to be used with the extJS tree panel.
>
> Signed-off-by: Aaron Lauterer
> ---
> v1 -> v2:
> * simplified the code
> * refactored according to feedback from fabian [1]
>
> The call to PVE::VZDump->get_included_guests($job) will definitely
> change as that patch need another version [0]
>
> [0] https://pve.proxmox.com/pipermail/pve-devel/2020-April/042795.html
> [1] https://pve.proxmox.com/pipermail/pve-devel/2020-January/041361.html
>
> PVE/API2/Backup.pm | 176 +
> 1 file changed, 176 insertions(+)
>
> diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
> index 86377c0a..685a196c 100644
> --- a/PVE/API2/Backup.pm
> +++ b/PVE/API2/Backup.pm
> @@ -324,4 +324,180 @@ __PACKAGE__->register_method({
> die "$@" if ($@);
> }});
>
> +__PACKAGE__->register_method({
> +name => 'get_volume_backup_included',
> +path => '{id}/included_volumes',
> +method => 'GET',
> +protected => 1,
> +description => "Returns included guests and the backup status of their
> disks. Optimized to be used in ExtJS tree views.",
> +permissions => {
> + check => ['perm', '/', ['Sys.Audit']],
> +},
> +parameters => {
> + additionalProperties => 0,
> + properties => {
> + id => $vzdump_job_id_prop
> + },
> +},
> +returns => {
> + type => 'object',
> + description => 'Root node of the tree object. Children represent
> guests, grandchildren represent volumes of that guest.',
> + properties => {
> + not_all_permissions => {
> + type => 'boolean',
> + optional => 1,
> + description => 'Wheter the user is missing permissions to view
> all guests.',
s/Wheter/Whether/
> + },
> + children => {
> + type => 'array',
> + items => {
> + type => 'object',
> + properties => {
> + id => {
> + type => 'integer',
> + description => 'VMID of the guest.',
> + },
> + name => {
> + type => 'string',
> + description => 'Name of the guest',
> + optional => 1,
> + },
> + type => {
> + type => 'string',
> + description => 'Type of the guest, VM or CT.',
> + enum => ['qemu', 'lxc', 'unknown'],
We die in the unknown case, so that cannot be returned
> + },
> + children => {
> + type => 'array',
> + optional => 1,
> + description => 'The volumes of the guest with the
> information if they will be included in backups.',
> + items => {
> + type => 'object',
> + properties => {
> + id => {
> + type => 'string',
> + description => 'Configuration key of
> the volume.',
> + },
> + name => {
> + type => 'string',
> + description => 'Name of the volume.',
> + },
> + included => {
> + type => 'boolean',
> + description => 'Wheter the volume is
> included in the backup or not.',
> + },
> + reason => {
> + type => 'string',
> + description => 'The reason why the
> volume is included (or excluded).',
> + },
> + },
> + },
> + },
> + },
> + },
> + },
> + },
> +},
> +code => sub {
> + my ($param) = @_;
> +
> + my $rpcenv = PVE::RPCEnvironment::get();
> +
> + my $user = $rpcenv->get_user();
> +
> + my $vzconf = cfs_read_file('vzdump.cron');
> + my $all_jobs = $vzconf->{jobs} || [];
> + my $job;
> + my $rrd = PVE::Cluster::rrd_dump();
> +
> + foreach my $j (@$all_jobs) {
> + $job = $j if $j->{id} eq $param->{id};
This could let the reader think that this is a bug, as it looks like it gets
overwritten, plus on huge amount of jobs you would iterate a lot of those for
nothing, if the $job with the requested id was found early. Rather do:
if ($j->{id} eq
[pve-devel] [RFC v2 manager 1/5] api: backup: add endpoint to list included guests and volumes
This patch adds a new API endpoint that returns a list of included
guests, their volumes and whether they are included in a backup.
The output is formatted to be used with the extJS tree panel.
Signed-off-by: Aaron Lauterer
---
v1 -> v2:
* simplified the code
* refactored according to feedback from fabian [1]
The call to PVE::VZDump->get_included_guests($job) will definitely
change as that patch need another version [0]
[0] https://pve.proxmox.com/pipermail/pve-devel/2020-April/042795.html
[1] https://pve.proxmox.com/pipermail/pve-devel/2020-January/041361.html
PVE/API2/Backup.pm | 176 +
1 file changed, 176 insertions(+)
diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 86377c0a..685a196c 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -324,4 +324,180 @@ __PACKAGE__->register_method({
die "$@" if ($@);
}});
+__PACKAGE__->register_method({
+name => 'get_volume_backup_included',
+path => '{id}/included_volumes',
+method => 'GET',
+protected => 1,
+description => "Returns included guests and the backup status of their
disks. Optimized to be used in ExtJS tree views.",
+permissions => {
+ check => ['perm', '/', ['Sys.Audit']],
+},
+parameters => {
+ additionalProperties => 0,
+ properties => {
+ id => $vzdump_job_id_prop
+ },
+},
+returns => {
+ type => 'object',
+ description => 'Root node of the tree object. Children represent
guests, grandchildren represent volumes of that guest.',
+ properties => {
+ not_all_permissions => {
+ type => 'boolean',
+ optional => 1,
+ description => 'Wheter the user is missing permissions to view
all guests.',
+ },
+ children => {
+ type => 'array',
+ items => {
+ type => 'object',
+ properties => {
+ id => {
+ type => 'integer',
+ description => 'VMID of the guest.',
+ },
+ name => {
+ type => 'string',
+ description => 'Name of the guest',
+ optional => 1,
+ },
+ type => {
+ type => 'string',
+ description => 'Type of the guest, VM or CT.',
+ enum => ['qemu', 'lxc', 'unknown'],
+ },
+ children => {
+ type => 'array',
+ optional => 1,
+ description => 'The volumes of the guest with the
information if they will be included in backups.',
+ items => {
+ type => 'object',
+ properties => {
+ id => {
+ type => 'string',
+ description => 'Configuration key of
the volume.',
+ },
+ name => {
+ type => 'string',
+ description => 'Name of the volume.',
+ },
+ included => {
+ type => 'boolean',
+ description => 'Wheter the volume is
included in the backup or not.',
+ },
+ reason => {
+ type => 'string',
+ description => 'The reason why the
volume is included (or excluded).',
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+},
+code => sub {
+ my ($param) = @_;
+
+ my $rpcenv = PVE::RPCEnvironment::get();
+
+ my $user = $rpcenv->get_user();
+
+ my $vzconf = cfs_read_file('vzdump.cron');
+ my $all_jobs = $vzconf->{jobs} || [];
+ my $job;
+ my $rrd = PVE::Cluster::rrd_dump();
+
+ foreach my $j (@$all_jobs) {
+ $job = $j if $j->{id} eq $param->{id};
+ }
+ raise_param_exc({ id => "No such job '$param->{id}'" }) if !$job;
+
+ my $vmlist = PVE::Cluster::get_vmlist();
+
+ my @vmids = @{PVE::VZDump->get_included_guests($job)};
+
+ # remove VMIDs to which the user has no permission to not leak infos
+ # like the guest name
+ my $not_all_permissions = 0;
+ @vmids = grep {
+ my $allowed = $rpcenv->check($user, "/vms/$_", [ 'VM.Backup' ],
1);
+
