[PATCH 6/6] Require resize target sizes to yield aligned partitions. (#1120964)
Anne Mulhern
amulhern at redhat.com
Wed Oct 29 12:58:18 UTC 2014
----- Original Message -----
From: "David Lehman" <dlehman at redhat.com>
To: anaconda-patches at lists.fedorahosted.org
Sent: Tuesday, October 28, 2014 3:20:03 PM
Subject: [PATCH 6/6] Require resize target sizes to yield aligned partitions. (#1120964)
New method StorageDevice.alignTargetSize can be used to modify target
sizes as needed to account for alignment.
Modify minSize and maxSize to return values that will yield an aligned
partition.
---
blivet/devices/partition.py | 47 ++++++++++-
blivet/devices/storage.py | 17 +++-
tests/devices_test.py | 193 ++++++++++++++++++++++++++++++++++++++++++++
tests/storagetestcase.py | 2 +
4 files changed, 255 insertions(+), 4 deletions(-)
diff --git a/blivet/devices/partition.py b/blivet/devices/partition.py
index d8e5de8..0c130f8 100644
--- a/blivet/devices/partition.py
+++ b/blivet/devices/partition.py
@@ -224,11 +224,36 @@ class PartitionDevice(StorageDevice):
"flags": self.partedPartition.getFlagsAsString()})
return d
+ def alignTargetSize(self, newsize):
+ """ Return newsize adjusted to allow for an end-aligned partition.
+
+ :param :class:`~.Size` newsize: proposed/unaligned target size
+ :raises _ped.CreateException: if the size extends beyond the end of
+ the disk
+ :returns: newsize modified to yield an end-aligned partition
+ :rtype: :class:`~.Size`
+ """
+ if newsize == 0:
!!! More proper to do comparison with Size(0) here.
+ return newsize
+
+ (_constraint, geometry) = self._computeResize(self.partedPartition,
+ newsize=newsize)
+ return Size(geometry.getLength(unit="B"))
+
def _setTargetSize(self, newsize):
if not isinstance(newsize, Size):
raise ValueError("new size must of type Size")
if newsize != self.size:
+ try:
+ aligned = self.alignTargetSize(newsize)
+ except _ped.CreateException:
+ # this gets handled in superclass setter, below
+ aligned = newsize
+
+ if aligned != newsize:
+ raise ValueError("new size will not yield an aligned partition")
+
# change this partition's geometry in-memory so that other
# partitioning operations can complete (e.g., autopart)
super(PartitionDevice, self)._setTargetSize(newsize)
@@ -748,16 +773,34 @@ class PartitionDevice(StorageDevice):
pass
else:
if partition.type == parted.PARTITION_FREESPACE:
- maxPartSize = self.size + Size(partition.getLength(unit="B"))
+ maxPartSize += Size(partition.getLength(unit="B"))
return maxPartSize
@property
+ def minSize(self):
+ min_size = super(PartitionDevice, self).minSize
+ if self.resizable and min_size:
+ # Adjust the min size as needed so that aligning the end sector
+ # won't drive the actual size below the formatting's minimum.
+ # align the end sector (up, if possible)
+ aligned = self.alignTargetSize(min_size)
+ if aligned < min_size:
+ # If it aligned down, that must mean it cannot align up. Just
+ # return our current size.
+ current = self.currentSize
!!! The line above and the variable current are unnecessary, can just set min_size to
self.currentSize below.
!!! OR use current in debug message (probably original idea).
+ log.debug("failed to align min size up; returning current size")
+ min_size = current
+
+ return min_size
+
+ @property
def maxSize(self):
""" The maximum size this partition can be. """
maxPartSize = self._unalignedMaxPartSize
maxFormatSize = self.format.maxSize
- return min(maxFormatSize, maxPartSize) if maxFormatSize else maxPartSize
+ unalignedMax = min(maxFormatSize, maxPartSize) if maxFormatSize else maxPartSize
+ return self.alignTargetSize(unalignedMax)
!!! Should both candidates be aligned and THEN the values compared?
@property
def currentSize(self):
diff --git a/blivet/devices/storage.py b/blivet/devices/storage.py
index 531ab8c..991b3d4 100644
--- a/blivet/devices/storage.py
+++ b/blivet/devices/storage.py
@@ -238,6 +238,16 @@ class StorageDevice(Device):
super(StorageDevice, self)._setName(value)
+ def alignTargetSize(self, newsize):
+ """ Return a proposed target size adjusted for device specifics.
+
+ :param :class:`~.Size` newsize: the proposed/unaligned target size
+ :returns: newsize modified to yield an aligned device
+ :rtype: :class:`~.Size`
+ """
+
+ return newsize
+
def _getTargetSize(self):
return self._targetSize
@@ -254,6 +264,9 @@ class StorageDevice(Device):
newsize, self.minSize)
raise ValueError("size is smaller than the minimum for this device")
+ if self.alignTargetSize(newsize) != newsize:
+ raise ValueError("new size would violate alignment requirements")
+
self._targetSize = newsize
targetSize = property(lambda s: s._getTargetSize(),
@@ -586,12 +599,12 @@ class StorageDevice(Device):
@property
def minSize(self):
""" The minimum size this device can be. """
- return self.format.minSize if self.resizable else self.currentSize
+ return self.alignTargetSize(self.format.minSize) if self.resizable else self.currentSize
@property
def maxSize(self):
""" The maximum size this device can be. """
- return self.format.maxSize if self.resizable else self.currentSize
+ return self.alignTargetSize(self.format.maxSize) if self.resizable else self.currentSize
@property
def status(self):
diff --git a/tests/devices_test.py b/tests/devices_test.py
index 7221b62..b039774 100644
--- a/tests/devices_test.py
+++ b/tests/devices_test.py
@@ -1,6 +1,7 @@
#!/usr/bin/python
# vim:set fileencoding=utf-8
+import os
import unittest
from mock import Mock
@@ -14,6 +15,7 @@ from blivet.devices import BTRFSSnapShotDevice
from blivet.devices import BTRFSSubVolumeDevice
from blivet.devices import BTRFSVolumeDevice
from blivet.devices import DiskDevice
+from blivet.devices import DiskFile
from blivet.devices import LVMLogicalVolumeDevice
from blivet.devices import LVMSnapShotDevice
from blivet.devices import LVMThinPoolDevice
@@ -22,11 +24,13 @@ from blivet.devices import LVMThinSnapShotDevice
from blivet.devices import LVMVolumeGroupDevice
from blivet.devices import MDRaidArrayDevice
from blivet.devices import OpticalDevice
+from blivet.devices import PartitionDevice
from blivet.devices import StorageDevice
from blivet.devices import ParentList
from blivet.devicelibs import btrfs
from blivet.devicelibs import mdraid
from blivet.size import Size
+from blivet.util import sparsetmpfile
from blivet.formats import getFormat
@@ -770,6 +774,195 @@ class LVMDeviceTest(unittest.TestCase):
snap1.exists = True
self.assertEqual(snap1.dependsOn(thinlv), False)
+ def testTargetSize(self):
+ pv = StorageDevice("pv1", fmt=blivet.formats.getFormat("lvmpv"),
+ size=Size("1 GiB"))
+ vg = LVMVolumeGroupDevice("testvg", parents=[pv])
+ orig_size = Size("800 MiB")
+ lv = LVMLogicalVolumeDevice("testlv", parents=[vg], size=orig_size,
+ fmt=blivet.formats.getFormat("ext4"),
+ exists=True)
+
+ min_size = Size("200 MiB")
+ lv.format.exists = True
+ lv.format._minInstanceSize = min_size
+
+ # Make sure things are as expected to begin with.
+ self.assertEqual(lv.minSize, min_size)
+ self.assertEqual(lv.maxSize, Size("1020 MiB"))
+ self.assertEqual(lv.size, orig_size)
+
+ # ValueError if size smaller than minSize
+ with self.assertRaisesRegexp(ValueError,
+ "size.*smaller than the minimum"):
+ lv.targetSize = Size("1 MiB")
+
+ # target size should be unchanged
+ self.assertEqual(lv.targetSize, orig_size)
+
+ # ValueError if size larger than maxSize
+ with self.assertRaisesRegexp(ValueError,
+ "size.*larger than the maximum"):
+ lv.targetSize = Size("1 GiB")
+
+ # target size should be unchanged
+ self.assertEqual(lv.targetSize, orig_size)
+
+ # successful set of target size should also be reflected in size attr
+ new_target = Size("900 MiB")
+ lv.targetSize = new_target
+ self.assertEqual(lv.targetSize, new_target)
+ self.assertEqual(lv.size, new_target)
+
+ # reset target size to original size
+ lv.targetSize = orig_size
+ self.assertEqual(lv.targetSize, orig_size)
+ self.assertEqual(lv.size, orig_size)
+
+class PartitionDeviceTestCase(unittest.TestCase):
+
+ def testTargetSize(self):
+ with sparsetmpfile("targetsizetest", Size("10 MiB")) as disk_file:
+ disk = DiskFile(disk_file)
+ disk.format = getFormat("disklabel", device=disk.path)
+ grain_size = disk.format.alignment.grainSize
+ sector_size = disk.format.partedDevice.sectorSize
!!! Could you turn the two above into Sizes(), to make their units explicit?
+ start = grain_size
+ orig_size = Size("6 MiB")
+ end = start + (orig_size / sector_size) - 1
!!! This line is confusing to me, because of the mix of units.
It seems that start should be a Size, but orig_size / sector_size has no units,
and I'm not sure about 1.
+ disk.format.addPartition(start, end)
+ partition = disk.format.partedDisk.getPartitionBySector(start)
+ self.assertNotEqual(partition, None)
+ self.assertEqual(orig_size, Size(partition.getLength(unit='B')))
+
+ device = PartitionDevice(os.path.basename(partition.path),
+ size=orig_size)
+ device.disk = disk
+ device.exists = True
+ device.partedPartition = partition
+
+ device.format = getFormat("ext4", device=device.path)
+ device.format.exists = True
+ # grain size should be 1 MiB
+ device.format._minInstanceSize = Size("2 MiB") + (grain_size / 2)
+
+ # Make sure things are as expected to begin with.
+ self.assertEqual(device.size, orig_size)
+ self.assertEqual(device.minSize, Size("3 MiB"))
+ # start sector's at 1 MiB
+ self.assertEqual(device.maxSize, Size("9 MiB"))
+
+ # ValueError if not Size
+ with self.assertRaisesRegexp(ValueError,
+ "new size must.*type Size"):
+ device.targetSize = 22
+
+ self.assertEqual(device.targetSize, orig_size)
+
+ # ValueError if size smaller than minSize
+ with self.assertRaisesRegexp(ValueError,
+ "size.*smaller than the minimum"):
+ device.targetSize = Size("1 MiB")
+
+ self.assertEqual(device.targetSize, orig_size)
+
+ # ValueError if size larger than maxSize
+ with self.assertRaisesRegexp(ValueError,
+ "size.*larger than the maximum"):
+ device.targetSize = Size("11 MiB")
+
+ self.assertEqual(device.targetSize, orig_size)
+
+ # ValueError if unaligned
+ with self.assertRaisesRegexp(ValueError, "new size.*not.*aligned"):
+ device.targetSize = Size("3.1 MiB")
+
+ self.assertEqual(device.targetSize, orig_size)
+
+ # successfully set a new target size
+ new_target = device.maxSize
+ device.targetSize = new_target
+ self.assertEqual(device.targetSize, new_target)
+ self.assertEqual(device.size, new_target)
+ parted_size = Size(device.partedPartition.getLength(unit='B'))
+ self.assertEqual(parted_size, device.targetSize)
+
+ # reset target size to original size
+ device.targetSize = orig_size
+ self.assertEqual(device.targetSize, orig_size)
+ self.assertEqual(device.size, orig_size)
+ parted_size = Size(device.partedPartition.getLength(unit='B'))
+ self.assertEqual(parted_size, device.targetSize)
+
+ def testMinMaxSizeAlignment(self):
+ with sparsetmpfile("minsizetest", Size("10 MiB")) as disk_file:
+ disk = DiskFile(disk_file)
+ disk.format = getFormat("disklabel", device=disk.path)
+ grain_size = disk.format.alignment.grainSize
+ sector_size = disk.format.partedDevice.sectorSize
!!! Also be explicit about sizes here.
+ start = grain_size
+ end = start + (Size("6 MiB") / sector_size)
!!! Here, too is a somewhat confusing mix of units.
+ disk.format.addPartition(start, end)
+ partition = disk.format.partedDisk.getPartitionBySector(start)
+ self.assertNotEqual(partition, None)
+
+ device = PartitionDevice(os.path.basename(partition.path))
+ device.disk = disk
+ device.exists = True
+ device.partedPartition = partition
+
+ # Typical sector size is 512 B.
+ # Default optimum alignment grain size is 2048 sectors, or 1 MiB.
+ device.format = getFormat("ext4", device=device.path)
+ device.format.exists = True
+ device.format._minInstanceSize = Size("2 MiB") + (grain_size / 2)
+
+ ##
+ ## minSize
+ ##
+
+ # The end sector based only on format min size should be unaligned.
+ min_sectors = int(device.format.minSize / sector_size)
+ min_end_sector = partition.geometry.start + min_sectors - 1
!!! Again there is a mix of units here or units are unspecified.
+ self.assertEqual(
+ disk.format.endAlignment.isAligned(partition.geometry,
+ min_end_sector),
+ False)
+
+ # The end sector based on device min size should be aligned.
+ min_sectors = int(device.minSize / sector_size)
+ min_end_sector = partition.geometry.start + min_sectors - 1
!!! Uncertain of units here as well.
+ self.assertEqual(
+ disk.format.endAlignment.isAligned(partition.geometry,
+ min_end_sector),
+ True)
+
+ ##
+ ## maxSize
+ ##
+
+ # Add a partition starting three sectors past an aligned sector and
+ # extending to the end of the disk so that there's a free region
+ # immediately following the first partition with an unaligned end
+ # sector.
+ free = disk.format.partedDisk.getFreeSpaceRegions()[-1]
+ raw_start = int(Size("9 MiB") / sector_size)
+ start = disk.format.alignment.alignUp(free, raw_start) + 3
!!! Units?
+ disk.format.addPartition(start, disk.partedDevice.length - 1)
+
!!! Units?
+ # Verify the end of the free region immediately following the first
+ # partition is unaligned.
+ free = disk.format.partedDisk.getFreeSpaceRegions()[1]
+ self.assertEqual(disk.format.endAlignment.isAligned(free, free.end),
+ False)
+
+ # The end sector based on device min size should be aligned.
+ max_sectors = int(device.maxSize / sector_size)
+ max_end_sector = partition.geometry.start + max_sectors - 1
!!! Units?
+ self.assertEqual(
+ disk.format.endAlignment.isAligned(free, max_end_sector),
+ True)
+
class DeviceNameTestCase(unittest.TestCase):
"""Test device name validation"""
diff --git a/tests/storagetestcase.py b/tests/storagetestcase.py
index 11de630..693d119 100644
--- a/tests/storagetestcase.py
+++ b/tests/storagetestcase.py
@@ -36,7 +36,9 @@ class StorageTestCase(unittest.TestCase):
# prevent PartitionDevice from trying to dig around in the partition's
# geometry
blivet.devices.PartitionDevice._setTargetSize = StorageDevice._setTargetSize
+ blivet.devices.PartitionDevice.alignTargetSize = StorageDevice.alignTargetSize
blivet.devices.PartitionDevice.maxSize = StorageDevice.maxSize
+ blivet.devices.PartitionDevice.minSize = StorageDevice.minSize
def partition_probe(device):
if isinstance(device._partedPartition, Mock):
--
1.9.3
_______________________________________________
anaconda-patches mailing list
anaconda-patches at lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
A bunch of inline comments, all preceded by !!! for easy location.
- mulhern
More information about the anaconda-patches
mailing list