Merge lp:~blake-rouse/maas/fix-mbr-max-partition-size into lp:~maas-committers/maas/trunk

[Blake Rouse]

Don't allow creating partitions larger than 2TiB for MBR partition tables. When creating the LVM storage layout add extra partitions to fill the disks.

[Ricardo Bánffy (rbanffy)]

Looks good. I'd rename a couple tests to make the expected result more explicit (I had to read the code to get what was being tested).

review: Approve

[Blake Rouse (blake-rouse)]

Thanks for the review. I made the suggested changes.

=== modified file 'src/maasserver/models/partition.py'
--- src/maasserver/models/partition.py	2015-09-28 17:43:23 +0000
+++ src/maasserver/models/partition.py	2015-10-07 14:48:54 +0000
@@ -44,6 +44,7 @@
 
 
 MIN_PARTITION_SIZE = MIN_BLOCK_DEVICE_SIZE
+MAX_PARTITION_SIZE_FOR_MBR = 2 * (1024 ** 4)  # 2TiB
 
 
 class PartitionManager(Manager):
@@ -247,6 +248,24 @@
                 else:
                     self.size = adjusted_size
 
+            # Check that the size is not larger than MBR allows.
+            if (self.partition_table.table_type == PARTITION_TABLE_TYPE.MBR and
+                    self.size > MAX_PARTITION_SIZE_FOR_MBR):
+                if self.id is not None:
+                    raise ValidationError({
+                        "size": [
+                            "Partition %s cannot be resized to fit on the "
+                            "block device; size is larger than the MBR "
+                            "2TiB maximum." % (
+                                self.id)],
+                        })
+                else:
+                    raise ValidationError({
+                        "size": [
+                            "Partition cannot be saved; size is larger than "
+                            "the MBR 2TiB maximum."],
+                        })
+
     def delete(self):
         """Delete the partition.
 

=== modified file 'src/maasserver/models/partitiontable.py'
--- src/maasserver/models/partitiontable.py	2015-09-24 16:22:12 +0000
+++ src/maasserver/models/partitiontable.py	2015-10-07 14:48:54 +0000
@@ -29,7 +29,10 @@
 )
 from maasserver.models.blockdevice import BlockDevice
 from maasserver.models.cleansave import CleanSave
-from maasserver.models.partition import Partition
+from maasserver.models.partition import (
+    MAX_PARTITION_SIZE_FOR_MBR,
+    Partition,
+)
 from maasserver.models.timestampedmodel import TimestampedModel
 from maasserver.utils.converters import round_size_to_nearest_block
 
@@ -109,8 +112,9 @@
         """
         if size is None:
             size = self.get_available_size()
-        else:
-            size = round_size_to_nearest_block(size, self.get_block_size())
+            if self.table_type == PARTITION_TABLE_TYPE.MBR:
+                size = min(size, MAX_PARTITION_SIZE_FOR_MBR)
+        size = round_size_to_nearest_block(size, self.get_block_size())
         return Partition.objects.create(
             partition_table=self, size=size, uuid=uuid, bootable=bootable)
 

=== modified file 'src/maasserver/models/tests/test_filesystemgroup.py'
--- src/maasserver/models/tests/test_filesystemgroup.py	2015-09-24 16:22:12 +0000
+++ src/maasserver/models/tests/test_filesystemgroup.py	2015-10-07 14:48:54 +0000
@@ -29,6 +29,7 @@
     FILESYSTEM_GROUP_TYPE,
     FILESYSTEM_TYPE,
     NODE_PERMISSION,
+    PARTITION_TABLE_TYPE,
 )
 from maasserver.models.blockdevice import MIN_BLOCK_DEVICE_SIZE
 from maasserver.models.filesystem import Filesystem
@@ -1927,10 +1928,11 @@
         way as to make the RAID invalid (a 1-device RAID-0/1, a 2-device RAID-5
         etc). The goal is to make sure we trigger the RAID internal validation.
         """
-        node = factory.make_Node()
+        node = factory.make_Node(bios_boot_method="uefi")
         device_size = 10 * 1000 ** 4
         partitions = [
             factory.make_PartitionTable(
+                table_type=PARTITION_TABLE_TYPE.GPT,
                 block_device=factory.make_PhysicalBlockDevice(
                     node=node, size=device_size)).add_partition()
             for _ in range(4)
@@ -1997,10 +1999,11 @@
         self.assertEqual(6 * partition_size, raid.get_size())
 
     def test_remove_invalid_partition_from_array_fails(self):
-        node = factory.make_Node()
+        node = factory.make_Node(bios_boot_method="uefi")
         device_size = 10 * 1000 ** 4
         partitions = [
             factory.make_PartitionTable(
+                table_type=PARTITION_TABLE_TYPE.GPT,
                 block_device=factory.make_PhysicalBlockDevice(
                     node=node, size=device_size)).add_partition()
             for _ in range(10)

=== modified file 'src/maasserver/models/tests/test_partition.py'
--- src/maasserver/models/tests/test_partition.py	2015-09-24 16:22:12 +0000
+++ src/maasserver/models/tests/test_partition.py	2015-10-07 14:48:54 +0000
@@ -243,6 +243,41 @@
                 "block device; not enough free space." % partition.id],
             }, error.error_dict)
 
+    def test_test_cannot_create_mbr_partition_larger_than_2TiB(self):
+        block_device = factory.make_BlockDevice(size=3 * (1024 ** 4))  # 3TiB
+        partition_table = factory.make_PartitionTable(
+            block_device=block_device, table_type=PARTITION_TABLE_TYPE.MBR)
+        error = self.assertRaises(
+            ValidationError, factory.make_Partition,
+            partition_table=partition_table,
+            size=partition_table.get_available_size())
+        self.assertEquals({
+            "size": [
+                "Partition cannot be saved; size is larger than "
+                "the MBR 2TiB maximum."],
+            }, error.error_dict)
+
+    def test_test_cannot_resize_mbr_partition_to_more_than_2TiB(self):
+        block_device = factory.make_BlockDevice(size=3 * (1024 ** 4))  # 3TiB
+        partition_table = factory.make_PartitionTable(
+            block_device=block_device, table_type=PARTITION_TABLE_TYPE.MBR)
+        partition = partition_table.add_partition(size=1 * (1024 ** 4))
+        partition.size = 2.5 * (1024 ** 4)
+        error = self.assertRaises(ValidationError, partition.save)
+        self.assertEquals({
+            "size": [
+                "Partition %s cannot be resized to fit on the "
+                "block device; size is larger than the MBR "
+                "2TiB maximum." % partition.id],
+            }, error.error_dict)
+
+    def test_validate_can_save_gpt_larger_than_2TiB(self):
+        block_device = factory.make_BlockDevice(size=3 * (1024 ** 4))  # 3TiB
+        partition_table = factory.make_PartitionTable(
+            block_device=block_device, table_type=PARTITION_TABLE_TYPE.GPT)
+        # Test is that an error is not raised.
+        partition_table.add_partition()
+
     def test_validate_enough_space_will_round_down_a_block(self):
         partition_table = factory.make_PartitionTable()
         partition = partition_table.add_partition()

=== modified file 'src/maasserver/models/tests/test_partitiontable.py'
--- src/maasserver/models/tests/test_partitiontable.py	2015-08-14 13:48:59 +0000
+++ src/maasserver/models/tests/test_partitiontable.py	2015-10-07 14:48:54 +0000
@@ -17,6 +17,7 @@
 from django.core.exceptions import ValidationError
 from maasserver.enum import PARTITION_TABLE_TYPE
 from maasserver.models.blockdevice import MIN_BLOCK_DEVICE_SIZE
+from maasserver.models.partition import MAX_PARTITION_SIZE_FOR_MBR
 from maasserver.models.partitiontable import PARTITION_TABLE_EXTRA_SPACE
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
@@ -70,6 +71,17 @@
         self.assertEqual(
             partition.size, MIN_BLOCK_DEVICE_SIZE * 2)
 
+    def test_add_partition_no_size_sets_mbr_max(self):
+        block_size = 4096
+        device = factory.make_BlockDevice(
+            size=3 * (1024 ** 4),
+            block_size=block_size)
+        partition_table = factory.make_PartitionTable(
+            table_type=PARTITION_TABLE_TYPE.MBR, block_device=device)
+        partition = partition_table.add_partition()
+        self.assertEqual(
+            partition.size, MAX_PARTITION_SIZE_FOR_MBR)
+
     def test_add_second_partition_no_size(self):
         """Tests whether a second partition with no specified size starts from
         the end of the previous partition and stretches to the end of the

=== modified file 'src/maasserver/storage_layouts.py'
--- src/maasserver/storage_layouts.py	2015-09-24 16:22:12 +0000
+++ src/maasserver/storage_layouts.py	2015-10-07 14:48:54 +0000
@@ -31,6 +31,10 @@
     is_precentage,
 )
 from maasserver.models.cacheset import CacheSet
+from maasserver.models.partition import (
+    MAX_PARTITION_SIZE_FOR_MBR,
+    MIN_PARTITION_SIZE,
+)
 from maasserver.utils.forms import (
     compose_invalid_choice_text,
     set_form_error,
@@ -193,15 +197,29 @@
                 label="boot",
                 mount_point="/boot")
         root_device = self.get_root_device()
+        root_size = self.get_root_size()
         if root_device is None or root_device == self.boot_disk:
+            # Fix the maximum root_size for MBR.
+            max_mbr_size = (
+                MAX_PARTITION_SIZE_FOR_MBR - self.boot_disk.block_size)
+            if (boot_partition_table.table_type == PARTITION_TABLE_TYPE.MBR and
+                    root_size is not None and root_size > max_mbr_size):
+                root_size = max_mbr_size
             root_partition = boot_partition_table.add_partition(
-                size=self.get_root_size())
+                size=root_size)
+            return root_partition, boot_partition_table
         else:
             root_partition_table = PartitionTable.objects.create(
                 block_device=root_device)
+            # Fix the maximum root_size for MBR.
+            max_mbr_size = (
+                MAX_PARTITION_SIZE_FOR_MBR - root_device.block_size)
+            if (root_partition_table.table_type == PARTITION_TABLE_TYPE.MBR and
+                    root_size is not None and root_size > max_mbr_size):
+                root_size = max_mbr_size
             root_partition = root_partition_table.add_partition(
-                size=self.get_root_size())
-        return root_partition
+                size=root_size)
+            return root_partition, root_partition_table
 
     def configure(self, allow_fallback=True):
         """Configure the storage for the node."""
@@ -231,7 +249,7 @@
         """Create the flat configuration."""
         # Circular imports.
         from maasserver.models.filesystem import Filesystem
-        root_partition = self.create_basic_layout()
+        root_partition, _ = self.create_basic_layout()
         Filesystem.objects.create(
             partition=root_partition,
             fstype=FILESYSTEM_TYPE.EXT4,
@@ -320,9 +338,20 @@
         # Circular imports.
         from maasserver.models.filesystem import Filesystem
         from maasserver.models.filesystemgroup import VolumeGroup
-        root_partition = self.create_basic_layout()
+        root_partition, root_partition_table = self.create_basic_layout()
+
+        # Add extra partitions if MBR and extra space.
+        partitions = [root_partition]
+        if root_partition_table.table_type == PARTITION_TABLE_TYPE.MBR:
+            available_size = root_partition_table.get_available_size()
+            while available_size > MIN_PARTITION_SIZE:
+                part = root_partition_table.add_partition()
+                partitions.append(part)
+                available_size -= part.size
+
+        # Create the volume group and logical volume.
         volume_group = VolumeGroup.objects.create_volume_group(
-            self.get_vg_name(), block_devices=[], partitions=[root_partition])
+            self.get_vg_name(), block_devices=[], partitions=partitions)
         logical_volume = volume_group.create_logical_volume(
             self.get_lv_name(), self.get_calculated_lv_size(volume_group))
         Filesystem.objects.create(
@@ -488,7 +517,7 @@
         boot_size = self.get_boot_size()
         if boot_size == 0:
             boot_size = 1 * 1024 ** 3
-        root_partition = self.create_basic_layout(boot_size=boot_size)
+        root_partition, _ = self.create_basic_layout(boot_size=boot_size)
         cache_set = self.create_cache_set()
         bcache = Bcache.objects.create_bcache(
             cache_mode=self.get_cache_mode(), cache_set=cache_set,

=== modified file 'src/maasserver/tests/test_storage_layouts.py'
--- src/maasserver/tests/test_storage_layouts.py	2015-09-24 16:22:12 +0000
+++ src/maasserver/tests/test_storage_layouts.py	2015-10-07 14:48:54 +0000
@@ -25,6 +25,7 @@
 )
 from maasserver.models.blockdevice import MIN_BLOCK_DEVICE_SIZE
 from maasserver.models.filesystemgroup import VolumeGroup
+from maasserver.models.partition import MAX_PARTITION_SIZE_FOR_MBR
 from maasserver.models.partitiontable import PARTITION_TABLE_EXTRA_SPACE
 from maasserver.storage_layouts import (
     BcacheStorageLayout,
@@ -484,6 +485,34 @@
                 mount_point="/",
                 ))
 
+    def test__creates_layout_with_maximum_mbr_partition_size(self):
+        node = factory.make_Node()
+        boot_disk = factory.make_PhysicalBlockDevice(
+            node=node, size=3 * (1024 ** 4))
+        layout = FlatStorageLayout(node)
+        layout.configure()
+
+        # Validate partition table.
+        partition_table = boot_disk.get_partitiontable()
+        self.assertEquals(PARTITION_TABLE_TYPE.MBR, partition_table.table_type)
+
+        # Validate root partition.
+        partitions = partition_table.partitions.order_by('id').all()
+        root_partition = partitions[0]
+        self.assertIsNotNone(root_partition)
+        self.assertEquals(
+            round_size_by_blocks(
+                MAX_PARTITION_SIZE_FOR_MBR,
+                boot_disk.block_size),
+            root_partition.size)
+        self.assertThat(
+            root_partition.get_effective_filesystem(),
+            MatchesStructure.byEquality(
+                fstype=FILESYSTEM_TYPE.EXT4,
+                label="root",
+                mount_point="/",
+                ))
+
     def test__creates_layout_with_uefi_defaults(self):
         node = make_Node_with_uefi_boot_method()
         boot_disk = factory.make_PhysicalBlockDevice(
@@ -961,6 +990,25 @@
                 mount_point="/",
                 ))
 
+    def test__creates_layout_with_multiple_mbr_partitions(self):
+        node = factory.make_Node()
+        boot_disk = factory.make_PhysicalBlockDevice(
+            node=node, size=7 * (1024 ** 4))
+        layout = LVMStorageLayout(node)
+        layout.configure()
+
+        # Validate the volume group on root partition.
+        partition_table = boot_disk.get_partitiontable()
+        partitions = partition_table.partitions.order_by('id').all()
+        root_partition = partitions[0]
+        volume_group = VolumeGroup.objects.get(
+            filesystems__partition=root_partition)
+        self.assertIsNotNone(volume_group)
+        self.assertEquals(
+            4, partition_table.partitions.count(),
+            "Should have 4 partitions.")
+        self.assertEquals(MAX_PARTITION_SIZE_FOR_MBR, root_partition.size)
+
 
 class TestBcacheStorageLayoutBase(MAASServerTestCase):