[pve-devel] [PATCH pve-installer v3 3/7] close #5887: add sanity check for LVM swapsize

[Michael Köppl]

On 7/7/25 14:07, Christoph Heiss wrote:
> On Thu Jun 26, 2025 at 5:11 PM CEST, Michael Köppl wrote:
> [..]
>> 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");
> 
> Very much a high-level nit; but: Could we maybe use the chance and unify
> the error messages between GUI & TUI here?
> 
> E.g. in the TUI its
> 
>   "ext4: Swap size x GiB cannot .."
> 
> vs. in the GUI:
> 
>   "Warning: swap size x GiB cannot .."
> 
> Especially since the above (including the 4Kn check) are not warnings
> but hard errors, i.e. not letting the user continue the installation.
> So stating it as a warning does not really make sense in the GUI.

Yes, absolutely. I'll update the error messages for v4. Makes sense to
do this now as it also makes for a more polished user experience. I
think in general there are many errors messages across the TUI and GUI
installers that could maybe be updated in the future or at least
unified. There are also cases where in one of the installers you get
details about an error and in the other you don't.

> 
>> diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
>> index af119e2..f29e1d4 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,15 @@ 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);
>> +            }
> 
> How about just
> 
>   check_swapsize(swapsize, hdsize)?
> 
> here?
> 
> (See also below w.r.t. anyhow)

Ack. Thanks for the hint

> 
>> +        }
>> +    }
>> +
>>      Ok(())
>>  }
> [..]
>> diff --git a/proxmox-installer-common/src/disk_checks.rs b/proxmox-installer-common/src/disk_checks.rs
>> index d535837..37a791f 100644
>> --- a/proxmox-installer-common/src/disk_checks.rs
>> +++ b/proxmox-installer-common/src/disk_checks.rs
> [..]
>> @@ -49,6 +49,36 @@ 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(())
>> +}
> 
> How about using an `anyhow::Result` here? Then it could just be a
> bail!("Swap size .."), or using ensure!()
> 
> As for proxmox-tui-installer; we already pull in the anyhow crate
> transitively there through proxmox-installer-common. Long-term we want
> to use it there too for new code / refactorings anyway, so that would be
> fine IMO.

Ack, makes sense to already use anyhow here then if it'll be used across
the entire installer repo in the long-term. Thanks, will update for v4.

> 
> [..]
>> 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}"))?;
> 
> With anyhow, this then also can use `.context(fstype.to_string())`
> instead.

Ack

> 
>> +
>>              Ok(BootdiskOptions {
>>                  disks: vec![disk],
>>                  fstype,
>