Merge ~dbungert/curtin:lp-2004609-swap-size-dropped into curtin:master

[Dan Bungert]

block/v1: handle msdos+swap

[Dan Bungert (dbungert)]

I needed to revert ebff651 to be able to run the integration tests.

@mwhudson, what's going on with losetup --partscan, I couldn't find mention of a conflict with --sector-size.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:eca1a43b08eb42dd77cb07b5815ca81350af8d4a
https://jenkins.canonical.com/server-team/job/curtin-ci/71/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/71/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/71/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/71/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/71/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/71//rebuild

review: Approve (continuous-integration)

ae78301... by Dan Bungert on 2023-02-06

swap: clarify debug message

This log message is shown, among other scenarios, if a swap partition is
used, which is very confusing.
Clarify that we're talking about swap file.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:418c9c91f51628def58cbcbd6a3247712f12ce21
https://jenkins.canonical.com/server-team/job/curtin-ci/72/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/72/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/72/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/72/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/72/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/72//rebuild

review: Approve (continuous-integration)

[Michael Hudson-Doyle (mwhudson)]

> I needed to revert ebff651 to be able to run the integration tests.

Hmm on which version of Ubuntu? It works for me on kinetic. What goes wrong?

> @mwhudson, what's going on with losetup --partscan, I couldn't find mention of
> a conflict with --sector-size.

losetup with --partscan and --sector-size used to do this:

1) tell the kernel to create a device backed by the supplied file
2) tell the kernel what sector size to use
3) tell the kernel to scan for partitions

At some point, a shiny new ioctl was added that let you do all of this at once but unfortunately losetup was changed to actually do this:

1) tell the kernel to create a device backed by the supplied file and scan for partitions
2) tell the kernel what sector size to use

As you need to know the sector size to interpret the partition table, this doesn't work.

losetup has been fixed to include the sector size in the ioctl to set everything up (https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/commit/?id=854abba0dd9b45b4f40a9c934694b3f14052eceb) but this hasn't been released yet it seems.

[Dan Bungert (dbungert)]

> Hmm on which version of Ubuntu? It works for me on kinetic.

Up-to-date Lunar

> What goes wrong?

5 tests, all msdos ptable but not every msdos ptable test, fail. I don't have to revert the full commit listed above to make them 'pass', but commenting out the partprobe done by loop_dev() is enough to make them pass. Which doesn't give me high confidence.

FAILED tests/integration/test_block_meta.py::TestBlockMeta::test_delete_logical_partition
FAILED tests/integration/test_block_meta.py::TestBlockMeta::test_logical_v2
FAILED tests/integration/test_block_meta.py::TestBlockMeta::test_mix_of_operations_msdos
FAILED tests/integration/test_block_meta.py::TestBlockMeta::test_resize_extended
FAILED tests/integration/test_block_meta.py::TestBlockMeta::test_resize_logical

This is unrelated to this PR, master does so as well. This seems to be in the realm of 'sysfs is giving unexpected results', as sfdisk -J output looks reasonable if run just before the partprobe.

[Michael Hudson-Doyle (mwhudson)]

So the lesson here is that we should use v2 partitioning for everything?

review: Approve

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:ae78301728d0c9e1982da24cf8fd39a8496ebb94
https://jenkins.canonical.com/server-team/job/curtin-ci/78/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/78/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/78/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/78/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/78/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/78//rebuild

review: Approve (continuous-integration)

diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 74c491c..c0e595d 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -1097,9 +1097,11 @@ def partition_handler(info, storage_config, context):
                 partition_type = flag
             else:
                 partition_type = "primary"
-            cmd = ["parted", disk, "--script", "mkpart", partition_type,
-                   "%ss" % offset_sectors, "%ss" % str(offset_sectors +
-                                                       length_sectors)]
+            cmd = ["parted", disk, "--script", "mkpart", partition_type]
+            if flag == 'swap':
+                cmd.append("linux-swap")
+            cmd.append("%ss" % offset_sectors)
+            cmd.append("%ss" % (offset_sectors + length_sectors))
             if flag == 'boot':
                 cmd.extend(['set', str(partnumber), 'boot', 'on'])
 
diff --git a/curtin/swap.py b/curtin/swap.py
index 7a0128d..5aea0b8 100644
--- a/curtin/swap.py
+++ b/curtin/swap.py
@@ -113,7 +113,7 @@ def setup_swapfile(target, fstab=None, swapfile=None, size=None, maxsize=None,
         size = suggested_swapsize(fsys=target, maxsize=maxsize)
 
     if size == 0:
-        LOG.debug("Not creating swap: suggested size was 0")
+        LOG.debug("Not creating swapfile: suggested size was 0")
         return
 
     if swapfile is None:
@@ -130,7 +130,7 @@ def setup_swapfile(target, fstab=None, swapfile=None, size=None, maxsize=None,
         if force:
             LOG.warning('swapfile may not work: %s', err)
         else:
-            LOG.debug('Not creating swap: %s', err)
+            LOG.debug('Not creating swapfile: %s', err)
             return
 
     allocate_cmd = 'fallocate -l "${2}M" "$1"'
@@ -142,7 +142,7 @@ def setup_swapfile(target, fstab=None, swapfile=None, size=None, maxsize=None,
         allocate_cmd = 'dd if=/dev/zero "of=$1" bs=1M "count=$2"'
 
     mbsize = str(int(size / (2 ** 20)))
-    msg = "creating swap file '%s' of %sMB" % (swapfile, mbsize)
+    msg = "creating swapfile '%s' of %sMB" % (swapfile, mbsize)
     fpath = os.path.sep.join([target, swapfile])
     try:
         util.ensure_dir(os.path.dirname(fpath))
diff --git a/tests/integration/test_block_meta.py b/tests/integration/test_block_meta.py
index 38b188a..ead1698 100644
--- a/tests/integration/test_block_meta.py
+++ b/tests/integration/test_block_meta.py
@@ -1230,3 +1230,16 @@ table-length: 256'''.encode()
             self.run_bm(config.render())
         self.assertPartitions(
             PartData(number=1, offset=1 << 20, size=1 << 20))
+
+    @parameterized.expand(((1,), (2,)))
+    def test_swap(self, sv):
+        self.img = self.tmp_path('image.img')
+        config = StorageConfigBuilder(version=sv)
+        config.add_image(path=self.img, create=True, size='20M',
+                         ptable='msdos')
+        config.add_part(number=1, offset=1 << 20, size=1 << 20, flag='swap')
+        self.run_bm(config.render())
+
+        self.assertPartitions(
+            PartData(number=1, offset=1 << 20, size=1 << 20, boot=False,
+                     partition_type='82'))
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 7067bc0..0c198e6 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -654,6 +654,62 @@ class TestBlockMeta(CiTestCase):
                    'mkpart', 'primary', '2048s', '1001471s',
                    'set', '1', 'boot', 'on'], capture=True)])
 
+    def test_partition_handler_creates_swap(self):
+        """ Create a swap partition if the flag has requested as much """
+        self.config = {
+            'storage': {
+                'version': 1,
+                'config': [
+                    {'grub_device': True,
+                     'id': 'sda',
+                     'name': 'sda',
+                     'path': '/wark/xxx',
+                     'ptable': 'msdos',
+                     'type': 'disk',
+                     'wipe': 'superblock'},
+                    {'device': 'sda',
+                     'flag': 'swap',
+                     'id': 'sda-part1',
+                     'name': 'sda-part1',
+                     'number': 1,
+                     'offset': '4194304B',
+                     'size': '511705088B',
+                     'type': 'partition',
+                     'wipe': 'superblock'},
+                    {'id': 'sda1-root',
+                     'type': 'format',
+                     'fstype': 'swap',
+                     'volume': 'sda-part1'},
+                ],
+            }
+        }
+        self.storage_config = (
+            block_meta.extract_storage_ordered_dict(self.config))
+
+        disk_info = self.storage_config.get('sda')
+        part_info = self.storage_config.get('sda-part1')
+        disk_kname = disk_info.get('path')
+        part_kname = disk_kname + '1'
+        self.mock_getpath.side_effect = iter([
+            disk_kname,
+            part_kname,
+        ])
+        self.mock_block_get_part_table_type.return_value = 'dos'
+        kname = 'xxx'
+        self.mock_block_path_to_kname.return_value = kname
+        self.mock_block_sys_block_path.return_value = '/sys/class/block/xxx'
+        self.mock_block_sector_size.return_value = (512, 512)
+
+        block_meta.partition_handler(
+            part_info, self.storage_config, empty_context)
+        part_offset = 2048 * 512
+        self.mock_block_zero_file.assert_called_with(disk_kname, [part_offset],
+                                                     exclusive=False)
+        self.mock_subp.assert_has_calls(
+            [call(['parted', disk_kname, '--script',
+                   'mkpart', 'primary', 'linux-swap', '2048s', '1001471s',
+                   ], capture=True)])
+
     @patch('curtin.util.write_file')
     def test_mount_handler_defaults(self, mock_write_file):
         """Test mount_handler has defaults to 'defaults' for mount options"""