Use provided device paths

Ensure that device paths provided by end users are used for OSD's,
rather than the link target device as this may change between
reboots. The specific use case is bcache, where:

   /dev/bcacheX:
        changes between reboots
   /dev/disk/by-dname/bcacheX:
        udev managed and consistent

This change also ensures that any unit data is updated to switch
back to using the provided block device path, rather than the
actual target which may have been used in prior charm revisions.

Change-Id: If5e88d93b9323052ea762d3a4b66f2442d4a19be
Depends-On: If0e1fbc62bfe7d0f9e21db9bfdeee761060de846
Closes-Bug: 1782439

hooks/ceph_hooks.py

@@ -89,6 +89,7 @@ from charmhelpers.contrib.storage.linux.ceph import (
     CephConfContext)
 from charmhelpers.contrib.storage.linux.utils import (
     is_device_mounted,
+    is_block_device,
 )
 from charmhelpers.contrib.charmsupport import nrpe
 from charmhelpers.contrib.hardening.harden import harden
@@ -522,10 +523,19 @@ def get_devices():
     if config('osd-devices'):
         for path in config('osd-devices').split(' '):
             path = path.strip()
+            # Ensure that only block devices
+            # are considered for evaluation as block devices.
+            # This avoids issues with relative directories
+            # being passed via configuration, and ensures that
+            # the path to a block device provided by the user
+            # is used, rather than its target which may change
+            # between reboots in the case of bcache devices.
+            if is_block_device(path):
+                devices.append(path)
             # Make sure its a device which is specified using an
             # absolute path so that the current working directory
             # or any relative path under this directory is not used
-            if os.path.isabs(path):
+            elif os.path.isabs(path):
                 devices.append(os.path.realpath(path))
 
     # List storage instances for the 'osd-devices'
@@ -562,6 +572,32 @@ def upgrade_charm():
     apt_install(packages=filter_installed_packages(ceph.determine_packages()),
                 fatal=True)
     install_udev_rules()
+    remap_resolved_targets()
+
+
+def remap_resolved_targets():
+    '''Remap any previous fully resolved target devices to provided names'''
+    # NOTE(jamespage): Deal with any prior provided dev to
+    # target device resolution which occurred in prior
+    # releases of the charm - the user provided value
+    # should be used in preference to the target path
+    # to the block device as in some instances this
+    # is not consistent between reboots (bcache).
+    db = kv()
+    touched_devices = db.get('osd-devices', [])
+    osd_devices = get_devices()
+    for dev in osd_devices:
+        real_path = os.path.realpath(dev)
+        if real_path != dev and real_path in touched_devices:
+            log('Device {} already processed by charm using '
+                'actual device path {}, updating block device '
+                'usage with provided device path '
+                'and skipping'.format(dev,
+                                      real_path))
+            touched_devices.remove(real_path)
+            touched_devices.append(dev)
+    db.set('osd-devices', touched_devices)
+    db.flush()
 
 
 @hooks.hook('nrpe-external-master-relation-joined',

lib/ceph/utils.py

@@ -1699,8 +1699,10 @@ def is_mapped_luks_device(dev):
     :param: dev: A full path to a block device to be checked
     :returns: boolean: indicates whether a device is mapped
     """
-    _, dirs, _ = next(os.walk('/sys/class/block/{}/holders/'
+    _, dirs, _ = next(os.walk(
-                              .format(os.path.basename(dev))))
+        '/sys/class/block/{}/holders/'
+        .format(os.path.basename(os.path.realpath(dev))))
+    )
     is_held = len(dirs) > 0
     return is_held and is_luks_device(dev)
 

unit_tests/test_ceph_hooks.py

@@ -395,26 +395,31 @@ class CephHooksTestCase(unittest.TestCase):
             call('ceph-osd@2'),
         ])
 
+    @patch.object(ceph_hooks, 'is_block_device')
     @patch.object(ceph_hooks, 'storage_list')
     @patch.object(ceph_hooks, 'config')
-    def test_get_devices(self, mock_config, mock_storage_list):
+    def test_get_devices(self, mock_config, mock_storage_list,
+                         mock_is_block_device):
         '''Devices returned as expected'''
         config = {'osd-devices': '/dev/vda /dev/vdb'}
         mock_config.side_effect = lambda key: config[key]
         mock_storage_list.return_value = []
+        mock_is_block_device.return_value = True
         devices = ceph_hooks.get_devices()
         self.assertEqual(devices, ['/dev/vda', '/dev/vdb'])
 
+    @patch.object(ceph_hooks, 'is_block_device')
     @patch.object(ceph_hooks, 'get_blacklist')
     @patch.object(ceph_hooks, 'storage_list')
     @patch.object(ceph_hooks, 'config')
     def test_get_devices_blacklist(self, mock_config, mock_storage_list,
-                                   mock_get_blacklist):
+                                   mock_get_blacklist, mock_is_block_device):
         '''Devices returned as expected when blacklist in effect'''
         config = {'osd-devices': '/dev/vda /dev/vdb'}
         mock_config.side_effect = lambda key: config[key]
         mock_storage_list.return_value = []
         mock_get_blacklist.return_value = ['/dev/vda']
+        mock_is_block_device.return_value = True
         devices = ceph_hooks.get_devices()
         mock_storage_list.assert_called()
         mock_get_blacklist.assert_called()

unit_tests/test_config.py

@@ -34,6 +34,7 @@ with patch('charmhelpers.contrib.hardening.harden.harden') as mock_dec:
 
 TO_PATCH = [
     'config',
+    'is_block_device',
 ]
 
 
@@ -43,6 +44,13 @@ class GetDevicesTestCase(test_utils.CharmTestCase):
         super(GetDevicesTestCase, self).setUp(hooks, TO_PATCH)
         self.config.side_effect = self.test_config.get
         self.tmp_dir = tempfile.mkdtemp()
+        self.bd = {
+            os.path.join(self.tmp_dir, "device1"): True,
+            os.path.join(self.tmp_dir, "device1"): True,
+            os.path.join(self.tmp_dir, "link"): True,
+            os.path.join(self.tmp_dir, "device"): True,
+        }
+        self.is_block_device.side_effect = lambda x: self.bd.get(x, False)
         self.addCleanup(shutil.rmtree, self.tmp_dir)
 
     def test_get_devices_empty(self):
@@ -93,11 +101,11 @@ class GetDevicesTestCase(test_utils.CharmTestCase):
 
     def test_get_devices_symlink(self):
         """
-        If a symlink is specified in osd-devices, get_devices() resolves
+        If a symlink is specified in osd-devices, get_devices() does not
-        it and returns the link target.
+        resolve it and returns the symlink provided.
         """
         device = os.path.join(self.tmp_dir, "device")
         link = os.path.join(self.tmp_dir, "link")
         os.symlink(device, link)
         self.test_config.set("osd-devices", link)
-        self.assertEqual([device], hooks.get_devices())
+        self.assertEqual([link], hooks.get_devices())