diff --git a/cinder/api/contrib/qos_specs_manage.py b/cinder/api/contrib/qos_specs_manage.py index 8f97ed794c8..80025574726 100644 --- a/cinder/api/contrib/qos_specs_manage.py +++ b/cinder/api/contrib/qos_specs_manage.py @@ -98,9 +98,6 @@ class QoSSpecsController(wsgi.Controller): max_length=255, remove_whitespaces=True) name = name.strip() - # Validate the key-value pairs in the qos spec. - utils.validate_dictionary_string_length(specs) - try: spec = qos_specs.create(context, name, specs) notifier_info = dict(name=name, specs=specs) diff --git a/cinder/tests/unit/api/contrib/test_qos_specs_manage.py b/cinder/tests/unit/api/contrib/test_qos_specs_manage.py index 6e00e063d17..e2bfc819d5c 100644 --- a/cinder/tests/unit/api/contrib/test_qos_specs_manage.py +++ b/cinder/tests/unit/api/contrib/test_qos_specs_manage.py @@ -381,8 +381,7 @@ class QoSSpecManageApiTest(test.TestCase): @mock.patch('cinder.volume.qos_specs.create', side_effect=return_qos_specs_create) - @mock.patch('cinder.utils.validate_dictionary_string_length') - def test_create(self, mock_validate, mock_qos_spec_create): + def test_create(self, mock_qos_spec_create): body = {"qos_specs": {"name": "qos_specs_%s" % fake.QOS_SPEC_ID, "key1": "value1"}} @@ -395,7 +394,6 @@ class QoSSpecManageApiTest(test.TestCase): self.assertEqual(1, self.notifier.get_notification_count()) self.assertEqual('qos_specs_%s' % fake.QOS_SPEC_ID, res_dict['qos_specs']['name']) - self.assertTrue(mock_validate.called) @mock.patch('cinder.volume.qos_specs.create', side_effect=return_qos_specs_create) @@ -506,6 +504,18 @@ class QoSSpecManageApiTest(test.TestCase): req, fake.INVALID_ID, body) self.assertEqual(1, self.notifier.get_notification_count()) + @ddt.data({'qos_specs': {'key1': ['value1']}}, + {'qos_specs': {1: 'value1'}} + ) + def test_update_non_string_key_or_value(self, body): + req = fakes.HTTPRequest.blank('/v2/%s/qos-specs/%s' % + (fake.PROJECT_ID, fake.UUID1), + use_admin_context=True) + self.assertRaises(exception.InvalidQoSSpecs, + self.controller.update, + req, fake.UUID1, body) + self.assertEqual(1, self.notifier.get_notification_count()) + @mock.patch('cinder.volume.qos_specs.update', side_effect=return_qos_specs_update) def test_update_failed(self, mock_qos_update): diff --git a/cinder/tests/unit/test_qos_specs.py b/cinder/tests/unit/test_qos_specs.py index 100fccda492..596a8d656b5 100644 --- a/cinder/tests/unit/test_qos_specs.py +++ b/cinder/tests/unit/test_qos_specs.py @@ -115,7 +115,7 @@ class QoSSpecsTestCase(test.TestCase): # qos specs must exists self.assertRaises(exception.QoSSpecsNotFound, - qos_specs.update, self.ctxt, 'fake_id', qos) + qos_specs.update, self.ctxt, 'fake_id', qos['specs']) specs_id = self._create_qos_specs('Name', qos['consumer'], diff --git a/cinder/tests/unit/volume/drivers/ibm/test_storwize_svc.py b/cinder/tests/unit/volume/drivers/ibm/test_storwize_svc.py index 6fcd5305506..25acff1a499 100644 --- a/cinder/tests/unit/volume/drivers/ibm/test_storwize_svc.py +++ b/cinder/tests/unit/volume/drivers/ibm/test_storwize_svc.py @@ -4536,7 +4536,7 @@ class StorwizeSVCCommonDriverTestCase(test.TestCase): def test_storwize_svc_get_vdisk_params(self): self.driver.do_setup(None) - fake_qos = {'qos:IOThrottling': 5000} + fake_qos = {'qos:IOThrottling': '5000'} expected_qos = {'IOThrottling': 5000} fake_opts = self._get_default_opts() # The parameters retured should be the same to the default options, @@ -4622,7 +4622,7 @@ class StorwizeSVCCommonDriverTestCase(test.TestCase): # If the QoS is set both via the qos association and the # extra specs, the one from the qos association will take effect. - fake_qos_associate = {'qos:IOThrottling': 6000} + fake_qos_associate = {'qos:IOThrottling': '6000'} expected_qos_associate = {'IOThrottling': 6000} vol_type_qos = self._create_volume_type_qos_both(fake_qos, fake_qos_associate) diff --git a/cinder/volume/qos_specs.py b/cinder/volume/qos_specs.py index 33be69c29fc..1f5204d1a3d 100644 --- a/cinder/volume/qos_specs.py +++ b/cinder/volume/qos_specs.py @@ -24,6 +24,7 @@ from cinder import db from cinder import exception from cinder.i18n import _ from cinder import objects +from cinder import utils from cinder.volume import volume_types @@ -40,6 +41,9 @@ def create(context, name, specs=None): 'total_iops_sec': 1000, 'total_bytes_sec': 1024000} """ + # Validate the key-value pairs in the qos spec. + utils.validate_dictionary_string_length(specs) + consumer = specs.get('consumer') if consumer: # If we need to modify specs, copy so we don't cause unintended @@ -67,6 +71,7 @@ def update(context, qos_specs_id, specs): LOG.debug('qos_specs.update(): specs %s', specs) try: + utils.validate_dictionary_string_length(specs) qos_spec = objects.QualityOfServiceSpecs.get_by_id(context, qos_specs_id) @@ -81,6 +86,8 @@ def update(context, qos_specs_id, specs): qos_spec.specs.update(specs) qos_spec.save() + except exception.InvalidInput as e: + raise exception.InvalidQoSSpecs(reason=e) except db_exc.DBError: LOG.exception('DB error:') raise exception.QoSSpecsUpdateFailed(specs_id=qos_specs_id,