LVM support
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
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-mountmenu 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_formattinghas been split up into multiple smaller functions-
format_encryptedwhich will now format a a encrypted device -
encryptwhich will encrypt a given device
-
-
_validate_partitionsas well as_format_partitionshas been moved intoFilesystemHandleras it makes more sense to keep the orchestration for filesystem operations encapsulated inside there and use theDiskHandlerfor the actual operations
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
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.
Yep I've noticed :)
@Torxed @codefiles this is finally at a good stage and can now be tested and reviewed
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.
Putting them in different commits may be tricky, but I'll describe them in more detail in the description
I object to this being merged without a proper review.
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
Yes, please. That would be most ideal.
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.
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.
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
I updated the PR with with the merge fixes and also run through some installations with and without LVM and things seem to work
@Torxed could we push out a new release so this could go ahead with merging?
@Torxed could we push out a new release so this could go ahead with merging?
Fixing this weekend!
As the new release is out I'll merge this in to avoid conflicts and to allow for more testing
I was hoping to review this before it was merged. Oh well.
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