From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 6853260FA0 for ; Fri, 14 Jan 2022 13:39:17 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 601EE29B52 for ; Fri, 14 Jan 2022 13:39:17 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 7CF5E29B47 for ; Fri, 14 Jan 2022 13:39:16 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 589174606D for ; Fri, 14 Jan 2022 13:39:16 +0100 (CET) Message-ID: <089ed420-6932-1c47-5281-272cb244a4cf@proxmox.com> Date: Fri, 14 Jan 2022 13:39:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.1 Content-Language: en-US To: =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= , Proxmox VE development discussion References: <20211202110005.38699-1-f.ebner@proxmox.com> <1642159551.i8ghw8jx3f.astroid@nora.none> From: Fabian Ebner In-Reply-To: <1642159551.i8ghw8jx3f.astroid@nora.none> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.136 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH manager 1/3] close #3476: allow user to prepare storage for activation in job-start hook X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Jan 2022 12:39:17 -0000 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 >> --- >> 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 >> >> >>