From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id DC3A11FF09C for ; Mon, 05 Oct 2026 16:49:44 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id B297C217FA; Mon, 05 Oct 2026 16:48:41 +0200 (CEST) From: =?UTF-8?q?Michael=20K=C3=B6ppl?= To: pve-devel@lists.proxmox.com Subject: [PATCH guest-common v7 06/24] tests: add tests for used guest ID tracking Date: Mon, 5 Oct 2026 16:47:47 +0200 Message-ID: <20261005144805.825538-7-m.koeppl@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261005144805.825538-1-m.koeppl@proxmox.com> References: <20261005144805.825538-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: 1791211689077 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.354 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: DQVQ56UXBTUUKH3PHB7ZHOZCIH3DY537 X-Message-ID-Hash: DQVQ56UXBTUUKH3PHB7ZHOZCIH3DY537 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 | 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