----- Original Message -----
From: "Vratislav Podzimek" <vpodzime(a)redhat.com>
To: "anaconda patch review" <anaconda-patches(a)lists.fedorahosted.org>
Sent: Friday, May 30, 2014 3:14:29 AM
Subject: Re: [blivet:master 05/26] No longer use _standard_levels as the default set of
RAID levels.
On Thu, 2014-05-29 at 14:47 -0400, mulhern wrote:
> The notion of a "standard" RAID level is being eroded too rapidly to make
> it
> really useful. ALL_LEVELS is a better notion.
> This change has the added benefit of simplifying the RAIDlevels
> constructor.
> Also, make _raid_levels property a set instead of a list since no real
> ordering make sense.
I believe the ordering makes sense -- users would be quite confused if
we offered them to choose from the following RAID levels:
1. RAID 4
2. RAID 0
3. RAID 5
4. RAID 10
...
Maybe the RAID levels don't have to be ordered internally, but when
queried, they should be returned as a list from the lowest number to the
biggest one. Anaconda does the ordering by its Glade files, but that
doesn't have to be the case of other tools using blivet.
Where do Linear, Single, Container, RAID01, Dup, etc. fit in your ordering?
I'ld rather have clients decide what is best for them then impose an ordering
that will surely be wrong for some.
>
> Signed-off-by: mulhern <amulhern(a)redhat.com>
> ---
> blivet/devicelibs/mdraid.py | 2 +-
> blivet/devicelibs/raid.py | 77
> ++++++++++++--------------------------
> tests/devicelibs_test/raid_test.py | 13 ++++---
> 3 files changed, 31 insertions(+), 61 deletions(-)
>
> diff --git a/blivet/devicelibs/mdraid.py b/blivet/devicelibs/mdraid.py
> index aeb554c..2106548 100644
> --- a/blivet/devicelibs/mdraid.py
> +++ b/blivet/devicelibs/mdraid.py
> @@ -44,7 +44,7 @@ class MDRaidLevels(raid.RAIDLevels):
> except AttributeError:
> return False
>
> -_RAID_levels = MDRaidLevels()
> +_RAID_levels = MDRaidLevels(["raid0", "raid1",
"raid4", "raid5", "raid6",
> "raid10"])
>
> class Container(object):
> name = "container"
> diff --git a/blivet/devicelibs/raid.py b/blivet/devicelibs/raid.py
> index bdc17f7..86b584e 100644
> --- a/blivet/devicelibs/raid.py
> +++ b/blivet/devicelibs/raid.py
> @@ -245,38 +245,24 @@ class RAIDLevels(object):
> for its RAID levels.
> """
>
> - def __init__(self, levels=True):
> + def __init__(self, levels=None):
> """Add the specified standard levels to the levels in this
object.
>
> - :param levels: the standard levels to be added to this object
> - :type levels: bool or a list of valid RAID level descriptors
> + :param levels: the levels to be added to this object
> + :type levels: list of valid RAID level descriptors
>
> If levels is True, add all standard levels. Else, levels
> must be a list of valid level descriptors of standard levels.
> Duplicate descriptors are ignored.
> """
> - self._raid_levels = []
> - if levels is False:
> - pass
> - elif levels is True:
> - for l in self.standard_levels:
> - self.addRaidLevel(l)
> - else:
> - try:
> - for level in levels:
> - matches = [l for l in self.standard_levels if level in
> l.names]
> - if len(matches) != 1:
> - raise RaidError("invalid standard RAID level
> descriptor %s" % level)
> - else:
> - self.addRaidLevel(matches[0])
> - except TypeError:
> - raise RaidError("levels must be a boolean or an
iterable")
> -
> -
> - _standard_levels = []
> -
> - standard_levels = property(lambda s: s._standard_levels,
> - doc="any standard RAID level classes defined in this package.")
> + levels = levels or []
> + self._raid_levels = set()
> + for level in levels:
> + matches = [l for l in ALL_LEVELS if level in l.names]
> + if len(matches) != 1:
> + raise RaidError("invalid standard RAID level descriptor
> %s" % level)
> + else:
> + self.addRaidLevel(matches[0])
>
> @classmethod
> def isRaidLevel(cls, level):
> @@ -291,12 +277,11 @@ class RAIDLevels(object):
> The name property must be defined; it should be one of the
> elements in the names list.
>
> - All RAID objects in standard_levels are guaranteed to pass
> these
> + All RAID objects that extend RAIDlevel are guaranteed to pass
> these
> minimum requirements.
>
> This method should not be overridden in any subclass so that it
> - is so restrictive that a RAID object in standard_levels does
> - not satisfy it.
> + is so restrictive that a RAIDlevel object does not satisfy it.
> """
> try:
> name = level.names[0]
> @@ -307,23 +292,6 @@ class RAIDLevels(object):
> return False
> return True
>
> - @classmethod
> - def addRAIDLevelToStandardLevels(cls, level):
> - """Adds this RAID level to the internal list of standard
RAID
> levels
> - defined in this package.
> -
> - :param level: an object representing a RAID level
> - :type level: object
> -
> - Raises a RaidError if level is not a valid RAID level.
> -
> - Does not allow duplicate level objects.
> - """
> - if not cls.isRaidLevel(level):
> - raise RaidError('level is not a valid RAID level')
> - if not level in cls._standard_levels:
> - cls._standard_levels.append(level)
> -
> def raidLevel(self, descriptor):
> """Return RAID object corresponding to descriptor.
>
> @@ -338,7 +306,7 @@ class RAIDLevels(object):
> raise RaidError("invalid RAID level descriptor %s" % descriptor)
>
> def addRaidLevel(self, level):
> - """Adds level to the list of levels if it is not already
there.
> + """Adds level to levels if it is not already there.
>
> :param object level: an object representing a RAID level
>
> @@ -348,12 +316,13 @@ class RAIDLevels(object):
> """
> if not self.isRaidLevel(level):
> raise RaidError("level is not a valid RAID level")
> - if not level in self._raid_levels:
> - self._raid_levels.append(level)
> + self._raid_levels.add(level)
>
> def __iter__(self):
> return iter(self._raid_levels)
>
> +ALL_LEVELS = RAIDLevels()
> +
> class RAID0(RAIDLevel):
>
> level = property(lambda s: "0")
> @@ -376,7 +345,7 @@ class RAID0(RAIDLevel):
> return member_count * 16
>
> RAID0 = RAID0()
> -RAIDLevels.addRAIDLevelToStandardLevels(RAID0)
> +ALL_LEVELS.addRaidLevel(RAID0)
>
> class RAID1(RAIDLevel):
> level = property(lambda s: "1")
> @@ -399,7 +368,7 @@ class RAID1(RAIDLevel):
> return None
>
> RAID1 = RAID1()
> -RAIDLevels.addRAIDLevelToStandardLevels(RAID1)
> +ALL_LEVELS.addRaidLevel(RAID1)
>
> class RAID4(RAIDLevel):
> level = property(lambda s: "4")
> @@ -422,7 +391,7 @@ class RAID4(RAIDLevel):
> return (member_count - 1) * 16
>
> RAID4 = RAID4()
> -RAIDLevels.addRAIDLevelToStandardLevels(RAID4)
> +ALL_LEVELS.addRaidLevel(RAID4)
>
> class RAID5(RAIDLevel):
> level = property(lambda s: "5")
> @@ -445,7 +414,7 @@ class RAID5(RAIDLevel):
> return (member_count - 1) * 16
>
> RAID5 = RAID5()
> -RAIDLevels.addRAIDLevelToStandardLevels(RAID5)
> +ALL_LEVELS.addRaidLevel(RAID5)
>
> class RAID6(RAIDLevel):
> level = property(lambda s: "6")
> @@ -468,7 +437,7 @@ class RAID6(RAIDLevel):
> return None
>
> RAID6 = RAID6()
> -RAIDLevels.addRAIDLevelToStandardLevels(RAID6)
> +ALL_LEVELS.addRaidLevel(RAID6)
>
> class RAID10(RAIDLevel):
> level = property(lambda s: "10")
> @@ -491,4 +460,4 @@ class RAID10(RAIDLevel):
> return None
>
> RAID10 = RAID10()
> -RAIDLevels.addRAIDLevelToStandardLevels(RAID10)
> +ALL_LEVELS.addRaidLevel(RAID10)
> diff --git a/tests/devicelibs_test/raid_test.py
> b/tests/devicelibs_test/raid_test.py
> index 7c7501b..e192aea 100644
> --- a/tests/devicelibs_test/raid_test.py
> +++ b/tests/devicelibs_test/raid_test.py
> @@ -1,6 +1,7 @@
> #!/usr/bin/python
> import unittest
>
> +import blivet.devicelibs.mdraid as mdraid
> import blivet.devicelibs.raid as raid
> import blivet.errors as errors
> from blivet.size import Size
> @@ -8,8 +9,8 @@ from blivet.size import Size
> class RaidTestCase(unittest.TestCase):
>
> def setUp(self):
> - self.levels = raid.RAIDLevels()
> - self.levels_none = raid.RAIDLevels(False)
> + self.levels = raid.RAIDLevels(["raid0", "raid1",
"raid4", "raid5",
> "raid6", "raid10"])
> + self.levels_none = raid.RAIDLevels([])
> self.levels_some = raid.RAIDLevels(["mirror", 6])
>
> def testRaid(self):
> @@ -99,7 +100,7 @@ class RaidTestCase(unittest.TestCase):
> ##
> ## size
> ##
> - for r in raid.RAIDLevels():
> + for r in [l for l in raid.ALL_LEVELS if l is not
> mdraid.Container]:
This and all the other similar lines should use generator comprehension
instead of list comprehension.
Done.
> self.assertEqual(r.get_size([ Size("32MiB"),
> Size("128MiB"),
> Size("128MiB"),
> @@ -109,7 +110,7 @@ class RaidTestCase(unittest.TestCase):
> lambda x: Size(0)),
> r.get_net_array_size(4, Size("32MiB")))
>
> - for r in raid.RAIDLevels():
> + for r in [l for l in raid.ALL_LEVELS if l is not
> mdraid.Container]:
> self.assertEqual(r.get_size([ Size("32MiB"),
> Size("128MiB"),
> Size("128MiB"),
> @@ -119,7 +120,7 @@ class RaidTestCase(unittest.TestCase):
> lambda x: Size(0)),
> r.get_net_array_size(5, Size("32MiB")))
>
> - for r in raid.RAIDLevels():
> + for r in [l for l in raid.ALL_LEVELS if l is not
> mdraid.Container]:
> self.assertEqual(r.get_size([ Size("32MiB"),
> Size("128MiB"),
> Size("128MiB"),
> @@ -129,7 +130,7 @@ class RaidTestCase(unittest.TestCase):
> lambda x: Size("32MiB")),
> 0)
>
> - for r in raid.RAIDLevels():
> + for r in [l for l in raid.ALL_LEVELS if l is not
> mdraid.Container]:
> self.assertEqual(r.get_size([ Size("32MiB"),
> Size("128MiB"),
> Size("128MiB"),
--
Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
_______________________________________________
anaconda-patches mailing list
anaconda-patches(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches