From 8368cab41a1f34452b4c624768245517391ce400 Mon Sep 17 00:00:00 2001 From: Vojtech Trefny Date: Fri, 16 May 2025 17:15:17 +0200 Subject: [PATCH 1/5] Allow ActionDestroyFormat to be marked as optional When we are also planning to remove the device, failing to remove the format is not critical so we can ignore it in these cases. Resolves: RHEL-84685 Resolves: RHEL-84663 --- blivet/deviceaction.py | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/blivet/deviceaction.py b/blivet/deviceaction.py index b22e00c36..2e6a8489f 100644 --- a/blivet/deviceaction.py +++ b/blivet/deviceaction.py @@ -734,12 +734,13 @@ class ActionDestroyFormat(DeviceAction): obj = ACTION_OBJECT_FORMAT type_desc_str = N_("destroy format") - def __init__(self, device): + def __init__(self, device, optional=False): if device.format_immutable: raise ValueError("this device's formatting cannot be modified") DeviceAction.__init__(self, device) self.orig_format = self.device.format + self.optional = optional if not device.format.destroyable: raise ValueError("resource to destroy this format type %s is unavailable" % device.format.type) @@ -758,21 +759,29 @@ def execute(self, callbacks=None): """ wipe the filesystem signature from the device """ # remove any flag if set super(ActionDestroyFormat, self).execute(callbacks=callbacks) - status = self.device.status - self.device.setup(orig=True) - if hasattr(self.device, 'set_rw'): - self.device.set_rw() - self.format.destroy() - udev.settle() - if isinstance(self.device, PartitionDevice) and self.device.disklabel_supported: - if self.format.parted_flag: - self.device.unset_flag(self.format.parted_flag) - self.device.disk.original_format.commit_to_disk() - udev.settle() + try: + status = self.device.status + self.device.setup(orig=True) + if hasattr(self.device, 'set_rw'): + self.device.set_rw() - if not status: - self.device.teardown() + self.format.destroy() + udev.settle() + if isinstance(self.device, PartitionDevice) and self.device.disklabel_supported: + if self.format.parted_flag: + self.device.unset_flag(self.format.parted_flag) + self.device.disk.original_format.commit_to_disk() + udev.settle() + + if not status: + self.device.teardown() + except Exception as e: # pylint: disable=broad-except + if self.optional: + log.error("Ignoring error when executing optional action: Failed to destroy format on %s: %s.", + self.device.name, str(e)) + else: + raise def cancel(self): if not self._applied: From 94e0ec7f24129159ac5f4fe455f37b85ceb9a004 Mon Sep 17 00:00:00 2001 From: Vojtech Trefny Date: Fri, 16 May 2025 17:28:40 +0200 Subject: [PATCH 2/5] Make ActionDestroyFormat optional when device is also removed In both destroy_device and recursive_remove we try to remove both the device and its format. In these cases the format destroy can be considered to be optional and we don't need to fail just because we failed to remove the format. Resolves: RHEL-84685 Resolves: RHEL-84663 --- blivet/blivet.py | 2 +- blivet/devicetree.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/blivet/blivet.py b/blivet/blivet.py index 399992a41..53206d973 100644 --- a/blivet/blivet.py +++ b/blivet/blivet.py @@ -915,7 +915,7 @@ def destroy_device(self, device): if device.format.exists and device.format.type and \ not device.format_immutable: # schedule destruction of any formatting while we're at it - self.devicetree.actions.add(ActionDestroyFormat(device)) + self.devicetree.actions.add(ActionDestroyFormat(device, optional=True)) action = ActionDestroyDevice(device) self.devicetree.actions.add(action) diff --git a/blivet/devicetree.py b/blivet/devicetree.py index 6a27b1e71..4ec955002 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -261,7 +261,7 @@ def recursive_remove(self, device, actions=True, remove_device=True, modparent=T if actions: if leaf.format.exists and not leaf.protected and \ not leaf.format_immutable: - self.actions.add(ActionDestroyFormat(leaf)) + self.actions.add(ActionDestroyFormat(leaf, optional=True)) self.actions.add(ActionDestroyDevice(leaf)) else: @@ -273,7 +273,7 @@ def recursive_remove(self, device, actions=True, remove_device=True, modparent=T if not device.format_immutable: if actions: - self.actions.add(ActionDestroyFormat(device)) + self.actions.add(ActionDestroyFormat(device, optional=True)) else: device.format = None From 610b65450fa00a9b8b129ef733536ca080edc6fe Mon Sep 17 00:00:00 2001 From: Vojtech Trefny Date: Mon, 19 May 2025 14:24:06 +0200 Subject: [PATCH 3/5] tests: Add a simple test case for optional format destroy action Related: RHEL-84685 Related: RHEL-84663 --- tests/unit_tests/devices_test/lvm_test.py | 28 +++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/unit_tests/devices_test/lvm_test.py b/tests/unit_tests/devices_test/lvm_test.py index ed30772fd..7ec3ed0ae 100644 --- a/tests/unit_tests/devices_test/lvm_test.py +++ b/tests/unit_tests/devices_test/lvm_test.py @@ -1172,3 +1172,31 @@ def test_vdo_compression_deduplication_change(self): with patch("blivet.devices.lvm.blockdev.lvm") as lvm: self.b.do_it() lvm.vdo_enable_deduplication.assert_called_with(vg.name, vdopool.lvname) + + +@patch("blivet.devices.lvm.LVMLogicalVolumeDevice._external_dependencies", new=[]) +@patch("blivet.devices.lvm.LVMLogicalVolumeBase._external_dependencies", new=[]) +@patch("blivet.devices.dm.DMDevice._external_dependencies", new=[]) +class BlivetLVMOptionalDestroyTest(BlivetLVMUnitTest): + + def test_optional_format_destroy(self, *args): # pylint: disable=unused-argument + pv = StorageDevice("pv1", fmt=blivet.formats.get_format("lvmpv"), + size=Size("10 GiB"), exists=True) + vg = LVMVolumeGroupDevice("testvg", parents=[pv], exists=True) + lv = LVMLogicalVolumeDevice("testlv", parents=[vg], exists=True, size=Size("5 GiB"), + fmt=blivet.formats.get_format("xfs", exists=True)) + + for dev in (pv, vg, lv): + self.b.devicetree._add_device(dev) + + self.b.destroy_device(lv) + fmt_ac = self.b.devicetree.actions.find(action_type="destroy", object_type="format") + self.assertTrue(fmt_ac) + self.assertTrue(fmt_ac[0].optional) + + with patch("blivet.devices.lvm.blockdev.lvm") as lvm: + lvm.lvactivate.side_effect = RuntimeError() + try: + self.b.do_it() + except RuntimeError: + self.fail("Optional format destroy action is not optional") From d5c9b690f702d38a9db5bed5d728a1a25fe31077 Mon Sep 17 00:00:00 2001 From: Vojtech Trefny Date: Tue, 20 May 2025 13:02:00 +0200 Subject: [PATCH 4/5] tests: Add test case for removing broken thin pool Related: RHEL-84685 Related: RHEL-84663 --- tests/storage_tests/devices_test/lvm_test.py | 51 ++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/storage_tests/devices_test/lvm_test.py b/tests/storage_tests/devices_test/lvm_test.py index 25d9d71bb..aae9da8b5 100644 --- a/tests/storage_tests/devices_test/lvm_test.py +++ b/tests/storage_tests/devices_test/lvm_test.py @@ -1,6 +1,7 @@ import os import shutil import subprocess +import tempfile from ..storagetestcase import StorageTestCase @@ -574,3 +575,53 @@ def test_lvm_pv_size(self): self.assertEqual(vg_size, vg.size) vg_free = self._get_vg_free(vg.name) self.assertEqual(vg_free, vg.free_space) + + def _break_thin_pool(self): + os.system("vgchange -an %s >/dev/null 2>&1" % self.vgname) + + # changing transaction_id for the pool prevents it from being activated + with tempfile.NamedTemporaryFile(prefix="blivet_test") as temp: + os.system("vgcfgbackup -f %s %s >/dev/null 2>&1" % (temp.name, self.vgname)) + os.system("sed -i 's/transaction_id =.*/transaction_id = 123456/' %s >/dev/null 2>&1" % temp.name) + os.system("vgcfgrestore -f %s %s --force >/dev/null 2>&1" % (temp.name, self.vgname)) + + def test_lvm_broken_thin(self): + disk = self.storage.devicetree.get_device_by_path(self.vdevs[0]) + self.assertIsNotNone(disk) + + self.storage.initialize_disk(disk) + + pv = self.storage.new_partition(size=blivet.size.Size("100 MiB"), fmt_type="lvmpv", + parents=[disk]) + self.storage.create_device(pv) + + blivet.partitioning.do_partitioning(self.storage) + + vg = self.storage.new_vg(name=self.vgname, parents=[pv]) + self.storage.create_device(vg) + + pool = self.storage.new_lv(thin_pool=True, size=blivet.size.Size("50 MiB"), + parents=[vg], name="blivetTestPool") + self.storage.create_device(pool) + + self.storage.do_it() + + # intentionally break the thin pool created above + self._break_thin_pool() + + self.storage.reset() + + pool = self.storage.devicetree.get_device_by_name("%s-blivetTestPool" % self.vgname) + self.assertIsNotNone(pool) + + # check that the pool cannot be activated + try: + pool.setup() + except Exception: # pylint: disable=broad-except + pass + else: + self.fail("Failed to break thinpool for tests") + + # verify that the pool can be destroyed even if it cannot be activated + self.storage.recursive_remove(pool) + self.storage.do_it() From 6f0625e06a2ea69be8042cf5e76048b97a1025e1 Mon Sep 17 00:00:00 2001 From: Vojtech Trefny Date: Tue, 29 Apr 2025 08:09:06 +0200 Subject: [PATCH 5/5] Fix expected exception type when activating devices in populor We are no longer raising libblockdev exceptions in our public API calls (see #1014) so when calling setup() ourselves we need to catch our exceptions instead of libblockdev ones as well. Related: RHEL-84685 Related: RHEL-84663 --- blivet/populator/helpers/luks.py | 2 +- blivet/populator/helpers/lvm.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/blivet/populator/helpers/luks.py b/blivet/populator/helpers/luks.py index 72da248ed..0b72920e3 100644 --- a/blivet/populator/helpers/luks.py +++ b/blivet/populator/helpers/luks.py @@ -161,7 +161,7 @@ def run(self): self.device.format.passphrase = passphrase try: self.device.format.setup() - except blockdev.BlockDevError: + except LUKSError: self.device.format.passphrase = None else: break diff --git a/blivet/populator/helpers/lvm.py b/blivet/populator/helpers/lvm.py index e22c52088..cdf97e405 100644 --- a/blivet/populator/helpers/lvm.py +++ b/blivet/populator/helpers/lvm.py @@ -29,7 +29,7 @@ from ... import udev from ...devicelibs import lvm from ...devices.lvm import LVMVolumeGroupDevice, LVMLogicalVolumeDevice, LVMInternalLVtype -from ...errors import DeviceTreeError, DuplicateVGError +from ...errors import DeviceTreeError, DuplicateVGError, LVMError from ...flags import flags from ...size import Size from ...storage_log import log_method_call @@ -289,7 +289,7 @@ def add_lv(lv): if flags.auto_dev_updates: try: lv_device.setup() - except blockdev.LVMError: + except LVMError: log.warning("failed to activate lv %s", lv_device.name) lv_device.controllable = False