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

[Alwin Antreich]

On Fri, Oct 11, 2019 at 07:10:53AM +0200, Thomas Lamprecht wrote:
> On 10/10/19 3:58 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 removes the path generation on rollback. It uses the vmstate
> > and de-/activates the state volume in vm_start. This in turn disallows
> > the use of path based statefiles when used with the '--stateuri' option
> > on 'qm start'. Only 'tcp', 'unix' and our storage based URIs can be
> > used now.
> > 
> > Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> > ---
> >  PVE/QemuConfig.pm | 3 +--
> >  PVE/QemuServer.pm | 8 +++++---
> >  2 files changed, 6 insertions(+), 5 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 ac9dfde..d4feae9 100644
> > --- a/PVE/QemuServer.pm
> > +++ b/PVE/QemuServer.pm
> > @@ -5340,6 +5340,7 @@ sub vm_start {
> >  	die "you can't start a vm if it's a template\n" if PVE::QemuConfig->is_template($conf);
> >  
> >  	my $is_suspended = PVE::QemuConfig->has_lock($conf, 'suspended');
> > +	$conf->{vmstate} = $statefile if ($statefile && $statefile ne 'tcp' && $statefile ne 'unix');
> 
> why? I mean you then get it out of this hash in the same submethod, i.e,
> same scope, below again? 
No, the config_to_command takes care of activation and path generation
of the vmstate. The same way as the resume of a hibernated VM.

> 
> And even if you'd need it and you just decided to not explain why
> in the commit message it would be still better to get it ...
> 
> >  
> >  	PVE::QemuConfig->check_lock($conf)
> >  	    if !($skiplock || $is_suspended);
> > @@ -5465,8 +5466,6 @@ sub vm_start {
> >  		push @$cmd, '-incoming', $migrate_uri;
> >  		push @$cmd, '-S';
> >  
> > -	    } else {
> > -		push @$cmd, '-loadstate', $statefile;
> 
> ... here, as we really have exact the condition you checked
> above: $statefile truthy, but neither 'tcp' or 'unix'...
> 
> But as said, I'd rather not have it in the $conf (which can get written out
> again) but maybe rather:
> 
> $statefile //= $conf->{vmstate};
> 
> and then just use $statefile... (I mean rename it to $vmstate, if you want)
My first version was in this intention. After talking with Fabain G., I
made the v2, to re-use the same method as the resume of an hibernated
VM. I have no bias here, either way is fine for me.

> 
> >  	    }
> >  	} elsif ($paused) {
> >  	    push @$cmd, '-S';
> > @@ -5616,12 +5615,15 @@ sub vm_start {
> >  		    property => "guest-stats-polling-interval",
> >  		    value => 2) if (!defined($conf->{balloon}) || $conf->{balloon});
> >  
> > -	if ($is_suspended && (my $vmstate = $conf->{vmstate})) {
> > +	my $vmstate = $conf->{vmstate};
> > +	if ($is_suspended && $vmstate) {
> >  	    print "Resumed VM, removing state\n";
> >  	    delete $conf->@{qw(lock vmstate runningmachine)};
> >  	    PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
> >  	    PVE::Storage::vdisk_free($storecfg, $vmstate);
> >  	    PVE::QemuConfig->write_config($vmid, $conf);
> > +	} elsif ($vmstate) {
> > +	    PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
> >  	}
> 
> to be more clear that we always want to deactivate and for nicer code
> in general I'd do:
> 
>         if ($vmstate) {
>             # always deactive vmstate volume again!
>             PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
>             if ($is_suspended) {
>                 print "Resumed VM, removing state\n";
>                 delete $conf->@{qw(lock vmstate runningmachine)};
>                 PVE::Storage::vdisk_free($storecfg, $vmstate);
>                 PVE::QemuConfig->write_config($vmid, $conf);
>             }
>         }
> 
> 
> 
> As then you have a clear linear flow in the if branches.
> (note: $vmstate is $statefile, or whatever we call it then)
Yes, that looks better then my version. :)

Thanks for reviewing.