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

[Alwin Antreich]

On Fri, Oct 11, 2019 at 12:17:28PM +0200, Thomas Lamprecht wrote:
> On 10/11/19 12:02 PM, Alwin Antreich wrote:
> > 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.
> > 
> 
> then write/explain such things in the commit message...
I will add this to the next version.

> 
> >>
> >> 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.
> 
> but you can still do it here even if you put it in the config, here is the
> correct place to do:
> 
> $conf->{vmstate} //= $statefile; 
Do you mean by "correct place", the else clause with the "--loadstate"?
It can't go there because the config_to_command has to happen before, as
it assigns the @$cmd. The other options are then pushed in addition to the
@$cmd, if the statefile is equal to tcp or unix.

> 
> I.e., I never said you should go back and handle it in your v1.. I approve
> with Fabians suggestion. The above postif just seemed out-of-place, especially
> if we have a $statefile handling already here..
Prior to this patch, the $statefile contained a path to the state
file/image on rollback of a snapshot and the loadstate could directly
take the $statefile. But the storage was not activated, eg. this made the
loadstate fail on RBD. Because of the config_to_command I placed the
assignment close to the top, where the config is loaded.