curtin-dev team mailing list archive

[Michael Hudson-Doyle <mp+417829@xxxxxxxxxxxxxxxxxx>]

Looking good. But I think not quite here yet.

And there's always more scope for more tests.

Diff comments:

> diff --git a/curtin/commands/block_meta_v2.py b/curtin/commands/block_meta_v2.py
> index 051649b..9870e58 100644
> --- a/curtin/commands/block_meta_v2.py
> +++ b/curtin/commands/block_meta_v2.py
> @@ -214,6 +226,67 @@ def _wipe_for_action(action):
>      return 'superblock'
>  
>  
> +def verify_format(devpath, storage_config, part_action):
> +    actions = [v for v in storage_config.values()
> +               if 'volume' in v and v['volume'] == part_action['id']]
> +    if not actions:
> +        return
> +    fstype = _get_volume_fstype(devpath)
> +    target_fstype = actions[0]['fstype']
> +    msg = (
> +        'Verifying %s format, expecting %s, found %s' % (
> +         devpath, fstype, target_fstype))
> +    LOG.debug(msg)
> +    if fstype != target_fstype:
> +        raise RuntimeError(msg)
> +
> +
> +def partition_verify_sfdisk_v2(part_action, label, sfdisk_part_info,
> +                               storage_config):
> +    devpath = os.path.realpath(sfdisk_part_info['node'])
> +    verify_format(devpath, storage_config, part_action)
> +    if not part_action.get('resize', False):
> +        verify_size(devpath, int(util.human2bytes(part_action['size'])),
> +                    sfdisk_part_info)
> +    expected_flag = part_action.get('flag')
> +    if expected_flag:
> +        verify_ptable_flag(devpath, expected_flag, label, sfdisk_part_info)
> +
> +
> +def prepare_resize(part_action, storage_config):
> +    if not part_action.get('resize', False):
> +        return None
> +
> +    disk_id = part_action['device']
> +    disk_action = storage_config[disk_id]
> +    part_num = part_action['number']
> +    disk_dev = disk_action['dev']
> +    part_dev = f'{disk_dev}p{part_num}'

This isn't going to work in general for a few reasons.  Partition 1 of /dev/sda is /dev/sda1, not /dev/sdap1 for one thing, but more interestingly the number of a logical partition isn't necessarily stable if a preceding (in the order of the ebrs, not necessarily in offset) partition is deleted. I have generally chosen in this code to identify partitions by offset, and I think this can work here.  In fact, we loop over old and new partitions already just before and after the table.apply() call, so I think if you store the offsets of the partitions to resize up and the offsets of the partitions to resize down, you can do the resize2fs calls in those loops.

> +
> +    start_size = get_part_size_bytes(part_dev, part_action)
> +    end_size = util.human2bytes(part_action['size'])
> +
> +    if end_size == start_size:
> +        LOG.debug('Skipping resize of %s - partition is already target size',
> +                  part_dev)
> +        return None
> +
> +    direction = 'up' if start_size < end_size else 'down'
> +
> +    return ResizeOperation(start_size=start_size, end_size=end_size,
> +                           device=part_dev, direction=direction)
> +
> +
> +def process_resizes(resizes, direction):
> +    for op in resizes:
> +        if op is not None and direction == op.direction:
> +            LOG.debug('Resizing %s %s from %s to %s',
> +                      op.device, op.direction, op.start_size, op.end_size)
> +            util.subp(['e2fsck', '-p', '-f', op.device])
> +            end_size_k = op.end_size // 1024
> +            util.subp(['resize2fs', op.device, f'{end_size_k}k'])
> +
> +
>  def disk_handler_v2(info, storage_config, handlers):
>      disk_handler_v1(info, storage_config, handlers)
>  
> diff --git a/doc/topics/storage.rst b/doc/topics/storage.rst
> index 5fae90f..ab89b16 100644
> --- a/doc/topics/storage.rst
> +++ b/doc/topics/storage.rst
> @@ -429,6 +429,16 @@ filesystem or be mounted anywhere on the system.
>  
>  If the preserve flag is set to true, curtin will verify that the partition
>  exists and that  the ``size`` and ``flag`` match the configuration provided.
> +See also the resize flag, which adjust this behavior.
> +
> +**resize**: *true, false*
> +
> +Only applicable to v2 storage configuration.
> +If the preserve flag is set to false, this value is not applicable.
> +If the preserve flag is set to true, curtin will adjust the size of the
> +partition to the new size.  When adjusting smaller, the size of the contents
> +must permit that.  When adjusting larger, there must already be a gap beyond
> +the partition in question.

I'm not sure this last part is true any more is it?

>  
>  **name**: *<name>*
>  
> diff --git a/tests/integration/test_block_meta.py b/tests/integration/test_block_meta.py
> index 0c74cd6..dffb5aa 100644
> --- a/tests/integration/test_block_meta.py
> +++ b/tests/integration/test_block_meta.py
> @@ -415,3 +439,128 @@ class TestBlockMeta(IntegrationTestCase):
>                      )
>          finally:
>              server.stop()
> +
> +    def _do_test_resize(self, start, end):
> +        start <<= 20
> +        end <<= 20
> +        img = self.tmp_path('image.img')
> +        config = StorageConfigBuilder(version=2)
> +        config.add_image(path=img, size='200M', ptable='gpt')
> +        p1 = config.add_part(size=start, offset=1 << 20, number=1)
> +        config.add_format(part=p1)
> +        self.run_bm(config.render())
> +
> +        expected = 'preserve my data!'
> +        with loop_dev(img) as dev:
> +            self.assertEqual(
> +                summarize_partitions(dev), [
> +                    PartData(number=1, offset=1 << 20, size=start),
> +                ])
> +            with self.mount(dev, p1) as mnt_point:
> +                with open(f'{mnt_point}/data.txt', 'w') as fp:
> +                    fp.write(expected)
> +            orig_fs_size = _get_filesystem_size(dev, p1)
> +            self.assertEqual(start, orig_fs_size)
> +
> +        config.set_preserve()
> +        p1['resize'] = True
> +        p1['size'] = end
> +        # self.run_bm(config.render(), fake=True)
> +        self.run_bm(config.render())
> +
> +        with loop_dev(img) as dev:
> +            self.assertEqual(
> +                summarize_partitions(dev), [
> +                    PartData(number=1, offset=1 << 20, size=end),
> +                ])
> +            with self.mount(dev, p1) as mnt_point:
> +                with open(f'{mnt_point}/data.txt', 'r') as fp:
> +                    self.assertEqual(expected, fp.read())
> +            resized_fs_size = _get_filesystem_size(dev, p1)
> +            self.assertEqual(end, resized_fs_size)
> +
> +    def test_resize_up(self):
> +        self._do_test_resize(40, 80)
> +
> +    def test_resize_down(self):
> +        self._do_test_resize(80, 40)
> +
> +    def test_sizes(self):
> +        for size in (1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048):
> +            size <<= 20
> +            img = self.tmp_path('image.img')
> +            config = StorageConfigBuilder(version=2)
> +            config.add_image(path=img, size='2100M', ptable='gpt')
> +            p1 = config.add_part(size=size, offset=1 << 20, number=1)
> +            config.add_format(part=p1)
> +            self.run_bm(config.render())
> +
> +            with loop_dev(img) as dev:
> +                actual = _get_filesystem_size(dev, p1)
> +                self.assertEqual(size, actual)
> +
> +    def test_resizes_up(self):
> +        start = 1 << 20
> +        for end in (2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048):
> +            end <<= 20
> +            img = self.tmp_path('image.img')
> +            config = StorageConfigBuilder(version=2)
> +            config.add_image(path=img, size='2100M', ptable='gpt')
> +            p1 = config.add_part(size=start, offset=1 << 20, number=1)
> +            config.add_format(part=p1)
> +            self.run_bm(config.render())
> +
> +            with loop_dev(img) as dev:
> +                self.assertEqual(
> +                    summarize_partitions(dev), [
> +                        PartData(number=1, offset=1 << 20, size=start),
> +                    ])
> +                actual = _get_filesystem_size(dev, p1)
> +                self.assertEqual(start, actual, 'initial size')
> +
> +            config.set_preserve()
> +            p1['resize'] = True
> +            p1['size'] = end
> +            self.run_bm(config.render())
> +
> +            with loop_dev(img) as dev:
> +                self.assertEqual(
> +                    summarize_partitions(dev), [
> +                        PartData(number=1, offset=1 << 20, size=end),
> +                    ])
> +                actual = _get_filesystem_size(dev, p1)
> +                self.assertEqual(end, actual, 'resized size')
> +
> +    def test_resizes_down(self):
> +        start = 2048 << 20
> +        for end in (128, 256, 512, 1024):
> +            # resize2fs doesn't want to resize smaller than 128 for a 2048
> +            # start
> +            end <<= 20
> +            img = self.tmp_path('image.img')
> +            config = StorageConfigBuilder(version=2)
> +            config.add_image(path=img, size='2100M', ptable='gpt')
> +            p1 = config.add_part(size=start, offset=1 << 20, number=1)
> +            config.add_format(part=p1)
> +            self.run_bm(config.render())
> +
> +            with loop_dev(img) as dev:
> +                self.assertEqual(
> +                    summarize_partitions(dev), [
> +                        PartData(number=1, offset=1 << 20, size=start),
> +                    ])
> +                actual = _get_filesystem_size(dev, p1)
> +                self.assertEqual(start, actual, 'initial size')
> +
> +            config.set_preserve()
> +            p1['resize'] = True
> +            p1['size'] = end
> +            self.run_bm(config.render())
> +
> +            with loop_dev(img) as dev:
> +                self.assertEqual(
> +                    summarize_partitions(dev), [
> +                        PartData(number=1, offset=1 << 20, size=end),
> +                    ])
> +                actual = _get_filesystem_size(dev, p1)
> +                self.assertEqual(end, actual, 'resized size')

As tedious as it's going to be to write, I think some tests that do a mix of partition ops all at once would be valuable. And a test of resizing a logical partition (errr how does resizing an extended partition work I wonder?).



-- 
https://code.launchpad.net/~dbungert/curtin/+git/curtin/+merge/417829
Your team curtin developers is requested to review the proposed merge of ~dbungert/curtin:resize into curtin:master.