From 46726f762fd475a4761bda3040023a685d588be7 Mon Sep 17 00:00:00 2001
From: Helen Walsh <helen.walsh@emc.com>
Date: Thu, 29 Sep 2016 15:41:45 +0100
Subject: [PATCH] VMAX driver - PortGroup rollback error path scenario.

An errorMessage is being overwritten by
_get_port_group_name_from_mv if an existing masking view
component (the initiator group or storage group) could not
be verified, but the masking view and port group still
exist on the array. This would cause the errorMessage to
be set to None (as port group can successfully be
retrieved) and the code would continue on, causing errors
later. Even if there is an error in retrieving the port
group, _get_port_group_name_from_mv does not correctly
return the errorMessage variable to the calling function.

Change-Id: I20f5473ccfa59019fd9b638f85e9c89dccc54b20
Closes-Bug: #1578714
---
 .../unit/volume/drivers/emc/test_emc_vmax.py  | 65 +++++++++++++++++++
 cinder/volume/drivers/emc/emc_vmax_masking.py |  5 +-
 2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py b/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py
index 3b21dfce235..26ce08ef905 100644
--- a/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py
+++ b/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py
@@ -8102,6 +8102,71 @@ class EMCVMAXMaskingTest(test.TestCase):
                     extraSpecs)
                 self.assertFalse(verify)
 
+    @mock.patch.object(
+        emc_vmax_masking.EMCVMAXMasking,
+        "_check_adding_volume_to_storage_group",
+        return_value=None)
+    @mock.patch.object(
+        emc_vmax_masking.EMCVMAXMasking,
+        "_validate_masking_view",
+        return_value=("mv_instance", "sg_instance", None))
+    @mock.patch.object(
+        emc_vmax_masking.EMCVMAXMasking,
+        "_get_and_remove_from_storage_group_v3")
+    @mock.patch.object(
+        emc_vmax_masking.EMCVMAXMasking,
+        '_check_if_rollback_action_for_masking_required')
+    def test_get_or_create_masking_view_and_map_lun(self, check_rb, rm_sg,
+                                                    validate_mv, check_sg):
+        common = self.driver.common
+        common.conn = self.fake_ecom_connection()
+        masking = common.masking
+        connector = self.data.connector
+        extraSpecs = self.data.extra_specs
+        controllerConfigService = (
+            self.driver.utils.find_controller_configuration_service(
+                common.conn, self.data.storage_system))
+        defaultStorageGroupInstanceName = (
+            {'CreationClassName': 'CIM_DeviceMaskingGroup',
+             'ElementName': 'OS-SRP_1-Bronze-DSS-SG'})
+        volumeInstanceName = (
+            common.conn.EnumerateInstanceNames("EMC_StorageVolume")[0])
+        volumeInstance = common.conn.GetInstance(volumeInstanceName)
+        with mock.patch.object(common, '_find_lun',
+                               return_value=volumeInstance):
+            maskingViewDict = common._populate_masking_dict(
+                self.data.test_volume_v3, connector, extraSpecs)
+        maskingViewDict['isLiveMigration'] = False
+        rollbackDict = {}
+        rollbackDict['controllerConfigService'] = controllerConfigService
+        rollbackDict['defaultStorageGroupInstanceName'] = (
+            defaultStorageGroupInstanceName)
+        rollbackDict['volumeInstance'] = volumeInstance
+        rollbackDict['volumeName'] = self.data.test_volume_v3['name']
+        rollbackDict['fastPolicyName'] = None
+        rollbackDict['isV3'] = True
+        rollbackDict['extraSpecs'] = extraSpecs
+        rollbackDict['sgGroupName'] = 'OS-fakehost-SRP_1-Bronze-DSS-I-SG'
+        rollbackDict['igGroupName'] = self.data.initiatorgroup_name
+        rollbackDict['pgGroupName'] = self.data.port_group
+        rollbackDict['connector'] = self.data.connector
+        # path 1: masking view creation or retrieval is successful
+        with mock.patch.object(masking, "_get_port_group_name_from_mv",
+                               return_value=(self.data.port_group, None)):
+            deviceDict = masking.get_or_create_masking_view_and_map_lun(
+                common.conn, maskingViewDict, extraSpecs)
+            (masking._check_if_rollback_action_for_masking_required.
+             assert_not_called())
+            self.assertEqual(rollbackDict, deviceDict)
+        # path 2: masking view creation or retrieval is unsuccessful
+        with mock.patch.object(masking, "_get_port_group_name_from_mv",
+                               return_value=(None, "error_message")):
+            rollbackDict['storageSystemName'] = self.data.storage_system
+            rollbackDict['slo'] = u'Bronze'
+            self.assertRaises(exception.VolumeBackendAPIException,
+                              masking.get_or_create_masking_view_and_map_lun,
+                              common.conn, maskingViewDict, extraSpecs)
+
 
 class EMCVMAXFCTest(test.TestCase):
     def setUp(self):
diff --git a/cinder/volume/drivers/emc/emc_vmax_masking.py b/cinder/volume/drivers/emc/emc_vmax_masking.py
index dbf2ba51cbd..5c0854a7a3b 100644
--- a/cinder/volume/drivers/emc/emc_vmax_masking.py
+++ b/cinder/volume/drivers/emc/emc_vmax_masking.py
@@ -132,11 +132,14 @@ class EMCVMAXMasking(object):
                 {'maskingViewName': maskingViewDict['maskingViewName']})
             errorMessage = e
 
-        rollbackDict['pgGroupName'], errorMessage = (
+        rollbackDict['pgGroupName'], pg_errorMessage = (
             self._get_port_group_name_from_mv(
                 conn, maskingViewDict['maskingViewName'],
                 maskingViewDict['storageSystemName']))
 
+        if pg_errorMessage:
+            errorMessage = pg_errorMessage
+
         if not errorMessage:
             # Only after the masking view has been validated, add the
             # volume to the storage group and recheck that it has been