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 410066103E for ; Fri, 14 Jan 2022 14:08:00 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2E9682A297 for ; Fri, 14 Jan 2022 14:07:30 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id B6EF32A28B for ; Fri, 14 Jan 2022 14:07:26 +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 2F06846CBE for ; Fri, 14 Jan 2022 14:07:26 +0100 (CET) Date: Fri, 14 Jan 2022 14:07:19 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Fabian Ebner , Proxmox VE development discussion References: <20211202110005.38699-1-f.ebner@proxmox.com> <1642159551.i8ghw8jx3f.astroid@nora.none> <089ed420-6932-1c47-5281-272cb244a4cf@proxmox.com> In-Reply-To: <089ed420-6932-1c47-5281-272cb244a4cf@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1642165500.23a1yq5zb9.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 13:08:00 -0000 On January 14, 2022 1:39 pm, Fabian Ebner wrote: > Am 14.01.22 um 12:46 schrieb Fabian Gr=C3=BCnbichler: >> 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). >>=20 >> 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? >>=20 >=20 > I wanted to avoid a too similar hook point when there's already=20 > job-start, but you now convinced me that there's enough need for a=20 > distinction :) >=20 > About the hacky bit: before `if ($opts->{storage}) {`, the instance is=20 > already blessed, so that shouldn't be an issue either? true - I somehow expected that to happen later on in new() before=20 returning (as usual), but in this case we do it early on and then 'fill=20 it out' as we go :)