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
(cherry picked from commit 26de1b1d829849665dae921b8be739194b84515d)

diff --git a/cinder/tests/test_storwize_svc.py b/cinder/tests/test_storwize_svc.py
index 0ecaa64e52a641381ef9640d891a8463f63b5144..cbe681540931e44a4744b560dbe7463f56859634 100644 (file)
--- 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)
+            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)
-            add_vdisk_qos.assert_called_with(volume['name'], fake_opts['qos'])
+            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)
+            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)
-            add_vdisk_qos.assert_called_with(volume['name'], fake_opts['qos'])
+            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 d6ad506aa8f35b8fafa6ec116e42f490ee0715cf..62ad086cff501c1637d73f36bcdb2c8ed91702de 100644 (file)
--- 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 c71f21fed2e5a7470f38c291da8d82ed49c0fd56..6f8540211042ce3791461510780db86c89afaa14 100644 (file)
--- 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():
+            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():
-            self.ssh.chvdisk(vdisk, ['-' + key, str(value)])
+            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: