[pve-devel] [PATCH v7 qemu-server 10/10] Include "-cpu" parameter with snapshots/suspend

[Stefan Reiter]

On 1/27/20 9:54 AM, Thomas Lamprecht wrote:
> On 1/16/20 4:40 PM, 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.
> 
> few general things:
> 
> 1. wouldn't this belong before or in patch 03/10 to avoid temporary
>     breakage possibility? I mean a user has to actively enable it so
>     it could be OK, but still.

Rebasing 09/10 & 10/10 after 04/10 is the earliest it works without 
(trivial, but still) conflicts, but that should be fine as well (since 
only 06/10 allows a user to actually choose a custom CPU for a VM). So 
yes, I agree it makes sense to do that.

> 
> 2. Doesn't this need a bump of the perversion for the machine? Else
>     this fails on migration with due to the new option, which could
>     be confusing/seem like a bug. Maybe only allow/add the whole custom
>     thing with a bumped machine version?
> 

The only issue I see is with taking a snapshot and trying to restore on 
an older version? What scenario did you come up with where it breaks?


Slightly off-topic, but relevant: should it be possible to run newer 
+pveX machine types on older qemu-server versions? I.e. we currently 
check for QEMU version ("Installed QEMU version FOO is too old to run 
machine type BAR ..."), but accept a newer pveversion, say 
'pc-q35-4.1+pve42', without warning/error.

>>
>> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
>> ---
>>
>> New in v7.
>>
>>   PVE/API2/Qemu.pm  |  1 +
>>   PVE/QemuConfig.pm | 36 +++++++++++++++++++++++++++---------
>>   PVE/QemuServer.pm | 28 ++++++++++++++++++++--------
>>   3 files changed, 48 insertions(+), 17 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index f0b9e58..c2f6513 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -1098,6 +1098,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 1ba728a..852c309 100644
>> --- a/PVE/QemuConfig.pm
>> +++ b/PVE/QemuConfig.pm
>> @@ -5,9 +5,10 @@ use warnings;
>>   
>>   use PVE::AbstractConfig;
>>   use PVE::INotify;
>> +use PVE::QemuServer;
>> +use PVE::QemuServer::CPUConfig;
>>   use PVE::QemuServer::Helpers;
>>   use PVE::QemuServer::Monitor qw(mon_cmd);
>> -use PVE::QemuServer;
>>   use PVE::QemuServer::Machine;
>>   use PVE::Storage;
>>   use PVE::Tools;
>> @@ -156,15 +157,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;
>>   }
>>   
>> @@ -310,6 +322,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).
>> @@ -362,7 +379,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, $data->{forcecpu});
>>   }
>>   
>>   sub __snapshot_rollback_get_unused {
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 4a5dfac..d7b9c09 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -532,8 +532,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.",
>> @@ -2364,7 +2371,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};
>>       }
>>   
>> @@ -5231,8 +5239,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";
>>   	}
>>   
>> @@ -5478,7 +5487,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);
>>   	}
>>   
>> @@ -5491,13 +5500,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
>>   
>> @@ -5506,7 +5517,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);
>>   }
>> @@ -5780,7 +5792,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 $@;
>>
>