From: "Christoph Heiss" <c.heiss@proxmox.com>
To: "Michael Köppl" <m.koeppl@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH pve-installer v2 3/6] close #5887: add sanity check for LVM swapsize and maxroot
Date: Tue, 06 May 2025 13:48:52 +0200 [thread overview]
Message-ID: <D9P1YXB42LGJ.ULII1HUIPAWQ@proxmox.com> (raw)
In-Reply-To: <20250429140940.161711-4-m.koeppl@proxmox.com>
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
next prev parent reply other threads:[~2025-05-06 11:48 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` [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 [this message]
2025-05-09 11:07 ` Michael Köppl
2025-05-27 16:19 ` Michael Köppl
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 ` [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
2025-05-06 9:21 ` Christoph Heiss
2025-05-09 8:51 ` Michael Köppl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D9P1YXB42LGJ.ULII1HUIPAWQ@proxmox.com \
--to=c.heiss@proxmox.com \
--cc=m.koeppl@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal