-
Notifications
You must be signed in to change notification settings - Fork 352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for /home reuse to automatic partitioning #5814
base: master
Are you sure you want to change the base?
Conversation
Hello @rvykydal! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-10-04 08:35:44 UTC |
ef788a0
to
ca47dad
Compare
ca47dad
to
2a671cb
Compare
Hello @vojtechtrefny, could you check if the solution makes sense? |
d1e6b66
to
7367c72
Compare
7367c72
to
20ed5df
Compare
20ed5df
to
af8c029
Compare
af8c029
to
fab73b1
Compare
fab73b1
to
913ca57
Compare
913ca57
to
56b53e6
Compare
|
51b6946
to
ffa8298
Compare
ffa8298
to
0634fd6
Compare
Fixing tests. |
0634fd6
to
413995e
Compare
/kickstart-test --testtype smoke |
/kickstart-test --testtype storage |
@vojtechtrefny I've added one commit reusing the mount options of reused /home: home reuse: reuse mount options of reused mountpoins |
@jkonecny12 @M4rtinK could you please do a review? (Example of the API used in the frontend: https://github.com/rvykydal/anaconda-webui/blob/e4a42dc79d7c4589d5b3898280f3cd535332efdd/src/apis/storage_partitioning.js#L102) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not able to find anything problematic.
raise StorageError(_("Multiple boot loader partitions required: %s"), bootloader_parts) | ||
if not bootloader_parts: | ||
log.debug("No bootloader partition required") | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return values are not used anywhere, you can just return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me. However, for the amount of added new methods to process reused etc. it seems that 1 adjusted test case and 1 new test case is a bit low.
As you can see here: https://app.codecov.io/gh/rhinstaller/anaconda/pull/5814 the automatic_partitioning.py
is -24% coverage.
We use 'destroy' only for plain partition mountpoints as we can't destroy lv or btrfs subvolume - it would not free space for the new one (and its vg or btrfs volume). For lvs and btrfs subvols we reuse 'reformat' from mount point assignment (real erase would be perhaps better, even for mount point assignment as recreating the device complicates the logic)
But handle efi vs biosboot etc automagically.
413995e
to
c3e9eff
Compare
On Jirka's request I am going to add the tests for the new code, which will probably result in making some of the functions static and better factored. |
Thanks a lot and sorry for complicating this... |
This is a draft, POC of an approach trying to follow the analysis by @poncovka and @vojtechtrefny which is linked in the https://issues.redhat.com/browse/INSTALLER-4020
It is extending autopartitioning (partitioning
AUTOMATIC
) with some mount point assignment (partitioningMANUAL
) mechanisms.Tested (supposed) to be working on all partitioning schemes (plain, btrfs, lvm, thinlv), reusing unencrypted autopart.
The idea is to just provide flexible enough mechanism (extending the automatic partitioning request structure), most of the logic, checks, assumptions about the OSs / partitions found should happen in the UI / client providing the feature. (Maybe adding some helper API later)
The backend logic assumes unique existing mount points (eg /home) for the requests (reuse/erase/remove). We could extend it with the device specification if/when we support multiple existing mount points and their selection in the UI (passing the device to the backend instead of mountpoint) but I am not sure we even want to support this case.
The kickstart API doesn't have to be public/published, now it is there just as ground for the discussion, tests, PartitioningRequest structure update and work on webui UI support.
Pykickstart support draft PR: pykickstart/pykickstart#499
Draft of kickstart tests: rhinstaller/kickstart-tests#1278
Webui draft PR: rhinstaller/anaconda-webui#424
TODO:
/home
is missing compression opts). I am afraid we don't have this data - but it must be the same for reusing /home via mount point assignment - need to check.