archinstall icon indicating copy to clipboard operation
archinstall copied to clipboard

LVM support

Open svartkanin opened this issue 2 years ago • 6 comments

LVM support

This PR intoduces LVM support. When choosing a default partitioning layout, the LVM option becomes available and a user can pick a default LVM layout.

The default layout was taken from https://wiki.archlinux.org/title/Dm-crypt/Encrypting_an_entire_system#Overview

Related Issues

This should fix the open issues

  • https://github.com/archlinux/archinstall/issues/2296
  • https://github.com/archlinux/archinstall/issues/584

New LVM menu

image

Encryption

When the default LVM layout has been chosen, the encryption menu will provide 2 new encryption options

  • Lvm on Luks
  • Luks on LVM

It is expected that the user knows the difference

Future

  • Rework entire menu for partitioning/LVM as it's very distributed and messy
  • Support encryption for LVMs across partitions (limited possibilities)
  • Provide option to manually setup LVM
  • Handle pre-mounted LVM volumes for the Pre-mount menu option

Testing

[x] Lvm with systemd [x] Lvm with grub [x] LvmOnLuks with systemd [x] LvmOnLuks with grub [x] LuksOnLvm with systemd [x] LuksOnLvm with grub

General changes

DiskHandler

  • _perform_formatting has been split up into multiple smaller functions
    • format_encrypted which will now format a a encrypted device
    • encrypt which will encrypt a given device
  • _validate_partitions as well as _format_partitions has been moved into FilesystemHandler as it makes more sense to keep the orchestration for filesystem operations encapsulated inside there and use the DiskHandler for the actual operations

svartkanin avatar Sep 24 '23 09:09 svartkanin

In menu - Add default LVM layout option (@Torxed any suggestions here?)

Sadly I don't have much feedback on it. I'd say whatever the wiki recommends is the way to go here.

I've only ever used LVM with Ubuntu back in.. 2012 last time. That's the main reason my LVM work got along so slowly so I appreciate you taking up the challenge ^^

  • Currently the LVM only shows up when a partitioning option is selected, is that good?

I think that makes sense.

  • Should LVM be supported on pre-mounted (might be difficult not to mess up)

Yes, that would be ideal :D

Torxed avatar Sep 24 '23 14:09 Torxed

Some changes to the device model etc might need a merge before I merge more PR's as future merges might make merge conflicts horrible if I continue hehe.

Torxed avatar Nov 20 '23 11:11 Torxed

Yep I've noticed :)

svartkanin avatar Nov 20 '23 20:11 svartkanin

@Torxed @codefiles this is finally at a good stage and can now be tested and reviewed

svartkanin avatar Jan 27 '24 08:01 svartkanin

Could you put the style/formatting changes and other changes unrelated to LVM in separate commits if you do not intend to submit them as separate pull requests? Either would make reviewing easier.

codefiles avatar Jan 28 '24 14:01 codefiles

Putting them in different commits may be tricky, but I'll describe them in more detail in the description

svartkanin avatar Jan 28 '24 19:01 svartkanin

I object to this being merged without a proper review.

codefiles avatar Mar 07 '24 12:03 codefiles

That's okay, I'd say lets get all other open PRs in for now, then do a safe release with those and then we can review and merge this so it sits in master gor a bit

svartkanin avatar Mar 08 '24 00:03 svartkanin

Yes, please. That would be most ideal.

codefiles avatar Mar 08 '24 01:03 codefiles

I have resolved all conflicts (I hope without any mistakes) but I didn't get to testing things fully again which is still open after the master merge.

@Torxed can we release a new version with all the new merged PRs so that's out and then I'll run tests on this one. If @codefiles would like to have a go at it as well that'd be great but to avoid continous conflict resolving I'd be keen to merge this to master after the new version is out.

svartkanin avatar Mar 13 '24 11:03 svartkanin

Please don't proceed with a merge without my input. Just glancing at this I have spotted a mistake that was made when merging in master. If I were to look more I am sure I would find more. Please review your PR for these mistakes and fix them before requesting a merge.

As I have mentioned before, you have style and formatting changes in this pull request. They have nothing to do with the intent of the PR. The more changes you have in a PR the harder it is to keep up-to-date with master. It also means a larger diff and then more that has to be combed through when reviewing. Just something to keep in mind for the future.

codefiles avatar Mar 13 '24 13:03 codefiles

Fair point, I'll go over all the changes and yeah something went haywire with the formatting there somehow I'll address that as well

svartkanin avatar Mar 13 '24 21:03 svartkanin

I updated the PR with with the merge fixes and also run through some installations with and without LVM and things seem to work

svartkanin avatar Mar 16 '24 11:03 svartkanin

@Torxed could we push out a new release so this could go ahead with merging?

svartkanin avatar Apr 07 '24 04:04 svartkanin

@Torxed could we push out a new release so this could go ahead with merging?

Fixing this weekend!

Torxed avatar Apr 11 '24 05:04 Torxed

As the new release is out I'll merge this in to avoid conflicts and to allow for more testing

svartkanin avatar Apr 15 '24 08:04 svartkanin

I was hoping to review this before it was merged. Oh well.

codefiles avatar Apr 15 '24 12:04 codefiles

I do agree that this PR isn't the way changes should be done but rather broken apart in smaller pieces. Given the larger latencies between merges the alternative is to create a dozen smaller hierarchical PRs that keep lingering around. I do hope that this is one of a kind and moving forward it'll be easier to shorten PR cycle times and thereby keep the PRs to a reasonable size.

@codefiles I wanted to merge this to avoid further conflicts and potential rebase challenges. I'll happily consider any review still taking place and address all comments in follow up PRs as necessary

svartkanin avatar Apr 15 '24 12:04 svartkanin