Fix encryption symlink issues

This patch fixes 2 issues related to the symlinks, or lack of, that
connectors' connect_volume methods return.

Some connectors always return the block device instead of a symlink for
encrypted volumes, and other connectors return a symlink that is owned
by the system's udev rules.  Both cases are problematic

Returning the real device can prevent the encryptor connect_volume to
complete successfully, and in other cases (such as nvmeof) it completes,
but on the connector's disconnect volume it will leave the device behind
(i.e., /dev/nvme0n1) preventing new connections that would use that same
device name.

Returning a symlink owned by the system's udev rules means that they can
be reclaimed by those rules at any time.  This can happen with
cryptsetup, because when it creates a new mapping it triggers udev rules
for the device that can reclaim the symlink after os-brick has replaced
it.

This patch creates a couple of decorators to facilitate this for all
connectors. These decorators transform the paths so that the callers
gets the expected symlink, but the connector doesn't need to worry about
it and will always see the value it returns regardless of what symlink
the caller gets.

From this moment onwards we use our own custom symlink that starts with
"/dev/disk/by-id/os-brick".

The patch fixes bugs in other connectors (such as the RBD local
connection), but since there are no open bugs they have not been
reported.

This commit also includes a squash to add a fix and releasenote from
Ib001e2b4d1347754c2b46730bc10d86e8cdab7ad to reduce backporting
workload.

Closes-Bug: #1964379
Closes-Bug: #1967790
Closes-Bug: #1981455
Change-Id: Ie373ab050dcc0a35c749d9a53b6cf5ca060bcb58
(cherry picked from commit 1583a50)
(cherry picked from commit 0bd5dc9)
(cherry picked from commit b31b109)
Conflicts: os_brick/utils.py
	os_brick/initiator/connectors/iscsi.py
	os_brick/initiator/connectors/nvmeof.py
(cherry picked from commit cf69f92)

os_brick/initiator/connectors/fibre_channel.py

@@ -170,6 +170,7 @@ def get_volume_paths(self, connection_properties):
 
     @utils.trace
     @synchronized('extend_volume', external=True)
+    @utils.connect_volume_undo_prepare_result
     def extend_volume(self, connection_properties):
         """Update the local kernel's size information.
 
@@ -190,6 +191,7 @@ def extend_volume(self, connection_properties):
             raise exception.VolumePathsNotFound()
 
     @utils.trace
+    @utils.connect_volume_prepare_result
     @synchronized('connect_volume', external=True)
     def connect_volume(self, connection_properties):
         """Attach the volume to instance_name.
@@ -316,6 +318,7 @@ def _get_possible_devices(self, hbas, targets):
 
     @utils.trace
     @synchronized('connect_volume', external=True)
+    @utils.connect_volume_undo_prepare_result(unlink_after=True)
     def disconnect_volume(self, connection_properties, device_info,
                           force=False, ignore_errors=False):
         """Detach the volume from instance_name.
@@ -357,8 +360,7 @@ def _remove_devices(self, connection_properties, devices, device_info):
         # There may have been more than 1 device mounted
         # by the kernel for this volume.  We have to remove
         # all of them
-        path_used = self._linuxscsi.get_dev_path(connection_properties,
-                                                 device_info)
+        path_used = utils.get_dev_path(connection_properties, device_info)
         # NOTE: Due to bug #1897787 device_info may have a real path for some
         # single paths instead of a symlink as it should have, so it'll only
         # be a multipath if it was a symlink (not real path) and it wasn't a

os_brick/initiator/connectors/iscsi.py

@@ -469,6 +469,7 @@ def _run_iscsiadm_update_discoverydb(self, connection_properties,
 
     @utils.trace
     @synchronized('extend_volume', external=True)
+    @utils.connect_volume_undo_prepare_result
     def extend_volume(self, connection_properties):
         """Update the local kernel's size information.
 
@@ -491,6 +492,7 @@ def extend_volume(self, connection_properties):
             raise exception.VolumePathsNotFound()
 
     @utils.trace
+    @utils.connect_volume_prepare_result
     @synchronized('connect_volume', external=True)
     def connect_volume(self, connection_properties):
         """Attach the volume to instance_name.
@@ -517,41 +519,8 @@ def connect_volume(self, connection_properties):
             with excutils.save_and_reraise_exception():
                 self._cleanup_connection(connection_properties, force=True)
 
-    @utils.retry(exceptions=(exception.VolumeDeviceNotFound))
-    def _get_device_link(self, wwn, device, mpath):
-        # These are the default symlinks that should always be there
-        if mpath:
-            symlink = '/dev/disk/by-id/dm-uuid-mpath-' + mpath
-        else:
-            symlink = '/dev/disk/by-id/scsi-' + wwn
-
-        # If default symlinks are not there just search for anything that links
-        # to our device.  In my experience this will return the last added link
-        # first, so if we are going to succeed this should be fast.
-        if not os.path.realpath(symlink) == device:
-            links_path = '/dev/disk/by-id/'
-            for symlink in os.listdir(links_path):
-                symlink = links_path + symlink
-                if os.path.realpath(symlink) == device:
-                    break
-            else:
-                # Raising this will trigger the next retry
-                raise exception.VolumeDeviceNotFound(device='/dev/disk/by-id')
-        return symlink
-
     def _get_connect_result(self, con_props, wwn, devices_names, mpath=None):
         device = '/dev/' + (mpath or devices_names[0])
-
-        # NOTE(geguileo): This is only necessary because of the current
-        # encryption flow that requires that connect_volume returns a symlink
-        # because first we do the volume attach, then the libvirt config is
-        # generated using the path returned by the atach, and then we do the
-        # encryption attach, which is forced to preserve the path that was used
-        # in the libvirt config.  If we fix that flow in OS-brick, Nova, and
-        # Cinder we can remove this and just return the real path.
-        if con_props.get('encrypted'):
-            device = self._get_device_link(wwn, device, mpath)
-
         result = {'type': 'block', 'scsi_wwn': wwn, 'path': device}
         if mpath:
             result['multipath_id'] = wwn
@@ -855,6 +824,7 @@ def _get_connection_devices(self, connection_properties,
 
     @utils.trace
     @synchronized('connect_volume', external=True)
+    @utils.connect_volume_undo_prepare_result(unlink_after=True)
     def disconnect_volume(self, connection_properties, device_info,
                           force=False, ignore_errors=False):
         """Detach the volume from instance_name.
@@ -920,8 +890,7 @@ def _cleanup_connection(self, connection_properties, ips_iqns_luns=None,
         for remove, __ in devices_map.values():
             remove_devices.update(remove)
 
-        path_used = self._linuxscsi.get_dev_path(connection_properties,
-                                                 device_info)
+        path_used = utils.get_dev_path(connection_properties, device_info)
         was_multipath = (path_used.startswith('/dev/dm-') or
                          'mpath' in path_used)
         multipath_name = self._linuxscsi.remove_connection(

os_brick/initiator/connectors/nvmeof.py

@@ -221,6 +221,7 @@ def _try_disconnect_volume(self, conn_nqn, ignore_errors=False):
                 raise
 
     @utils.trace
+    @utils.connect_volume_prepare_result
     @synchronized('connect_volume', external=True)
     def connect_volume(self, connection_properties):
         """Discover and attach the volume.
@@ -269,6 +270,7 @@ def connect_volume(self, connection_properties):
 
     @utils.trace
     @synchronized('disconnect_volume', external=True)
+    @utils.connect_volume_undo_prepare_result(unlink_after=True)
     def disconnect_volume(self, connection_properties, device_info,
                           force=False, ignore_errors=False):
         """Detach and flush the volume.
@@ -321,6 +323,7 @@ def disconnect_volume(self, connection_properties, device_info,
 
     @utils.trace
     @synchronized('extend_volume', external=True)
+    @utils.connect_volume_undo_prepare_result
     def extend_volume(self, connection_properties):
         """Update the local kernel's size information.
 

os_brick/initiator/connectors/rbd.py

@@ -258,6 +258,7 @@ def _local_attach_volume(self, connection_properties):
         return res
 
     @utils.trace
+    @utils.connect_volume_prepare_result
     def connect_volume(self, connection_properties):
         """Connect to a volume.
 
@@ -336,6 +337,7 @@ def _find_root_device(self, connection_properties, conf):
         return None
 
     @utils.trace
+    @utils.connect_volume_undo_prepare_result(unlink_after=True)
     def disconnect_volume(self, connection_properties, device_info,
                           force=False, ignore_errors=False):
         """Disconnect a volume.
@@ -399,6 +401,7 @@ def check_valid_device(self, path, run_as_root=True):
         # For backward compatibility ignore run_as_root param with handles
         return self._check_valid_device(path)
 
+    @utils.connect_volume_undo_prepare_result
     def extend_volume(self, connection_properties):
         """Refresh local volume view and return current size in bytes."""
         # Nothing to do, RBD attached volumes are automatically refreshed, but

os_brick/initiator/connectors/scaleio.py

@@ -337,6 +337,7 @@ def get_config(self, connection_properties):
         return device_info
 
     @utils.trace
+    @utils.connect_volume_prepare_result
     @lockutils.synchronized('scaleio', 'scaleio-', external=True)
     def connect_volume(self, connection_properties):
         """Connect the volume.
@@ -452,6 +453,7 @@ def connect_volume(self, connection_properties):
 
     @utils.trace
     @lockutils.synchronized('scaleio', 'scaleio-', external=True)
+    @utils.connect_volume_undo_prepare_result(unlink_after=True)
     def disconnect_volume(self, connection_properties, device_info,
                           force=False, ignore_errors=False):
         """Disconnect the ScaleIO volume.
@@ -515,6 +517,7 @@ def disconnect_volume(self, connection_properties, device_info,
                 LOG.error(msg)
                 raise exception.BrickException(message=msg)
 
+    @utils.connect_volume_undo_prepare_result
     def extend_volume(self, connection_properties):
         """Update the local kernel's size information.
 

os_brick/initiator/connectors/storpool.py

@@ -23,6 +23,7 @@
 
 from os_brick import exception
 from os_brick.initiator.connectors import base
+from os_brick import utils
 
 LOG = logging.getLogger(__name__)
 
@@ -52,6 +53,7 @@ def get_connector_properties(root_helper, *args, **kwargs):
         """The StorPool connector properties."""
         return {}
 
+    @utils.connect_volume_prepare_result
     def connect_volume(self, connection_properties):
         """Connect to a volume.
 
@@ -87,6 +89,7 @@ def connect_volume(self, connection_properties):
         self._attach.sync(req_id, None)
         return {'type': 'block', 'path': '/dev/storpool/' + volume}
 
+    @utils.connect_volume_undo_prepare_result(unlink_after=True)
     def disconnect_volume(self, connection_properties, device_info,
                           force=False, ignore_errors=False):
         """Disconnect a volume from the local host.
@@ -197,6 +200,7 @@ def _get_device_size(self, device):
         else:
             return None
 
+    @utils.connect_volume_undo_prepare_result
     def extend_volume(self, connection_properties):
         """Update the attached volume's size.
 

os_brick/initiator/initiator_connector.py

@@ -129,6 +129,11 @@ def connect_volume(self, connection_properties):
          whenever there's a connection between the host's HBA and the storage
          array's target ports.
 
+        Encrypted volumes have some peculiar requirements on the path that must
+        be returned, so it is recommended to decorate the method with the
+        os_brick.utils.connect_volume_prepare_result to ensure that the right
+        device path is returned to the caller.
+
         :param connection_properties: The dictionary that describes all
                                       of the target volume attributes.
         :type connection_properties: dict
@@ -144,6 +149,15 @@ def disconnect_volume(self, connection_properties, device_info,
         The connection_properties are the same as from connect_volume.
         The device_info is returned from connect_volume.
 
+        If the connector's connect_volume is decorated with
+        os_brick.utils.connect_volume_prepare_result then the path will
+        have been changed by the decorator if the volume was encrypted, so if
+        we need to have the original path that the connector returned instead
+        of the modified one (for example to identify the WWN from the symlink)
+        then we should use the
+        os_brick.utils.connect_volume_undo_prepare_result decorator with the
+        unlink_after=True parameter.
+
         :param connection_properties: The dictionary that describes all
                                       of the target volume attributes.
         :type connection_properties: dict
@@ -190,6 +204,15 @@ def extend_volume(self, connection_properties):
         system.  The new volume size in bytes will be returned.
         If there is a failure to update, then None will be returned.
 
+        If the connector's connect_volume is decorated with
+        os_brick.utils.connect_volume_prepare_result then the path will
+        have been changed by the decorator if the volume was encrypted, so if
+        this method uses the original path as a shortcut to know which device
+        to extend (instead of using the other connection information) then it
+        should use the os_brick.utils.connect_volume_undo_prepare_result
+        decorator on this method so that it gets the original path instead of
+        the modified symlink one.
+
         :param connection_properties: The volume connection properties.
         :returns: new size of the volume.
         """

os_brick/initiator/linuxscsi.py

@@ -249,14 +249,6 @@ def find_sysfs_multipath_dm(self, device_names):
                 return dm
         return None
 
-    @staticmethod
-    def get_dev_path(connection_properties, device_info):
-        """Determine what path was used by Nova/Cinder to access volume."""
-        if device_info and device_info.get('path'):
-            return device_info.get('path')
-
-        return connection_properties.get('device_path') or ''
-
     @staticmethod
     def requires_flush(path, path_used, was_multipath):
         """Check if a device needs to be flushed when detaching.

os_brick/privileged/rootwrap.py

@@ -213,8 +213,25 @@ def unlink_root(*links, **kwargs):
     raise_at_end = kwargs.get('raise_at_end', False)
     exc = exception.ExceptionChainer()
     catch_exception = no_errors or raise_at_end
+    LOG.debug('Unlinking %s', links)
     for link in links:
         with exc.context(catch_exception, 'Unlink failed for %s', link):
             os.unlink(link)
     if not no_errors and raise_at_end and exc:
         raise exc
+
+
+@privileged.default.entrypoint
+def link_root(target, link_name, force=True):
+    """Create a symbolic link with sys admin privileges.
+
+    This method behaves like the "ln -s" command, including the force parameter
+    where it will replace the link_name file even if it's not a symlink.
+    """
+    LOG.debug('Linking %s -> %s', link_name, target)
+    if force:
+        try:
+            os.remove(link_name)
+        except FileNotFoundError:
+            pass
+    os.symlink(target, link_name)

os_brick/tests/initiator/connectors/test_fibre_channel.py

@@ -741,7 +741,7 @@ def test__add_targets_to_connection_properties(self, target_info,
     @ddt.unpack
     @mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.remove_scsi_device')
     @mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.requires_flush')
-    @mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.get_dev_path')
+    @mock.patch('os_brick.utils.get_dev_path')
     def test__remove_devices(self, path_used, was_multipath, get_dev_path_mock,
                              flush_mock, remove_mock):
         get_dev_path_mock.return_value = path_used