Ignore non rules related qos_policy changes

QoSAgentExtension should invoke QoSAgentDriver
to reflush qos policy rules only if there is any rules related change.
QoS policy changes, such as description change, should not cause
reconfiguration of the qos policy rules.

Closes-Bug: #1509232
Change-Id: I036b0449c7b5521adeb32602a0c0e6cf2d27510a

diff --git a/neutron/agent/l2/extensions/qos.py b/neutron/agent/l2/extensions/qos.py
index f5a2594c9470ad4fb0cd20969135241d32c7e948..20ec764be3f48efe7e000ecf2397eabf95f83c39 100644 (file)
--- a/neutron/agent/l2/extensions/qos.py
+++ b/neutron/agent/l2/extensions/qos.py
@@ -237,14 +237,20 @@ class QosAgentExtension(agent_extension.AgentCoreResourceExtension):
     def delete_port(self, context, port):
         self._process_reset_port(port)
 
+    def _policy_rules_modified(self, old_policy, policy):
+        return not (len(old_policy.rules) == len(policy.rules) and
+                    all(i in old_policy.rules for i in policy.rules))
+
     def _process_update_policy(self, qos_policy):
         old_qos_policy = self.policy_map.get_policy(qos_policy.id)
-        for port in self.policy_map.get_ports(qos_policy):
-            #NOTE(QoS): for now, just reflush the rules on the port. Later, we
-            #           may want to apply the difference between the old and
-            #           new rule lists.
-            self.qos_driver.delete(port, old_qos_policy)
-            self.qos_driver.update(port, qos_policy)
+        if old_qos_policy:
+            if self._policy_rules_modified(old_qos_policy, qos_policy):
+                for port in self.policy_map.get_ports(qos_policy):
+                    #NOTE(QoS): for now, just reflush the rules on the port.
+                    #           Later, we may want to apply the difference
+                    #           between the old and new rule lists.
+                    self.qos_driver.delete(port, old_qos_policy)
+                    self.qos_driver.update(port, qos_policy)
             self.policy_map.update_policy(qos_policy)
 
     def _process_reset_port(self, port):

diff --git a/neutron/tests/unit/agent/l2/extensions/test_qos.py b/neutron/tests/unit/agent/l2/extensions/test_qos.py
index d0a6206f0bf2ebd532cfa21bcd41305934f25bbb..25cf371e5d208924c696b6fa4eaeb2f9470b52bf 100755 (executable)
--- a/neutron/tests/unit/agent/l2/extensions/test_qos.py
+++ b/neutron/tests/unit/agent/l2/extensions/test_qos.py
@@ -30,8 +30,15 @@ from neutron.services.qos import qos_consts
 from neutron.tests import base
 
 
-TEST_POLICY = policy.QosPolicy(context=None,
-                               name='test1', id='fake_policy_id')
+BASE_TEST_POLICY = {'context': None,
+                    'name': 'test1',
+                    'id': 'fake_policy_id'}
+
+TEST_POLICY = policy.QosPolicy(**BASE_TEST_POLICY)
+
+TEST_POLICY_DESCR = policy.QosPolicy(description='fake_descr',
+                                     **BASE_TEST_POLICY)
+
 TEST_POLICY2 = policy.QosPolicy(context=None,
                                 name='test2', id='fake_policy_id_2')
 
@@ -217,6 +224,7 @@ class QosExtensionRpcTestCase(QosExtensionBaseTestCase):
         port2 = self._create_test_port_dict(qos_policy_id=TEST_POLICY2.id)
         self.qos_ext.policy_map.set_port_policy(port1, TEST_POLICY)
         self.qos_ext.policy_map.set_port_policy(port2, TEST_POLICY2)
+        self.qos_ext._policy_rules_modified = mock.Mock(return_value=True)
 
         policy_obj = mock.Mock()
         policy_obj.id = port1['qos_policy_id']
@@ -228,6 +236,27 @@ class QosExtensionRpcTestCase(QosExtensionBaseTestCase):
         self.qos_ext._process_update_policy(policy_obj)
         self.qos_ext.qos_driver.update.assert_called_with(port2, policy_obj)
 
+    def test__process_update_policy_descr_not_propagated_into_driver(self):
+        port = self._create_test_port_dict(qos_policy_id=TEST_POLICY.id)
+        self.qos_ext.policy_map.set_port_policy(port, TEST_POLICY)
+        self.qos_ext._policy_rules_modified = mock.Mock(return_value=False)
+        self.qos_ext._process_update_policy(TEST_POLICY_DESCR)
+        self.qos_ext._policy_rules_modified.assert_called_with(TEST_POLICY,
+            TEST_POLICY_DESCR)
+        self.assertFalse(self.qos_ext.qos_driver.delete.called)
+        self.assertFalse(self.qos_ext.qos_driver.update.called)
+        self.assertEqual(TEST_POLICY_DESCR,
+                         self.qos_ext.policy_map.get_policy(TEST_POLICY.id))
+
+    def test__process_update_policy_not_known(self):
+        self.qos_ext._policy_rules_modified = mock.Mock()
+        self.qos_ext._process_update_policy(TEST_POLICY_DESCR)
+        self.assertFalse(self.qos_ext._policy_rules_modified.called)
+        self.assertFalse(self.qos_ext.qos_driver.delete.called)
+        self.assertFalse(self.qos_ext.qos_driver.update.called)
+        self.assertIsNone(self.qos_ext.policy_map.get_policy(
+            TEST_POLICY_DESCR.id))
+
     def test__process_reset_port(self):
         port1 = self._create_test_port_dict(qos_policy_id=TEST_POLICY.id)
         port2 = self._create_test_port_dict(qos_policy_id=TEST_POLICY2.id)
@@ -262,6 +291,61 @@ class QosExtensionInitializeTestCase(QosExtensionBaseTestCase):
         subscribe_mock.assert_called_with(mock.ANY, resources.QOS_POLICY)
 
 
+class QosExtensionReflushRulesTestCase(QosExtensionBaseTestCase):
+
+    def setUp(self):
+        super(QosExtensionReflushRulesTestCase, self).setUp()
+        self.qos_ext.initialize(
+            self.connection, constants.EXTENSION_DRIVER_TYPE)
+
+        self.pull_mock = mock.patch.object(
+            self.qos_ext.resource_rpc, 'pull',
+            return_value=TEST_POLICY).start()
+
+        self.policy = policy.QosPolicy(**BASE_TEST_POLICY)
+        self.rule = (
+            rule.QosBandwidthLimitRule(context=None, id='fake_rule_id',
+                                       qos_policy_id=self.policy.id,
+                                       max_kbps=100, max_burst_kbps=10))
+        self.policy.rules = [self.rule]
+        self.port = {'port_id': uuidutils.generate_uuid(),
+                     'qos_policy_id': TEST_POLICY.id}
+        self.new_policy = policy.QosPolicy(description='descr',
+                                           **BASE_TEST_POLICY)
+
+    def test_is_reflush_required_change_policy_descr(self):
+        self.qos_ext.policy_map.set_port_policy(self.port, self.policy)
+        self.new_policy.rules = [self.rule]
+        self.assertFalse(self.qos_ext._policy_rules_modified(self.policy,
+                                                             self.new_policy))
+
+    def test_is_reflush_required_change_policy_rule(self):
+        self.qos_ext.policy_map.set_port_policy(self.port, self.policy)
+        updated_rule = (rule.QosBandwidthLimitRule(context=None,
+                                                id='fake_rule_id',
+                                                qos_policy_id=self.policy.id,
+                                                max_kbps=200,
+                                                max_burst_kbps=20))
+        self.new_policy.rules = [updated_rule]
+        self.assertTrue(self.qos_ext._policy_rules_modified(self.policy,
+                                                            self.new_policy))
+
+    def test_is_reflush_required_remove_rules(self):
+        self.qos_ext.policy_map.set_port_policy(self.port, self.policy)
+        self.new_policy.rules = []
+        self.assertTrue(self.qos_ext._policy_rules_modified(self.policy,
+                                                            self.new_policy))
+
+    def test_is_reflush_required_add_rules(self):
+        self.qos_ext.policy_map.set_port_policy(self.port, self.policy)
+        self.new_policy.rules = [self.rule]
+        fake_rule = QosFakeRule(context=None, id='really_fake_rule_id',
+                                qos_policy_id=self.policy.id)
+        self.new_policy.rules.append(fake_rule)
+        self.assertTrue(self.qos_ext._policy_rules_modified(self.policy,
+                                                            self.new_policy))
+
+
 class PortPolicyMapTestCase(base.BaseTestCase):
 
     def setUp(self):