Re: [PATCH qemu-server v3 1/8] tests: hotplug: add initial hotplug test harness

[Fiona Ebner <f.ebner@proxmox.com>]

Am 07.10.26 um 4:23 PM schrieb Dominik Csapak:
> Add a new test harness for testing the hotplug mechanism. A test case
> consists of failure modes and a vm config with a [PENDING] section.
> After setting up the initial device list via config_to_command, the
> pending section is applied and all qmp/hmp/etc. calls are recorded and
> compared with the expected file for each test.
> 
> In addition, there are certain constraints tested, like hotplugging into
> a hotplugged bridge fails, a bus is not hotpluggable, or the device
> already exists.
> 
> Note that errors in tests don't let the tests fail, but are recorded in
> the output so that we can test behavior that we might need to keep to
> preserve backwards compatibility.
> 
> After hotplugging, the resulting config is checked again with
> config_to_command and the device lists are compared (any difference here
> could lead to a resource leak and/or live migration failure).
> 
> The harness is modeled after cfg2cmd. It supports cases for x86 and
> aarch64 host architectures.
> 
> Since most mocks and setup steps overlap with the cfg2cmd tests, factor
> out the relevant code into separate modules which can be used by
> multiple tests. The exact setup and diff code change slightly, but the
> comparison and output should behave the same.
> 
> No test cases are added yet, these come in separate patches.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

Good work! Still got some comments, but mostly just polishing.

> ---
>  src/test/Makefile                        |   8 +-
>  src/test/TestsCommon/CommandLineMocks.pm | 537 +++++++++++++++
>  src/test/TestsCommon/Utils.pm            |  31 +
>  src/test/run_config2command_tests.pl     | 553 +--------------
>  src/test/run_hotplug_tests.pl            | 834 +++++++++++++++++++++++

Nit: the addition of the new test runner and the refactoring could be
separate patches.

>  src/test/run_parse_config_tests.pl       |  25 +-
>  6 files changed, 1433 insertions(+), 555 deletions(-)
>  create mode 100644 src/test/TestsCommon/CommandLineMocks.pm
>  create mode 100644 src/test/TestsCommon/Utils.pm

Nit: this is bikeshedding, but TestsCommon sounds generic, maybe
QemuServerTest?

But feel free to keep them as they are.

---snip 8<---

> diff --git a/src/test/TestsCommon/CommandLineMocks.pm b/src/test/TestsCommon/CommandLineMocks.pm
> new file mode 100644
> index 00000000..50540e8f
> --- /dev/null
> +++ b/src/test/TestsCommon/CommandLineMocks.pm
> @@ -0,0 +1,537 @@
> +package TestsCommon::CommandLineMocks;
> +
> +# Mocks for generating the QEMU command line of a VM config in tests, shared
> +# between the test scripts for the command line itself and for hotplugging. The
> +# environment (storage configuration, PCI devices, mappings, ...) is fixed,
> +# while a few properties of the host can be set per test via set_test_env().
> +# Also contains helpers that are useful and needed by multiple test harnesses.
> +
> +use v5.36;
> +
> +use JSON qw(decode_json);
> +use Socket qw(AF_INET AF_INET6);
> +use Test::MockModule;
> +
> +use PVE::INotify;
> +use PVE::Mapping::PCI;
> +use PVE::Mapping::USB;
> +use PVE::ProcFSTools;
> +use PVE::QemuServer::CPUConfig;
> +use PVE::QemuServer::Drive;
> +use PVE::QemuServer::Helpers;
> +use PVE::QemuServer::Memory;
> +use PVE::QemuServer::OVMF;
> +use PVE::QemuServer::PCI;
> +use PVE::QemuServer;
> +use PVE::Storage::RBDPlugin;
> +use PVE::Storage::ZFSPlugin;
> +use PVE::Storage;
> +use PVE::SysFSTools;
> +use PVE::Tools qw(run_command);

Style nit: not grouped according to style guide:
https://pve.proxmox.com/wiki/Perl_Style_Guide#Module_Dependencies

> +
> +use base 'Exporter';
> +
> +our @EXPORT_OK = qw(
> +    get_storage_config
> +    get_test_qemu_version
> +    set_test_env
> +);
> +
> +my $real_qemu_version = PVE::QemuServer::Helpers::kvm_user_version(); # can be overwritten by individual tests

Style nit: too long line

---snip 8<---

> +# Set the properties of the (mocked) host for the following test, fields are:
> +#   qemu_version: version of the installed QEMU binary (defaults to the real version)
> +#   host_arch: x86_64 | aarch64 (defaults to x86_64)
> +#   host-cpu-vendor: AuthenticAMD | GenuineIntel | etc. (defaults to GenuineIntel)

Should we initialize the defaults for the above three when setting the
test environment already? Currently, fallback to the defaults happens
upon access, so we need to be careful to re-apply the same default when
adding another use site in the future.

> +#   hw_capabilities: a CPU from $cpu_hw_capabilities above or a raw JSON string
> +sub set_test_env($env) {
> +    $test_env = $env;
> +}
> +

---snip 8<---

> diff --git a/src/test/TestsCommon/Utils.pm b/src/test/TestsCommon/Utils.pm
> new file mode 100644
> index 00000000..e738520b
> --- /dev/null
> +++ b/src/test/TestsCommon/Utils.pm
> @@ -0,0 +1,31 @@
> +package TestsCommon::Utils;
> +
> +use v5.36;
> +
> +use File::Temp;
> +
> +use PVE::Tools qw(run_command);

Nit: please use PVE::Cmd::run for new code.

> +
> +use base 'Exporter';
> +
> +our @EXPORT_OK = qw(
> +    diff
> +);
> +
> +sub diff($expected, $got) {
> +    return if $expected eq $got;
> +
> +    my $tmp = File::Temp->new();
> +    print $tmp $expected;
> +    $tmp->flush();
> +
> +    my $diff = '';
> +    run_command(
> +        ['diff', '-up', '--label', 'expected', $tmp->filename(), '--label', 'got', '-'],
> +        input => $got,
> +        outfunc => sub { $diff .= "$_[0]\n"; },
> +        noerr => 1,
> +    );
> +
> +    return $diff || "inputs differ, but `diff` produced no output.";
> +}

Is there a reason you re-implemented this rather than copying over the
existing implementation? Should be mentioned in the commit message.
Splitting out the diff helper might be nicer as its own patch.

---snip 8<---

> diff --git a/src/test/run_hotplug_tests.pl b/src/test/run_hotplug_tests.pl
> new file mode 100644
> index 00000000..fc0523ca
> --- /dev/null
> +++ b/src/test/run_hotplug_tests.pl
> @@ -0,0 +1,834 @@
> +#!/usr/bin/perl
> +
> +# Regression tests for applying pending changes to a running VM via hotplug.
> +#
> +# Each test is a VM config in 'hotplug/*.conf' with the changes to apply in a [PENDING] section. The
> +# monitor of the VM is mocked with a minimal model of a running QEMU instance, that tracks the
> +# present devices, buses, block nodes, backends and QOM objects and enforces the constraints between
> +# them. Every monitor command and every host-side action (network, SDN, storage, cgroup) that is
> +# issued while applying the pending changes is recorded. Afterwards, the model is compared to the
> +# model of a VM freshly started with the resulting config, which is what the target of a live
> +# migration is, to detect deviations like differing PCI addresses or left-over devices. The
> +# recordings are compared to the corresponding 'hotplug/*.conf.expected' file. If that file does not
> +# exist yet, it is generated and needs to be manually verified. Tests for an aarch64 host are in
> +# 'hotplug/aarch64/'.

Nit: the script does not automatically do anything for the aarch64
directory. Maybe mention that the directory can be specified via the
argument and hotplug is the default?

---snip 8<---

> +# Parse a comma separated option string like 'driver,id=x,bus=y' into a hash. A leading value
> +# without key is stored under $first_key.
> +sub parse_options($string, $first_key = undef) {
> +    my $options = {};
> +    for my $part (split(/,/, $string)) {
> +        if ($part =~ m/^([^=]+)=(.*)$/) {
> +            $options->{$1} = $2;
> +        } elsif (defined($first_key)) {
> +            $options->{$first_key} = $part;
> +        }

else die to catch something unexpected?

> +    }
> +    return $options;
> +}
> +
> +# Parse the devices from a QEMU config file (-readconfig), which is shipped in 'usr/' of the repo.
> +sub parse_readconfig($path) {
> +    my ($name) = $path =~ m|([^/]+)$|;
> +    my $devices = {};
> +    my $current;
> +    for my $line (split(/\n/, file_get_contents("../usr/$name"))) {
> +        if ($line =~ m/^\[device "([^"]+)"\]/) {
> +            $current = $devices->{$1} = { id => $1 };
> +        } elsif ($line =~ m/^\[/) {
> +            $current = undef;
> +        } elsif ($current && $line =~ m/^\s*(\S+)\s*=\s*"([^"]*)"/) {
> +            $current->{$1} = $2;
> +        }
> +    }
> +    return $devices;
> +}
> +
> +# Resolve the full final machine version, even for values like 'pc' or 'q35'.
> +sub resolve_machine_alias($machine) {
> +    my ($type, $pve_version) = split(/\+/, $machine, 2);
> +    if ($type && $type !~ m/^(?:pc|q35|virt)$/) {
> +        return $machine;
> +    }
> +
> +    my $kvm_version = get_test_qemu_version();
> +    my $new_machine =
> +        PVE::QemuServer::Machine::windows_get_pinned_machine_version($type, undef, $kvm_version);
> +
> +    if ($new_machine !~ m/\+pve\d+$/) {
> +        $new_machine = $new_machine . ($pve_version ? "+$pve_version" : "+pve0");
> +    }
> +    return $new_machine;
> +}
> +
> +# Returns the model of a VM freshly started with the given config, derived from the command line
> +# that config_to_command() generates for it.
> +sub vm_state_from_config($conf) {
> +    my $cmd = PVE::QemuServer::config_to_command(
> +        $storecfg, $vmid, dclone($conf), PVE::QemuServer::load_defaults(), {},
> +    );
> +
> +    my $machine = PVE::QemuServer::Machine::get_vm_machine($conf);
> +
> +    my $state = {
> +        machine => resolve_machine_alias($machine),

Won't this be overwritten in any case below? We could assert if not. And
if we drop this here, we can assert in resolve_machine_alias() that we
already got a +pve version.

Then we could avoid reference to windows_get_pinned_machine_version() if
we extract its
    if ($machine eq 'pc') {
        $machine = "pc-i440fx-$pin_version";
    } elsif ($machine eq 'q35') {
        $machine = "pc-q35-$pin_version";
    } elsif ($machine eq 'virt') {
        $machine = "virt-$pin_version";
    } else {
        warn "unknown machine type '$machine', not touching that!\n";
    }
part into a dedicated helper first. This should simplify the
resolve_machine_alias() a bit.

Or am I missing something?

> +        devices => {},
> +        hotplugged => {},
> +        netdevs => {},
> +        chardevs => {},
> +        drives => {},
> +        blocknodes => {},
> +        objects => {},
> +    };
> +
> +    for (my $i = 0; $i < scalar($cmd->@*) - 1; $i++) {
> +        my ($opt, $arg) = $cmd->@[$i, $i + 1];
> +        if ($opt eq '-machine') {
> +            $state->{machine} = resolve_machine_alias(parse_options($arg)->{type});
> +        } elsif ($opt eq '-device') {
> +            my $device = parse_options($arg, 'driver');
> +            $state->{devices}->{ $device->{id} } = $device if $device->{id};
> +        } elsif ($opt eq '-readconfig') {
> +            my $devices = parse_readconfig($arg);
> +            $state->{devices}->{$_} = $devices->{$_} for keys $devices->%*;
> +        } elsif ($opt eq '-netdev') {
> +            $state->{netdevs}->{ parse_options($arg)->{id} } = 1;
> +        } elsif ($opt eq '-chardev') {
> +            $state->{chardevs}->{ parse_options($arg, 'backend')->{id} } = 1;
> +        } elsif ($opt eq '-drive') {
> +            $state->{drives}->{ parse_options($arg)->{id} } = 1;
> +        } elsif ($opt eq '-blockdev') {
> +            $state->{blocknodes}->{ decode_json($arg)->{'node-name'} } = 1;
> +        } elsif ($opt eq '-object') {
> +            my $object = $arg =~ m/^\{/ ? decode_json($arg) : parse_options($arg, 'qom-type');
> +            # sizes on the command line have a unit, QMP uses bytes
> +            if (defined($object->{size}) && $object->{size} =~ m/^(\d+)([MG])$/) {

What about K/T suffixes? Would be nice to cover at least those already
to avoid potential test failure in the future.

> +                $object->{size} = $1 * 1024 * 1024 * ($2 eq 'G' ? 1024 : 1);
> +            }
> +            $state->{objects}->{ $object->{id} } = $object;
> +        }
> +    }
> +
> +    $state->{buses} = default_buses_for_machine($conf, $state->{machine});
> +
> +    return $state;
> +}
> +
> +sub setup_vm_state($conf) {
> +    $vm_state = vm_state_from_config($conf);
> +}
> +
> +# Throttle limits set via QMP contain all properties, while the throttle group generated for the
> +# command line only contains the configured ones. Drop the defaults to make them comparable.
> +sub normalized_object($object) {
> +    $object = dclone($object);
> +    if (($object->{'qom-type'} // '') eq 'throttle-group' && $object->{limits}) {
> +        my $limits = $object->{limits};
> +        my $is_default = sub { $limits->{ $_[0] } == ($_[0] =~ m/-max-length$/ ? 1 : 0) };
> +        $object->{limits} =
> +            { map { $_ => $limits->{$_} } grep { !$is_default->($_) } keys %$limits };
> +    }
> +    return to_json($object, { canonical => 1 });
> +}
> +
> +# Compare the model of the running VM with the model of a VM freshly started with the resulting
> +# config, like the target of a live migration is. Returns a list of the differences. The boot index
> +# is not compared, as hotplugged devices are added without one.
> +sub compare_with_fresh_vm($conf) {
> +    my $running = $vm_state;
> +    my $fresh = vm_state_from_config($conf);
> +    my $differences = [];
> +
> +    my $compare_ids = sub {
> +        my ($kind, $running_ids, $fresh_ids) = @_;

Style nit: should use signature for consistency.

> +        my $all_keys = [keys $running_ids->%*, keys $fresh_ids->%*];
> +        for my $key ($all_keys->@*) {
> +            push $differences->@*, "$kind: $key: only present in running VM"
> +                if !$fresh_ids->{$key};
> +            push $differences->@*, "$kind: $key: only present in freshly started VM"
> +                if !$running_ids->{$key};
> +        }
> +    };
> +
> +    push $differences->@*, "machine: running '$running->{machine}' vs fresh '$fresh->{machine}'"
> +        if $running->{machine} ne $fresh->{machine};
> +
> +    $compare_ids->('device', $running->{devices}, $fresh->{devices});
> +    for my $id (sort grep { $fresh->{devices}->{$_} } keys $running->{devices}->%*) {
> +        my $running_device = $running->{devices}->{$id};
> +        my $fresh_device = $fresh->{devices}->{$id};
> +        my %options = map { $_ => 1 } keys $running_device->%*, keys $fresh_device->%*;
> +        delete $options{bootindex};

A comment would be nice about why bootindex is removed.

> +        for my $option (sort keys %options) {
> +            my $running_value = $running_device->{$option} // '<undef>';
> +            my $fresh_value = $fresh_device->{$option} // '<undef>';
> +            push $differences->@*,
> +                "device $id option $option: running '$running_value' vs fresh '$fresh_value'"
> +                if "$running_value" ne "$fresh_value";

Style nit: mutli-line post-if

---snip 8<---

> +# warnings are part of the expected outcome, e.g. when cleaning up after a failed hotplug
> +$SIG{__WARN__} = sub {
> +    my $warning = shift;
> +    chomp $warning;
> +    $warning =~ s/ at \S+ line \d+\.?$//;

Maybe keep a placeholder for the line number, then we can more easily
catch ones that are still missing a newline?

> +    record_action('warn', $warning);
> +};
> +
> +sub do_test($config_fn) {
> +    die "no such input test config: $config_fn\n" if !-f $config_fn;
> +
> +    $recorded_actions = []; # also record warnings while parsing the config
> +    parse_test($config_fn);
> +
> +    my $testname = $current_test->{testname};
> +    my $conf = $current_test->{config};
> +
> +    my $errors = {};
> +    eval {
> +        setup_vm_state($conf);
> +        PVE::QemuServer::vmconfig_hotplug_pending($vmid, $conf, $storecfg, undef, $errors);
> +    };
> +    if (my $err = $@) {
> +        $errors->{died} = $err;
> +    }
> +
> +    # error messages without trailing newline contain the source location, which changes too often
> +    my $clean_error = sub {
> +        my ($key, $msg) = @_;
> +        chomp $msg;
> +        $msg =~ s/ at \S+ line \d+\.?$//;

Same here. We might want to add newlines for many of them anyway? But no
need to change that with your series already.