From 89470bf87de7176ec78e0bb6053dd662d032d6c0 Mon Sep 17 00:00:00 2001 From: Mark Sturdevant Date: Tue, 12 Aug 2014 17:18:54 -0700 Subject: [PATCH] HP 3PAR manage_existing with volume-type support Make manage_existing() modify and tune the volume based on the volume-type. Leverages retype() code. Partially Implements: blueprint add-export-import-volumes Change-Id: I2923596a6f2299788a7182fb86a5dfc897436302 --- cinder/tests/test_hp3par.py | 207 +++++++++++++++--- .../volume/drivers/san/hp/hp_3par_common.py | 118 +++++++--- 2 files changed, 264 insertions(+), 61 deletions(-) diff --git a/cinder/tests/test_hp3par.py b/cinder/tests/test_hp3par.py index 86f7374f5..8a159ea88 100644 --- a/cinder/tests/test_hp3par.py +++ b/cinder/tests/test_hp3par.py @@ -17,6 +17,8 @@ import mock +import ast + from cinder import context from cinder import exception from cinder.openstack.common import log as logging @@ -47,6 +49,16 @@ CHAP_PASS_KEY = "HPQ-cinder-CHAP-secret" class HP3PARBaseDriver(object): + class CommentMatcher(object): + def __init__(self, f, expect): + self.assertEqual = f + self.expect = expect + + def __eq__(self, actual): + actual_as_dict = dict(ast.literal_eval(actual)) + self.assertEqual(self.expect, actual_as_dict) + return True + VOLUME_ID = 'd03338a9-9115-48a3-8dfc-35cdfcdc15a7' CLONE_ID = 'd03338a9-9115-48a3-8dfc-000000000000' VOLUME_NAME = 'volume-' + VOLUME_ID @@ -269,6 +281,13 @@ class HP3PARBaseDriver(object): } } + MANAGE_VOLUME_INFO = { + 'userCPG': 'testUserCpg0', + 'snapCPG': 'testSnapCpg0', + 'provisioningType': 1, + 'comment': "{'display_name': 'Foo Volume'}" + } + RETYPE_TEST_COMMENT = "{'retype_test': 'test comment'}" RETYPE_VOLUME_INFO_0 = { @@ -1443,68 +1462,148 @@ class HP3PARBaseDriver(object): @mock.patch.object(volume_types, 'get_volume_type') def test_manage_existing(self, _mock_volume_types): + _mock_volume_types.return_value = self.volume_type mock_client = self.setup_driver() - _mock_volume_types.return_value = { - 'name': 'gold', - 'extra_specs': { - 'cpg': HP3PAR_CPG, - 'snap_cpg': HP3PAR_CPG_SNAP, - 'vvs_name': self.VVS_NAME, - 'qos': self.QOS, - 'tpvv': True, - 'volume_type': self.volume_type}} - comment = ( - '{"display_name": "Foo Volume"}') - new_comment = ( - '{"volume_type_name": "gold",' - ' "display_name": "Foo Volume",' - ' "name": "volume-007dbfce-7579-40bc-8f90-a20b3902283e",' - ' "volume_type_id": "acfa9fa4-54a0-4340-a3d8-bfcf19aea65e",' - ' "volume_id": "007dbfce-7579-40bc-8f90-a20b3902283e",' - ' "qos": {},' - ' "type": "OpenStack"}') + new_comment = {"display_name": "Foo Volume", + "name": "volume-007dbfce-7579-40bc-8f90-a20b3902283e", + "volume_id": "007dbfce-7579-40bc-8f90-a20b3902283e", + "type": "OpenStack"} + volume = {'display_name': None, 'volume_type': 'gold', 'volume_type_id': 'acfa9fa4-54a0-4340-a3d8-bfcf19aea65e', 'id': '007dbfce-7579-40bc-8f90-a20b3902283e'} - mock_client.getVolume.return_value = {'comment': comment} + mock_client.getVolume.return_value = self.MANAGE_VOLUME_INFO + mock_client.modifyVolume.return_value = ("anyResponse", {'taskid': 1}) + mock_client.getTask.return_value = self.STATUS_DONE unm_matcher = self.driver.common._get_3par_unm_name(self.volume['id']) osv_matcher = self.driver.common._get_3par_vol_name(volume['id']) + vvs_matcher = self.driver.common._get_3par_vvs_name(volume['id']) existing_ref = {'source-name': unm_matcher} obj = self.driver.manage_existing(volume, existing_ref) expected_obj = {'display_name': 'Foo Volume'} - expected = [ + + expected_manage = [ mock.call.login(HP3PAR_USER_NAME, HP3PAR_USER_PASS), mock.call.getVolume(existing_ref['source-name']), mock.call.modifyVolume(existing_ref['source-name'], {'newName': osv_matcher, - 'comment': new_comment}), + 'comment': self.CommentMatcher( + self.assertEqual, new_comment)}), + ] + + retype_comment_qos = { + "display_name": "Foo Volume", + "volume_type_name": self.volume_type['name'], + "volume_type_id": self.volume_type['id'], + "qos": { + 'maxIOPS': '1000', + 'maxBWS': '50', + 'minIOPS': '100', + 'minBWS': '25', + 'latency': '25', + 'priority': 'low' + } + } + + expected_retype_modify = [ + mock.call.modifyVolume(osv_matcher, + {'comment': self.CommentMatcher( + self.assertEqual, retype_comment_qos), + 'snapCPG': 'OpenStackCPGSnap'}), + mock.call.deleteVolumeSet(vvs_matcher), + ] + + expected_retype_specs = [ + mock.call.createVolumeSet(vvs_matcher, None), + mock.call.createQoSRules( + vvs_matcher, + {'ioMinGoal': 100, 'ioMaxLimit': 1000, + 'bwMinGoalKB': 25600, 'priority': 1, 'latencyGoal': 25, + 'bwMaxLimitKB': 51200}), + mock.call.addVolumeToVolumeSet(vvs_matcher, osv_matcher), + mock.call.modifyVolume(osv_matcher, + {'action': 6, 'userCPG': 'OpenStackCPG', + 'conversionOperation': 1, + 'tuneOperation': 1}), + mock.call.getTask(1), mock.call.logout() ] - mock_client.assert_has_calls(expected) + mock_client.assert_has_calls(expected_manage) + mock_client.assert_has_calls(expected_retype_modify) + mock_client.assert_has_calls(expected_retype_specs) self.assertEqual(expected_obj, obj) - volume['display_name'] = 'Test Volume' + @mock.patch.object(volume_types, 'get_volume_type') + def test_manage_existing_vvs(self, _mock_volume_types): + test_volume_type = self.RETYPE_VOLUME_TYPE_2 + vvs = test_volume_type['extra_specs']['vvs'] + _mock_volume_types.return_value = test_volume_type + mock_client = self.setup_driver() + + mock_client.getVolume.return_value = self.MANAGE_VOLUME_INFO + mock_client.modifyVolume.return_value = ("anyResponse", {'taskid': 1}) + mock_client.getTask.return_value = self.STATUS_DONE + + id = '007abcde-7579-40bc-8f90-a20b3902283e' + new_comment = {"display_name": "Test Volume", + "name": ("volume-%s" % id), + "volume_id": id, + "type": "OpenStack"} + + volume = {'display_name': 'Test Volume', + 'volume_type': 'gold', + 'volume_type_id': 'acfa9fa4-54a0-4340-a3d8-bfcf19aea65e', + 'id': id} + + unm_matcher = self.driver.common._get_3par_unm_name(self.volume['id']) + osv_matcher = self.driver.common._get_3par_vol_name(volume['id']) + vvs_matcher = self.driver.common._get_3par_vvs_name(volume['id']) + + existing_ref = {'source-name': unm_matcher} obj = self.driver.manage_existing(volume, existing_ref) expected_obj = {'display_name': 'Test Volume'} - expected = [ + expected_manage = [ mock.call.login(HP3PAR_USER_NAME, HP3PAR_USER_PASS), mock.call.getVolume(existing_ref['source-name']), mock.call.modifyVolume(existing_ref['source-name'], {'newName': osv_matcher, - 'comment': new_comment}), + 'comment': self.CommentMatcher( + self.assertEqual, new_comment)}) + ] + + retype_comment_vvs = { + "display_name": "Foo Volume", + "volume_type_name": test_volume_type['name'], + "volume_type_id": test_volume_type['id'], + "vvs": vvs + } + + expected_retype = [ + mock.call.modifyVolume(osv_matcher, + {'comment': self.CommentMatcher( + self.assertEqual, retype_comment_vvs), + 'snapCPG': 'OpenStackCPGSnap'}), + mock.call.deleteVolumeSet(vvs_matcher), + mock.call.addVolumeToVolumeSet(vvs, osv_matcher), + mock.call.modifyVolume(osv_matcher, + {'action': 6, 'userCPG': 'OpenStackCPG', + 'conversionOperation': 1, + 'tuneOperation': 1}), + mock.call.getTask(1), mock.call.logout() ] - mock_client.assert_has_calls(expected) + mock_client.assert_has_calls(expected_manage) + mock_client.assert_has_calls(expected_retype) self.assertEqual(expected_obj, obj) def test_manage_existing_no_volume_type(self): @@ -1519,6 +1618,7 @@ class HP3PARBaseDriver(object): ' "volume_id": "007dbfce-7579-40bc-8f90-a20b3902283e"}') volume = {'display_name': None, 'volume_type': None, + 'volume_type_id': None, 'id': '007dbfce-7579-40bc-8f90-a20b3902283e'} mock_client.getVolume.return_value = {'comment': comment} @@ -1630,6 +1730,59 @@ class HP3PARBaseDriver(object): mock_client.assert_has_calls(expected) + @mock.patch.object(volume_types, 'get_volume_type') + def test_manage_existing_retype_exception(self, _mock_volume_types): + mock_client = self.setup_driver() + _mock_volume_types.return_value = { + 'name': 'gold', + 'id': 'gold-id', + 'extra_specs': { + 'cpg': HP3PAR_CPG, + 'snap_cpg': HP3PAR_CPG_SNAP, + 'vvs_name': self.VVS_NAME, + 'qos': self.QOS, + 'tpvv': True, + 'volume_type': self.volume_type}} + + volume = {'display_name': None, + 'volume_type': 'gold', + 'volume_type_id': 'bcfa9fa4-54a0-4340-a3d8-bfcf19aea65e', + 'id': '007dbfce-7579-40bc-8f90-a20b3902283e'} + + mock_client.getVolume.return_value = self.MANAGE_VOLUME_INFO + mock_client.modifyVolume.return_value = ("anyResponse", {'taskid': 1}) + mock_client.getTask.return_value = self.STATUS_DONE + mock_client.getCPG.side_effect = [ + {'domain': 'domain1'}, + {'domain': 'domain2'} + ] + + unm_matcher = self.driver.common._get_3par_unm_name(self.volume['id']) + osv_matcher = self.driver.common._get_3par_vol_name(volume['id']) + + existing_ref = {'source-name': unm_matcher} + + self.assertRaises(exception.Invalid3PARDomain, + self.driver.manage_existing, + volume=volume, + existing_ref=existing_ref) + + expected = [ + mock.call.login(HP3PAR_USER_NAME, HP3PAR_USER_PASS), + mock.call.getVolume(unm_matcher), + mock.call.modifyVolume( + unm_matcher, {'newName': osv_matcher, 'comment': mock.ANY}), + mock.call.getVolume(osv_matcher), + mock.call.getCPG('testUserCpg0'), + mock.call.getCPG('OpenStackCPG'), + mock.call.modifyVolume( + osv_matcher, {'newName': unm_matcher, + 'comment': self.MANAGE_VOLUME_INFO['comment']}), + mock.call.logout() + ] + + mock_client.assert_has_calls(expected) + def test_manage_existing_get_size(self): mock_client = self.setup_driver() mock_client.getVolume.return_value = {'sizeMiB': 2048} diff --git a/cinder/volume/drivers/san/hp/hp_3par_common.py b/cinder/volume/drivers/san/hp/hp_3par_common.py index 928e8daef..0087c7ca5 100644 --- a/cinder/volume/drivers/san/hp/hp_3par_common.py +++ b/cinder/volume/drivers/san/hp/hp_3par_common.py @@ -144,10 +144,11 @@ class HP3PARCommon(object): 2.0.17 - Added iSCSI CHAP support This update now requires 3.1.3 MU1 firmware and hp3parclient 3.1.0 + 2.0.18 - HP 3PAR manage_existing with volume-type support """ - VERSION = "2.0.17" + VERSION = "2.0.18" stats = {} @@ -293,8 +294,11 @@ class HP3PARCommon(object): {'source-name': } """ # Check for the existence of the virtual volume. + old_comment_str = "" try: vol = self.client.getVolume(existing_ref['source-name']) + if 'comment' in vol: + old_comment_str = vol['comment'] except hpexceptions.HTTPNotFound: err = (_("Virtual volume '%s' doesn't exist on array.") % existing_ref['source-name']) @@ -324,24 +328,15 @@ class HP3PARCommon(object): new_comment['name'] = name new_comment['type'] = 'OpenStack' - # Create new comments for the existing volume depending on - # whether the user's volume type choice. - # TODO(Anthony) when retype is available handle retyping of - # a volume. - if volume['volume_type']: + volume_type = None + if volume['volume_type_id']: try: - settings = self.get_volume_settings_from_type(volume) + volume_type = self._get_volume_type(volume['volume_type_id']) except Exception: reason = (_("Volume type ID '%s' is invalid.") % volume['volume_type_id']) raise exception.ManageExistingVolumeTypeMismatch(reason=reason) - volume_type = self._get_volume_type(volume['volume_type_id']) - - new_comment['volume_type_name'] = volume_type['name'] - new_comment['volume_type_id'] = volume['volume_type_id'] - new_comment['qos'] = settings['qos'] - # Update the existing volume with the new name and comments. self.client.modifyVolume(existing_ref['source-name'], {'newName': new_vol_name, @@ -349,6 +344,28 @@ class HP3PARCommon(object): LOG.info(_("Virtual volume '%(ref)s' renamed to '%(new)s'.") % {'ref': existing_ref['source-name'], 'new': new_vol_name}) + + if volume_type: + LOG.info(_("Virtual volume %(disp)s '%(new)s' is being retyped.") % + {'disp': display_name, 'new': new_vol_name}) + + try: + self._retype_from_no_type(volume, volume_type) + LOG.info(_("Virtual volume %(disp)s successfully retyped to " + "%(new_type)s.") % + {'disp': display_name, + 'new_type': volume_type.get('name')}) + except Exception: + with excutils.save_and_reraise_exception(): + LOG.warning(_("Failed to manage virtual volume %(disp)s " + "due to error during retype.") % + {'disp': display_name}) + # Try to undo the rename and clear the new comment. + self.client.modifyVolume( + new_vol_name, + {'newName': existing_ref['source-name'], + 'comment': old_comment_str}) + LOG.info(_("Virtual volume %(disp)s '%(new)s' is now being managed.") % {'disp': display_name, 'new': new_vol_name}) @@ -1610,17 +1627,19 @@ class HP3PARCommon(object): if new_persona: self.validate_persona(new_persona) - (host_type, host_id, host_cpg) = ( - host['capabilities']['location_info']).split(':') + if host is not None: + (host_type, host_id, host_cpg) = ( + host['capabilities']['location_info']).split(':') - if not (host_type == 'HP3PARDriver'): - reason = (_("Cannot retype from HP3PARDriver to %s.") % host_type) - raise exception.InvalidHost(reason) + if not (host_type == 'HP3PARDriver'): + reason = (_("Cannot retype from HP3PARDriver to %s.") % + host_type) + raise exception.InvalidHost(reason) - sys_info = self.client.getStorageSystemInfo() - if not (host_id == sys_info['serialNumber']): - reason = (_("Cannot retype from one 3PAR array to another.")) - raise exception.InvalidHost(reason) + sys_info = self.client.getStorageSystemInfo() + if not (host_id == sys_info['serialNumber']): + reason = (_("Cannot retype from one 3PAR array to another.")) + raise exception.InvalidHost(reason) if not old_snap_cpg: reason = (_("Invalid current snapCPG name for retype. The volume " @@ -1678,27 +1697,23 @@ class HP3PARCommon(object): 'old_comment': old_comment }) - def retype(self, volume, new_type, diff, host): - """Convert the volume to be of the new type. + def _retype_from_old_to_new(self, volume, new_type, old_volume_settings, + host): + """Convert the volume to be of the new type. Given old type settings. Returns True if the retype was successful. Uses taskflow to revert changes if errors occur. :param volume: A dictionary describing the volume to retype :param new_type: A dictionary describing the volume type to convert to - :param diff: A dictionary with the difference between the two types + :param old_volume_settings: Volume settings describing the old type. :param host: A dictionary describing the host, where host['host'] is its name, and host['capabilities'] is a - dictionary of its reported capabilities. + dictionary of its reported capabilities. Host validation + is just skipped if host is None. """ - LOG.debug(("enter: retype: id=%(id)s, new_type=%(new_type)s," - "diff=%(diff)s, host=%(host)s") % {'id': volume['id'], - 'new_type': new_type, - 'diff': diff, - 'host': host}) volume_id = volume['id'] volume_name = self._get_3par_vol_name(volume_id) - new_type_name = new_type['name'] new_type_id = new_type['id'] new_volume_settings = self.get_volume_settings_from_type_id( @@ -1712,8 +1727,6 @@ class HP3PARCommon(object): new_hp3par_keys = new_volume_settings['hp3par_keys'] if 'persona' in new_hp3par_keys: new_persona = new_hp3par_keys['persona'] - - old_volume_settings = self.get_volume_settings_from_type(volume) old_qos = old_volume_settings['qos'] old_vvs = old_volume_settings['vvs_name'] @@ -1738,6 +1751,43 @@ class HP3PARCommon(object): old_vvs, new_vvs, old_qos, new_qos, old_comment) return True + def _retype_from_no_type(self, volume, new_type): + """Convert the volume to be of the new type. Starting from no type. + + Returns True if the retype was successful. + Uses taskflow to revert changes if errors occur. + + :param volume: A dictionary describing the volume to retype. Except the + volume-type is not used here. This method uses None. + :param new_type: A dictionary describing the volume type to convert to + """ + none_type_settings = self.get_volume_settings_from_type_id(None) + return self._retype_from_old_to_new(volume, new_type, + none_type_settings, None) + + def retype(self, volume, new_type, diff, host): + """Convert the volume to be of the new type. + + Returns True if the retype was successful. + Uses taskflow to revert changes if errors occur. + + :param volume: A dictionary describing the volume to retype + :param new_type: A dictionary describing the volume type to convert to + :param diff: A dictionary with the difference between the two types + :param host: A dictionary describing the host, where + host['host'] is its name, and host['capabilities'] is a + dictionary of its reported capabilities. Host validation + is just skipped if host is None. + """ + LOG.debug(("enter: retype: id=%(id)s, new_type=%(new_type)s," + "diff=%(diff)s, host=%(host)s") % {'id': volume['id'], + 'new_type': new_type, + 'diff': diff, + 'host': host}) + old_volume_settings = self.get_volume_settings_from_type(volume) + return self._retype_from_old_to_new(volume, new_type, + old_volume_settings, host) + class TaskWaiter(object): """TaskWaiter waits for task to be not active and returns status.""" -- 2.45.2