[pve-devel] [PATCH qemu-server] Fix #2171: VM statefile was not activated

[Fabian Grünbichler]

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.antreich at 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 at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>