[pve-devel] [PATCH qemu-server] implement PVE Version addition for QEMU machine
Stefan Reiter
s.reiter at proxmox.com
Mon Nov 25 12:33:31 CET 2019
Approach is correct IMO, probably the easiest way, and I agree that it
seems unlikely that QEMU's patch version will be important in the future.
Some stuff inline.
On 11/25/19 11:27 AM, Thomas Lamprecht wrote:
> With our QEMU 4.1.1 package we can pass a additional internal version
> to QEMU's machine, it will be split out there and ignored, but
> returned on a QMP 'query-machines' call.
>
> This allows us to use it for reducing the granularity with which we
> can roll-out HW layout changes/additions for VMs. Until now we
> required a machine version bump, happening normally every major
> release of QEMU, with seldom, for us irrelevant, exceptions.
> This often delays rolling out a feature, which would break
> live-migration, by several months. That can now be avoided, the new
> "pve-version" component of the machine can be bumped at will, and
> thus we are much more flexible.
>
> That versions orders after the ($major, $minor) version components
> from an stable release - it can thus also be reset on the next
> release.
>
> The implementation extends the qemu-machine REGEX, remembers
> "pve-version" when doing a "query-machines" and integrates support
> into the min_version and extract_version helpers.
>
> We start out with a version of 1.
>
> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> ---
> PVE/QemuServer.pm | 17 +++++++++-------
> PVE/QemuServer/Helpers.pm | 6 +++---
> PVE/QemuServer/Machine.pm | 27 ++++++++++++++++++++------
> test/cfg2cmd/minimal-defaults.conf.cmd | 2 +-
> test/cfg2cmd/spice-linux-4.1.conf.cmd | 2 +-
> 5 files changed, 36 insertions(+), 18 deletions(-)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index fcedcf1..78a0a02 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -93,7 +93,7 @@ PVE::JSONSchema::register_standard_option('pve-qm-image-format', {
> PVE::JSONSchema::register_standard_option('pve-qemu-machine', {
> description => "Specifies the Qemu machine type.",
> type => 'string',
> - pattern => '(pc|pc(-i440fx)?-\d+(\.\d+)+(\.pxe)?|q35|pc-q35-\d+(\.\d+)+(\.pxe)?|virt(?:-\d+(\.\d+)+)?)',
> + pattern => '(pc|pc(-i440fx)?-\d+(\.\d+)+(\+pve\d+)?(\.pxe)?|q35|pc-q35-\d+(\.\d+)+(\+pve\d+)(\.pxe)?|virt(?:-\d+(\.\d+)+)?(\+pve\d+)?)',
Does this really work with '.pxe'? When is that even used, i.e. how
would one test this?
If QEMU needs to know about this, it has to be placed before the '+pve'
version, otherwise we just cut it off, no?
> maxLength => 40,
> optional => 1,
> });
> @@ -1875,7 +1875,7 @@ sub print_drivedevice_full {
> }
>
> # for compatibility only, we prefer scsi-hd (#2408, #2355, #2380)
> - my $version = PVE::QemuServer::Machine::extract_version($machine_type) // kvm_user_version();
> + my $version = PVE::QemuServer::Machine::extract_version($machine_type, kvm_user_version());
> if ($path =~ m/^iscsi\:\/\// &&
> !min_version($version, 4, 1)) {
> $devicetype = 'generic';
> @@ -2740,7 +2740,7 @@ sub write_vm_config {
> &$cleanup_config($conf->{pending}, 1);
>
> foreach my $snapname (keys %{$conf->{snapshots}}) {
> - die "internal error: snapshot name '$snapname' is forbidden" if lc($snapname) eq 'pending';
> + die "internal error" if $snapname eq 'pending';
Doesn't belong to this patch.
> &$cleanup_config($conf->{snapshots}->{$snapname}, undef, $snapname);
> }
>
> @@ -3361,13 +3361,14 @@ my $default_machines = {
> };
>
> sub get_vm_machine {
> - my ($conf, $forcemachine, $arch) = @_;
> + my ($conf, $forcemachine, $arch, $add_pve_version) = @_;
>
> my $machine = $forcemachine || $conf->{machine};
>
> if (!$machine) {
> $arch //= 'x86_64';
> $machine ||= $default_machines->{$arch};
> + $machine .= "+pve$PVE::QemuServer::Machine::PVE_MACHINE_VERSION" if $add_pve_version;
> }
>
> return $machine;
> @@ -3469,8 +3470,10 @@ sub config_to_command {
> my $kvm_binary = get_command_for_arch($arch);
> my $kvmver = kvm_user_version($kvm_binary);
>
> - my $machine_type = get_vm_machine($conf, $forcemachine, $arch);
> - my $machine_version = PVE::QemuServer::Machine::extract_version($machine_type) // $kvmver;
> + my $add_pve_version = min_version($kvmver, 4, 1);
> +
> + my $machine_type = get_vm_machine($conf, $forcemachine, $arch, $add_pve_version);
> + my $machine_version = PVE::QemuServer::Machine::extract_version($machine_type, $kvmver);
> $kvm //= 1 if is_native($arch);
>
> if ($kvm) {
> @@ -7048,7 +7051,7 @@ sub qemu_use_old_bios_files {
> $machine_type = $1;
> $use_old_bios_files = 1;
> } else {
> - my $version = PVE::QemuServer::Machine::extract_version($machine_type) // kvm_user_version();
> + my $version = PVE::QemuServer::Machine::extract_version($machine_type, kvm_user_version());
> # Note: kvm version < 2.4 use non-efi pxe files, and have problems when we
> # load new efi bios files on migration. So this hack is required to allow
> # live migration from qemu-2.2 to qemu-2.4, which is sometimes used when
> diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
> index 0e78f36..fcc9392 100644
> --- a/PVE/QemuServer/Helpers.pm
> +++ b/PVE/QemuServer/Helpers.pm
> @@ -103,10 +103,10 @@ sub vm_running_locally {
> }
>
> sub min_version {
> - my ($verstr, $version_major, $version_minor) = @_;
> + my ($verstr, $major, $minor, $pve) = @_;
>
> - if ($verstr =~ m/^(\d+)\.(\d+)/) {
> - return 1 if version_cmp($1, $version_major, $2, $version_minor) >= 0;
> + if ($verstr =~ m/^(\d+)\.(\d+)(?:\.(\d+))?(?:\+pve(\d+))?/) {
> + return 1 if version_cmp($1, $major, $2, $minor, $4, $pve) >= 0;
> return 0;
> }
>
> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
> index 40c8eeb..98b2975 100644
> --- a/PVE/QemuServer/Machine.pm
> +++ b/PVE/QemuServer/Machine.pm
> @@ -6,6 +6,10 @@ use warnings;
> use PVE::QemuServer::Helpers;
> use PVE::QemuServer::Monitor;
>
> +# Bump this for VM HW layout changes during a release (where the QEMU machine
> +# version stays the same)
> +our $PVE_MACHINE_VERSION = 1;
> +
> sub machine_type_is_q35 {
> my ($conf) = @_;
>
> @@ -18,31 +22,42 @@ sub get_current_qemu_machine {
>
> my $res = PVE::QemuServer::Monitor::mon_cmd($vmid, 'query-machines');
>
> - my ($current, $default);
> + my ($current, $pve_version, $default);
> foreach my $e (@$res) {
> $default = $e->{name} if $e->{'is-default'};
> $current = $e->{name} if $e->{'is-current'};
> + $pve_version = $e->{'pve-version'} if $e->{'pve-version'};
> }
>
> + $current .= "+$pve_version" if $current && $pve_version;
> +
> # fallback to the default machine if current is not supported by qemu
> return $current || $default || 'pc';
> }
>
> +# returns a string with major.minor.pveversion, patch is ignored as it's seldom
> +# ressembling a real QEMU machine type, so it would be 0 99% of the time..
It returns major.minor+pve{version}, not major.minor.pve - works fine,
just the comment is misleading.
> sub extract_version {
> - my ($machine_type) = @_;
> + my ($machine_type, $kvmversion) = @_;
>
> - if ($machine_type && $machine_type =~ m/^((?:pc(-i440fx|-q35)?|virt)-(\d+)\.(\d+))/) {
> - return "$3.$4";
> + if (defined($machine_type) && $machine_type =~ m/^(?:pc(?:-i440fx|-q35)?|virt)-(\d+)\.(\d+)(?:\.(\d+))?(\+pve\d+)?/) {
> + my $versionstr = "$1.$2";
> + $versionstr .= $4 if $4;
> + return $versionstr;
> + } elsif (defined($kvmversion)) {
> + if ($kvmversion =~ m/^(\d+)\.(\d+)/) {
> + return "$1.$2+pve$PVE_MACHINE_VERSION";
> + }
> }
>
> return undef;
> }
>
> sub machine_version {
> - my ($machine_type, $version_major, $version_minor) = @_;
> + my ($machine_type, $major, $minor, $pve) = @_;
>
> return PVE::QemuServer::Helpers::min_version(
> - extract_version($machine_type), $version_major, $version_minor);
> + extract_version($machine_type), $major, $minor, $pve);
> }
>
> # dies if a) VM not running or not exisiting b) Version query failed
> diff --git a/test/cfg2cmd/minimal-defaults.conf.cmd b/test/cfg2cmd/minimal-defaults.conf.cmd
> index 5abebe9..83ae328 100644
> --- a/test/cfg2cmd/minimal-defaults.conf.cmd
> +++ b/test/cfg2cmd/minimal-defaults.conf.cmd
> @@ -21,4 +21,4 @@
> -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
> -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3' \
> -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
> - -machine 'type=pc'
> + -machine 'type=pc+pve1'
Good idea to test, but this will break everytime the PVE_MACHINE_VERSION
is bumped. Seems like something to fix in cfg2cmd.pl and not here though
(maybe leave the +pve1 from the .cmd file and dynamically insert it with
the current version during the test?).
Also, maybe a test with pinned pve-version (not sure this has a real
use-case, but it works 'for free' and I don't see any downside).
> diff --git a/test/cfg2cmd/spice-linux-4.1.conf.cmd b/test/cfg2cmd/spice-linux-4.1.conf.cmd
> index 158e73b..4ed6fd2 100644
> --- a/test/cfg2cmd/spice-linux-4.1.conf.cmd
> +++ b/test/cfg2cmd/spice-linux-4.1.conf.cmd
> @@ -27,4 +27,4 @@
> -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
> -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
> -device 'virtio-net-pci,mac=A2:C0:43:67:08:A1,netdev=net0,bus=pci.0,addr=0x12,id=net0,bootindex=300' \
> - -machine 'type=pc'
> + -machine 'type=pc+pve1'
>
More information about the pve-devel
mailing list