[PATCH test-tools 1/6] runner: add vmid-range option for test guest VMIDs

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

Test cases draw the VMIDs of the guests they create at random from a
large range, which makes it unlikely that two runs against one cluster
pick the same id, or that a just freed id, which a cleanup task may
still hold a lock on, is reused right away.

Expose that range on the command line, so runs that must not collide
can be given disjoint ones instead of relying on chance.

Add it as a global option because every suite draws through the same
helper and any subcommand can end up on a shared cluster. run-local
passes no inventory, so its test cases target the host they run on, and
a run inventory is only a set of addresses, which may name a development
cluster.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 README.md                        |  7 ++-
 proxmox-test-runner/src/cli.rs   | 28 ++++++++++--
 proxmox-test-runner/src/main.rs  |  1 +
 proxmox-test-runner/src/types.rs | 78 ++++++++++++++++++++++++++++++++
 4 files changed, 109 insertions(+), 5 deletions(-)
 create mode 100644 proxmox-test-runner/src/types.rs

diff --git a/README.md b/README.md
index 9b2f7bc6..d9bc8aa6 100644
--- a/README.md
+++ b/README.md
@@ -243,8 +243,11 @@ testcase file first and can be started from anywhere.
 All subcommands accept the global options `--report <path>` and
 `--report-format text|json|junit|markdown` to write a report file, `--suite-version` to record
 the suite version in the report for provenance, `--verbose` to print the entire output of each
-test, and `--tags` to select tests: a comma-separated list of tags replaces the suite's default
-selection, while entries prefixed with `+` add to it (e.g. `--tags +storage-plugin-guest`).
+test, `--tags` to select tests: a comma-separated list of tags replaces the suite's default
+selection, while entries prefixed with `+` add to it (e.g. `--tags +storage-plugin-guest`), and
+`--vmid-range <start>:<end>` to confine the VMIDs of the guests a test case creates. Ideally, give
+concurrent runs against one cluster disjoint ranges so they cannot draw the same ID. Without it
+the test cases draw from `1000000-9999999`.
 
 ## proxmox-test-scheduler
 
diff --git a/proxmox-test-runner/src/cli.rs b/proxmox-test-runner/src/cli.rs
index 5e558b8c..ab0d55ab 100644
--- a/proxmox-test-runner/src/cli.rs
+++ b/proxmox-test-runner/src/cli.rs
@@ -14,6 +14,7 @@ use proxmox_test_common::secrets::SecretResolver;
 use crate::config::TestcaseConfig;
 use crate::report::{ReportFormat, ReportMeta, report_outcomes, write_report_to_file};
 use crate::test_runner::{TestOutcomes, TestRunner};
+use crate::types::VmIdRange;
 
 #[api(
     properties: {
@@ -43,6 +44,10 @@ use crate::test_runner::{TestOutcomes, TestRunner};
             optional: true,
             default: false,
         },
+        "vmid-range": {
+            type: VmIdRange,
+            optional: true,
+        },
     },
 )]
 /// Options shared by every subcommand. Registered as a global option group so they are accepted at
@@ -55,6 +60,7 @@ pub struct CommonArgs {
     pub suite_version: Option<String>,
     pub tags: Option<String>,
     pub verbose: bool,
+    pub vmid_range: Option<VmIdRange>,
 }
 
 /// The shared options, read back from the CLI environment they were parsed into. Defaults to all
@@ -166,6 +172,21 @@ fn resolve_secrets(secrets: Option<PathBuf>) -> Result<(Option<PathBuf>, SecretR
     Ok((path, resolver))
 }
 
+/// Map an explicit VMID range onto the env vars the shared `random_vmid` test helper reads. A
+/// suite given no range falls back to the default of the helper.
+fn vmid_range_env(vmid_range: &Option<VmIdRange>) -> Vec<(String, String)> {
+    let Some(range) = vmid_range else {
+        return Vec::new();
+    };
+    vec![
+        (
+            "PROXMOX_TEST_VMID_RANGE_START".into(),
+            range.start.to_string(),
+        ),
+        ("PROXMOX_TEST_VMID_RANGE_END".into(), range.end.to_string()),
+    ]
+}
+
 /// Map the storage-plugin options onto the `PLUGIN_*` env vars the perl test scripts read.
 fn build_env(
     storage_id: &str,
@@ -221,7 +242,7 @@ fn run_local_suite(
         &inventory,
         &secret_resolver,
         common.verbose,
-        Vec::new(),
+        vmid_range_env(&common.vmid_range),
     );
     let meta = ReportMeta::capture(suite_version, None, None)?;
     let outcomes = test_runner.run_tests(&PathBuf::new(), &override_secrets_path, false)?;
@@ -275,7 +296,7 @@ pub fn run(
         &test_inventory,
         &secret_resolver,
         common.verbose,
-        Vec::new(),
+        vmid_range_env(&common.vmid_range),
     );
     let meta = ReportMeta::capture(common.suite_version, None, None)?;
     let outcomes = test_runner.run_tests(&inventory, &override_secrets_path, start_instances)?;
@@ -442,7 +463,7 @@ pub fn storage_plugin_validation(
         log::log_step("Storage", storage_id);
 
         let inventory = TestInventory::default();
-        let extra_env = build_env(
+        let mut extra_env = build_env(
             storage_id,
             &iso_url,
             &backup_fallback_storage,
@@ -450,6 +471,7 @@ pub fn storage_plugin_validation(
             &migration_target_node,
             &qga_image,
         );
+        extra_env.extend(vmid_range_env(&common.vmid_range));
         let test_runner = TestRunner::new(
             &testcase_cfg,
             &inventory,
diff --git a/proxmox-test-runner/src/main.rs b/proxmox-test-runner/src/main.rs
index 72d8cec6..eacb72d0 100644
--- a/proxmox-test-runner/src/main.rs
+++ b/proxmox-test-runner/src/main.rs
@@ -9,6 +9,7 @@ mod cli;
 mod config;
 mod report;
 mod test_runner;
+mod types;
 
 fn main() -> Result<(), Error> {
     log::init();
diff --git a/proxmox-test-runner/src/types.rs b/proxmox-test-runner/src/types.rs
new file mode 100644
index 00000000..54b52775
--- /dev/null
+++ b/proxmox-test-runner/src/types.rs
@@ -0,0 +1,78 @@
+use std::str::FromStr;
+
+use anyhow::{Context, Error, bail, format_err};
+use proxmox_schema::{ApiStringFormat, ApiType, Schema, StringSchema};
+use serde::{Deserialize, Serialize};
+
+const VMID_MIN: u32 = 100;
+const VMID_MAX: u32 = 999_999_999;
+
+#[derive(Clone)]
+pub struct VmIdRange {
+    pub start: u32,
+    pub end: u32,
+}
+
+impl ApiType for VmIdRange {
+    const API_SCHEMA: Schema = StringSchema::new(
+        "Range of VMIDs to randomly draw from for test guests, as '<start>:<end>' \
+        with start lower than end",
+    )
+    .format(&ApiStringFormat::VerifyFn(|s| {
+        s.parse::<VmIdRange>().map(drop)
+    }))
+    .schema();
+}
+
+impl FromStr for VmIdRange {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let (start, end) = s
+            .split_once(':')
+            .ok_or_else(|| format_err!("expected '<start>:<end>', got '{s}'"))?;
+
+        let start: u32 = start
+            .parse()
+            .with_context(|| format!("invalid range start '{start}'"))?;
+
+        let end: u32 = end
+            .parse()
+            .with_context(|| format!("invalid range end '{end}'"))?;
+
+        if start >= end {
+            bail!("range start {start} must be lower than its end {end}");
+        }
+
+        if start < VMID_MIN || end > VMID_MAX {
+            bail!("range must be within {VMID_MIN}..={VMID_MAX}");
+        }
+
+        Ok(Self { start, end })
+    }
+}
+
+impl std::fmt::Display for VmIdRange {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}:{}", self.start, self.end)
+    }
+}
+
+impl Serialize for VmIdRange {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        serializer.collect_str(self)
+    }
+}
+
+impl<'d> Deserialize<'d> for VmIdRange {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'d>,
+    {
+        let s = String::deserialize(deserializer)?;
+        s.parse().map_err(serde::de::Error::custom)
+    }
+}
-- 
2.47.3