[pve-devel] [PATCH qemu-server v2 3/6] implement suspend to disk for running vms

[Thomas Lamprecht]

On 3/13/19 9:55 AM, Dominik Csapak wrote:
> the idea is to have the same logic as with snapshots, but without
> the snapshotting of the disks, and after saving the vm state (incl memory),
> we hard shut off the guest.
> 
> this way the disks will not be touched anymore by the guest
> 
> to prevent any alteration of the vm (incl migration, hw changes, etc) we
> add a config lock 'suspend'
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---

> changes from v1:
> * handle backup lock better (abort in that case)
> * use 'suspend-YYYY-MM-DD' as state name
> * remove lock on error
> * s/stat/state/

>  PVE/QemuServer.pm | 77 ++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 68 insertions(+), 9 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 02bb798..3a54671 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -300,7 +300,7 @@ my $confdesc = {
>  	optional => 1,
>  	type => 'string',
>  	description => "Lock/unlock the VM.",
> -	enum => [qw(backup clone create migrate rollback snapshot snapshot-delete)],
> +	enum => [qw(backup clone create migrate rollback snapshot snapshot-delete suspend)],

nit: maybe call the lock 'suspended'? Underlines that most of the time this is set
the suspend was already done?

>      },
>      cpulimit => {
>  	optional => 1,
> @@ -5658,16 +5658,65 @@ sub vm_stop {
>  }
>  
>  sub vm_suspend {
> -    my ($vmid, $skiplock) = @_;
> +    my ($vmid, $skiplock, $includestate) = @_;
>  
>      PVE::QemuConfig->lock_config($vmid, sub {
>  
>  	my $conf = PVE::QemuConfig->load_config($vmid);
>  
> +	my $is_backing_up = PVE::QemuConfig->has_lock($conf, 'backup');
>  	PVE::QemuConfig->check_lock($conf)
> -	    if !($skiplock || PVE::QemuConfig->has_lock($conf, 'backup'));
> +	    if !($skiplock || $is_backing_up);
> +
> +	die "cannot suspend to disk during backup\n"
> +	    if $is_backing_up && $includestate;
>  
>  	vm_mon_cmd($vmid, "stop");
> +
> +	if ($includestate) {
> +	    $conf->{lock} = 'suspend';
> +	    PVE::QemuConfig->write_config($vmid, $conf);

you're in a lock_config closure here which uses lock_file_full which is intended for
rather short things to do. Maybe it makes sense to get out of that method here and
do the rest of the "$includestate == true" code path in the config-locked, but not
flocked context?

Maybe it could then also make sense to have two step lock approach with "suspending"
and "suspended" lock. In failure cases one then has also the info if it failed during
or after the suspend was done. What do you think?

> +
> +	    # save vm state
> +	    my $date = strftime("%Y-%m-%d", localtime(time()));
> +	    my $storecfg = PVE::Storage::config();
> +	    my $vmstate = prepare_save_vmstate($vmid, $conf, "suspend-$date", $storecfg, 1);
> +	    my $path = PVE::Storage::path($storecfg, $vmstate);
> +	    PVE::Storage::activate_volumes($storecfg, [$vmstate]);
> +
> +	    eval {
> +		PVE::QemuServer::vm_mon_cmd($vmid, "savevm-start", statefile => $path);
> +		for(;;) {
> +		    my $state = PVE::QemuServer::vm_mon_cmd_nocheck($vmid, "query-savevm");
> +		    if (!$state->{status}) {
> +			die "savevm not active\n";
> +		    } elsif ($state->{status} eq 'active') {
> +			sleep(1);
> +			next;
> +		    } elsif ($state->{status} eq 'completed') {
> +			last;
> +		    } else {
> +			die "query-savevm returned status '$state->{status}'\n";
> +		    }
> +		}
> +	    };
> +	    if (my $err = $@) {
> +		# cleanup
> +		warn $err;
> +		eval {
> +		    delete $conf->{lock};

you delete the lock but have return below, so the "write_config" a few lines below never gets hit,
is there another "write_config" in the return path?

> +		    vm_mon_cmd($vmid, "savevm-end");
> +		    PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
> +		    PVE::Storage::vdisk_free($storecfg, $vmstate);
> +		};
> +		warn $@ if $@;
> +		return;
> +	    }
> +
> +	    # save changes and write config
> +	    PVE::QemuConfig->write_config($vmid, $conf);
> +	    vm_qmp_command($vmid, { execute => "quit" });
> +	}
>      });
>  }
>  
> @@ -7101,9 +7150,7 @@ sub nbd_stop {
>  }
>  
>  sub prepare_save_vmstate {
> -    my ($vmid, $conf, $snapname, $storecfg) = @_;
> -
> -    my $snap = $conf->{snapshots}->{$snapname};
> +    my ($vmid, $conf, $snapname, $storecfg, $suspend) = @_;
>  
>      # first, use explicitly configured storage
>      my $target = $conf->{vmstatestorage};
> @@ -7125,12 +7172,24 @@ sub prepare_save_vmstate {
>  
>      my $driver_state_size = 500; # assume 500MB is enough to safe all driver state;
>      my $size = $conf->{memory}*2 + $driver_state_size;
> +    my $scfg = PVE::Storage::storage_config($storecfg, $target);
>  
>      my $name = "vm-$vmid-state-$snapname";
> -    my $scfg = PVE::Storage::storage_config($storecfg, $target);
>      $name .= ".raw" if $scfg->{path}; # add filename extension for file base storage
> -    $snap->{vmstate} = PVE::Storage::vdisk_alloc($storecfg, $target, $vmid, 'raw', $name, $size*1024);
> -    $snap->{runningmachine} = PVE::QemuServer::get_current_qemu_machine($vmid);
> +
> +    my $statefile = PVE::Storage::vdisk_alloc($storecfg, $target, $vmid, 'raw', $name, $size*1024);
> +    my $runningmachine = PVE::QemuServer::get_current_qemu_machine($vmid);
> +
> +    if ($suspend) {
> +	$conf->{vmstate} = $statefile;
> +	$conf->{runningmachine} = $runningmachine;
> +    } else {
> +	my $snap = $conf->{snapshots}->{$snapname};
> +	$snap->{vmstate} = $statefile;
> +	$snap->{runningmachine} = $runningmachine;
> +    }
> +
> +    return $statefile;
>  }
>  
>  # bash completion helper
>