[pve-devel] [PATCH v9 qemu-server 2/6] Include "-cpu" parameter with snapshots/suspend

[Fabian Ebner]

On 26.03.20 16:13, Stefan Reiter wrote:
> Just like with live-migration, custom CPU models might change after a
> snapshot has been taken (or a VM suspended), which would lead to a
> different QEMU invocation on rollback/resume.
> 
> Save the "-cpu" argument as a new "runningcpu" option into the VM conf
> akin to "runningmachine" and use as override during rollback/resume.
> 
> No functional change with non-custom CPU types intended.
> 

The changes apply to all CPU types, but that's not a bad thing. If one 
takes a snapshot, restarts the VM with a different CPU type and does a 
rollback, it'll use the CPU at the time of the snapshot even if no 
custom models are involved.

> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
>   PVE/API2/Qemu.pm  |  1 +
>   PVE/QemuConfig.pm | 34 ++++++++++++++++++++++++++--------
>   PVE/QemuServer.pm | 28 ++++++++++++++++++++--------
>   3 files changed, 47 insertions(+), 16 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 80fd7ea..2c7e998 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -1129,6 +1129,7 @@ my $update_vm_api  = sub {
>   		push @delete, 'lock'; # this is the real deal to write it out
>   	    }
>   	    push @delete, 'runningmachine' if $conf->{runningmachine};
> +	    push @delete, 'runningcpu' if $conf->{runningcpu};
>   	}
>   
>   	PVE::QemuConfig->check_lock($conf) if !$skiplock;
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index 8d03774..386223d 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -5,6 +5,7 @@ use warnings;
>   
>   use PVE::AbstractConfig;
>   use PVE::INotify;
> +use PVE::QemuServer::CPUConfig;
>   use PVE::QemuServer::Drive;
>   use PVE::QemuServer::Helpers;
>   use PVE::QemuServer::Monitor qw(mon_cmd);
> @@ -155,15 +156,26 @@ sub __snapshot_save_vmstate {
>       my $statefile = PVE::Storage::vdisk_alloc($storecfg, $target, $vmid, 'raw', $name, $size*1024);
>       my $runningmachine = PVE::QemuServer::Machine::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;
> +    # get current QEMU -cpu argument
> +    my $runningcpu;
> +    if (my $pid = PVE::QemuServer::check_running($vmid)) {
> +	my $cmdline = PVE::QemuServer::Helpers::parse_cmdline($pid);
> +	die "could not read commandline of running machine\n"
> +	    if !$cmdline->{cpu}->{value};
> +
> +	# sanitize and untaint value
> +	$cmdline->{cpu}->{value} =~ $PVE::QemuServer::CPUConfig::qemu_cmdline_cpu_re;
> +	$runningcpu = $1;
> +    }
> +
> +    if (!$suspend) {
> +	$conf = $conf->{snapshots}->{$snapname};
>       }
>   
> +    $conf->{vmstate} = $statefile;
> +    $conf->{runningmachine} = $runningmachine;
> +    $conf->{runningcpu} = $runningcpu;
> +
>       return $statefile;
>   }
>   
> @@ -309,6 +321,11 @@ sub __snapshot_rollback_hook {
>   	if (defined($conf->{runningmachine})) {
>   	    $data->{forcemachine} = $conf->{runningmachine};
>   	    delete $conf->{runningmachine};
> +
> +	    # runningcpu is newer than runningmachine, so assume it only exists
> +	    # here, if at all
> +	    $data->{forcecpu} = delete $conf->{runningcpu}
> +		if defined($conf->{runningcpu});
>   	} else {
>   	    # Note: old code did not store 'machine', so we try to be smart
>   	    # and guess the snapshot was generated with kvm 1.4 (pc-i440fx-1.4).
> @@ -361,7 +378,8 @@ sub __snapshot_rollback_vm_start {
>       my ($class, $vmid, $vmstate, $data) = @_;
>   
>       my $storecfg = PVE::Storage::config();
> -    PVE::QemuServer::vm_start($storecfg, $vmid, $vmstate, undef, undef, undef, $data->{forcemachine});
> +    PVE::QemuServer::vm_start($storecfg, $vmid, $vmstate, undef, undef, undef,
> +	$data->{forcemachine}, undef, undef, undef, undef, undef, undef, $data->{forcecpu});
>   }
>   
>   sub __snapshot_rollback_get_unused {
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 98c2a9a..70a5234 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -567,8 +567,15 @@ EODESCR
>   	optional => 1,
>       }),
>       runningmachine => get_standard_option('pve-qemu-machine', {
> -	description => "Specifies the Qemu machine type of the running vm. This is used internally for snapshots.",
> +	description => "Specifies the QEMU machine type of the running vm. This is used internally for snapshots.",
>       }),
> +    runningcpu => {
> +	description => "Specifies the QEMU '-cpu' parameter of the running vm. This is used internally for snapshots.",
> +	optional => 1,
> +	type => 'string',
> +	pattern => $PVE::QemuServer::CPUConfig::qemu_cmdline_cpu_re,
> +	format_description => 'QEMU -cpu parameter'
> +    },
>       machine => get_standard_option('pve-qemu-machine'),
>       arch => {
>   	description => "Virtual processor architecture. Defaults to the host.",
> @@ -1948,7 +1955,8 @@ sub json_config_properties {
>       my $prop = shift;
>   
>       foreach my $opt (keys %$confdesc) {
> -	next if $opt eq 'parent' || $opt eq 'snaptime' || $opt eq 'vmstate' || $opt eq 'runningmachine';
> +	next if $opt eq 'parent' || $opt eq 'snaptime' || $opt eq 'vmstate' ||
> +	    $opt eq 'runningmachine' || $opt eq 'runningcpu';
>   	$prop->{$opt} = $confdesc->{$opt};
>       }
>   
> @@ -4802,8 +4810,9 @@ sub vm_start {
>   	PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'pre-start', 1);
>   
>   	if ($is_suspended) {
> -	    # enforce machine type on suspended vm to ensure HW compatibility
> +	    # enforce machine type and CPU on suspended vm to ensure HW compatibility
>   	    $forcemachine = $conf->{runningmachine};
> +	    $force_cpu = $conf->{runningcpu};
>   	    print "Resuming suspended VM\n";
>   	}
>   
> @@ -5058,7 +5067,7 @@ sub vm_start {
>   		PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
>   		PVE::Storage::vdisk_free($storecfg, $vmstate);
>   	    }
> -	    delete $conf->@{qw(lock vmstate runningmachine)};
> +	    delete $conf->@{qw(lock vmstate runningmachine runningcpu)};
>   	    PVE::QemuConfig->write_config($vmid, $conf);
>   	}
>   
> @@ -5071,13 +5080,15 @@ sub vm_commandline {
>   
>       my $conf = PVE::QemuConfig->load_config($vmid);
>       my $forcemachine;
> +    my $forcecpu;
>   
>       if ($snapname) {
>   	my $snapshot = $conf->{snapshots}->{$snapname};
>   	die "snapshot '$snapname' does not exist\n" if !defined($snapshot);
>   
> -	# check for a 'runningmachine' in snapshot
> -	$forcemachine = $snapshot->{runningmachine} if $snapshot->{runningmachine};
> +	# check for machine or CPU overrides in snapshot
> +	$forcemachine = $snapshot->{runningmachine};
> +	$forcecpu = $snapshot->{runningcpu};
>   
>   	$snapshot->{digest} = $conf->{digest}; # keep file digest for API
>   
> @@ -5086,7 +5097,8 @@ sub vm_commandline {
>   
>       my $defaults = load_defaults();
>   
> -    my $cmd = config_to_command($storecfg, $vmid, $conf, $defaults, $forcemachine);
> +    my $cmd = config_to_command($storecfg, $vmid, $conf, $defaults,
> +	$forcemachine, $forcecpu);
>   
>       return PVE::Tools::cmd2string($cmd);
>   }
> @@ -5360,7 +5372,7 @@ sub vm_suspend {
>   		    mon_cmd($vmid, "savevm-end");
>   		    PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
>   		    PVE::Storage::vdisk_free($storecfg, $vmstate);
> -		    delete $conf->@{qw(vmstate runningmachine)};
> +		    delete $conf->@{qw(vmstate runningmachine runningcpu)};
>   		    PVE::QemuConfig->write_config($vmid, $conf);
>   		};
>   		warn $@ if $@;
>