On 12/21/22 11:44, Fabian Grünbichler wrote:
this is v2, right? ;)
Oh no - for some reason it's only in the cover letter..
On December 12, 2022 2:43 pm, Stefan Hanreich wrote:
This commit adds hooks to the snapshotting process, which can be used
to run additional setup scripts to prepare the VM for snapshotting.
Examples for use cases include:
* forcing processes to flush their writes
* blocking processes from writing
* altering the configuration of the VM to make snapshotting possible
The prepare step has been split into two parts, so the configuration
can be locked a bit earlier during the snapshotting process. Doing it
this way ensures that the configuration is already locked during the
pre-snapshot hook. Because of this split, the VM config gets written
in two stages now, rather than one.
In case of failure during the preparation step - after the lock is
written - error handling has been added so the lock gets released
properly. The failed-snapshot hook runs when the snapshot fails, if
the pre-snapshot hook ran already. This enables users to revert any
changes done during the pre-snapshot hookscript.
see below
The preparation step assumes that the hook does not convert the
current VM into a template, which is why the basic checks are not
re-run after the pre-snapshot hook. The storage check runs after the
pre-snapshot hook, because the hook might get used to setup the
storage for snapshotting. If the hook would run after the storage
checks, this becomes impossible.
cfs_update() gets called after every invocation of a hookscript, since
it is impossible to know which changes get made by the hookscript.
Doing this ensures that we see the updated state of the CFS after the
hookscript got invoked.
Signed-off-by: Stefan Hanreich <s.hanre...@proxmox.com>
---
src/PVE/AbstractConfig.pm | 49 ++++++++++++++++++++++++++++++++++-----
1 file changed, 43 insertions(+), 6 deletions(-)
diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
index a0c0bc6..3bff600 100644
--- a/src/PVE/AbstractConfig.pm
+++ b/src/PVE/AbstractConfig.pm
@@ -710,8 +710,7 @@ sub __snapshot_prepare {
my $snap;
- my $updatefn = sub {
-
+ my $run_checks = sub {
my $conf = $class->load_config($vmid);
die "you can't take a snapshot if it's a template\n"
@@ -721,15 +720,21 @@ sub __snapshot_prepare {
$conf->{lock} = 'snapshot';
- my $snapshots = $conf->{snapshots};
-
die "snapshot name '$snapname' already used\n"
- if defined($snapshots->{$snapname});
+ if defined($conf->{snapshots}->{$snapname});
+
+ $class->write_config($vmid, $conf);
+ };
+ my $updatefn = sub {
+ my $conf = $class->load_config($vmid);
my $storecfg = PVE::Storage::config();
+
die "snapshot feature is not available\n"
if !$class->has_feature('snapshot', $conf, $storecfg, undef, undef,
$snapname eq 'vzdump');
+ my $snapshots = $conf->{snapshots};
+
for my $snap (sort keys %$snapshots) {
my $parent_name = $snapshots->{$snap}->{parent} // '';
if ($snapname eq $parent_name) {
@@ -753,7 +758,32 @@ sub __snapshot_prepare {
$class->write_config($vmid, $conf);
};
- $class->lock_config($vmid, $updatefn);
+ $class->lock_config($vmid, $run_checks);
+
+ eval {
+ my $conf = $class->load_config($vmid);
+ PVE::GuestHelpers::exec_hookscript($conf, $vmid, "pre-snapshot", 1);
+ };
+ my $err = $@;
+
+ PVE::Cluster::cfs_update();
+
+ if ($err) {
+ $class->remove_lock($vmid, 'snapshot');
+ die $err;
+ }
+
I wonder if we don't also want to call the 'failed-snapshot' phase when just the
pre-snapshot invocation failed? might be possible to combine the error handling
then, although I am not sure it makes it more readable if combined..
I thought about it, but I thought that if the user die's in his perl
script he should be able to run any cleanup code before that. This
doesn't consider any problems in the hookscript unforeseen by the user
though, so I think your approach is better, since it is easier to use.
This places less burden on the author of the hookscript. Might make the
code a bit more convoluted though (depending on how we want to handle
errors in failed-snapshot), but the upsides are way better imo.
One thing that would be easier with making the user do his cleanup in
pre-snapshot would be that the pre-snapshot hook knows exactly what
failed in pre-snapshot, so cleanup-code could use that information to
skip certain steps. But again, it assumes that pre-snapshot will
properly handle any possible error, which might be a bit much to assume.
+
+ if (my $err = $@) {
+ my $conf = $class->load_config($vmid);
+ PVE::GuestHelpers::exec_hookscript($conf, $vmid, "failed-snapshot");
this exec_hookscript needs to be inside an eval {}, with warn in case it fails..
Isn't this already handled by the exec_hookscript function, since I am
not passing $stop_on_error ? It should exit with warn instead of die
then. Maybe I am misunderstanding something.
See:
https://git.proxmox.com/?p=pve-guest-common.git;a=blob;f=src/PVE/GuestHelpers.pm;h=b4ccbaa73a3fd08ba5d34350ebd57ee31355035b;hb=HEAD#l125
also, this call here happens when preparing for making the snapshot, after
possibly saving the VM state, but before taking the volume snapshots..
This should be alleviated by the envvars you proposed below, because
then we could pass that information to the hookscript and the user
decides what to do with this information, right?
+ PVE::Cluster::cfs_update();
+
+ $class->remove_lock($vmid, 'snapshot');
+ die $err;
+ }
return $snap;
}
@@ -837,11 +867,18 @@ sub snapshot_create {
if ($err) {
warn "snapshot create failed: starting cleanup\n";
+
+ PVE::GuestHelpers::exec_hookscript($conf, $vmid, "failed-snapshot");
eval + warn as well
see above
this call here happens when the volume snapshots might or might not have been
created already (depending on what exactly the error cause is).
same here - should be alleviated by adding envvars, right?
+ PVE::Cluster::cfs_update();
+
eval { $class->snapshot_delete($vmid, $snapname, 1, $drivehash); };
warn "$@" if $@;
die "$err\n";
}
+ PVE::GuestHelpers::exec_hookscript($conf, $vmid, "post-snapshot");
and here we have a similar issue (no eval), what should happen if post-snapshot
fails?
A die immediately (very likely wrong, current)
B eval + warn but proceed with commit (possibly leaving leftover hook changes
around)
C eval + warn, call failed-snapshot but proceed with commit (gives the
hookscript a chance to cleanup, but how does it differentiate between the
different failed-snapshot call sites?)
D eval + delete snapshot (seems suboptimal)
E eval + call failed-snapshot + delete snapshot (same, and also the issue of the
hookscript being able to know what's going on again)
B and C seem most sensible to me, but C adds to the issue of "missing
failed-snapshot context", depending on what the hookscript is doing..
again, see above - I think it currently actually behaves like B because
of how exec_hookscript works if I understand correctly.
Similar idea to not running the failed-snapshot hook if pre-snapshot
fails. I thought that the user should be aware that his hookscript
failed at some point and run possible cleanup code before returning. As
I said above that's probably a worse idea than just running
failed-snapshot. It also enables the user to just have all the cleanup
handled by failed-snapshot instead of having to add it to pre/post/failed.
one way to pass information is via the environment, we do that for the migration
case already (setting PVE_MIGRATED_FROM, so that the pre-start/post-start
hookscript can know the start happens in a migration context, and where to
(possibly) find the guest config..
for example, we could set PVE_SNAPSHOT_PHASE here, and have prepare/commit/post
as sub-phases, or even pass a list of volumes already snapshotted (or created,
in case of vmstate), or
That's a good idea, I'll look into sensible values for
PVE_SNAPSHOT_PHASE as well as look into how we could pass the
information about volumes to the hookscript best.
obviously setting the environment is only allowed in a forked worker context,
else it would affect the next API endpoint handled by the pveproxy/pvedaemon/..
process, so it might be worth double-checking and cleaning up to avoid
side-effects with replication/migration/.. if we go down that route..
very good remark - thanks. I would not have thought of it even though it
is kinda obvious now you pointed it out.
+ PVE::Cluster::cfs_update();
+
$class->__snapshot_commit($vmid, $snapname);
}
--
2.30.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Many thanks for the review!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
