FC: Fix not flushing on detach

When doing multipathed Fibre Channel we may end up not flushing the
volume when disconnecting it if didn't find a multipath when doing
the connection.

The issue is caused by the `connect_volume` call returning a real path
instead of a symlink and us passing the wrong device_info to the
_remove_devices method, which makes the disconnect code incorrectly
identify when a multipath was used to decide whether to do a flush or
not.

This patch fixes these things in order to flush the individual path when
necessary.

Closes-Bug: #1897787
Change-Id: Ie3266c81b3c6bb5a2c213a12410539d404d1febe
(cherry picked from commit 1432c369fb)
(cherry picked from commit 3adcb40678)

os_brick/initiator/connectors/fibre_channel.py

@@ -225,8 +225,8 @@ class FibreChannelConnector(base.BaseLinuxConnector):
                           {'device': device})
                 if os.path.exists(device) and self.check_valid_device(device):
                     self.host_device = device
-                    # get the /dev/sdX device.  This is used
-                    # to find the multipath device.
+                    # get the /dev/sdX device.  This variable is maintained to
+                    # keep the same log output.
                     self.device_name = os.path.realpath(device)
                     raise loopingcall.LoopingCallDone()
 
@@ -260,8 +260,11 @@ class FibreChannelConnector(base.BaseLinuxConnector):
         # see if the new drive is part of a multipath
         # device.  If so, we'll use the multipath device.
         if self.use_multipath:
+            # Pass a symlink, not a real path, otherwise we'll get a real path
+            # back if we don't find a multipath and we'll return that to the
+            # caller, breaking Nova's encryption which requires a symlink.
             (device_path, multipath_id) = self._discover_mpath_device(
-                device_wwn, connection_properties, self.device_name)
+                device_wwn, connection_properties, self.host_device)
             if multipath_id:
                 # only set the multipath_id if we found one
                 device_info['multipath_id'] = multipath_id
@@ -344,8 +347,8 @@ class FibreChannelConnector(base.BaseLinuxConnector):
                 mpath_path = self._linuxscsi.find_multipath_device_path(wwn)
                 if mpath_path:
                     self._linuxscsi.flush_multipath_device(mpath_path)
-            device_info = self._linuxscsi.get_device_info(real_path)
-            devices.append(device_info)
+            dev_info = self._linuxscsi.get_device_info(real_path)
+            devices.append(dev_info)
 
         LOG.debug("devices to remove = %s", devices)
         self._remove_devices(connection_properties, devices, device_info)
@@ -356,7 +359,15 @@ class FibreChannelConnector(base.BaseLinuxConnector):
         # all of them
         path_used = self._linuxscsi.get_dev_path(connection_properties,
                                                  device_info)
-        was_multipath = '/pci-' not in path_used
+        # 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
+        # single path symlink (those have filenames starting with pci-)
+        # We don't use os.path.islink in case the file is no longer there.
+        was_symlink = path_used.count(os.sep) > 2
+        # We check for /pci because that's the value we return for single
+        # paths, whereas for multipaths we have multiple link formats.
+        was_multipath = '/pci-' not in path_used and was_symlink
         for device in devices:
             device_path = device['device']
             flush = self._linuxscsi.requires_flush(device_path,

os_brick/initiator/linuxscsi.py

@@ -262,16 +262,23 @@ class LinuxSCSI(executor.Executor):
         """
         # No used path happens on failed attachs, when we don't care about
         # individual flushes.
-        # When Nova used a multipath we don't need to do individual flushes.
-        if not path_used or was_multipath:
+        if not path_used:
             return False
 
-        # We need to flush the single path that was used.
-        # For encrypted volumes the symlink has been replaced, so realpath
-        # won't return device under /dev but under /dev/disk/...
         path = os.path.realpath(path)
         path_used = os.path.realpath(path_used)
-        return path_used == path or '/dev' != os.path.split(path_used)[0]
+
+        # Need to flush this device if we used this specific path.  We check
+        # this before checking if it's multipath in case we don't detect it
+        # being multipath correctly (as in bug #1897787).
+        if path_used == path:
+            return True
+
+        # We flush individual path if Nova didn't use a multipath and we
+        # replaced the symlink to a real device with a link to the decrypted
+        # DM.  We know we replaced it because it doesn't link to /dev/XYZ,
+        # instead it maps to /dev/mapped/crypt-XYZ
+        return not was_multipath and '/dev' != os.path.split(path_used)[0]
 
     def remove_connection(self, devices_names, is_multipath, force=False,
                           exc=None, path_used=None, was_multipath=False):

os_brick/tests/initiator/connectors/test_fibre_channel.py

@@ -474,6 +474,9 @@ class FibreChannelConnectorTestCase(test_connector.ConnectorTestCase):
             find_mp_dev_mock, 'rw', False)
 
         self.assertNotIn('multipathd_id', connection_info['data'])
+        # Ensure we don't call it with the real path
+        device_name = discover_mp_dev_mock.call_args[0][-1]
+        self.assertNotEqual(realpath_mock.return_value, device_name)
 
     @mock.patch.object(fibre_channel.FibreChannelConnector, 'get_volume_paths')
     def test_extend_volume_no_path(self, mock_volume_paths):
@@ -731,6 +734,8 @@ class FibreChannelConnectorTestCase(test_connector.ConnectorTestCase):
 
     @ddt.data(('/dev/mapper/<WWN>', True),
               ('/dev/mapper/mpath0', True),
+              # Check real devices are properly detected as non multipaths
+              ('/dev/sda', False),
               ('/dev/disk/by-path/pci-1-fc-1-lun-1', False))
     @ddt.unpack
     @mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.remove_scsi_device')

os_brick/tests/initiator/test_linuxscsi.py

@@ -1141,8 +1141,15 @@ loop0                                     0"""
     def test_get_dev_path_no_path(self, con_props, dev_info):
         self.assertEqual('', self.linuxscsi.get_dev_path(con_props, dev_info))
 
-    @ddt.data(('/dev/sda', '/dev/sda', True, False, None),
+    @ddt.data(('/dev/sda', '/dev/sda', False, True, None),
+              # This checks that we ignore the was_multipath parameter if it
+              # doesn't make sense (because the used path is the one we are
+              # asking about)
+              ('/dev/sda', '/dev/sda', True, True, None),
               ('/dev/sda', '', True, False, None),
+              # Check for encrypted volume
+              ('/dev/link_sda', '/dev/disk/by-path/pci-XYZ', False, True,
+               ('/dev/sda', '/dev/mapper/crypt-pci-XYZ')),
               ('/dev/link_sda', '/dev/link_sdb', False, False, ('/dev/sda',
                                                                 '/dev/sdb')),
               ('/dev/link_sda', '/dev/link2_sda', False, True, ('/dev/sda',

releasenotes/notes/fc-flush-single-path-22ed6cc7b56a6d9b.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    `Bug #1897787 <https://bugs.launchpad.net/cinder/+bug/1897787>`_: Fix
+    Fibre Channel not flushing volumes on detach when a multipath connection
+    was requested on their attach, but one was not found.