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

[Thomas Lamprecht]

On 3/11/19 3:45 PM, 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>
> ---
>  PVE/QemuServer.pm | 69 ++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 61 insertions(+), 8 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 02bb798..46620de 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)],
>      },
>      cpulimit => {
>  	optional => 1,
> @@ -5658,7 +5658,7 @@ sub vm_stop {
>  }
>  
>  sub vm_suspend {
> -    my ($vmid, $skiplock) = @_;
> +    my ($vmid, $skiplock, $includestate) = @_;
>  
>      PVE::QemuConfig->lock_config($vmid, sub {
>  
> @@ -5668,6 +5668,49 @@ sub vm_suspend {
>  	    if !($skiplock || PVE::QemuConfig->has_lock($conf, 'backup'));
>  
>  	vm_mon_cmd($vmid, "stop");
> +
> +	if ($includestate) {
> +	    $conf->{lock} = 'suspend';
> +	    PVE::QemuConfig->write_config($vmid, $conf);
> +
> +	    # save vm state
> +	    my $storecfg = PVE::Storage::config();
> +	    my $vmstate = prepare_save_vmstate($vmid, $conf, undef, $storecfg);
> +	    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 $stat = PVE::QemuServer::vm_mon_cmd_nocheck($vmid, "query-savevm");

s/stat/state/ ? 

> +		    if (!$stat->{status}) {
> +			die "savevm not active\n";
> +		    } elsif ($stat->{status} eq 'active') {
> +			sleep(1);
> +			next;
> +		    } elsif ($stat->{status} eq 'completed') {
> +			last;
> +		    } else {
> +			die "query-savevm returned status '$stat->{status}'\n";
> +		    }
> +		}
> +	    };
> +	    if (my $err = $@) {
> +		# cleanup
> +		warn $err;
> +		eval {
> +		    vm_mon_cmd($vmid, "savevm-end");
> +		    PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
> +		    PVE::Storage::vdisk_free($storecfg, $vmstate);

keeping the lock though?

> +		};
> +		warn $@ if $@;
> +		return;
> +	    }
> +
> +	    # save changes and write config
> +	    PVE::QemuConfig->write_config($vmid, $conf);
> +	    vm_qmp_command($vmid, { execute => "quit" });
> +	}
>      });
>  }
>  
> @@ -7103,8 +7146,6 @@ sub nbd_stop {
>  sub prepare_save_vmstate {
>      my ($vmid, $conf, $snapname, $storecfg) = @_;
>  
> -    my $snap = $conf->{snapshots}->{$snapname};
> -
>      # first, use explicitly configured storage
>      my $target = $conf->{vmstatestorage};
>  
> @@ -7125,12 +7166,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 $name = "vm-$vmid-state-$snapname";
>      my $scfg = PVE::Storage::storage_config($storecfg, $target);
> +
> +    my $name = "vm-$vmid-state-" . ($snapname // 'suspend');

'suspend' is a valid snapshot name, shouldn't this be a little more robust?

>      $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 (defined($snapname)) {

I do not really like this implicit behaviour decided through the defindness of $snapname.
A more direct, easer to read out could be nicer. E.g., an additional function parameter.
$snapname could be always passed, either the real $snapname or some unique "$suspend-$datetime"
name?

> +	my $snap = $conf->{snapshots}->{$snapname};
> +	$snap->{vmstate} = $statefile;
> +	$snap->{runningmachine} = $runningmachine;
> +    } else {
> +	$conf->{vmstate} = $statefile;
> +	$conf->{runningmachine} = $runningmachine;
> +    }
> +
> +    return $statefile;
>  }
>  
>  # bash completion helper
>