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.rei...@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 $@;



_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Reply via email to