From 2b18cc2389e7cf79d21ccd359b00822d536b11e9 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Wed, 19 Feb 2025 12:48:06 +0100 Subject: [PATCH 1/3] snapcraft: update curtin revision to show GPT partition name Full commit log: * 67b539b9 block: prevent ntfsresize from filling the logs with progress info * 6c2ef700 storage_config: use ID_PART_ENTRY_NAME instead of PARTNAME * 3d29eb7a storage-config: read the (GPT) partition name if present * 0c79fe0a Merge remote-tracking branch 'sjg1/boot2' * f5e93cf8 Add initial support for extlinux bootloader * c91f9519 Adjust get_kernel_list() ordering * 87c9c905 Create a function to find kernels * 374655e1 Update tests to use the top-level boot key * 8be682f6 Introduce a new boot builtin hook * 61c3d646 Move grub config above curthooks * 099421b6 Move handling of grub_install_devices to setup_boot() * a7c3cd3a Add a new setup_boot() function * 704bb1a1 Drop parser for install_grub * 5529602d Rename grubcfg to bootcfg * 234f203b Add a table-of-contents in the configuration page * d5d8e542 Fix a few problems in the storage docs * 8bde61c7 Add doc/_build to the .gitignore list * 0e1110de storage-config: drop visible-on-ptable, unset ptable when relevant Signed-off-by: Olivier Gayot --- snapcraft.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/snapcraft.yaml b/snapcraft.yaml index fa7d4484a..4cf158536 100644 --- a/snapcraft.yaml +++ b/snapcraft.yaml @@ -80,7 +80,7 @@ parts: source: https://git.launchpad.net/curtin source-type: git - source-commit: "05217835c0222d4b4b84220b33a0a839549009b0" + source-commit: "67b539b9c448e1b2751b01fee8a854b4c369685f" override-pull: | craftctl default From 7ab7026c159c69a3c11d945616f74435728b342c Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Wed, 19 Feb 2025 15:58:44 +0100 Subject: [PATCH 2/3] storage: fix edit_partition_POST tests The edit_partition_POST tests mocked the return value of FilesystemController.get_partition() with a Partition object. However, it was meant to be a Partition fsobj, not a Partition object used by the API. Signed-off-by: Olivier Gayot --- .../server/controllers/tests/test_filesystem.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index 91c857b7f..55fc2e741 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/subiquity/server/controllers/tests/test_filesystem.py @@ -452,11 +452,12 @@ async def test_v2_delete_partition_POST(self): async def test_v2_edit_partition_POST_change_boot(self): self.fsc.locked_probe_data = False + self.fsc.model, d = make_model_and_disk() data = ModifyPartitionV2( - disk_id="dev-sda", + disk_id=d.id, partition=Partition(number=1, boot=True), ) - existing = Partition(number=1, size=1000 << 20, boot=False) + existing = make_partition(self.fsc.model, d, size=1000 << 20) with mock.patch.object(self.fsc, "partition_disk_handler") as handler: with mock.patch.object(self.fsc, "get_partition", return_value=existing): with self.assertRaisesRegex(ValueError, r"changing\ boot"): @@ -466,12 +467,13 @@ async def test_v2_edit_partition_POST_change_boot(self): async def test_v2_edit_partition_POST_unsupported_ptable(self): self.fsc.locked_probe_data = False - self.fsc.model, d = make_model_and_disk(ptable="unsupported") + self.fsc.model, d = make_model_and_disk() data = ModifyPartitionV2( disk_id=d.id, partition=Partition(number=1, boot=True), ) - existing = Partition(number=1, size=1000 << 20, boot=False) + existing = make_partition(self.fsc.model, d, size=1000 << 20) + d.ptable = "unsupported" with mock.patch.object(self.fsc, "partition_disk_handler") as handler: with mock.patch.object(self.fsc, "get_partition", return_value=existing): with self.assertRaisesRegex( @@ -483,11 +485,12 @@ async def test_v2_edit_partition_POST_unsupported_ptable(self): async def test_v2_edit_partition_POST(self): self.fsc.locked_probe_data = False + self.fsc.model, d = make_model_and_disk() data = ModifyPartitionV2( - disk_id="dev-sda", + disk_id=d.id, partition=Partition(number=1), ) - existing = Partition(number=1, size=1000 << 20, boot=False) + existing = make_partition(self.fsc.model, d, size=1000 << 20) with mock.patch.object(self.fsc, "partition_disk_handler") as handler: with mock.patch.object(self.fsc, "get_partition", return_value=existing): await self.fsc.v2_edit_partition_POST(data) From 49719560925561db2d2a7a29e2ff7015d4bed9bf Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Wed, 19 Feb 2025 12:42:50 +0100 Subject: [PATCH 3/3] storage: add (GPT) partition name in the API but don't allow to set it The /storage/v2 endpoint will now return a "name" field for each partition. It will either be a string or null if the partition does not have a name. Currently, the name property of a partition is read-only. It cannot be changed using the API. The request handler for POST /storage/v2/edit_partition, one can omit the "name" property entirely or repeat the value that the partition already had. Examples: * Partition(number=1, name=None) * Partition(number=2, name="Microsoft Reserved Partition") POST /storage/v2/edit_partition > {..., "partition": {"number": 1, ...}} -> OK (not trying to change the name) POST /storage/v2/edit_partition > {..., "partition": {"number": 2, ...}} -> OK (not trying to change the name) POST /storage/v2/edit_partition > {..., "partition": {"number": 1, "name": null, ...}} -> OK (value of name hasn't changed) POST /storage/v2/edit_partition > {..., "partition": {"number": 1, "name": "something", ...}} -> KO (trying to change the value to "something") LP: #2099893 Signed-off-by: Olivier Gayot --- subiquity/common/filesystem/labels.py | 1 + .../common/filesystem/tests/test_labels.py | 12 ++++++ subiquity/common/types/storage.py | 3 ++ subiquity/models/filesystem.py | 4 ++ subiquity/server/controllers/filesystem.py | 23 +++++++++++ .../controllers/tests/test_filesystem.py | 39 +++++++++++++++++++ subiquity/tests/api/test_api.py | 25 ++++++++++++ 7 files changed, 107 insertions(+) diff --git a/subiquity/common/filesystem/labels.py b/subiquity/common/filesystem/labels.py index 47d7e6cd7..e388888e4 100644 --- a/subiquity/common/filesystem/labels.py +++ b/subiquity/common/filesystem/labels.py @@ -352,6 +352,7 @@ def _for_client_partition(partition, *, min_size=0): offset=partition.offset, resize=partition.resize, path=partition._path(), + name=partition.partition_name, estimated_min_size=partition.estimated_min_size, mount=partition.mount, format=partition.format, diff --git a/subiquity/common/filesystem/tests/test_labels.py b/subiquity/common/filesystem/tests/test_labels.py index 6081b7cff..b670d66ba 100644 --- a/subiquity/common/filesystem/tests/test_labels.py +++ b/subiquity/common/filesystem/tests/test_labels.py @@ -131,3 +131,15 @@ def test_for_client_disk_supported_ptable(self): def test_for_client_disk_unsupported_ptable(self): _, disk = make_model_and_disk(ptable="unsupported") self.assertTrue(for_client(disk).requires_reformat) + + def test_for_client_partition_no_name(self): + model, disk = make_model_and_disk(ptable="gpt") + part = make_partition(model, disk, partition_name=None) + + self.assertIsNone(for_client(part).name) + + def test_for_client_partition_with_name(self): + model, disk = make_model_and_disk(ptable="gpt") + part = make_partition(model, disk, partition_name="Foobar") + + self.assertEqual("Foobar", for_client(part).name) diff --git a/subiquity/common/types/storage.py b/subiquity/common/types/storage.py index 586d9ebaf..92b69c70a 100644 --- a/subiquity/common/types/storage.py +++ b/subiquity/common/types/storage.py @@ -65,6 +65,9 @@ class Partition: estimated_min_size: Optional[int] = -1 resize: Optional[bool] = None path: Optional[str] = None + # Be careful, this corresponds to the partition_name field (not the name + # field) in the associated fsobject + name: Optional[str] = None # Was this partition mounted when the installer started? is_in_use: bool = False diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index de61f53ec..e13b10b4d 100644 --- a/subiquity/models/filesystem.py +++ b/subiquity/models/filesystem.py @@ -916,11 +916,15 @@ class Partition(_Formattable): number: Optional[int] = None preserve: bool = False grub_device: bool = False + # This field controls the existence of a /dev/disk/by-dname/ symlink. + # Not to be confused with "partition_name" below. name: Optional[str] = None multipath: Optional[str] = None offset: Optional[int] = None resize: Optional[bool] = None partition_type: Optional[str] = None + # This is the actual name of the partition, if supported. It maps to the + # "name" field in the Partition API object. partition_name: Optional[str] = None path: Optional[str] = None uuid: Optional[str] = None diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index f57277fb3..2e69dd6f6 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -1455,6 +1455,10 @@ async def v2_add_partition_POST(self, data: AddPartitionV2) -> StorageResponseV2 spec["fstype"] = fstype if data.partition.mount is not None: spec["mount"] = data.partition.mount + if data.partition.name is not None: + raise StorageRecoverableError( + "setting the partition name is not implemented" + ) gap = gaps.at_offset(disk, data.gap.offset).split(requested_size)[0] self.create_partition(disk, gap, spec, wipe="superblock") @@ -1492,6 +1496,25 @@ async def v2_edit_partition_POST( raise ValueError("edit_partition does not support changing size") if data.partition.boot is not None and data.partition.boot != partition.boot: raise ValueError("edit_partition does not support changing boot") + if data.partition.name != partition.name: + if data.partition is None: + # FIXME Instead of checking if data.partition.name is None, + # what we really want to know is whether the name field is + # present in the request. Unfortunately, None is the default + # value so there is no easy way to make the distinction between + # these two scenarios: + # 1. No intention to change the partition name: + # {"partition": {"number": 1, ...} + # 2. Attemping to reset the partition name: + # {"partition": {"number": 1, "name": null, ...} + log.warning( + "cannot tell if the user wants to keep the current" + " partition name or reset it ; assuming they want to keep it" + ) + else: + raise ValueError( + "edit_partition does not support changing partition name" + ) spec: PartitionSpec = {"mount": data.partition.mount or partition.mount} if data.partition.format is not None: if data.partition.format != partition.original_fstype(): diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index 55fc2e741..bb67c0224 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/subiquity/server/controllers/tests/test_filesystem.py @@ -377,6 +377,29 @@ async def test_v2_add_partition_POST_too_large(self): self.assertTrue(self.fsc.locked_probe_data) create_part.assert_not_called() + async def test_v2_add_partition_POST_change_pname(self): + self.fsc.locked_probe_data = False + data = AddPartitionV2( + disk_id="dev-sda", + partition=Partition( + format="ext4", + mount="/", + name="Foobar", + ), + gap=Gap( + offset=1 << 20, + size=1000 << 20, + usable=GapUsable.YES, + ), + ) + with mock.patch.object(self.fsc, "create_partition") as create_part: + with self.assertRaisesRegex( + StorageRecoverableError, r"partition name is not implemented" + ): + await self.fsc.v2_add_partition_POST(data) + self.assertTrue(self.fsc.locked_probe_data) + create_part.assert_not_called() + async def test_v2_add_partition_POST_unsupported_ptable(self): self.fsc.locked_probe_data = False self.fsc.model, d = make_model_and_disk(ptable="unsupported") @@ -465,6 +488,22 @@ async def test_v2_edit_partition_POST_change_boot(self): self.assertTrue(self.fsc.locked_probe_data) handler.assert_not_called() + async def test_v2_edit_partition_POST_change_pname(self): + self.fsc.locked_probe_data = False + self.fsc.model, d = make_model_and_disk() + data = ModifyPartitionV2( + disk_id=d.id, + partition=Partition(number=1, name="Foobar"), + ) + + existing = make_partition(self.fsc.model, d, size=1000 << 20) + with mock.patch.object(self.fsc, "partition_disk_handler") as handler: + with mock.patch.object(self.fsc, "get_partition", return_value=existing): + with self.assertRaisesRegex(ValueError, r"changing partition name"): + await self.fsc.v2_edit_partition_POST(data) + self.assertTrue(self.fsc.locked_probe_data) + handler.assert_not_called() + async def test_v2_edit_partition_POST_unsupported_ptable(self): self.fsc.locked_probe_data = False self.fsc.model, d = make_model_and_disk() diff --git a/subiquity/tests/api/test_api.py b/subiquity/tests/api/test_api.py index 3f37b6494..2eff8578c 100644 --- a/subiquity/tests/api/test_api.py +++ b/subiquity/tests/api/test_api.py @@ -905,6 +905,31 @@ async def test_edit_no_change_grub(self): with self.assertRaises(ClientResponseError): await inst.post("/storage/v2/edit_partition", data) + @timeout() + async def test_edit_no_change_pname(self): + async with start_server("examples/machines/win11-along-ubuntu.json") as inst: + disk_id = "disk-sda" + resp = await inst.get("/storage/v2") + + [sda] = match(resp["disks"], id=disk_id) + [sda2] = match(sda["partitions"], number=2) + + self.assertIsNotNone(sda2["name"]) + + # This should be a no-op since "name" is not present. + data = { + "disk_id": disk_id, + "partition": { + "number": 2, + }, + } + await inst.post("/storage/v2/edit_partition", data) + + # Now, it should refuse the update + data["partition"]["name"] = "foo" + with self.assertRaises(ClientResponseError): + await inst.post("/storage/v2/edit_partition", data) + @timeout() async def test_edit_format(self): async with start_server("examples/machines/win10.json") as inst: