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

[Stefan Reiter]

On 11/25/19 1:22 PM, Thomas Lamprecht wrote:
> 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

BTW, shouldn't that be *increasing* the granularity?

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

Ah I see. I read that sub as a getter, didn't realize it was used to 
also modify the $machine_type.

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

Makes sense.

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

Looking through the tests also made me realize we need to add the 
pve-version if a 'machine' is specified in the config but without a version.

E.g. having 'machine: pc' in the config is not versioned, so it should 
also use the newest pve-version (it already uses the newest machine 
version since that is aliased by QEMU itself).