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 4041560F28 for ; Fri, 14 Jan 2022 12:46:43 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 36212292FA for ; Fri, 14 Jan 2022 12:46:43 +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 7DDE7292ED for ; Fri, 14 Jan 2022 12:46:41 +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 572B946C06 for ; Fri, 14 Jan 2022 12:46:41 +0100 (CET) Date: Fri, 14 Jan 2022 12:46:34 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion , Fabian Ebner References: <20211202110005.38699-1-f.ebner@proxmox.com> In-Reply-To: <20211202110005.38699-1-f.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1642159551.i8ghw8jx3f.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.221 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 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 11:46:43 -0000 while the approach in this series works, I am a bit unhappy about the=20 fact that it requires a cfs_update in the (sort of) middle of a worker,=20 after having already parsed vmlist and storage.cfg (and making decisions=20 based on their contents / saving intermediate results for using later on=20 in the process). as an alternative, wouldn't calling the hook script either directly=20 after forking the vzdump worker (in the API) or somewhere at the start=20 of PVE::VZDump->new() work as well? e.g., with a new 'phase' 'job-setup'=20 or 'job-init' and just storage/dumpdir and script set. IMHO a sensible=20 point would be before the `if ($opts->{storage}) {` in new() that this=20 series touches - we have the basic, simple opts handled at that point,=20 but haven't started any of the logic/active parts of the initialization=20 of the vzdump job, so we are still okay to do a cfs_update without=20 worrying about subtle side-effects. and since it would be a new phase,=20 we can just document that the storage might not be activated yet at that=20 point. obviously this approach is also a bit hacky (run_hookscript=20 supposedly takes a PVE::VZDump instance which doesn't exist yet at this=20 point ;)), but it seems less involved / with less potential for=20 side-effects than the current one? 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(-) >=20 > 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 { > =20 > if ($opts->{storage}) { > my $storage_cfg =3D 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 =3D $@) { > - chomp($err); > - $errors .=3D "could not activate storage '$opts->{storage}': $err"; > - } > =20 > my $info =3D eval { storage_info ($opts->{storage}) }; > if (my $err =3D $@) { > @@ -1168,6 +1165,12 @@ sub exec_backup { > =20 > $self->run_hook_script ('job-start', undef, $job_start_fd); > =20 > + my $storage_cfg =3D 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 +=3D 1 if $task->{state} ne 'ok'; > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20