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

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

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@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] [RFC manager 2/3] vzdump: exec_backup: pick up latest version of storage.cfg after job-start hook

[Fabian Ebner]

Without this, a storage that is enabled during the job-start hook
would still be seen as disabled.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Makes sense in the context of #3476, so that the storage that is only online
e.g. once a day doesn't need to be kept enabled. But it also means that we have
to be careful not to assert that a storage is enabled before the pre-start hook
in the future.

 PVE/VZDump.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 667c92d3..8c70906d 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -1165,6 +1165,7 @@ sub exec_backup {
 
 	$self->run_hook_script ('job-start', undef, $job_start_fd);
 
+	PVE::Cluster::cfs_update(); # Pick up eventual changes made by the hook script.
 	my $storage_cfg = PVE::Storage::config();
 
 	# activating after job-start hook, so it has a chance to prepare, e.g. wake up remote node.
-- 
2.30.2

[pve-devel] [RFC manager 3/3] vzdump: new: add reminder to get rid of duplicate activate_storage

[Fabian Ebner]

While it's unlikely that something breaks in practice, because
pvestatd calls activate_storage() for enabled storages every few
seconds, there's no rush to remove the duplicate call.

What could require the storage to be active between the activation in
new() and the activation in exec_backup() is:
1. An external storage plugin that requires the storage to be active
   for its get_subdir() implementation (unlikely to exist).
2. A job-start hook-script that requires the storage to be active.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/VZDump.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 8c70906d..4a28e74f 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -501,6 +501,7 @@ sub new {
 
     if ($opts->{storage}) {
 	my $storage_cfg = PVE::Storage::config();
+	# FIXME: remove for PVE 8.0
 	# Ignore errors here. exec_backup will die if activation fails there.
 	eval { PVE::Storage::activate_storage($storage_cfg, $opts->{storage}) };
 
-- 
2.30.2

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

[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?

On December 2, 2021 12:00 pm, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@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@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

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

[Fabian Ebner]

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@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@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>

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

[Fabian Grünbichler]

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 :)