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

@@ -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.")
 

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)

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):

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)

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):

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()