[blivet:master 1/3] Factor out MDRaidArrayDevice w/ type in ("mdcontainer", "mdbiosraidarray")
David Lehman
dlehman at redhat.com
Fri Dec 12 20:52:23 UTC 2014
On 12/12/2014 12:35 PM, mulhern wrote:
> This patch factors out functionality associated w/ type
> "mdcontainer" or "mdbiosraidarray" into separate subclasses.
>
> It is also able to revert commit c197d2d39104cdef234daecf5c44098bc345b506
> entirely, since the refactoring into MDBiosRaidArrayDevice w/
> formatClassName None, handles all that functionality.
>
> Leave some checks in MDRaidArrayDevice.__init__ to prevent using this class
> instead of its subclass.
>
> I tried to make the transformation as semantics preserving as possible.
> It is kind of nice that the only changes needed to be made to
> devices tests were to change the names of constructors.
Just a couple of comments inline...
>
> Signed-off-by: mulhern <amulhern at redhat.com>
> ---
> blivet/devicelibs/mdraid.py | 2 +-
> blivet/devices/__init__.py | 2 +-
> blivet/devices/container.py | 18 +--
> blivet/devices/md.py | 189 +++++++++++++++--------
> tests/devicelibs_test/mdraid_interrogate_test.py | 4 +-
> tests/devicelibs_test/mdraid_test.py | 1 -
> tests/devices_test.py | 14 +-
> 7 files changed, 141 insertions(+), 89 deletions(-)
>
> diff --git a/blivet/devicelibs/mdraid.py b/blivet/devicelibs/mdraid.py
> index 11e06d7..5479a03 100644
> --- a/blivet/devicelibs/mdraid.py
> +++ b/blivet/devicelibs/mdraid.py
> @@ -45,7 +45,7 @@ class MDRaidLevels(raid.RAIDLevels):
> hasattr(level, 'get_recommended_stride') and \
> hasattr(level, 'get_size')
>
> -RAID_levels = MDRaidLevels(["raid0", "raid1", "raid4", "raid5", "raid6", "raid10", "container", "linear"])
> +RAID_levels = MDRaidLevels(["raid0", "raid1", "raid4", "raid5", "raid6", "raid10", "linear"])
>
> def get_raid_superblock_size(size, version=None):
> """ mdadm has different amounts of space reserved for its use depending
> diff --git a/blivet/devices/__init__.py b/blivet/devices/__init__.py
> index 0b2eef2..cf1ec34 100644
> --- a/blivet/devices/__init__.py
> +++ b/blivet/devices/__init__.py
> @@ -27,7 +27,7 @@ from .partition import PartitionDevice
> from .dm import DMDevice, DMLinearDevice, DMCryptDevice
> from .luks import LUKSDevice
> from .lvm import LVMVolumeGroupDevice, LVMLogicalVolumeDevice, LVMSnapShotDevice, LVMThinPoolDevice, LVMThinLogicalVolumeDevice, LVMThinSnapShotDevice
> -from .md import MDRaidArrayDevice
> +from .md import MDBiosRaidArrayDevice, MDContainerDevice, MDRaidArrayDevice
> from .btrfs import BTRFSDevice, BTRFSVolumeDevice, BTRFSSubVolumeDevice, BTRFSSnapShotDevice
> from .file import FileDevice, DirectoryDevice, SparseFileDevice
> from .loop import LoopDevice
> diff --git a/blivet/devices/container.py b/blivet/devices/container.py
> index 4604231..612b9fc 100644
> --- a/blivet/devices/container.py
> +++ b/blivet/devices/container.py
> @@ -62,18 +62,6 @@ class ContainerDevice(StorageDevice):
>
> super(ContainerDevice, self).__init__(*args, **kwargs)
>
> - def _verifyMemberFormat(self, member):
> - """ Whether the member has the format expected by the device.
> -
> - :param member: the member device to add
> - :type member: :class:`.StorageDevice`
> - :returns: error msg if the member has incorrect format, else None
> - :rtype: str or NoneType
> - """
> - if not isinstance(member.format, self.formatClass):
> - return "Member format %(format)s is not a subtype of expected format %(expected)s." % {'format' : member.format, 'expected' : self.formatClass}
> - return None
> -
> def _verifyMemberUuid(self, member, expect_equality=True, require_existence=True):
> """ Whether the member's array UUID has the proper relationship
> with its array's UUID.
> @@ -126,10 +114,8 @@ class ContainerDevice(StorageDevice):
> contents at all.
> """
> log_method_call(self, self.name, member=member.name)
> -
> - error = self._verifyMemberFormat(member)
> - if error:
> - raise ValueError(error)
> + if not isinstance(member.format, self.formatClass):
> + raise ValueError("member has wrong format")
>
> error = self._verifyMemberUuid(member)
> if error:
> diff --git a/blivet/devices/md.py b/blivet/devices/md.py
> index dcd0c54..4067f2f 100644
> --- a/blivet/devices/md.py
> +++ b/blivet/devices/md.py
> @@ -86,9 +86,6 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
> parents=parents,
> sysfsPath=sysfsPath)
>
> - if level == "container":
> - self._type = "mdcontainer"
> -
> # avoid attribute-defined-outside-init pylint warning
> self._level = None
>
> @@ -112,11 +109,8 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
> else:
> self.metadataVersion = metadataVersion
>
> - # For container members probe size now, as we cannot determine it
> - # when teared down.
> - if self.parents and self.parents[0].type == "mdcontainer":
> - self._size = self.currentSize
> - self._type = "mdbiosraidarray"
> + if self.parents and self.parents[0].type == "mdcontainer" and self.type != "mdbiosraidarray":
> + raise errors.DeviceError("A device with mdcontainer member must be mdbiosraidarray.")
>
> if self.exists and self.mdadmFormatUUID and not flags.testing:
> # this is a hack to work around mdadm's insistence on giving
> @@ -124,12 +118,6 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
> with open("/etc/mdadm.conf", "a") as c:
> c.write("ARRAY %s UUID=%s\n" % (self.path, self.mdadmFormatUUID))
>
> - def _verifyMemberFormat(self, member):
> - if member.type == "mdcontainer":
> - return None
> -
> - return super(MDRaidArrayDevice, self)._verifyMemberFormat(member)
> -
> @property
> def mdadmFormatUUID(self):
> """ This array's UUID, formatted for external use.
> @@ -156,6 +144,11 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
> """
> return self._level
>
> + @property
> + def _levels(self):
> + """ Allowed RAID level for this type of device."""
> + return mdraid.RAID_levels
> +
> @level.setter
> def level(self, value):
> """ Set the RAID level and enforce restrictions based on it.
> @@ -167,7 +160,7 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
> :returns: None
> """
> try:
> - level = self._getLevel(value, mdraid.RAID_levels)
> + level = self._getLevel(value, self._levels)
> except ValueError as e:
> raise errors.DeviceError(e)
>
> @@ -207,11 +200,6 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
> """Returns the actual or estimated size depending on whether or
> not the array exists.
> """
> - # For container members return probed size, as we cannot determine it
> - # when teared down.
> - if self.type == "mdbiosraidarray":
> - return self._size
> -
> if not self.exists or not self.partedDevice:
> try:
> size = self.level.get_size([d.size for d in self.devices],
> @@ -230,14 +218,8 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
>
> @property
> def description(self):
> - if self.type == "mdcontainer":
> - return "BIOS RAID container"
> - else:
> - levelstr = self.level.nick if self.level.nick else self.level.name
> - if self.type == "mdbiosraidarray":
> - return "BIOS RAID set (%s)" % levelstr
> - else:
> - return "MDRAID set (%s)" % levelstr
> + levelstr = self.level.nick if self.level.nick else self.level.name
> + return "MDRAID set (%s)" % levelstr
>
> def __repr__(self):
> s = StorageDevice.__repr__(self)
> @@ -266,11 +248,6 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
> if self.memberDevices is None or not self.mdadmFormatUUID:
> raise errors.DeviceError("array is not fully defined", self.name)
>
> - # containers and the sets within must only have a UUID= parameter
> - if self.type == "mdcontainer" or self.type == "mdbiosraidarray":
> - fmt = "ARRAY %s UUID=%s\n"
> - return fmt % (self.path, self.mdadmFormatUUID)
> -
> fmt = "ARRAY %s level=%s num-devices=%d UUID=%s\n"
> return fmt % (self.path, self.level, self.memberDevices, self.mdadmFormatUUID)
>
> @@ -345,6 +322,11 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
> self.memberDevices -= 1
>
> @property
> + def _trueStatusStrings(self):
> + """ Strings in state file for which status() should return True."""
> + return ("clean", "active", "active-idle", "readonly", "read-auto")
> +
> + @property
> def status(self):
> """ This device's status.
>
> @@ -373,10 +355,7 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
> state_file = "%s/md/array_state" % self.sysfsPath
> try:
> state = open(state_file).read().strip()
> - if state in ("clean", "active", "active-idle", "readonly", "read-auto"):
> - status = True
> - # mdcontainers have state inactive when started (clear if stopped)
> - if self.type == "mdcontainer" and state == "inactive":
> + if state in self._trueStatusStrings:
> status = True
> except IOError:
> status = False
> @@ -412,17 +391,9 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
> def members(self):
> """ Returns this array's members.
>
> - If the array is a BIOS RAID array then its unique parent
> - is a container and its actual member devices are the
> - container's parents.
> -
> :rtype: list of :class:`StorageDevice`
> """
> - if self.type == "mdbiosraidarray":
> - members = self.parents[0].parents
> - else:
> - members = self.parents
> - return list(members)
> + return list(self.parents)
>
> @property
> def complete(self):
> @@ -468,12 +439,6 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
> # see comment just above mddeactivate call
> self._preTeardown(recursive=recursive)
>
> - # Since BIOS RAID sets (containers in mdraid terminology) never change
> - # there is no need to stop them and later restart them. Not stopping
> - # (and thus also not starting) them also works around bug 523334
> - if self.type == "mdcontainer" or self.type == "mdbiosraidarray":
> - return
> -
> # We don't really care what the array's state is. If the device
> # file exists, we want to deactivate it. mdraid has too many
> # states.
> @@ -562,14 +527,7 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
>
> @property
> def mediaPresent(self):
> - # Containers should not get any format handling done
> - # (the device node does not allow read / write calls)
> - if self.type == "mdcontainer":
> - return False
> - # BIOS RAID sets should show as present even when teared down
> - elif self.type == "mdbiosraidarray":
> - return True
> - elif flags.testing:
> + if flags.testing:
> return True
> else:
> return self.partedDevice is not None
> @@ -580,11 +538,11 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
>
> @property
> def partitionable(self):
> - return self.type == "mdbiosraidarray"
> + return False
>
> @property
> def isDisk(self):
> - return self.type == "mdbiosraidarray"
> + return False
The above are both default values, so you can just remove partitionable
and isDisk from MDRaidArrayDevice.
>
> def dracutSetupArgs(self):
> return set(["rd.md.uuid=%s" % self.mdadmFormatUUID])
> @@ -599,3 +557,110 @@ class MDRaidArrayDevice(ContainerDevice, RaidDevice):
> data.members = ["raid.%d" % p.id for p in self.parents]
> data.preexist = self.exists
> data.device = self.name
> +
> +class MDContainerDevice(MDRaidArrayDevice):
> +
> + _type = "mdcontainer"
> +
> + def __init__(self, name, **kwargs):
> + kwargs['level'] = raid.Container
> + super(MDContainerDevice, self).__init__(name, **kwargs)
> +
> + @property
> + def _levels(self):
> + return mdraid.MDRaidLevels(["container"])
> +
> + @property
> + def description(self):
> + return "BIOS RAID container"
> +
> + @property
> + def mdadmConfEntry(self):
> + if not self.uuid:
> + raise errors.DeviceError("array is not fully defined", self.name)
> +
> + return "ARRAY %s UUID=%s\n" % (self.path, self.mdadmFormatUUID)
> +
> + @property
> + def _trueStatusStrings(self):
> + return ("clean", "active", "active-idle", "readonly", "read-auto", "inactive")
> +
> + def teardown(self, recursive=None):
> + log_method_call(self, self.name, status=self.status,
> + controllable=self.controllable)
> + # we don't really care about the return value of _preTeardown here.
> + # see comment just above mddeactivate call
> + self._preTeardown(recursive=recursive)
> +
> + # Since BIOS RAID sets (containers in mdraid terminology) never change
> + # there is no need to stop them and later restart them. Not stopping
> + # (and thus also not starting) them also works around bug 523334
> + return
> +
> + @property
> + def mediaPresent(self):
> + # Containers should not get any format handling done
> + # (the device node does not allow read / write calls)
> + return False
> +
> +class MDBiosRaidArrayDevice(MDRaidArrayDevice):
> +
> + _type = "mdbiosraidarray"
> + _formatClassName = property(lambda s: None)
> +
> + def __init__(self, name, **kwargs):
> + super(MDBiosRaidArrayDevice, self).__init__(name, **kwargs)
> +
> + # For container members probe size now, as we cannot determine it
> + # when teared down.
> + self._size = self.currentSize
> +
> + @property
> + def size(self):
> + # For container members return probed size, as we cannot determine it
> + # when teared down.
> + return self._size
> +
> + @property
> + def description(self):
> + levelstr = self.level.nick if self.level.nick else self.level.name
> + return "BIOS RAID set (%s)" % levelstr
> +
> + @property
> + def mdadmConfEntry(self):
> + if not self.uuid:
> + raise errors.DeviceError("array is not fully defined", self.name)
> +
> + return "ARRAY %s UUID=%s\n" % (self.path, self.mdadmFormatUUID)
> +
> + @property
> + def members(self):
> + # If the array is a BIOS RAID array then its unique parent
> + # is a container and its actual member devices are the
> + # container's parents.
> + return list(self.parents[0].parents)
> +
> + def teardown(self, recursive=None):
> + log_method_call(self, self.name, status=self.status,
> + controllable=self.controllable)
> + # we don't really care about the return value of _preTeardown here.
> + # see comment just above mddeactivate call
> + self._preTeardown(recursive=recursive)
> +
> + # Since BIOS RAID sets (containers in mdraid terminology) never change
> + # there is no need to stop them and later restart them. Not stopping
> + # (and thus also not starting) them also works around bug 523334
> + return
> +
> + @property
> + def mediaPresent(self):
> + # BIOS RAID sets should show as present even when teared down
> + return True
> +
> + @property
> + def isDisk(self):
> + return True
The intended way to do this is to set the class attribute _isDisk to True.
> +
> + @property
> + def partitionable(self):
> + return True
> diff --git a/tests/devicelibs_test/mdraid_interrogate_test.py b/tests/devicelibs_test/mdraid_interrogate_test.py
> index 034c929..33bb5ad 100755
> --- a/tests/devicelibs_test/mdraid_interrogate_test.py
> +++ b/tests/devicelibs_test/mdraid_interrogate_test.py
> @@ -91,7 +91,7 @@ class MDExamineTestCase(MDRaidInterrogateTestCase):
> - RAID level and number of devices are correct
> - UUIDs have canonical form
> """
> - level = mdraid.RAID_levels.raidLevel(level or raid.RAID1)
> + level = raid.getRaidLevel(level or raid.RAID1)
> mdraid.mdcreate(self._dev_name, level, self.loopDevices, metadataVer=metadataVersion, spares=spares)
> time.sleep(2) # wait for raid to settle
>
> @@ -194,7 +194,7 @@ class MDDetailTestCase(MDRaidInterrogateTestCase):
> - exactly the predicted names are returned by mddetail
> - UUIDs have canonical form
> """
> - level = mdraid.RAID_levels.raidLevel(level) if level is not None else raid.RAID1
> + level = raid.getRaidLevel(level) if level is not None else raid.RAID1
>
> mdraid.mdcreate(self._dev_name, level, self.loopDevices, metadataVer=metadataVersion, spares=spares)
> time.sleep(2) # wait for raid to settle
> diff --git a/tests/devicelibs_test/mdraid_test.py b/tests/devicelibs_test/mdraid_test.py
> index bbf4f94..4cd50a3 100755
> --- a/tests/devicelibs_test/mdraid_test.py
> +++ b/tests/devicelibs_test/mdraid_test.py
> @@ -16,7 +16,6 @@ class MDRaidTestCase(unittest.TestCase):
> ##
> ## level lookup
> ##
> - self.assertEqual(mdraid.RAID_levels.raidLevel("container").name, "container")
> self.assertEqual(mdraid.RAID_levels.raidLevel("stripe").name, "raid0")
> self.assertEqual(mdraid.RAID_levels.raidLevel("mirror").name, "raid1")
> self.assertEqual(mdraid.RAID_levels.raidLevel("4").name, "raid4")
> diff --git a/tests/devices_test.py b/tests/devices_test.py
> index 0302fdf..3d7c28e 100644
> --- a/tests/devices_test.py
> +++ b/tests/devices_test.py
> @@ -22,6 +22,8 @@ from blivet.devices import LVMThinPoolDevice
> from blivet.devices import LVMThinLogicalVolumeDevice
> from blivet.devices import LVMThinSnapShotDevice
> from blivet.devices import LVMVolumeGroupDevice
> +from blivet.devices import MDBiosRaidArrayDevice
> +from blivet.devices import MDContainerDevice
> from blivet.devices import MDRaidArrayDevice
> from blivet.devices import OpticalDevice
> from blivet.devices import PartitionDevice
> @@ -133,7 +135,7 @@ class MDRaidArrayDeviceTestCase(DeviceStateTestCase):
> parents = [
> DiskDevice("name1", fmt=getFormat("mdmember"))
> ]
> - self.dev1 = MDRaidArrayDevice("dev1", level="container", parents=parents)
> + self.dev1 = MDContainerDevice("dev1", level="container", parents=parents)
>
> parents = [
> DiskDevice("name1", fmt=getFormat("mdmember")),
> @@ -183,7 +185,7 @@ class MDRaidArrayDeviceTestCase(DeviceStateTestCase):
> parents_1 = [
> DiskDevice("name1", fmt=getFormat("mdmember"))
> ]
> - dev_1 = MDRaidArrayDevice(
> + dev_1 = MDContainerDevice(
> "parent",
> level="container",
> parents=parents_1
> @@ -198,7 +200,7 @@ class MDRaidArrayDeviceTestCase(DeviceStateTestCase):
> fmt=getFormat("mdmember"),
> parents=parents_2
> )
> - self.dev9 = MDRaidArrayDevice(
> + self.dev9 = MDBiosRaidArrayDevice(
> "dev9",
> level="raid0",
> memberDevices=2,
> @@ -219,7 +221,7 @@ class MDRaidArrayDeviceTestCase(DeviceStateTestCase):
> parents_1 = [
> DiskDevice("name1", fmt=getFormat("mdmember"))
> ]
> - dev_1 = MDRaidArrayDevice(
> + dev_1 = MDContainerDevice(
> "parent",
> level="container",
> parents=parents
> @@ -234,7 +236,7 @@ class MDRaidArrayDeviceTestCase(DeviceStateTestCase):
> fmt=getFormat("mdmember"),
> parents=parents_2
> )
> - self.dev11 = MDRaidArrayDevice(
> + self.dev11 = MDBiosRaidArrayDevice(
> "dev11",
> level=1,
> memberDevices=2,
> @@ -242,7 +244,7 @@ class MDRaidArrayDeviceTestCase(DeviceStateTestCase):
> size=Size("32 MiB"),
> totalDevices=2)
>
> - self.dev12 = MDRaidArrayDevice(
> + self.dev12 = MDBiosRaidArrayDevice(
> "dev12",
> level=1,
> memberDevices=2,
>
More information about the anaconda-patches
mailing list