From 8c5f293c28edefaccb4822b1ff3b996429b8b6bd Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Mon, 23 Sep 2024 21:10:47 +0200 Subject: [PATCH] storage: when handling a partition, ignore any FS at the disk level The parts_and_gaps function returned no partition nor gap when the disk itself has (or had) a file-system signature. This is to make sure we don't partition a disk that is already used unpartioned. However, when we are operating on an existing partition of a disk, we should not pretend that the underlying disk has no partition ; even if the disk itself has a FS signature. This leads to a common issue when doing GuidedTargetReformat: Traceback (most recent call last): File "subiquity/common/api/server.py", line 164, in handler result = await implementation(**args) File "subiquity/server/controllers/filesystem.py", line 1184, in v2_guided_POST await self.guided(data) File "subiquity/server/controllers/filesystem.py", line 726, in guided gap = self.start_guided(choice.target, disk) File "/usr/lib/python3.10/functools.py", line 926, in _method return method.__get__(obj, cls)(*args, **kwargs) File "subiquity/server/controllers/filesystem.py", line 645, in start_guided_reformat self.reformat(disk, wipe="superblock-recursive") File "subiquity/common/filesystem/manipulator.py", line 257, in reformat self.delete_partition(p, True) File "subiquity/common/filesystem/manipulator.py", line 120, in delete_partition self.model.remove_partition(part) File "subiquity/models/filesystem.py", line 2110, in remove_partition for p2 in movable_trailing_partitions_and_gap_size(part)[0]: File "/usr/lib/python3.10/functools.py", line 889, in wrapper return dispatch(args[0].__class__)(*args, **kw) File "subiquity/common/filesystem/gaps.py", line 281, in _movable_trailing_partitions_and_gap_size_partition part_idx = pgs.index(partition) ValueError: Partition(device=disk-sda, ...) is not in list LP: #2081738 Signed-off-by: Olivier Gayot --- subiquity/common/filesystem/gaps.py | 10 +++---- .../common/filesystem/tests/test_gaps.py | 28 +++++++++++++++++-- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/subiquity/common/filesystem/gaps.py b/subiquity/common/filesystem/gaps.py index 3d9defe15..56b3645a9 100644 --- a/subiquity/common/filesystem/gaps.py +++ b/subiquity/common/filesystem/gaps.py @@ -88,7 +88,7 @@ def within(self): @functools.singledispatch -def parts_and_gaps(device): +def parts_and_gaps(device, ignore_disk_fs=False): raise NotImplementedError(device) @@ -191,8 +191,8 @@ def maybe_add_gap(start, end, in_extended): @parts_and_gaps.register(Disk) @parts_and_gaps.register(Raid) -def parts_and_gaps_disk(device): - if device._fs is not None: +def parts_and_gaps_disk(device, ignore_disk_fs=False): + if device._fs is not None and not ignore_disk_fs: return [] if device._m.storage_version == 1: return find_disk_gaps_v1(device) @@ -201,7 +201,7 @@ def parts_and_gaps_disk(device): @parts_and_gaps.register(LVM_VolGroup) -def _parts_and_gaps_vg(device): +def _parts_and_gaps_vg(device, ignore_disk_fs=False): used = 0 r = [] for lv in device._partitions: @@ -277,7 +277,7 @@ def movable_trailing_partitions_and_gap_size(partition): def _movable_trailing_partitions_and_gap_size_partition( partition: Partition, ) -> Tuple[List[Partition], int]: - pgs = parts_and_gaps(partition.device) + pgs = parts_and_gaps(partition.device, ignore_disk_fs=True) part_idx = pgs.index(partition) trailing_partitions = [] in_extended = partition.is_logical diff --git a/subiquity/common/filesystem/tests/test_gaps.py b/subiquity/common/filesystem/tests/test_gaps.py index 381c7ae18..00c11be90 100644 --- a/subiquity/common/filesystem/tests/test_gaps.py +++ b/subiquity/common/filesystem/tests/test_gaps.py @@ -14,7 +14,6 @@ # along with this program. If not, see . import unittest -from functools import partial from unittest import mock from subiquity.common.filesystem import gaps @@ -44,7 +43,11 @@ def use_alignment_data(self, alignment_data): m = mock.patch("subiquity.common.filesystem.gaps.parts_and_gaps") p = m.start() self.addCleanup(m.stop) - p.side_effect = partial(gaps.find_disk_gaps_v2, info=alignment_data) + + def fake_parts_and_gaps(device, ignore_disk_fs=False): + return gaps.find_disk_gaps_v2(device) + + p.side_effect = fake_parts_and_gaps for cls in Disk, Raid: md = mock.patch.object(cls, "alignment_data") @@ -59,6 +62,27 @@ def test_basic(self): self.assertTrue(isinstance(gap, gaps.Gap)) self.assertEqual(MiB, gap.offset) + def test_disk_with_filesystem(self): + disk = make_disk() + disk._fs = mock.Mock() + + self.assertFalse(gaps.parts_and_gaps(disk)) + [gap] = gaps.parts_and_gaps(disk, ignore_disk_fs=True) + self.assertTrue(isinstance(gap, gaps.Gap)) + self.assertEqual(MiB, gap.offset) + + def test_disk_with_filesystem_and_partitions(self): + model, disk = make_model_and_disk() + make_partition(model, disk, size=20 * MiB) + + disk._fs = mock.Mock() + + self.assertFalse(gaps.parts_and_gaps(disk)) + [part, gap] = gaps.parts_and_gaps(disk, ignore_disk_fs=True) + self.assertTrue(isinstance(part, gaps.Partition)) + self.assertTrue(isinstance(gap, gaps.Gap)) + self.assertEqual(MiB + 20 * MiB, gap.offset) + class TestSplitGap(GapTestCase): def test_equal(self):