On October 7, 2019 2:41 pm, Alwin Antreich wrote:
> Machine states that were created on snapshots with memory could not be
> restored on rollback. The state volume was not activated so KVM couldn't
> load the state.
>
> This patch moves the path generation into vm_start and de-/activates the
> state volume.
alternatively, the following could also work and re-use more code so
that we don't miss the next special handling of some corner case.
rolling back from a snapshot with state is just like resuming, but we
want to keep the statefile instead of deleting it.
(untested):
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index edbf1a7..b70c276 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -358,9 +358,7 @@ sub __snapshot_rollback_vm_stop {
sub __snapshot_rollback_vm_start {
my ($class, $vmid, $vmstate, $data) = @_;
- my $storecfg = PVE::Storage::config();
- my $statefile = PVE::Storage::path($storecfg, $vmstate);
- PVE::QemuServer::vm_start($storecfg, $vmid, $statefile, undef, undef,
undef, $data->{forcemachine});
+ PVE::QemuServer::vm_start($storecfg, $vmid, $vmstate, undef, undef, undef,
$data->{forcemachine});
}
sub __snapshot_rollback_get_unused {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 8376260..f2d19e1 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5418,6 +5418,11 @@ sub vm_start {
print "Resuming suspended VM\n";
}
+ if ($statefile && $statefile ne 'tcp' && $statefile ne 'unix') {
+ # re-use resume code
+ $conf->{vmstate} = $statefile;
+ }
+
my ($cmd, $vollist, $spice_port) = config_to_command($storecfg, $vmid,
$conf, $defaults, $forcemachine);
my $migrate_port = 0;
@@ -5465,8 +5470,6 @@ sub vm_start {
push @$cmd, '-incoming', $migrate_uri;
push @$cmd, '-S';
- } else {
- push @$cmd, '-loadstate', $statefile;
}
} elsif ($paused) {
push @$cmd, '-S';
@@ -5616,11 +5619,16 @@ sub vm_start {
property => "guest-stats-polling-interval",
value => 2) if (!defined($conf->{balloon}) ||
$conf->{balloon});
- if ($is_suspended && (my $vmstate = $conf->{vmstate})) {
- print "Resumed VM, removing state\n";
- delete $conf->@{qw(lock vmstate runningmachine)};
+ if (my $vmstate = $conf->{vmstate}) {
PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
- PVE::Storage::vdisk_free($storecfg, $vmstate);
+ delete $conf->{vmstate};
+
+ if ($is_suspended) {
+ print "Resumed VM, removing state\n";
+ delete $conf->@{qw(lock runningmachine)};
+ PVE::Storage::vdisk_free($storecfg, $vmstate);
+ }
+
PVE::QemuConfig->write_config($vmid, $conf);
}
>
> Signed-off-by: Alwin Antreich <a.antre...@proxmox.com>
> ---
> PVE/QemuConfig.pm | 3 +--
> PVE/QemuServer.pm | 10 +++++++++-
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index edbf1a7..e9796a3 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -359,8 +359,7 @@ sub __snapshot_rollback_vm_start {
> my ($class, $vmid, $vmstate, $data) = @_;
>
> my $storecfg = PVE::Storage::config();
> - my $statefile = PVE::Storage::path($storecfg, $vmstate);
> - PVE::QemuServer::vm_start($storecfg, $vmid, $statefile, undef, undef,
> undef, $data->{forcemachine});
> + PVE::QemuServer::vm_start($storecfg, $vmid, $vmstate, undef, undef,
> undef, $data->{forcemachine});
> }
>
> sub __snapshot_rollback_get_unused {
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 8376260..39315b3 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5420,6 +5420,7 @@ sub vm_start {
>
> my ($cmd, $vollist, $spice_port) = config_to_command($storecfg, $vmid,
> $conf, $defaults, $forcemachine);
>
> +
> my $migrate_port = 0;
> my $migrate_uri;
> if ($statefile) {
> @@ -5466,7 +5467,12 @@ sub vm_start {
> push @$cmd, '-S';
>
> } else {
> - push @$cmd, '-loadstate', $statefile;
> + my $sfile = $statefile;
> + if (!-e $statefile) {
> + PVE::Storage::activate_volumes($storecfg, [$statefile]);
> + $sfile = PVE::Storage::path($storecfg, $statefile);
> + }
why only activate if the file does not exist? $statefile should be a
full volume ID including storage at this point, it does not make sense
to check for existence?
> + push @$cmd, '-loadstate', $sfile;
> }
> } elsif ($paused) {
> push @$cmd, '-S';
> @@ -5622,6 +5628,8 @@ sub vm_start {
> PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
> PVE::Storage::vdisk_free($storecfg, $vmstate);
> PVE::QemuConfig->write_config($vmid, $conf);
> + } elsif ($statefile && (!-e $statefile)) {
> + PVE::Storage::deactivate_volumes($storecfg, [$statefile]);
this check looks wrong? we should always deactivate, no need to check
whether it does not exist? (and like above, it always does not exist
AFAICT?)
> }
>
> PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'post-start');
> --
> 2.20.1
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
