Send only one rule in queue on rule create/delete

Now we send all labels and rules per rule create/delete
and rebuild whole iptables chains.
In this patch we send only affected rule and create/
delete only this rule from iptables.

Change-Id: I58ebd8d810c62980c09a340ee1680be17c12b74a
Closes-Bug: #1400280

neutron/api/rpc/agentnotifiers/metering_rpc_agent_api.py

@@ -90,6 +90,12 @@ class MeteringAgentNotifyAPI(object):
     def update_metering_label_rules(self, context, routers):
         self._notification(context, 'update_metering_label_rules', routers)
 
+    def add_metering_label_rule(self, context, routers):
+        self._notification(context, 'add_metering_label_rule', routers)
+
+    def remove_metering_label_rule(self, context, routers):
+        self._notification(context, 'remove_metering_label_rule', routers)
+
     def add_metering_label(self, context, routers):
         self._notification(context, 'add_metering_label', routers)
 

neutron/db/metering/metering_db.py

@@ -186,9 +186,10 @@ class MeteringDbMixin(metering.MeteringPluginBase,
                 rule = self._get_by_id(context, MeteringLabelRule, rule_id)
             except orm.exc.NoResultFound:
                 raise metering.MeteringLabelRuleNotFound(rule_id=rule_id)
-
             context.session.delete(rule)
 
+        return self._make_metering_label_rule_dict(rule)
+
     def _get_metering_rules_dict(self, metering_label):
         rules = []
         for rule in metering_label.rules:
@@ -235,6 +236,25 @@ class MeteringDbMixin(metering.MeteringPluginBase,
 
         return routers_dict.values()
 
+    def get_sync_data_for_rule(self, context, rule):
+        label = context.session.query(MeteringLabel).get(
+            rule['metering_label_id'])
+
+        if label.shared:
+            routers = self._get_collection_query(context, l3_db.Router)
+        else:
+            routers = label.routers
+
+        routers_dict = {}
+        for router in routers:
+            router_dict = routers_dict.get(router['id'],
+                                           self._make_router_dict(router))
+            data = {'id': label['id'], 'rule': rule}
+            router_dict[constants.METERING_LABEL_KEY].append(data)
+            routers_dict[router['id']] = router_dict
+
+        return routers_dict.values()
+
     def get_sync_data_metering(self, context, label_id=None, router_ids=None):
         labels = context.session.query(MeteringLabel)
 

neutron/services/metering/agents/metering_agent.py

@@ -216,6 +216,14 @@ class MeteringAgent(MeteringPluginRpc, manager.Manager):
         LOG.debug("Get router traffic counters")
         return self._invoke_driver(context, routers, 'get_traffic_counters')
 
+    def add_metering_label_rule(self, context, routers):
+        return self._invoke_driver(context, routers,
+                                   'add_metering_label_rule')
+
+    def remove_metering_label_rule(self, context, routers):
+        return self._invoke_driver(context, routers,
+                                   'remove_metering_label_rule')
+
     def update_metering_label_rules(self, context, routers):
         LOG.debug("Update metering rules from agent")
         return self._invoke_driver(context, routers,

neutron/services/metering/drivers/iptables/iptables_driver.py

@@ -139,8 +139,42 @@ class IptablesMeteringDriver(abstract_driver.MeteringAbstractDriver):
             return
 
         for rule in rules:
-            remote_ip = rule['remote_ip_prefix']
+            self._add_rule_to_chain(ext_dev, rule, im,
+                                    label_chain, rules_chain)
 
+    def _process_metering_label_rule_add(self, rm, rule, ext_dev,
+                                         label_chain, rules_chain):
+        im = rm.iptables_manager
+        self._add_rule_to_chain(ext_dev, rule, im, label_chain, rules_chain)
+
+    def _process_metering_label_rule_delete(self, rm, rule, ext_dev,
+                                            label_chain, rules_chain):
+        im = rm.iptables_manager
+        self._remove_rule_from_chain(ext_dev, rule, im,
+                                     label_chain, rules_chain)
+
+    def _add_rule_to_chain(self, ext_dev, rule, im,
+                           label_chain, rules_chain):
+        ipt_rule = self._prepare_rule(ext_dev, rule, label_chain)
+        if rule['excluded']:
+            im.ipv4['filter'].add_rule(rules_chain, ipt_rule,
+                                       wrap=False, top=True)
+        else:
+            im.ipv4['filter'].add_rule(rules_chain, ipt_rule,
+                                       wrap=False, top=False)
+
+    def _remove_rule_from_chain(self, ext_dev, rule, im,
+                                label_chain, rules_chain):
+        ipt_rule = self._prepare_rule(ext_dev, rule, label_chain)
+        if rule['excluded']:
+            im.ipv4['filter'].remove_rule(rules_chain, ipt_rule,
+                                          wrap=False, top=True)
+        else:
+            im.ipv4['filter'].remove_rule(rules_chain, ipt_rule,
+                                          wrap=False, top=False)
+
+    def _prepare_rule(self, ext_dev, rule, label_chain):
+        remote_ip = rule['remote_ip_prefix']
         if rule['direction'] == 'egress':
             dir_opt = '-o %s -s %s' % (ext_dev, remote_ip)
         else:
@@ -148,12 +182,9 @@ class IptablesMeteringDriver(abstract_driver.MeteringAbstractDriver):
 
         if rule['excluded']:
             ipt_rule = '%s -j RETURN' % dir_opt
-                im.ipv4['filter'].add_rule(rules_chain, ipt_rule,
-                                           wrap=False, top=True)
         else:
             ipt_rule = '%s -j %s' % (dir_opt, label_chain)
-                im.ipv4['filter'].add_rule(rules_chain, ipt_rule,
+        return ipt_rule
-                                           wrap=False, top=False)
 
     def _process_associate_metering_label(self, router):
         self._update_router(router)
@@ -222,11 +253,58 @@ class IptablesMeteringDriver(abstract_driver.MeteringAbstractDriver):
         for router in routers:
             self._process_associate_metering_label(router)
 
+    @log.log
+    def add_metering_label_rule(self, context, routers):
+        for router in routers:
+            self._add_metering_label_rule(router)
+
+    @log.log
+    def remove_metering_label_rule(self, context, routers):
+        for router in routers:
+            self._remove_metering_label_rule(router)
+
     @log.log
     def update_metering_label_rules(self, context, routers):
         for router in routers:
             self._update_metering_label_rules(router)
 
+    def _add_metering_label_rule(self, router):
+        self._process_metering_rule_action(router, 'create')
+
+    def _remove_metering_label_rule(self, router):
+        self._process_metering_rule_action(router, 'delete')
+
+    def _process_metering_rule_action(self, router, action):
+        rm = self.routers.get(router['id'])
+        if not rm:
+            return
+        ext_dev = self.get_external_device_name(rm.router['gw_port_id'])
+        if not ext_dev:
+            return
+        with IptablesManagerTransaction(rm.iptables_manager):
+            labels = router.get(constants.METERING_LABEL_KEY, [])
+            for label in labels:
+                label_id = label['id']
+                label_chain = iptables_manager.get_chain_name(WRAP_NAME +
+                                                              LABEL + label_id,
+                                                              wrap=False)
+
+                rules_chain = iptables_manager.get_chain_name(WRAP_NAME +
+                                                              RULE + label_id,
+                                                              wrap=False)
+                rule = label.get('rule')
+                if rule:
+                    if action == 'create':
+                        self._process_metering_label_rule_add(rm, rule,
+                                                              ext_dev,
+                                                              label_chain,
+                                                              rules_chain)
+                    elif action == 'delete':
+                        self._process_metering_label_rule_delete(rm, rule,
+                                                                 ext_dev,
+                                                                 label_chain,
+                                                                 rules_chain)
+
     def _update_metering_label_rules(self, router):
         rm = self.routers.get(router['id'])
         if not rm:

neutron/services/metering/drivers/noop/noop_driver.py

@@ -30,6 +30,14 @@ class NoopMeteringDriver(abstract_driver.MeteringAbstractDriver):
     def update_metering_label_rules(self, context, routers):
         pass
 
+    @log.log
+    def add_metering_label_rule(self, context, routers):
+        pass
+
+    @log.log
+    def remove_metering_label_rule(self, context, routers):
+        pass
+
     @log.log
     def add_metering_label(self, context, routers):
         pass

neutron/services/metering/metering_plugin.py

@@ -57,8 +57,8 @@ class MeteringPlugin(metering_db.MeteringDbMixin):
         rule = super(MeteringPlugin, self).create_metering_label_rule(
             context, metering_label_rule)
 
-        data = self.get_sync_data_metering(context)
+        data = self.get_sync_data_for_rule(context, rule)
-        self.meter_rpc.update_metering_label_rules(context, data)
+        self.meter_rpc.add_metering_label_rule(context, data)
 
         return rule
 
@@ -66,7 +66,6 @@ class MeteringPlugin(metering_db.MeteringDbMixin):
         rule = super(MeteringPlugin, self).delete_metering_label_rule(
             context, rule_id)
 
-        data = self.get_sync_data_metering(context)
+        data = self.get_sync_data_for_rule(context, rule)
-        self.meter_rpc.update_metering_label_rules(context, data)
+        self.meter_rpc.remove_metering_label_rule(context, data)
-
         return rule

neutron/tests/unit/services/metering/drivers/test_iptables_driver.py

@@ -55,6 +55,37 @@ TEST_ROUTERS = [
      'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'},
 ]
 
+TEST_ROUTERS_WITH_ONE_RULE = [
+    {'_metering_labels': [
+        {'id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
+         'rule': {
+             'direction': 'ingress',
+             'excluded': False,
+             'id': '7f1a261f-2489-4ed1-870c-a62754501379',
+             'metering_label_id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
+             'remote_ip_prefix': '30.0.0.0/24'}}],
+     'admin_state_up': True,
+     'gw_port_id': '6d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
+     'id': '473ec392-1711-44e3-b008-3251ccfc5099',
+     'name': 'router1',
+     'status': 'ACTIVE',
+     'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'},
+    {'_metering_labels': [
+        {'id': 'eeef45da-c600-4a2a-b2f4-c0fb6df73c83',
+         'rule': {
+             'direction': 'egress',
+             'excluded': False,
+             'id': 'fa2441e8-2489-4ed1-870c-a62754501379',
+             'metering_label_id': 'eeef45da-c600-4a2a-b2f4-c0fb6df73c83',
+             'remote_ip_prefix': '40.0.0.0/24'}}],
+     'admin_state_up': True,
+     'gw_port_id': '7d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
+     'id': '373ec392-1711-44e3-b008-3251ccfc5099',
+     'name': 'router2',
+     'status': 'ACTIVE',
+     'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'},
+]
+
 
 class IptablesDriverTestCase(base.BaseTestCase):
     def setUp(self):
@@ -215,7 +246,7 @@ class IptablesDriverTestCase(base.BaseTestCase):
 
         self.v4filter_inst.assert_has_calls(calls)
 
-    def test_remove_metering_label_rule(self):
+    def test_remove_metering_label_rule_in_update(self):
         routers = copy.deepcopy(TEST_ROUTERS[:1])
         routers[0]['_metering_labels'][0]['rules'].append({
             'direction': 'ingress',
@@ -257,6 +288,40 @@ class IptablesDriverTestCase(base.BaseTestCase):
 
         self.v4filter_inst.assert_has_calls(calls)
 
+    def test_add_metering_label_rule(self):
+        new_routers_rules = TEST_ROUTERS_WITH_ONE_RULE
+        self.metering.update_routers(None, TEST_ROUTERS)
+        self.metering.add_metering_label_rule(None, new_routers_rules)
+        calls = [
+                 mock.call.add_rule('neutron-meter-r-c5df2fe5-c60',
+                                    '-i qg-6d411f48-ec -d 30.0.0.0/24'
+                                    ' -j neutron-meter-l-c5df2fe5-c60',
+                                    wrap=False, top=False),
+                 mock.call.add_rule('neutron-meter-r-eeef45da-c60',
+                                    '-o qg-7d411f48-ec -s 40.0.0.0/24'
+                                    ' -j neutron-meter-l-eeef45da-c60',
+                                    wrap=False, top=False),
+
+                ]
+        self.v4filter_inst.assert_has_calls(calls)
+
+    def test_remove_metering_label_rule(self):
+        new_routers_rules = TEST_ROUTERS_WITH_ONE_RULE
+        self.metering.update_routers(None, TEST_ROUTERS)
+        self.metering.add_metering_label_rule(None, new_routers_rules)
+        self.metering.remove_metering_label_rule(None, new_routers_rules)
+        calls = [
+            mock.call.remove_rule('neutron-meter-r-c5df2fe5-c60',
+                                  '-i qg-6d411f48-ec -d 30.0.0.0/24'
+                                  ' -j neutron-meter-l-c5df2fe5-c60',
+                                  wrap=False, top=False),
+            mock.call.remove_rule('neutron-meter-r-eeef45da-c60',
+                                  '-o qg-7d411f48-ec -s 40.0.0.0/24'
+                                  ' -j neutron-meter-l-eeef45da-c60',
+                                  wrap=False, top=False)
+                ]
+        self.v4filter_inst.assert_has_calls(calls)
+
     def test_remove_metering_label(self):
         routers = TEST_ROUTERS[:1]
 

neutron/tests/unit/services/metering/test_metering_agent.py

@@ -35,6 +35,15 @@ ROUTERS = [{'status': 'ACTIVE',
                                   'id': LABEL_ID}],
             'id': _uuid()}]
 
+ROUTERS_WITH_RULE = [{'status': 'ACTIVE',
+                      'name': 'router1',
+                      'gw_port_id': None,
+                      'admin_state_up': True,
+                      'tenant_id': TENANT_ID,
+                      '_metering_labels': [{'rule': {},
+                                            'id': LABEL_ID}],
+                      'id': _uuid()}]
+
 
 class TestMeteringOperations(base.BaseTestCase,
                              testlib_plugin.NotificationSetupHelper):
@@ -78,6 +87,14 @@ class TestMeteringOperations(base.BaseTestCase,
         self.agent.update_metering_label_rules(None, ROUTERS)
         self.assertEqual(self.driver.update_metering_label_rules.call_count, 1)
 
+    def test_add_metering_label_rule(self):
+        self.agent.add_metering_label_rule(None, ROUTERS_WITH_RULE)
+        self.assertEqual(self.driver.add_metering_label_rule.call_count, 1)
+
+    def test_remove_metering_label_rule(self):
+        self.agent.remove_metering_label_rule(None, ROUTERS_WITH_RULE)
+        self.assertEqual(self.driver.remove_metering_label_rule.call_count, 1)
+
     def test_routers_updated(self):
         self.agent.routers_updated(None, ROUTERS)
         self.assertEqual(self.driver.update_routers.call_count, 1)

neutron/tests/unit/services/metering/test_metering_plugin.py

@@ -107,6 +107,18 @@ class TestMeteringPlugin(test_db_plugin.NeutronDbPluginV2TestCase,
         self.update_patch = mock.patch(update)
         self.mock_update = self.update_patch.start()
 
+        add_rule = ('neutron.api.rpc.agentnotifiers.' +
+                    'metering_rpc_agent_api.MeteringAgentNotifyAPI' +
+                    '.add_metering_label_rule')
+        self.add_rule_patch = mock.patch(add_rule)
+        self.mock_add_rule = self.add_rule_patch.start()
+
+        remove_rule = ('neutron.api.rpc.agentnotifiers.' +
+                       'metering_rpc_agent_api.MeteringAgentNotifyAPI' +
+                       '.remove_metering_label_rule')
+        self.remove_rule_patch = mock.patch(remove_rule)
+        self.mock_remove_rule = self.remove_rule_patch.start()
+
     def test_add_metering_label_rpc_call(self):
         second_uuid = 'e27fe2df-376e-4ac7-ae13-92f050a21f84'
         expected = [{'status': 'ACTIVE',
@@ -207,7 +219,7 @@ class TestMeteringPlugin(test_db_plugin.NeutronDbPluginV2TestCase,
                                  label['metering_label']['id'])
                 self.mock_remove.assert_called_with(self.ctx, expected_remove)
 
-    def test_update_metering_label_rules_rpc_call(self):
+    def test_add_and_remove_metering_label_rule_rpc_call(self):
         second_uuid = 'e27fe2df-376e-4ac7-ae13-92f050a21f84'
         expected_add = [{'status': 'ACTIVE',
                          'name': 'router1',
@@ -215,17 +227,12 @@ class TestMeteringPlugin(test_db_plugin.NeutronDbPluginV2TestCase,
                          'admin_state_up': True,
                          'tenant_id': self.tenant_id,
                          '_metering_labels': [
-                             {'rules': [
+                             {'rule': {
-                                 {'remote_ip_prefix': '10.0.0.0/24',
+                                 'remote_ip_prefix': '10.0.0.0/24',
                                  'direction': 'ingress',
                                  'metering_label_id': self.uuid,
                                  'excluded': False,
-                                  'id': self.uuid},
+                                 'id': second_uuid},
-                                 {'remote_ip_prefix': '10.0.0.0/24',
-                                  'direction': 'egress',
-                                  'metering_label_id': self.uuid,
-                                  'excluded': False,
-                                  'id': second_uuid}],
                              'id': self.uuid}],
                          'id': self.uuid}]
 
@@ -235,12 +242,12 @@ class TestMeteringPlugin(test_db_plugin.NeutronDbPluginV2TestCase,
                          'admin_state_up': True,
                          'tenant_id': self.tenant_id,
                          '_metering_labels': [
-                             {'rules': [
+                             {'rule': {
-                                 {'remote_ip_prefix': '10.0.0.0/24',
+                                  'remote_ip_prefix': '10.0.0.0/24',
                                   'direction': 'ingress',
                                   'metering_label_id': self.uuid,
                                   'excluded': False,
-                                  'id': self.uuid}],
+                                   'id': second_uuid},
                              'id': self.uuid}],
                          'id': self.uuid}]
 
@@ -248,15 +255,12 @@ class TestMeteringPlugin(test_db_plugin.NeutronDbPluginV2TestCase,
             with self.metering_label(tenant_id=self.tenant_id,
                                      set_context=True) as label:
                 l = label['metering_label']
-                with self.metering_label_rule(l['id']):
                 self.mock_uuid.return_value = second_uuid
-                    with self.metering_label_rule(l['id'],
+                with self.metering_label_rule(l['id']):
-                                                  direction='egress') as rule:
+                    self.mock_add_rule.assert_called_with(self.ctx,
-                        self.mock_update.assert_called_with(self.ctx,
                                                           expected_add)
-                        self._delete('metering-label-rules',
+                    self._delete('metering-label-rules', second_uuid)
-                                     rule['metering_label_rule']['id'])
+                self.mock_remove_rule.assert_called_with(self.ctx,
-                    self.mock_update.assert_called_with(self.ctx,
                                                          expected_del)
 
     def test_delete_metering_label_does_not_clear_router_tenant_id(self):