Re: [pve-devel] [PATCH pve-installer v2 3/6] close #5887: add sanity check for LVM swapsize and maxroot

["Christoph Heiss" <c.heiss@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