[pve-devel] [PATCH manager 1/3] close #3476: allow user to prepare storage for activation in job-start hook
Fabian Ebner
f.ebner at proxmox.com
Fri Jan 14 13:39:10 CET 2022
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 at 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 at lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
More information about the pve-devel
mailing list