The first two are very minor cleanups. The third is tests that pass. The remaining four address two related bugs. * mdadm formats UUIDs funny, we store UUIDs as strings just as they are obtained by our tools, so lookup may fail where it should succeed, because the format of the lookup UUID doesn't match the format with which it was stored. * We're making MDRaidMember format objects where the UUID of the device and the UUID of the array of which it is a member are the same, which is wrong.
mulhern (7): Remove unnecessary fanciness about importing devices. Do not use udev info to get the name of the device. Add a few small tests for mdexamine Add a method to get the uuid of an md array member (#1070095) Canonicalize mdadm generated UUIDS (#1070095) Reset the uuid of the mdraid member device from mdexamine info (#1070095) Remove always succeeding check from udev_device_get_uuid() (#1070095)
blivet/devicelibs/mdraid.py | 9 +++++++++ blivet/devicetree.py | 11 ++++++++--- blivet/udev.py | 17 ++++++++--------- tests/devicelibs_test/mdraid_test.py | 24 ++++++++++++++++++++++++ 4 files changed, 49 insertions(+), 12 deletions(-)
The name will go away when the function is exited, it does not need to be explicitly deleted. The name does not conflict with any other names in the method, to which it is local. There are no global 'devices'.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/udev.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/blivet/udev.py b/blivet/udev.py index c670e26..a1ff007 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -99,7 +99,7 @@ def udev_resolve_devspec(devspec): if not devspec: return None
- from . import devices as _devices + from . import devices ret = None for dev in udev_get_block_devices(): if devspec.startswith("LABEL="): @@ -110,7 +110,7 @@ def udev_resolve_devspec(devspec): if udev_device_get_uuid(dev) == devspec[5:]: ret = dev break - elif udev_device_get_name(dev) == _devices.devicePathToName(devspec): + elif udev_device_get_name(dev) == devices.devicePathToName(devspec): ret = dev break else: @@ -123,7 +123,6 @@ def udev_resolve_devspec(devspec): ret = dev break
- del _devices if ret: return udev_device_get_name(ret)
On Tue, 2014-06-24 at 13:04 -0400, mulhern wrote:
The name will go away when the function is exited, it does not need to be explicitly deleted. The name does not conflict with any other names in the method, to which it is local. There are no global 'devices'.
Signed-off-by: mulhern amulhern@redhat.com
blivet/udev.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/blivet/udev.py b/blivet/udev.py index c670e26..a1ff007 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -99,7 +99,7 @@ def udev_resolve_devspec(devspec): if not devspec: return None
- from . import devices as _devices
- from . import devices
Do we need to do this import locally in the first place? If not, we should move it to the global scope, if yes, there should be a comment explaining why.
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, June 25, 2014 1:52:40 AM Subject: Re: [blivet:master 1/7] Remove unnecessary fanciness about importing devices.
On Tue, 2014-06-24 at 13:04 -0400, mulhern wrote:
The name will go away when the function is exited, it does not need to be explicitly deleted. The name does not conflict with any other names in the method, to which it is local. There are no global 'devices'.
Signed-off-by: mulhern amulhern@redhat.com
blivet/udev.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/blivet/udev.py b/blivet/udev.py index c670e26..a1ff007 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -99,7 +99,7 @@ def udev_resolve_devspec(devspec): if not devspec: return None
- from . import devices as _devices
- from . import devices
Do we need to do this import locally in the first place? If not, we should move it to the global scope, if yes, there should be a comment explaining why.
-- 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
Needed to avoid an import cycle. Done.
- mulhern
This name is only used in the warning, and device.name seems like a better choice.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/devicetree.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/blivet/devicetree.py b/blivet/devicetree.py index 98f34f3..591583f 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -1572,8 +1572,6 @@ class DeviceTree(object):
def handleUdevMDMemberFormat(self, info, device): log_method_call(self, name=device.name, type=device.format.type) - # either look up or create the array device - name = udev.udev_device_get_name(info)
md_array = self.getDeviceByUuid(device.format.mdUuid, incomplete=True) if device.format.mdUuid and md_array: @@ -1586,7 +1584,7 @@ class DeviceTree(object): md_devices = int(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", name, e) + log.warning("invalid data for %s: %s", device.name, e) return
md_metadata = info.get("MD_METADATA")
Signed-off-by: mulhern amulhern@redhat.com --- tests/devicelibs_test/mdraid_test.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/tests/devicelibs_test/mdraid_test.py b/tests/devicelibs_test/mdraid_test.py index cba8487..74dded7 100755 --- a/tests/devicelibs_test/mdraid_test.py +++ b/tests/devicelibs_test/mdraid_test.py @@ -66,6 +66,11 @@ class MDRaidAsRootTestCase(loopbackedtestcase.LoopBackedTestCase):
super(MDRaidAsRootTestCase, self).tearDown()
+ def testNonMDRaid(self): + # invoking mdexamine on a device that is not an array member yields {} + info = mdraid.mdexamine(self.loopDevices[0]) + self.assertEqual(info, {}) + def testMDRaidAsRoot(self): ## ## mdcreate @@ -75,6 +80,20 @@ class MDRaidAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): # wait for raid to settle time.sleep(2)
+ info = mdraid.mdexamine(self._dev_name) + self.assertEqual(info, {}) + + info = mdraid.mdexamine(self.loopDevices[0]) + self.assertEqual(info['MD_DEVICES'], '2') + self.assertEqual(info['MD_LEVEL'], 'raid1') + self.assertTrue('DEVICE' in info) + self.assertTrue('MD_EVENTS' in info) + self.assertTrue('MD_DEV_UUID' in info) + self.assertTrue('MD_METADATA' in info) + self.assertTrue('MD_NAME' in info) + self.assertTrue('MD_UPDATE_TIME' in info) + self.assertTrue('MD_UUID' in info) + # fail self.assertRaises(MDRaidError, mdraid.mdcreate, "/dev/md1", "raid1", ["/not/existing/dev0", "/not/existing/dev1"])
On Tue, 2014-06-24 at 13:04 -0400, mulhern wrote:
Signed-off-by: mulhern amulhern@redhat.com
tests/devicelibs_test/mdraid_test.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/tests/devicelibs_test/mdraid_test.py b/tests/devicelibs_test/mdraid_test.py index cba8487..74dded7 100755 --- a/tests/devicelibs_test/mdraid_test.py +++ b/tests/devicelibs_test/mdraid_test.py @@ -66,6 +66,11 @@ class MDRaidAsRootTestCase(loopbackedtestcase.LoopBackedTestCase):
super(MDRaidAsRootTestCase, self).tearDown()
- def testNonMDRaid(self):
# invoking mdexamine on a device that is not an array member yields {}info = mdraid.mdexamine(self.loopDevices[0])self.assertEqual(info, {})- def testMDRaidAsRoot(self): ## ## mdcreate
@@ -75,6 +80,20 @@ class MDRaidAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): # wait for raid to settle time.sleep(2)
info = mdraid.mdexamine(self._dev_name)self.assertEqual(info, {})info = mdraid.mdexamine(self.loopDevices[0])self.assertEqual(info['MD_DEVICES'], '2')self.assertEqual(info['MD_LEVEL'], 'raid1')self.assertTrue('DEVICE' in info)self.assertTrue('MD_EVENTS' in info)self.assertTrue('MD_DEV_UUID' in info)self.assertTrue('MD_METADATA' in info)self.assertTrue('MD_NAME' in info)self.assertTrue('MD_UPDATE_TIME' in info)self.assertTrue('MD_UUID' in info)
There's also self.assertIn() so we might use it here.
Related: rhbz#1070095
Signed-off-by: mulhern amulhern@redhat.com --- blivet/udev.py | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/blivet/udev.py b/blivet/udev.py index a1ff007..126fbf9 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -375,8 +375,13 @@ def udev_device_get_md_devices(info): return int(info["MD_DEVICES"])
def udev_device_get_md_uuid(info): + """ The uuid of the array of which this device is a member. """ return info["MD_UUID"]
+def udev_device_get_md_device_uuid(info): + """ The uuid of the device, not of the array of which it is a member. """ + return info['MD_DEV_UUID'] + def udev_device_get_md_container(info): return info.get("MD_CONTAINER")
On Tue, Jun 24, 2014 at 01:04:25PM -0400, mulhern wrote:
Related: rhbz#1070095
Signed-off-by: mulhern amulhern@redhat.com
blivet/udev.py | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/blivet/udev.py b/blivet/udev.py index a1ff007..126fbf9 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -375,8 +375,13 @@ def udev_device_get_md_devices(info): return int(info["MD_DEVICES"])
def udev_device_get_md_uuid(info):
- """ The uuid of the array of which this device is a member. """ return info["MD_UUID"]
+def udev_device_get_md_device_uuid(info):
- """ The uuid of the device, not of the array of which it is a member. """
- return info['MD_DEV_UUID']
def udev_device_get_md_container(info): return info.get("MD_CONTAINER")
-- 1.9.3
Since you're changing them you may as well make them complete Sphinx docstrings.
----- Original Message -----
From: "Brian C. Lane" bcl@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Tuesday, June 24, 2014 9:16:13 PM Subject: Re: [blivet:master 4/7] Add a method to get the uuid of an md array member (#1070095)
On Tue, Jun 24, 2014 at 01:04:25PM -0400, mulhern wrote:
Related: rhbz#1070095
Signed-off-by: mulhern amulhern@redhat.com
blivet/udev.py | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/blivet/udev.py b/blivet/udev.py index a1ff007..126fbf9 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -375,8 +375,13 @@ def udev_device_get_md_devices(info): return int(info["MD_DEVICES"])
def udev_device_get_md_uuid(info):
- """ The uuid of the array of which this device is a member. """ return info["MD_UUID"]
+def udev_device_get_md_device_uuid(info):
- """ The uuid of the device, not of the array of which it is a member.
"""
- return info['MD_DEV_UUID']
def udev_device_get_md_container(info): return info.get("MD_CONTAINER")
-- 1.9.3
Since you're changing them you may as well make them complete Sphinx docstrings.
-- Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT) _______________________________________________ anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Done.
- mulhern
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 | 5 +++++ 2 files changed, 14 insertions(+)
diff --git a/blivet/devicelibs/mdraid.py b/blivet/devicelibs/mdraid.py index f46571a..ae500a4 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(''.join(v.split(':')))) + 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 74dded7..28a866b 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 @@ -94,6 +95,10 @@ class MDRaidAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): self.assertTrue('MD_UPDATE_TIME' in info) self.assertTrue('MD_UUID' in info)
+ # verify that uuids are in canonical form + self.assertTrue(str(uuid.UUID(info['MD_UUID'])) == info['MD_UUID']) + self.assertTrue(str(uuid.UUID(info['MD_DEV_UUID'])) == info['MD_DEV_UUID']) + # fail self.assertRaises(MDRaidError, mdraid.mdcreate, "/dev/md1", "raid1", ["/not/existing/dev0", "/not/existing/dev1"])
On Tue, 2014-06-24 at 13:04 -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 | 5 +++++ 2 files changed, 14 insertions(+)
diff --git a/blivet/devicelibs/mdraid.py b/blivet/devicelibs/mdraid.py index f46571a..ae500a4 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")):
This almost looks like a candidate for PEP to enable things like: for k, v in info.iteritems() if k.endswith("UUID"): directly. Thanks for using the generator and generating method of info.
info[k] = str(uuid.UUID(''.join(v.split(':'))))
I believe str(uuid.UUID(v.replace(":", "")) would be better readable.
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, June 25, 2014 2:01:15 AM Subject: Re: [blivet:master 5/7] Canonicalize mdadm generated UUIDS (#1070095)
On Tue, 2014-06-24 at 13:04 -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 | 5 +++++ 2 files changed, 14 insertions(+)
diff --git a/blivet/devicelibs/mdraid.py b/blivet/devicelibs/mdraid.py index f46571a..ae500a4 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")):
This almost looks like a candidate for PEP to enable things like: for k, v in info.iteritems() if k.endswith("UUID"): directly. Thanks for using the generator and generating method of info.
info[k] = str(uuid.UUID(''.join(v.split(':'))))I believe str(uuid.UUID(v.replace(":", "")) would be 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
Done.
- mulhern
Resolves: rhbz#1070095
Surround with a try/catch block because the code that obtains the uuid for the array, just above this code, uses a try/catch block, and I believe that the likelihood that the device uuid is unavailable is the same as the likelihood that the array uuid is unavailable.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/devicetree.py | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/blivet/devicetree.py b/blivet/devicetree.py index 591583f..8d960f5 100644 --- a/blivet/devicetree.py +++ b/blivet/devicetree.py @@ -1827,6 +1827,13 @@ class DeviceTree(object): kwargs["mdUuid"] = udev.udev_device_get_md_uuid(info) except KeyError: log.warning("mdraid member %s has no md uuid", name) + + # reset the uuid using information obtained from mdexamine + try: + kwargs["uuid"] = udev.udev_device_get_md_device_uuid(info) + except KeyError: + log.warning("mdraid member %s has no device uuid", name) + kwargs["biosraid"] = udev.udev_device_is_biosraid_member(info) elif format_type == "LVM2_member": # lvm
Related: rhbz#1070095
This function is always called before mdexamine is called for the device. This means that MD_UUID will never be set, and so the condition, if checked, will always succeed. Note that the regular expression in the condition was used to address the issue of oddly formatted UUIDs from mdadm, now addressed in a previous patch.
Expect that the previous patches will ensure that the uuid of the device is reset correctly if the device is a member of an md array.
An alternative to some of the previous patches is to cause udev_get_device() to invoke mdexamine, either unconditionally, or if ID_FS_TYPE indicates that the device is an mdraid member. This would allow avoiding invoking mdexamine anywhere else. One obvious drawback is that we would wish to be consistent and do something similar for, e.g., lvm info, but that data has been buttressed about with a lot of cacheing, because it is expensive to get, and mdexamine probably suffers from a similar expensiveness. Meanwhile, udev_get_device() is called whenever udev_get_block_device() is called, which is whenever, udev_get_block_devices() is called which is really quite frequently.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/udev.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/blivet/udev.py b/blivet/udev.py index 126fbf9..343a8bc 100644 --- a/blivet/udev.py +++ b/blivet/udev.py @@ -207,12 +207,7 @@ def udev_device_get_format(udev_info):
def udev_device_get_uuid(udev_info): """ Get the UUID from the device's format as reported by udev. """ - 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 - if len(uuid) > 0 and \ - re.sub(r'\W', '', md_uuid) != re.sub(r'\W', '', uuid): - return udev_info.get("ID_FS_UUID") + return udev_info.get("ID_FS_UUID")
def udev_device_get_label(udev_info): """ Get the label from the device's format as reported by udev. """
On Tue, 2014-06-24 at 13:04 -0400, mulhern wrote:
The first two are very minor cleanups. The third is tests that pass. The remaining four address two related bugs.
- mdadm formats UUIDs funny, we store UUIDs as strings just as they are
obtained by our tools, so lookup may fail where it should succeed, because the format of the lookup UUID doesn't match the format with which it was stored.
- We're making MDRaidMember format objects where the UUID of the device
and the UUID of the array of which it is a member are the same, which is wrong.
mulhern (7): Remove unnecessary fanciness about importing devices. Do not use udev info to get the name of the device. Add a few small tests for mdexamine Add a method to get the uuid of an md array member (#1070095) Canonicalize mdadm generated UUIDS (#1070095) Reset the uuid of the mdraid member device from mdexamine info (#1070095) Remove always succeeding check from udev_device_get_uuid() (#1070095)
blivet/devicelibs/mdraid.py | 9 +++++++++ blivet/devicetree.py | 11 ++++++++--- blivet/udev.py | 17 ++++++++--------- tests/devicelibs_test/mdraid_test.py | 24 ++++++++++++++++++++++++ 4 files changed, 49 insertions(+), 12 deletions(-)
Other than those per-patch comments this looks good to me.
anaconda-patches@lists.fedorahosted.org
-
Anne Mulhern -
Brian C. Lane -
Vratislav Podzimek