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

[Michael Köppl]

On 4/28/25 14:00, Christoph Heiss wrote:
> On Tue Apr 22, 2025 at 6:27 PM CEST, Michael Köppl wrote:
>> Check that the configured swapsize is not greater than the total size
>> of the disk and that maxroot is at most hdsize / 4. Define the
>> behavior for the auto-installer as well as the TUI and GUI installers.
>>
>> Signed-off-by: Michael Köppl <m.koeppl at proxmox.com>
>> ---
>> The changes implemented in this patch regarding swap size only avoid
>> rather obvious misconfigurations. However, users could still configure a
>> 32GB swap volume on a 32GB hard disk and the installer would fail later
>> on. Users could benefit another check that ensures the swap volume size
>> does not go beyond a certain threshold. One option could be to set the
>> limit similar to the 8GB maximum when the swap volume size is calculated
>> from the size of the memory. OTOH, this might also be
>> considered too restrictive. Would love some input on this.
> 
> I'd limit the swapsize to hdsize/8, as we state in the admin guide [0].
> Although I'm not sure if we actually check this somewhere currently.

As far as I can tell, the hdsize/8 limit in the admin guide at the 
moment only refers to the value being calculated from the memory size. I 
added a check to limit the maximum swap size entered by users to 
hdsize/8 for the v2.

> 
> [0] https://pve.proxmox.com/pve-docs/pve-admin-guide.html#advanced_lvm_options
> 
>> [..]
>> diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
>> index d6bc6e3..85a1f52 100644
>> --- a/proxmox-auto-installer/src/utils.rs
>> +++ b/proxmox-auto-installer/src/utils.rs
>> @@ -396,6 +396,19 @@ pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
>>               );
>>           }
>>       }
>> +
>> +    if let answer::FsOptions::LVM(lvm) = &answer.disks.fs_options {
>> +        if lvm.swapsize > lvm.hdsize {
>> +            bail!("LVM swapsize cannot be greater than hdsize");
>> +        }
>> +
>> +        if let Some((maxroot, hdsize)) = lvm.maxroot.zip(lvm.hdsize) {
>> +            if maxroot > hdsize / 4.0 {
>> +                bail!("LVM maxroot cannot be greater than hdsize / 4");
>> +            }
>> +        }
>> +    }
> 
> If feasible, could these checks be de-duplicated with the ones below?
> Since they do check the exact same things.

I added 2 check functions (swap and maxroot) that are used by both the 
autoinstaller utils and in the installer-common disk_checks for the v2. 
Thanks for the suggestion!