Merge pull request #1418 from ogayot/LP#1988407

filesystem: fix crash when editing LVM logical volume

subiquity/common/filesystem/gaps.py

@@ -14,6 +14,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import functools
+from typing import Tuple, List
 
 import attr
 
@@ -23,6 +24,7 @@ from subiquity.models.filesystem import (
     align_down,
     Disk,
     LVM_CHUNK_SIZE,
+    LVM_LogicalVolume,
     LVM_VolGroup,
     Partition,
     Raid,
@@ -240,7 +242,22 @@ def largest_gap_size(device, in_extended=None):
     return 0
 
 
+@functools.singledispatch
 def movable_trailing_partitions_and_gap_size(partition):
+    """ For a given partition (or LVM logical volume), return the total,
+    potentially available, free space immediately following the partition.
+    By potentially available, we mean that to claim that much free space, some
+    other partitions might need to be moved.
+    The return value is a tuple that has two values:
+     * the list of partitions that would need to be moved
+     * the total potentially available free space
+    """
+    raise NotImplementedError
+
+
+@movable_trailing_partitions_and_gap_size.register
+def _movable_trailing_partitions_and_gap_size_partition(partition: Partition) \
+        -> Tuple[List[Partition], int]:
     pgs = parts_and_gaps(partition.device)
     part_idx = pgs.index(partition)
     trailing_partitions = []
@@ -260,6 +277,15 @@ def movable_trailing_partitions_and_gap_size(partition):
     return (trailing_partitions, 0)
 
 
+@movable_trailing_partitions_and_gap_size.register
+def _movable_trailing_partitions_and_gap_size_lvm(volume: LVM_LogicalVolume) \
+        -> Tuple[List[LVM_LogicalVolume], int]:
+    # In a Volume Group, there is no need to move partitions around, one can
+    # always use the remaining space.
+
+    return ([], largest_gap_size(volume.volgroup))
+
+
 def at_offset(device, offset):
     for pg in parts_and_gaps(device):
         if isinstance(pg, Gap):

subiquity/common/filesystem/tests/test_gaps.py

@@ -21,15 +21,20 @@ from parameterized import parameterized
 
 from subiquity.models.filesystem import (
     Disk,
+    LVM_CHUNK_SIZE,
+    LVM_OVERHEAD,
     MiB,
     PartitionAlignmentData,
     Raid,
     )
 from subiquity.models.tests.test_filesystem import (
     make_disk,
+    make_lv,
     make_model,
     make_model_and_disk,
+    make_model_and_lv,
     make_partition,
+    make_vg,
     )
 
 from subiquity.common.filesystem import gaps
@@ -449,6 +454,20 @@ class TestMovableTrailingPartitionsAndGapSize(GapTestCase):
             ([p2], 30),
             gaps.movable_trailing_partitions_and_gap_size(p1))
 
+    def test_two_trailing_movable_partitions_and_gap(self):
+        self.use_alignment_data(PartitionAlignmentData(
+            part_align=10, min_gap_size=1, min_start_offset=10,
+            min_end_offset=10, primary_part_limit=10))
+        # 0----10---20---30---40---50---60---70---80---90---100
+        # #####[ p1               ][ p2 ][ p3 ]        #####
+        m, d = make_model_and_disk(size=100)
+        p1 = make_partition(m, d, offset=10, size=40)
+        p2 = make_partition(m, d, offset=50, size=10)
+        p3 = make_partition(m, d, offset=60, size=10)
+        self.assertEqual(
+            ([p2, p3], 20),
+            gaps.movable_trailing_partitions_and_gap_size(p1))
+
     def test_one_trailing_movable_partition_and_no_gap(self):
         self.use_alignment_data(PartitionAlignmentData(
             part_align=10, min_gap_size=1, min_start_offset=10,
@@ -548,6 +567,21 @@ class TestMovableTrailingPartitionsAndGapSize(GapTestCase):
             ([], 0),
             gaps.movable_trailing_partitions_and_gap_size(p1))
 
+    def test_trailing_lvm_logical_volume_no_gap(self):
+        model, lv = make_model_and_lv()
+        self.assertEqual(
+            ([], 0),
+            gaps.movable_trailing_partitions_and_gap_size(lv))
+
+    def test_trailing_lvm_logical_volume_with_gap(self):
+        model, disk = make_model_and_disk(
+                size=100 * LVM_CHUNK_SIZE + LVM_OVERHEAD)
+        vg = make_vg(model, pvs={disk})
+        lv = make_lv(model, vg=vg, size=40 * LVM_CHUNK_SIZE)
+        self.assertEqual(
+            ([], LVM_CHUNK_SIZE * 60),
+            gaps.movable_trailing_partitions_and_gap_size(lv))
+
 
 class TestLargestGaps(unittest.TestCase):
     def test_basic(self):

subiquity/models/tests/test_filesystem.py

@@ -205,10 +205,13 @@ def make_model_and_raid(bootloader=None):
     return model, make_raid(model)
 
 
-def make_vg(model):
+def make_vg(model, pvs=None):
     name = 'vg%s' % len(model._actions)
-    return model.add_volgroup(
-        name, {make_disk(model)})
+
+    if pvs is None:
+        pvs = {make_disk(model)}
+
+    return model.add_volgroup(name, pvs)
 
 
 def make_model_and_vg(bootloader=None):
@@ -216,15 +219,17 @@ def make_model_and_vg(bootloader=None):
     return model, make_vg(model)
 
 
-def make_lv(model):
-    vg = make_vg(model)
+def make_lv(model, vg=None, size=None):
+    if vg is None:
+        vg = make_vg(model)
     name = 'lv%s' % len(model._actions)
-    return model.add_logical_volume(vg, name, gaps.largest_gap_size(vg))
+    size = gaps.largest_gap_size(vg) if size is None else size
+    return model.add_logical_volume(vg, name, size)
 
 
-def make_model_and_lv(bootloader=None):
+def make_model_and_lv(bootloader=None, lv_size=None):
     model = make_model(bootloader)
-    return model, make_lv(model)
+    return model, make_lv(model, size=lv_size)
 
 
 class TestFilesystemModel(unittest.TestCase):