I realized that I had been mistaken about somethings, and I noticed a few other things that ought to be fixed up.
First two patches are just testing. The next two are just documentation. The next two are just using udev_device_get_md_*() methods according to their spec. The next two are a very minor bug fix and some accompanying stuff. The final three are the bug fix. Canoncalize... is unchanged, the rest are changed a bit.
mulhern (11): Add test for raid level descriptor None. Add a few small tests for mdexamine Document udev_device_get_uuid() method. Add documentation to udev_device_get_md_*() methods. Do not convert the result of udev_device_get_md_devices() to int. Check for md_level of None Change use of METADATA to MD_METADATA. Add a udev_device_get_md_metadata() method to udev and use it. Canonicalize mdadm generated UUIDS (#1070095) Add new method udev_device_get_md_device_uuid() method (#1070095) Attempt to reset the uuid of the mdraid member device (#1070095)
blivet/devicelibs/mdraid.py | 9 ++++ blivet/devicetree.py | 24 ++++++--- blivet/udev.py | 75 ++++++++++++++++++++++++++- tests/devicelibs_test/mdraid_test.py | 98 +++++++++++++++++++++++++++++++++++- tests/devicelibs_test/raid_test.py | 1 + 5 files changed, 198 insertions(+), 9 deletions(-)
Signed-off-by: mulhern amulhern@redhat.com --- tests/devicelibs_test/raid_test.py | 1 + 1 file changed, 1 insertion(+)
diff --git a/tests/devicelibs_test/raid_test.py b/tests/devicelibs_test/raid_test.py index 75c45f1..61fc23a 100644 --- a/tests/devicelibs_test/raid_test.py +++ b/tests/devicelibs_test/raid_test.py @@ -54,6 +54,7 @@ class RaidTestCase(unittest.TestCase): self.assertIs(self.levels.raidLevel(raid.RAID0), raid.RAID0)
self.assertRaises(errors.RaidError, self.levels.raidLevel, "bogus") + self.assertRaises(errors.RaidError, self.levels.raidLevel, None)
## ## get_max_spares
On Thu, 2014-07-03 at 11:19 -0400, mulhern wrote:
Signed-off-by: mulhern amulhern@redhat.com
tests/devicelibs_test/raid_test.py | 1 + 1 file changed, 1 insertion(+)
diff --git a/tests/devicelibs_test/raid_test.py b/tests/devicelibs_test/raid_test.py index 75c45f1..61fc23a 100644 --- a/tests/devicelibs_test/raid_test.py +++ b/tests/devicelibs_test/raid_test.py @@ -54,6 +54,7 @@ class RaidTestCase(unittest.TestCase): self.assertIs(self.levels.raidLevel(raid.RAID0), raid.RAID0)
self.assertRaises(errors.RaidError, self.levels.raidLevel, "bogus")
self.assertRaises(errors.RaidError, self.levels.raidLevel, None)
Just a note on this: could we, at some point, transform these checks to: with self.assertRaises(errors.RaidError): self.levels.raidLevel(None)
etc.? I think that syntax is much better readable.
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Friday, July 4, 2014 2:57:00 AM Subject: Re: [blivet:master 01/11] Add test for raid level descriptor None.
On Thu, 2014-07-03 at 11:19 -0400, mulhern wrote:
Signed-off-by: mulhern amulhern@redhat.com
tests/devicelibs_test/raid_test.py | 1 + 1 file changed, 1 insertion(+)
diff --git a/tests/devicelibs_test/raid_test.py b/tests/devicelibs_test/raid_test.py index 75c45f1..61fc23a 100644 --- a/tests/devicelibs_test/raid_test.py +++ b/tests/devicelibs_test/raid_test.py @@ -54,6 +54,7 @@ class RaidTestCase(unittest.TestCase): self.assertIs(self.levels.raidLevel(raid.RAID0), raid.RAID0)
self.assertRaises(errors.RaidError, self.levels.raidLevel, "bogus")
self.assertRaises(errors.RaidError, self.levels.raidLevel, None)
Just a note on this: could we, at some point, transform these checks to: with self.assertRaises(errors.RaidError): self.levels.raidLevel(None)
etc.? I think that syntax is much better readable.
-- Vratislav Podzimek
Anaconda Rider | RHCE | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Yes, it is a good deal easier to parse. I'll add in a patch that transforms these all to the with syntax unless anybody raises an objection. Might as well do it right now as leave it until later.
- mulhern
Signed-off-by: mulhern amulhern@redhat.com --- tests/devicelibs_test/mdraid_test.py | 92 +++++++++++++++++++++++++++++++++++- 1 file changed, 90 insertions(+), 2 deletions(-)
diff --git a/tests/devicelibs_test/mdraid_test.py b/tests/devicelibs_test/mdraid_test.py index cba8487..760643f 100755 --- a/tests/devicelibs_test/mdraid_test.py +++ b/tests/devicelibs_test/mdraid_test.py @@ -51,6 +51,35 @@ class MDRaidTestCase(unittest.TestCase):
class MDRaidAsRootTestCase(loopbackedtestcase.LoopBackedTestCase):
+ names_0 = [ + 'DEVICE', + 'MD_DEVICES', + 'MD_EVENTS', + 'MD_LEVEL', + 'MD_UPDATE_TIME', + 'MD_UUID' + ] + + names_1 = [ + 'DEVICE', + 'MD_ARRAY_SIZE', + 'MD_DEV_UUID', + 'MD_DEVICES', + 'MD_EVENTS', + 'MD_LEVEL', + 'MD_METADATA', + 'MD_NAME', + 'MD_UPDATE_TIME', + 'MD_UUID' + ] + + names_container = [ + 'MD_DEVICES', + 'MD_LEVEL', + 'MD_METADATA', + 'MD_UUID' + ] + def __init__(self, methodName='runTest'): """Set up the structure of the mdraid array.""" super(MDRaidAsRootTestCase, self).__init__(methodName=methodName) @@ -66,14 +95,73 @@ class MDRaidAsRootTestCase(loopbackedtestcase.LoopBackedTestCase):
super(MDRaidAsRootTestCase, self).tearDown()
+ def testMDExamineMDRaidArray(self): + mdraid.mdcreate(self._dev_name, raid.RAID1, self.loopDevices) + # wait for raid to settle + time.sleep(2) + + # examining the array itself yield no data + info = mdraid.mdexamine(self._dev_name) + self.assertEqual(info, {}) + + def testMDExamineNonMDRaid(self): + # invoking mdexamine on a device that is not an array member yields {} + info = mdraid.mdexamine(self.loopDevices[0]) + self.assertEqual(info, {}) + + def _testMDExamine(self, names, metadataVersion=None, level=None): + """ Test mdexamine for a specified metadataVersion. + + :param list names: mdexamine's expected list of names to return + :param str metadataVersion: the metadata version for the array + :param object level: any valid RAID level descriptor + + Verifies that: + - exactly the predicted names are returned by mdexamine + - RAID level and number of devices are correct + """ + level = mdraid.RAID_levels.raidLevel(level or raid.RAID1) + mdraid.mdcreate(self._dev_name, level, self.loopDevices, metadataVer=metadataVersion) + # wait for raid to settle + time.sleep(2) + + info = mdraid.mdexamine(self.loopDevices[0]) + + # info contains values for exactly names + for n in names: + self.assertIn(n, info, msg="name '%s' not in info" % n) + + for n in info.keys(): + self.assertIn(n, names, msg="unexpected name '%s' in info" % n) + + # check names with predictable values + self.assertEqual(info['MD_DEVICES'], '2') + self.assertEqual(info['MD_LEVEL'], str(level)) + + def testMDExamineContainerDefault(self): + self._testMDExamine(self.names_container, level="container") + + def testMDExamineDefault(self): + self._testMDExamine(self.names_1) + + def testMDExamine0(self): + self._testMDExamine(self.names_0, metadataVersion='0') + + def testMDExamine0_90(self): + self._testMDExamine(self.names_0, metadataVersion='0.90') + + def testMDExamine1(self): + self._testMDExamine(self.names_1, metadataVersion='1') + + def testMDExamine1_2(self): + self._testMDExamine(self.names_1, metadataVersion='1.2') + def testMDRaidAsRoot(self): ## ## mdcreate ## # pass self.assertEqual(mdraid.mdcreate(self._dev_name, raid.RAID1, self.loopDevices), None) - # wait for raid to settle - time.sleep(2)
# fail self.assertRaises(MDRaidError, mdraid.mdcreate, "/dev/md1", "raid1", ["/not/existing/dev0", "/not/existing/dev1"])
Signed-off-by: mulhern amulhern@redhat.com --- blivet/udev.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/blivet/udev.py b/blivet/udev.py index 09001e4..8b8c04a 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -208,7 +208,12 @@ def udev_device_get_format(udev_info): return udev_info.get("ID_FS_TYPE")
def udev_device_get_uuid(udev_info): - """ Get the UUID from the device's format as reported by udev. """ + """ Get the UUID from the device's format as reported by udev. + + :param dict udev_info: dictionary of name-value pairs as strings + :returns: a UUID or None + :rtype: str or NoneType + """ md_uuid = udev_info.get("MD_UUID", '') uuid = udev_info.get("ID_FS_UUID", '') # we don't want to return the array's uuid as a member's uuid
The inline comments indicate the likely origin of the value.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/udev.py | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
diff --git a/blivet/udev.py b/blivet/udev.py index 8b8c04a..bc4e21f 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -376,18 +376,62 @@ def udev_device_get_minor(info): return int(info["MINOR"])
def udev_device_get_md_level(info): + """ Returns the RAID level of the array of which this device is a member. + + :param dict info: dictionary of name-value pairs as strings + :returns: the RAID level of this device's md array + :rtype: str or NoneType + """ + # Value for MD_LEVEL known to be obtained from: + # * pyudev/libudev + # * mdraid/mdadm (all numeric metadata versions and container default) return info.get("MD_LEVEL")
def udev_device_get_md_devices(info): + """ Returns the number of devices in this devices's array. + + :param dict info: dictionary of name-value pairs as strings + :returns: the number of devices belonging to this device's md array + :rtype: int + :raises: KeyError, ValueError + """ + # Value for MD_DEVICES known to be obtained from: + # * pyudev/libudev + # * mdraid/mdadm (all numeric metadata versions and container default) return int(info["MD_DEVICES"])
def udev_device_get_md_uuid(info): + """ Returns the uuid of the array of which this device is a member. + + :param dict info: dictionary of name-value pairs as strings + :returns: the UUID of this device's md array + :rtype: str + :raises: KeyError + """ + # Value for MD_UUID known to be obtained from: + # * pyudev/libudev + # * mdraid/mdadm (all numeric metadata versions and container default) return info["MD_UUID"]
def udev_device_get_md_container(info): + """ + :param dict info: dictionary of name-value pairs as strings + :rtype: str or NoneType + """ + # Value for MD_CONTAINER known to be obtained from: + # * None return info.get("MD_CONTAINER")
def udev_device_get_md_name(info): + """ Returns the name of the array of which this device is a member. + + :param dict info: dictionary of name-value pairs as strings + :returns: the name of this device's md array + :rtype: str or NoneType + """ + # Value for MD_DEVNAME known to be obtained from: + # * pyudev/libudev + # * No known metadata versions for mdraid/mdadm return info.get("MD_DEVNAME")
def udev_device_get_vg_name(info):
udev_device_get_md_devices() does the conversion.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/devicetree.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/blivet/devicetree.py b/blivet/devicetree.py index 9403fc3..669e4bf 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -1044,7 +1044,7 @@ class DeviceTree(object):
kwargs["parents"] = [container] kwargs["level"] = udev.udev_device_get_md_level(info) - kwargs["memberDevices"] = int(udev.udev_device_get_md_devices(info)) + kwargs["memberDevices"] = udev.udev_device_get_md_devices(info) kwargs["uuid"] = udev.udev_device_get_md_uuid(info) kwargs["exists"] = True del kwargs["serial"] @@ -1572,7 +1572,7 @@ class DeviceTree(object): try: # level is reported as, eg: "raid1" md_level = udev.udev_device_get_md_level(info) - md_devices = int(udev.udev_device_get_md_devices(info)) + md_devices = udev.udev_device_get_md_devices(info) md_uuid = udev.udev_device_get_md_uuid(info) except (KeyError, ValueError) as e: log.warning("invalid data for %s: %s", device.name, e)
udev_device_get_md_level() does not raise a KeyError, it returns None.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/devicetree.py | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/blivet/devicetree.py b/blivet/devicetree.py index 669e4bf..57e0a47 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -1578,6 +1578,10 @@ class DeviceTree(object): log.warning("invalid data for %s: %s", device.name, e) return
+ if md_level is None: + log.warning("invalid data for %s: no RAID level", device.name) + return + md_metadata = info.get("MD_METADATA") md_name = None
Remove useless access before loop and add an explanatory comment.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/devicetree.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/blivet/devicetree.py b/blivet/devicetree.py index 57e0a47..aaa287c 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -1582,7 +1582,7 @@ class DeviceTree(object): log.warning("invalid data for %s: no RAID level", device.name) return
- md_metadata = info.get("MD_METADATA") + md_metadata = None md_name = None
# check the list of devices udev knows about to see if the array @@ -1614,8 +1614,9 @@ class DeviceTree(object):
break
- if not md_metadata: - md_metadata = info.get("METADATA", "0.90") + # mdexamine yields MD_METADATA only for metadata version > 0.90 + # if MD_METADATA is missing, assume metadata version is 0.90 + md_metadata = md_metadata or info.get("MD_METADATA", "0.90")
if not md_name: md_path = info.get("DEVICE", "")
Signed-off-by: mulhern amulhern@redhat.com --- blivet/devicetree.py | 6 ++++-- blivet/udev.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/blivet/devicetree.py b/blivet/devicetree.py index aaa287c..64aa5ac 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -1602,7 +1602,7 @@ class DeviceTree(object):
if dev_uuid == md_uuid and dev_level == md_level: md_name = udev.udev_device_get_md_name(dev) - md_metadata = dev.get("MD_METADATA") + md_metadata = udev.udev_device_get_md_metadata(dev) if not md_name: # containers don't typically have names and they also # don't have a symlink in /dev/md @@ -1616,7 +1616,9 @@ class DeviceTree(object):
# mdexamine yields MD_METADATA only for metadata version > 0.90 # if MD_METADATA is missing, assume metadata version is 0.90 - md_metadata = md_metadata or info.get("MD_METADATA", "0.90") + md_metadata = md_metadata or \ + udev.udev_device_get_md_metadata(info) or \ + "0.90"
if not md_name: md_path = info.get("DEVICE", "") diff --git a/blivet/udev.py b/blivet/udev.py index bc4e21f..d3a8259 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -434,6 +434,18 @@ def udev_device_get_md_name(info): # * No known metadata versions for mdraid/mdadm return info.get("MD_DEVNAME")
+def udev_device_get_md_metadata(info): + """ Return the metadata version number. + + :param dict info: dictionary of name-value pairs as strings + :returns: the metadata version number of the md array + :rtype: str or NoneType + """ + # Value for MD_METADATA known to be obtained from: + # * pyudev/libudev + # * mdraid/mdadm (not version numbers < 1) + return info.get("MD_METADATA") + def udev_device_get_vg_name(info): return info['LVM2_VG_NAME']
On Thu, 2014-07-03 at 11:19 -0400, mulhern wrote:
Signed-off-by: mulhern amulhern@redhat.com
blivet/devicetree.py | 6 ++++-- blivet/udev.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/blivet/devicetree.py b/blivet/devicetree.py index aaa287c..64aa5ac 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -1602,7 +1602,7 @@ class DeviceTree(object):
if dev_uuid == md_uuid and dev_level == md_level: md_name = udev.udev_device_get_md_name(dev)
md_metadata = dev.get("MD_METADATA")
md_metadata = udev.udev_device_get_md_metadata(dev) if not md_name: # containers don't typically have names and they also # don't have a symlink in /dev/md
@@ -1616,7 +1616,9 @@ class DeviceTree(object):
# mdexamine yields MD_METADATA only for metadata version > 0.90 # if MD_METADATA is missing, assume metadata version is 0.90
md_metadata = md_metadata or info.get("MD_METADATA", "0.90")
md_metadata = md_metadata or \
udev.udev_device_get_md_metadata(info) or \
"0.90" if not md_name: md_path = info.get("DEVICE", "")
diff --git a/blivet/udev.py b/blivet/udev.py index bc4e21f..d3a8259 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -434,6 +434,18 @@ def udev_device_get_md_name(info): # * No known metadata versions for mdraid/mdadm return info.get("MD_DEVNAME")
+def udev_device_get_md_metadata(info):
- """ Return the metadata version number.
:param dict info: dictionary of name-value pairs as strings
:returns: the metadata version number of the md array
:rtype: str or NoneType
- """
- # Value for MD_METADATA known to be obtained from:
- # * pyudev/libudev
- # * mdraid/mdadm (not version numbers < 1)
- return info.get("MD_METADATA")
def udev_device_get_vg_name(info): return info['LVM2_VG_NAME']
A similar note here inspired by a new udev_ function added to the udev module -- could we remove the 'udev_' prefix from all those functions? It made sense when there were * imports from the module, but it's just pain now.
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Friday, July 4, 2014 3:03:14 AM Subject: Re: [blivet:master 08/11] Add a udev_device_get_md_metadata() method to udev and use it.
On Thu, 2014-07-03 at 11:19 -0400, mulhern wrote:
Signed-off-by: mulhern amulhern@redhat.com
blivet/devicetree.py | 6 ++++-- blivet/udev.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/blivet/devicetree.py b/blivet/devicetree.py index aaa287c..64aa5ac 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -1602,7 +1602,7 @@ class DeviceTree(object):
if dev_uuid == md_uuid and dev_level == md_level: md_name = udev.udev_device_get_md_name(dev)
md_metadata = dev.get("MD_METADATA")
md_metadata = udev.udev_device_get_md_metadata(dev) if not md_name: # containers don't typically have names and they also # don't have a symlink in /dev/md
@@ -1616,7 +1616,9 @@ class DeviceTree(object):
# mdexamine yields MD_METADATA only for metadata version > 0.90 # if MD_METADATA is missing, assume metadata version is 0.90
md_metadata = md_metadata or info.get("MD_METADATA", "0.90")
md_metadata = md_metadata or \
udev.udev_device_get_md_metadata(info) or \
"0.90" if not md_name: md_path = info.get("DEVICE", "")
diff --git a/blivet/udev.py b/blivet/udev.py index bc4e21f..d3a8259 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -434,6 +434,18 @@ def udev_device_get_md_name(info): # * No known metadata versions for mdraid/mdadm return info.get("MD_DEVNAME")
+def udev_device_get_md_metadata(info):
- """ Return the metadata version number.
:param dict info: dictionary of name-value pairs as strings
:returns: the metadata version number of the md array
:rtype: str or NoneType
- """
- # Value for MD_METADATA known to be obtained from:
- # * pyudev/libudev
- # * mdraid/mdadm (not version numbers < 1)
- return info.get("MD_METADATA")
def udev_device_get_vg_name(info): return info['LVM2_VG_NAME']
A similar note here inspired by a new udev_ function added to the udev module -- could we remove the 'udev_' prefix from all those functions? It made sense when there were * imports from the module, but it's just pain now.
-- Vratislav Podzimek
Anaconda Rider | RHCE | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Unless anybody objects I'll go ahead and add a patch for this as well.
It will affect around 13 files and 350 lines.
Pros: more pythonic and less verbose naming scheme Cons: a lot of lines changed for a mostly cosmetic change
- mulhern
On Mon, 2014-07-07 at 09:04 -0400, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Friday, July 4, 2014 3:03:14 AM Subject: Re: [blivet:master 08/11] Add a udev_device_get_md_metadata() method to udev and use it.
On Thu, 2014-07-03 at 11:19 -0400, mulhern wrote:
Signed-off-by: mulhern amulhern@redhat.com
blivet/devicetree.py | 6 ++++-- blivet/udev.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/blivet/devicetree.py b/blivet/devicetree.py index aaa287c..64aa5ac 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -1602,7 +1602,7 @@ class DeviceTree(object):
if dev_uuid == md_uuid and dev_level == md_level: md_name = udev.udev_device_get_md_name(dev)
md_metadata = dev.get("MD_METADATA")
md_metadata = udev.udev_device_get_md_metadata(dev) if not md_name: # containers don't typically have names and they also # don't have a symlink in /dev/md
@@ -1616,7 +1616,9 @@ class DeviceTree(object):
# mdexamine yields MD_METADATA only for metadata version > 0.90 # if MD_METADATA is missing, assume metadata version is 0.90
md_metadata = md_metadata or info.get("MD_METADATA", "0.90")
md_metadata = md_metadata or \
udev.udev_device_get_md_metadata(info) or \
"0.90" if not md_name: md_path = info.get("DEVICE", "")
diff --git a/blivet/udev.py b/blivet/udev.py index bc4e21f..d3a8259 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -434,6 +434,18 @@ def udev_device_get_md_name(info): # * No known metadata versions for mdraid/mdadm return info.get("MD_DEVNAME")
+def udev_device_get_md_metadata(info):
- """ Return the metadata version number.
:param dict info: dictionary of name-value pairs as strings
:returns: the metadata version number of the md array
:rtype: str or NoneType
- """
- # Value for MD_METADATA known to be obtained from:
- # * pyudev/libudev
- # * mdraid/mdadm (not version numbers < 1)
- return info.get("MD_METADATA")
def udev_device_get_vg_name(info): return info['LVM2_VG_NAME']
A similar note here inspired by a new udev_ function added to the udev module -- could we remove the 'udev_' prefix from all those functions? It made sense when there were * imports from the module, but it's just pain now.
-- Vratislav Podzimek
Anaconda Rider | RHCE | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Unless anybody objects I'll go ahead and add a patch for this as well.
Thanks!
It will affect around 13 files and 350 lines.
Pros: more pythonic and less verbose naming scheme Cons: a lot of lines changed for a mostly cosmetic change
I think the Pros outweigh the Cons.
Related: rhbz#1070095
Examined alternatives are: 1) To make use of the uuid.UUID class to store uuids. The difficulty with this idea is that some things that are labeled uuids are not proper uuids at all. 2) To make the udev_info dict more object oriented, and have it sanitize uuids when they are added. A lot of work for very limited benefit. 3) To canonicalize uuids everytime they are extracted from the info. Brittle, since you can't stop code from accessing the entries directly, and annoyingly inefficient.
This alternative is pretty simple, and to a certain extent it's a workaround for a bug in mdadm, so it wins.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/devicelibs/mdraid.py | 9 +++++++++ tests/devicelibs_test/mdraid_test.py | 6 ++++++ 2 files changed, 15 insertions(+)
diff --git a/blivet/devicelibs/mdraid.py b/blivet/devicelibs/mdraid.py index f46571a..31e5b80 100644 --- a/blivet/devicelibs/mdraid.py +++ b/blivet/devicelibs/mdraid.py @@ -242,6 +242,15 @@ def mdexamine(device): if name == "metadata": info["MD_METADATA"] = value
+ # mdadm's UUIDs are actual 128 bit uuids, but it formats them strangely. + # This converts the uuids to canonical form. + # Example: + # mdadm UUID: '3386ff85:f5012621:4a435f06:1eb47236' + # canonical UUID: '3386ff85-f501-2621-4a43-5f061eb47236' + import uuid + for k, v in ((k,v) for (k,v) in info.iteritems() if k.endswith("UUID")): + info[k] = str(uuid.UUID(v.replace(':', ''))) + return info
def md_node_from_name(name): diff --git a/tests/devicelibs_test/mdraid_test.py b/tests/devicelibs_test/mdraid_test.py index 760643f..4f2e6d8 100755 --- a/tests/devicelibs_test/mdraid_test.py +++ b/tests/devicelibs_test/mdraid_test.py @@ -1,6 +1,7 @@ #!/usr/bin/python import unittest import time +import uuid
import blivet.devicelibs.raid as raid import blivet.devicelibs.mdraid as mdraid @@ -119,6 +120,7 @@ class MDRaidAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): Verifies that: - exactly the predicted names are returned by mdexamine - RAID level and number of devices are correct + - UUIDs have canonical form """ level = mdraid.RAID_levels.raidLevel(level or raid.RAID1) mdraid.mdcreate(self._dev_name, level, self.loopDevices, metadataVer=metadataVersion) @@ -138,6 +140,10 @@ class MDRaidAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): self.assertEqual(info['MD_DEVICES'], '2') self.assertEqual(info['MD_LEVEL'], str(level))
+ # verify that uuids are in canonical form + for name in (k for k in iter(info.keys()) if k.endswith('UUID')): + self.assertTrue(str(uuid.UUID(info[name])) == info[name]) + def testMDExamineContainerDefault(self): self._testMDExamine(self.names_container, level="container")
On Thu, 2014-07-03 at 11:19 -0400, mulhern wrote:
Related: rhbz#1070095
Examined alternatives are:
- To make use of the uuid.UUID class to store uuids. The difficulty
with this idea is that some things that are labeled uuids are not proper uuids at all. 2) To make the udev_info dict more object oriented, and have it sanitize uuids when they are added. A lot of work for very limited benefit. 3) To canonicalize uuids everytime they are extracted from the info. Brittle, since you can't stop code from accessing the entries directly, and annoyingly inefficient.
This alternative is pretty simple, and to a certain extent it's a workaround for a bug in mdadm, so it wins.
Signed-off-by: mulhern amulhern@redhat.com
blivet/devicelibs/mdraid.py | 9 +++++++++ tests/devicelibs_test/mdraid_test.py | 6 ++++++ 2 files changed, 15 insertions(+)
diff --git a/blivet/devicelibs/mdraid.py b/blivet/devicelibs/mdraid.py index f46571a..31e5b80 100644 --- a/blivet/devicelibs/mdraid.py +++ b/blivet/devicelibs/mdraid.py @@ -242,6 +242,15 @@ def mdexamine(device): if name == "metadata": info["MD_METADATA"] = value
- # mdadm's UUIDs are actual 128 bit uuids, but it formats them strangely.
- # This converts the uuids to canonical form.
- # Example:
- # mdadm UUID: '3386ff85:f5012621:4a435f06:1eb47236'
- # canonical UUID: '3386ff85-f501-2621-4a43-5f061eb47236'
- import uuid
Any reason for the local import? If yes, it should either be fixed or documented.
Related: rhbz#1070095
Note that the value is only obtained from mdadm and only if recent metadata version is used.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/udev.py | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/blivet/udev.py b/blivet/udev.py index d3a8259..8552f43 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -446,6 +446,18 @@ def udev_device_get_md_metadata(info): # * mdraid/mdadm (not version numbers < 1) return info.get("MD_METADATA")
+def udev_device_get_md_device_uuid(info): + """ Returns the uuid of a device which is a member of an md array. + + :param dict info: dictionary of name-value pairs as strings + :returns: the uuid of this device (which is a member of an md array) + :rtype: str or NoneType + """ + # Value for MD_UUID known to be obtained from: + # * pyudev/libudev + # * mdraid/mdadm (only 1.x metadata versions) + return info.get('MD_DEV_UUID') + def udev_device_get_vg_name(info): return info['LVM2_VG_NAME']
Resolves: rhbz#1070095
Note that this information is only available for metadata versions > 0.90.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/devicetree.py | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/blivet/devicetree.py b/blivet/devicetree.py index 64aa5ac..ac355b5 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -1825,6 +1825,11 @@ class DeviceTree(object): kwargs["mdUuid"] = udev.udev_device_get_md_uuid(info) except KeyError: log.warning("mdraid member %s has no md uuid", name) + + # attempt to reset the uuid using mdexamine info + # will succeed only if metadata version > 0.90 + kwargs["uuid"] = udev.udev_device_get_md_device_uuid(info) or kwargs["uuid"] + kwargs["biosraid"] = udev.udev_device_is_biosraid_member(info) elif format_type == "LVM2_member": # lvm
On Thu, 2014-07-03 at 11:19 -0400, mulhern wrote:
I realized that I had been mistaken about somethings, and I noticed a few other things that ought to be fixed up.
First two patches are just testing. The next two are just documentation. The next two are just using udev_device_get_md_*() methods according to their spec. The next two are a very minor bug fix and some accompanying stuff. The final three are the bug fix. Canoncalize... is unchanged, the rest are changed a bit.
mulhern (11): Add test for raid level descriptor None. Add a few small tests for mdexamine Document udev_device_get_uuid() method. Add documentation to udev_device_get_md_*() methods. Do not convert the result of udev_device_get_md_devices() to int. Check for md_level of None Change use of METADATA to MD_METADATA. Add a udev_device_get_md_metadata() method to udev and use it. Canonicalize mdadm generated UUIDS (#1070095) Add new method udev_device_get_md_device_uuid() method (#1070095) Attempt to reset the uuid of the mdraid member device (#1070095)
blivet/devicelibs/mdraid.py | 9 ++++ blivet/devicetree.py | 24 ++++++--- blivet/udev.py | 75 ++++++++++++++++++++++++++- tests/devicelibs_test/mdraid_test.py | 98 +++++++++++++++++++++++++++++++++++- tests/devicelibs_test/raid_test.py | 1 + 5 files changed, 198 insertions(+), 9 deletions(-)
Other than those minor comments these look good to me.
anaconda-patches@lists.fedorahosted.org