* [pve-devel] [PATCH docs/installer v4 0/9] add early disk and network sanity checks
@ 2025-07-11 16:27 Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH docs v4 1/1] installation: remove maxroot size requirement and mention default instead Michael Köppl
` (9 more replies)
0 siblings, 10 replies; 13+ messages in thread
From: Michael Köppl @ 2025-07-11 16:27 UTC (permalink / raw)
To: pve-devel
The goal of this series is to add additional sanity checks to the
auto-installer and the TUI and GUI installers. The following checks were
added:
* Btrfs / ZFS RAID: check if the required number of disks is available
* LVM: check if swapsize < hdsize
* Duplicate disks in answer file disk selection
* Networking: check if IPv4 address is valid within subnet (RFC)
Additionally, the documentation regarding maxroot is adapted to remove
the maximum maxroot size requirement since there was no check and adding
a sanity check would have prevented users from installing on setups with
very small disks even if the installation could have succeeded.
The disk-related checks aim to close [0], whereas the IPv4 check would
partially close [1].
[0] https://bugzilla.proxmox.com/show_bug.cgi?id=5887
[1] https://bugzilla.proxmox.com/show_bug.cgi?id=5757
Changes since v3:
- Rebased on master
- Moved unit tests for ZfsRaidLevel and BtrfsRaidLevel to
proxmox_installer_common::otions
- Rename check_disks() proxmox_installer_common to check_raid_disks_setup()
- Move dummy_disk() function for tests into Disk struct
- Add new patch that unifies error messages regarding disk and RAID
setup checks across TUI and GUI (mostly, see comment in 8/8)
- Use anyhow::Result to handle errors in LVM disk setups (swapsize, ...)
- Move the refactoring of CidrAddressParseError's Display implementation
to an earlier patch instead of rewriting later on
- Add FIXME to CidrAddressEditView's FormViewGetValue implementation
regarding the proper return type that should be used, needs a bigger
refactoring
Changes since v2:
- Rebased on master
- Removed maxroot check and instead adapted documentation
- Remove some changes that were the result of formatting otherwise
unchanged lines of code
- Added patch to improve error handling when checking CIDR address TUI
network dialog
- In installer 7/7, use the limit variable for the check_mask_limit
function instead of hardcoded 32
Changes since v1:
- Removed rustfmt patch
- Removed patch that changed check_zfs_raid_config (replaced with patch
below)
- Added patch that moves implementation of RAID setup checks to
implementation of ZfsRaidLevel and BtrfsRaidLevel
- Adapted tests for RAID setup validity
- Added configured values in GiB to error messages for invalid
swapsize and maxroot
- Changed maximum allowed swapsize to hdsize/8
- De-duplicated swapsize and maxroot check implementations in
autoinstaller utils and common's disk_checks
- Added patch that adds a check for duplicate disks in answer file disk
selection
- Refactored Display implementation of CidrAddressParseError
- Simplified Deserialize implementation of CidrAddress
- Explicitly allow /31 and /32 subnets in CIDR validity check
Thanks to @Christoph for reviewing the previous versions and his suggestions.
pve-docs:
Michael Köppl (1):
installation: remove maxroot size requirement and mention default
instead
pve-installation.adoc | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
pve-installer:
Michael Köppl (8):
auto: add early answer file sanity check for RAID configurations
move RAID setup checks to RAID level enum implementations
close #5887: add sanity check for LVM swapsize
auto: add check for duplicate disks in answer file
common: add more descriptive errors for invalid network configs
tui: change get_value return type for easier error handling
common: add checks for valid subnet mask and IPv4 address within
subnet
tui, gui: streamline error messages around disk and RAID checks
Proxmox/Install.pm | 24 ++-
proxinstall | 10 +-
proxmox-auto-install-assistant/src/main.rs | 3 +-
proxmox-auto-installer/src/utils.rs | 38 +++-
proxmox-auto-installer/tests/parse-answer.rs | 9 +-
.../parse_answer/ipv4_and_subnet_31.json | 19 ++
.../parse_answer/ipv4_and_subnet_31.toml | 18 ++
.../btrfs_raid_single_disk.json | 3 +
.../btrfs_raid_single_disk.toml | 15 ++
.../parse_answer_fail/duplicate_disk.json | 3 +
.../parse_answer_fail/duplicate_disk.toml | 15 ++
.../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 ++
.../lvm_swapsize_greater_than_hdsize.json | 3 +
.../lvm_swapsize_greater_than_hdsize.toml | 16 ++
.../zfs_raid_single_disk.json | 3 +
.../zfs_raid_single_disk.toml | 15 ++
proxmox-installer-common/src/disk_checks.rs | 168 +++---------------
proxmox-installer-common/src/options.rs | 147 +++++++++++++++
proxmox-installer-common/src/utils.rs | 85 +++++++--
proxmox-tui-installer/Cargo.toml | 1 +
proxmox-tui-installer/src/main.rs | 8 +-
proxmox-tui-installer/src/views/bootdisk.rs | 17 +-
proxmox-tui-installer/src/views/mod.rs | 9 +-
26 files changed, 488 insertions(+), 183 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/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/duplicate_disk.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.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
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
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
Summary over all repositories:
27 files changed, 491 insertions(+), 184 deletions(-)
--
Generated by git-murpp 0.8.0
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH docs v4 1/1] installation: remove maxroot size requirement and mention default instead
2025-07-11 16:27 [pve-devel] [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
@ 2025-07-11 16:27 ` Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 1/8] auto: add early answer file sanity check for RAID configurations Michael Köppl
` (8 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Michael Köppl @ 2025-07-11 16:27 UTC (permalink / raw)
To: pve-devel
The requirement of hdsize/4 was not checked anywhere and adding sanity
checks for maxroot<=hdsize/4 would stop users from installing PVE on
smaller disks (see [0]), whereas the installation actually tries its
best to successfully install even on disks below 12GB. So instead of
adding sanity checks, relax the documentation regarding this requirement
and instead refer to the defaults used during installation to provide
guidance for users.
[0] https://lore.proxmox.com/pve-devel/D9P1YXB42LGJ.ULII1HUIPAWQ@proxmox.com/
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
pve-installation.adoc | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/pve-installation.adoc b/pve-installation.adoc
index 223bb716..30ad6b81 100644
--- a/pve-installation.adoc
+++ b/pve-installation.adoc
@@ -281,7 +281,9 @@ NOTE: If set to `0`, no `swap` volume will be created.
`maxroot`::
Defines the maximum size of the `root` volume, which stores the operation
-system. The maximum limit of the `root` volume size is `hdsize/4`.
+system. With more than 48GiB storage available, the default is `hdsize/4`
+with a maximum of 96 GiB. Below 48GiB, the `root` volume size is at least
+`hdsize/2`.
`maxvz`::
--
2.47.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH installer v4 1/8] auto: add early answer file sanity check for RAID configurations
2025-07-11 16:27 [pve-devel] [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH docs v4 1/1] installation: remove maxroot size requirement and mention default instead Michael Köppl
@ 2025-07-11 16:27 ` Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 2/8] move RAID setup checks to RAID level enum implementations Michael Köppl
` (7 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Michael Köppl @ 2025-07-11 16:27 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 | 3 +-
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, 90 insertions(+), 3 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 320de2d..5d6c1d5 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -20,7 +20,7 @@ use proxmox_auto_installer::{
sysinfo::SysInfo,
utils::{
AutoInstSettings, FetchAnswerFrom, HttpOptions, default_partition_label,
- get_matched_udev_indexes, get_nic_list, get_single_udev_index,
+ 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,
},
@@ -877,6 +877,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)?;
Ok(answer)
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index e301c33..ad2db84 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -5,7 +5,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,
};
@@ -385,6 +386,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");
@@ -415,6 +430,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.47.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH installer v4 2/8] move RAID setup checks to RAID level enum implementations
2025-07-11 16:27 [pve-devel] [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH docs v4 1/1] installation: remove maxroot size requirement and mention default instead Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 1/8] auto: add early answer file sanity check for RAID configurations Michael Köppl
@ 2025-07-11 16:27 ` Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 3/8] close #5887: add sanity check for LVM swapsize Michael Köppl
` (6 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Michael Köppl @ 2025-07-11 16:27 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. Additionally, also move the unit
tests accordingly.
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
proxmox-installer-common/src/disk_checks.rs | 163 +-------------------
proxmox-installer-common/src/options.rs | 115 ++++++++++++++
proxmox-tui-installer/src/views/bootdisk.rs | 13 +-
3 files changed, 130 insertions(+), 161 deletions(-)
diff --git a/proxmox-installer-common/src/disk_checks.rs b/proxmox-installer-common/src/disk_checks.rs
index ecc43bd..62d5eab 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,108 +49,12 @@ 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 super::*;
- fn dummy_disk(index: usize) -> Disk {
- Disk {
- index: index.to_string(),
- path: format!("/dev/dummy{index}"),
- model: Some("Dummy disk".to_owned()),
- size: 1024. * 1024. * 1024. * 8.,
- block_size: Some(512),
- }
- }
-
fn dummy_disks(num: usize) -> Vec<Disk> {
- (0..num).map(dummy_disk).collect()
+ (0..num).map(Disk::dummy).collect()
}
#[test]
@@ -158,13 +62,13 @@ mod tests {
assert!(check_for_duplicate_disks(&dummy_disks(2)).is_ok());
assert_eq!(
check_for_duplicate_disks(&[
- dummy_disk(0),
- dummy_disk(1),
- dummy_disk(2),
- dummy_disk(2),
- dummy_disk(3),
+ Disk::dummy(0),
+ Disk::dummy(1),
+ Disk::dummy(2),
+ Disk::dummy(2),
+ Disk::dummy(3),
]),
- Err(&dummy_disk(2)),
+ Err(&Disk::dummy(2)),
);
}
@@ -189,55 +93,4 @@ mod tests {
assert!(check_disks_4kn_legacy_boot(BootType::Efi, &disks).is_ok());
}
}
-
- #[test]
- 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());
- }
-
- #[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());
- }
}
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 9271b8b..30418c2 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_raid_disks_setup(&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_raid_disks_setup(&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);
@@ -316,6 +375,19 @@ pub struct Disk {
pub block_size: Option<usize>,
}
+impl Disk {
+ #[cfg(test)]
+ pub fn dummy(index: usize) -> Disk {
+ Disk {
+ index: index.to_string(),
+ path: format!("/dev/dummy{index}"),
+ model: Some("Dummy disk".to_owned()),
+ size: 1024. * 1024. * 1024. * 8.,
+ block_size: Some(512),
+ }
+ }
+}
+
impl fmt::Display for Disk {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
// TODO: Format sizes properly with `proxmox-human-byte` once merged
@@ -542,6 +614,49 @@ mod tests {
use std::collections::BTreeMap;
use std::net::{IpAddr, Ipv4Addr};
+ fn dummy_disks(num: usize) -> Vec<Disk> {
+ (0..num).map(Disk::dummy).collect()
+ }
+
+ #[test]
+ fn btrfs_raid() {
+ let disks = dummy_disks(10);
+
+ let btrfs_raid_variants = [
+ BtrfsRaidLevel::Raid0,
+ BtrfsRaidLevel::Raid1,
+ BtrfsRaidLevel::Raid10,
+ ];
+
+ for v in btrfs_raid_variants {
+ assert!(v.check_raid_disks_setup(&[]).is_err());
+ assert!(v.check_raid_disks_setup(&disks[..v.get_min_disks() - 1]).is_err());
+ assert!(v.check_raid_disks_setup(&disks[..v.get_min_disks()]).is_ok());
+ assert!(v.check_raid_disks_setup(&disks).is_ok());
+ }
+ }
+
+ #[test]
+ fn zfs_raid() {
+ let disks = dummy_disks(10);
+
+ 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_raid_disks_setup(&[]).is_err());
+ assert!(v.check_raid_disks_setup(&disks[..v.get_min_disks() - 1]).is_err());
+ assert!(v.check_raid_disks_setup(&disks[..v.get_min_disks()]).is_ok());
+ assert!(v.check_raid_disks_setup(&disks).is_ok());
+ }
+ }
+
fn mock_setup_network() -> (SetupInfo, NetworkInfo) {
let mut interfaces = BTreeMap::new();
interfaces.insert(
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 313a3c9..6f3478f 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_raid_disks_setup(&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_raid_disks_setup(&disks)
+ .map_err(|err| format!("{fstype}: {err}"))?;
}
Ok(BootdiskOptions {
--
2.47.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH installer v4 3/8] close #5887: add sanity check for LVM swapsize
2025-07-11 16:27 [pve-devel] [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
` (2 preceding siblings ...)
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 2/8] move RAID setup checks to RAID level enum implementations Michael Köppl
@ 2025-07-11 16:27 ` Michael Köppl
2025-07-11 17:54 ` Thomas Lamprecht
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 4/8] auto: add check for duplicate disks in answer file Michael Köppl
` (5 subsequent siblings)
9 siblings, 1 reply; 13+ messages in thread
From: Michael Köppl @ 2025-07-11 16:27 UTC (permalink / raw)
To: pve-devel
Check that the configured swapsize is not greater than hdsize / 8 as
stated in the admin guide [0]. 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>
---
Proxmox/Install.pm | 8 +++++
proxinstall | 4 ++-
proxmox-auto-installer/src/utils.rs | 8 +++++
proxmox-auto-installer/tests/parse-answer.rs | 1 +
.../lvm_swapsize_greater_than_hdsize.json | 3 ++
.../lvm_swapsize_greater_than_hdsize.toml | 16 +++++++++
proxmox-installer-common/src/disk_checks.rs | 33 ++++++++++++++++++-
proxmox-tui-installer/Cargo.toml | 1 +
proxmox-tui-installer/src/views/bootdisk.rs | 10 +++++-
9 files changed, 81 insertions(+), 3 deletions(-)
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 95b7b30..f852147 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -599,6 +599,14 @@ 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;
+}
+
my sub chroot_chown {
my ($root, $path, %param) = @_;
diff --git a/proxinstall b/proxinstall
index 904668e..84f1a91 100755
--- a/proxinstall
+++ b/proxinstall
@@ -1488,7 +1488,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);
@@ -1607,9 +1607,11 @@ 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);
};
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 ad2db84..24ff4ea 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -12,6 +12,7 @@ use crate::{
};
use proxmox_installer_common::{
ROOT_PASSWORD_MIN_LENGTH,
+ disk_checks::check_swapsize,
options::{FsType, NetworkOptions, ZfsChecksumOption, ZfsCompressOption, email_validate},
setup::{
InstallBtrfsOption, InstallConfig, InstallFirstBootSetup, InstallRootPassword,
@@ -397,6 +398,13 @@ 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) {
+ check_swapsize(swapsize, hdsize)?;
+ }
+ }
+
Ok(())
}
diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index 92dba63..ca8c09f 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -145,6 +145,7 @@ mod tests {
btrfs_raid_single_disk,
fqdn_from_dhcp_no_default_domain,
fqdn_hostname_only,
+ 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_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 16b488e..b8b9c46 100644
--- a/proxmox-installer-common/src/disk_checks.rs
+++ b/proxmox-installer-common/src/disk_checks.rs
@@ -1,6 +1,8 @@
use std::collections::HashSet;
-use crate::options::Disk;
+use anyhow::ensure;
+
+use crate::options::{Disk, LvmBootdiskOptions};
use crate::setup::BootType;
/// Checks a list of disks for duplicate entries, using their index as key.
@@ -49,6 +51,35 @@ 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) -> anyhow::Result<()> {
+ let threshold = hdsize / 8.0;
+ ensure!(
+ swapsize <= threshold,
+ "Swap size {swapsize} GiB cannot be greater than {threshold} GiB (hard disk size / 8)"
+ );
+ Ok(())
+}
+
+/// Checks whether a user-supplied LVM setup is valid or not, such as the swapsize not
+/// exceeding a certain threshold.
+///
+/// # Arguments
+///
+/// * `bootdisk_opts` - The LVM options set by the user.
+pub fn check_lvm_bootdisk_opts(bootdisk_opts: &LvmBootdiskOptions) -> anyhow::Result<()> {
+ if let Some(swap_size) = bootdisk_opts.swap_size {
+ check_swapsize(swap_size, bootdisk_opts.total_size)?;
+ }
+
+ Ok(())
+}
+
#[cfg(test)]
mod tests {
use crate::options::{BtrfsRaidLevel, ZfsRaidLevel};
diff --git a/proxmox-tui-installer/Cargo.toml b/proxmox-tui-installer/Cargo.toml
index 139c85c..cc2baeb 100644
--- a/proxmox-tui-installer/Cargo.toml
+++ b/proxmox-tui-installer/Cargo.toml
@@ -9,6 +9,7 @@ homepage = "https://www.proxmox.com"
[dependencies]
proxmox-installer-common.workspace = true
+anyhow.workspace = true
serde_json.workspace = true
cursive = { version = "0.21", default-features = false, features = ["crossterm-backend"] }
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 6f3478f..9f6d235 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -3,6 +3,8 @@ use std::{
sync::{Arc, Mutex},
};
+use anyhow::Context;
+
use cursive::{
Cursive, Vec2, View,
view::{Nameable, Resizable, ViewWrapper},
@@ -17,7 +19,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 +265,10 @@ impl AdvancedBootdiskOptionsView {
.get_values()
.ok_or("Failed to retrieve advanced bootdisk options")?;
+ check_lvm_bootdisk_opts(&advanced)
+ .context(fstype.to_string())
+ .map_err(|err| format!("{:#}", err))?;
+
Ok(BootdiskOptions {
disks: vec![disk],
fstype,
--
2.47.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH installer v4 4/8] auto: add check for duplicate disks in answer file
2025-07-11 16:27 [pve-devel] [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
` (3 preceding siblings ...)
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 3/8] close #5887: add sanity check for LVM swapsize Michael Köppl
@ 2025-07-11 16:27 ` Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 5/8] common: add more descriptive errors for invalid network configs Michael Köppl
` (4 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Michael Köppl @ 2025-07-11 16:27 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 24ff4ea..62646f2 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -1,7 +1,10 @@
use anyhow::{Context, Result, bail};
use glob::Pattern;
use log::info;
-use std::{collections::BTreeMap, process::Command};
+use std::{
+ collections::{BTreeMap, HashSet},
+ process::Command,
+};
use crate::{
answer::{
@@ -397,6 +400,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 ca8c09f..8b7eac4 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_swapsize_greater_than_hdsize,
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.47.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH installer v4 5/8] common: add more descriptive errors for invalid network configs
2025-07-11 16:27 [pve-devel] [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
` (4 preceding siblings ...)
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 4/8] auto: add check for duplicate disks in answer file Michael Köppl
@ 2025-07-11 16:27 ` Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 6/8] tui: change get_value return type for easier error handling Michael Köppl
` (3 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Michael Köppl @ 2025-07-11 16:27 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
proxmox-installer-common/src/utils.rs | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
index 8adcec0..1f225c8 100644
--- a/proxmox-installer-common/src/utils.rs
+++ b/proxmox-installer-common/src/utils.rs
@@ -18,6 +18,22 @@ pub enum CidrAddressParseError {
InvalidMask(Option<ParseIntError>),
}
+impl fmt::Display for CidrAddressParseError {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "Invalid CIDR: ")?;
+
+ match self {
+ CidrAddressParseError::NoDelimiter => {
+ write!(f, "no delimiter for separating address and mask was found")
+ }
+ CidrAddressParseError::InvalidAddr(err) => write!(f, "{err}"),
+ CidrAddressParseError::InvalidMask(err) => {
+ write!(f, "{:?}", err)
+ }
+ }
+ }
+}
+
/// An IP address (IPv4 or IPv6), including network mask.
///
/// See the [`IpAddr`] type for more information how IP addresses are handled.
@@ -109,8 +125,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.47.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH installer v4 6/8] tui: change get_value return type for easier error handling
2025-07-11 16:27 [pve-devel] [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
` (5 preceding siblings ...)
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 5/8] common: add more descriptive errors for invalid network configs Michael Köppl
@ 2025-07-11 16:27 ` Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 7/8] common: add checks for valid subnet mask and IPv4 address within subnet Michael Köppl
` (2 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Michael Köppl @ 2025-07-11 16:27 UTC (permalink / raw)
To: pve-devel
Adapt the return type of CidrAddressEditView's get_value implementation
for the FormViewGetValue trait to handle errors in case of invalid CIDR
similarly to other (parsing) errors done in the TUIs network dialog.
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
proxmox-tui-installer/src/main.rs | 8 +++++---
proxmox-tui-installer/src/views/mod.rs | 9 +++++----
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 57a334f..15ee5d3 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -20,9 +20,8 @@ use proxmox_installer_common::{
ROOT_PASSWORD_MIN_LENGTH,
options::{BootdiskOptions, NetworkOptions, TimezoneOptions, email_validate},
setup::{LocaleInfo, ProxmoxProduct, RuntimeInfo, SetupInfo, installer_setup},
- utils::Fqdn,
+ utils::{CidrAddress, Fqdn},
};
-
mod setup;
mod system;
@@ -536,7 +535,10 @@ fn network_dialog(siv: &mut Cursive) -> InstallerView {
let address = view
.get_value::<CidrAddressEditView, _>(2)
- .ok_or("failed to retrieve host address")?;
+ .ok_or("failed to retrieve host address".to_string())
+ .and_then(|(ip_addr, mask)| {
+ CidrAddress::new(ip_addr, mask).map_err(|err| err.to_string())
+ })?;
let gateway = view
.get_value::<EditView, _>(3)
diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs
index 4364489..537e3ed 100644
--- a/proxmox-tui-installer/src/views/mod.rs
+++ b/proxmox-tui-installer/src/views/mod.rs
@@ -387,8 +387,9 @@ where
}
}
-impl FormViewGetValue<CidrAddress> for CidrAddressEditView {
- fn get_value(&self) -> Option<CidrAddress> {
+impl FormViewGetValue<(IpAddr, usize)> for CidrAddressEditView {
+ // FIXME: return CidrAddress (again) with proper error handling through Result
+ fn get_value(&self) -> Option<(IpAddr, usize)> {
self.get_values()
}
}
@@ -569,7 +570,7 @@ impl CidrAddressEditView {
.fixed_width(4)
}
- fn get_values(&self) -> Option<CidrAddress> {
+ fn get_values(&self) -> Option<(IpAddr, usize)> {
let addr = self
.view
.get_child(0)?
@@ -587,7 +588,7 @@ impl CidrAddressEditView {
.get_content()
.ok()?;
- CidrAddress::new(addr, mask).ok()
+ Some((addr, mask))
}
}
--
2.47.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH installer v4 7/8] common: add checks for valid subnet mask and IPv4 address within subnet
2025-07-11 16:27 [pve-devel] [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
` (6 preceding siblings ...)
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 6/8] tui: change get_value return type for easier error handling Michael Köppl
@ 2025-07-11 16:27 ` Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 8/8] tui, gui: streamline error messages around disk and RAID checks Michael Köppl
2025-07-15 9:50 ` [pve-devel] superseded: [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
9 siblings, 0 replies; 13+ messages in thread
From: Michael Köppl @ 2025-07-11 16:27 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>
---
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 | 72 ++++++++++++++-----
8 files changed, 138 insertions(+), 16 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 8b7eac4..88f30aa 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_swapsize_greater_than_hdsize,
no_fqdn_from_dhcp,
no_root_password_set,
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 1f225c8..929cc6b 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,7 +15,11 @@ 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 {
@@ -27,9 +31,9 @@ impl fmt::Display for CidrAddressParseError {
write!(f, "no delimiter for separating address and mask was found")
}
CidrAddressParseError::InvalidAddr(err) => write!(f, "{err}"),
- CidrAddressParseError::InvalidMask(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"),
}
}
}
@@ -63,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.
@@ -103,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 })
}
}
@@ -135,6 +137,44 @@ fn mask_limit(addr: &IpAddr) -> usize {
if addr.is_ipv4() { 32 } else { 128 }
}
+fn check_mask_limit(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
+ let limit = mask_limit(&addr);
+ return if mask > limit {
+ Err(CidrAddressParseError::InvalidMask(
+ format!("mask cannot be greater than {limit}").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.47.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH installer v4 8/8] tui, gui: streamline error messages around disk and RAID checks
2025-07-11 16:27 [pve-devel] [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
` (7 preceding siblings ...)
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 7/8] common: add checks for valid subnet mask and IPv4 address within subnet Michael Köppl
@ 2025-07-11 16:27 ` Michael Köppl
2025-07-15 9:50 ` [pve-devel] superseded: [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
9 siblings, 0 replies; 13+ messages in thread
From: Michael Köppl @ 2025-07-11 16:27 UTC (permalink / raw)
To: pve-devel
Adapt error messages around these checks to follow a similar structure
as in the TUI (<filesystem>: <message>) instead of displaying them as
warnings, since they actually stop users from continuing anyway.
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
This is mostly to have both the TUI and GUI display actual errors with
context instead of warnings since users cannot continue because it was a
bit irritating to have the TUI display an error with context and the GUI
just saying "Warning". Additionally, the error messages now have a
similar format in both installers. The TUI uses ZFS (RAID0), BTRFS
(RAID0), etc., whereas the GUI uses zfs (RAID0), etc. I decided not to
change that for now since it would be a bit out of scope for this
series, I think. Unifying any informational or error messages
throughout the installers in general would make sense for a future
series, though.
Proxmox/Install.pm | 16 ++++++++--------
proxinstall | 6 +++---
proxmox-auto-installer/src/utils.rs | 4 ++--
.../btrfs_raid_single_disk.json | 2 +-
.../parse_answer_fail/duplicate_disk.json | 2 +-
.../parse_answer_fail/zfs_raid_single_disk.json | 2 +-
6 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index f852147..c6dd8c4 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -287,7 +287,7 @@ sub get_zfs_raid_setup {
my $devlist = &$get_raid_devlist();
my $diskcount = scalar(@$devlist);
- die "$filesys needs at least one device\n" if $diskcount < 1;
+ die "$filesys: need at least one device\n" if $diskcount < 1;
my $cmd = '';
if ($filesys eq 'zfs (RAID0)') {
@@ -296,7 +296,7 @@ sub get_zfs_raid_setup {
$cmd .= " @$hd[1]";
}
} elsif ($filesys eq 'zfs (RAID1)') {
- die "zfs (RAID1) needs at least 2 devices\n" if $diskcount < 2;
+ die "$filesys: need at least 2 devices\n" if $diskcount < 2;
$cmd .= ' mirror ';
my $hd = @$devlist[0];
my $expected_size = @$hd[2]; # all disks need approximately same size
@@ -306,8 +306,8 @@ sub get_zfs_raid_setup {
$cmd .= " @$hd[1]";
}
} elsif ($filesys eq 'zfs (RAID10)') {
- die "zfs (RAID10) needs at least 4 devices\n" if $diskcount < 4;
- die "zfs (RAID10) needs an even number of devices\n" if $diskcount & 1;
+ die "$filesys: need at least 4 devices\n" if $diskcount < 4;
+ die "$filesys: need an even number of devices\n" if $diskcount & 1;
for (my $i = 0; $i < $diskcount; $i += 2) {
my $hd1 = @$devlist[$i];
@@ -321,7 +321,7 @@ sub get_zfs_raid_setup {
} elsif ($filesys =~ m/^zfs \(RAIDZ-([123])\)$/) {
my $level = $1;
my $mindisks = 2 + $level;
- die "zfs (RAIDZ-$level) needs at least $mindisks devices\n"
+ die "zfs (RAIDZ-$level): need at least $mindisks devices\n"
if scalar(@$devlist) < $mindisks;
my $hd = @$devlist[0];
my $expected_size = @$hd[2]; # all disks need approximately same size
@@ -355,7 +355,7 @@ sub get_btrfs_raid_setup {
my $devlist = &$get_raid_devlist();
my $diskcount = scalar(@$devlist);
- die "$filesys needs at least one device\n" if $diskcount < 1;
+ die "$filesys: need at least one device\n" if $diskcount < 1;
foreach my $hd (@$devlist) {
legacy_bios_4k_check(@$hd[4]);
@@ -369,10 +369,10 @@ sub get_btrfs_raid_setup {
if ($filesys eq 'btrfs (RAID0)') {
$mode = 'raid0';
} elsif ($filesys eq 'btrfs (RAID1)') {
- die "btrfs (RAID1) needs at least 2 devices\n" if $diskcount < 2;
+ die "$filesys: need at least 2 devices\n" if $diskcount < 2;
$mode = 'raid1';
} elsif ($filesys eq 'btrfs (RAID10)') {
- die "btrfs (RAID10) needs at least 4 devices\n" if $diskcount < 4;
+ die "$filesys: need at least 4 devices\n" if $diskcount < 4;
$mode = 'raid10';
} else {
die "unknown btrfs mode '$filesys'\n";
diff --git a/proxinstall b/proxinstall
index 84f1a91..5e7eb1b 100755
--- a/proxinstall
+++ b/proxinstall
@@ -1593,7 +1593,7 @@ sub create_hdsel_view {
apply_raid_disk_selection();
my ($devlist) = eval { Proxmox::Install::get_zfs_raid_setup() };
if (my $err = $@) {
- Proxmox::UI::message("Warning: $err\nPlease fix ZFS setup first.");
+ Proxmox::UI::message("$err");
return;
}
$target_hds = [map { $_->[1] } @$devlist];
@@ -1601,7 +1601,7 @@ sub create_hdsel_view {
apply_raid_disk_selection();
my ($devlist) = eval { Proxmox::Install::get_btrfs_raid_setup() };
if (my $err = $@) {
- Proxmox::UI::message("Warning: $err\nPlease fix BTRFS setup first.");
+ Proxmox::UI::message("$err");
return;
}
$target_hds = [map { $_->[1] } @$devlist];
@@ -1614,7 +1614,7 @@ sub create_hdsel_view {
Proxmox::Install::swapsize_check($hdsize);
};
if (my $err = $@) {
- Proxmox::UI::message("Warning: $err\n");
+ Proxmox::UI::message("$filesys: $err");
return;
}
$target_hds = [$target_hd];
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index 62646f2..7d42f2c 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -395,7 +395,7 @@ pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
let min_disks = answer.disks.fs_type.get_min_disks();
if selection.len() < min_disks {
bail!(
- "{} requires at least {} disks",
+ "{}: need at least {} disks",
answer.disks.fs_type,
min_disks
);
@@ -404,7 +404,7 @@ pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
let mut disk_set = HashSet::new();
for disk in selection {
if !disk_set.insert(disk) {
- bail!("List of disks contains duplicate disk {disk}");
+ bail!("List of disks contains duplicate device {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
index 37f35fe..957cb49 100644
--- 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
@@ -1,3 +1,3 @@
{
- "error": "BTRFS (RAID1) requires at least 2 disks"
+ "error": "BTRFS (RAID1): need at least 2 disks"
}
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
index d01fabe..93f6b6c 100644
--- a/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json
@@ -1,3 +1,3 @@
{
- "error": "List of disks contains duplicate disk sda"
+ "error": "List of disks contains duplicate device 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
index 9a8cc90..41c99d8 100644
--- 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
@@ -1,3 +1,3 @@
{
- "error": "ZFS (RAID10) requires at least 4 disks"
+ "error": "ZFS (RAID10): need at least 4 disks"
}
--
2.47.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH installer v4 3/8] close #5887: add sanity check for LVM swapsize
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 3/8] close #5887: add sanity check for LVM swapsize Michael Köppl
@ 2025-07-11 17:54 ` Thomas Lamprecht
2025-07-15 9:43 ` Michael Köppl
0 siblings, 1 reply; 13+ messages in thread
From: Thomas Lamprecht @ 2025-07-11 17:54 UTC (permalink / raw)
To: Proxmox VE development discussion, Michael Köppl
Am 11.07.25 um 18:27 schrieb Michael Köppl:
> Check that the configured swapsize is not greater than hdsize / 8 as
> stated in the admin guide [0]. 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>
> ---
> Proxmox/Install.pm | 8 +++++
> proxinstall | 4 ++-
> proxmox-auto-installer/src/utils.rs | 8 +++++
> proxmox-auto-installer/tests/parse-answer.rs | 1 +
> .../lvm_swapsize_greater_than_hdsize.json | 3 ++
> .../lvm_swapsize_greater_than_hdsize.toml | 16 +++++++++
> proxmox-installer-common/src/disk_checks.rs | 33 ++++++++++++++++++-
> proxmox-tui-installer/Cargo.toml | 1 +
> proxmox-tui-installer/src/views/bootdisk.rs | 10 +++++-
> 9 files changed, 81 insertions(+), 3 deletions(-)
> 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
>
this one doesn't apply, seems something broken or amiss related to
proxmox-installer-common/src/disk_checks.rs, please re-check this.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH installer v4 3/8] close #5887: add sanity check for LVM swapsize
2025-07-11 17:54 ` Thomas Lamprecht
@ 2025-07-15 9:43 ` Michael Köppl
0 siblings, 0 replies; 13+ messages in thread
From: Michael Köppl @ 2025-07-15 9:43 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion
On 7/11/25 19:54, Thomas Lamprecht wrote:
> Am 11.07.25 um 18:27 schrieb Michael Köppl:
>> Check that the configured swapsize is not greater than hdsize / 8 as
>> stated in the admin guide [0]. 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>
>> ---
>> Proxmox/Install.pm | 8 +++++
>> proxinstall | 4 ++-
>> proxmox-auto-installer/src/utils.rs | 8 +++++
>> proxmox-auto-installer/tests/parse-answer.rs | 1 +
>> .../lvm_swapsize_greater_than_hdsize.json | 3 ++
>> .../lvm_swapsize_greater_than_hdsize.toml | 16 +++++++++
>> proxmox-installer-common/src/disk_checks.rs | 33 ++++++++++++++++++-
>> proxmox-tui-installer/Cargo.toml | 1 +
>> proxmox-tui-installer/src/views/bootdisk.rs | 10 +++++-
>> 9 files changed, 81 insertions(+), 3 deletions(-)
>> 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
>>
>
> this one doesn't apply, seems something broken or amiss related to
> proxmox-installer-common/src/disk_checks.rs, please re-check this.
Took some time to figure out why this didn't apply, only to find out
that I made a mistake during preparation of the series. Sorry about
that. Sent an updated v5 [0]. Double-checked this time that all patches
apply!
[0]
https://lore.proxmox.com/pve-devel/20250715094021.154914-1-m.koeppl@proxmox.com
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] superseded: [PATCH docs/installer v4 0/9] add early disk and network sanity checks
2025-07-11 16:27 [pve-devel] [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
` (8 preceding siblings ...)
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 8/8] tui, gui: streamline error messages around disk and RAID checks Michael Köppl
@ 2025-07-15 9:50 ` Michael Köppl
9 siblings, 0 replies; 13+ messages in thread
From: Michael Köppl @ 2025-07-15 9:50 UTC (permalink / raw)
To: pve-devel
Superseded-by:
https://lore.proxmox.com/pve-devel/20250715094021.154914-1-m.koeppl@proxmox.com
On 7/11/25 18:27, Michael Köppl wrote:
> The goal of this series is to add additional sanity checks to the
> auto-installer and the TUI and GUI installers. The following checks were
> added:
> * Btrfs / ZFS RAID: check if the required number of disks is available
> * LVM: check if swapsize < hdsize
> * Duplicate disks in answer file disk selection
> * Networking: check if IPv4 address is valid within subnet (RFC)
>
> Additionally, the documentation regarding maxroot is adapted to remove
> the maximum maxroot size requirement since there was no check and adding
> a sanity check would have prevented users from installing on setups with
> very small disks even if the installation could have succeeded.
>
> The disk-related checks aim to close [0], whereas the IPv4 check would
> partially close [1].
>
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=5887
> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=5757
>
> Changes since v3:
> - Rebased on master
> - Moved unit tests for ZfsRaidLevel and BtrfsRaidLevel to
> proxmox_installer_common::otions
> - Rename check_disks() proxmox_installer_common to check_raid_disks_setup()
> - Move dummy_disk() function for tests into Disk struct
> - Add new patch that unifies error messages regarding disk and RAID
> setup checks across TUI and GUI (mostly, see comment in 8/8)
> - Use anyhow::Result to handle errors in LVM disk setups (swapsize, ...)
> - Move the refactoring of CidrAddressParseError's Display implementation
> to an earlier patch instead of rewriting later on
> - Add FIXME to CidrAddressEditView's FormViewGetValue implementation
> regarding the proper return type that should be used, needs a bigger
> refactoring
>
> Changes since v2:
> - Rebased on master
> - Removed maxroot check and instead adapted documentation
> - Remove some changes that were the result of formatting otherwise
> unchanged lines of code
> - Added patch to improve error handling when checking CIDR address TUI
> network dialog
> - In installer 7/7, use the limit variable for the check_mask_limit
> function instead of hardcoded 32
>
> Changes since v1:
> - Removed rustfmt patch
> - Removed patch that changed check_zfs_raid_config (replaced with patch
> below)
> - Added patch that moves implementation of RAID setup checks to
> implementation of ZfsRaidLevel and BtrfsRaidLevel
> - Adapted tests for RAID setup validity
> - Added configured values in GiB to error messages for invalid
> swapsize and maxroot
> - Changed maximum allowed swapsize to hdsize/8
> - De-duplicated swapsize and maxroot check implementations in
> autoinstaller utils and common's disk_checks
> - Added patch that adds a check for duplicate disks in answer file disk
> selection
> - Refactored Display implementation of CidrAddressParseError
> - Simplified Deserialize implementation of CidrAddress
> - Explicitly allow /31 and /32 subnets in CIDR validity check
>
> Thanks to @Christoph for reviewing the previous versions and his suggestions.
>
> pve-docs:
>
> Michael Köppl (1):
> installation: remove maxroot size requirement and mention default
> instead
>
> pve-installation.adoc | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>
> pve-installer:
>
> Michael Köppl (8):
> auto: add early answer file sanity check for RAID configurations
> move RAID setup checks to RAID level enum implementations
> close #5887: add sanity check for LVM swapsize
> auto: add check for duplicate disks in answer file
> common: add more descriptive errors for invalid network configs
> tui: change get_value return type for easier error handling
> common: add checks for valid subnet mask and IPv4 address within
> subnet
> tui, gui: streamline error messages around disk and RAID checks
>
> Proxmox/Install.pm | 24 ++-
> proxinstall | 10 +-
> proxmox-auto-install-assistant/src/main.rs | 3 +-
> proxmox-auto-installer/src/utils.rs | 38 +++-
> proxmox-auto-installer/tests/parse-answer.rs | 9 +-
> .../parse_answer/ipv4_and_subnet_31.json | 19 ++
> .../parse_answer/ipv4_and_subnet_31.toml | 18 ++
> .../btrfs_raid_single_disk.json | 3 +
> .../btrfs_raid_single_disk.toml | 15 ++
> .../parse_answer_fail/duplicate_disk.json | 3 +
> .../parse_answer_fail/duplicate_disk.toml | 15 ++
> .../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 ++
> .../lvm_swapsize_greater_than_hdsize.json | 3 +
> .../lvm_swapsize_greater_than_hdsize.toml | 16 ++
> .../zfs_raid_single_disk.json | 3 +
> .../zfs_raid_single_disk.toml | 15 ++
> proxmox-installer-common/src/disk_checks.rs | 168 +++---------------
> proxmox-installer-common/src/options.rs | 147 +++++++++++++++
> proxmox-installer-common/src/utils.rs | 85 +++++++--
> proxmox-tui-installer/Cargo.toml | 1 +
> proxmox-tui-installer/src/main.rs | 8 +-
> proxmox-tui-installer/src/views/bootdisk.rs | 17 +-
> proxmox-tui-installer/src/views/mod.rs | 9 +-
> 26 files changed, 488 insertions(+), 183 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/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/duplicate_disk.json
> create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.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
> 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
> 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
>
>
> Summary over all repositories:
> 27 files changed, 491 insertions(+), 184 deletions(-)
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-07-15 9:49 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-07-11 16:27 [pve-devel] [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH docs v4 1/1] installation: remove maxroot size requirement and mention default instead Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 1/8] auto: add early answer file sanity check for RAID configurations Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 2/8] move RAID setup checks to RAID level enum implementations Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 3/8] close #5887: add sanity check for LVM swapsize Michael Köppl
2025-07-11 17:54 ` Thomas Lamprecht
2025-07-15 9:43 ` Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 4/8] auto: add check for duplicate disks in answer file Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 5/8] common: add more descriptive errors for invalid network configs Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 6/8] tui: change get_value return type for easier error handling Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 7/8] common: add checks for valid subnet mask and IPv4 address within subnet Michael Köppl
2025-07-11 16:27 ` [pve-devel] [PATCH installer v4 8/8] tui, gui: streamline error messages around disk and RAID checks Michael Köppl
2025-07-15 9:50 ` [pve-devel] superseded: [PATCH docs/installer v4 0/9] add early disk and network sanity checks Michael Köppl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox