From a91d84cfb44a9def517f2990d164b4972023709a 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 --- 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 ffa396956a2..0b5b495bd15 100644 --- a/neutron/common/exceptions.py +++ b/neutron/common/exceptions.py @@ -194,6 +194,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 cbcc9fd8813..08541f486fb 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 21b88c6451a..a4bc2af3692 100644 --- a/neutron/objects/qos/rule.py +++ b/neutron/objects/qos/rule.py @@ -66,6 +66,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 @@ -111,6 +130,8 @@ class QosBandwidthLimitRule(QosRule): default=constants.EGRESS_DIRECTION) } + duplicates_compare_fields = ['direction'] + rule_type = qos_consts.RULE_TYPE_BANDWIDTH_LIMIT def obj_make_compatible(self, primitive, target_version): @@ -152,6 +173,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 945cc1df68c..fe699631196 100644 --- a/neutron/services/qos/qos_plugin.py +++ b/neutron/services/qos/qos_plugin.py @@ -314,6 +314,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) @@ -353,6 +354,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 e816baa250d..7e401114f99 100644 --- a/neutron/tests/unit/objects/qos/test_rule.py +++ b/neutron/tests/unit/objects/qos/test_rule.py @@ -137,6 +137,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): @@ -165,6 +185,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): @@ -193,6 +226,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 0962021bd22..d1c7faaa02b 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,7 +414,9 @@ class TestQosPlugin(base.BaseQosTestCase): @mock.patch.object(rule_object.QosBandwidthLimitRule, 'create') def test_create_policy_rule(self, mock_qos_rule_create, mock_qos_policy_get): - mock_qos_policy_get.return_value = self.policy + _policy = copy.copy(self.policy) + setattr(_policy, "rules", []) + mock_qos_policy_get.return_value = _policy mock_manager = mock.Mock() mock_manager.attach_mock(mock_qos_rule_create, 'create') mock_manager.attach_mock(self.qos_plugin.driver_manager, 'driver') @@ -477,6 +481,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()