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

[Alwin Antreich]

On Mon, Oct 14, 2019 at 11:44:59AM +0200, Thomas Lamprecht wrote:
> On 10/11/19 1:45 PM, Alwin Antreich wrote:
> > 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
> 
> this is also API breakage, or? Why not a simple path check fallback in cfg2cmd?
That's what I am not sure about, see my earlier email. Should I check
for file/device paths or just drop it?
https://pve.proxmox.com/pipermail/pve-devel/2019-October/039465.html

> 
> >>>>> 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(-)
> >>>>>
> 
> >>>>
> 
> >>>>>  	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.
> > 
> 
> but we could move that below?
> Do a @extra_cmd (or the like) and have then one unified statefile handling?
> I mean this method is already very big and the more things inside are spread
> out the harder it's to maintain it..
Yes, I will go ahead and put this into v3.

> 
> Also this makes your "It uses the vmstate and de-/activates the state volume
> in vm_start" sentence from the commit message false, as it's activated in
> config_to_command not vm_start ...
True, I will rewrite the commit message in v3 to also better reflect the
outcome of the discussion.