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

[Alwin Antreich]

On Tue, Oct 08, 2019 at 12:31:06PM +0200, Fabian Grünbichler wrote:
> On October 8, 2019 11:25 am, Alwin Antreich wrote:
> > 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.
> 
> true, I missed that (we use it for migration, but only with 'tcp' / 
> 'unix' as values). but in this case, we need to actually check whether 
> it is an absolute path, or a PVE-managed volume. in the latter case, we 
> can just push it to $vollist to get activation/deactivation handled like 
> the other volumes?
As Fabian and I talked off-list, here a recap and some questions.

The stateuri parameter can only be used by root. So we could re-use the
resume code if we only allow tcp / unix (migration) for the stateuri in
the vm_start API call. Then the suspend & snapshot vmstate could be
handled together.

Besides that someone might use it for arbitrary state loading. Would
there be any other reason not to be more restrictive?

Would this be considered a breaking change?

Thanks.