[pve-devel] [PATCH manager 1/2] fix #1594: add API path to trigger vzdump job manually

[Fabian Grünbichler]

On July 31, 2019 1:23 pm, Stefan Reiter wrote:
> Since we cannot easily proxy the vzdump command to other nodes, we
> expect the client to call this on all nodes they want to run the backup
> job on (and use standard proxyto=>'node').
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
> 
> I wasn't too sure regarding permissions, so I left it at the rather strict
> Sys.Modify. This way, at least no one can use this to do stuff they couldn't
> before (as in: if you had Sys.Modify permissions, you could trigger them
> "immediately" before this patch too, by setting the day and time to "now" and
> waiting a few minutes).
> 
>  PVE/API2/Backup.pm | 61 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
> index ec6b4541..bf9a3330 100644
> --- a/PVE/API2/Backup.pm
> +++ b/PVE/API2/Backup.pm
> @@ -9,7 +9,7 @@ use PVE::Tools qw(extract_param);
>  use PVE::Cluster qw(cfs_register_file cfs_lock_file cfs_read_file cfs_write_file);
>  use PVE::RESTHandler;
>  use PVE::RPCEnvironment;
> -use PVE::JSONSchema;
> +use PVE::JSONSchema qw(get_standard_option);
>  use PVE::Storage;
>  use PVE::Exception qw(raise_param_exc);
>  use PVE::VZDump;
> @@ -491,4 +491,63 @@ __PACKAGE__->register_method({
>  	die "$@" if ($@);
>      }});
>  
> +# this method is most useful when called on all nodes in parallel
> +# (to correctly mimic scheduled behaviour)
> +__PACKAGE__->register_method({
> +    name => 'run_job',
> +    path => '{id}/{node}/run',

this is under /cluster/backup, so I'd like a bit more input on whether 
we want to put this API call here since it's node specific.. another 
approach would be to call Backup->read_job and VZDump->vzdump directly 
from the client, since we most likely already have the job data the 
former might even be not needed, and we would not need to introduce a 
new API call unless I am missing something?

> +    method => 'POST',
> +    description => "Run vzdump backup job definition on a specified node immediately.",
> +    permissions => {
> +	check => ['perm', '/', ['Sys.Modify']],
> +    },
> +    protected => 1,
> +    proxyto => 'node',
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    id => {
> +		type => 'string',
> +		description => "The job ID.",
> +		maxLength => 50,

since this would now be the fourth time in this file, it might make sense 
to refactor it into at least a local variable

> +	    },
> +	    node => get_standard_option('pve-node')
> +	},
> +    },
> +    returns => { type => 'string' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $run_job = sub {
> +	    my $data = cfs_read_file('vzdump.cron');
> +	    my $jobs = $data->{jobs} || [];
> +
> +	    my $found_job;
> +	    foreach my $job (@$jobs) {
> +		if ($job->{id} eq $param->{id}) {
> +		    $found_job = $job;
> +		    last;
> +		}
> +	    }

this could also be a single grep ;)

> +
> +	    raise_param_exc({ id => "No such job '$param->{id}'" })
> +		if !$found_job;

the whole block is basically the read_job API method, maybe we could 
factor that out and/or reuse it (and clean it up regarding dead code 
:-P)?
> +
> +	    # remove scheduling-specific parameters
> +	    delete $found_job->{enabled};
> +	    delete $found_job->{starttime};
> +	    delete $found_job->{dow};
> +	    delete $found_job->{id};
> +
> +	    $found_job->{lockwait} = 0;
> +
> +	    # returns a upid
> +	    return PVE::API2::VZDump->vzdump($found_job);

missing use for that API path (not a problem now, but might hit us when 
this gets called from somewhere other then pveproxy/pvedaemon).

> +	};
> +	my $val = cfs_lock_file('vzdump.cron', undef, $run_job);

do we really need this? we don't modify the crontab file, we just read 
it..

we could basically

my $job = __PACKAGE__->read_job($param);
[..]
return PVE::API2::VZDump->vzdump($job);

unless I am missing something?

> +	die "$@" if ($@);
> +
> +	return $val;
> +    }});
> +
>  1;
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>