[pve-devel] [PATCH qemu-server] implement PVE Version addition for QEMU machine

[Thomas Lamprecht]

On 11/25/19 12:33 PM, Stefan Reiter wrote:
> 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?

The q35 one misses an ? after the (\+pve\d+), but else yes.

> If QEMU needs to know about this, it has to be placed before the '+pve' version, otherwise we just cut it off, no?
> 

No, QEMU cannot parse that, it's "PVE" information for for an old
live-migration compat code path, we remove the .pxe part in 
qemu_use_old_bios_files:

if ($machine_type =~ m/^(\S+)\.pxe$/) {
    $machine_type = $1;
    ...

>>       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.

you're right, fallout from rebasing and saving over the applied
changes from Oguz. :)

>>   @@ -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.

yes, tried some different approaches, and forgot to update the comment.
Thanks for noticing.

>> 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?).

That's the purpose for the minimal-defaults config, to show the
resulting current minimal default command. So this is by design.

> 
> 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'
>>it may make sense to filter it for others though, else this could get
a bit noisy soon.