From 6f6f1153b93f3d7e2b1748eb44dd1b4e36510328 Mon Sep 17 00:00:00 2001
From: Vojtech Trefny <vtrefny@redhat.com>
Date: Wed, 31 Jul 2024 11:06:28 +0200
Subject: [PATCH] fix: Remove partition table from disk removed from a VG

When removing a PV from a VG we currently remove only the PV
partition and not the partition table on the disk. This fixes this
by removing the partition table too.
---
 library/blivet.py                | 37 +++++++++++++++++++-------------
 tests/tests_lvm_pool_members.yml | 18 ++++++++++++++++
 2 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/library/blivet.py b/library/blivet.py
index e784d90e..efcf0b26 100644
--- a/library/blivet.py
+++ b/library/blivet.py
@@ -1841,10 +1841,10 @@ def _manage_members(self):
             return
 
         add_disks = [d for d in self._disks if d not in self._device.ancestors]
-        remove_disks = [pv for pv in self._device.pvs if not any(d in pv.ancestors for d in self._disks)]
+        remove_pvs = [pv for pv in self._device.pvs if not any(d in pv.ancestors for d in self._disks)]
 
         if self._pool['grow_to_fill']:
-            grow_pv_candidates = [pv for pv in self._device.pvs if pv not in remove_disks and pv not in add_disks]
+            grow_pv_candidates = [pv for pv in self._device.pvs if pv not in remove_pvs and pv not in add_disks]
 
             for pv in grow_pv_candidates:
                 if abs(self._device.size - self._device.current_size) < 2 * self._device.pe_size:
@@ -1859,12 +1859,12 @@ def _manage_members(self):
                 else:
                     log.warning("cannot grow/resize PV '%s', format is not resizable", pv.name)
 
-        if not (add_disks or remove_disks):
+        if not (add_disks or remove_pvs):
             return
 
-        if remove_disks and safe_mode:
+        if remove_pvs and safe_mode:
             raise BlivetAnsibleError("cannot remove members '%s' from pool '%s' in safe mode" %
-                                     (", ".join(d.name for d in remove_disks),
+                                     (", ".join(d.name for d in remove_pvs),
                                       self._device.name))
 
         if self._is_raid:
@@ -1882,26 +1882,33 @@ def _manage_members(self):
                                                                                        self._pool['name'],
                                                                                        str(e)))
 
-        for disk in remove_disks:
-            if self._device.free_space < disk.size:
-                raise BlivetAnsibleError("disk '%s' cannot be removed from pool '%s'" % (disk.name,
-                                                                                         self._pool['name']))
+        for pv in remove_pvs:
+            if self._device.free_space < pv.size:
+                raise BlivetAnsibleError("member '%s' cannot be removed from pool '%s'" % (pv.name,
+                                                                                           self._pool['name']))
 
             try:
-                ac = ActionRemoveMember(self._device, disk)
+                ac = ActionRemoveMember(self._device, pv)
                 # XXX: scheduling ActionRemoveMember is currently broken, we need to execute
                 # the action now manually, see https://bugzilla.redhat.com/show_bug.cgi?id=2076956
                 # self._blivet.devicetree.actions.add(ac)
                 ac.apply()
                 ac.execute()
             except Exception as e:
-                raise BlivetAnsibleError("failed to remove disk '%s' from pool '%s': %s" % (disk.name,
-                                                                                            self._pool['name'],
-                                                                                            str(e)))
+                raise BlivetAnsibleError("failed to remove member '%s' from pool '%s': %s" % (pv.name,
+                                                                                              self._pool['name'],
+                                                                                              str(e)))
             # XXX workaround for https://github.com/storaged-project/blivet/pull/1040
-            disk.format.vg_name = None
+            pv.format.vg_name = None
+
+            self._blivet.devicetree.recursive_remove(pv.raw_device)
 
-            self._blivet.devicetree.recursive_remove(disk.raw_device)
+            # if we are using partitions remove also the disklabel from the disk (but make sure
+            # there aren't other partitions first)
+            if use_partitions and not pv.is_disk:
+                for disk in pv.disks:
+                    if disk.format.type == "disklabel" and not disk.format.partitions:
+                        self._blivet.devicetree.recursive_remove(disk)
 
     def _create(self):
         if not self._device:
diff --git a/tests/tests_lvm_pool_members.yml b/tests/tests_lvm_pool_members.yml
index 7d907d78..43db46af 100644
--- a/tests/tests_lvm_pool_members.yml
+++ b/tests/tests_lvm_pool_members.yml
@@ -109,6 +109,15 @@
               - name: foo
                 disks: "{{ [unused_disks[0]] }}"
 
+        - name: Get the partition table type on disk removed from the VG
+          command: lsblk -o PTTYPE --noheadings --nodeps /dev/{{ unused_disks[1] }}
+          register: storage_test_removed_pttype
+          changed_when: false
+
+        - name: Verify that removing the PV from the VG also removed the partition table
+          assert:
+            that: storage_test_removed_pttype.stdout == ''
+
         - name: Verify role results
           include_tasks: verify-role-results.yml
 
@@ -279,6 +288,15 @@
                   - name: test
                     size: "{{ volume_size }}"
 
+        - name: Get the partition table type on disk removed from the VG
+          command: lsblk -o PTTYPE --noheadings --nodeps /dev/{{ unused_disks[0] }}
+          register: storage_test_removed_pttype
+          changed_when: false
+
+        - name: Verify that removing the PV from the VG also removed the partition table
+          assert:
+            that: storage_test_removed_pttype.stdout == ''
+
         - name: Get UUID of the 'foo' volume group
           command: vgs --noheading -o vg_uuid foo
           changed_when: false