[pve-devel] [PATCH pve-guest-common/pve-docs 0/1]

[pve-devel] [PATCH pve-guest-common/pve-docs 0/1]

[Stefan Hanreich]

This patch adds hooks that run when the user creates a snapshot 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-guest-common:

Stefan Hanreich (1):
  add pre/post-snapshot hooks

 src/PVE/AbstractConfig.pm | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

pve-docs:

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

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

-- 
2.30.2

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

[Stefan Hanreich]

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

The example script currently enumerates the different phases (first, second,
...). I have opted to not continue this enumeration as I couldn't see any
particular reason for this and I will add lots of new phases in subsequent patch
series. Am I missing a particular reason for the numbering? I think it might be
smart to create another patch after merging all the different hook patches, that
cleans up the comments/ordering in the example script and removes the
enumerations.

 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..e4f032b 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-snapshot') {
+
+    # Phase 'pre-snapshot' will be executed before taking a snapshot of
+    # the guest (via UI or CLI)
+
+    print "$vmid will be snapshotted.\n";
+
+} elsif ($phase eq 'post-snapshot') {
+
+    # Phase 'post-snapshot' will be executed after taking a snapshot of
+    # the guest (via UI or CLI)
+
+    print "$vmid has been successfully snapshotted.\n";
+
 } else {
     die "got unknown phase '$phase'\n";
 }
-- 
2.30.2

[pve-devel] [PATCH pve-guest-common 1/1] add pre/post-snapshot hooks

[Stefan Hanreich]

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

I have opted to include the snapshot hooks in the abstract class, since this
seemed like the more straightforward way.
The other option would have been duplicating the calls of the hooks into the
respective Backends, but I couldn't see any upsides to this.

This hook runs before the preparation steps, since otherwise in case of a
failing pre-snapshot hook the VM/CT is left in a locked state, which I would
need to clean up, which would add unnecessary complexity imo.

Otoh, there is a case to be made that the hook should only run after we checked
every precondition and are as certain as we can be that the snapshot will be
successfully created. This would be more convenient from a user's pov.
This can be particularly convenient as it would avoid errors with user scripts
that are not idempotent. Although those would still fail in case of a failure
during the snapshotting process.

What do you think would be the better solution?

 src/PVE/AbstractConfig.pm | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
index a0c0bc6..8052fde 100644
--- a/src/PVE/AbstractConfig.pm
+++ b/src/PVE/AbstractConfig.pm
@@ -799,11 +799,15 @@ sub __snapshot_activate_storages {
 sub snapshot_create {
     my ($class, $vmid, $snapname, $save_vmstate, $comment) = @_;
 
+    my $conf = $class->load_config($vmid);
+    PVE::GuestHelpers::exec_hookscript($conf, $vmid, "pre-snapshot", 1);
+
     my $snap = $class->__snapshot_prepare($vmid, $snapname, $save_vmstate, $comment);
 
     $save_vmstate = 0 if !$snap->{vmstate};
 
-    my $conf = $class->load_config($vmid);
+    # reload config after changes in snapshot preparation step
+    $conf = $class->load_config($vmid);
 
     my ($running, $freezefs) = $class->__snapshot_check_freeze_needed($vmid, $conf, $snap->{vmstate});
 
@@ -843,6 +847,8 @@ sub snapshot_create {
     }
 
     $class->__snapshot_commit($vmid, $snapname);
+
+    PVE::GuestHelpers::exec_hookscript($conf, $vmid, "post-snapshot");
 }
 
 # Check if the snapshot might still be needed by a replication job.
-- 
2.30.2

Re: [pve-devel] [PATCH pve-guest-common 1/1] add pre/post-snapshot hooks

[Fiona Ebner]

Am 22.09.22 um 13:54 schrieb Stefan Hanreich:
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> 

Should there be a third hook that's called when the snapshot fails? That
would allow doing cleanup in all cases. Could still be added later when
actually requested by users of course.

What are the common use cases you have in mind for these hooks?

> I have opted to include the snapshot hooks in the abstract class, since this
> seemed like the more straightforward way.
> The other option would have been duplicating the calls of the hooks into the
> respective Backends, but I couldn't see any upsides to this.
> 
> This hook runs before the preparation steps, since otherwise in case of a
> failing pre-snapshot hook the VM/CT is left in a locked state, which I would
> need to clean up, which would add unnecessary complexity imo.
> 
> Otoh, there is a case to be made that the hook should only run after we checked
> every precondition and are as certain as we can be that the snapshot will be
> successfully created. This would be more convenient from a user's pov.
> This can be particularly convenient as it would avoid errors with user scripts
> that are not idempotent. Although those would still fail in case of a failure
> during the snapshotting process.

Calling the hook script only after setting the lock in the config would
add protection against other modifications happening at the same time.
Stupid example: if we add another such hook outside a lock, and both
that and the 'pre-snapshot' hook would modify the config, they could
interfere when happening right after another. But it doesn't need to be
another hook script, the config modification could also come from
somewhere else.

Although this approach does requires users to pass the --skiplock option
to modify the config if using our API/CLI. And we need to repeat the
checks after calling the hook script, because they might not apply any
more ;)

> 
> What do you think would be the better solution?
> 
>  src/PVE/AbstractConfig.pm | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
> index a0c0bc6..8052fde 100644
> --- a/src/PVE/AbstractConfig.pm
> +++ b/src/PVE/AbstractConfig.pm
> @@ -799,11 +799,15 @@ sub __snapshot_activate_storages {
>  sub snapshot_create {
>      my ($class, $vmid, $snapname, $save_vmstate, $comment) = @_;
>  
> +    my $conf = $class->load_config($vmid);
> +    PVE::GuestHelpers::exec_hookscript($conf, $vmid, "pre-snapshot", 1);
> +
>      my $snap = $class->__snapshot_prepare($vmid, $snapname, $save_vmstate, $comment);
>  
>      $save_vmstate = 0 if !$snap->{vmstate};
>  
> -    my $conf = $class->load_config($vmid);
> +    # reload config after changes in snapshot preparation step

I think there should be a cfs_update() call?

> +    $conf = $class->load_config($vmid);
>  
>      my ($running, $freezefs) = $class->__snapshot_check_freeze_needed($vmid, $conf, $snap->{vmstate});
>  
> @@ -843,6 +847,8 @@ sub snapshot_create {
>      }
>  
>      $class->__snapshot_commit($vmid, $snapname);
> +
> +    PVE::GuestHelpers::exec_hookscript($conf, $vmid, "post-snapshot");
>  }
>  
>  # Check if the snapshot might still be needed by a replication job.

Re: [pve-devel] [PATCH pve-guest-common 1/1] add pre/post-snapshot hooks

[Stefan Hanreich]

On 11/14/22 09:51, Fiona Ebner wrote:
> Am 22.09.22 um 13:54 schrieb Stefan Hanreich:
>> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
>> ---
>>
> 
> Should there be a third hook that's called when the snapshot fails? That
> would allow doing cleanup in all cases. Could still be added later when
> actually requested by users of course.
> 
> What are the common use cases you have in mind for these hooks?
> 

Quoting from the user in #2530:
Imagine hooks for pre- and post-snapshot too, it will helps with making 
consistent backups, clones, replication greatly, reduces recovery time 
after firing HA or backup restore… If one will be able to ensure that 
your databases and other stuff oflloads data correctly before snapshoting.

Apparently they want to use it to force DBs and similar applications to 
flush all their writes and maybe block new writes in the meanwhile.
Adding a failed-snapshot hook might be a good idea then, because in 
those cases one would still have to clean up some stuff that has been 
done in the pre-snapshot hook...

>> I have opted to include the snapshot hooks in the abstract class, since this
>> seemed like the more straightforward way.
>> The other option would have been duplicating the calls of the hooks into the
>> respective Backends, but I couldn't see any upsides to this.
>>
>> This hook runs before the preparation steps, since otherwise in case of a
>> failing pre-snapshot hook the VM/CT is left in a locked state, which I would
>> need to clean up, which would add unnecessary complexity imo.
>>
>> Otoh, there is a case to be made that the hook should only run after we checked
>> every precondition and are as certain as we can be that the snapshot will be
>> successfully created. This would be more convenient from a user's pov.
>> This can be particularly convenient as it would avoid errors with user scripts
>> that are not idempotent. Although those would still fail in case of a failure
>> during the snapshotting process.
> 
> Calling the hook script only after setting the lock in the config would
> add protection against other modifications happening at the same time.
> Stupid example: if we add another such hook outside a lock, and both
> that and the 'pre-snapshot' hook would modify the config, they could
> interfere when happening right after another. But it doesn't need to be
> another hook script, the config modification could also come from
> somewhere else.
> 
> Although this approach does requires users to pass the --skiplock option
> to modify the config if using our API/CLI. And we need to repeat the
> checks after calling the hook script, because they might not apply any
> more ;)
>

Yes, from this POV it's probably a smarter idea to do it this way. Not 
sure if we can trust the users to properly lock the config file in their 
hook scripts when editing the config. Definitely the safer option. 
Especially if you think about the case that the hookscript config option 
could also change sometime inbetween. That could lead to some weird 
behaviour.

I can actually see some use-cases for editing the config with regards to 
PCI passthrough. One could script removing / adding PCI-E devices in 
order to be able to take snapshots. Together with the restore hooks, one 
could even try scripting automatically detaching/attaching PCI-E devices 
when snapshotting/restoring. Not sure if this is practical though, 
because of the issues it could cause.

>>
>> What do you think would be the better solution?
>>
>>   src/PVE/AbstractConfig.pm | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
>> index a0c0bc6..8052fde 100644
>> --- a/src/PVE/AbstractConfig.pm
>> +++ b/src/PVE/AbstractConfig.pm
>> @@ -799,11 +799,15 @@ sub __snapshot_activate_storages {
>>   sub snapshot_create {
>>       my ($class, $vmid, $snapname, $save_vmstate, $comment) = @_;
>>   
>> +    my $conf = $class->load_config($vmid);
>> +    PVE::GuestHelpers::exec_hookscript($conf, $vmid, "pre-snapshot", 1);
>> +
>>       my $snap = $class->__snapshot_prepare($vmid, $snapname, $save_vmstate, $comment);
>>   
>>       $save_vmstate = 0 if !$snap->{vmstate};
>>   
>> -    my $conf = $class->load_config($vmid);
>> +    # reload config after changes in snapshot preparation step
> 
> I think there should be a cfs_update() call?

Yes, definitely. If I understand correctly load_config uses a cache to 
get already loaded configurations instead of hitting the disk everytime? 
That's why this is required?

> 
>> +    $conf = $class->load_config($vmid);
>>   
>>       my ($running, $freezefs) = $class->__snapshot_check_freeze_needed($vmid, $conf, $snap->{vmstate});
>>   
>> @@ -843,6 +847,8 @@ sub snapshot_create {
>>       }
>>   
>>       $class->__snapshot_commit($vmid, $snapname);
>> +
>> +    PVE::GuestHelpers::exec_hookscript($conf, $vmid, "post-snapshot");
>>   }
>>   
>>   # Check if the snapshot might still be needed by a replication job.

Re: [pve-devel] [PATCH pve-guest-common 1/1] add pre/post-snapshot hooks

[Fiona Ebner]

Am 17.11.22 um 12:27 schrieb Stefan Hanreich:> On 11/14/22 09:51, Fiona
Ebner wrote:
>> Am 22.09.22 um 13:54 schrieb Stefan Hanreich:
>>> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
>>> ---
>>>
>>
>> Should there be a third hook that's called when the snapshot fails? That
>> would allow doing cleanup in all cases. Could still be added later when
>> actually requested by users of course.
>>
>> What are the common use cases you have in mind for these hooks?
>>
> 
> Quoting from the user in #2530:
> Imagine hooks for pre- and post-snapshot too, it will helps with making
> consistent backups, clones, replication greatly, reduces recovery time
> after firing HA or backup restore… If one will be able to ensure that
> your databases and other stuff oflloads data correctly before snapshoting.
> 
> Apparently they want to use it to force DBs and similar applications to
> flush all their writes and maybe block new writes in the meanwhile.
> Adding a failed-snapshot hook might be a good idea then, because in
> those cases one would still have to clean up some stuff that has been
> done in the pre-snapshot hook...

Makes sense. Would be nice have a "partially close #2530" prefix in the
commit title ;) and maybe also briefly describe the use case in the
commit message.

So the proposed use-case is not about editing configs, but I'm sure some
users are going to want that too.

> 
>>> I have opted to include the snapshot hooks in the abstract class,
>>> since this
>>> seemed like the more straightforward way.
>>> The other option would have been duplicating the calls of the hooks
>>> into the
>>> respective Backends, but I couldn't see any upsides to this.
>>>
>>> This hook runs before the preparation steps, since otherwise in case
>>> of a
>>> failing pre-snapshot hook the VM/CT is left in a locked state, which
>>> I would
>>> need to clean up, which would add unnecessary complexity imo.
>>>
>>> Otoh, there is a case to be made that the hook should only run after
>>> we checked
>>> every precondition and are as certain as we can be that the snapshot
>>> will be
>>> successfully created. This would be more convenient from a user's pov.
>>> This can be particularly convenient as it would avoid errors with
>>> user scripts
>>> that are not idempotent. Although those would still fail in case of a
>>> failure
>>> during the snapshotting process.
>>
>> Calling the hook script only after setting the lock in the config would
>> add protection against other modifications happening at the same time.
>> Stupid example: if we add another such hook outside a lock, and both
>> that and the 'pre-snapshot' hook would modify the config, they could
>> interfere when happening right after another. But it doesn't need to be
>> another hook script, the config modification could also come from
>> somewhere else.
>>
>> Although this approach does requires users to pass the --skiplock option
>> to modify the config if using our API/CLI. And we need to repeat the
>> checks after calling the hook script, because they might not apply any
>> more ;)
>>
> 
> Yes, from this POV it's probably a smarter idea to do it this way. Not
> sure if we can trust the users to properly lock the config file in their
> hook scripts when editing the config. Definitely the safer option.
> Especially if you think about the case that the hookscript config option
> could also change sometime inbetween. That could lead to some weird
> behaviour.

Locking in the hookscript wouldn't even help, because you could have:
1. pre-snapshot hook locks config and finishes
2. hook for other operation locks and modifies config and finishes
3. snapshot operation locks config ending up with modified config

Well, I'd say if users change the hookscript option during the
hookscript, they are responsible for the consequences :P

> 
> I can actually see some use-cases for editing the config with regards to
> PCI passthrough. One could script removing / adding PCI-E devices in
> order to be able to take snapshots. Together with the restore hooks, one
> could even try scripting automatically detaching/attaching PCI-E devices
> when snapshotting/restoring. Not sure if this is practical though,
> because of the issues it could cause.
> 

If users *really* want to edit the config, they would just do it
manually. IMHO the need to use skiplock is not that big of a downside
compared to the upside of having protection against modifications from
concurrent operations. But if we go for that approach we should properly
test and document it of course :)

>>>
>>> What do you think would be the better solution?
>>>
>>>   src/PVE/AbstractConfig.pm | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
>>> index a0c0bc6..8052fde 100644
>>> --- a/src/PVE/AbstractConfig.pm
>>> +++ b/src/PVE/AbstractConfig.pm
>>> @@ -799,11 +799,15 @@ sub __snapshot_activate_storages {
>>>   sub snapshot_create {
>>>       my ($class, $vmid, $snapname, $save_vmstate, $comment) = @_;
>>>   +    my $conf = $class->load_config($vmid);
>>> +    PVE::GuestHelpers::exec_hookscript($conf, $vmid, "pre-snapshot",
>>> 1);
>>> +
>>>       my $snap = $class->__snapshot_prepare($vmid, $snapname,
>>> $save_vmstate, $comment);
>>>         $save_vmstate = 0 if !$snap->{vmstate};
>>>   -    my $conf = $class->load_config($vmid);
>>> +    # reload config after changes in snapshot preparation step
>>
>> I think there should be a cfs_update() call?
> 
> Yes, definitely. If I understand correctly load_config uses a cache to
> get already loaded configurations instead of hitting the disk everytime?
> That's why this is required?
> 

Yes.

Re: [pve-devel] [PATCH pve-guest-common/pve-docs 0/1]

[Daniel Tschlatscher]

The hookscripts for pre- and post- snapshot work as intended.
I tested creating snapshots in the GUI and on the CLI without finding
any problems.
When the hookscript fails the behavior was as I would expect it: An
error in 'pre-hookscript' fails the whole task. An error in the
'post-hookscript' states that an error in the hookscript occured, but
still ends with 'TASK OK', with the snapshot being created.


One general suggestion:
When the 'post-snapshot' hook fails it would be nice if a warning could
be printed. As the snapshot still succeeds in this case, just looking at
the task log list, it looks like the task finished without any problems.

This would require some changes in 'exec_hookscript' and probably for
other 'post-x' hooks as well, though, this is not in the scope of these
patches. I just thought to put it somewhere.


So, consider this series

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

On 9/22/22 13:54, Stefan Hanreich wrote:
> This patch adds hooks that run when the user creates a snapshot 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-guest-common:
> 
> Stefan Hanreich (1):
>   add pre/post-snapshot hooks
> 
>  src/PVE/AbstractConfig.pm | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> pve-docs:
> 
> Stefan Hanreich (1):
>   add pre/post snapshot events to example hookscript
> 
>  examples/guest-example-hookscript.pl | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>