Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for /home reuse to automatic partitioning #5814

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add support for /home reuse to automatic partitioning #5814

wants to merge 6 commits into from
+421 −106

Conversation

rvykydal
Copy link
Contributor

@rvykydal rvykydal commented Aug 9, 2024 •
edited
Loading

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 (partitioning MANUAL) 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:

  • we should handle bootloader partitions automagically - detect required partitions and check there is unambigous (efi or biosboot...) partition to delete (or preserve)
  • check consistency of partitioning scheme
  • reuse fstab mount options (/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.
  • update unit tests if needed, fix the tests
  • ? check dual boot with Windows (currently only tested with multiple Fedora OSs)

@pep8speaks
Copy link

pep8speaks commented Aug 9, 2024 •
edited
Loading

Hello @rvykydal! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 118:100: E501 line too long (100 > 99 characters)
Line 362:100: E501 line too long (103 > 99 characters)

Line 80:100: E501 line too long (100 > 99 characters)

Line 102:37: E127 continuation line over-indented for visual indent

Comment last updated at 2024-10-04 08:35:44 UTC

@github-actions github-actions bot added the f41 label Aug 9, 2024
@rvykydal rvykydal mentioned this pull request Aug 9, 2024
Closed
@rvykydal
Copy link
Contributor Author

rvykydal commented Aug 26, 2024 •
edited
Loading

Hello @vojtechtrefny, could you check if the solution makes sense?

@rvykydal rvykydal mentioned this pull request Aug 30, 2024
Open
11 tasks
@rvykydal rvykydal mentioned this pull request Sep 3, 2024
Draft
@rvykydal rvykydal changed the title [WIP] Add support for /home reuse to automatic partitioning Add support for /home reuse to automatic partitioning Sep 3, 2024
@rvykydal rvykydal marked this pull request as ready for review September 3, 2024 09:08
@rvykydal
Copy link
Contributor Author

rvykydal commented Sep 19, 2024 •
edited
Loading

  • Renamed "erase" to "reformat"
  • Also updated unit tests.

@rvykydal rvykydal marked this pull request as draft September 23, 2024 13:09
@rvykydal
Copy link
Contributor Author

rvykydal commented Sep 23, 2024 •
edited
Loading

Fixing tests.
The one failing test is caused by the make _get_windows_data more robust commit which is not needed (issiu fixed in blivet) and will be removed.

@rvykydal
Copy link
Contributor Author

/kickstart-test --testtype smoke

@rvykydal rvykydal marked this pull request as ready for review September 23, 2024 16:18
@rvykydal
Copy link
Contributor Author

/kickstart-test --testtype storage

@KKoukiou KKoukiou added f42 Fedora 42 and removed f41 labels Sep 24, 2024
@rvykydal
Copy link
Contributor Author

rvykydal commented Oct 1, 2024

@vojtechtrefny I've added one commit reusing the mount options of reused /home: home reuse: reuse mount options of reused mountpoins

@rvykydal
Copy link
Contributor Author

rvykydal commented Oct 1, 2024

@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)

KKoukiou
KKoukiou approved these changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@KKoukiou KKoukiou left a 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.

pyanaconda/modules/storage/partitioning/automatic/automatic_partitioning.py
raise StorageError(_("Multiple boot loader partitions required: %s"), bootloader_parts)
if not bootloader_parts:
log.debug("No bootloader partition required")
return False
Copy link
Contributor

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.

jkonecny12
jkonecny12 requested changes Oct 3, 2024
View reviewed changes
Copy link
Member

@jkonecny12 jkonecny12 left a 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.

@rvykydal
home reuse: add support for /home reuse to automatic partitioning
4f12dfd 
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)
@rvykydal
home reuse: update existing OSs when applying partitioning
096c447
@rvykydal
home reuse: remove bootloader partitions implicitly
313a1bb
@rvykydal
home reuse: require removing of bootloader partition explicitly
0aa7b53 
But handle efi vs biosboot etc automagically.
@rvykydal
home reuse: check autopartitioning scheme against reused mountpoints
025b9bc
@rvykydal
home reuse: reuse mount options of reused mountpoins
c3e9eff
@rvykydal
Copy link
Contributor Author

rvykydal commented Oct 4, 2024

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.

@jkonecny12
Copy link
Member

Thanks a lot and sorry for complicating this...

@rvykydal
Copy link
Contributor Author

rvykydal commented Oct 7, 2024 •
edited
Loading

Thanks a lot and sorry for complicating this...

Thank you. My bad, the tests should have been already there in the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkonecny12 jkonecny12 jkonecny12 requested changes

@KKoukiou KKoukiou KKoukiou approved these changes

@M4rtinK M4rtinK Awaiting requested review from M4rtinK

@vojtechtrefny vojtechtrefny Awaiting requested review from vojtechtrefny

Assignees
No one assigned
Labels
f42 Fedora 42
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rvykydal @pep8speaks @jkonecny12 @vojtechtrefny @jstodola @KKoukiou