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

[Fabian Ebner <f.ebner@proxmox.com>]

Am 14.01.22 um 12:46 schrieb Fabian Grünbichler:
> 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?
> 

I wanted to avoid a too similar hook point when there's already 
job-start, but you now convinced me that there's enough need for a 
distinction :)

About the hacky bit: before `if ($opts->{storage}) {`, the instance is 
already blessed, so that shouldn't be an issue either?

> 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
>>
>>
>>