From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 218EF1FF0AF for ; Thu, 24 Sep 2026 18:16:24 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 9D64E21740; Thu, 24 Sep 2026 18:15:41 +0200 (CEST) From: =?UTF-8?q?Michael=20K=C3=B6ppl?= To: pve-devel@lists.proxmox.com Subject: [PATCH guest-common v6 05/18] tests: add tests for used guest ID tracking Date: Thu, 24 Sep 2026 18:14:57 +0200 Message-ID: <20260924161510.847362-6-m.koeppl@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260924161510.847362-1-m.koeppl@proxmox.com> References: <20260924161510.847362-1-m.koeppl@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790266512788 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.457 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: VOCTDYTRFVAILM6KE3UVRBLVHZL5VE6E X-Message-ID-Hash: VOCTDYTRFVAILM6KE3UVRBLVHZL5VE6E X-MailFrom: m.koeppl@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 --- debian/control | 1 + src/tests/Makefile | 7 +- src/tests/guest-id-tests.pl | 201 ++++++++++++++++++++++++++++++++++++ 3 files changed, 207 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..3be492a --- /dev/null +++ b/src/tests/guest-id-tests.pl @@ -0,0 +1,201 @@ +#!/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 stubbed used-guest-ids file +my $writes = 0; +my $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 $write_error if defined($write_error); + $writes++; + $raw = $write->("/etc/pve/$filename", $data); + }, + cfs_lock_file => sub($filename, $timeout, $code) { + return eval { $code->() }; + }, + get_vmlist => sub() { return $vmlist }, +); + +require PVE::GuestID; + +sub guests(@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 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"], + ['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', "100-200\n300-400\n", [], 50, 50], + ['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) ? guests($existing->@*) : undef; + + is(PVE::GuestID::get_next_unused_id($id), $expected, $name); + } +}; + +subtest 'register_used_id' => sub { + $raw = undef; + + my $steps = [ + ['first ID', 100, "100\n"], + ['extends a range at its end', 101, "100-101\n"], + ['starts a new range after a gap', 103, "100-101\n103\n"], + ['bridges two ranges', 102, "100-103\n"], + ['extends a range at its start', 99, "99-103\n"], + ['starts a new range above all others', 200, "99-103\n200\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(200); + is($writes, 0, 'already recorded IDs are not written again'); + is($raw, "99-103\n200\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, $fatal) = $test->@*; + + $dc_conf = $conf; + $raw = "100\n"; + $write_error = "write failed\n"; + + my @warnings; + local $SIG{__WARN__} = sub { push @warnings, $_[0] }; + + eval { PVE::GuestID::register_used_id(500) }; + + if ($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"); + } + + $write_error = undef; + $dc_conf = {}; +}; + +done_testing(); -- 2.47.3