sts-sponsors team mailing list archive

[Alberto Donato <mp+442046@xxxxxxxxxxxxxxxxxx>]

Review: Approve

+1

small nits inline

Diff comments:

> diff --git a/src/maasserver/models/partition.py b/src/maasserver/models/partition.py
> index 84a8fba..76515e5 100644
> --- a/src/maasserver/models/partition.py
> +++ b/src/maasserver/models/partition.py
> @@ -304,44 +304,26 @@ class Partition(CleanSave, TimestampedModel):
>                          }
>                      )
>  
> -    def is_vmfs6_partition(self):
> -        # Avoid circular imports.
> -        from maasserver.storage_layouts import VMFS6StorageLayout
> -
> -        vmfs_layout = VMFS6StorageLayout(self.get_node())
> -        vmfs_bd = vmfs_layout.is_layout()
> -        if vmfs_bd is None:
> -            return False
> -        if vmfs_bd.id != self.partition_table.block_device_id:
> -            return False
> -        if self.index >= len(vmfs_layout.base_partitions) + 2:
> -            # A user may apply the VMFS6 layout and leave space at the end of
> -            # the disk for additional VMFS datastores. Those partitions may be
> -            # deleted, the base partitions may not as they are part of the DD.
> -            # The + 2 is to account for partition 4 being skipped.
> -            return False
> -        return True
> -
> -    def is_vmfs7_partition(self):
> +    def is_vmfs_partition(self):
>          # Avoid circular imports.
> -        from maasserver.storage_layouts import VMFS7StorageLayout
> -
> -        vmfs_layout = VMFS7StorageLayout(self.get_node())
> -        vmfs_bd = vmfs_layout.is_layout()
> -        if vmfs_bd is None:
> -            return False
> -        if vmfs_bd.id != self.partition_table.block_device_id:
> -            return False
> -        if self.index < len(vmfs_layout.base_partitions) + 3:
> -            # A user may apply the VMFS7 layout and leave space at the end of
> -            # the disk for additional VMFS datastores. Those partitions may be
> -            # deleted, the base partitions may not as they are part of the DD.
> -            # The + 3 is to account for partition 2-4 being skipped.
> -            return True
> -        return False
> +        from maasserver.storage_layouts import (
> +            VMFS6StorageLayout,
> +            VMFS7StorageLayout,
> +        )
>  
> -    def is_vmfs_partition(self):
> -        return self.is_vmfs6_partition() or self.is_vmfs7_partition()
> +        part_blk_dev_id = self.partition_table.block_device_id
> +
> +        for layout_class in (VMFS7StorageLayout, VMFS6StorageLayout):
> +            vmfs_layout = layout_class(self.get_node())

please get the node once outside of the loop

> +            vmfs_bd = vmfs_layout.is_layout()
> +            if vmfs_bd is None or vmfs_bd.id != part_blk_dev_id:
> +                continue
> +            if self.index <= vmfs_layout.last_base_partition_index:
> +                # A user may apply the VMFS layout and leave space at the end of
> +                # the disk for additional VMFS datastores. Those partitions may be
> +                # deleted, the base partitions may not as they are part of the DD.
> +                return True
> +        return False
>  
>      def delete(self):
>          """Delete the partition.
> diff --git a/src/maasserver/models/tests/test_partition.py b/src/maasserver/models/tests/test_partition.py
> index 4956d8f..2c9f3bb 100644
> --- a/src/maasserver/models/tests/test_partition.py
> +++ b/src/maasserver/models/tests/test_partition.py
> @@ -544,45 +544,26 @@ class TestPartition(MAASServerTestCase):
>              [partition.index for partition in partitions],
>          )
>  
> -    def test_is_vmfs6_partition(self):
> -        node = factory.make_Node(with_boot_disk=False)
> -        bd = factory.make_PhysicalBlockDevice(
> -            node=node, size=LARGE_BLOCK_DEVICE
> -        )
> -        layout = VMFS6StorageLayout(node)
> -        layout.configure()
> -        pt = bd.get_partitiontable()
> -        for partition in pt.partitions.all():
> -            self.assertTrue(partition.is_vmfs6_partition())
> -
> -    def test_is_vmfs7_partition(self):
> -        node = factory.make_Node(with_boot_disk=False)
> -        bd = factory.make_PhysicalBlockDevice(
> -            node=node, size=LARGE_BLOCK_DEVICE
> -        )
> -        layout = VMFS7StorageLayout(node)
> -        layout.configure()
> -        pt = bd.get_partitiontable()
> -        for partition in pt.partitions.all():
> -            if partition.index >= 8:
> -                self.assertFalse(partition.is_vmfs7_partition())
> -            else:
> -                self.assertTrue(partition.is_vmfs7_partition())
> -
>      def test_is_vmfs_partition(self):
>          node = factory.make_Node(with_boot_disk=False)
>          bd = factory.make_PhysicalBlockDevice(
>              node=node, size=LARGE_BLOCK_DEVICE
>          )
> -        vmfs_layout = random.choice([VMFS6StorageLayout, VMFS7StorageLayout])
> -        layout = vmfs_layout(node)
> -        layout_name = layout.configure()
> -        pt = bd.get_partitiontable()
> -        for partition in pt.partitions.all():
> -            if layout_name == "vmfs7" and partition.index >= 8:
> -                self.assertFalse(partition.is_vmfs_partition())
> -            else:
> -                self.assertTrue(partition.is_vmfs_partition())
> +        for vmfs_layout in [VMFS6StorageLayout, VMFS7StorageLayout]:
> +            layout = vmfs_layout(node)
> +            layout_name = layout.configure()
> +            pt = bd.get_partitiontable()
> +            for partition in pt.partitions.all():
> +                if layout_name == "vmfs7" and partition.index >= 8:
> +                    self.assertFalse(
> +                        partition.is_vmfs_partition(),
> +                        f"{layout_name} index {partition.index} is VMFS",
> +                    )
> +                else:
> +                    self.assertTrue(
> +                        partition.is_vmfs_partition(),
> +                        f"{layout_name} index {partition.index} is VMFS",
> +                    )

I'd add two separate tests for each of the layout rather than using conditionals

>  
>      def test_is_vmfs_partition_false_no_vmfs(self):
>          partition = factory.make_Partition()
> diff --git a/src/maasserver/tests/test_storage_layouts.py b/src/maasserver/tests/test_storage_layouts.py
> index 0fd7a94..84128ad 100644
> --- a/src/maasserver/tests/test_storage_layouts.py
> +++ b/src/maasserver/tests/test_storage_layouts.py
> @@ -1992,18 +1992,18 @@ class TestVMFS7StorageLayout(MAASServerTestCase):
>          pt = node.boot_disk.get_partitiontable()
>          self.assertEqual(
>              {
> -                "%s-part1" % node.boot_disk.name: 105 * 1024**2,
> -                "%s-part5" % node.boot_disk.name: 1074 * 1024**2,
> -                "%s-part6" % node.boot_disk.name: 1074 * 1024**2,
> -                "%s-part7" % node.boot_disk.name: 8704 * 1024**2,
> +                "%s-part1" % node.boot_disk.name: 100 * 1024**2,
> +                "%s-part5" % node.boot_disk.name: 4 * 1024**3,
> +                "%s-part6" % node.boot_disk.name: 4 * 1024**3,
> +                "%s-part7" % node.boot_disk.name: 24320 * 1024**2,

can you move these (and ones below to f-strings while at it?

>                  "%s-part8"
>                  % node.boot_disk.name: (
>                      node.boot_disk.size
> -                    - 105 * 1024**2
> -                    - 1074 * 1024**2
> -                    - 1074 * 1024**2
> -                    - 8704 * 1024**2
> -                    - 7 * 1024**2
> +                    - 100 * 1024**2
> +                    - 4 * 1024**3
> +                    - 4 * 1024**3
> +                    - 24320 * 1024**2
> +                    - 8 * 1024**2  # rounding
>                  ),
>              },
>              {part.name: part.size for part in pt.partitions.all()},


-- 
https://code.launchpad.net/~alexsander-souza/maas/+git/maas/+merge/442046
Your team MAAS Committers is subscribed to branch maas:master.