* [pve-devel] [PATCH pve-installer v2 1/6] auto: add early answer file sanity check for RAID configurations
2025-04-29 14:09 [pve-devel] [PATCH installer v2 0/6] add early disk and network sanity checks Michael Köppl
@ 2025-04-29 14:09 ` Michael Köppl
2025-04-29 14:09 ` [pve-devel] [PATCH pve-installer v2 2/6] move RAID setup checks to RAID level enum implementations Michael Köppl
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Michael Köppl @ 2025-04-29 14:09 UTC (permalink / raw)
To: pve-devel
The GUI and TUI installers already implement checks to ensure systems
have the minimum required number of disks available for the various RAID
configurations (min 2 disks for RAID1, min 4 disks for RAID10, etc).
This change adds an early check of the answer file to the
auto-installer, improving the user experience by avoiding failure during
the actual installation.
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
proxmox-auto-install-assistant/src/main.rs | 7 ++--
proxmox-auto-installer/src/utils.rs | 18 ++++++++++-
proxmox-auto-installer/tests/parse-answer.rs | 4 ++-
.../btrfs_raid_single_disk.json | 3 ++
.../btrfs_raid_single_disk.toml | 15 +++++++++
.../zfs_raid_single_disk.json | 3 ++
.../zfs_raid_single_disk.toml | 15 +++++++++
proxmox-installer-common/src/options.rs | 32 +++++++++++++++++++
8 files changed, 92 insertions(+), 5 deletions(-)
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.toml
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.toml
diff --git a/proxmox-auto-install-assistant/src/main.rs b/proxmox-auto-install-assistant/src/main.rs
index b64623b..3cf3abc 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -14,9 +14,9 @@ use proxmox_auto_installer::{
answer::{Answer, FilterMatch},
sysinfo::SysInfo,
utils::{
- AutoInstSettings, FetchAnswerFrom, HttpOptions, get_matched_udev_indexes, get_nic_list,
- get_single_udev_index, verify_email_and_root_password_settings, verify_first_boot_settings,
- verify_locale_settings,
+ get_matched_udev_indexes, get_nic_list, get_single_udev_index, verify_disks_settings,
+ verify_email_and_root_password_settings, verify_first_boot_settings,
+ verify_locale_settings, AutoInstSettings, FetchAnswerFrom, HttpOptions,
},
};
use proxmox_installer_common::{FIRST_BOOT_EXEC_MAX_SIZE, FIRST_BOOT_EXEC_NAME};
@@ -597,6 +597,7 @@ fn parse_answer(path: impl AsRef<Path> + fmt::Debug) -> Result<Answer> {
match toml::from_str(&contents) {
Ok(answer) => {
verify_locale_settings(&answer, &serde_json::from_str(LOCALE_INFO)?)?;
+ verify_disks_settings(&answer)?;
verify_first_boot_settings(&answer)?;
verify_email_and_root_password_settings(&answer)?;
println!("The answer file was parsed successfully, no errors found!");
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index 365e01a..d6bc6e3 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -6,7 +6,8 @@ use std::{collections::BTreeMap, process::Command};
use crate::{
answer::{
- self, Answer, FirstBootHookSourceMode, FqdnConfig, FqdnExtendedConfig, FqdnSourceMode,
+ self, Answer, DiskSelection, FirstBootHookSourceMode, FqdnConfig, FqdnExtendedConfig,
+ FqdnSourceMode,
},
udevinfo::UdevInfo,
};
@@ -384,6 +385,20 @@ pub fn verify_email_and_root_password_settings(answer: &Answer) -> Result<()> {
}
}
+pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
+ if let DiskSelection::Selection(selection) = &answer.disks.disk_selection {
+ let min_disks = answer.disks.fs_type.get_min_disks();
+ if selection.len() < min_disks {
+ bail!(
+ "{} requires at least {} disks",
+ answer.disks.fs_type,
+ min_disks
+ );
+ }
+ }
+ Ok(())
+}
+
pub fn verify_first_boot_settings(answer: &Answer) -> Result<()> {
info!("Verifying first boot settings");
@@ -414,6 +429,7 @@ pub fn parse_answer(
let network_settings = get_network_settings(answer, udev_info, runtime_info, setup_info)?;
verify_locale_settings(answer, locales)?;
+ verify_disks_settings(answer)?;
verify_email_and_root_password_settings(answer)?;
verify_first_boot_settings(answer)?;
diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index 34bc969..92dba63 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -142,11 +142,13 @@ mod tests {
run_named_fail_parse_test,
// Keep below entries alphabetically sorted
both_password_and_hashed_set,
+ btrfs_raid_single_disk,
fqdn_from_dhcp_no_default_domain,
fqdn_hostname_only,
no_fqdn_from_dhcp,
no_root_password_set,
- short_password
+ short_password,
+ zfs_raid_single_disk
);
}
}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.json
new file mode 100644
index 0000000..37f35fe
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.json
@@ -0,0 +1,3 @@
+{
+ "error": "BTRFS (RAID1) requires at least 2 disks"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.toml
new file mode 100644
index 0000000..e96fb16
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.toml
@@ -0,0 +1,15 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "btrfs-raid-single-disk.fail.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root-password = "12345678"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "btrfs"
+btrfs.raid = "raid1"
+disk-list = ["sda"]
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.json
new file mode 100644
index 0000000..9a8cc90
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.json
@@ -0,0 +1,3 @@
+{
+ "error": "ZFS (RAID10) requires at least 4 disks"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.toml
new file mode 100644
index 0000000..94ae748
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.toml
@@ -0,0 +1,15 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "zfs-raid-single-disk.fail.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root-password = "12345678"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "zfs"
+zfs.raid = "raid10"
+disk-list = ["sda"]
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 9cc4ee0..9271b8b 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -20,6 +20,16 @@ pub enum BtrfsRaidLevel {
Raid10,
}
+impl BtrfsRaidLevel {
+ pub fn get_min_disks(&self) -> usize {
+ match self {
+ BtrfsRaidLevel::Raid0 => 1,
+ BtrfsRaidLevel::Raid1 => 2,
+ BtrfsRaidLevel::Raid10 => 4,
+ }
+ }
+}
+
serde_plain::derive_display_from_serialize!(BtrfsRaidLevel);
#[derive(Copy, Clone, Debug, Deserialize, Serialize, Eq, PartialEq)]
@@ -48,6 +58,19 @@ pub enum ZfsRaidLevel {
RaidZ3,
}
+impl ZfsRaidLevel {
+ pub fn get_min_disks(&self) -> usize {
+ match self {
+ ZfsRaidLevel::Raid0 => 1,
+ ZfsRaidLevel::Raid1 => 2,
+ ZfsRaidLevel::Raid10 => 4,
+ ZfsRaidLevel::RaidZ => 3,
+ ZfsRaidLevel::RaidZ2 => 4,
+ ZfsRaidLevel::RaidZ3 => 5,
+ }
+ }
+}
+
serde_plain::derive_display_from_serialize!(ZfsRaidLevel);
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -67,6 +90,15 @@ impl FsType {
pub fn is_lvm(&self) -> bool {
matches!(self, FsType::Ext4 | FsType::Xfs)
}
+
+ pub fn get_min_disks(&self) -> usize {
+ match self {
+ FsType::Ext4 => 1,
+ FsType::Xfs => 1,
+ FsType::Zfs(level) => level.get_min_disks(),
+ FsType::Btrfs(level) => level.get_min_disks(),
+ }
+ }
}
impl fmt::Display for FsType {
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* [pve-devel] [PATCH pve-installer v2 2/6] move RAID setup checks to RAID level enum implementations
2025-04-29 14:09 [pve-devel] [PATCH installer v2 0/6] add early disk and network sanity checks Michael Köppl
2025-04-29 14:09 ` [pve-devel] [PATCH pve-installer v2 1/6] auto: add early answer file sanity check for RAID configurations Michael Köppl
@ 2025-04-29 14:09 ` Michael Köppl
2025-04-29 14:09 ` [pve-devel] [PATCH pve-installer v2 3/6] close #5887: add sanity check for LVM swapsize and maxroot Michael Köppl
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Michael Köppl @ 2025-04-29 14:09 UTC (permalink / raw)
To: pve-devel
Instead of having parts of the RAID setup checks scattered in multiple
places, move the core of the checks to implementations of the
ZfsRaidLevel and BtrfsRaidLevel enums.
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
proxmox-installer-common/src/disk_checks.rs | 156 ++++----------------
proxmox-installer-common/src/options.rs | 59 ++++++++
proxmox-tui-installer/src/views/bootdisk.rs | 13 +-
3 files changed, 96 insertions(+), 132 deletions(-)
diff --git a/proxmox-installer-common/src/disk_checks.rs b/proxmox-installer-common/src/disk_checks.rs
index ecc43bd..d535837 100644
--- a/proxmox-installer-common/src/disk_checks.rs
+++ b/proxmox-installer-common/src/disk_checks.rs
@@ -1,6 +1,6 @@
use std::collections::HashSet;
-use crate::options::{BtrfsRaidLevel, Disk, ZfsRaidLevel};
+use crate::options::Disk;
use crate::setup::BootType;
/// Checks a list of disks for duplicate entries, using their index as key.
@@ -49,94 +49,10 @@ pub fn check_disks_4kn_legacy_boot(boot_type: BootType, disks: &[Disk]) -> Resul
Ok(())
}
-/// Checks whether a user-supplied ZFS RAID setup is valid or not, such as disk sizes andminimum
-/// number of disks.
-///
-/// # Arguments
-///
-/// * `level` - The targeted ZFS RAID level by the user.
-/// * `disks` - List of disks designated as RAID targets.
-pub fn check_zfs_raid_config(level: ZfsRaidLevel, disks: &[Disk]) -> Result<(), String> {
- // See also Proxmox/Install.pm:get_zfs_raid_setup()
-
- let check_mirror_size = |disk1: &Disk, disk2: &Disk| {
- if (disk1.size - disk2.size).abs() > disk1.size / 10. {
- Err(format!(
- "Mirrored disks must have same size:\n\n * {disk1}\n * {disk2}"
- ))
- } else {
- Ok(())
- }
- };
-
- match level {
- ZfsRaidLevel::Raid0 => check_raid_min_disks(disks, 1)?,
- ZfsRaidLevel::Raid1 => {
- check_raid_min_disks(disks, 2)?;
- for disk in disks {
- check_mirror_size(&disks[0], disk)?;
- }
- }
- ZfsRaidLevel::Raid10 => {
- check_raid_min_disks(disks, 4)?;
-
- if disks.len() % 2 != 0 {
- return Err(format!(
- "Needs an even number of disks, currently selected: {}",
- disks.len(),
- ));
- }
-
- // Pairs need to have the same size
- for i in (0..disks.len()).step_by(2) {
- check_mirror_size(&disks[i], &disks[i + 1])?;
- }
- }
- // For RAID-Z: minimum disks number is level + 2
- ZfsRaidLevel::RaidZ => {
- check_raid_min_disks(disks, 3)?;
- for disk in disks {
- check_mirror_size(&disks[0], disk)?;
- }
- }
- ZfsRaidLevel::RaidZ2 => {
- check_raid_min_disks(disks, 4)?;
- for disk in disks {
- check_mirror_size(&disks[0], disk)?;
- }
- }
- ZfsRaidLevel::RaidZ3 => {
- check_raid_min_disks(disks, 5)?;
- for disk in disks {
- check_mirror_size(&disks[0], disk)?;
- }
- }
- }
-
- Ok(())
-}
-
-/// Checks whether a user-supplied Btrfs RAID setup is valid or not, such as minimum
-/// number of disks.
-///
-/// # Arguments
-///
-/// * `level` - The targeted Btrfs RAID level by the user.
-/// * `disks` - List of disks designated as RAID targets.
-pub fn check_btrfs_raid_config(level: BtrfsRaidLevel, disks: &[Disk]) -> Result<(), String> {
- // See also Proxmox/Install.pm:get_btrfs_raid_setup()
-
- match level {
- BtrfsRaidLevel::Raid0 => check_raid_min_disks(disks, 1)?,
- BtrfsRaidLevel::Raid1 => check_raid_min_disks(disks, 2)?,
- BtrfsRaidLevel::Raid10 => check_raid_min_disks(disks, 4)?,
- }
-
- Ok(())
-}
-
#[cfg(test)]
mod tests {
+ use crate::options::{BtrfsRaidLevel, ZfsRaidLevel};
+
use super::*;
fn dummy_disk(index: usize) -> Disk {
@@ -194,50 +110,38 @@ mod tests {
fn btrfs_raid() {
let disks = dummy_disks(10);
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid0, &[]).is_err());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid0, &disks[..1]).is_ok());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid0, &disks).is_ok());
-
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &[]).is_err());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks[..1]).is_err());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks[..2]).is_ok());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks).is_ok());
-
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &[]).is_err());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..3]).is_err());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..4]).is_ok());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks).is_ok());
+ let btrfs_raid_variants = [
+ BtrfsRaidLevel::Raid0,
+ BtrfsRaidLevel::Raid1,
+ BtrfsRaidLevel::Raid10,
+ ];
+
+ for v in btrfs_raid_variants {
+ assert!(v.check_disks(&[]).is_err());
+ assert!(v.check_disks(&disks[..v.get_min_disks() - 1]).is_err());
+ assert!(v.check_disks(&disks[..v.get_min_disks()]).is_ok());
+ assert!(v.check_disks(&disks).is_ok());
+ }
}
#[test]
fn zfs_raid() {
let disks = dummy_disks(10);
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid0, &[]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid0, &disks[..1]).is_ok());
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid0, &disks).is_ok());
-
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid1, &[]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid1, &disks[..2]).is_ok());
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid1, &disks).is_ok());
-
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid10, &[]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid10, &dummy_disks(4)).is_ok());
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid10, &disks).is_ok());
-
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &[]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &disks[..2]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &disks[..3]).is_ok());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &disks).is_ok());
-
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &[]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &disks[..3]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &disks[..4]).is_ok());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &disks).is_ok());
-
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &[]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &disks[..4]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &disks[..5]).is_ok());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &disks).is_ok());
+ let zfs_raid_variants = [
+ ZfsRaidLevel::Raid0,
+ ZfsRaidLevel::Raid1,
+ ZfsRaidLevel::Raid10,
+ ZfsRaidLevel::RaidZ,
+ ZfsRaidLevel::RaidZ2,
+ ZfsRaidLevel::RaidZ3,
+ ];
+
+ for v in zfs_raid_variants {
+ assert!(v.check_disks(&[]).is_err());
+ assert!(v.check_disks(&disks[..v.get_min_disks() - 1]).is_err());
+ assert!(v.check_disks(&disks[..v.get_min_disks()]).is_ok());
+ assert!(v.check_disks(&disks).is_ok());
+ }
}
}
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 9271b8b..0552954 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -6,6 +6,7 @@ use std::str::FromStr;
use std::sync::OnceLock;
use std::{cmp, fmt};
+use crate::disk_checks::check_raid_min_disks;
use crate::setup::{LocaleInfo, NetworkInfo, RuntimeInfo, SetupInfo};
use crate::utils::{CidrAddress, Fqdn};
@@ -28,6 +29,17 @@ impl BtrfsRaidLevel {
BtrfsRaidLevel::Raid10 => 4,
}
}
+
+ /// Checks whether a user-supplied Btrfs RAID setup is valid or not, such as minimum
+ /// number of disks.
+ ///
+ /// # Arguments
+ ///
+ /// * `disks` - List of disks designated as RAID targets.
+ pub fn check_disks(&self, disks: &[Disk]) -> Result<(), String> {
+ check_raid_min_disks(disks, self.get_min_disks())?;
+ Ok(())
+ }
}
serde_plain::derive_display_from_serialize!(BtrfsRaidLevel);
@@ -69,6 +81,53 @@ impl ZfsRaidLevel {
ZfsRaidLevel::RaidZ3 => 5,
}
}
+
+ fn check_mirror_size(&self, disk1: &Disk, disk2: &Disk) -> Result<(), String> {
+ if (disk1.size - disk2.size).abs() > disk1.size / 10. {
+ Err(format!(
+ "Mirrored disks must have same size:\n\n * {disk1}\n * {disk2}"
+ ))
+ } else {
+ Ok(())
+ }
+ }
+
+ /// Checks whether a user-supplied ZFS RAID setup is valid or not, such as disk sizes andminimum
+ /// number of disks.
+ ///
+ /// # Arguments
+ ///
+ /// * `disks` - List of disks designated as RAID targets.
+ pub fn check_disks(&self, disks: &[Disk]) -> Result<(), String> {
+ check_raid_min_disks(disks, self.get_min_disks())?;
+
+ match self {
+ ZfsRaidLevel::Raid0 => {}
+ ZfsRaidLevel::Raid10 => {
+ if disks.len() % 2 != 0 {
+ return Err(format!(
+ "Needs an even number of disks, currently selected: {}",
+ disks.len(),
+ ));
+ }
+
+ // Pairs need to have the same size
+ for i in (0..disks.len()).step_by(2) {
+ self.check_mirror_size(&disks[i], &disks[i + 1])?;
+ }
+ }
+ ZfsRaidLevel::Raid1
+ | ZfsRaidLevel::RaidZ
+ | ZfsRaidLevel::RaidZ2
+ | ZfsRaidLevel::RaidZ3 => {
+ for disk in disks {
+ self.check_mirror_size(&disks[0], disk)?;
+ }
+ }
+ }
+
+ Ok(())
+ }
}
serde_plain::derive_display_from_serialize!(ZfsRaidLevel);
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 313a3c9..e87b040 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -17,10 +17,7 @@ use crate::InstallerState;
use crate::options::FS_TYPES;
use proxmox_installer_common::{
- disk_checks::{
- check_btrfs_raid_config, check_disks_4kn_legacy_boot, check_for_duplicate_disks,
- check_zfs_raid_config,
- },
+ disk_checks::{check_disks_4kn_legacy_boot, check_for_duplicate_disks},
options::{
AdvancedBootdiskOptions, BTRFS_COMPRESS_OPTIONS, BootdiskOptions, BtrfsBootdiskOptions,
Disk, FsType, LvmBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
@@ -275,7 +272,9 @@ impl AdvancedBootdiskOptionsView {
.ok_or("Failed to retrieve advanced bootdisk options")?;
if let FsType::Zfs(level) = fstype {
- check_zfs_raid_config(level, &disks).map_err(|err| format!("{fstype}: {err}"))?;
+ level
+ .check_disks(&disks)
+ .map_err(|err| format!("{fstype}: {err}"))?;
}
Ok(BootdiskOptions {
@@ -289,7 +288,9 @@ impl AdvancedBootdiskOptionsView {
.ok_or("Failed to retrieve advanced bootdisk options")?;
if let FsType::Btrfs(level) = fstype {
- check_btrfs_raid_config(level, &disks).map_err(|err| format!("{fstype}: {err}"))?;
+ level
+ .check_disks(&disks)
+ .map_err(|err| format!("{fstype}: {err}"))?;
}
Ok(BootdiskOptions {
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* [pve-devel] [PATCH pve-installer v2 3/6] close #5887: add sanity check for LVM swapsize and maxroot
2025-04-29 14:09 [pve-devel] [PATCH installer v2 0/6] add early disk and network sanity checks Michael Köppl
2025-04-29 14:09 ` [pve-devel] [PATCH pve-installer v2 1/6] auto: add early answer file sanity check for RAID configurations Michael Köppl
2025-04-29 14:09 ` [pve-devel] [PATCH pve-installer v2 2/6] move RAID setup checks to RAID level enum implementations Michael Köppl
@ 2025-04-29 14:09 ` Michael Köppl
2025-05-06 11:48 ` Christoph Heiss
2025-04-29 14:09 ` [pve-devel] [PATCH pve-installer v2 4/6] auto: add check for duplicate disks in answer file Michael Köppl
` (2 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Michael Köppl @ 2025-04-29 14:09 UTC (permalink / raw)
To: pve-devel
Check that the configured swapsize is not greater than hdsize / 8 as
stated in the admin guide [0]. Additionally check that maxroot is at
most hdsize / 4. Define the behavior for the auto-installer as well as
the TUI and GUI installers.
[0] https://pve.proxmox.com/pve-docs/pve-admin-guide.html#advanced_lvm_options
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
Note regarding the change around set_hdsize in proxinstall: Before this
change, the hdsize was only set if the user manually changed it. If the
user did not change it, any checks against hdsize would check against
undefined.
Proxmox/Install.pm | 16 ++++++
proxinstall | 5 +-
proxmox-auto-installer/src/utils.rs | 16 ++++++
proxmox-auto-installer/tests/parse-answer.rs | 4 +-
.../lvm_maxroot_greater_than_maximum.json | 3 ++
.../lvm_maxroot_greater_than_maximum.toml | 16 ++++++
.../lvm_swapsize_greater_than_hdsize.json | 3 ++
.../lvm_swapsize_greater_than_hdsize.toml | 16 ++++++
proxmox-installer-common/src/disk_checks.rs | 52 ++++++++++++++++++-
proxmox-tui-installer/src/views/bootdisk.rs | 6 ++-
10 files changed, 133 insertions(+), 4 deletions(-)
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.toml
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml
diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index f673604..7c55d1f 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -592,6 +592,22 @@ sub compute_swapsize {
return $swapsize_kb;
}
+sub swapsize_check {
+ my ($hdsize) = @_;
+ my $swapsize = Proxmox::Install::Config::get_swapsize();
+ my $threshold = $hdsize / 8;
+ die "swap size ${swapsize} GiB cannot be greater than ${threshold} GiB (hard disk size / 8)\n"
+ if $swapsize > $threshold;
+}
+
+sub maxroot_size_check {
+ my ($hdsize) = @_;
+ my $maxroot = Proxmox::Install::Config::get_maxroot();
+ my $threshold = $hdsize / 4;
+ die "maximum root volume size ${maxroot} GiB cannot be greater than ${threshold} GiB (hard disk size / 4)\n"
+ if $maxroot > $threshold;
+}
+
my sub chroot_chown {
my ($root, $path, %param) = @_;
diff --git a/proxinstall b/proxinstall
index bc9ade6..e9ff6e8 100755
--- a/proxinstall
+++ b/proxinstall
@@ -1406,7 +1406,7 @@ sub create_hdoption_view {
my $tmp;
- if (($tmp = &$get_float($spinbutton_hdsize)) && ($tmp != $hdsize)) {
+ if (defined($tmp = &$get_float($spinbutton_hdsize))) {
Proxmox::Install::Config::set_hdsize($tmp);
} else {
Proxmox::Install::Config::set_hdsize(undef);
@@ -1521,9 +1521,12 @@ sub create_hdsel_view {
$target_hds = [ map { $_->[1] } @$devlist ];
} else {
my $target_hd = Proxmox::Install::Config::get_target_hd();
+ my $hdsize = Proxmox::Install::Config::get_hdsize();
eval {
my $target_block_size = Proxmox::Sys::Block::logical_blocksize($target_hd);
Proxmox::Install::legacy_bios_4k_check($target_block_size);
+ Proxmox::Install::swapsize_check($hdsize);
+ Proxmox::Install::maxroot_size_check($hdsize);
};
if (my $err = $@) {
Proxmox::UI::message("Warning: $err\n");
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index d6bc6e3..75696bd 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -13,6 +13,7 @@ use crate::{
};
use proxmox_installer_common::{
ROOT_PASSWORD_MIN_LENGTH,
+ disk_checks::{check_maxroot, check_swapsize},
options::{FsType, NetworkOptions, ZfsChecksumOption, ZfsCompressOption, email_validate},
setup::{
InstallBtrfsOption, InstallConfig, InstallFirstBootSetup, InstallRootPassword,
@@ -396,6 +397,21 @@ pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
);
}
}
+
+ if let answer::FsOptions::LVM(lvm) = &answer.disks.fs_options {
+ if let Some((swapsize, hdsize)) = lvm.swapsize.zip(lvm.hdsize) {
+ if let Err(err) = check_swapsize(swapsize, hdsize) {
+ bail!(err);
+ }
+ }
+
+ if let Some((maxroot, hdsize)) = lvm.maxroot.zip(lvm.hdsize) {
+ if let Err(err) = check_maxroot(maxroot, hdsize) {
+ bail!(err);
+ }
+ }
+ }
+
Ok(())
}
diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index 92dba63..e615672 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -7,7 +7,7 @@ use proxmox_auto_installer::udevinfo::UdevInfo;
use proxmox_auto_installer::utils::parse_answer;
use proxmox_installer_common::setup::{
- LocaleInfo, RuntimeInfo, SetupInfo, load_installer_setup_files, read_json,
+ load_installer_setup_files, read_json, LocaleInfo, RuntimeInfo, SetupInfo,
};
fn get_test_resource_path() -> Result<PathBuf, String> {
@@ -145,6 +145,8 @@ mod tests {
btrfs_raid_single_disk,
fqdn_from_dhcp_no_default_domain,
fqdn_hostname_only,
+ lvm_maxroot_greater_than_maximum,
+ lvm_swapsize_greater_than_hdsize,
no_fqdn_from_dhcp,
no_root_password_set,
short_password,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json
new file mode 100644
index 0000000..bab12e6
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json
@@ -0,0 +1,3 @@
+{
+ "error": "Maximum root volume size 8.01 GiB cannot be greater than 8 GiB (hard disk size / 4)"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.toml
new file mode 100644
index 0000000..e934d29
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.toml
@@ -0,0 +1,16 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "btrfs-raid-single-disk.fail.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root-password = "12345678"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "ext4"
+lvm.maxroot = 8.01
+lvm.hdsize = 32
+disk-list = ["sda"]
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
new file mode 100644
index 0000000..aa4f7fe
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
@@ -0,0 +1,3 @@
+{
+ "error": "Swap size 4.01 GiB cannot be greater than 4 GiB (hard disk size / 8)"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml
new file mode 100644
index 0000000..ffe16dc
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml
@@ -0,0 +1,16 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "btrfs-raid-single-disk.fail.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root-password = "12345678"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "ext4"
+lvm.swapsize = 4.01
+lvm.hdsize = 32
+disk-list = ["sda"]
diff --git a/proxmox-installer-common/src/disk_checks.rs b/proxmox-installer-common/src/disk_checks.rs
index d535837..77104ae 100644
--- a/proxmox-installer-common/src/disk_checks.rs
+++ b/proxmox-installer-common/src/disk_checks.rs
@@ -1,6 +1,6 @@
use std::collections::HashSet;
-use crate::options::Disk;
+use crate::options::{Disk, LvmBootdiskOptions};
use crate::setup::BootType;
/// Checks a list of disks for duplicate entries, using their index as key.
@@ -49,6 +49,56 @@ pub fn check_disks_4kn_legacy_boot(boot_type: BootType, disks: &[Disk]) -> Resul
Ok(())
}
+/// Checks whether the configured swap size exceeds the allowed threshold.
+///
+/// # Arguments
+///
+/// * `swapsize` - The size of the swap in GiB
+/// * `hdsize` - The total size of the hard disk in GiB
+pub fn check_swapsize(swapsize: f64, hdsize: f64) -> Result<(), String> {
+ let threshold = hdsize / 8.0;
+ if swapsize > threshold {
+ return Err(format!(
+ "Swap size {swapsize} GiB cannot be greater than {threshold} GiB (hard disk size / 8)",
+ ));
+ }
+ Ok(())
+}
+
+/// Checks whether the configured root volume size exceeds the allowed threshold.
+///
+/// # Arguments
+///
+/// * `maxroot` - The size of the root volume in GiB
+/// * `hdsize` - The total size of the hard disk in GiB
+pub fn check_maxroot(maxroot: f64, hdsize: f64) -> Result<(), String> {
+ let threshold = hdsize / 4.0;
+ if maxroot > threshold {
+ return Err(format!(
+ "Maximum root volume size {maxroot} GiB cannot be greater than {threshold} GiB (hard disk size / 4)",
+ ));
+ }
+ Ok(())
+}
+
+/// Checks whether a user-supplied LVM setup is valid or not, such as the swapsize or maxroot not
+/// exceeding certain thresholds.
+///
+/// # Arguments
+///
+/// * `bootdisk_opts` - The LVM options set by the user.
+pub fn check_lvm_bootdisk_opts(bootdisk_opts: &LvmBootdiskOptions) -> Result<(), String> {
+ if let Some(swap_size) = bootdisk_opts.swap_size {
+ check_swapsize(swap_size, bootdisk_opts.total_size)?;
+ }
+
+ if let Some(max_root_size) = bootdisk_opts.max_root_size {
+ check_maxroot(max_root_size, bootdisk_opts.total_size)?;
+ }
+
+ Ok(())
+}
+
#[cfg(test)]
mod tests {
use crate::options::{BtrfsRaidLevel, ZfsRaidLevel};
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index e87b040..b94cf38 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -17,7 +17,9 @@ use crate::InstallerState;
use crate::options::FS_TYPES;
use proxmox_installer_common::{
- disk_checks::{check_disks_4kn_legacy_boot, check_for_duplicate_disks},
+ disk_checks::{
+ check_disks_4kn_legacy_boot, check_for_duplicate_disks, check_lvm_bootdisk_opts,
+ },
options::{
AdvancedBootdiskOptions, BTRFS_COMPRESS_OPTIONS, BootdiskOptions, BtrfsBootdiskOptions,
Disk, FsType, LvmBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
@@ -261,6 +263,8 @@ impl AdvancedBootdiskOptionsView {
.get_values()
.ok_or("Failed to retrieve advanced bootdisk options")?;
+ check_lvm_bootdisk_opts(&advanced).map_err(|err| format!("{fstype}: {err}"))?;
+
Ok(BootdiskOptions {
disks: vec![disk],
fstype,
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [pve-devel] [PATCH pve-installer v2 3/6] close #5887: add sanity check for LVM swapsize and maxroot
2025-04-29 14:09 ` [pve-devel] [PATCH pve-installer v2 3/6] close #5887: add sanity check for LVM swapsize and maxroot Michael Köppl
@ 2025-05-06 11:48 ` Christoph Heiss
2025-05-09 11:07 ` Michael Köppl
0 siblings, 1 reply; 12+ messages in thread
From: Christoph Heiss @ 2025-05-06 11:48 UTC (permalink / raw)
To: Michael Köppl; +Cc: Proxmox VE development discussion
After testing this change and thinking about the maxroot change again,
$hdsize / 4 doesn't really make sense. E.g. for an (unrealistically
small, but still) disk of 8 GiB; if its unset, pve-root will be ~6.5 GiB
in size, with the limit of 2 GiB, the installation fails due to
ENOSPACE.
The default calculations try really hard to make installations possible
even on small disks, in Proxmox/Install.pm:create_lvm_volumes()
So I'm not sure if we really should restrict it that much, or rather
relax it in the documentation.
On Tue Apr 29, 2025 at 4:09 PM CEST, Michael Köppl wrote:
> Check that the configured swapsize is not greater than hdsize / 8 as
> stated in the admin guide [0]. Additionally check that maxroot is at
> most hdsize / 4. Define the behavior for the auto-installer as well as
> the TUI and GUI installers.
>
> [0] https://pve.proxmox.com/pve-docs/pve-admin-guide.html#advanced_lvm_options
>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
> Note regarding the change around set_hdsize in proxinstall: Before this
> change, the hdsize was only set if the user manually changed it. If the
> user did not change it, any checks against hdsize would check against
> undefined.
>
> Proxmox/Install.pm | 16 ++++++
> proxinstall | 5 +-
> proxmox-auto-installer/src/utils.rs | 16 ++++++
> proxmox-auto-installer/tests/parse-answer.rs | 4 +-
> .../lvm_maxroot_greater_than_maximum.json | 3 ++
> .../lvm_maxroot_greater_than_maximum.toml | 16 ++++++
> .../lvm_swapsize_greater_than_hdsize.json | 3 ++
> .../lvm_swapsize_greater_than_hdsize.toml | 16 ++++++
> proxmox-installer-common/src/disk_checks.rs | 52 ++++++++++++++++++-
> proxmox-tui-installer/src/views/bootdisk.rs | 6 ++-
> 10 files changed, 133 insertions(+), 4 deletions(-)
> create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json
> create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.toml
> create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
> create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml
>
> diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
> index f673604..7c55d1f 100644
> --- a/Proxmox/Install.pm
> +++ b/Proxmox/Install.pm
> @@ -592,6 +592,22 @@ sub compute_swapsize {
> return $swapsize_kb;
> }
>
> +sub swapsize_check {
> + my ($hdsize) = @_;
> + my $swapsize = Proxmox::Install::Config::get_swapsize();
> + my $threshold = $hdsize / 8;
> + die "swap size ${swapsize} GiB cannot be greater than ${threshold} GiB (hard disk size / 8)\n"
> + if $swapsize > $threshold;
> +}
> +
> +sub maxroot_size_check {
> + my ($hdsize) = @_;
> + my $maxroot = Proxmox::Install::Config::get_maxroot();
> + my $threshold = $hdsize / 4;
> + die "maximum root volume size ${maxroot} GiB cannot be greater than ${threshold} GiB (hard disk size / 4)\n"
> + if $maxroot > $threshold;
> +}
> +
> my sub chroot_chown {
> my ($root, $path, %param) = @_;
>
> diff --git a/proxinstall b/proxinstall
> index bc9ade6..e9ff6e8 100755
> --- a/proxinstall
> +++ b/proxinstall
> @@ -1406,7 +1406,7 @@ sub create_hdoption_view {
>
> my $tmp;
>
> - if (($tmp = &$get_float($spinbutton_hdsize)) && ($tmp != $hdsize)) {
> + if (defined($tmp = &$get_float($spinbutton_hdsize))) {
> Proxmox::Install::Config::set_hdsize($tmp);
> } else {
> Proxmox::Install::Config::set_hdsize(undef);
> @@ -1521,9 +1521,12 @@ sub create_hdsel_view {
> $target_hds = [ map { $_->[1] } @$devlist ];
> } else {
> my $target_hd = Proxmox::Install::Config::get_target_hd();
> + my $hdsize = Proxmox::Install::Config::get_hdsize();
> eval {
> my $target_block_size = Proxmox::Sys::Block::logical_blocksize($target_hd);
> Proxmox::Install::legacy_bios_4k_check($target_block_size);
> + Proxmox::Install::swapsize_check($hdsize);
> + Proxmox::Install::maxroot_size_check($hdsize);
> };
> if (my $err = $@) {
> Proxmox::UI::message("Warning: $err\n");
> diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
> index d6bc6e3..75696bd 100644
> --- a/proxmox-auto-installer/src/utils.rs
> +++ b/proxmox-auto-installer/src/utils.rs
> @@ -13,6 +13,7 @@ use crate::{
> };
> use proxmox_installer_common::{
> ROOT_PASSWORD_MIN_LENGTH,
> + disk_checks::{check_maxroot, check_swapsize},
> options::{FsType, NetworkOptions, ZfsChecksumOption, ZfsCompressOption, email_validate},
> setup::{
> InstallBtrfsOption, InstallConfig, InstallFirstBootSetup, InstallRootPassword,
> @@ -396,6 +397,21 @@ pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
> );
> }
> }
> +
> + if let answer::FsOptions::LVM(lvm) = &answer.disks.fs_options {
> + if let Some((swapsize, hdsize)) = lvm.swapsize.zip(lvm.hdsize) {
> + if let Err(err) = check_swapsize(swapsize, hdsize) {
> + bail!(err);
> + }
> + }
> +
> + if let Some((maxroot, hdsize)) = lvm.maxroot.zip(lvm.hdsize) {
> + if let Err(err) = check_maxroot(maxroot, hdsize) {
> + bail!(err);
> + }
> + }
> + }
> +
> Ok(())
> }
>
> diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
> index 92dba63..e615672 100644
> --- a/proxmox-auto-installer/tests/parse-answer.rs
> +++ b/proxmox-auto-installer/tests/parse-answer.rs
> @@ -7,7 +7,7 @@ use proxmox_auto_installer::udevinfo::UdevInfo;
> use proxmox_auto_installer::utils::parse_answer;
>
> use proxmox_installer_common::setup::{
> - LocaleInfo, RuntimeInfo, SetupInfo, load_installer_setup_files, read_json,
> + load_installer_setup_files, read_json, LocaleInfo, RuntimeInfo, SetupInfo,
> };
>
> fn get_test_resource_path() -> Result<PathBuf, String> {
> @@ -145,6 +145,8 @@ mod tests {
> btrfs_raid_single_disk,
> fqdn_from_dhcp_no_default_domain,
> fqdn_hostname_only,
> + lvm_maxroot_greater_than_maximum,
> + lvm_swapsize_greater_than_hdsize,
> no_fqdn_from_dhcp,
> no_root_password_set,
> short_password,
> diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json
> new file mode 100644
> index 0000000..bab12e6
> --- /dev/null
> +++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json
> @@ -0,0 +1,3 @@
> +{
> + "error": "Maximum root volume size 8.01 GiB cannot be greater than 8 GiB (hard disk size / 4)"
> +}
> diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.toml
> new file mode 100644
> index 0000000..e934d29
> --- /dev/null
> +++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.toml
> @@ -0,0 +1,16 @@
> +[global]
> +keyboard = "de"
> +country = "at"
> +fqdn = "btrfs-raid-single-disk.fail.testinstall"
> +mailto = "mail@no.invalid"
> +timezone = "Europe/Vienna"
> +root-password = "12345678"
> +
> +[network]
> +source = "from-dhcp"
> +
> +[disk-setup]
> +filesystem = "ext4"
> +lvm.maxroot = 8.01
> +lvm.hdsize = 32
> +disk-list = ["sda"]
> diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
> new file mode 100644
> index 0000000..aa4f7fe
> --- /dev/null
> +++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
> @@ -0,0 +1,3 @@
> +{
> + "error": "Swap size 4.01 GiB cannot be greater than 4 GiB (hard disk size / 8)"
> +}
> diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml
> new file mode 100644
> index 0000000..ffe16dc
> --- /dev/null
> +++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml
> @@ -0,0 +1,16 @@
> +[global]
> +keyboard = "de"
> +country = "at"
> +fqdn = "btrfs-raid-single-disk.fail.testinstall"
> +mailto = "mail@no.invalid"
> +timezone = "Europe/Vienna"
> +root-password = "12345678"
> +
> +[network]
> +source = "from-dhcp"
> +
> +[disk-setup]
> +filesystem = "ext4"
> +lvm.swapsize = 4.01
> +lvm.hdsize = 32
> +disk-list = ["sda"]
> diff --git a/proxmox-installer-common/src/disk_checks.rs b/proxmox-installer-common/src/disk_checks.rs
> index d535837..77104ae 100644
> --- a/proxmox-installer-common/src/disk_checks.rs
> +++ b/proxmox-installer-common/src/disk_checks.rs
> @@ -1,6 +1,6 @@
> use std::collections::HashSet;
>
> -use crate::options::Disk;
> +use crate::options::{Disk, LvmBootdiskOptions};
> use crate::setup::BootType;
>
> /// Checks a list of disks for duplicate entries, using their index as key.
> @@ -49,6 +49,56 @@ pub fn check_disks_4kn_legacy_boot(boot_type: BootType, disks: &[Disk]) -> Resul
> Ok(())
> }
>
> +/// Checks whether the configured swap size exceeds the allowed threshold.
> +///
> +/// # Arguments
> +///
> +/// * `swapsize` - The size of the swap in GiB
> +/// * `hdsize` - The total size of the hard disk in GiB
> +pub fn check_swapsize(swapsize: f64, hdsize: f64) -> Result<(), String> {
> + let threshold = hdsize / 8.0;
> + if swapsize > threshold {
> + return Err(format!(
> + "Swap size {swapsize} GiB cannot be greater than {threshold} GiB (hard disk size / 8)",
> + ));
> + }
> + Ok(())
> +}
> +
> +/// Checks whether the configured root volume size exceeds the allowed threshold.
> +///
> +/// # Arguments
> +///
> +/// * `maxroot` - The size of the root volume in GiB
> +/// * `hdsize` - The total size of the hard disk in GiB
> +pub fn check_maxroot(maxroot: f64, hdsize: f64) -> Result<(), String> {
> + let threshold = hdsize / 4.0;
> + if maxroot > threshold {
> + return Err(format!(
> + "Maximum root volume size {maxroot} GiB cannot be greater than {threshold} GiB (hard disk size / 4)",
> + ));
> + }
> + Ok(())
> +}
> +
> +/// Checks whether a user-supplied LVM setup is valid or not, such as the swapsize or maxroot not
> +/// exceeding certain thresholds.
> +///
> +/// # Arguments
> +///
> +/// * `bootdisk_opts` - The LVM options set by the user.
> +pub fn check_lvm_bootdisk_opts(bootdisk_opts: &LvmBootdiskOptions) -> Result<(), String> {
> + if let Some(swap_size) = bootdisk_opts.swap_size {
> + check_swapsize(swap_size, bootdisk_opts.total_size)?;
> + }
> +
> + if let Some(max_root_size) = bootdisk_opts.max_root_size {
> + check_maxroot(max_root_size, bootdisk_opts.total_size)?;
> + }
> +
> + Ok(())
> +}
> +
> #[cfg(test)]
> mod tests {
> use crate::options::{BtrfsRaidLevel, ZfsRaidLevel};
> diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
> index e87b040..b94cf38 100644
> --- a/proxmox-tui-installer/src/views/bootdisk.rs
> +++ b/proxmox-tui-installer/src/views/bootdisk.rs
> @@ -17,7 +17,9 @@ use crate::InstallerState;
> use crate::options::FS_TYPES;
>
> use proxmox_installer_common::{
> - disk_checks::{check_disks_4kn_legacy_boot, check_for_duplicate_disks},
> + disk_checks::{
> + check_disks_4kn_legacy_boot, check_for_duplicate_disks, check_lvm_bootdisk_opts,
> + },
> options::{
> AdvancedBootdiskOptions, BTRFS_COMPRESS_OPTIONS, BootdiskOptions, BtrfsBootdiskOptions,
> Disk, FsType, LvmBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
> @@ -261,6 +263,8 @@ impl AdvancedBootdiskOptionsView {
> .get_values()
> .ok_or("Failed to retrieve advanced bootdisk options")?;
>
> + check_lvm_bootdisk_opts(&advanced).map_err(|err| format!("{fstype}: {err}"))?;
> +
> Ok(BootdiskOptions {
> disks: vec![disk],
> fstype,
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [pve-devel] [PATCH pve-installer v2 3/6] close #5887: add sanity check for LVM swapsize and maxroot
2025-05-06 11:48 ` Christoph Heiss
@ 2025-05-09 11:07 ` Michael Köppl
2025-05-27 16:19 ` Michael Köppl
0 siblings, 1 reply; 12+ messages in thread
From: Michael Köppl @ 2025-05-09 11:07 UTC (permalink / raw)
To: Christoph Heiss; +Cc: Proxmox VE development discussion
On 5/6/25 13:48, Christoph Heiss wrote:
> After testing this change and thinking about the maxroot change again,
> $hdsize / 4 doesn't really make sense. E.g. for an (unrealistically
> small, but still) disk of 8 GiB; if its unset, pve-root will be ~6.5 GiB
> in size, with the limit of 2 GiB, the installation fails due to
> ENOSPACE.
>
> The default calculations try really hard to make installations possible
> even on small disks, in Proxmox/Install.pm:create_lvm_volumes()
>
> So I'm not sure if we really should restrict it that much, or rather
> relax it in the documentation.
Thought a bit about this. I agree that the sanity check should not
entirely stop users from creating setups that would work. An alternative
approach might be not to enforce the maxroot limit of $hdsize / 4 for
smaller disks. create_lvm_volumes() considers 12 GiB to be small, if I'm
not mistaken, and basically does a best-effort install. I'm not sure if
I like setting such an arbitrary limit for a sanity check, though. It
makes the sanity check intransparent if users cannot configure a root
volume size greater than 4 GiB if their hdsize is 16 GiB, but at the
same time are free to set it to 6 GiB if the hdsize is 8 GiB. What do
you think?
Another alternative might be to simply display a warning that doesn't
stop users from installing, i.e. "Recommended maximum root volume size
is hdsize / 4".
In both cases the documentation should be updated to reflect that.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pve-devel] [PATCH pve-installer v2 3/6] close #5887: add sanity check for LVM swapsize and maxroot
2025-05-09 11:07 ` Michael Köppl
@ 2025-05-27 16:19 ` Michael Köppl
0 siblings, 0 replies; 12+ messages in thread
From: Michael Köppl @ 2025-05-27 16:19 UTC (permalink / raw)
To: Christoph Heiss; +Cc: Proxmox VE development discussion
Ping, still applies
On 5/9/25 13:07, Michael Köppl wrote:
> On 5/6/25 13:48, Christoph Heiss wrote:
>> After testing this change and thinking about the maxroot change again,
>> $hdsize / 4 doesn't really make sense. E.g. for an (unrealistically
>> small, but still) disk of 8 GiB; if its unset, pve-root will be ~6.5 GiB
>> in size, with the limit of 2 GiB, the installation fails due to
>> ENOSPACE.
>>
>> The default calculations try really hard to make installations possible
>> even on small disks, in Proxmox/Install.pm:create_lvm_volumes()
>>
>> So I'm not sure if we really should restrict it that much, or rather
>> relax it in the documentation.
>
> Thought a bit about this. I agree that the sanity check should not
> entirely stop users from creating setups that would work. An alternative
> approach might be not to enforce the maxroot limit of $hdsize / 4 for
> smaller disks. create_lvm_volumes() considers 12 GiB to be small, if I'm
> not mistaken, and basically does a best-effort install. I'm not sure if
> I like setting such an arbitrary limit for a sanity check, though. It
> makes the sanity check intransparent if users cannot configure a root
> volume size greater than 4 GiB if their hdsize is 16 GiB, but at the
> same time are free to set it to 6 GiB if the hdsize is 8 GiB. What do
> you think?
>
> Another alternative might be to simply display a warning that doesn't
> stop users from installing, i.e. "Recommended maximum root volume size
> is hdsize / 4".
>
> In both cases the documentation should be updated to reflect that.
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [pve-devel] [PATCH pve-installer v2 4/6] auto: add check for duplicate disks in answer file
2025-04-29 14:09 [pve-devel] [PATCH installer v2 0/6] add early disk and network sanity checks Michael Köppl
` (2 preceding siblings ...)
2025-04-29 14:09 ` [pve-devel] [PATCH pve-installer v2 3/6] close #5887: add sanity check for LVM swapsize and maxroot Michael Köppl
@ 2025-04-29 14:09 ` Michael Köppl
2025-04-29 14:09 ` [pve-devel] [PATCH pve-installer v2 5/6] common: add more descriptive errors for invalid network configs Michael Köppl
2025-04-29 14:09 ` [pve-devel] [RFC PATCH pve-installer v2 6/6] common: add checks for valid subnet mask and IPv4 address within subnet Michael Köppl
5 siblings, 0 replies; 12+ messages in thread
From: Michael Köppl @ 2025-04-29 14:09 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
proxmox-auto-installer/src/utils.rs | 12 +++++++++++-
proxmox-auto-installer/tests/parse-answer.rs | 1 +
.../parse_answer_fail/duplicate_disk.json | 3 +++
.../parse_answer_fail/duplicate_disk.toml | 15 +++++++++++++++
4 files changed, 30 insertions(+), 1 deletion(-)
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.toml
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index 75696bd..3a1573e 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -2,7 +2,10 @@ use anyhow::{Context, Result, bail};
use clap::ValueEnum;
use glob::Pattern;
use log::info;
-use std::{collections::BTreeMap, process::Command};
+use std::{
+ collections::{BTreeMap, HashSet},
+ process::Command,
+};
use crate::{
answer::{
@@ -396,6 +399,13 @@ pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
min_disks
);
}
+
+ let mut disk_set = HashSet::new();
+ for disk in selection {
+ if !disk_set.insert(disk) {
+ bail!("List of disks contains duplicate disk {disk}");
+ }
+ }
}
if let answer::FsOptions::LVM(lvm) = &answer.disks.fs_options {
diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index e615672..f560483 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -143,6 +143,7 @@ mod tests {
// Keep below entries alphabetically sorted
both_password_and_hashed_set,
btrfs_raid_single_disk,
+ duplicate_disk,
fqdn_from_dhcp_no_default_domain,
fqdn_hostname_only,
lvm_maxroot_greater_than_maximum,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json
new file mode 100644
index 0000000..d01fabe
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json
@@ -0,0 +1,3 @@
+{
+ "error": "List of disks contains duplicate disk sda"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.toml
new file mode 100644
index 0000000..bff1631
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.toml
@@ -0,0 +1,15 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "btrfs-raid-single-disk.fail.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root-password = "12345678"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "btrfs"
+btrfs.raid = "raid1"
+disk-list = ["sda", "sdb", "sda"]
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* [pve-devel] [PATCH pve-installer v2 5/6] common: add more descriptive errors for invalid network configs
2025-04-29 14:09 [pve-devel] [PATCH installer v2 0/6] add early disk and network sanity checks Michael Köppl
` (3 preceding siblings ...)
2025-04-29 14:09 ` [pve-devel] [PATCH pve-installer v2 4/6] auto: add check for duplicate disks in answer file Michael Köppl
@ 2025-04-29 14:09 ` Michael Köppl
2025-04-29 14:09 ` [pve-devel] [RFC PATCH pve-installer v2 6/6] common: add checks for valid subnet mask and IPv4 address within subnet Michael Köppl
5 siblings, 0 replies; 12+ messages in thread
From: Michael Köppl @ 2025-04-29 14:09 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
proxmox-installer-common/src/utils.rs | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
index 8adcec0..1fe6a74 100644
--- a/proxmox-installer-common/src/utils.rs
+++ b/proxmox-installer-common/src/utils.rs
@@ -18,6 +18,20 @@ pub enum CidrAddressParseError {
InvalidMask(Option<ParseIntError>),
}
+impl fmt::Display for CidrAddressParseError {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ let msg = match &self {
+ CidrAddressParseError::NoDelimiter => {
+ String::from("No delimiter for separating address and mask was found")
+ }
+ CidrAddressParseError::InvalidAddr(addr_parse_error) => format!("{addr_parse_error}"),
+ CidrAddressParseError::InvalidMask(parse_int_error) => format!("{:?}", parse_int_error),
+ };
+
+ write!(f, "Invalid CIDR: {msg}")
+ }
+}
+
/// An IP address (IPv4 or IPv6), including network mask.
///
/// See the [`IpAddr`] type for more information how IP addresses are handled.
@@ -109,8 +123,7 @@ impl<'de> Deserialize<'de> for CidrAddress {
D: serde::Deserializer<'de>,
{
let s: String = Deserialize::deserialize(deserializer)?;
- s.parse()
- .map_err(|_| serde::de::Error::custom("invalid CIDR"))
+ s.parse().map_err(serde::de::Error::custom)
}
}
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* [pve-devel] [RFC PATCH pve-installer v2 6/6] common: add checks for valid subnet mask and IPv4 address within subnet
2025-04-29 14:09 [pve-devel] [PATCH installer v2 0/6] add early disk and network sanity checks Michael Köppl
` (4 preceding siblings ...)
2025-04-29 14:09 ` [pve-devel] [PATCH pve-installer v2 5/6] common: add more descriptive errors for invalid network configs Michael Köppl
@ 2025-04-29 14:09 ` Michael Köppl
2025-05-06 9:21 ` Christoph Heiss
5 siblings, 1 reply; 12+ messages in thread
From: Michael Köppl @ 2025-04-29 14:09 UTC (permalink / raw)
To: pve-devel
Add checks for valid subnet mask (greater than /0 and at most /32 for
IPv4). In addition, check if the address entered by the user is valid
within the given subnet, i.e. not a network address or broadcast
address. /31 is considered an exception in accordance with RFC3021 [0],
considering any of the 2 available addresses to be valid host addresses.
[0] https://datatracker.ietf.org/doc/html/rfc3021
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
Some input / discussion would be much appreciated here, since this might
again be considered too restrictive.
proxmox-auto-installer/tests/parse-answer.rs | 3 +
.../parse_answer/ipv4_and_subnet_31.json | 19 +++++
.../parse_answer/ipv4_and_subnet_31.toml | 18 +++++
.../ipv4_and_subnet_addr_is_network.json | 3 +
.../ipv4_and_subnet_addr_is_network.toml | 18 +++++
.../ipv4_and_subnet_mask_33.json | 3 +
.../ipv4_and_subnet_mask_33.toml | 18 +++++
proxmox-installer-common/src/utils.rs | 81 ++++++++++++++-----
8 files changed, 143 insertions(+), 20 deletions(-)
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml
diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index f560483..6f627a0 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -127,6 +127,7 @@ mod tests {
fqdn_from_dhcp_no_dhcp_domain_with_default_domain,
full_fqdn_from_dhcp_with_default_domain,
hashed_root_password,
+ ipv4_and_subnet_31,
minimal,
nic_matching,
specific_nic,
@@ -146,6 +147,8 @@ mod tests {
duplicate_disk,
fqdn_from_dhcp_no_default_domain,
fqdn_hostname_only,
+ ipv4_and_subnet_addr_is_network,
+ ipv4_and_subnet_mask_33,
lvm_maxroot_greater_than_maximum,
lvm_swapsize_greater_than_hdsize,
no_fqdn_from_dhcp,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json
new file mode 100644
index 0000000..2a475fe
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json
@@ -0,0 +1,19 @@
+{
+ "autoreboot": 1,
+ "cidr": "10.10.10.10/31",
+ "country": "at",
+ "dns": "10.10.10.1",
+ "domain": "testinstall",
+ "filesys": "ext4",
+ "gateway": "10.10.10.1",
+ "hdsize": 223.57088470458984,
+ "existing_storage_auto_rename": 1,
+ "hostname": "pveauto",
+ "keymap": "de",
+ "mailto": "mail@no.invalid",
+ "mngmt_nic": "enp129s0f1np1",
+ "root_password": { "plain": "12345678" },
+ "target_hd": "/dev/sda",
+ "timezone": "Europe/Vienna",
+ "first_boot": { "enabled": 0 }
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml
new file mode 100644
index 0000000..0ca6ca5
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml
@@ -0,0 +1,18 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password = "12345678"
+
+[network]
+source = "from-answer"
+cidr = "10.10.10.10/31"
+dns = "10.10.10.1"
+gateway = "10.10.10.1"
+filter.ID_NET_NAME = "enp129s0f1np1"
+
+[disk-setup]
+filesystem = "ext4"
+disk_list = ["sda"]
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json
new file mode 100644
index 0000000..b28699d
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json
@@ -0,0 +1,3 @@
+{
+ "parse-error": "error parsing answer.toml: Invalid CIDR: address is a network address"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml
new file mode 100644
index 0000000..7a9141b
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml
@@ -0,0 +1,18 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password = "12345678"
+
+[network]
+source = "from-answer"
+cidr = "10.10.10.32/27"
+dns = "10.10.10.1"
+gateway = "10.10.10.1"
+filter.ID_NET_NAME = "enp129s0f1np1"
+
+[disk-setup]
+filesystem = "ext4"
+disk_list = ["sda"]
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json
new file mode 100644
index 0000000..6b2888b
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json
@@ -0,0 +1,3 @@
+{
+ "parse-error": "error parsing answer.toml: Invalid CIDR: mask cannot be greater than 32"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml
new file mode 100644
index 0000000..16a560f
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml
@@ -0,0 +1,18 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password = "12345678"
+
+[network]
+source = "from-answer"
+cidr = "10.10.10.10/33"
+dns = "10.10.10.1"
+gateway = "10.10.10.1"
+filter.ID_NET_NAME = "enp129s0f1np1"
+
+[disk-setup]
+filesystem = "ext4"
+disk_list = ["sda"]
diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
index 1fe6a74..fea98db 100644
--- a/proxmox-installer-common/src/utils.rs
+++ b/proxmox-installer-common/src/utils.rs
@@ -1,7 +1,7 @@
use std::{
+ error::Error,
fmt,
net::{AddrParseError, IpAddr},
- num::ParseIntError,
str::FromStr,
};
@@ -15,20 +15,26 @@ pub enum CidrAddressParseError {
/// The IP address part could not be parsed.
InvalidAddr(AddrParseError),
/// The mask could not be parsed.
- InvalidMask(Option<ParseIntError>),
+ InvalidMask(Box<dyn Error>),
+ /// The IP address is a network address.
+ IsNetworkAddr,
+ /// The IP address is a broadcast address.
+ IsBroadcastAddr,
}
impl fmt::Display for CidrAddressParseError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
- let msg = match &self {
+ write!(f, "Invalid CIDR: ")?;
+
+ match self {
CidrAddressParseError::NoDelimiter => {
- String::from("No delimiter for separating address and mask was found")
+ write!(f, "no delimiter for separating address and mask was found")
}
- CidrAddressParseError::InvalidAddr(addr_parse_error) => format!("{addr_parse_error}"),
- CidrAddressParseError::InvalidMask(parse_int_error) => format!("{:?}", parse_int_error),
- };
-
- write!(f, "Invalid CIDR: {msg}")
+ CidrAddressParseError::InvalidAddr(err) => write!(f, "{err}"),
+ CidrAddressParseError::InvalidMask(err) => write!(f, "{err}"),
+ CidrAddressParseError::IsNetworkAddr => write!(f, "address is a network address"),
+ CidrAddressParseError::IsBroadcastAddr => write!(f, "address is a broadcast address"),
+ }
}
}
@@ -61,11 +67,10 @@ impl CidrAddress {
pub fn new<T: Into<IpAddr>>(addr: T, mask: usize) -> Result<Self, CidrAddressParseError> {
let addr = addr.into();
- if mask > mask_limit(&addr) {
- Err(CidrAddressParseError::InvalidMask(None))
- } else {
- Ok(Self { addr, mask })
- }
+ check_mask_limit(&addr, mask)?;
+ check_addr_valid_in_subnet(&addr, mask)?;
+
+ Ok(Self { addr, mask })
}
/// Returns only the IP address part of the address.
@@ -101,13 +106,12 @@ impl FromStr for CidrAddress {
let mask = mask
.parse()
- .map_err(|err| CidrAddressParseError::InvalidMask(Some(err)))?;
+ .map_err(|err| CidrAddressParseError::InvalidMask(Box::new(err)))?;
- if mask > mask_limit(&addr) {
- Err(CidrAddressParseError::InvalidMask(None))
- } else {
- Ok(Self { addr, mask })
- }
+ check_mask_limit(&addr, mask)?;
+ check_addr_valid_in_subnet(&addr, mask)?;
+
+ Ok(Self { addr, mask })
}
}
@@ -133,6 +137,43 @@ fn mask_limit(addr: &IpAddr) -> usize {
if addr.is_ipv4() { 32 } else { 128 }
}
+fn check_mask_limit(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
+ return if mask > mask_limit(&addr) {
+ Err(CidrAddressParseError::InvalidMask(
+ "mask cannot be greater than 32".into(),
+ ))
+ } else {
+ Ok(())
+ };
+}
+
+fn check_addr_valid_in_subnet(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
+ match &addr {
+ IpAddr::V4(addr_v4) => {
+ if mask >= 31 {
+ return Ok(());
+ }
+
+ let num_host_bits = 32 - mask;
+ let host_part_mask = (1u32 << num_host_bits) - 1;
+
+ let ip_bits = u32::from_be_bytes(addr_v4.octets());
+
+ let network_addr = ip_bits & !host_part_mask;
+ let broadcast_addr = network_addr | host_part_mask;
+
+ if ip_bits == network_addr {
+ Err(CidrAddressParseError::IsNetworkAddr)
+ } else if ip_bits == broadcast_addr {
+ Err(CidrAddressParseError::IsBroadcastAddr)
+ } else {
+ Ok(())
+ }
+ }
+ IpAddr::V6(_) => Ok(()),
+ }
+}
+
/// Possible errors that might occur when parsing FQDNs.
#[derive(Debug, Eq, PartialEq)]
pub enum FqdnParseError {
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [pve-devel] [RFC PATCH pve-installer v2 6/6] common: add checks for valid subnet mask and IPv4 address within subnet
2025-04-29 14:09 ` [pve-devel] [RFC PATCH pve-installer v2 6/6] common: add checks for valid subnet mask and IPv4 address within subnet Michael Köppl
@ 2025-05-06 9:21 ` Christoph Heiss
2025-05-09 8:51 ` Michael Köppl
0 siblings, 1 reply; 12+ messages in thread
From: Christoph Heiss @ 2025-05-06 9:21 UTC (permalink / raw)
To: Michael Köppl; +Cc: Proxmox VE development discussion
Apart from the one inline comment, looks good to me.
IMO it's not too restrictive, since IPv4 network/broadcast addresses
shouldn't really be used (and probably introduce subtle breakages)
outside of /31 prefixes anyway.
On Tue Apr 29, 2025 at 4:09 PM CEST, Michael Köppl wrote:
> Add checks for valid subnet mask (greater than /0 and at most /32 for
> IPv4). In addition, check if the address entered by the user is valid
> within the given subnet, i.e. not a network address or broadcast
> address. /31 is considered an exception in accordance with RFC3021 [0],
> considering any of the 2 available addresses to be valid host addresses.
>
> [0] https://datatracker.ietf.org/doc/html/rfc3021
>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
> [..]
> diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
> index 1fe6a74..fea98db 100644
> --- a/proxmox-installer-common/src/utils.rs
> +++ b/proxmox-installer-common/src/utils.rs
> [..]
> @@ -133,6 +137,43 @@ fn mask_limit(addr: &IpAddr) -> usize {
> if addr.is_ipv4() { 32 } else { 128 }
> }
>
> +fn check_mask_limit(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
> + return if mask > mask_limit(&addr) {
> + Err(CidrAddressParseError::InvalidMask(
> + "mask cannot be greater than 32".into(),
s/32/mask_limit(&addr)/g
> + ))
> + } else {
> + Ok(())
> + };
> +}
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [pve-devel] [RFC PATCH pve-installer v2 6/6] common: add checks for valid subnet mask and IPv4 address within subnet
2025-05-06 9:21 ` Christoph Heiss
@ 2025-05-09 8:51 ` Michael Köppl
0 siblings, 0 replies; 12+ messages in thread
From: Michael Köppl @ 2025-05-09 8:51 UTC (permalink / raw)
To: Christoph Heiss; +Cc: Proxmox VE development discussion
On 5/6/25 11:21, Christoph Heiss wrote:
> Apart from the one inline comment, looks good to me.
>
> IMO it's not too restrictive, since IPv4 network/broadcast addresses
> shouldn't really be used (and probably introduce subtle breakages)
> outside of /31 prefixes anyway.
>
> On Tue Apr 29, 2025 at 4:09 PM CEST, Michael Köppl wrote:
>> Add checks for valid subnet mask (greater than /0 and at most /32 for
>> IPv4). In addition, check if the address entered by the user is valid
>> within the given subnet, i.e. not a network address or broadcast
>> address. /31 is considered an exception in accordance with RFC3021 [0],
>> considering any of the 2 available addresses to be valid host addresses.
>>
>> [0] https://datatracker.ietf.org/doc/html/rfc3021
>>
>> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
>> ---
>> [..]
>> diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
>> index 1fe6a74..fea98db 100644
>> --- a/proxmox-installer-common/src/utils.rs
>> +++ b/proxmox-installer-common/src/utils.rs
>> [..]
>> @@ -133,6 +137,43 @@ fn mask_limit(addr: &IpAddr) -> usize {
>> if addr.is_ipv4() { 32 } else { 128 }
>> }
>>
>> +fn check_mask_limit(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
>> + return if mask > mask_limit(&addr) {
>> + Err(CidrAddressParseError::InvalidMask(
>> + "mask cannot be greater than 32".into(),
>
> s/32/mask_limit(&addr)/g
Will update this bit when I send a v3 including a better solution for
the maxroot check. Thanks!
>
>> + ))
>> + } else {
>> + Ok(())
>> + };
>> +}
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 12+ messages in thread