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.lampre...@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.
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
