Merge pull request #2169 from ogayot/partition+pname

Add read-only access to GPT partition name to /storage/v2 API

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

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,

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)

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
 

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/<name> 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

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():

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")
@@ -452,26 +475,44 @@ 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"):
                     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_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(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 +524,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)

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: