Re: [pve-devel] [PATCH manager 1/3] close #3476: allow user to prepare storage for activation in job-start hook

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

while the approach in this series works, I am a bit unhappy about the 
fact that it requires a cfs_update in the (sort of) middle of a worker, 
after having already parsed vmlist and storage.cfg (and making decisions 
based on their contents / saving intermediate results for using later on 
in the process).

as an alternative, wouldn't calling the hook script either directly 
after forking the vzdump worker (in the API) or somewhere at the start 
of PVE::VZDump->new() work as well? e.g., with a new 'phase' 'job-setup' 
or 'job-init' and just storage/dumpdir and script set. IMHO a sensible 
point would be before the `if ($opts->{storage}) {` in new() that this 
series touches - we have the basic, simple opts handled at that point, 
but haven't started any of the logic/active parts of the initialization 
of the vzdump job, so we are still okay to do a cfs_update without 
worrying about subtle side-effects. and since it would be a new phase, 
we can just document that the storage might not be activated yet at that 
point. obviously this approach is also a bit hacky (run_hookscript 
supposedly takes a PVE::VZDump instance which doesn't exist yet at this 
point ;)), but it seems less involved / with less potential for 
side-effects than the current one?

On December 2, 2021 12:00 pm, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/VZDump.pm | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index b5a5fadd..667c92d3 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -501,11 +501,8 @@ sub new {
>  
>      if ($opts->{storage}) {
>  	my $storage_cfg = PVE::Storage::config();
> +	# Ignore errors here. exec_backup will die if activation fails there.
>  	eval { PVE::Storage::activate_storage($storage_cfg, $opts->{storage}) };
> -	if (my $err = $@) {
> -	    chomp($err);
> -	    $errors .= "could not activate storage '$opts->{storage}': $err";
> -	}
>  
>  	my $info = eval { storage_info ($opts->{storage}) };
>  	if (my $err = $@) {
> @@ -1168,6 +1165,12 @@ sub exec_backup {
>  
>  	$self->run_hook_script ('job-start', undef, $job_start_fd);
>  
> +	my $storage_cfg = PVE::Storage::config();
> +
> +	# activating after job-start hook, so it has a chance to prepare, e.g. wake up remote node.
> +	eval { PVE::Storage::activate_storage($storage_cfg, $opts->{storage}) };
> +	die "could not activate storage '$opts->{storage}': $@" if $@;
> +
>  	foreach my $task (@$tasklist) {
>  	    $self->exec_backup_task ($task);
>  	    $errcount += 1 if $task->{state} ne 'ok';
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>