Add unit tests and fix related bugs

This change adds more unit tests to the rolling OSD upgrade
scenario to ensure more complete coverage.

Change-Id: Ibd7c8d6c46520957a3298446efc6b5fff210a51a
Related-Bug: #1662591

ceph/__init__.py

@@ -561,7 +561,8 @@ def _get_child_dirs(path):
     if not os.path.isdir(path):
         raise ValueError('Specified path "%s" is not a directory' % path)
 
-    return filter(os.path.isdir, os.listdir(path))
+    files_in_dir = [os.path.join(path, f) for f in os.listdir(path)]
+    return list(filter(os.path.isdir, files_in_dir))
 
 
 def _get_osd_num_from_dirname(dirname):
@@ -1559,6 +1560,7 @@ def lock_and_roll(upgrade_key, service, my_name, version):
                                                   my_name,
                                                   version,
                                                   stop_timestamp))
+    status_set('maintenance', 'Finishing upgrade')
     monitor_key_set(upgrade_key, "{}_{}_{}_done".format(service,
                                                         my_name,
                                                         version),
@@ -1744,7 +1746,7 @@ def _upgrade_single_osd(osd_num, osd_dir):
     """
     stop_osd(osd_num)
     disable_osd(osd_num)
-    update_owner(os.path.join(OSD_BASE_DIR, osd_dir))
+    update_owner(osd_dir)
     enable_osd(osd_num)
     start_osd(osd_num)
 
@@ -1860,8 +1862,8 @@ def update_owner(path, recurse_dirs=True):
     cmd = ['chown', user_group, path]
     if os.path.isdir(path) and recurse_dirs:
         status_set('maintenance', ('Updating ownership of %s to %s' %
-                                   (path.split('/')[-1], user)))
-        cmd.insert('-R', 1)
+                                   (path, user)))
+        cmd.insert(1, '-R')
 
     log('Changing ownership of {path} to {user}'.format(
         path=path, user=user_group), DEBUG)
@@ -1910,14 +1912,12 @@ def dirs_need_ownership_update(service):
     expected_owner = expected_group = ceph_user()
     path = os.path.join(CEPH_BASE_DIR, service)
     for child in _get_child_dirs(path):
-        child_path = os.path.join(path, child)
-        curr_owner, curr_group = owner(child_path)
+        curr_owner, curr_group = owner(child)
 
         if (curr_owner == expected_owner) and (curr_group == expected_group):
             continue
 
-        log('Directory "%s" needs its ownership updated' % child_path,
-            DEBUG)
+        log('Directory "%s" needs its ownership updated' % child, DEBUG)
         return True
 
     # All child directories had the expected ownership

unit_tests/test_osd_upgrade_roll.py

@@ -155,7 +155,7 @@ class UpgradeRollingTestCase(unittest.TestCase):
     @patch.object(ceph, 'start_osd')
     def test_upgrade_single_osd(self, start_osd, enable_osd, update_owner,
                                 disable_osd, stop_osd):
-        ceph._upgrade_single_osd(1, 'ceph-1')
+        ceph._upgrade_single_osd(1, '/var/lib/ceph/osd/ceph-1')
         stop_osd.assert_called_with(1)
         disable_osd.assert_called_with(1)
         update_owner.assert_called_with('/var/lib/ceph/osd/ceph-1')
@@ -294,6 +294,73 @@ class UpgradeRollingTestCase(unittest.TestCase):
                                          upgrade_key='osd-upgrade',
                                          version='0.94.1')
 
+    @patch('os.path.exists')
+    @patch('os.listdir')
+    @patch('os.path.isdir')
+    def test__get_child_dirs(self, isdir, listdir, exists):
+        isdir.side_effect = [True, True, True, False, True]
+        listdir.return_value = ['mon', 'bootstrap-osd', 'foo', 'bootstrap-mon']
+        exists.return_value = True
+
+        child_dirs = ceph._get_child_dirs('/var/lib/ceph')
+        isdir.assert_has_calls([call('/var/lib/ceph'),
+                                call('/var/lib/ceph/mon'),
+                                call('/var/lib/ceph/bootstrap-osd'),
+                                call('/var/lib/ceph/foo'),
+                                call('/var/lib/ceph/bootstrap-mon')])
+        self.assertListEqual(['/var/lib/ceph/mon',
+                              '/var/lib/ceph/bootstrap-osd',
+                              '/var/lib/ceph/bootstrap-mon'], child_dirs)
+
+    @patch('os.path.exists')
+    @patch('os.path.isdir')
+    def test__get_child_dirs_not_dir(self, isdir, exists):
+        isdir.return_value = False
+        exists.return_value = False
+
+        with self.assertRaises(ValueError):
+            ceph._get_child_dirs('/var/lib/ceph')
+
+    @patch('os.path.exists')
+    def test__get_child_dirs_no_exist(self, exists):
+        exists.return_value = False
+
+        with self.assertRaises(ValueError):
+            ceph._get_child_dirs('/var/lib/ceph')
+
+    @patch('ceph.ceph_user')
+    @patch('os.path.isdir')
+    @patch('subprocess.check_call')
+    @patch('ceph.status_set')
+    def test_update_owner_no_recurse(self, status_set, check_call,
+                                     isdir, ceph_user):
+        ceph_user.return_value = 'ceph'
+        isdir.return_value = True
+        ceph.update_owner('/var/lib/ceph', False)
+        check_call.assert_called_with(['chown', 'ceph:ceph', '/var/lib/ceph'])
+
+    @patch('ceph.ceph_user')
+    @patch('os.path.isdir')
+    @patch('subprocess.check_call')
+    @patch('ceph.status_set')
+    def test_update_owner_recurse_file(self, status_set, check_call,
+                                       isdir, ceph_user):
+        ceph_user.return_value = 'ceph'
+        isdir.return_value = False
+        ceph.update_owner('/var/lib/ceph', True)
+        check_call.assert_called_with(['chown', 'ceph:ceph', '/var/lib/ceph'])
+
+    @patch('ceph.ceph_user')
+    @patch('os.path.isdir')
+    @patch('subprocess.check_call')
+    @patch('ceph.status_set')
+    def test_update_owner_recurse(self, status_set, check_call,
+                                  isdir, ceph_user):
+        ceph_user.return_value = 'ceph'
+        isdir.return_value = True
+        ceph.update_owner('/var/lib/ceph', True)
+        check_call.assert_called_with(['chown', '-R', 'ceph:ceph',
+                                       '/var/lib/ceph'])
 
 """
     @patch('ceph.log')