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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On January 14, 2022 1:39 pm, Fabian Ebner wrote:
> 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?

true - I somehow expected that to happen later on in new() before 
returning (as usual), but in this case we do it early on and then 'fill 
it out' as we go :)