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

[Fabian Ebner]

On 06.04.20 15:01, Stefan Reiter wrote:
> On 06/04/2020 14:31, Fabian Ebner wrote:
>> 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.
>>
> 
> Was that a thing before? It should have worked that way anyhow, since 
> the 'cpu' setting was stored in the snapshot's section in the config.
> 

Ah, you're right. As long as the defaults for the standard models don't 
change, vm_start will re-generate the same -cpu option when no custom 
model is 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 $@;
>>>