From ff07bacc9080ab5d904f4b82f5d16da4fd61df6e Mon Sep 17 00:00:00 2001 From: bno1 Date: Sun, 23 Jun 2019 00:51:02 +0300 Subject: [PATCH] Retry creating iptables managers and adding metering rules This change makes the metering agent retry creating the iptables managers for each router and applying the metering rules. This is needed in case the metering agent starts before some or all of the namespaces are created. Change-Id: Ifc565feb98c7f02df5c2831a3607c3e526a2e703 Closes-Bug: #1807153 (cherry picked from commit 9f541521bbbcf36325bfc250e3ce27a138ddef3c) --- .../metering/agents/metering_agent.py | 5 ++ .../metering/drivers/abstract_driver.py | 4 + .../drivers/iptables/iptables_driver.py | 81 ++++++++++++++----- .../metering/drivers/noop/noop_driver.py | 4 + .../metering/agents/test_metering_agent.py | 4 + .../metering/drivers/test_iptables.py | 34 ++++++++ 6 files changed, 114 insertions(+), 18 deletions(-) diff --git a/neutron/services/metering/agents/metering_agent.py b/neutron/services/metering/agents/metering_agent.py index 9e6751120eb..8795410956d 100644 --- a/neutron/services/metering/agents/metering_agent.py +++ b/neutron/services/metering/agents/metering_agent.py @@ -149,6 +149,7 @@ class MeteringAgent(MeteringPluginRpc, manager.Manager): self._add_metering_info(label_id, acc['pkts'], acc['bytes']) def _metering_loop(self): + self._sync_router_namespaces(self.context, self.routers.values()) self._add_metering_infos() ts = timeutils.utcnow_ts() @@ -215,6 +216,10 @@ class MeteringAgent(MeteringPluginRpc, manager.Manager): LOG.debug("Get router traffic counters") return self._invoke_driver(context, routers, 'get_traffic_counters') + def _sync_router_namespaces(self, context, routers): + LOG.debug("Sync router namespaces") + return self._invoke_driver(context, routers, 'sync_router_namespaces') + def add_metering_label_rule(self, context, routers): return self._invoke_driver(context, routers, 'add_metering_label_rule') diff --git a/neutron/services/metering/drivers/abstract_driver.py b/neutron/services/metering/drivers/abstract_driver.py index 5169bdd97ba..ce7e8561d63 100644 --- a/neutron/services/metering/drivers/abstract_driver.py +++ b/neutron/services/metering/drivers/abstract_driver.py @@ -47,3 +47,7 @@ class MeteringAbstractDriver(object): @abc.abstractmethod def get_traffic_counters(self, context, routers): pass + + @abc.abstractmethod + def sync_router_namespaces(self, context, routers): + pass diff --git a/neutron/services/metering/drivers/iptables/iptables_driver.py b/neutron/services/metering/drivers/iptables/iptables_driver.py index 69c3955a421..9c601efff0f 100644 --- a/neutron/services/metering/drivers/iptables/iptables_driver.py +++ b/neutron/services/metering/drivers/iptables/iptables_driver.py @@ -73,7 +73,19 @@ class RouterWithMetering(object): self.ns_name = NS_PREFIX + self.id self.iptables_manager = None self.snat_iptables_manager = None - if self.router['distributed']: + self.metering_labels = {} + + self.create_iptables_managers() + + def create_iptables_managers(self): + """Creates iptables managers if the are not already created + + Returns True if any manager is created + """ + + created = False + + if self.router['distributed'] and self.snat_iptables_manager is None: # If distributed routers then we need to apply the # metering agent label rules in the snat namespace as well. snat_ns_name = dvr_snat_ns.SnatNamespace.get_snat_ns_name( @@ -86,17 +98,25 @@ class RouterWithMetering(object): binary_name=WRAP_NAME, state_less=True, use_ipv6=ipv6_utils.is_enabled_and_bind_by_default()) - # Check of namespace existence before we assign the iptables_manager - # NOTE(Swami): If distributed routers, all external traffic on a - # compute node will flow through the rfp interface in the router - # namespace. - if ip_lib.network_namespace_exists(self.ns_name): - self.iptables_manager = iptables_manager.IptablesManager( - namespace=self.ns_name, - binary_name=WRAP_NAME, - state_less=True, - use_ipv6=ipv6_utils.is_enabled_and_bind_by_default()) - self.metering_labels = {} + + created = True + + if self.iptables_manager is None: + # Check of namespace existence before we assign the + # iptables_manager + # NOTE(Swami): If distributed routers, all external traffic on a + # compute node will flow through the rfp interface in the router + # namespace. + if ip_lib.network_namespace_exists(self.ns_name): + self.iptables_manager = iptables_manager.IptablesManager( + namespace=self.ns_name, + binary_name=WRAP_NAME, + state_less=True, + use_ipv6=ipv6_utils.is_enabled_and_bind_by_default()) + + created = True + + return created class IptablesMeteringDriver(abstract_driver.MeteringAbstractDriver): @@ -109,8 +129,11 @@ class IptablesMeteringDriver(abstract_driver.MeteringAbstractDriver): self.driver = common_utils.load_interface_driver(self.conf) def _update_router(self, router): - r = self.routers.get(router['id'], - RouterWithMetering(self.conf, router)) + r = self.routers.get(router['id']) + + if r is None: + r = RouterWithMetering(self.conf, router) + r.router = router self.routers[r.id] = r @@ -138,10 +161,17 @@ class IptablesMeteringDriver(abstract_driver.MeteringAbstractDriver): else: old_rm_im = old_rm.iptables_manager - with IptablesManagerTransaction(old_rm_im): - self._process_disassociate_metering_label(router) - if gw_port_id: - self._process_associate_metering_label(router) + # In case the selected manager is None pick another one. + # This is not ideal sometimes. + old_rm_im = (old_rm_im or + old_rm.snat_iptables_manager or + old_rm.iptables_manager) + + if old_rm_im: + with IptablesManagerTransaction(old_rm_im): + self._process_disassociate_metering_label(router) + if gw_port_id: + self._process_associate_metering_label(router) elif gw_port_id: self._process_associate_metering_label(router) @@ -430,3 +460,18 @@ class IptablesMeteringDriver(abstract_driver.MeteringAbstractDriver): del self.routers[router_id] return accs + + @log_helpers.log_method_call + def sync_router_namespaces(self, context, routers): + for router in routers: + rm = self.routers.get(router['id']) + if not rm: + continue + + # NOTE(bno1): Sometimes a router is added before its namespaces are + # created. The metering agent has to periodically check if the + # namespaces for the missing iptables managers have appearead and + # create the managers for them. When a new manager is created, the + # metering rules have to be added to it. + if rm.create_iptables_managers(): + self._process_associate_metering_label(router) diff --git a/neutron/services/metering/drivers/noop/noop_driver.py b/neutron/services/metering/drivers/noop/noop_driver.py index 25df069fa72..2bacbe3543e 100644 --- a/neutron/services/metering/drivers/noop/noop_driver.py +++ b/neutron/services/metering/drivers/noop/noop_driver.py @@ -50,3 +50,7 @@ class NoopMeteringDriver(abstract_driver.MeteringAbstractDriver): @log_helpers.log_method_call def get_traffic_counters(self, context, routers): pass + + @log_helpers.log_method_call + def sync_router_namespaces(self, context, routers): + pass diff --git a/neutron/tests/unit/services/metering/agents/test_metering_agent.py b/neutron/tests/unit/services/metering/agents/test_metering_agent.py index 9358034b8c5..f8c3bcafe20 100644 --- a/neutron/tests/unit/services/metering/agents/test_metering_agent.py +++ b/neutron/tests/unit/services/metering/agents/test_metering_agent.py @@ -103,6 +103,10 @@ class TestMeteringOperations(base.BaseTestCase): self.agent._get_traffic_counters(None, ROUTERS) self.assertEqual(1, self.driver.get_traffic_counters.call_count) + def test_sync_router_namespaces(self): + self.agent._sync_router_namespaces(None, ROUTERS) + self.assertEqual(1, self.driver.sync_router_namespaces.call_count) + def test_notification_report(self): self.agent.routers_updated(None, ROUTERS) diff --git a/neutron/tests/unit/services/metering/drivers/test_iptables.py b/neutron/tests/unit/services/metering/drivers/test_iptables.py index aaaf16780d3..dcbdfe53fae 100644 --- a/neutron/tests/unit/services/metering/drivers/test_iptables.py +++ b/neutron/tests/unit/services/metering/drivers/test_iptables.py @@ -683,3 +683,37 @@ class IptablesDriverTestCase(base.BaseTestCase): self.assertIn(expected_label_id, counters) self.assertEqual(1, counters[expected_label_id]['pkts']) self.assertEqual(8, counters[expected_label_id]['bytes']) + + def test_sync_router_namespaces(self): + routers = TEST_DVR_ROUTER[:1] + + self.metering._process_ns_specific_metering_label = mock.Mock() + self.namespace_exists.return_value = False + self.metering.add_metering_label(None, routers) + rm = self.metering.routers[routers[0]['id']] + self.assertEqual( + 0, self.metering._process_ns_specific_metering_label.call_count) + self.assertIsNone(rm.snat_iptables_manager) + self.assertIsNone(rm.iptables_manager) + + self.namespace_exists.side_effect = [True, False] + self.metering.sync_router_namespaces(None, routers) + self.assertIsNotNone(rm.snat_iptables_manager) + self.assertIsNone(rm.iptables_manager) + self.assertEqual( + 1, self.metering._process_ns_specific_metering_label.call_count) + + self.namespace_exists.side_effect = [True] + self.metering.sync_router_namespaces(None, routers) + self.assertIsNotNone(rm.snat_iptables_manager) + self.assertIsNotNone(rm.iptables_manager) + self.assertEqual( + 3, self.metering._process_ns_specific_metering_label.call_count) + + # syncing again should have no effect + self.namespace_exists.side_effect = [RuntimeError('Unexpected call')] + self.metering.sync_router_namespaces(None, routers) + self.assertIsNotNone(rm.snat_iptables_manager) + self.assertIsNotNone(rm.iptables_manager) + self.assertEqual( + 3, self.metering._process_ns_specific_metering_label.call_count)