Merge lp:~raharper/curtin/fix-lp1531520 into lp:~curtin-dev/curtin/trunk

[Ryan Harper]

mdadm: fix issues exposed by use via block_meta

Using mdadm via block_meta exposed a few issues.
 - block_meta calls block.md_check, this should be mdadm.md_check
 - mdadm.sysfs_read_attr had thinko in check if it should attempt to read
   an attribute and avoiding reading files
 - mdadm.sysfs_read_attr should use util.load_file
 - ensure all entrypoints to mdadm code check that device names are device paths.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:337
https://server-team-jenkins.canonical.com/job/curtin-ci/95/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/92/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/95/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

I'm surprised / disapppointed that pyflakes didn't catch the curtin.block / curtin.mdadm issue.
pylint does:
% pylint curtin/commands/block_meta.py | grep ^E
No config file found, using default configuration
E:919,15: Module 'curtin.block' has no 'md_check' member (no-member)

[Ryan Harper (raharper)]

On Thu, Jan 7, 2016 at 8:59 AM, Scott Moser <email address hidden> wrote:

> I'm surprised / disapppointed that pyflakes didn't catch the curtin.block
> / curtin.mdadm issue.
> pylint does:
> % pylint curtin/commands/block_meta.py | grep ^E
> No config file found, using default configuration
> E:919,15: Module 'curtin.block' has no 'md_check' member (no-member)
>

Same here.

>
>
> Diff comments:
>
> > === modified file 'curtin/block/mdadm.py'
> > --- curtin/block/mdadm.py 2016-01-04 16:24:45 +0000
> > +++ curtin/block/mdadm.py 2016-01-06 22:32:40 +0000
> > @@ -201,16 +208,16 @@
> >
> >
> > def mdadm_stop(devpath):
> > - if not devpath:
> > - raise ValueError('mdadm_stop: missing parameter devpath')
> > + if not valid_devpath(devpath):
> > + raise ValueError('Invalid devpath', devpath)
>
> Is there a reason:
> raise ValueError('Invalid devpath', devpath)
> rather than:
> raise ValueError('Invalid devpath: %s' % devpath)
>

Yeah, I suppose we should have a common style, but the parameter produces
better output because
we're not string formatting.

 >>> raise ValueError('My Error %s' % '')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: My Error

Empty variable output is hidden, but if we pass it as parameter not only
are we typing less but it's quoted properly

>>> raise ValueError('My Error', '')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: ('My Error', '')

> The difference can be seen here
> >>> v1 = ValueError('Invalid devpath', devpath)
> >>> v1.message
>
>

In python3, my ValueError doesn't have a .message

but it does have args

>>> v1.args
('Invalid devpath', 'foo')

> >>> str(v1)
> "('Invalid devpath', 'foo')"
>
> Versus:
> >>> v2 = ValueError('Invalid devpath: %s' % devpath)
> >>> v2.message
> 'Invalid devpath: foo'
> >>> str(v2)
> 'Invalid devpath: foo'
>

str(v1)
"('Invalid devpath', 'foo')"

>
> >
> > LOG.info("mdadm stopping: %s" % devpath)
> > util.subp(["mdadm", "--stop", devpath], rcs=[0, 1], capture=True)
> >
> >
> > def mdadm_remove(devpath):
> > - if not devpath:
> > - raise ValueError('mdadm_remove: missing parameter devpath')
> > + if not valid_devpath(devpath):
> > + raise ValueError('Invalid devpath', devpath)
> >
> > LOG.info("mdadm removing: %s" % devpath)
> > util.subp(["mdadm", "--remove", devpath], rcs=[0, 1], capture=True)
> > @@ -469,6 +483,9 @@
> >
> >
> > def md_get_devices_list(devpath):
> > + if not valid_devpath(devpath):
> > + raise ValueError('Invalid devpath: [{}]'.format(devpath))
>
> then you changed to another message here...
>

I missed the conversion. Thanks,

>
> maybe you could just:
> def assert_valid_devpath(devpath):
> if not valid_devpath(devpath):
> raise ValueError('Invalid devpath: %s' % devpath)
>
> then just use the one line:
> assert_valid_devpath(devpath)
>
> > +
> > sysfs_md = sys_block_path(devpath) + '/md'
> > if not os.path.exists(sysfs_md):
> > raise ValueError('Cannot find md sysfs directory: ' +
>
>
> --
> https://code.launchpad.net/~raharper/curtin/fix-lp1531...

[lp:~raharper/curtin/fix-lp1531520]

338. By Ryan Harper on 2016-01-07

Use common message formatting when raising ValueError exceptions

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:338
https://server-team-jenkins.canonical.com/job/curtin-ci/96/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/93/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/96/rebuild

review: Approve (continuous-integration)

[lp:~raharper/curtin/fix-lp1531520]

339. By Ryan Harper on 2016-01-07

Introduce assert_valid_devpath

Reduce duplicate code of raising ValueError when provided an invalid devpath.

340. By Ryan Harper on 2016-01-07

from trunk

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:340
https://server-team-jenkins.canonical.com/job/curtin-ci/98/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/95/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/98/rebuild

review: Approve (continuous-integration)

=== modified file 'curtin/block/mdadm.py'
--- curtin/block/mdadm.py	2016-01-04 16:24:45 +0000
+++ curtin/block/mdadm.py	2016-01-07 17:16:36 +0000
@@ -126,7 +126,7 @@
         cmd += ['--scan']
     else:
         valid_mdname(md_devname)
-        cmd += [dev_path(md_devname), "--run"] + devices
+        cmd += [md_devname, "--run"] + devices
         if spares:
             cmd += spares
 
@@ -139,8 +139,10 @@
               'md_name=%s raidlevel=%s ' % (md_devname, raidlevel) +
               ' devices=%s spares=%s name=%s' % (devices, spares, md_name))
 
+    assert_valid_devpath(md_devname)
+
     if raidlevel not in VALID_RAID_LEVELS:
-        raise ValueError('Invalid raidlevel: ' + str(raidlevel))
+        raise ValueError('Invalid raidlevel: [{}]'.format(raidlevel))
 
     min_devices = md_minimum_devices(raidlevel)
     if len(devices) < min_devices:
@@ -152,7 +154,7 @@
         err = ('Raidlevel does not support spare devices: ' + str(raidlevel))
         raise ValueError(err)
 
-    cmd = ["mdadm", "--create", dev_path(md_devname), "--run",
+    cmd = ["mdadm", "--create", md_devname, "--run",
            "--level=%s" % raidlevel, "--raid-devices=%s" % len(devices)]
     if md_name:
         cmd.append("--name=%s" % md_name)
@@ -174,13 +176,16 @@
     util.subp(["udevadm", "control", "--stop-exec-queue"])
     util.subp(cmd, capture=True)
     util.subp(["udevadm", "control", "--start-exec-queue"])
-    util.subp(["udevadm", "settle", "--exit-if-exists=%s" % md_devname])
+    util.subp(["udevadm", "settle",
+               "--exit-if-exists=%s" % md_devname])
 
 
 def mdadm_examine(devpath, export=MDADM_USE_EXPORT):
     ''' exectute mdadm --examine, and optionally
         append --export.
         Parse and return dict of key=val from output'''
+    assert_valid_devpath(devpath)
+
     cmd = ["mdadm", "--examine"]
     if export:
         cmd.extend(["--export"])
@@ -201,16 +206,14 @@
 
 
 def mdadm_stop(devpath):
-    if not devpath:
-        raise ValueError('mdadm_stop: missing parameter devpath')
+    assert_valid_devpath(devpath)
 
     LOG.info("mdadm stopping: %s" % devpath)
     util.subp(["mdadm", "--stop", devpath], rcs=[0, 1], capture=True)
 
 
 def mdadm_remove(devpath):
-    if not devpath:
-        raise ValueError('mdadm_remove: missing parameter devpath')
+    assert_valid_devpath(devpath)
 
     LOG.info("mdadm removing: %s" % devpath)
     util.subp(["mdadm", "--remove", devpath], rcs=[0, 1], capture=True)
@@ -241,21 +244,29 @@
 
 # ------------------------------ #
 def valid_mdname(md_devname):
-    if md_devname is None:
-        raise ValueError('Parameter: md_devname is None')
-        return False
+    assert_valid_devpath(md_devname)
 
-    if not is_valid_device(dev_path(md_devname)):
-        raise ValueError('Specified md device does not exist: ' +
-                         dev_path(md_devname))
+    if not is_valid_device(md_devname):
+        raise ValueError('Specified md device does not exist: ' + md_devname)
         return False
 
     return True
 
 
+def valid_devpath(devpath):
+    if devpath:
+        return devpath.startswith('/dev')
+    return False
+
+
+def assert_valid_devpath(devpath):
+    if not valid_devpath(devpath):
+        raise ValueError('Invalid devpath: %s' % devpath)
+
+
 def md_sysfs_attr(md_devname, attrname):
     if not valid_mdname(md_devname):
-        raise ValueError('Invalid md devicename')
+        raise ValueError('Invalid md devicename: [{}]'.format(md_devname))
 
     attrdata = ''
     #  /sys/class/block/<md_short>/md
@@ -263,9 +274,8 @@
 
     #  /sys/class/block/<md_short>/md/attrname
     sysfs_attr_path = os.path.join(sysmd, attrname)
-    if not os.path.isfile(sysfs_attr_path):
-        with open(sysfs_attr_path) as fp:
-            attrdata = fp.read().strip()
+    if os.path.isfile(sysfs_attr_path):
+        attrdata = util.load_file(sysfs_attr_path).strip()
 
     return attrdata
 
@@ -452,6 +462,8 @@
 
 
 def md_get_spares_list(devpath):
+    assert_valid_devpath(devpath)
+
     sysfs_md = sys_block_path(devpath) + '/md'
 
     if not os.path.exists(sysfs_md):
@@ -469,6 +481,8 @@
 
 
 def md_get_devices_list(devpath):
+    assert_valid_devpath(devpath)
+
     sysfs_md = sys_block_path(devpath) + '/md'
     if not os.path.exists(sysfs_md):
         raise ValueError('Cannot find md sysfs directory: ' +
@@ -483,6 +497,8 @@
 
 
 def md_check_array_uuid(md_devname, md_uuid):
+    valid_mdname(md_devname)
+
     # confirm we have /dev/{mdname} by following the udev symlink
     mduuid_path = ('/dev/disk/by-id/md-uuid-' + md_uuid)
     mdlink_devname = dev_path(os.path.realpath(mduuid_path))
@@ -495,8 +511,7 @@
 
 
 def md_get_uuid(md_devname):
-    if not valid_mdname(md_devname):
-        raise ValueError('Invalid md devicename')
+    valid_mdname(md_devname)
 
     md_query = mdadm_query_detail(md_devname)
     return md_query.get('MD_UUID', None)
@@ -543,12 +558,12 @@
     # check array state
 
     writable = md_check_array_state_rw(md_devname)
-    degraded = int(md_sysfs_attr(md_devname, 'degraded'))
+    degraded = md_sysfs_attr(md_devname, 'degraded')
     sync_action = md_sysfs_attr(md_devname, 'sync_action')
 
     if not writable:
         raise ValueError('Array not in writable state: ' + md_devname)
-    if degraded > 0:
+    if degraded != "0":
         raise ValueError('Array in degraded state: ' + md_devname)
     if sync_action != "idle":
         raise ValueError('Array syncing, not idle state: ' + md_devname)
@@ -615,6 +630,7 @@
                                                                  raidlevel,
                                                                  devices,
                                                                  spares))
+    assert_valid_devpath(md_devname)
 
     md_check_array_state(md_devname)
     md_check_raidlevel(raidlevel)

=== modified file 'curtin/commands/block_meta.py'
--- curtin/commands/block_meta.py	2016-01-06 22:18:33 +0000
+++ curtin/commands/block_meta.py	2016-01-07 17:16:36 +0000
@@ -896,6 +896,7 @@
     devices = info.get('devices')
     raidlevel = info.get('raidlevel')
     spare_devices = info.get('spare_devices')
+    md_devname = block.dev_path(info.get('name'))
     if not devices:
         raise ValueError("devices for raid must be specified")
     if raidlevel not in ['linear', 'raid0', 0, 'stripe', 'raid1', 1, 'mirror',
@@ -916,23 +917,22 @@
     # Handle preserve flag
     if info.get('preserve'):
         # check if the array is already up, if not try to assemble
-        if not block.md_check(info.get('name'), raidlevel,
+        if not mdadm.md_check(md_devname, raidlevel,
                               device_paths, spare_device_paths):
             LOG.info("assembling preserved raid for "
-                     "{}".format(info.get('name')))
+                     "{}".format(md_devname))
 
-            mdadm.mdadm_assemble(info.get('name'),
-                                 device_paths, spare_device_paths)
+            mdadm.mdadm_assemble(md_devname, device_paths, spare_device_paths)
 
             # try again after attempting to assemble
-            if not mdadm.md_check(info.get('name'), raidlevel,
+            if not mdadm.md_check(md_devname, raidlevel,
                                   devices, spare_device_paths):
                 raise ValueError("Unable to confirm preserved raid array: "
-                                 " {}".format(info.get('name')))
+                                 " {}".format(md_devname))
         # raid is all OK
         return
 
-    mdadm.mdadm_create(info.get('name'), raidlevel,
+    mdadm.mdadm_create(md_devname, raidlevel,
                        device_paths, spare_device_paths,
                        info.get('mdname', ''))
 

=== modified file 'tests/unittests/test_block_mdadm.py'
--- tests/unittests/test_block_mdadm.py	2015-12-19 19:48:06 +0000
+++ tests/unittests/test_block_mdadm.py	2016-01-07 17:16:36 +0000
@@ -5,12 +5,6 @@
 import os
 import subprocess
 
-from sys import version_info
-if version_info.major == 2:
-    import __builtin__ as builtins
-else:
-    import builtins
-
 
 class MdadmTestBase(TestCase):
     def setUp(self):
@@ -60,15 +54,9 @@
         self.mock_util.subp.assert_has_calls(expected_calls)
 
     def test_mdadm_assemble_md_devname_short(self):
-        md_devname = "md0"
-        mdadm.mdadm_assemble(md_devname=md_devname)
-
-        expected_calls = [
-            call(["mdadm", "--assemble", "/dev/md0", "--run"], capture=True,
-                 rcs=[0, 1, 2]),
-            call(["udevadm", "settle"]),
-        ]
-        self.mock_util.subp.assert_has_calls(expected_calls)
+        with self.assertRaises(ValueError):
+            md_devname = "md0"
+            mdadm.mdadm_assemble(md_devname=md_devname)
 
     def test_mdadm_assemble_md_devname_none(self):
         with self.assertRaises(ValueError):
@@ -149,6 +137,15 @@
                            devices=devices, spares=spares)
         self.mock_util.subp.assert_has_calls(expected_calls)
 
+    def test_mdadm_create_raid0_devshort(self):
+        md_devname = "md0"
+        raidlevel = 0
+        devices = ["/dev/vdc1", "/dev/vdd1"]
+        spares = []
+        with self.assertRaises(ValueError):
+            mdadm.mdadm_create(md_devname=md_devname, raidlevel=raidlevel,
+                               devices=devices, spares=spares)
+
     def test_mdadm_create_raid0_with_spares(self):
         md_devname = "/dev/md0"
         raidlevel = 0
@@ -488,12 +485,8 @@
 
     def test_valid_mdname_short(self):
         mdname = "md0"
-        result = mdadm.valid_mdname(mdname)
-        expected_calls = [
-            call("/dev/md0")
-        ]
-        self.mock_valid.assert_has_calls(expected_calls)
-        self.assertTrue(result)
+        with self.assertRaises(ValueError):
+            mdadm.valid_mdname(mdname)
 
     def test_valid_mdname_none(self):
         mdname = None
@@ -506,14 +499,15 @@
         with self.assertRaises(ValueError):
             mdadm.valid_mdname(mdname)
 
-    @patch.object(builtins, "open")
-    def test_md_sysfs_attr(self, mock_open):
+    @patch('curtin.block.mdadm.os.path.isfile')
+    def test_md_sysfs_attr(self, mock_isfile):
         mdname = "/dev/md0"
         attr_name = 'array_state'
         sysfs_path = '/sys/class/block/{}/md/{}'.format(dev_short(mdname),
                                                         attr_name)
+        mock_isfile.return_value = True
         mdadm.md_sysfs_attr(mdname, attr_name)
-        mock_open.assert_called_with(sysfs_path)
+        self.mock_util.load_file.assert_called_with(sysfs_path)
 
     def test_md_sysfs_attr_devname_none(self):
         mdname = None
@@ -729,6 +723,17 @@
             mdadm.md_check_array_state(mdname)
 
     @patch('curtin.block.mdadm.md_sysfs_attr')
+    def test_md_check_array_state_degraded_empty(self, mock_attr):
+        mdname = '/dev/md0'
+        mock_attr.side_effect = [
+            'clean',  # array_state
+            '',  # unknown
+            'idle',  # sync_action
+        ]
+        with self.assertRaises(ValueError):
+            mdadm.md_check_array_state(mdname)
+
+    @patch('curtin.block.mdadm.md_sysfs_attr')
     def test_md_check_array_state_sync(self, mock_attr):
         mdname = '/dev/md0'
         mock_attr.side_effect = [
@@ -888,4 +893,14 @@
         mock_spare.assert_has_calls([call(md_devname, spares)])
         mock_member.assert_has_calls([call(md_devname, devices + spares)])
 
+    def test_md_check_all_good_devshort(self):
+        md_devname = 'md0'
+        raidlevel = 1
+        devices = ['/dev/vda', '/dev/vdb']
+        spares = ['/dev/vdc']
+
+        with self.assertRaises(ValueError):
+            mdadm.md_check(md_devname, raidlevel, devices=devices,
+                           spares=spares)
+
 # vi: ts=4 expandtab syntax=python