Use blivet's "device ID" as a unique device identifier #5435

vojtechtrefny · 2024-01-30T13:56:19Z

Anaconda currently uses name to identify devices internally which causes a lot of problems because multiple devices can share the same name. To fix this blivet introduced a new "device ID" which is unique even for devices with the same name (for technologies that support multiple devices with the same name and for devices sharing the same name but using a different storage technology).

This is not yet finished, but it should be usable for testing and further development on the WebUI side. The blivet change (storaged-project/blivet#1182) is merged but not yet released. Unit tests should be passing on this, I also tried running manual installation with mount point assignment in TUI and also running (most of) kickstart tests and everything seems to be working so far. The Gtk custom partitioning is not yet adjusted to this change but is somewhat working if you ignore the weird "names" displayed in the UI.

pep8speaks · 2024-01-30T13:56:45Z

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

In the file tests/unit_tests/pyanaconda_tests/modules/storage/test_module_device_tree.py:

Line 28:63: E231 missing whitespace after ','

Comment last updated at 2024-07-31 12:52:41 UTC

M4rtinK

Looks good to me code wise, but I'm afraid I can't really judge the full context of this change in regards to Blivet & Web UI. :P

rvykydal · 2024-02-13T09:22:05Z

If I create biosboot partition, boot partition and btrfs volume with a subvolume in terminal:

set -x

DISK=$1

sgdisk --zap-all ${DISK}
sgdisk --new=0:0:+1MiB --typecode=0:ef02 ${DISK}
sgdisk --new=0:0:+1GB ${DISK}
mkfs.ext4 -F ${DISK}2
sgdisk --new=0:0:+10GiB ${DISK}
mkfs.btrfs -f -L v3 ${DISK}3

# DUP
#sgdisk --new=0:0:+1GiB ${DISK}
#mkfs.btrfs -f -L v4 ${DISK}4


##mkfs.btrfs -f -f -L btrfstest ${DISK}3
##sgdisk --new=0:0:+2GiB ${DISK}
##mkfs.btrfs -f -f -L btrfstest ${DISK}4

TMP_MOUNT="/tmp/btrfs-mount-test"
mkdir -p ${TMP_MOUNT}

mount ${DISK}3 ${TMP_MOUNT}
btrfs subvolume create ${TMP_MOUNT}/root
umount ${TMP_MOUNT}

# DUP
#mount ${DISK}4 ${TMP_MOUNT}
#btrfs subvolume create ${TMP_MOUNT}/root
#umount ${TMP_MOUNT}

rmdir ${TMP_MOUNT}


#sgdisk --new=0:0:0 ${DISK}
#mkfs.xfs -f ${DISK}4


udevadm trigger
udevadm settle --timeout=120

lsblk
blkid

Then rescan the storage, go to mount points assignment, assign the partitions with reformatting I get failure in:

INFO:blivet:registered action: [224] create device btrfs subvolume root (id 219)
DEBUG:blivet:             DeviceTree.get_device_by_device_id: device_id: BTRFS-dafddd9d-7a42-414d-b66c-223d290d6b03-root ; incomplete: False ; hidden: False ;
DEBUG:blivet:             DeviceTree.get_device_by_device_id returned None
INFO:anaconda.core.threads:Thread Failed: AnaTaskThread-ManualPartitioningTask-1 (139822322681536)
ERROR:anaconda.modules.common.task.task:Thread AnaTaskThread-ManualPartitioningTask-1 has failed: Traceback (most recent call last):
  File "/usr/lib64/python3.12/site-packages/pyanaconda/core/threads.py", line 280, in run
    threading.Thread.run(self)
  File "/usr/lib64/python3.12/threading.py", line 1010, in run
    self._target(*self._args, **self._kwargs)
  File "/usr/lib64/python3.12/site-packages/pyanaconda/modules/common/task/task.py", line 94, in _thread_run_callback
    self._task_run_callback()
  File "/usr/lib64/python3.12/site-packages/pyanaconda/modules/common/task/task.py", line 107, in _task_run_callback
    self._set_result(self.run())
                     ^^^^^^^^^^
  File "/usr/lib64/python3.12/site-packages/pyanaconda/modules/storage/partitioning/base_partitioning.py", line 53, in run
    self._run(self._storage)
  File "/usr/lib64/python3.12/site-packages/pyanaconda/modules/storage/partitioning/automatic/noninteractive_partitioning.py", line 43, in _run
    self._configure_partitioning(storage)
  File "/usr/lib64/python3.12/site-packages/pyanaconda/modules/storage/partitioning/manual/manual_partitioning.py", line 53, in _configure_partitioning
    self._setup_mount_point(storage, mount_data)
  File "/usr/lib64/python3.12/site-packages/pyanaconda/modules/storage/partitioning/manual/manual_partitioning.py", line 116, in _setup_mount_point
    mount_data.mount_options = device.format.options
                               ^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'format'

storage.log

I think it happens because the recreated subvolume is not found here:
https://github.com/vojtechtrefny/anaconda/blob/8dba5f232bfc90141a16859fe18f465f7a822345/pyanaconda/modules/storage/partitioning/manual/manual_partitioning.py#L169

It used to work (at least got past this point as I checked) but I am not 100% sure my use case makes sense. (Maybe the issue is that the volume is recreated as well because the only subvolume is destroyed so the uuids are new?)
I am looking into it.

rvykydal · 2024-02-13T09:57:03Z

It used to work (at least got past this point as I checked) but I am not 100% sure my use case makes sense. (Maybe the issue is that the volume is recreated as well because the only subvolume is destroyed so the uuids are new?) I am looking into it.

Yes, the issue here seems that if the volume is destroyed together with the subvolume and then recreated, finding the subvolume by device_id containing uuid of the former subvolume fails. I don't hit the issue if the volume contains 2 subvolumes and I reformat one of them in mount point assignment.

I'll update the webui tests for this PR, add a test for this specific case.
Then I'll perhaps update presentation of the device uuid in the WebUI to the user (like BTRFS-f586c782-f9da-4cfa-b9a4-c3da16914bae-data -> BTRFS subvolume data of volume vol (f586c782-f9da-4cfa-b9a4-c3da16914) - but this will be subject of discussion I guess.

vojtechtrefny · 2024-02-15T12:38:26Z

@rvykydal I've added a new commit that should fix this issue.

rvykydal · 2024-02-15T13:56:05Z

@rvykydal I've added a new commit that should fix this issue.

Thank you, I have just a few nitpicks.

pyanaconda/modules/storage/partitioning/manual/manual_partitioning.py

github-actions · 2024-04-30T02:05:48Z

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

KKoukiou

Should be mark in the specfile tha tthis current anaconda-core is incomptatible with the current version of the Web UI package?

pyanaconda/ui/gui/spokes/lib/resize.glade

pyanaconda/modules/storage/devicetree/viewer.py

pyanaconda/ui/lib/storage.py

tests/unit_tests/pyanaconda_tests/modules/storage/test_module_storage.py

rvykydal

Looks good to me.

Anaconda currently uses name to identify devices internally which causes a lot of problems because multiple devices can share the same name. To fix this blivet introduced a new "device ID" which is unique even for devices with the same name (for technologies that support multiple devices with the same name and for devices sharing the same name but using a different storage technology).

We can't use the device factory in the manual partitioning because when it removes the last subvolume it also removes the volume which want to keep untouched when just recreating the subvolume.

We want to show device name (or description) in the reclaim space dialog but use the unique device ID internally.

This should cover all places where name was used as device identifier in the storage, advanced storage and custom spokes.

vojtechtrefny · 2024-08-01T05:40:27Z

/kickstart-test --testtype smoke

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) TODO: + unify biosboot partition with other stuff (configurable) + look at all roots + test with plain + test with lvm + test with thin lvm + --remove should fail silently? - kickstart support (--erase --reuse --remove) - kickstart tests - reuse fstab mount options (/home is missing compression) + add checks that the device found is unique (including biosboot/efi) - checks for recreate only on plain partition? - create check for usability of the /home reuse (for UI) - update unit tests - luks ? - efi partitions, we have only biosboot so far - update for device_id: rhinstaller#5435 - add better mount point assignment kickstart tests, we update code there (fedora, lvm, plain)

KKoukiou · 2024-08-05T13:56:02Z

/build-image --live

github-actions · 2024-08-05T14:22:39Z

Images built based on commit 2489951:

Live: success

Download the images from the bottom of the job status page.

KKoukiou · 2024-08-05T14:56:27Z

/build-image

github-actions · 2024-08-05T15:07:59Z

Images built based on commit 2489951:

boot.iso: success

Download the images from the bottom of the job status page.

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) TODO: + unify biosboot partition with other stuff (configurable) + look at all roots + test with plain + test with lvm + test with thin lvm + --remove should fail silently? + kickstart support (--erase --reuse --remove) - kickstart tests - reuse fstab mount options (/home is missing compression) + add checks that the device found is unique (including biosboot/efi) - checks for recreate only on plain partition? - create check for usability of the /home reuse (for UI) - update unit tests - luks ? - efi partitions, we have only biosboot so far - update for device_id: rhinstaller#5435 - add better mount point assignment kickstart tests, we update code there (fedora, lvm, plain)

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) TODO: + unify biosboot partition with other stuff (configurable) + look at all roots + test with plain + test with lvm + test with thin lvm + --remove should fail silently? + kickstart support (--erase --reuse --remove) - kickstart tests - reuse fstab mount options (/home is missing compression) + add checks that the device found is unique (including biosboot/efi) - checks for recreate only on plain partition? - create check for usability of the /home reuse (for UI) - update unit tests - luks? .. same as mount-point-assignment ? - efi partitions, we have only biosboot so far + update for device_id: rhinstaller#5435 - add better mount point assignment kickstart tests, we update code there (fedora, lvm, plain)

vojtechtrefny temporarily deployed to gh-cockpituous January 30, 2024 13:56 — with GitHub Actions Inactive

github-actions bot added the f40 label Jan 30, 2024

M4rtinK approved these changes Jan 30, 2024

View reviewed changes

vojtechtrefny force-pushed the master_devices-device-id branch from 804ac0b to 8dba5f2 Compare February 1, 2024 08:08

vojtechtrefny temporarily deployed to gh-cockpituous February 1, 2024 08:08 — with GitHub Actions Inactive

vojtechtrefny temporarily deployed to gh-cockpituous February 15, 2024 09:29 — with GitHub Actions Inactive

rvykydal mentioned this pull request Feb 15, 2024

Update tests for device_id change rhinstaller/anaconda-webui#177

Closed

2 tasks

rvykydal reviewed Feb 15, 2024

View reviewed changes

pyanaconda/modules/storage/partitioning/manual/manual_partitioning.py Outdated Show resolved Hide resolved

rvykydal reviewed Feb 15, 2024

View reviewed changes

pyanaconda/modules/storage/partitioning/manual/manual_partitioning.py Outdated Show resolved Hide resolved

vojtechtrefny force-pushed the master_devices-device-id branch from 537b079 to ffd316a Compare February 15, 2024 15:10

vojtechtrefny temporarily deployed to gh-cockpituous February 15, 2024 15:10 — with GitHub Actions Inactive

vojtechtrefny force-pushed the master_devices-device-id branch from ffd316a to 98db250 Compare February 19, 2024 09:46

vojtechtrefny temporarily deployed to gh-cockpituous February 19, 2024 09:46 — with GitHub Actions Inactive

KKoukiou added f41 and removed f40 labels Feb 29, 2024

github-actions bot added the stale label Apr 30, 2024

KKoukiou removed the stale label May 7, 2024

vojtechtrefny force-pushed the master_devices-device-id branch from 98db250 to 3270ab2 Compare June 25, 2024 08:18

vojtechtrefny temporarily deployed to gh-cockpituous June 25, 2024 08:18 — with GitHub Actions Inactive

vojtechtrefny force-pushed the master_devices-device-id branch from 3270ab2 to 2f1b9a3 Compare June 26, 2024 10:12

vojtechtrefny temporarily deployed to gh-cockpituous June 26, 2024 10:12 — with GitHub Actions Inactive

vojtechtrefny temporarily deployed to gh-cockpituous June 26, 2024 12:58 — with GitHub Actions Inactive

vojtechtrefny force-pushed the master_devices-device-id branch from 3b6955b to c107e89 Compare July 8, 2024 13:01

vojtechtrefny had a problem deploying to gh-cockpituous July 8, 2024 13:01 — with GitHub Actions Failure

vojtechtrefny force-pushed the master_devices-device-id branch from 651f24c to 922df8c Compare July 12, 2024 13:14

vojtechtrefny temporarily deployed to gh-cockpituous July 12, 2024 13:14 — with GitHub Actions Inactive

KKoukiou reviewed Jul 26, 2024

View reviewed changes

pyanaconda/ui/gui/spokes/lib/resize.glade Show resolved Hide resolved

rvykydal reviewed Jul 31, 2024

View reviewed changes

pyanaconda/modules/storage/devicetree/viewer.py Outdated Show resolved Hide resolved

rvykydal reviewed Jul 31, 2024

View reviewed changes

pyanaconda/ui/lib/storage.py Show resolved Hide resolved

rvykydal reviewed Jul 31, 2024

View reviewed changes

tests/unit_tests/pyanaconda_tests/modules/storage/test_module_storage.py Show resolved Hide resolved

rvykydal approved these changes Jul 31, 2024

View reviewed changes

vojtechtrefny force-pushed the master_devices-device-id branch from 922df8c to 9176e7d Compare July 31, 2024 12:23

vojtechtrefny had a problem deploying to gh-cockpituous July 31, 2024 12:23 — with GitHub Actions Failure

vojtechtrefny added 4 commits July 31, 2024 14:51

Add a custom function for recreating btrfs subvolumes

f2f5318

We can't use the device factory in the manual partitioning because when it removes the last subvolume it also removes the volume which want to keep untouched when just recreating the subvolume.

Adjust resize module to the device ID API

b13375d

We want to show device name (or description) in the reclaim space dialog but use the unique device ID internally.

Adjust custom partitioning and storage spokes to the device ID API

2489951

This should cover all places where name was used as device identifier in the storage, advanced storage and custom spokes.

vojtechtrefny force-pushed the master_devices-device-id branch from 9176e7d to 2489951 Compare July 31, 2024 12:52

vojtechtrefny had a problem deploying to gh-cockpituous July 31, 2024 12:52 — with GitHub Actions Failure

rvykydal mentioned this pull request Aug 5, 2024

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

Closed

KKoukiou merged commit 3453d88 into rhinstaller:master Aug 6, 2024
16 of 17 checks passed

rvykydal mentioned this pull request Aug 7, 2024

mountpoint-assignment-X test failing on daily-iso with "org.fedoraproject.Anaconda.Modules.Storage:AttributeError: 'NoneType' object has no attribute 'device_id'" rhinstaller/kickstart-tests#1273

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use blivet's "device ID" as a unique device identifier #5435

Use blivet's "device ID" as a unique device identifier #5435

vojtechtrefny commented Jan 30, 2024

pep8speaks commented Jan 30, 2024 •

edited

Loading

M4rtinK left a comment

rvykydal commented Feb 13, 2024

rvykydal commented Feb 13, 2024 •

edited

Loading

vojtechtrefny commented Feb 15, 2024

rvykydal commented Feb 15, 2024

github-actions bot commented Apr 30, 2024

KKoukiou left a comment

rvykydal left a comment

vojtechtrefny commented Aug 1, 2024

KKoukiou commented Aug 5, 2024

github-actions bot commented Aug 5, 2024

KKoukiou commented Aug 5, 2024

github-actions bot commented Aug 5, 2024

Use blivet's "device ID" as a unique device identifier #5435

Use blivet's "device ID" as a unique device identifier #5435

Conversation

vojtechtrefny commented Jan 30, 2024

pep8speaks commented Jan 30, 2024 • edited Loading

Comment last updated at 2024-07-31 12:52:41 UTC

M4rtinK left a comment

Choose a reason for hiding this comment

rvykydal commented Feb 13, 2024

rvykydal commented Feb 13, 2024 • edited Loading

vojtechtrefny commented Feb 15, 2024

rvykydal commented Feb 15, 2024

github-actions bot commented Apr 30, 2024

KKoukiou left a comment

Choose a reason for hiding this comment

rvykydal left a comment

Choose a reason for hiding this comment

vojtechtrefny commented Aug 1, 2024

KKoukiou commented Aug 5, 2024

github-actions bot commented Aug 5, 2024

KKoukiou commented Aug 5, 2024

github-actions bot commented Aug 5, 2024

pep8speaks commented Jan 30, 2024 •

edited

Loading

rvykydal commented Feb 13, 2024 •

edited

Loading