Add support for /home reuse to automatic partitioning #5814

rvykydal · 2024-08-09T13:20:44Z

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
? support dual boot with Windows (currently only tested with multiple Fedora OSs)

pep8speaks · 2024-08-09T13:20:52Z

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

In the file pyanaconda/modules/storage/partitioning/automatic/automatic_partitioning.py:

Line 112:100: E501 line too long (100 > 99 characters)
Line 352:100: E501 line too long (103 > 99 characters)

In the file pyanaconda/modules/storage/partitioning/manual/manual_partitioning.py:

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

In the file pyanaconda/modules/storage/partitioning/manual/utils.py:

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

Comment last updated at 2024-09-19 14:47:12 UTC

rvykydal · 2024-08-26T08:31:35Z

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

jkonecny12 · 2024-09-03T15:03:18Z

A few questions.

How API / kickstart will work on multiboot systems?
I know that we don't want to support that now but it doesn't mean someone will not us it this way.

Is the erase naming correct?
I guess it is doing reformat most of the time and I'm not 100% sure that erase is understandable name. The only format which is not reformat is BTRFS and I'm not sure we want to complicate things because of that.

How will this feature work in plain partitioning?
I mean how are we going to reuse the place when it could be split all over the place.

What will happen if user of existing BTRFS will use autopart for simple partitioning?
I see potential issue of mixing types of partitioning, not sure how problematic this could be.

Do we even want to have pykickstart API? I know that pykickstart is usually taken as the most powerful installation type but I see this more like mainly useful for UI and dangerous for Kickstart use feature type.

rvykydal · 2024-09-03T15:03:30Z

I have a feeling that we should do a brainstorming to discuss proposed API and kickstart change. I'm not saying this is wrong idea only that it would be great to have multiple eyes to think about things like future extensions etc...

I agree. As I said, we don't need to expose kickstart API at this point, just extend the structure as UI entry point, but it would be nice to have a way to test (and maybe use) the feature via kickstart.

rvykydal · 2024-09-04T06:53:55Z

The commit storage: make _get_windows_data more robust is needed to fix webui tests using the feature. I made it a separate PR: #5868

rvykydal · 2024-09-04T07:21:18Z

A few questions.

Thank you for great questions.

How API / kickstart will work on multiboot systems? I know that we don't want to support that now but it doesn't mean someone will not us it this way.

Currently there is an assumption in the PR that there is unique mountpoint found among existing installations (on selected disks). It is the business of the UI to ensure it is the case before requesting home reuse. If the backend finds non-unique mountpoints (or in some cases no mountpoint - in some cases this maybe does not have to be fatal, like if requesting removing /boot?) it raises exception which is handled in the UI by using another method (or perhaps modifying storage in cockpit storage and trying again?).

If we want to support multiple mountpoints (eg /homes) existing, we could perhaps (just a quick idea) extend the data with explicit setting of the device (UI would need to provide that, maybe with help of some functions of the backend), like /home:DEVICE_ID (or some suitable separator if : is not good). The backend would not look up the unique device, but use the provided one.

Is the erase naming correct? I guess it is doing reformat most of the time and I'm not 100% sure that erase is understandable name. The only format which is not reformat is BTRFS and I'm not sure we want to complicate things because of that.

Good point I think we cau use reformat as in mount-point-assignment.

How will this feature work in plain partitioning? I mean how are we going to reuse the place when it could be split all over the place.

I am not sure I understand the question.
In plain we can remove the also the / (see the kstest example from the kstest PR) so the space would be reused when creating the root partiotion with autopart again?

What will happen if user of existing BTRFS will use autopart for simple partitioning? I see potential issue of mixing types of partitioning, not sure how problematic this could be.

Yes, good question, it is the second item in the TODO list above.

Do we even want to have pykickstart API? I know that pykickstart is usually taken as the most powerful installation type but I see this more like mainly useful for UI and dangerous for Kickstart use feature type.

I agree with the concern, and as I said in the fist comment we don't:

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.

It is just nice for testing (I am using it heavily), discussion of the use cases (plain vs btrfs partitioning) etc.
We will be covered by webui tests (WIP), but maybe not for other than btrfs partitioning schemes.
Definitely we can postpone its definition / publication for later. We just should not close the door for possible future exetensions, which the suggested extending of the PartitioningRequest in the PR hopefully does not (considering the possible enhancements of mount point specifications, like MOUNTPOINT:DEVICE_ID or similar.

vojtechtrefny · 2024-09-04T10:48:03Z

Currently there is an assumption in the PR that there is unique mountpoint found among existing installations (on selected disks). It is the business of the UI to ensure it is the case before requesting home reuse. If the backend finds non-unique mountpoints (or in some cases no mountpoint - in some cases this maybe does not have to be fatal, like if requesting removing /boot?) it raises exception which is handled in the UI by using another method (or perhaps modifying storage in cockpit storage and trying again?).

If we want to support multiple mountpoints (eg /homes) existing, we could perhaps (just a quick idea) extend the data with explicit setting of the device (UI would need to provide that, maybe with help of some functions of the backend), like /home:DEVICE_ID (or some suitable separator if : is not good). The backend would not look up the unique device, but use the provided one.

I think it's better to stick to the original proposal and not support setups with multiple /homes. At least for now, if someone asks for that, we might reconsider in the future, but I think with disk selection we should be able to cover most multi-distro setups even with this.

Is the erase naming correct? I guess it is doing reformat most of the time and I'm not 100% sure that erase is understandable name. The only format which is not reformat is BTRFS and I'm not sure we want to complicate things because of that.

Good point I think we cau use reformat as in mount-point-assignment.

We could even just use reuse for / as well and assume that "everyone knows" that reusing / means reformatting it. But that might be too dangerous.

vojtechtrefny · 2024-09-04T10:50:30Z

pyanaconda/modules/storage/partitioning/automatic/automatic_partitioning.py

+ if len(devices) > 1:
+ raise StorageError(_("Multiple devices found for bootloader partition '{}': {}")
+ .format(part_type,
+ ", ".join([device.name for device in devices])))


So does this fail with two disks both with a BIOS boot partition? Because that's not a problem (it's not necessary, but also doesn't break anything).

Good point, I'll think about looseing the requirement.

vojtechtrefny

Looks good to me in general. This is surprisingly simple change, I like that :-)

jkonecny12 · 2024-09-05T07:13:52Z

Good point about testing. If the KS support helps us with testing I guess it is worth the effort.

I would also like to know what @jstodola thinks about this. How problematic would be to support this on RHEL, if we want to support it there.

jkonecny12 · 2024-09-05T07:25:37Z

About future possible support for multiple installed systems. First, I totally agree to go with only single installed system.
The question is if now or in future we should specify devices instead of mount points... but I understand that having

autopart --reuse=/home

is much more helpful than

autopart --reuse=/dev/sda1

On the other hand having something like

autopart --use-free-space --remove=/dev/sda1

might be highly valuable future feature.

Maybe I'm getting too wild here. But from the above example it doesn't seem possible to support mounts or devs. And having mount:dev solution seems contra productive to me because dev is the only required in that case.

jstodola · 2024-09-05T15:52:57Z

I would also like to know what @jstodola thinks about this. How problematic would be to support this on RHEL, if we want to support it there.

We can consider it if there are customer requests for this feature, otherwise I vote for not supporting it on RHEL.

As for the kickstart support - I would prefer not having it to avoid additional maintenance. Or, if it is really so valuable for testing, the new options could be marked as experimental, unsupported and not recommended for general use.

rvykydal · 2024-09-06T07:03:23Z

About future possible support for multiple installed systems. First, I totally agree to go with only single installed system. The question is if now or in future we should specify devices instead of mount points... but I understand that having

autopart --reuse=/home

is much more helpful than

autopart --reuse=/dev/sda1

On the other hand having something like

autopart --use-free-space --remove=/dev/sda1

might be highly valuable future feature.

What is this supposed to do?

I don't think we (backend/API) should not make the decision about which mps/devices to reuse / remove and reformat. It should be requested explicitly by client/user. Otherwise I'd be afraid of getting to the point where we won't be able to support too generic API. Explicit API makes less commitments. Although it may be less comfortable for the user, I think main use case for /home reuse is UI (where also ambiguities can be solved and result passed to the backend).

Maybe I'm getting too wild here. But from the above example it doesn't seem possible to support mounts or devs. And having mount:dev solution seems contra productive to me because dev is the only required in that case.

It think is not, I think we don't want to support removing arbitrary devices in scope of this feature.
Having mount:dev shows user knows what he does (which he should in the case when he specifies the device) and the backend can check for the user that the device really is on the mounpoint.
I don't think we want to add support for removing arbitrary devices (at least in scope of this feature).

rvykydal · 2024-09-06T07:05:36Z

I would also like to know what @jstodola thinks about this. How problematic would be to support this on RHEL, if we want to support it there.

We can consider it if there are customer requests for this feature, otherwise I vote for not supporting it on RHEL.

As for the kickstart support - I would prefer not having it to avoid additional maintenance. Or, if it is really so valuable for testing, the new options could be marked as experimental, unsupported and not recommended for general use.

I think for delivering the feature we can do just with the request data structure extension without any kickstart support / API.

jkonecny12 · 2024-09-06T07:19:24Z

What is this supposed to do?

It should allow users to use autopart in a way that only metioned partitions are remove and everything else stays the intact. However, thinking about it second time ignoredisk is able to provide you the same.

jkonecny12 · 2024-09-06T07:22:11Z

Another question, do we need --remove and --erase? Couldn't this be handled other ways before the autopart? I mean have devicetree which will get these actions and then start autopart on top of that? In that case only --reuse is really requirement so that autopart knows that it should be remounted.

rvykydal · 2024-09-06T09:58:58Z

Another question, do we need --remove and --erase? Couldn't this be handled other ways before the autopart? I mean have devicetree which will get these actions and then start autopart on top of that? In that case only --reuse is really requirement so that autopart knows that it should be remounted.

Hm, I'd need more concrete suggestion, I might be missing something but what you suggest seems to me like doing partitioning ("devicetree will get these actions" ^^^) before partitioning (autopart actions) to me. Both stages (clear partitions and schedule partitions) are done in scope of a partitioning application.
I'd maybe understand suggesting creating a new type of partitioning (sharing code with AUTOPART and MANUAL) but I am not sure there is enough reasons for having it as a new type. To me it seems we are kind of extending AUTOPART. It may be more clear when comparing plain scheme, where it is possible to remove '/' and do new autopart (creating device for '/') with btrfs, our main target for now (or lvm) where it is not possible to remove '/' and do autopart, you'd need to remove the whole partition backing the '/' subvolume (with all the other subvolumes). See for example https://github.com/rvykydal/anaconda/blob/5e950907eaf5e33c20cccaa8776aebf325de8e81/pyanaconda/modules/storage/partitioning/automatic/automatic_partitioning.py#L210.

jkonecny12 · 2024-09-09T15:03:14Z

I guess you are pointing me that sharing two partitioning types is not possible. In that case I understand. I somehow hoped that we would be able to share devicetree between two types but I see the issues there. We don't want to go that way.

rvykydal · 2024-09-18T13:07:19Z

Removed reading data from kickstart.
Added checking of automatic partitioning scheme

File "/usr/lib/python3.13/site-packages/dasbus/server/handler.py", line 265, in _handle_call return handler(*parameters, **additional_args) File "/usr/lib64/python3.13/site-packages/pyanaconda/modules/storage/devicetree/viewer_interface.py", line 194, in GetExistingSystems return OSData.to_structure_list(self.implementation.get_existing_systems()) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^ File "/usr/lib64/python3.13/site-packages/pyanaconda/modules/storage/devicetree/viewer.py", line 493, in get_existing_systems windows_data = self._get_windows_data() File "/usr/lib64/python3.13/site-packages/pyanaconda/modules/storage/devicetree/viewer.py", line 530, in _get_windows_data if str(device.part_type_uuid) == EFI_PARTITION_TYPE: ^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.13/site-packages/blivet/threads.py", line 49, in run_with_lock return m(*args, **kwargs) File "/usr/lib/python3.13/site-packages/blivet/devices/partition.py", line 380, in part_type_uuid if hasattr(parted.Partition, "type_uuid") and self.parted_partition.type_uuid: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AttributeError: 'NoneType' object has no attribute 'type_uuid'

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.

rvykydal · 2024-09-19T14:47:43Z

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

rvykydal temporarily deployed to gh-cockpituous August 9, 2024 13:20 — with GitHub Actions Inactive

github-actions bot added the f41 label Aug 9, 2024

rvykydal mentioned this pull request Aug 9, 2024

[WIP] Add support for /home reuse to automatic partitioning #5799

Closed

rvykydal force-pushed the home-reuse-poc branch from ef788a0 to ca47dad Compare August 22, 2024 13:51

rvykydal temporarily deployed to gh-cockpituous August 22, 2024 13:51 — with GitHub Actions Inactive

rvykydal force-pushed the home-reuse-poc branch from ca47dad to 2a671cb Compare August 22, 2024 13:54

rvykydal had a problem deploying to gh-cockpituous August 22, 2024 13:54 — with GitHub Actions Failure

rvykydal requested a review from vojtechtrefny August 26, 2024 08:29

rvykydal temporarily deployed to gh-cockpituous August 29, 2024 16:27 — with GitHub Actions Inactive

rvykydal force-pushed the home-reuse-poc branch from d1e6b66 to 7367c72 Compare August 29, 2024 16:36

rvykydal temporarily deployed to gh-cockpituous August 29, 2024 16:36 — with GitHub Actions Inactive

rvykydal force-pushed the home-reuse-poc branch from 7367c72 to 20ed5df Compare August 30, 2024 10:06

rvykydal temporarily deployed to gh-cockpituous August 30, 2024 10:06 — with GitHub Actions Inactive

rvykydal force-pushed the home-reuse-poc branch from 20ed5df to af8c029 Compare August 30, 2024 12:41

rvykydal temporarily deployed to gh-cockpituous August 30, 2024 12:41 — with GitHub Actions Inactive

rvykydal force-pushed the home-reuse-poc branch from af8c029 to fab73b1 Compare August 30, 2024 12:42

rvykydal had a problem deploying to gh-cockpituous August 30, 2024 12:42 — with GitHub Actions Failure

rvykydal mentioned this pull request Aug 30, 2024

[WIP] Home reuse rhinstaller/anaconda-webui#424

Draft

10 tasks

rvykydal requested review from M4rtinK, jkonecny12 and KKoukiou September 2, 2024 14:10

rvykydal force-pushed the home-reuse-poc branch from fab73b1 to 913ca57 Compare September 3, 2024 07:07

rvykydal temporarily deployed to gh-cockpituous September 3, 2024 07:07 — with GitHub Actions Inactive

rvykydal mentioned this pull request Sep 3, 2024

[WIP] add support for home reuse to autopart pykickstart/pykickstart#499

Draft

rvykydal force-pushed the home-reuse-poc branch from 913ca57 to 56b53e6 Compare September 3, 2024 07:25

rvykydal temporarily deployed to gh-cockpituous September 3, 2024 07:25 — with GitHub Actions Inactive

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 marked this pull request as ready for review September 3, 2024 09:08

vojtechtrefny reviewed Sep 4, 2024

View reviewed changes

rvykydal force-pushed the home-reuse-poc branch from 5e95090 to 06b5bb9 Compare September 18, 2024 13:03

rvykydal temporarily deployed to gh-cockpituous September 18, 2024 13:04 — with GitHub Actions Inactive

rvykydal force-pushed the home-reuse-poc branch from 06b5bb9 to fc6917e Compare September 19, 2024 14:46

rvykydal temporarily deployed to gh-cockpituous September 19, 2024 14:46 — with GitHub Actions Inactive

rvykydal added 6 commits September 19, 2024 16:47

home reuse: update existing OSs when applying partitioning

0da39a2

home reuse: remove bootloader partitions implicitly

10e9f2d

home reuse: require removing of bootloader partition explicitly

147708a

But handle efi vs biosboot etc automagically.

home reuse: check autopartitioning scheme against reused mountpoints

51b6946

rvykydal force-pushed the home-reuse-poc branch from fc6917e to 51b6946 Compare September 19, 2024 14:47

rvykydal had a problem deploying to gh-cockpituous September 19, 2024 14:47 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for /home reuse to automatic partitioning #5814

Add support for /home reuse to automatic partitioning #5814

rvykydal commented Aug 9, 2024 •

edited

Loading

pep8speaks commented Aug 9, 2024 •

edited

Loading

rvykydal commented Aug 26, 2024 •

edited

Loading

jkonecny12 commented Sep 3, 2024 •

edited

Loading

rvykydal commented Sep 3, 2024 •

edited

Loading

rvykydal commented Sep 4, 2024

rvykydal commented Sep 4, 2024 •

edited

Loading

vojtechtrefny commented Sep 4, 2024

vojtechtrefny Sep 4, 2024

rvykydal Sep 6, 2024 •

edited

Loading

vojtechtrefny left a comment

jkonecny12 commented Sep 5, 2024

jkonecny12 commented Sep 5, 2024

jstodola commented Sep 5, 2024

rvykydal commented Sep 6, 2024

rvykydal commented Sep 6, 2024 •

edited

Loading

jkonecny12 commented Sep 6, 2024

jkonecny12 commented Sep 6, 2024 •

edited

Loading

rvykydal commented Sep 6, 2024 •

edited

Loading

jkonecny12 commented Sep 9, 2024

rvykydal commented Sep 18, 2024

rvykydal commented Sep 19, 2024 •

edited

Loading

Add support for /home reuse to automatic partitioning #5814

Are you sure you want to change the base?

Add support for /home reuse to automatic partitioning #5814

Conversation

rvykydal commented Aug 9, 2024 • edited Loading

pep8speaks commented Aug 9, 2024 • edited Loading

Comment last updated at 2024-09-19 14:47:12 UTC

rvykydal commented Aug 26, 2024 • edited Loading

jkonecny12 commented Sep 3, 2024 • edited Loading

rvykydal commented Sep 3, 2024 • edited Loading

rvykydal commented Sep 4, 2024

rvykydal commented Sep 4, 2024 • edited Loading

vojtechtrefny commented Sep 4, 2024

vojtechtrefny Sep 4, 2024

Choose a reason for hiding this comment

rvykydal Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

vojtechtrefny left a comment

Choose a reason for hiding this comment

jkonecny12 commented Sep 5, 2024

jkonecny12 commented Sep 5, 2024

jstodola commented Sep 5, 2024

rvykydal commented Sep 6, 2024

rvykydal commented Sep 6, 2024 • edited Loading

jkonecny12 commented Sep 6, 2024

jkonecny12 commented Sep 6, 2024 • edited Loading

rvykydal commented Sep 6, 2024 • edited Loading

jkonecny12 commented Sep 9, 2024

rvykydal commented Sep 18, 2024

rvykydal commented Sep 19, 2024 • edited Loading

rvykydal commented Aug 9, 2024 •

edited

Loading

pep8speaks commented Aug 9, 2024 •

edited

Loading

rvykydal commented Aug 26, 2024 •

edited

Loading

jkonecny12 commented Sep 3, 2024 •

edited

Loading

rvykydal commented Sep 3, 2024 •

edited

Loading

rvykydal commented Sep 4, 2024 •

edited

Loading

rvykydal Sep 6, 2024 •

edited

Loading

rvykydal commented Sep 6, 2024 •

edited

Loading

jkonecny12 commented Sep 6, 2024 •

edited

Loading

rvykydal commented Sep 6, 2024 •

edited

Loading

rvykydal commented Sep 19, 2024 •

edited

Loading