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

[Fabian Grünbichler]

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?

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