From 26de1b1d829849665dae921b8be739194b84515d Mon Sep 17 00:00:00 2001 From: Vincent Hou Date: Fri, 12 Sep 2014 16:10:02 +0800 Subject: [PATCH] IBM Storwize driver: Retype the volume with correct empty QoS * Currently for Storwzie driver, if the new type does not have QoS configurations, the old QoS configurations remain in the volume after retyping it. It should be retyped into a volume with empty QoS for the Storwize driver. * Refactor three dicts into one for better maintainance of the QoS keys for Storwize driver. DocImpact Change-Id: I2b2801a4ef72ef02c11392ed00b56f5263a8a7e4 Closes-Bug: #1368595 --- cinder/tests/test_storwize_svc.py | 126 ++++++++++++++---- .../drivers/ibm/storwize_svc/__init__.py | 10 +- .../drivers/ibm/storwize_svc/helpers.py | 60 +++++++-- 3 files changed, 159 insertions(+), 37 deletions(-) diff --git a/cinder/tests/test_storwize_svc.py b/cinder/tests/test_storwize_svc.py index 0ecaa64e52a..cbe68154093 100644 --- a/cinder/tests/test_storwize_svc.py +++ b/cinder/tests/test_storwize_svc.py @@ -1716,7 +1716,7 @@ class StorwizeSVCDriverTestCase(test.TestCase): # If the qos is not empty, chvdisk should be called # for create_volume. - fake_opts['qos'] = {'rate': 5000} + fake_opts['qos'] = {'IOThrottling': 5000} get_vdisk_params.return_value = fake_opts self.driver.create_volume(vol) self._assert_vol_exists(vol['name'], True) @@ -1854,7 +1854,7 @@ class StorwizeSVCDriverTestCase(test.TestCase): # If the qos is not empty, chvdisk should be called # for create_volume_from_snapshot. - fake_opts['qos'] = {'rate': 5000} + fake_opts['qos'] = {'IOThrottling': 5000} get_vdisk_params.return_value = fake_opts self.driver.create_volume_from_snapshot(vol2, snap1) self._assert_vol_exists(vol2['name'], True) @@ -1876,7 +1876,7 @@ class StorwizeSVCDriverTestCase(test.TestCase): # If the qos is not empty, chvdisk should be called # for create_volume_from_snapshot. - fake_opts['qos'] = {'rate': 5000} + fake_opts['qos'] = {'IOThrottling': 5000} get_vdisk_params.return_value = fake_opts self.driver.create_cloned_volume(vol3, vol2) self._assert_vol_exists(vol3['name'], True) @@ -2442,8 +2442,10 @@ class StorwizeSVCDriverTestCase(test.TestCase): self.driver.migrate_volume(ctxt, volume, host) self._delete_volume(volume) - @mock.patch.object(helpers.StorwizeHelpers, 'add_vdisk_qos') - def test_storwize_svc_retype_no_copy(self, add_vdisk_qos): + @mock.patch.object(helpers.StorwizeHelpers, 'disable_vdisk_qos') + @mock.patch.object(helpers.StorwizeHelpers, 'update_vdisk_qos') + def test_storwize_svc_retype_no_copy(self, update_vdisk_qos, + disable_vdisk_qos): self.driver.do_setup(None) loc = ('StorwizeSVCDriver:' + self.driver._state['system_id'] + ':openstack') @@ -2475,24 +2477,62 @@ class StorwizeSVCDriverTestCase(test.TestCase): self.driver.delete_volume(volume) fake_opts = self._get_default_opts() + fake_opts_old = self._get_default_opts() + fake_opts_old['qos'] = {'IOThrottling': 4000} + fake_opts_qos = self._get_default_opts() + fake_opts_qos['qos'] = {'IOThrottling': 5000} self.driver.create_volume(volume) with mock.patch.object(storwize_svc.StorwizeSVCDriver, '_get_vdisk_params') as get_vdisk_params: - # If qos is empty, chvdisk will not be called for retype. - get_vdisk_params.return_value = fake_opts + # If qos is empty for both the source and target volumes, + # add_vdisk_qos and disable_vdisk_qos will not be called for + # retype. + get_vdisk_params.side_effect = [fake_opts, fake_opts] self.driver.retype(ctxt, volume, new_type, diff, host) - self.assertFalse(add_vdisk_qos.called) + self.assertFalse(update_vdisk_qos.called) + self.assertFalse(disable_vdisk_qos.called) self.driver.delete_volume(volume) self.driver.create_volume(volume) - add_vdisk_qos.reset_mock() + update_vdisk_qos.reset_mock() with mock.patch.object(storwize_svc.StorwizeSVCDriver, '_get_vdisk_params') as get_vdisk_params: - # If qos is not empty, chvdisk will be called for retype. - fake_opts['qos'] = {'rate': 5000} - get_vdisk_params.return_value = fake_opts + # If qos is specified for both source and target volumes, + # add_vdisk_qos will be called for retype, and disable_vdisk_qos + # will not be called. + get_vdisk_params.side_effect = [fake_opts_old, fake_opts_qos] self.driver.retype(ctxt, volume, new_type, diff, host) - add_vdisk_qos.assert_called_with(volume['name'], fake_opts['qos']) + update_vdisk_qos.assert_called_with(volume['name'], + fake_opts_qos['qos']) + self.assertFalse(disable_vdisk_qos.called) + self.driver.delete_volume(volume) + + self.driver.create_volume(volume) + update_vdisk_qos.reset_mock() + with mock.patch.object(storwize_svc.StorwizeSVCDriver, + '_get_vdisk_params') as get_vdisk_params: + # If qos is empty for source and speficied for target volume, + # add_vdisk_qos will be called for retype, and disable_vdisk_qos + # will not be called. + get_vdisk_params.side_effect = [fake_opts, fake_opts_qos] + self.driver.retype(ctxt, volume, new_type, diff, host) + update_vdisk_qos.assert_called_with(volume['name'], + fake_opts_qos['qos']) + self.assertFalse(disable_vdisk_qos.called) + self.driver.delete_volume(volume) + + self.driver.create_volume(volume) + update_vdisk_qos.reset_mock() + with mock.patch.object(storwize_svc.StorwizeSVCDriver, + '_get_vdisk_params') as get_vdisk_params: + # If qos is empty for target volume and specified for source + # volume, add_vdisk_qos will not be called for retype, and + # disable_vdisk_qos will be called. + get_vdisk_params.side_effect = [fake_opts_qos, fake_opts] + self.driver.retype(ctxt, volume, new_type, diff, host) + self.assertFalse(update_vdisk_qos.called) + disable_vdisk_qos.assert_called_with(volume['name'], + fake_opts_qos['qos']) self.driver.delete_volume(volume) def test_storwize_svc_retype_only_change_iogrp(self): @@ -2525,8 +2565,10 @@ class StorwizeSVCDriverTestCase(test.TestCase): 'failed') self.driver.delete_volume(volume) - @mock.patch.object(helpers.StorwizeHelpers, 'add_vdisk_qos') - def test_storwize_svc_retype_need_copy(self, add_vdisk_qos): + @mock.patch.object(helpers.StorwizeHelpers, 'disable_vdisk_qos') + @mock.patch.object(helpers.StorwizeHelpers, 'update_vdisk_qos') + def test_storwize_svc_retype_need_copy(self, update_vdisk_qos, + disable_vdisk_qos): self.driver.do_setup(None) loc = ('StorwizeSVCDriver:' + self.driver._state['system_id'] + ':openstack') @@ -2558,24 +2600,62 @@ class StorwizeSVCDriverTestCase(test.TestCase): self.driver.delete_volume(volume) fake_opts = self._get_default_opts() + fake_opts_old = self._get_default_opts() + fake_opts_old['qos'] = {'IOThrottling': 4000} + fake_opts_qos = self._get_default_opts() + fake_opts_qos['qos'] = {'IOThrottling': 5000} self.driver.create_volume(volume) with mock.patch.object(storwize_svc.StorwizeSVCDriver, '_get_vdisk_params') as get_vdisk_params: - # If qos is empty, chvdisk will not be called for retype. - get_vdisk_params.return_value = fake_opts + # If qos is empty for both the source and target volumes, + # add_vdisk_qos and disable_vdisk_qos will not be called for + # retype. + get_vdisk_params.side_effect = [fake_opts, fake_opts] self.driver.retype(ctxt, volume, new_type, diff, host) - self.assertFalse(add_vdisk_qos.called) + self.assertFalse(update_vdisk_qos.called) + self.assertFalse(disable_vdisk_qos.called) self.driver.delete_volume(volume) - add_vdisk_qos.reset_mock() self.driver.create_volume(volume) + update_vdisk_qos.reset_mock() with mock.patch.object(storwize_svc.StorwizeSVCDriver, '_get_vdisk_params') as get_vdisk_params: - # If qos is not empty, chvdisk will be called for retype. - fake_opts['qos'] = {'rate': 5000} - get_vdisk_params.return_value = fake_opts + # If qos is specified for both source and target volumes, + # add_vdisk_qos will be called for retype, and disable_vdisk_qos + # will not be called. + get_vdisk_params.side_effect = [fake_opts_old, fake_opts_qos] self.driver.retype(ctxt, volume, new_type, diff, host) - add_vdisk_qos.assert_called_with(volume['name'], fake_opts['qos']) + update_vdisk_qos.assert_called_with(volume['name'], + fake_opts_qos['qos']) + self.assertFalse(disable_vdisk_qos.called) + self.driver.delete_volume(volume) + + self.driver.create_volume(volume) + update_vdisk_qos.reset_mock() + with mock.patch.object(storwize_svc.StorwizeSVCDriver, + '_get_vdisk_params') as get_vdisk_params: + # If qos is empty for source and speficied for target volume, + # add_vdisk_qos will be called for retype, and disable_vdisk_qos + # will not be called. + get_vdisk_params.side_effect = [fake_opts, fake_opts_qos] + self.driver.retype(ctxt, volume, new_type, diff, host) + update_vdisk_qos.assert_called_with(volume['name'], + fake_opts_qos['qos']) + self.assertFalse(disable_vdisk_qos.called) + self.driver.delete_volume(volume) + + self.driver.create_volume(volume) + update_vdisk_qos.reset_mock() + with mock.patch.object(storwize_svc.StorwizeSVCDriver, + '_get_vdisk_params') as get_vdisk_params: + # If qos is empty for target volume and specified for source + # volume, add_vdisk_qos will not be called for retype, and + # disable_vdisk_qos will be called. + get_vdisk_params.side_effect = [fake_opts_qos, fake_opts] + self.driver.retype(ctxt, volume, new_type, diff, host) + self.assertFalse(update_vdisk_qos.called) + disable_vdisk_qos.assert_called_with(volume['name'], + fake_opts_qos['qos']) self.driver.delete_volume(volume) def test_set_storage_code_level_success(self): diff --git a/cinder/volume/drivers/ibm/storwize_svc/__init__.py b/cinder/volume/drivers/ibm/storwize_svc/__init__.py index d6ad506aa8f..62ad086cff5 100644 --- a/cinder/volume/drivers/ibm/storwize_svc/__init__.py +++ b/cinder/volume/drivers/ibm/storwize_svc/__init__.py @@ -920,9 +920,13 @@ class StorwizeSVCDriver(san.SanDriver): self._helpers.change_vdisk_options(volume['name'], vdisk_changes, new_opts, self._state) - qos = new_opts['qos'] or old_opts['qos'] - if qos: - self._helpers.add_vdisk_qos(volume['name'], qos) + if new_opts['qos']: + # Add the new QoS setting to the volume. If the volume has an + # old QoS setting, it will be overwritten. + self._helpers.update_vdisk_qos(volume['name'], new_opts['qos']) + elif old_opts['qos']: + # If the old_opts contain QoS keys, disable them. + self._helpers.disable_vdisk_qos(volume['name'], old_opts['qos']) # Add replica if needed if not old_type_replication and new_type_replication: diff --git a/cinder/volume/drivers/ibm/storwize_svc/helpers.py b/cinder/volume/drivers/ibm/storwize_svc/helpers.py index c71f21fed2e..6f854021104 100644 --- a/cinder/volume/drivers/ibm/storwize_svc/helpers.py +++ b/cinder/volume/drivers/ibm/storwize_svc/helpers.py @@ -38,8 +38,14 @@ LOG = logging.getLogger(__name__) class StorwizeHelpers(object): - svc_qos_keys = {'IOThrottling': int} - svc_qos_param_dict = {'IOThrottling': 'rate'} + # All the supported QoS key are saved in this dict. When a new + # key is going to add, three values MUST be set: + # 'default': to indicate the value, when the parameter is disabled. + # 'param': to indicate the corresponding parameter in the command. + # 'type': to indicate the type of this value. + svc_qos_keys = {'IOThrottling': {'default': '0', + 'param': 'rate', + 'type': int}} def __init__(self, run_ssh): self.ssh = storwize_ssh.StorwizeSSH(run_ssh) @@ -487,12 +493,13 @@ class StorwizeHelpers(object): # Add the QoS. if scope and scope == 'qos': - type_fn = self.svc_qos_keys[key] - try: - value = type_fn(value) - qos[self.svc_qos_param_dict[key]] = value - except ValueError: - continue + if key in self.svc_qos_keys.keys(): + try: + type_fn = self.svc_qos_keys[key]['type'] + value = type_fn(value) + qos[key] = value + except ValueError: + continue # Any keys that the driver should look at should have the # 'drivers' scope. @@ -525,10 +532,10 @@ class StorwizeHelpers(object): # Add the QoS. if scope and scope == 'qos': if key in self.svc_qos_keys.keys(): - type_fn = self.svc_qos_keys[key] try: + type_fn = self.svc_qos_keys[key]['type'] value = type_fn(value) - qos[self.svc_qos_param_dict[key]] = value + qos[key] = value except ValueError: continue return qos @@ -876,8 +883,39 @@ class StorwizeHelpers(object): return dest_pool def add_vdisk_qos(self, vdisk, qos): + """Add the QoS configuration to the volume.""" for key, value in qos.iteritems(): - self.ssh.chvdisk(vdisk, ['-' + key, str(value)]) + if key in self.svc_qos_keys.keys(): + param = self.svc_qos_keys[key]['param'] + self.ssh.chvdisk(vdisk, ['-' + param, str(value)]) + + def update_vdisk_qos(self, vdisk, qos): + """Update all the QoS in terms of a key and value. + + svc_qos_keys saves all the supported QoS parameters. Going through + this dict, we set the new values to all the parameters. If QoS is + available in the QoS configuration, the value is taken from it; + if not, the value will be set to default. + """ + for key, value in self.svc_qos_keys.iteritems(): + param = value['param'] + if key in qos.keys(): + # If the value is set in QoS, take the value from + # the QoS configuration. + v = qos[key] + else: + # If not, set the value to default. + v = value['default'] + self.ssh.chvdisk(vdisk, ['-' + param, str(v)]) + + def disable_vdisk_qos(self, vdisk, qos): + """Disable the QoS.""" + for key, value in qos.iteritems(): + if key in self.svc_qos_keys.keys(): + param = self.svc_qos_keys[key]['param'] + # Take the default value. + value = self.svc_qos_keys[key]['default'] + self.ssh.chvdisk(vdisk, ['-' + param, value]) def change_vdisk_options(self, vdisk, changes, opts, state): if 'warning' in opts: