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

[Alwin Antreich]

On Tue, Oct 08, 2019 at 08:36:57AM +0200, Fabian Grünbichler wrote:
> 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.
I will send another version with your alternative.

> 
> (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?
You can pass a stateuri on vm start (qm start <ID> --stateuri) and this
can be a file path. I added the if to not break this.

> 
> > +		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?)
True, besides that stateuri. :/