From 30b86eb169398ae6aea24439ffa5b233655099de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C5=82awek=20Kap=C5=82o=C5=84ski?= Date: Thu, 1 Feb 2018 12:59:02 +0100 Subject: [PATCH] Fix error message when duplicate QoS rule is created When user tries to create QoS rule which already exists in same QoS policy, only check if rule is duplicated was done on DB layer. Because of that, there was many retries of DB operations so user waits to response from Neutron server long time. Also error message returned from this DB related exception was not user friendly. This patch adds additional check of such duplicated rules before there is attempt to save new/updated rule in database so in case of error, response is send to user faster and it has proper message. Change-Id: I7d55df1eb931583c3dde064e073deb3e5479acc2 Closes-Bug: #1746526 (cherry picked from commit a91d84cfb44a9def517f2990d164b4972023709a) --- neutron/common/exceptions.py | 6 +++ neutron/objects/qos/qos_policy_validator.py | 21 ++++++++ neutron/objects/qos/rule.py | 23 +++++++++ neutron/services/qos/qos_plugin.py | 2 + neutron/tests/unit/objects/qos/test_rule.py | 50 +++++++++++++++++++ .../unit/services/qos/test_qos_plugin.py | 23 ++++++++- 6 files changed, 124 insertions(+), 1 deletion(-) diff --git a/neutron/common/exceptions.py b/neutron/common/exceptions.py index 9b8e5f36afe..8b518db791e 100644 --- a/neutron/common/exceptions.py +++ b/neutron/common/exceptions.py @@ -184,6 +184,12 @@ class QoSRuleParameterConflict(e.Conflict): "%(existing_value)s.") +class QoSRulesConflict(e.Conflict): + message = _("Rule %(new_rule_type)s conflicts with " + "rule %(rule_id)s which already exists in " + "QoS Policy %(policy_id)s.") + + class ExtensionsNotFound(e.NotFound): message = _("Extensions not found: %(extensions)s.") diff --git a/neutron/objects/qos/qos_policy_validator.py b/neutron/objects/qos/qos_policy_validator.py index 078a74ceb52..b375f994fe0 100644 --- a/neutron/objects/qos/qos_policy_validator.py +++ b/neutron/objects/qos/qos_policy_validator.py @@ -45,3 +45,24 @@ def check_bandwidth_rule_conflict(policy, rule_data): policy_id=policy["id"], existing_rule=rule.rule_type, existing_value=rule.max_kbps) + + +def check_rules_conflict(policy, rule_obj): + """Implementation of the QoS Policy rules conflicts. + + This function checks if the new rule to be associated with policy + doesn't have any duplicate rule already in policy. + Raises an exception if conflict is identified. + """ + + for rule in policy.rules: + # NOTE(slaweq): we don't want to raise exception when compared rules + # have got same id as it means that it is probably exactly the same + # rule so there is no conflict + if rule.id == getattr(rule_obj, "id", None): + continue + if rule.duplicates(rule_obj): + raise n_exc.QoSRulesConflict( + new_rule_type=rule_obj.rule_type, + rule_id=rule.id, + policy_id=policy.id) diff --git a/neutron/objects/qos/rule.py b/neutron/objects/qos/rule.py index f53f2ffcb8d..3bcbbe2dfd4 100644 --- a/neutron/objects/qos/rule.py +++ b/neutron/objects/qos/rule.py @@ -68,6 +68,25 @@ class QosRule(base.NeutronDbObject): # should be redefined in subclasses rule_type = None + duplicates_compare_fields = () + + def duplicates(self, other_rule): + """Returns True if rules have got same values in fields defined in + 'duplicates_compare_fields' list. + + In case when subclass don't have defined any field in + duplicates_compare_fields, only rule types are compared. + """ + + if self.rule_type != other_rule.rule_type: + return False + + if self.duplicates_compare_fields: + for field in self.duplicates_compare_fields: + if getattr(self, field) != getattr(other_rule, field): + return False + return True + def to_dict(self): dict_ = super(QosRule, self).to_dict() dict_['type'] = self.rule_type @@ -113,6 +132,8 @@ class QosBandwidthLimitRule(QosRule): default=n_const.EGRESS_DIRECTION) } + duplicates_compare_fields = ['direction'] + rule_type = qos_consts.RULE_TYPE_BANDWIDTH_LIMIT def obj_make_compatible(self, primitive, target_version): @@ -154,6 +175,8 @@ class QosMinimumBandwidthRule(QosRule): 'direction': common_types.FlowDirectionEnumField(), } + duplicates_compare_fields = ['direction'] + rule_type = qos_consts.RULE_TYPE_MINIMUM_BANDWIDTH def obj_make_compatible(self, primitive, target_version): diff --git a/neutron/services/qos/qos_plugin.py b/neutron/services/qos/qos_plugin.py index bc9902e6a5d..1c91dfb7077 100644 --- a/neutron/services/qos/qos_plugin.py +++ b/neutron/services/qos/qos_plugin.py @@ -313,6 +313,7 @@ class QoSPlugin(qos.QoSPluginBase): policy = self._get_policy_obj(context, policy_id) checker.check_bandwidth_rule_conflict(policy, rule_data) rule = rule_cls(context, qos_policy_id=policy_id, **rule_data) + checker.check_rules_conflict(policy, rule) rule.create() policy.obj_load_attr('rules') self.validate_policy(context, policy) @@ -352,6 +353,7 @@ class QoSPlugin(qos.QoSPluginBase): policy.get_rule_by_id(rule_id) rule = rule_cls(context, id=rule_id) rule.update_fields(rule_data, reset_changes=True) + checker.check_rules_conflict(policy, rule) rule.update() policy.obj_load_attr('rules') self.validate_policy(context, policy) diff --git a/neutron/tests/unit/objects/qos/test_rule.py b/neutron/tests/unit/objects/qos/test_rule.py index 931bcb41e6b..00b336d587a 100644 --- a/neutron/tests/unit/objects/qos/test_rule.py +++ b/neutron/tests/unit/objects/qos/test_rule.py @@ -138,6 +138,26 @@ class QosBandwidthLimitRuleObjectTestCase(test_base.BaseObjectIfaceTestCase): exception.IncompatibleObjectVersion, rule_obj.obj_to_primitive, '1.2') + def test_duplicate_rules(self): + policy_id = uuidutils.generate_uuid() + ingress_rule_1 = rule.QosBandwidthLimitRule( + self.context, qos_policy_id=policy_id, + max_kbps=1000, max_burst=500, + direction=constants.INGRESS_DIRECTION) + ingress_rule_2 = rule.QosBandwidthLimitRule( + self.context, qos_policy_id=policy_id, + max_kbps=2000, max_burst=500, + direction=constants.INGRESS_DIRECTION) + egress_rule = rule.QosBandwidthLimitRule( + self.context, qos_policy_id=policy_id, + max_kbps=1000, max_burst=500, + direction=constants.EGRESS_DIRECTION) + dscp_rule = rule.QosDscpMarkingRule( + self.context, qos_policy_id=policy_id, dscp_mark=16) + self.assertTrue(ingress_rule_1.duplicates(ingress_rule_2)) + self.assertFalse(ingress_rule_1.duplicates(egress_rule)) + self.assertFalse(ingress_rule_1.duplicates(dscp_rule)) + class QosBandwidthLimitRuleDbObjectTestCase(test_base.BaseDbObjectTestCase, testlib_api.SqlTestCase): @@ -166,6 +186,19 @@ class QosDscpMarkingRuleObjectTestCase(test_base.BaseObjectIfaceTestCase): self.assertRaises(exception.IncompatibleObjectVersion, dscp_rule.obj_to_primitive, '1.0') + def test_duplicate_rules(self): + policy_id = uuidutils.generate_uuid() + dscp_rule_1 = rule.QosDscpMarkingRule( + self.context, qos_policy_id=policy_id, dscp_mark=16) + dscp_rule_2 = rule.QosDscpMarkingRule( + self.context, qos_policy_id=policy_id, dscp_mark=32) + bw_limit_rule = rule.QosBandwidthLimitRule( + self.context, qos_policy_id=policy_id, + max_kbps=1000, max_burst=500, + direction=constants.EGRESS_DIRECTION) + self.assertTrue(dscp_rule_1.duplicates(dscp_rule_2)) + self.assertFalse(dscp_rule_1.duplicates(bw_limit_rule)) + class QosDscpMarkingRuleDbObjectTestCase(test_base.BaseDbObjectTestCase, testlib_api.SqlTestCase): @@ -194,6 +227,23 @@ class QosMinimumBandwidthRuleObjectTestCase(test_base.BaseObjectIfaceTestCase): self.assertRaises(exception.IncompatibleObjectVersion, min_bw_rule.obj_to_primitive, version) + def test_duplicate_rules(self): + policy_id = uuidutils.generate_uuid() + ingress_rule_1 = rule.QosMinimumBandwidthRule( + self.context, qos_policy_id=policy_id, + min_kbps=1000, direction=constants.INGRESS_DIRECTION) + ingress_rule_2 = rule.QosMinimumBandwidthRule( + self.context, qos_policy_id=policy_id, + min_kbps=2000, direction=constants.INGRESS_DIRECTION) + egress_rule = rule.QosMinimumBandwidthRule( + self.context, qos_policy_id=policy_id, + min_kbps=1000, direction=constants.EGRESS_DIRECTION) + dscp_rule = rule.QosDscpMarkingRule( + self.context, qos_policy_id=policy_id, dscp_mark=16) + self.assertTrue(ingress_rule_1.duplicates(ingress_rule_2)) + self.assertFalse(ingress_rule_1.duplicates(egress_rule)) + self.assertFalse(ingress_rule_1.duplicates(dscp_rule)) + class QosMinimumBandwidthRuleDbObjectTestCase(test_base.BaseDbObjectTestCase, testlib_api.SqlTestCase): diff --git a/neutron/tests/unit/services/qos/test_qos_plugin.py b/neutron/tests/unit/services/qos/test_qos_plugin.py index 60c4cf6c09f..d40a0061848 100644 --- a/neutron/tests/unit/services/qos/test_qos_plugin.py +++ b/neutron/tests/unit/services/qos/test_qos_plugin.py @@ -10,6 +10,8 @@ # License for the specific language governing permissions and limitations # under the License. +import copy + import mock from neutron_lib import context from neutron_lib import exceptions as lib_exc @@ -412,8 +414,10 @@ class TestQosPlugin(base.BaseQosTestCase): mock_manager.mock_calls.index(delete_mock_call)) def test_create_policy_rule(self): + _policy = copy.copy(self.policy) + setattr(_policy, "rules", []) with mock.patch('neutron.objects.qos.policy.QosPolicy.get_object', - return_value=self.policy), mock.patch( + return_value=_policy), mock.patch( 'neutron.objects.qos.qos_policy_validator' '.check_bandwidth_rule_conflict', return_value=None): self.qos_plugin.create_policy_bandwidth_limit_rule( @@ -464,6 +468,23 @@ class TestQosPlugin(base.BaseQosTestCase): self.ctxt, self.policy.id, self.rule_data) mock_qos_get_obj.assert_called_once_with(self.ctxt, id=_policy.id) + def test_create_policy_rule_duplicates(self): + _policy = self._get_policy() + setattr(_policy, "rules", [self.rule]) + new_rule_data = { + 'bandwidth_limit_rule': { + 'max_kbps': 5000, + 'direction': self.rule.direction + } + } + with mock.patch('neutron.objects.qos.policy.QosPolicy.get_object', + return_value=_policy) as mock_qos_get_obj: + self.assertRaises( + n_exc.QoSRulesConflict, + self.qos_plugin.create_policy_bandwidth_limit_rule, + self.ctxt, _policy.id, new_rule_data) + mock_qos_get_obj.assert_called_once_with(self.ctxt, id=_policy.id) + @mock.patch.object(rule_object.QosBandwidthLimitRule, 'update') def test_update_policy_rule(self, mock_qos_rule_update): mock_manager = mock.Mock()