public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks
@ 2022-11-10 15:33 Stefan Hanreich
  2022-11-10 15:33 ` [pve-devel] [PATCH pve-docs 1/1] add pre/post-restore events to example hookscript Stefan Hanreich
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Stefan Hanreich @ 2022-11-10 15:33 UTC (permalink / raw)
  To: pve-devel

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




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [pve-devel] [PATCH pve-docs 1/1] add pre/post-restore events to example hookscript
  2022-11-10 15:33 [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks Stefan Hanreich
@ 2022-11-10 15:33 ` Stefan Hanreich
  2022-11-11 13:58   ` Daniel Tschlatscher
  2022-11-10 15:33 ` [pve-devel] [PATCH pve-container 1/1] Add pre/post-restore hooks to CTs Stefan Hanreich
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Stefan Hanreich @ 2022-11-10 15:33 UTC (permalink / raw)
  To: pve-devel

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




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [pve-devel] [PATCH pve-container 1/1] Add pre/post-restore hooks to CTs
  2022-11-10 15:33 [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks Stefan Hanreich
  2022-11-10 15:33 ` [pve-devel] [PATCH pve-docs 1/1] add pre/post-restore events to example hookscript Stefan Hanreich
@ 2022-11-10 15:33 ` Stefan Hanreich
  2022-11-10 15:33 ` [pve-devel] [PATCH qemu-server 1/1] Add pre/post-restore hooks to VMs Stefan Hanreich
  2022-11-11 13:58 ` [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks Daniel Tschlatscher
  3 siblings, 0 replies; 8+ messages in thread
From: Stefan Hanreich @ 2022-11-10 15:33 UTC (permalink / raw)
  To: pve-devel

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




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [pve-devel] [PATCH qemu-server 1/1] Add pre/post-restore hooks to VMs
  2022-11-10 15:33 [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks Stefan Hanreich
  2022-11-10 15:33 ` [pve-devel] [PATCH pve-docs 1/1] add pre/post-restore events to example hookscript Stefan Hanreich
  2022-11-10 15:33 ` [pve-devel] [PATCH pve-container 1/1] Add pre/post-restore hooks to CTs Stefan Hanreich
@ 2022-11-10 15:33 ` Stefan Hanreich
  2022-11-11 13:58 ` [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks Daniel Tschlatscher
  3 siblings, 0 replies; 8+ messages in thread
From: Stefan Hanreich @ 2022-11-10 15:33 UTC (permalink / raw)
  To: pve-devel

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




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH pve-docs 1/1] add pre/post-restore events to example hookscript
  2022-11-10 15:33 ` [pve-devel] [PATCH pve-docs 1/1] add pre/post-restore events to example hookscript Stefan Hanreich
@ 2022-11-11 13:58   ` Daniel Tschlatscher
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Tschlatscher @ 2022-11-11 13:58 UTC (permalink / raw)
  To: pve-devel

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";
>  }




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks
  2022-11-10 15:33 [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks Stefan Hanreich
                   ` (2 preceding siblings ...)
  2022-11-10 15:33 ` [pve-devel] [PATCH qemu-server 1/1] Add pre/post-restore hooks to VMs Stefan Hanreich
@ 2022-11-11 13:58 ` Daniel Tschlatscher
  2022-11-11 14:02   ` Stefan Hanreich
  3 siblings, 1 reply; 8+ messages in thread
From: Daniel Tschlatscher @ 2022-11-11 13:58 UTC (permalink / raw)
  To: pve-devel

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




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks
  2022-11-11 13:58 ` [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks Daniel Tschlatscher
@ 2022-11-11 14:02   ` Stefan Hanreich
  2022-11-11 14:16     ` Daniel Tschlatscher
  0 siblings, 1 reply; 8+ messages in thread
From: Stefan Hanreich @ 2022-11-11 14:02 UTC (permalink / raw)
  To: Proxmox VE development discussion, Daniel Tschlatscher


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




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks
  2022-11-11 14:02   ` Stefan Hanreich
@ 2022-11-11 14:16     ` Daniel Tschlatscher
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Tschlatscher @ 2022-11-11 14:16 UTC (permalink / raw)
  To: Stefan Hanreich, Proxmox VE development discussion



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




^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-11-11 14:17 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-10 15:33 [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks Stefan Hanreich
2022-11-10 15:33 ` [pve-devel] [PATCH pve-docs 1/1] add pre/post-restore events to example hookscript Stefan Hanreich
2022-11-11 13:58   ` Daniel Tschlatscher
2022-11-10 15:33 ` [pve-devel] [PATCH pve-container 1/1] Add pre/post-restore hooks to CTs Stefan Hanreich
2022-11-10 15:33 ` [pve-devel] [PATCH qemu-server 1/1] Add pre/post-restore hooks to VMs Stefan Hanreich
2022-11-11 13:58 ` [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks Daniel Tschlatscher
2022-11-11 14:02   ` Stefan Hanreich
2022-11-11 14:16     ` Daniel Tschlatscher

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal