[pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks

[pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks

[Stefan Hanreich]

This patch adds hooks that run when the user restores a backup from the Web UI
/ CLI. I have tested this with both VMs/CTs via Web UI and CLI. Are there any
other places where the hook should get triggered that I missed?

Changes compared to v1:
- slightly moved the call site of the exec_hookscript in qemu-server and
pve-container, so necessary checks are run before the hookscript runs.


pve-container:

Stefan Hanreich (1):
  Add pre/post-restore hooks to CTs

 src/PVE/API2/LXC.pm | 7 +++++++
 1 file changed, 7 insertions(+)


pve-docs:

Stefan Hanreich (1):
  add pre/post-restore events to example hookscript

 examples/guest-example-hookscript.pl | 14 ++++++++++++++
 1 file changed, 14 insertions(+)


qemu-server:

Stefan Hanreich (1):
  Add pre/post-restore hooks to VMs

 PVE/API2/Qemu.pm | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH pve-docs 1/1] add pre/post-restore events to example hookscript

[Stefan Hanreich]

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 examples/guest-example-hookscript.pl | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/examples/guest-example-hookscript.pl b/examples/guest-example-hookscript.pl
index adeed59..19fe213 100755
--- a/examples/guest-example-hookscript.pl
+++ b/examples/guest-example-hookscript.pl
@@ -54,6 +54,20 @@ if ($phase eq 'pre-start') {
 
     print "$vmid stopped. Doing cleanup.\n";
 
+} elsif ($phase eq 'pre-restore') {
+
+    # Phase 'pre-restore' will be executed before restoring a backup. (via UI or
+    # CLI)
+
+    print "Restoring $vmid from backup.\n";
+
+} elsif ($phase eq 'post-restore') {
+
+    # Phase 'pre-restore' will be after before restoring a backup. (via UI or
+    # CLI)
+
+    print "$vmid finished restoring from backup.\n";
+
 } else {
     die "got unknown phase '$phase'\n";
 }
-- 
2.30.2

Re: [pve-devel] [PATCH pve-docs 1/1] add pre/post-restore events to example hookscript

[Daniel Tschlatscher]

On 11/10/22 16:33, Stefan Hanreich wrote:
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  examples/guest-example-hookscript.pl | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/examples/guest-example-hookscript.pl b/examples/guest-example-hookscript.pl
> index adeed59..19fe213 100755
> --- a/examples/guest-example-hookscript.pl
> +++ b/examples/guest-example-hookscript.pl
> @@ -54,6 +54,20 @@ if ($phase eq 'pre-start') {
>  
>      print "$vmid stopped. Doing cleanup.\n";
>  
> +} elsif ($phase eq 'pre-restore') {
> +
> +    # Phase 'pre-restore' will be executed before restoring a backup. (via UI or
> +    # CLI)
> +
> +    print "Restoring $vmid from backup.\n";
> +
> +} elsif ($phase eq 'post-restore') {
> +
> +    # Phase 'pre-restore' will be after before restoring a backup. (via UI or
> +    # CLI)

Nit: should say "'post-restore' will be executed after [...]"

> +
> +    print "$vmid finished restoring from backup.\n";
> +
>  } else {
>      die "got unknown phase '$phase'\n";
>  }

[pve-devel] [PATCH pve-container 1/1] Add pre/post-restore hooks to CTs

[Stefan Hanreich]

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/API2/LXC.pm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 79aecaa..0e8cb95 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -380,6 +380,9 @@ __PACKAGE__->register_method({
 		my $orig_mp_param; # only used if $restore
 		if ($restore) {
 		    die "can't overwrite running container\n" if PVE::LXC::check_running($vmid);
+
+		    PVE::GuestHelpers::exec_hookscript($old_conf, $vmid, 'pre-restore', 1);
+
 		    if ($archive ne '-') {
 			my $orig_conf;
 			print "recovering backed-up configuration from '$archive'\n";
@@ -505,6 +508,10 @@ __PACKAGE__->register_method({
 
 	    PVE::API2::LXC::Status->vm_start({ vmid => $vmid, node => $node })
 		if $start_after_create;
+
+	    if ($restore) {
+		PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'post-restore');
+	    }
 	};
 
 	my $workername = $restore ? 'vzrestore' : 'vzcreate';
-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/1] Add pre/post-restore hooks to VMs

[Stefan Hanreich]

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 PVE/API2/Qemu.pm | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index a539b5c..3d4079e 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -888,6 +888,7 @@ __PACKAGE__->register_method({
 	die "$emsg $@" if $@;
 
 	my $restored_data = 0;
+	my $hook_executed = 0;
 	my $restorefn = sub {
 	    my $conf = PVE::QemuConfig->load_config($vmid);
 
@@ -895,6 +896,9 @@ __PACKAGE__->register_method({
 
 	    die "$emsg vm is running\n" if PVE::QemuServer::check_running($vmid);
 
+	    PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'pre-restore', 1);
+	    $hook_executed = 1;
+
 	    my $realcmd = sub {
 		my $restore_options = {
 		    storage => $storage,
@@ -922,6 +926,8 @@ __PACKAGE__->register_method({
 		    eval { PVE::QemuServer::template_create($vmid, $restored_conf) };
 		    warn $@ if $@;
 		}
+
+		PVE::GuestHelpers::exec_hookscript($restored_conf, $vmid, 'post-restore');
 	    };
 
 	    # ensure no old replication state are exists
@@ -1016,10 +1022,10 @@ __PACKAGE__->register_method({
 		if (my $err = $@) {
 		    eval { PVE::QemuConfig->remove_lock($vmid, 'create') };
 		    warn $@ if $@;
-		    if ($restored_data) {
+		    if ($hook_executed && $restored_data) {
 			warn "error after data was restored, VM disks should be OK but config may "
 			    ."require adaptions. VM $vmid state is NOT cleaned up.\n";
-		    } else {
+		    } elsif ($hook_executed && !$restored_data) {
 			warn "error before or during data restore, some or all disks were not "
 			    ."completely restored. VM $vmid state is NOT cleaned up.\n";
 		    }
-- 
2.30.2

Re: [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks

[Daniel Tschlatscher]

The new hookscript example works nicely out of the box.

I tested restore for both VMs and containers via the GUI, the restore
and create commands in the respective CLI commands and with the API.

One thing which might some more consideration:
When restoring a backup that does not configure a hookscript, the
'pre-restore' hook will run, however, the 'post-restore' will not. This
was very confusing at first.
Similarly, if the config does not include a hookscript, but the backup
does, then the 'pre-restore' will not run but the 'post-restore' will.
While this is not breaking, it is definitely very unexpected for an
unsuspecting user.


Otherwise, the core part of the series works as intended, therefore:

Tested-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>


On 11/10/22 16:33, Stefan Hanreich wrote:
> This patch adds hooks that run when the user restores a backup from the Web UI
> / CLI. I have tested this with both VMs/CTs via Web UI and CLI. Are there any
> other places where the hook should get triggered that I missed?
> 
> Changes compared to v1:
> - slightly moved the call site of the exec_hookscript in qemu-server and
> pve-container, so necessary checks are run before the hookscript runs.
> 
> 
> pve-container:
> 
> Stefan Hanreich (1):
>   Add pre/post-restore hooks to CTs
> 
>  src/PVE/API2/LXC.pm | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> 
> pve-docs:
> 
> Stefan Hanreich (1):
>   add pre/post-restore events to example hookscript
> 
>  examples/guest-example-hookscript.pl | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> 
> qemu-server:
> 
> Stefan Hanreich (1):
>   Add pre/post-restore hooks to VMs
> 
>  PVE/API2/Qemu.pm | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 

Re: [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks

[Stefan Hanreich]

On 11/11/22 14:58, Daniel Tschlatscher wrote:
> The new hookscript example works nicely out of the box.
>
> I tested restore for both VMs and containers via the GUI, the restore
> and create commands in the respective CLI commands and with the API.
>
> One thing which might some more consideration:
> When restoring a backup that does not configure a hookscript, the
> 'pre-restore' hook will run, however, the 'post-restore' will not. This
> was very confusing at first.
> Similarly, if the config does not include a hookscript, but the backup
> does, then the 'pre-restore' will not run but the 'post-restore' will.
> While this is not breaking, it is definitely very unexpected for an
> unsuspecting user.

Yes, it might be smarter to use the old config for both pre/post-restore 
and not mix both configurations I think.

Because of this and the minor issues with the example hookscript I will 
create a v3. Some input on whether to use the old configuration for both 
pre/post-restore or not would be much appreciated

Ty for the review!

>
> Otherwise, the core part of the series works as intended, therefore:
>
> Tested-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
>
>
> On 11/10/22 16:33, Stefan Hanreich wrote:
>> This patch adds hooks that run when the user restores a backup from the Web UI
>> / CLI. I have tested this with both VMs/CTs via Web UI and CLI. Are there any
>> other places where the hook should get triggered that I missed?
>>
>> Changes compared to v1:
>> - slightly moved the call site of the exec_hookscript in qemu-server and
>> pve-container, so necessary checks are run before the hookscript runs.
>>
>>
>> pve-container:
>>
>> Stefan Hanreich (1):
>>    Add pre/post-restore hooks to CTs
>>
>>   src/PVE/API2/LXC.pm | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>>
>> pve-docs:
>>
>> Stefan Hanreich (1):
>>    add pre/post-restore events to example hookscript
>>
>>   examples/guest-example-hookscript.pl | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>>
>> qemu-server:
>>
>> Stefan Hanreich (1):
>>    Add pre/post-restore hooks to VMs
>>
>>   PVE/API2/Qemu.pm | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>

Re: [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks

[Daniel Tschlatscher]

On 11/11/22 15:02, Stefan Hanreich wrote:
> 
> On 11/11/22 14:58, Daniel Tschlatscher wrote:
>> The new hookscript example works nicely out of the box.
>>
>> I tested restore for both VMs and containers via the GUI, the restore
>> and create commands in the respective CLI commands and with the API.
>>
>> One thing which might some more consideration:
>> When restoring a backup that does not configure a hookscript, the
>> 'pre-restore' hook will run, however, the 'post-restore' will not. This
>> was very confusing at first.
>> Similarly, if the config does not include a hookscript, but the backup
>> does, then the 'pre-restore' will not run but the 'post-restore' will.
>> While this is not breaking, it is definitely very unexpected for an
>> unsuspecting user.
> 
> Yes, it might be smarter to use the old config for both pre/post-restore
> and not mix both configurations I think.

+1 for not mixing the configurations

Based on our discussion off-list:
I feel like it might be even better to make this setting config-version
agnostic. Or at least, to not overwrite/include it when making or
restoring backups, and to keep it the same each time.
At least that feels like the way I would expect it to work intuitively.

> 
> Because of this and the minor issues with the example hookscript I will
> create a v3. Some input on whether to use the old configuration for both
> pre/post-restore or not would be much appreciated
> 
> Ty for the review!
> 
>>
>> Otherwise, the core part of the series works as intended, therefore:
>>
>> Tested-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
>>
>>
>> On 11/10/22 16:33, Stefan Hanreich wrote:
>>> This patch adds hooks that run when the user restores a backup from
>>> the Web UI
>>> / CLI. I have tested this with both VMs/CTs via Web UI and CLI. Are
>>> there any
>>> other places where the hook should get triggered that I missed?
>>>
>>> Changes compared to v1:
>>> - slightly moved the call site of the exec_hookscript in qemu-server and
>>> pve-container, so necessary checks are run before the hookscript runs.
>>>
>>>
>>> pve-container:
>>>
>>> Stefan Hanreich (1):
>>>    Add pre/post-restore hooks to CTs
>>>
>>>   src/PVE/API2/LXC.pm | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>>
>>> pve-docs:
>>>
>>> Stefan Hanreich (1):
>>>    add pre/post-restore events to example hookscript
>>>
>>>   examples/guest-example-hookscript.pl | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>>
>>> qemu-server:
>>>
>>> Stefan Hanreich (1):
>>>    Add pre/post-restore hooks to VMs
>>>
>>>   PVE/API2/Qemu.pm | 10 ++++++++--
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>

[pve-devel] [PATCH pve-container/pve-docs/qemu-server 0/1] Add pre/post-restore hooks

[Stefan Hanreich]

This patch adds hooks that run when the user restores a backup from the Web UI
/ CLI. I have tested this with both VMs/CTs via Web UI and CLI. Are there any
other places where the hook should get triggered that I missed?

pve-container:

Stefan Hanreich (1):
  Add pre/post-restore hooks to CTs

 src/PVE/API2/LXC.pm | 6 ++++++
 1 file changed, 6 insertions(+)

qemu-server:

Stefan Hanreich (1):
  Add pre/post-restore hooks to VMs

 PVE/API2/Qemu.pm | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

pve-docs:

Stefan Hanreich (1):
  add pre/post-restore events to example hookscript

 examples/guest-example-hookscript.pl | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/1] Add pre/post-restore hooks to VMs

[Stefan Hanreich]

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---

There might be a better way to differentiate the different errors from
restorefn in the error handling logic, although I think in this case it is
still fine. This might get a bit messy though if in the future someone adds
another source for errors. Maybe add a single if before the restored_data checks
instead?

 PVE/API2/Qemu.pm | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 3ec31c2..fe41634 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -884,9 +884,13 @@ __PACKAGE__->register_method({
 	die "$emsg $@" if $@;
 
 	my $restored_data = 0;
+	my $hook_executed = 0;
 	my $restorefn = sub {
 	    my $conf = PVE::QemuConfig->load_config($vmid);
 
+	    PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'pre-restore', 1);
+	    $hook_executed = 1;
+
 	    PVE::QemuConfig->check_protection($conf, $emsg);
 
 	    die "$emsg vm is running\n" if PVE::QemuServer::check_running($vmid);
@@ -918,6 +922,8 @@ __PACKAGE__->register_method({
 		    eval { PVE::QemuServer::template_create($vmid, $restored_conf) };
 		    warn $@ if $@;
 		}
+
+		PVE::GuestHelpers::exec_hookscript($restored_conf, $vmid, 'post-restore');
 	    };
 
 	    # ensure no old replication state are exists
@@ -1012,10 +1018,10 @@ __PACKAGE__->register_method({
 		if (my $err = $@) {
 		    eval { PVE::QemuConfig->remove_lock($vmid, 'create') };
 		    warn $@ if $@;
-		    if ($restored_data) {
+		    if ($hook_executed && $restored_data) {
 			warn "error after data was restored, VM disks should be OK but config may "
 			    ."require adaptions. VM $vmid state is NOT cleaned up.\n";
-		    } else {
+		    } elsif ($hook_executed && !$restored_data) {
 			warn "error before or during data restore, some or all disks were not "
 			    ."completely restored. VM $vmid state is NOT cleaned up.\n";
 		    }
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server 1/1] Add pre/post-restore hooks to VMs

[Wolfgang Bumiller]

On Thu, Sep 22, 2022 at 03:19:43PM +0200, Stefan Hanreich wrote:
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> 
> There might be a better way to differentiate the different errors from
> restorefn in the error handling logic, although I think in this case it is
> still fine. This might get a bit messy though if in the future someone adds
> another source for errors. Maybe add a single if before the restored_data checks
> instead?
> 
>  PVE/API2/Qemu.pm | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 3ec31c2..fe41634 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -884,9 +884,13 @@ __PACKAGE__->register_method({
>  	die "$emsg $@" if $@;
>  
>  	my $restored_data = 0;
> +	my $hook_executed = 0;
>  	my $restorefn = sub {
>  	    my $conf = PVE::QemuConfig->load_config($vmid);
>  
> +	    PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'pre-restore', 1);
> +	    $hook_executed = 1;
> +
>  	    PVE::QemuConfig->check_protection($conf, $emsg);

This should probalby happen before the hook?

>  
>  	    die "$emsg vm is running\n" if PVE::QemuServer::check_running($vmid);
> @@ -918,6 +922,8 @@ __PACKAGE__->register_method({
>  		    eval { PVE::QemuServer::template_create($vmid, $restored_conf) };
>  		    warn $@ if $@;
>  		}
> +
> +		PVE::GuestHelpers::exec_hookscript($restored_conf, $vmid, 'post-restore');
>  	    };
>  
>  	    # ensure no old replication state are exists
> @@ -1012,10 +1018,10 @@ __PACKAGE__->register_method({
>  		if (my $err = $@) {
>  		    eval { PVE::QemuConfig->remove_lock($vmid, 'create') };
>  		    warn $@ if $@;
> -		    if ($restored_data) {
> +		    if ($hook_executed && $restored_data) {
>  			warn "error after data was restored, VM disks should be OK but config may "
>  			    ."require adaptions. VM $vmid state is NOT cleaned up.\n";
> -		    } else {
> +		    } elsif ($hook_executed && !$restored_data) {
>  			warn "error before or during data restore, some or all disks were not "
>  			    ."completely restored. VM $vmid state is NOT cleaned up.\n";
>  		    }
> -- 
> 2.30.2