From dec18aba4e84e87a430108b0b3b719faf621b9d2 Mon Sep 17 00:00:00 2001 From: Vojtech Trefny Date: Tue, 20 May 2025 16:06:31 +0200 Subject: [PATCH] Make ActionDestroyFormat optional when the device is also scheduled to be removed Resolves: RHEL-8008 Resolves: RHEL-8012 --- 0036-Make-ActionDestroyFormat-optional.patch | 258 +++++++++++++++++++ python-blivet.spec | 8 +- 2 files changed, 265 insertions(+), 1 deletion(-) create mode 100644 0036-Make-ActionDestroyFormat-optional.patch diff --git a/0036-Make-ActionDestroyFormat-optional.patch b/0036-Make-ActionDestroyFormat-optional.patch new file mode 100644 index 0000000..f5077f6 --- /dev/null +++ b/0036-Make-ActionDestroyFormat-optional.patch @@ -0,0 +1,258 @@ +From 68db0569b3508bbedf33d9ee3b69e8fc6a309b65 Mon Sep 17 00:00:00 2001 +From: Vojtech Trefny +Date: Fri, 16 May 2025 17:15:17 +0200 +Subject: [PATCH 1/4] 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-8008 +Resolves: RHEL-8012 +--- + blivet/deviceaction.py | 37 +++++++++++++++++++++++-------------- + 1 file changed, 23 insertions(+), 14 deletions(-) + +diff --git a/blivet/deviceaction.py b/blivet/deviceaction.py +index fc1ca4b65..a6fc211ea 100644 +--- a/blivet/deviceaction.py ++++ b/blivet/deviceaction.py +@@ -728,12 +728,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) +@@ -752,21 +753,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 fca71515094840ab1ca8821641284cfb0b687d82 Mon Sep 17 00:00:00 2001 +From: Vojtech Trefny +Date: Fri, 16 May 2025 17:28:40 +0200 +Subject: [PATCH 2/4] 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-8008 +Resolves: RHEL-8012 +--- + 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 dc066b036..2e86f5bf6 100644 +--- a/blivet/blivet.py ++++ b/blivet/blivet.py +@@ -897,7 +897,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 c6c1b4400..f94e3ca30 100644 +--- a/blivet/devicetree.py ++++ b/blivet/devicetree.py +@@ -264,7 +264,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: +@@ -276,7 +276,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 50efc63fa3053f863d03439a507b3e0a6d7b8168 Mon Sep 17 00:00:00 2001 +From: Vojtech Trefny +Date: Mon, 19 May 2025 14:24:06 +0200 +Subject: [PATCH 3/4] tests: Add a simple test case for optional format destroy + action + +Related: RHEL-8008 +Related: RHEL-8012 +--- + tests/unit_tests/devices_test/lvm_test.py | 29 +++++++++++++++++++++++ + 1 file changed, 29 insertions(+) + +diff --git a/tests/unit_tests/devices_test/lvm_test.py b/tests/unit_tests/devices_test/lvm_test.py +index e645309fc..34c2084a8 100644 +--- a/tests/unit_tests/devices_test/lvm_test.py ++++ b/tests/unit_tests/devices_test/lvm_test.py +@@ -1160,3 +1160,32 @@ def test_vdo_compression_deduplication_change(self): + with patch("blivet.devices.lvm.blockdev.lvm") as lvm: + 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(unittest.TestCase): ++ ++ def test_optional_format_destroy(self, *args): # pylint: disable=unused-argument ++ b = blivet.Blivet() ++ 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): ++ b.devicetree._add_device(dev) ++ ++ b.destroy_device(lv) ++ fmt_ac = 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: ++ b.do_it() ++ except RuntimeError: ++ self.fail("Optional format destroy action is not optional") + +From ea913c5fa8e60cd5c2fdd8196be51c067a2a73d8 Mon Sep 17 00:00:00 2001 +From: Vojtech Trefny +Date: Tue, 20 May 2025 13:02:00 +0200 +Subject: [PATCH 4/4] tests: Add test case for removing broken thin pool + +Related: RHEL-8008 +Related: RHEL-8012 +--- + tests/storage_tests/devices_test/lvm_test.py | 52 ++++++++++++++++++++ + 1 file changed, 52 insertions(+) + +diff --git a/tests/storage_tests/devices_test/lvm_test.py b/tests/storage_tests/devices_test/lvm_test.py +index a1064c9c4..10e7354ff 100644 +--- a/tests/storage_tests/devices_test/lvm_test.py ++++ b/tests/storage_tests/devices_test/lvm_test.py +@@ -1,5 +1,7 @@ + import os + import subprocess ++import tempfile ++from unittest.mock import patch + + from ..storagetestcase import StorageTestCase + +@@ -552,3 +554,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, vgname): ++ os.system("vgchange -an %s >/dev/null 2>&1" % 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, 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, 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="blivetTestVG", 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("blivetTestVG") ++ ++ self.storage.reset() ++ ++ pool = self.storage.devicetree.get_device_by_name("blivetTestVG-blivetTestPool") ++ 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() diff --git a/python-blivet.spec b/python-blivet.spec index 0219a4b..1c56af0 100644 --- a/python-blivet.spec +++ b/python-blivet.spec @@ -23,7 +23,7 @@ Version: 3.6.0 #%%global prerelease .b2 # prerelease, if defined, should be something like .a1, .b1, .b2.dev1, or .c2 -Release: 25%{?prerelease}%{?dist} +Release: 26%{?prerelease}%{?dist} Epoch: 1 License: LGPLv2+ %global realname blivet @@ -65,6 +65,7 @@ Patch31: 0032-Set-persistent-allow-discards-flag-for-new-LUKS-devices.patch Patch32: 0033-Do-not-remove-PVs-from-devices-file-if-disabled-or-doesnt-exist.patch Patch33: 0034-Include-additional-information-in-PartitioningError.patch Patch34: 0035-LVMPV-format-size-fix.patch +Patch35: 0036-Make-ActionDestroyFormat-optional.patch # Versions of required components (done so we make sure the buildrequires # match the requires versions of things). @@ -228,6 +229,11 @@ configuration. %endif %changelog +* Tue May 20 2025 Vojtech Trefny - 3.6.0-26 +- Make ActionDestroyFormat optional when the device is also scheduled to be removed + Resolves: RHEL-8008 + Resolves: RHEL-8012 + * Mon Apr 14 2025 Vojtech Trefny - 3.6.0-25 - Get the actual PV format size for LVMPV format Resolves: RHEL-74078