From 6abc6399df4903881a1ee292be9f721e0252c529 Mon Sep 17 00:00:00 2001 From: Ilya Sokolov Date: Tue, 23 Dec 2014 13:22:20 +0000 Subject: [PATCH] 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 --- .../agentnotifiers/metering_rpc_agent_api.py | 6 + neutron/db/metering/metering_db.py | 22 +++- .../metering/agents/metering_agent.py | 8 ++ .../drivers/iptables/iptables_driver.py | 104 +++++++++++++++--- .../metering/drivers/noop/noop_driver.py | 8 ++ neutron/services/metering/metering_plugin.py | 9 +- .../metering/drivers/test_iptables_driver.py | 67 ++++++++++- .../services/metering/test_metering_agent.py | 17 +++ .../services/metering/test_metering_plugin.py | 52 +++++---- 9 files changed, 249 insertions(+), 44 deletions(-) diff --git a/neutron/api/rpc/agentnotifiers/metering_rpc_agent_api.py b/neutron/api/rpc/agentnotifiers/metering_rpc_agent_api.py index d3be6b1079c..10607d0acc7 100644 --- a/neutron/api/rpc/agentnotifiers/metering_rpc_agent_api.py +++ b/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) diff --git a/neutron/db/metering/metering_db.py b/neutron/db/metering/metering_db.py index 242c76d5b90..e98b49d9535 100644 --- a/neutron/db/metering/metering_db.py +++ b/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) diff --git a/neutron/services/metering/agents/metering_agent.py b/neutron/services/metering/agents/metering_agent.py index aadaa8b0153..bd9079f9754 100644 --- a/neutron/services/metering/agents/metering_agent.py +++ b/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, diff --git a/neutron/services/metering/drivers/iptables/iptables_driver.py b/neutron/services/metering/drivers/iptables/iptables_driver.py index 661fa6be463..39b10c2bf83 100644 --- a/neutron/services/metering/drivers/iptables/iptables_driver.py +++ b/neutron/services/metering/drivers/iptables/iptables_driver.py @@ -139,21 +139,52 @@ 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) - if rule['direction'] == 'egress': - dir_opt = '-o %s -s %s' % (ext_dev, remote_ip) - else: - dir_opt = '-i %s -d %s' % (ext_dev, remote_ip) + 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) - 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, - wrap=False, top=False) + 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: + dir_opt = '-i %s -d %s' % (ext_dev, remote_ip) + + if rule['excluded']: + ipt_rule = '%s -j RETURN' % dir_opt + else: + ipt_rule = '%s -j %s' % (dir_opt, label_chain) + return ipt_rule 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: diff --git a/neutron/services/metering/drivers/noop/noop_driver.py b/neutron/services/metering/drivers/noop/noop_driver.py index 977b922e29f..e9925a8df31 100644 --- a/neutron/services/metering/drivers/noop/noop_driver.py +++ b/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 diff --git a/neutron/services/metering/metering_plugin.py b/neutron/services/metering/metering_plugin.py index 046938da580..5af10361559 100644 --- a/neutron/services/metering/metering_plugin.py +++ b/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) - self.meter_rpc.update_metering_label_rules(context, data) + data = self.get_sync_data_for_rule(context, rule) + 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) - self.meter_rpc.update_metering_label_rules(context, data) - + data = self.get_sync_data_for_rule(context, rule) + self.meter_rpc.remove_metering_label_rule(context, data) return rule diff --git a/neutron/tests/unit/services/metering/drivers/test_iptables_driver.py b/neutron/tests/unit/services/metering/drivers/test_iptables_driver.py index 292a7983963..4f62500203b 100644 --- a/neutron/tests/unit/services/metering/drivers/test_iptables_driver.py +++ b/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] diff --git a/neutron/tests/unit/services/metering/test_metering_agent.py b/neutron/tests/unit/services/metering/test_metering_agent.py index 2386a6edee9..f41f5c81e22 100644 --- a/neutron/tests/unit/services/metering/test_metering_agent.py +++ b/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) diff --git a/neutron/tests/unit/services/metering/test_metering_plugin.py b/neutron/tests/unit/services/metering/test_metering_plugin.py index 72b8f76011e..90d01985275 100644 --- a/neutron/tests/unit/services/metering/test_metering_plugin.py +++ b/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': [ - {'remote_ip_prefix': '10.0.0.0/24', - 'direction': 'ingress', - 'metering_label_id': self.uuid, - 'excluded': False, - 'id': self.uuid}, - {'remote_ip_prefix': '10.0.0.0/24', - 'direction': 'egress', - 'metering_label_id': self.uuid, - 'excluded': False, - 'id': second_uuid}], + {'rule': { + 'remote_ip_prefix': '10.0.0.0/24', + 'direction': 'ingress', + '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': [ - {'remote_ip_prefix': '10.0.0.0/24', + {'rule': { + '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,16 +255,13 @@ class TestMeteringPlugin(test_db_plugin.NeutronDbPluginV2TestCase, with self.metering_label(tenant_id=self.tenant_id, set_context=True) as label: l = label['metering_label'] + self.mock_uuid.return_value = second_uuid with self.metering_label_rule(l['id']): - self.mock_uuid.return_value = second_uuid - with self.metering_label_rule(l['id'], - direction='egress') as rule: - self.mock_update.assert_called_with(self.ctx, - expected_add) - self._delete('metering-label-rules', - rule['metering_label_rule']['id']) - self.mock_update.assert_called_with(self.ctx, - expected_del) + self.mock_add_rule.assert_called_with(self.ctx, + expected_add) + self._delete('metering-label-rules', second_uuid) + self.mock_remove_rule.assert_called_with(self.ctx, + expected_del) def test_delete_metering_label_does_not_clear_router_tenant_id(self): tenant_id = '654f6b9d-0f36-4ae5-bd1b-01616794ca60'