change default root partition size

kevin-ferrier commented 4 months ago

This fix issue: #2277

PR Description:

Increase the default root partition size following these rules: 10% of the total disk size, from 20GiB up to 50GiB maximum

Tests and Checks

[x] I have tested the code!

svartkanin commented 4 months ago

This only changes the example code not the actual core logic.

kevin-ferrier commented 4 months ago

This only changes the example code not the actual core logic.

My bad it was late, I updated the PR, it should be better.

svartkanin commented 4 months ago

    # root partition size processing
    root_partition_size_gib = 20
    if total_size_unit == disk.Unit.GiB:
        if 51 > total_size_value // 10 > 19:
            root_partition_size_gib = total_size_value // 10
    elif total_size_unit == disk.Unit.TiB:
        # maximum part size
        root_partition_size_gib = 50

This code is quite confusing and incomplete. It only handles the single partitioning layout suggestion not the multi disk layout suggestion. Checking for the unit of the returned total size to determine an actual partition size is not going to work well and will end up in unpredictable behavior.

From this code what I understand you're trying to

Set the root size to the total size of the disk if the total size is less than 51GB? So where does the home partition go?
If we're dealing with a large partition (implied by the fact that the disk reports the size in TiB) then it's 50GiB

I would recommend to normalize the received size first

total_device_size = device.device_info.total_size.convert(disk.Unit.B)

and then use that as the value to be interpreted for deciding how to partition

kevin-ferrier commented 4 months ago

  # root partition size processing
  root_partition_size_gib = 20
  if total_size_unit == disk.Unit.GiB:
      if 51 > total_size_value // 10 > 19:
          root_partition_size_gib = total_size_value // 10
  elif total_size_unit == disk.Unit.TiB:
      # maximum part size
      root_partition_size_gib = 50
This code is quite confusing and incomplete. It only handles the single partitioning layout suggestion not the multi disk layout suggestion. Checking for the unit of the returned total size to determine an actual partition size is not going to work well and will end up in unpredictable behavior.

From this code what I understand you're trying to

Set the root size to the total size of the disk if the total size is less than 51GB? So where does the home partition go?

If we're dealing with a large partition (implied by the fact that the disk reports the size in TiB) then it's 50GiB

I would recommend to normalize the received size first
total_device_size = device.device_info.total_size.convert(disk.Unit.B)
and then use that as the value to be interpreted for deciding how to partition

You're right it was confusing, it's much better with the convert function first, I updated the code.

For the multidisk behaviour, I think it's not required because the root will take the entire disk if I understood correctly.

For the singledisk, the idea was to put a minimum of 20GiB up to 50GiB depending on the disk size, from what I read on the issue discussion.

Also I was waiting for the workflow to generate the ISO but it seems broken at the moment so I didn't tested it yet but I'll do it in local.

Tell me what you think!

Torxed commented 4 months ago

Also I was waiting for the workflow to generate the ISO but it seems broken at the moment so I didn't tested it yet but I'll do it in local.

The build is simply blocked from automatically running on first-time contributors, but I've pushed it through so it's building right now :)

svartkanin commented 4 months ago

The example code still contains the first version and should be updated as well.

I think for multi disk support we're also using 20GB https://github.com/archlinux/archinstall/blob/master/archinstall%2Flib%2Finteractions%2Fdisk_conf.py#L359

Also, if the total size is 200 < x < 500 then you're assigning the entire total size?

kevin-ferrier commented 4 months ago

Also I was waiting for the workflow to generate the ISO but it seems broken at the moment so I didn't tested it yet but I'll do it in local.

The build is simply blocked from automatically running on first-time contributors, but I've pushed it through so it's building right now :)

Oh makes sense!

I tested yesterday with a locally generated ISO in VMs:

suggested disk layout ext4 one disk: 50gb disk gives me root part of 20gb
suggested disk layout ext4 one disk: 300gb disk gives me root part of 30gb
suggested disk layout ext4 one disk: 700gb disk gives me root part of 50gb
One full install of a disk of 300gb, install was working as expected

I'll test more with the generated ISO to make sure there isn't issue between local/deployed.

kevin-ferrier commented 4 months ago

The example code still contains the first version and should be updated as well.

I think for multi disk support we're also using 20GB https://github.com/archlinux/archinstall/blob/master/archinstall%2Flib%2Finteractions%2Fdisk_conf.py#L359

Also, if the total size is 200 < x < 500 then you're assigning the entire total size?

I did a revert for the example code, it should be good now.

If the size is between 200GB and 500 I am assigning 10% of this size for the root partition, I thought reading the discussions on the issue that it would be a good start, what do you think?

As for the multi disk support I missed it indeed! I'll need to read a bit more about it because I'm not super familiar with btrfs and subvolumes and then I'll suggest something.

I saw there are these 2 links in the comments in the code, I'll start there: https://www.reddit.com/r/btrfs/comments/m287gp/partition_strategy_for_two_physical_disks/ https://www.reddit.com/r/btrfs/comments/9us4hr/what_is_your_btrfs_partitionsubvolumes_scheme/

kevin-ferrier commented 3 months ago

So I looked a bit into it, from what I understood from the multidisk layout:

we are looking for a disk >=40GiB for /home and extend the partition to the entire disk
we are looking for the smallest disk >=20GiB for /boot + root partition, and put a partition of 20GiB for root

What I would suggest to improve a bit the situation regarding the size issue:

keep the same behaviour for /home
look for the smallest disk >=20GiB for root partition, and extend the size to the maximum available with a boundry of 50GiB.

I also looked into the subvolumes with BTRFS in multidisks layout (which doesn't seem to be handled here if I am not mistaken), and would propose something like this (taken from reddit submission):

select which disks we want to use
look for the smallest disk and put /boot on it
use all of the remaining space from every selected disk to create a volume "tank"
put the subvolumes in the tank: @ -> /, @home -> /home, @var-log -> /var/log like suggested on reddit (I like the idea)

I would also suggest to handle the part with subvolumes and multidisks on another PR though because I'm not yet familiar with the codebase and would require a bit more work. What do you think?

Torxed commented 3 months ago

I agree, and the root size has been discussed as a wanted change before.

And I agree that splitting up the changes regarding multidisk btrfs into a separate PR is ideal :)

svartkanin commented 3 months ago

That sounds good for now

kevin-ferrier commented 3 months ago

I actually made a mistake reading the code, sorry about that: the hardcoded 20GiB is only used to identify the disk for the /root part. The size is actually going to be the entire disk minus the /boot partition.

Just to make sure: do we want to change this behaviour in multidisk layout? That means we would have a /root going from the entire suggested disk to 20~50GiB maximum and leave extra space open on the disk.

If we want to keep using the entire disk for root in multidisk layout: I revert the last commit.

Here are the tests I did:

Done	Reason	Disk1	Disk2	Disk3	Config	Result
[x]	lower boundry	100GiB			btrfs, no subvolumes, separate /home	/boot 1GiB, /root 20GiB, /home 79GiB
[x]	middle	350GiB			btrfs, no subvolumes, seperate /home	/boot 1GiB, /root 35GiB, /home 314GiB
[x]	upper boundry	550GiB			btrfs, no subvolumes, separate /home	/boot 1GiB, /root 50GiB /home 499GiB
[x]	ensure subvolumes are still OK on single disk	100GiB			btrfs, subvolumes	/boot 1GiB + btrfs rest of the disk with @volumes
[x]	multi disk lower boundry	100GiB	100GiB		btrfs, no subvolumes	sda: /boot 1GiB + /root 20GiB sdb: /home entire disk
[x]	multi disk check disk selection with different sizes	60	80		btrfs, no subvolumes	sda(60GiB): /boot 1GiB + /root 20GiB, sdb(80GiB): /home entire disk
disk partition are as expected but can't boot	multi disk check disk selection with 3 disks	15	60	45	btrfs, no subvolumes, UEFI bios	sda(15GiB): empty sdb(60GiB): /home full disk sdc(45GiB): /boot 1GiB + /root 20GiB

Only one issue with the multi disk layout where the root isn't on the first disk, I guess it's an issue with the VM config? Not sure about that.

kevin-ferrier commented 3 months ago

Workflow isn't working on mkarchiso: grub-mkstandalone: /usr/lib/libdevmapper.so.1.02: versionDM_1_02_197' not found (required by grub-mkstandalone)` I saw an issue open 3 days ago (https://github.com/archlinux/archinstall/issues/2443) and a new release of archiso was released 3 days ago (https://github.com/archlinux/archiso/tags). Probably related?

vojkovic commented 3 months ago

Workflow isn't working on mkarchiso: grub-mkstandalone: /usr/lib/libdevmapper.so.1.02: versionDM_1_02_197' not found (required by grub-mkstandalone)` I saw an issue open 3 days ago (#2443) and a new release of archiso was released 3 days ago (https://github.com/archlinux/archiso/tags). Probably related?

Just to provide more information for the issue, it's been a problem for about 2 weeks. I have a similar workflow that I use myself and I noticed it was also failing here with the same error message, hence I opened an issue.

kevin-ferrier commented 2 months ago

I cleaned the commit history to remove the mess, removed the multi disk suggestion part and tested again with single layout (100GB / 250GB / 600GB) disks: suggestions are correct (20GB / 25GB / 50GB) and the OS is working after reboot.

Tell me if anything else is required!

svartkanin commented 2 months ago

That looks good now, thanks for this change it has been anticipated!

palontologist commented 1 month ago

Hiii Kevin I saw you mentioned on reboot I have actually been having this problem for a while now and resulted to deleting multifiles without success of update how do I implement this solution. Is it only with a new archinstall or I can do it manually.

kevin-ferrier commented 1 month ago

Hi, I am not sure about your question.

If you are asking if you can update an already installed archlinux with archinstall to do the root partition size update then the answer is no. It's only for new install. To update yours you would need to look for "resize linux root partition" and backup your data before doing so in case something goes wrong.

If you are asking why you don't see the root partition update by using archinstall, I think it's because this merge isn't yet bundled into a release. You can either wait for that or update the file yourself before installing arch with archinstall. The change is on the tab "File changed" of this PR.

Kevin

archlinux / archinstall

change default root partition size #2415

PR Description:

Tests and Checks