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)
This commit is contained in:
parent
a9195df1ce
commit
30b86eb169
neutron
common
objects/qos
services/qos
tests/unit
@ -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.")
|
||||
|
||||
|
@ -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)
|
||||
|
@ -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):
|
||||
|
@ -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)
|
||||
|
@ -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):
|
||||
|
@ -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()
|
||||
|
Loading…
x
Reference in New Issue
Block a user