[PATCH guest-common v8 06/22] tests: add tests for used guest ID tracking

["Michael Köppl" <m.koeppl@proxmox.com>]

The used guest ID range handling has a few cases that are easy to get
subtly wrong. Adjacent and contained ranges have to be merged on write,
the next unused ID has to be found across adjacent ranges and past
existing guests that were possibly never recorded in the file, etc.

Add tests to cover the parser, writer, get_next_unused_id() and
register_used_id().

Test::MockModule is used here and added as a build dependency because
that is what most other PVE repositories use.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Tested-by: Fiona Ebner <f.ebner@proxmox.com>
---
 debian/control              |   1 +
 src/tests/Makefile          |   7 +-
 src/tests/guest-id-tests.pl | 208 ++++++++++++++++++++++++++++++++++++
 3 files changed, 214 insertions(+), 2 deletions(-)
 create mode 100755 src/tests/guest-id-tests.pl

diff --git a/debian/control b/debian/control
index 21f0db2..539e3f0 100644
--- a/debian/control
+++ b/debian/control
@@ -6,6 +6,7 @@ Build-Depends: debhelper-compat (= 13),
                libpve-cluster-perl,
                libpve-common-perl,
                libpve-storage-perl (>= 8.3.4),
+               libtest-mockmodule-perl,
                pve-cluster,
 Standards-Version: 4.6.2
 Homepage: https://www.proxmox.com
diff --git a/src/tests/Makefile b/src/tests/Makefile
index 2fec185..f10564b 100644
--- a/src/tests/Makefile
+++ b/src/tests/Makefile
@@ -1,8 +1,11 @@
 
 all:
 
-.PHONY: check abstract-config
-check: abstract-config
+.PHONY: check abstract-config guest-id
+check: abstract-config guest-id
 
 abstract-config: abstract-config-tests.pl
 	perl -I.. ./abstract-config-tests.pl
+
+guest-id: guest-id-tests.pl
+	perl -I.. ./guest-id-tests.pl
diff --git a/src/tests/guest-id-tests.pl b/src/tests/guest-id-tests.pl
new file mode 100755
index 0000000..5cf796b
--- /dev/null
+++ b/src/tests/guest-id-tests.pl
@@ -0,0 +1,208 @@
+#!/usr/bin/perl
+
+use v5.36;
+
+use lib qw(..);
+
+use Test::MockModule;
+use Test::More;
+
+my $FILE = '/etc/pve/virtual-guest/used-guest-ids';
+
+my ($parse, $write);
+my $raw; # contents of the mocked used-guest-ids file
+my $writes = 0;
+my $injected_write_error;
+my $vmlist;
+my $dc_conf = {};
+
+my $pve_cluster_module = Test::MockModule->new('PVE::Cluster');
+$pve_cluster_module->mock(
+    cfs_register_file => sub($filename, $parser, $writer) {
+        ($parse, $write) = ($parser, $writer);
+    },
+    cfs_read_file => sub($filename) {
+        return $dc_conf if $filename eq 'datacenter.cfg';
+        return $parse->("/etc/pve/$filename", $raw);
+    },
+    cfs_write_file => sub($filename, $data) {
+        die $injected_write_error if defined($injected_write_error);
+        $writes++;
+        $raw = $write->("/etc/pve/$filename", $data);
+    },
+    cfs_lock_file => sub($filename, $timeout, $code) {
+        return eval { $code->() };
+    },
+    get_vmlist => sub() { return $vmlist },
+);
+
+# not 'use', since PVE::GuestID registers the parser and writer via cfs_register_file() when
+# loaded, so the mock above must be in place first to capture them
+require PVE::GuestID;
+
+sub range_to_vmlist(@ids) {
+    return { ids => { map { $_ => { node => 'localhost', type => 'qemu' } } @ids } };
+}
+
+subtest 'parse' => sub {
+    my $tests = [
+        ['canonical file', "100-250\n300\n900-1200\n", [[100, 250], [300, 300], [900, 1200]]],
+        [
+            'unordered file is sorted but not merged',
+            "300-400\n100-200\n201-250\n150-160\n",
+            [[100, 200], [150, 160], [201, 250], [300, 400]],
+        ],
+        ['missing newline', "100-250\n300\n900-1000", [[100, 250], [300, 300], [900, 1000]]],
+        ['missing file', undef, []],
+        ['empty file', '', []],
+        ['blank lines only', "\n  \n", []],
+    ];
+
+    for my $test ($tests->@*) {
+        my ($name, $input, $expected) = $test->@*;
+        is_deeply($parse->($FILE, $input), $expected, $name);
+    }
+
+    my @warnings;
+    local $SIG{__WARN__} = sub { push @warnings, $_[0] };
+
+    is_deeply(
+        $parse->($FILE, "100\nfoo\n50-10\n200\n"),
+        [[100, 100], [200, 200]],
+        'invalid entry and reversed range are skipped',
+    );
+    is_deeply(
+        \@warnings,
+        [
+            "skipping invalid entry in $FILE: foo\n",
+            "skipping reversed range in $FILE: 50-10\n",
+        ],
+        'skipped entries are warned about',
+    );
+};
+
+subtest 'write' => sub {
+    my $tests = [
+        [
+            'canonical file is written unchanged',
+            "100-250\n300\n900-1200\n",
+            "100-250\n300\n900-1200\n",
+        ],
+        [
+            'overlapping and adjacent ranges are merged',
+            "300-400\n100-200\n201-250\n150-160\n",
+            "100-250\n300-400\n",
+        ],
+        ['adjacent single IDs are merged', "100\n101\n102\n", "100-102\n"],
+        ['contained range keeps its container', "100-500\n200-300\n", "100-500\n"],
+        [
+            'ranges with adjacent IDs are merged',
+            "1000-1004\n999\n100-200\n1005\n",
+            "100-200\n999-1005\n",
+        ],
+        ['nothing recorded', '', ''],
+    ];
+
+    for my $test ($tests->@*) {
+        my ($name, $input, $expected) = $test->@*;
+        is($write->($FILE, $parse->($FILE, $input)), $expected, $name);
+    }
+};
+
+subtest 'get_next_unused_id' => sub {
+    my $tests = [
+        # name, file contents, existing guests, input ID, expected ID
+        ['below all ranges', "101-200\n300-400\n", [], 100, 100],
+        ['start of a range', "100-200\n300-400\n", [], 100, 201],
+        ['inside a range', "100-200\n300-400\n", [], 150, 201],
+        ['in a gap', "100-200\n300-400\n", [], 250, 250],
+        ['above all ranges', "100-200\n300-400\n", [], 500, 500],
+        ['across adjacent ranges', "100-200\n201-300\n", [], 150, 301],
+        ['existing guests are skipped', "100-105\n", [106, 107], 100, 108],
+        ['used range after an existing guest', "100-105\n107-110\n", [106], 100, 111],
+        ['existing guests not recorded in the file', '', [100, 101], 100, 102],
+        ['no vmlist', "100-105\n", undef, 100, 106],
+        ['nothing recorded, no guests', undef, [], 100, 100],
+    ];
+
+    for my $test ($tests->@*) {
+        my ($name, $file, $existing, $id, $expected) = $test->@*;
+
+        $raw = $file;
+        $vmlist = defined($existing) ? range_to_vmlist($existing->@*) : undef;
+
+        is(PVE::GuestID::get_next_unused_id($id), $expected, $name);
+    }
+};
+
+subtest 'register_used_id' => sub {
+    $raw = undef;
+
+    my $steps = [
+        ['first ID', 101, "101\n"],
+        ['extends a range at its end', 102, "101-102\n"],
+        ['starts a new range after a gap', 104, "101-102\n104\n"],
+        ['bridges two ranges', 103, "101-104\n"],
+        ['extends a range at its start', 100, "100-104\n"],
+    ];
+
+    for my $step ($steps->@*) {
+        my ($name, $id, $expected) = $step->@*;
+        PVE::GuestID::register_used_id($id);
+        is($raw, $expected, $name);
+    }
+
+    $writes = 0;
+    PVE::GuestID::register_used_id(101);
+    PVE::GuestID::register_used_id(104);
+    is($writes, 0, 'already recorded IDs are not written again');
+    is($raw, "100-104\n", 'file unchanged by already recorded IDs');
+
+    $raw = "300-400\n100-200\n201-250\n150-160\n";
+    PVE::GuestID::register_used_id(275);
+    is($raw, "100-250\n275\n300-400\n", 'unordered file is normalized on write');
+};
+
+subtest 'register_used_id errors' => sub {
+    my $tests = [
+        # name, datacenter.cfg, whether the error reaches the caller
+        ['unique IDs requested', { 'next-id' => { unique => 1 } }, 1],
+        ['unique IDs not requested', { 'next-id' => { lower => 100 } }, 0],
+        ['no next-id settings', {}, 0],
+    ];
+
+    for my $test ($tests->@*) {
+        my ($name, $conf, $expect_fatal) = $test->@*;
+
+        $dc_conf = $conf;
+        $raw = "100\n";
+        $injected_write_error = "write failed\n";
+
+        my @warnings;
+        local $SIG{__WARN__} = sub { push @warnings, $_[0] };
+
+        eval { PVE::GuestID::register_used_id(500) };
+
+        if ($expect_fatal) {
+            is(
+                $@,
+                "unable to record guest ID 500 as used - write failed\n",
+                "$name: error is passed on to the caller",
+            );
+            is_deeply(\@warnings, [], "$name: no warning");
+        } else {
+            is($@, '', "$name: error is not passed on");
+            is_deeply(
+                \@warnings,
+                ["unable to record guest ID 500 as used - write failed\n"],
+                "$name: error is warned about",
+            );
+        }
+        is($raw, "100\n", "$name: file unchanged");
+    }
+
+    $injected_write_error = undef;
+    $dc_conf = {};
+};
+
+done_testing();
-- 
2.47.3