VMAX driver - Detaching volumes if part of two or more MVs

When Live migration is used extensively there can be scenarios
where a regular attached volume can belong to two or more Masking
Views. Because of this, we did not remove the volume from the storage
group, which is not typical behaviour. In this fix we use a temporary
file to determine if a terminate_connection is a regular detach or a
part of the live migration process.

Change-Id: Ide38fa21d65859a5516c577a9983124d998a2e95
Closes-Bug: #1684595

cinder/tests/unit/volume/drivers/dell_emc/test_vmax.py

@@ -16,6 +16,7 @@
 import ast
 import os
 import shutil
+import sys
 import tempfile
 import unittest
 import uuid
@@ -3431,6 +3432,9 @@ class VMAXISCSIDriverNoFastTestCase(test.TestCase):
                           self.driver.delete_volume,
                           self.data.failed_delete_vol)
 
+    @mock.patch.object(
+        utils.VMAXUtils,
+        'insert_live_migration_record')
     @mock.patch.object(
         common.VMAXCommon,
         '_is_same_host',
@@ -3451,7 +3455,7 @@ class VMAXISCSIDriverNoFastTestCase(test.TestCase):
         return_value={'volume_backend_name': 'ISCSINoFAST'})
     def test_already_mapped_no_fast_success(
             self, _mock_volume_type, mock_wrap_group, mock_wrap_device,
-            mock_is_same_host):
+            mock_is_same_host, mock_rec):
         self.driver.common._get_correct_port_group = mock.Mock(
             return_value=self.data.port_group)
         self.driver.initialize_connection(self.data.test_volume,
@@ -3481,6 +3485,9 @@ class VMAXISCSIDriverNoFastTestCase(test.TestCase):
         self.driver.initialize_connection(self.data.test_volume,
                                           self.data.connector)
 
+    @mock.patch.object(
+        utils.VMAXUtils,
+        'insert_live_migration_record')
     @mock.patch.object(
         common.VMAXCommon,
         '_get_port_group_from_source',
@@ -3518,7 +3525,8 @@ class VMAXISCSIDriverNoFastTestCase(test.TestCase):
                                                 mock_device,
                                                 mock_same_host,
                                                 mock_sg_from_mv,
-                                                mock_pg_from_mv):
+                                                mock_pg_from_mv,
+                                                mock_rec):
         extraSpecs = self.data.extra_specs
         rollback_dict = self.driver.common._populate_masking_dict(
             self.data.test_volume, self.data.connector, extraSpecs)
@@ -3528,6 +3536,9 @@ class VMAXISCSIDriverNoFastTestCase(test.TestCase):
             self.driver.initialize_connection(self.data.test_volume,
                                               self.data.connector)
 
+    @mock.patch.object(
+        utils.VMAXUtils,
+        'insert_live_migration_record')
     @mock.patch.object(
         masking.VMAXMasking,
         '_get_initiator_group_from_masking_view',
@@ -3550,7 +3561,7 @@ class VMAXISCSIDriverNoFastTestCase(test.TestCase):
         return_value={'volume_backend_name': 'ISCSINoFAST'})
     def test_map_existing_masking_view_no_fast_success(
             self, _mock_volume_type, mock_wrap_group, mock_storage_group,
-            mock_initiator_group, mock_ig_from_mv):
+            mock_initiator_group, mock_ig_from_mv, mock_rec):
         self.driver.initialize_connection(self.data.test_volume,
                                           self.data.connector)
 
@@ -4447,6 +4458,9 @@ class VMAXISCSIDriverFastTestCase(test.TestCase):
                           self.driver.delete_volume,
                           self.data.failed_delete_vol)
 
+    @mock.patch.object(
+        utils.VMAXUtils,
+        'insert_live_migration_record')
     @mock.patch.object(
         common.VMAXCommon,
         '_is_same_host',
@@ -4467,7 +4481,7 @@ class VMAXISCSIDriverFastTestCase(test.TestCase):
         return_value={'volume_backend_name': 'ISCSIFAST'})
     def test_already_mapped_fast_success(
             self, _mock_volume_type, mock_wrap_group, mock_wrap_device,
-            mock_is_same_host):
+            mock_is_same_host, mock_rec):
         self.driver.common._get_correct_port_group = mock.Mock(
             return_value=self.data.port_group)
         self.driver.initialize_connection(self.data.test_volume,
@@ -5068,6 +5082,9 @@ class VMAXFCDriverNoFastTestCase(test.TestCase):
                           self.driver.delete_volume,
                           self.data.failed_delete_vol)
 
+    @mock.patch.object(
+        utils.VMAXUtils,
+        'insert_live_migration_record')
     @mock.patch.object(
         common.VMAXCommon,
         '_is_same_host',
@@ -5082,7 +5099,8 @@ class VMAXFCDriverNoFastTestCase(test.TestCase):
         return_value={'volume_backend_name': 'FCNoFAST',
                       'FASTPOLICY': 'FC_GOLD1'})
     def test_map_lookup_service_no_fast_success(
-            self, _mock_volume_type, mock_maskingview, mock_is_same_host):
+            self, _mock_volume_type, mock_maskingview, mock_is_same_host,
+            mock_rec):
         self.data.test_volume['volume_name'] = "vmax-1234567"
         common = self.driver.common
         common.get_target_wwns_from_masking_view = mock.Mock(
@@ -5597,6 +5615,9 @@ class VMAXFCDriverFastTestCase(test.TestCase):
                           self.driver.delete_volume,
                           self.data.failed_delete_vol)
 
+    @mock.patch.object(
+        utils.VMAXUtils,
+        'insert_live_migration_record')
     @mock.patch.object(
         common.VMAXCommon,
         '_is_same_host',
@@ -5611,7 +5632,7 @@ class VMAXFCDriverFastTestCase(test.TestCase):
         return_value={'volume_backend_name': 'FCFAST',
                       'FASTPOLICY': 'FC_GOLD1'})
     def test_map_fast_success(self, _mock_volume_type, mock_maskingview,
-                              mock_is_same_host):
+                              mock_is_same_host, mock_rec):
         common = self.driver.common
         common.get_target_wwns_list = mock.Mock(
             return_value=VMAXCommonData.target_wwns)
@@ -6660,6 +6681,9 @@ class EMCV3DriverTestCase(test.TestCase):
             self.data.test_ctxt, self.data.test_CG,
             add_volumes, remove_volumes)
 
+    @mock.patch.object(
+        utils.VMAXUtils,
+        'insert_live_migration_record')
     @mock.patch.object(
         utils.VMAXUtils,
         'get_volume_element_name',
@@ -6678,7 +6702,7 @@ class EMCV3DriverTestCase(test.TestCase):
         return_value={'volume_backend_name': 'V3_BE'})
     def test_map_v3_success(
             self, _mock_volume_type, mock_maskingview, mock_is_same_host,
-            mock_element_name):
+            mock_element_name, mock_rec):
         common = self.driver.common
         common.get_target_wwns_list = mock.Mock(
             return_value=VMAXCommonData.target_wwns)
@@ -8985,6 +9009,59 @@ class VMAXUtilsTest(test.TestCase):
         self.assertRaises(exception.VolumeBackendAPIException,
                           utils.get_array_and_device_id, volume, external_ref)
 
+    @mock.patch('builtins.open' if sys.version_info >= (3,)
+                else '__builtin__.open')
+    def test_insert_live_migration_record(self, mock_open):
+        volume = {'id': '12345678-87654321'}
+        tempdir = tempfile.mkdtemp()
+        utils.LIVE_MIGRATION_FILE = (
+            tempdir + '/livemigrationarray')
+        lm_file_name = ("%(prefix)s-%(volid)s"
+                        % {'prefix': utils.LIVE_MIGRATION_FILE,
+                           'volid': volume['id'][:8]})
+        self.driver.utils.insert_live_migration_record(volume)
+        mock_open.assert_called_once_with(lm_file_name, "w")
+        self.driver.utils.delete_live_migration_record(volume)
+        shutil.rmtree(tempdir)
+
+    def test_delete_live_migration_record(self):
+        volume = {'id': '12345678-87654321'}
+        tempdir = tempfile.mkdtemp()
+        utils.LIVE_MIGRATION_FILE = (
+            tempdir + '/livemigrationarray')
+        lm_file_name = ("%(prefix)s-%(volid)s"
+                        % {'prefix': utils.LIVE_MIGRATION_FILE,
+                           'volid': volume['id'][:8]})
+        m = mock.mock_open()
+        with mock.patch('{}.open'.format(__name__), m, create=True):
+            with open(lm_file_name, "w") as f:
+                f.write('live migration details')
+        self.driver.utils.insert_live_migration_record(volume)
+        self.driver.utils.delete_live_migration_record(volume)
+        m.assert_called_once_with(lm_file_name, "w")
+        shutil.rmtree(tempdir)
+
+    def test_get_live_migration_record(self):
+        volume = {'id': '12345678-87654321'}
+        tempdir = tempfile.mkdtemp()
+        utils.LIVE_MIGRATION_FILE = (
+            tempdir + '/livemigrationarray')
+        lm_file_name = ("%(prefix)s-%(volid)s"
+                        % {'prefix': utils.LIVE_MIGRATION_FILE,
+                           'volid': volume['id'][:8]})
+        self.driver.utils.insert_live_migration_record(volume)
+        record = self.driver.utils.get_live_migration_record(volume)
+        self.assertEqual(volume['id'], record[0])
+        os.remove(lm_file_name)
+        shutil.rmtree(tempdir)
+
+    def test_get_live_migration_file_name(self):
+        volume = {'id': '12345678-87654321'}
+        lm_live_migration = self.driver.utils.get_live_migration_file_name(
+            volume)
+        self.assertIn('/livemigrationarray-12345678', lm_live_migration)
+        self.assertIn('/tmp/', lm_live_migration)
+
 
 class VMAXCommonTest(test.TestCase):
     def setUp(self):

cinder/volume/drivers/dell_emc/vmax/common.py

@@ -527,7 +527,12 @@ class VMAXCommon(object):
         vol_instance = self._find_lun(volume)
         storage_system = vol_instance['SystemName']
 
-        if self._is_volume_multiple_masking_views(vol_instance):
+        livemigrationrecord = self.utils.get_live_migration_record(volume)
+        if livemigrationrecord:
+            self.utils.delete_live_migration_record(volume)
+
+        if livemigrationrecord and self._is_volume_multiple_masking_views(
+                vol_instance):
             return
 
         configservice = self.utils.find_controller_configuration_service(
@@ -613,12 +618,14 @@ class VMAXCommon(object):
                      "The device number is  %(deviceNumber)s.",
                      {'volume': volumeName,
                       'deviceNumber': deviceNumber})
+            self.utils.insert_live_migration_record(volume)
             # Special case, we still need to get the iscsi ip address.
             portGroupName = (
                 self._get_correct_port_group(
                     deviceInfoDict, maskingViewDict['storageSystemName']))
         else:
             if isLiveMigration:
+                self.utils.insert_live_migration_record(volume)
                 maskingViewDict['storageGroupInstanceName'] = (
                     self._get_storage_group_from_source(sourceInfoDict))
                 maskingViewDict['portGroupInstanceName'] = (
@@ -676,6 +683,9 @@ class VMAXCommon(object):
                     (rollbackDict['isV3'] is not None)):
                 (self.masking._check_if_rollback_action_for_masking_required(
                     self.conn, rollbackDict))
+            livemigrationrecord = self.utils.get_live_migration_record(volume)
+            if livemigrationrecord:
+                self.utils.delete_live_migration_record(volume)
             exception_message = (_("Error Attaching volume %(vol)s.")
                                  % {'vol': volumeName})
             raise exception.VolumeBackendAPIException(
@@ -1945,14 +1955,16 @@ class VMAXCommon(object):
         """
         maskedvols = []
         data = {}
+        isLiveMigration = False
+        source_data = {}
         foundController = None
         foundNumDeviceNumber = None
         foundMaskingViewName = None
         volumeName = volume['name']
         volumeInstance = self._find_lun(volume)
         storageSystemName = volumeInstance['SystemName']
-        isLiveMigration = False
+        if not volumeInstance:
-        source_data = {}
+            return data, isLiveMigration, source_data
 
         unitnames = self.conn.ReferenceNames(
             volumeInstance.path,
@@ -1965,7 +1977,15 @@ class VMAXCommon(object):
             if index > -1:
                 unitinstance = self.conn.GetInstance(unitname,
                                                      LocalOnly=False)
-                numDeviceNumber = int(unitinstance['DeviceNumber'], 16)
+                if unitinstance['DeviceNumber']:
+                    numDeviceNumber = int(unitinstance['DeviceNumber'], 16)
+                else:
+                    LOG.debug(
+                        "Device number not found for volume "
+                        "%(volumeName)s %(volumeInstance)s.",
+                        {'volumeName': volumeName,
+                         'volumeInstance': volumeInstance.path})
+                    break
                 foundNumDeviceNumber = numDeviceNumber
                 foundController = controller
                 controllerInstance = self.conn.GetInstance(controller,

cinder/volume/drivers/dell_emc/vmax/masking.py

@@ -2110,10 +2110,16 @@ class VMAXMasking(object):
         self._last_volume_delete_masking_view(
             conn, controllerConfigService, mvInstanceName,
             maskingViewName, extraSpecs)
-        self._last_volume_delete_initiator_group(
+        initiatorGroupInstance = conn.GetInstance(initiatorGroupInstanceName)
-            conn, controllerConfigService,
+        if initiatorGroupInstance:
-            initiatorGroupInstanceName, extraSpecs, host)
+            initiatorGroupName = initiatorGroupInstance['ElementName']
 
+            @coordination.synchronized('emc-ig-{initiatorGroupName}')
+            def inner_do_delete_initiator_group(initiatorGroupName):
+                self._last_volume_delete_initiator_group(
+                    conn, controllerConfigService,
+                    initiatorGroupInstanceName, extraSpecs, host)
+            inner_do_delete_initiator_group(initiatorGroupName)
         if not isV3:
             isTieringPolicySupported, tierPolicyServiceInstanceName = (
                 self._get_tiering_info(conn, storageSystemInstanceName,
@@ -2672,15 +2678,19 @@ class VMAXMasking(object):
         :param hardwareIdManagementService - hardware id management service
         :param hardwareIdPath - The path of the initiator object
         """
-        ret = conn.InvokeMethod('DeleteStorageHardwareID',
+        ret = -1
-                                hardwareIdManagementService,
+        try:
-                                HardwareID = hardwareIdPath)
+            ret = conn.InvokeMethod('DeleteStorageHardwareID',
+                                    hardwareIdManagementService,
+                                    HardwareID = hardwareIdPath)
+        except Exception:
+            pass
         if ret == 0:
             LOG.debug("Deletion of initiator path %(hardwareIdPath)s "
                       "is successful.", {'hardwareIdPath': hardwareIdPath})
         else:
-            LOG.warning("Deletion of initiator path %(hardwareIdPath)s "
+            LOG.debug("Deletion of initiator path %(hardwareIdPath)s "
-                        "is failed.", {'hardwareIdPath': hardwareIdPath})
+                      "failed.", {'hardwareIdPath': hardwareIdPath})
 
     def _delete_initiators_from_initiator_group(self, conn,
                                                 controllerConfigService,
@@ -2745,7 +2755,6 @@ class VMAXMasking(object):
                 "OS-%(shortHostName)s-%(protocol)s-IG"
                 % {'shortHostName': host,
                    'protocol': protocol}))
-
         if initiatorGroupName == defaultInitiatorGroupName:
             maskingViewInstanceNames = (
                 self.get_masking_views_by_initiator_group(

cinder/volume/drivers/dell_emc/vmax/utils.py

@@ -16,12 +16,15 @@
 import ast
 import datetime
 import hashlib
+import os
 import random
 import re
+import tempfile
 import time
 from xml.dom import minidom
 
 from oslo_log import log as logging
+from oslo_serialization import jsonutils
 from oslo_service import loopingcall
 from oslo_utils import units
 import six
@@ -53,7 +56,7 @@ EMC_ROOT = 'root/emc'
 CONCATENATED = 'concatenated'
 CINDER_EMC_CONFIG_FILE_PREFIX = '/etc/cinder/cinder_emc_config_'
 CINDER_EMC_CONFIG_FILE_POSTFIX = '.xml'
-LIVE_MIGRATION_FILE = '/etc/cinder/livemigrationarray'
+LIVE_MIGRATION_FILE = tempfile.gettempdir() + '/livemigrationarray'
 ISCSI = 'iscsi'
 FC = 'fc'
 JOB_RETRIES = 60
@@ -2978,3 +2981,68 @@ class VMAXUtils(object):
         default_dict[INTERVAL] = INTERVAL_10_SEC
         default_dict[RETRIES] = JOB_RETRIES
         return default_dict
+
+    def insert_live_migration_record(self, volume):
+        """Insert a record of live migration destination into a temporary file
+
+        :param volume: the volume dictionary
+        """
+        lm_file_name = self.get_live_migration_file_name(volume)
+        live_migration_details = self.get_live_migration_record(volume)
+        if live_migration_details:
+            return
+        else:
+            live_migration_details = {volume['id']: [volume['id']]}
+        try:
+            with open(lm_file_name, "w") as f:
+                jsonutils.dump(live_migration_details, f)
+        except Exception:
+            exceptionMessage = (_(
+                "Error in processing live migration file."))
+            LOG.exception(exceptionMessage)
+            raise exception.VolumeBackendAPIException(
+                data=exceptionMessage)
+
+    def delete_live_migration_record(self, volume):
+        """Delete record of live migration
+
+        Delete record of live migration destination from file and if
+        after deletion of record, delete file if empty.
+
+        :param volume: the volume dictionary
+        """
+        lm_file_name = self.get_live_migration_file_name(volume)
+        live_migration_details = self.get_live_migration_record(volume)
+        if live_migration_details:
+            if volume['id'] in live_migration_details:
+                os.remove(lm_file_name)
+
+    def get_live_migration_record(self, volume):
+        """get record of live migration destination from a temporary file
+
+        :param volume: the volume dictionary
+        :returns: returns a single record
+        """
+        returned_record = None
+        lm_file_name = self.get_live_migration_file_name(volume)
+        if os.path.isfile(lm_file_name):
+            with open(lm_file_name, "rb") as f:
+                live_migration_details = jsonutils.load(f)
+            if volume['id'] in live_migration_details:
+                returned_record = live_migration_details[volume['id']]
+            else:
+                LOG.debug("%(Volume)s doesn't exist in live migration "
+                          "record.",
+                          {'Volume': volume['id']})
+        return returned_record
+
+    def get_live_migration_file_name(self, volume):
+        """get name of temporary live migration file
+
+        :param volume: the volume dictionary
+        :returns: returns file name
+        """
+        lm_file_name = ("%(prefix)s-%(volid)s"
+                        % {'prefix': LIVE_MIGRATION_FILE,
+                           'volid': volume['id'][:8]})
+        return lm_file_name